From 1d4bebcb9e68e8388327021e4ca3307efe951770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Sch=C3=B6ldstr=C3=B6m?= Date: Fri, 31 Jul 2026 07:41:39 -0300 Subject: [PATCH] feat: record why a verification failed, not just that it did The decision log flattened every rejection into `missing` or `invalid`, which is the least useful place to lose detail. An expired challenge and a forged one are the same line in the log, but they mean opposite things: one says our validity window is too short for the form, the other says we're being attacked. That gap cost us. A tester reported the Snellman consumer-service form rejecting a submission with no field highlighted, and there was no way to tell from the log whether the challenge had expired or the payload had been replayed -- the two candidate causes have identical symptoms. Challenge::classify() now surfaces the reason the altcha-org library already computes (expired / invalid_signature / invalid_solution) instead of discarding it, plus missing and malformed. verify() keeps its bool contract and defers to classify(), so there's a single parsing path. For expired challenges the log also records `expired_for` -- seconds past expiry. Whether a visitor was ten seconds over or twenty minutes over is the difference between tuning the window and looking elsewhere, and it shows whether the widget's renewal is holding up in the field. Context stays non-PII: a form id, a reason, a duration. Co-Authored-By: Claude Opus 5 (1M context) --- src/Challenge.php | 94 ++++++++++++++++++++++++++++++++----- src/Integration.php | 47 +++++++++++++++++-- test/unit/ChallengeTest.php | 79 +++++++++++++++++++++++++++++++ 3 files changed, 204 insertions(+), 16 deletions(-) diff --git a/src/Challenge.php b/src/Challenge.php index c36fc08..a70ade4 100644 --- a/src/Challenge.php +++ b/src/Challenge.php @@ -77,29 +77,86 @@ public static function clampCost(int $cost): int } /** - * Verifies a base64-encoded payload as posted by the ALTCHA widget. Both - * client-solution payloads (the common case) and server-signature payloads - * (used by ALTCHA Sentinel / Spam Filter) are accepted — `altcha-org/altcha` - * tells the two apart by the presence of `verificationData`. + * A payload that passed every check. + */ + public const REASON_OK = 'ok'; + + /** + * No `altcha` field was posted at all — typically a bot posting straight at + * the form handler, or the widget script never running. + */ + public const REASON_MISSING = 'missing'; + + /** + * Present, but not a decodable ALTCHA payload. + */ + public const REASON_MALFORMED = 'malformed'; + + /** + * Solved correctly, but the challenge's validity window had already closed + * by the time the form was submitted — the visitor spent longer on the form + * than {@see self::DEFAULT_EXPIRES_SECONDS}. + */ + public const REASON_EXPIRED = 'expired'; + + /** + * Not a challenge we issued: forged, or signed with a different HMAC key + * (e.g. after a key rotation). + */ + public const REASON_INVALID_SIGNATURE = 'invalid_signature'; + + /** + * Our challenge, but the proof-of-work answer is wrong. + */ + public const REASON_INVALID_SOLUTION = 'invalid_solution'; + + /** + * Failed verification without the library attributing a specific cause. + */ + public const REASON_INVALID = 'invalid'; + + /** + * Verifies a base64-encoded payload as posted by the ALTCHA widget. */ public function verify(string $base64Payload): bool + { + return $this->classify($base64Payload) === self::REASON_OK; + } + + /** + * Verifies a payload and reports *why* it failed instead of collapsing the + * outcome to a bool. Telling an expired challenge apart from a forged one is + * the difference between "our validity window is too short for this form" + * and "we're being attacked" — so the reason is what makes the decision log + * worth keeping. + * + * Both client-solution payloads (the common case) and server-signature + * payloads (used by ALTCHA Sentinel / Spam Filter) are accepted — + * `altcha-org/altcha` tells the two apart by the presence of + * `verificationData`. + * + * @return string one of the REASON_* constants + */ + public function classify(string $base64Payload): string { if ($base64Payload === '') { - return false; + return self::REASON_MISSING; } $decoded = base64_decode($base64Payload, true); if ($decoded === false) { - return false; + return self::REASON_MALFORMED; } $payload = json_decode($decoded, true); if (! is_array($payload)) { - return false; + return self::REASON_MALFORMED; } if (isset($payload['verificationData'])) { - return ServerSignature::verifyServerSignature($payload, $this->hmacSecret)->verified; + return ServerSignature::verifyServerSignature($payload, $this->hmacSecret)->verified + ? self::REASON_OK + : self::REASON_INVALID; } if (! isset($payload['challenge'], $payload['solution']) @@ -107,7 +164,7 @@ public function verify(string $base64Payload): bool || ! is_array($payload['solution']) || ! is_array($payload['challenge']['parameters'] ?? null) ) { - return false; + return self::REASON_MALFORMED; } $challenge = new AltchaChallenge( @@ -121,10 +178,25 @@ public function verify(string $base64Payload): bool $altcha = new Altcha($this->hmacSecret); - return $altcha->verifySolution(new VerifySolutionOptions( + $result = $altcha->verifySolution(new VerifySolutionOptions( algorithm: new Pbkdf2, payload: new Payload($challenge, $solution), - ))->verified; + )); + + if ($result->verified) { + return self::REASON_OK; + } + if ($result->expired) { + return self::REASON_EXPIRED; + } + if ($result->invalidSignature) { + return self::REASON_INVALID_SIGNATURE; + } + if ($result->invalidSolution) { + return self::REASON_INVALID_SOLUTION; + } + + return self::REASON_INVALID; } /** diff --git a/src/Integration.php b/src/Integration.php index 5b7b765..bc731a5 100644 --- a/src/Integration.php +++ b/src/Integration.php @@ -8,6 +8,13 @@ class Integration public const SCRIPT_HANDLE = 'gravityforms-altcha-widget'; + /** + * A valid, correctly solved payload that had already been accepted once. + * Lives here rather than on {@see Challenge} because replay is enforced by + * this class, not by the verification itself. + */ + public const REASON_REPLAY = 'replay'; + public static function register(): void { $instance = new self; @@ -65,18 +72,19 @@ public function validate(array $result): array ? sanitize_text_field(wp_unslash($_POST[self::POST_FIELD])) : ''; - if ($this->challenge()->verify($payload)) { + $challenge = $this->challenge(); + $reason = $challenge->classify($payload); + + if ($reason === Challenge::REASON_OK) { if (! $this->isReplay($payload)) { Logger::record('altcha', 'pass', ['form' => $formId]); return $result; } - $reason = 'replay'; - } else { - $reason = $payload === '' ? 'missing' : 'invalid'; + $reason = self::REASON_REPLAY; } - Logger::record('altcha', 'fail', ['form' => $formId, 'reason' => $reason]); + Logger::record('altcha', 'fail', $this->failureContext($formId, $reason, $payload, $challenge)); $result['is_valid'] = false; $result['form']['validation_summary_message'] = $this->errorMessage(); @@ -85,6 +93,35 @@ public function validate(array $result): array return $result; } + /** + * Builds the non-PII context recorded alongside a rejection. + * + * `reason` alone answers most questions, but not the one that matters most + * for an expired challenge: was the visitor a few seconds over, or half an + * hour? `expired_for` (seconds past `expiresAt`) turns "expiry is hurting + * us" from a hunch into a number, and shows whether the widget's renewal is + * doing its job in the field. + * + * @return array + */ + private function failureContext(mixed $formId, string $reason, string $payload, Challenge $challenge): array + { + $context = ['form' => $formId, 'reason' => $reason]; + + if ($reason !== Challenge::REASON_EXPIRED) { + return $context; + } + + $fingerprint = $challenge->fingerprint($payload); + if ($fingerprint === null || $fingerprint['expiresAt'] === null) { + return $context; + } + + $context['expired_for'] = max(0, time() - $fingerprint['expiresAt']); + + return $context; + } + /** * One-time-use enforcement. A signed challenge stays verifiable until it * expires, so without this a bot could solve the proof once and replay the diff --git a/test/unit/ChallengeTest.php b/test/unit/ChallengeTest.php index 2ddc5b6..fa6eae7 100644 --- a/test/unit/ChallengeTest.php +++ b/test/unit/ChallengeTest.php @@ -148,6 +148,85 @@ public function test_fingerprint_returns_null_for_unparseable_payloads(): void $this->assertNull($challenge->fingerprint(base64_encode(json_encode(['no' => 'signature'])))); } + public function test_classify_accepts_a_valid_solution(): void + { + [, $payload] = $this->solvedPayload(); + + $this->assertSame(Challenge::REASON_OK, (new Challenge(self::SECRET))->classify($payload)); + } + + public function test_classify_reports_missing_for_an_empty_payload(): void + { + $this->assertSame(Challenge::REASON_MISSING, (new Challenge(self::SECRET))->classify('')); + } + + public function test_classify_reports_malformed_for_undecodable_payloads(): void + { + $challenge = new Challenge(self::SECRET); + + $this->assertSame(Challenge::REASON_MALFORMED, $challenge->classify('!!!not-base64!!!')); + $this->assertSame(Challenge::REASON_MALFORMED, $challenge->classify(base64_encode('not json'))); + $this->assertSame( + Challenge::REASON_MALFORMED, + $challenge->classify(base64_encode(json_encode(['challenge' => 'wrong-shape']))), + ); + } + + /** + * The distinction this whole classification exists for: a visitor who sat on + * a long form past the validity window must be distinguishable from an + * attacker posting a forged proof. + */ + public function test_classify_reports_expired_separately_from_invalid(): void + { + [, $payload] = $this->solvedPayload(expiresSeconds: -10); + + $this->assertSame(Challenge::REASON_EXPIRED, (new Challenge(self::SECRET))->classify($payload)); + } + + public function test_classify_reports_invalid_signature_for_a_foreign_challenge(): void + { + $altcha = new Altcha('attacker-secret'); + $foreign = (new Challenge('attacker-secret', cost: 1000))->create(); + + $solution = $altcha->solveChallenge(new SolveChallengeOptions( + algorithm: new Pbkdf2, + challenge: $foreign, + )); + $this->assertNotNull($solution); + + $payload = (new Payload($foreign, $solution))->toBase64(); + + $this->assertSame( + Challenge::REASON_INVALID_SIGNATURE, + (new Challenge(self::SECRET))->classify($payload), + ); + } + + public function test_classify_reports_invalid_solution_for_a_garbled_answer(): void + { + [$challenge, $payload] = $this->solvedPayload(); + + $arr = json_decode(base64_decode($payload, true), true); + $arr['solution']['derivedKey'] = str_repeat('0', strlen($arr['solution']['derivedKey'])); + + $this->assertSame( + Challenge::REASON_INVALID_SOLUTION, + (new Challenge(self::SECRET))->classify(base64_encode(json_encode($arr))), + ); + } + + public function test_verify_agrees_with_classify(): void + { + [, $valid] = $this->solvedPayload(); + [, $expired] = $this->solvedPayload(expiresSeconds: -10); + $challenge = new Challenge(self::SECRET); + + $this->assertTrue($challenge->verify($valid)); + $this->assertFalse($challenge->verify($expired)); + $this->assertFalse($challenge->verify('')); + } + public function test_clamp_cost_constrains_to_bounds(): void { $this->assertSame(Challenge::MIN_COST, Challenge::clampCost(1));