From 5f25cae7d3e8d24d2176fa642c00e71b2b5571fe Mon Sep 17 00:00:00 2001 From: Joe Fusco Date: Thu, 14 May 2026 10:16:52 -0400 Subject: [PATCH 1/3] test[faustwp]: regression test for hash_equals secret-key comparison Adds tests/integration/RestCallbacksTests.php covering the hash_equals() fix landed in #2312 (closes #2310), which previously had no direct regression coverage. canary now has the timing-safe comparison shipped but the test scaffolding was on a separate fork-branch that couldn't be pushed to; this PR brings the safety net in. Tests: - rest_authorize_permission_callback (x-faustwp-secret): match, mismatch, missing header, empty header, server-side secret unset - wpac_authorize_permission_callback (deprecated x-wpe-headless-secret): match, mismatch - Source-level guard test_secret_comparisons_use_constant_time_hash_equals asserts the known-bad '=== $header_key' and '!== $_SERVER[...]' patterns are not present and hash_equals() is. Behavioural tests cannot distinguish hash_equals() from === for valid/invalid inputs (boolean result is the same; the security difference is timing), so this source assertion is the only way to guard against a future revert of the #2310 fix. The 'secret unset' case deletes the faustwp_settings option directly rather than setting secret_key='' because the sanitize_option_faustwp_settings filter would silently restore the previous valid UUID. Verified red-on-revert against canary (manually reverted hash_equals back to ===/!==): the source guard fails cleanly. Restoring hash_equals greens the suite. 8 tests, 11 assertions. Full plugin suite green on single-site and multisite (composer test). Co-authored-by: latenighthackathon --- .../tests/integration/RestCallbacksTests.php | 167 ++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 plugins/faustwp/tests/integration/RestCallbacksTests.php diff --git a/plugins/faustwp/tests/integration/RestCallbacksTests.php b/plugins/faustwp/tests/integration/RestCallbacksTests.php new file mode 100644 index 000000000..477e1a0c5 --- /dev/null +++ b/plugins/faustwp/tests/integration/RestCallbacksTests.php @@ -0,0 +1,167 @@ +set_header( 'x-faustwp-secret', $value ); + } + return $request; + } + + private function make_request_with_wpe_header( $value ): \WP_REST_Request { + $request = new \WP_REST_Request( 'POST', '/wpac/v1/authorize' ); + if ( null !== $value ) { + $request->set_header( 'x-wpe-headless-secret', $value ); + } + return $request; + } + + // ----- rest_authorize_permission_callback (the current x-faustwp-secret route) ----- + + /** + * Happy path: configured secret matches the request header → authorized. + */ + public function test_rest_authorize_returns_true_when_header_matches_secret(): void { + $request = $this->make_request_with_faust_header( self::VALID_SECRET ); + + $this->assertTrue( rest_authorize_permission_callback( $request ) ); + } + + /** + * Wrong header value must be rejected. With hash_equals() this is a + * constant-time false; the previous `===` check would have returned + * the same boolean but via a fast short-circuit (the bug fixed by #2310). + */ + public function test_rest_authorize_returns_false_when_header_value_differs(): void { + $request = $this->make_request_with_faust_header( self::WRONG_SECRET ); + + $this->assertFalse( rest_authorize_permission_callback( $request ) ); + } + + /** + * Missing x-faustwp-secret header → unauthorized. Guards the early return + * at `if ( $secret_key && $header_key )`. + */ + public function test_rest_authorize_returns_false_when_header_is_missing(): void { + $request = $this->make_request_with_faust_header( null ); + + $this->assertFalse( rest_authorize_permission_callback( $request ) ); + } + + /** + * Empty-string header → unauthorized. Guards against a downstream caller + * sending `x-faustwp-secret: ` and accidentally being treated as authorized. + */ + public function test_rest_authorize_returns_false_when_header_is_empty_string(): void { + $request = $this->make_request_with_faust_header( '' ); + + $this->assertFalse( rest_authorize_permission_callback( $request ) ); + } + + /** + * Server-side secret unset → unauthorized regardless of what the client sends. + * Guards the early return for unconfigured installs. + * + * Note: we delete the option directly rather than calling + * faustwp_update_setting('secret_key', '') because the + * sanitize_option_faustwp_settings filter (at includes/settings/callbacks.php) + * silently restores the previous value when the new one isn't a valid UUID, + * so a string clear wouldn't actually clear it. + */ + public function test_rest_authorize_returns_false_when_secret_is_unset(): void { + delete_option( 'faustwp_settings' ); + $request = $this->make_request_with_faust_header( self::VALID_SECRET ); + + $this->assertFalse( rest_authorize_permission_callback( $request ) ); + } + + // ----- wpac_authorize_permission_callback (the deprecated x-wpe-headless-secret route) ----- + + /** + * The deprecated route still authorizes on a matching x-wpe-headless-secret. + * Kept under test until the route is removed so the deprecation period + * doesn't accidentally break older clients. + */ + public function test_wpac_authorize_returns_true_when_header_matches_secret(): void { + $request = $this->make_request_with_wpe_header( self::VALID_SECRET ); + + $this->assertTrue( wpac_authorize_permission_callback( $request ) ); + } + + /** + * Wrong header value rejected by the deprecated route too. + */ + public function test_wpac_authorize_returns_false_when_header_value_differs(): void { + $request = $this->make_request_with_wpe_header( self::WRONG_SECRET ); + + $this->assertFalse( wpac_authorize_permission_callback( $request ) ); + } + + // ----- timing-safe-comparison regression guard (source-level) ----- + + /** + * Source-level guard for #2310: the three secret-key comparisons in this + * codebase must use hash_equals(), not `===`/`!==`. Behavioural tests above + * cannot distinguish hash_equals() from `===` (both return the same boolean + * for valid/invalid inputs); the distinction is timing-safety, which can't + * be reliably asserted in unit tests. So we assert at the source level that + * the known-bad shapes are not present and hash_equals() is. + * + * Narrowly-scoped to the exact patterns that the #2310 fix removed — false + * positives are bounded to a future contributor reintroducing those exact + * literal expressions, which is exactly the regression we want to catch. + */ + public function test_secret_comparisons_use_constant_time_hash_equals(): void { + $rest_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/rest/callbacks.php' ); + $graphql_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/graphql/callbacks.php' ); + + // The three bad patterns this PR replaces: + $this->assertStringNotContainsString( '=== $header_key', $rest_callbacks, + 'rest_authorize_permission_callback must use hash_equals(), not ===.' ); + $this->assertStringNotContainsString( "!== \$_SERVER['HTTP_X_FAUST_SECRET']", $graphql_callbacks, + 'filter_introspection must use hash_equals(), not !==.' ); + + // And the positive: both files must contain hash_equals(). Use >= 1 rather + // than a hardcoded count so a future contributor adding a legitimate third + // hash_equals call (e.g. for a new permission callback) doesn't trip a + // false-positive "security regression". The not-contains assertions above + // are the real revert guards; these are affirmative checks that the timing- + // safe primitive is still present somewhere. + $this->assertGreaterThanOrEqual( 1, substr_count( $rest_callbacks, 'hash_equals' ), + 'rest/callbacks.php must contain at least one hash_equals call.' ); + $this->assertStringContainsString( 'hash_equals', $graphql_callbacks, + 'filter_introspection must use hash_equals.' ); + } +} From 435fe57341be790903f98bc690c5f37e70ac7a75 Mon Sep 17 00:00:00 2001 From: Joe Fusco Date: Thu, 14 May 2026 10:42:19 -0400 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- plugins/faustwp/tests/integration/RestCallbacksTests.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/faustwp/tests/integration/RestCallbacksTests.php b/plugins/faustwp/tests/integration/RestCallbacksTests.php index 477e1a0c5..d5540ed43 100644 --- a/plugins/faustwp/tests/integration/RestCallbacksTests.php +++ b/plugins/faustwp/tests/integration/RestCallbacksTests.php @@ -147,6 +147,9 @@ public function test_secret_comparisons_use_constant_time_hash_equals(): void { $rest_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/rest/callbacks.php' ); $graphql_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/graphql/callbacks.php' ); + $this->assertNotFalse( $rest_callbacks, 'Failed to read includes/rest/callbacks.php for regression guard.' ); + $this->assertNotFalse( $graphql_callbacks, 'Failed to read includes/graphql/callbacks.php for regression guard.' ); + // The three bad patterns this PR replaces: $this->assertStringNotContainsString( '=== $header_key', $rest_callbacks, 'rest_authorize_permission_callback must use hash_equals(), not ===.' ); From df186fc059d1e9907a158c6065326673e7bb043b Mon Sep 17 00:00:00 2001 From: Joe Fusco Date: Thu, 14 May 2026 10:44:27 -0400 Subject: [PATCH 3/3] test[faustwp]: clarify 'bad patterns' comment in hash_equals source guard Per Copilot review: the comment said 'three bad patterns' but only two assertStringNotContainsString calls follow. The original #2312 fix replaced ===/!== at three call sites (rest_authorize_permission_callback, wpac_authorize_permission_callback, filter_introspection), but the two REST sites share the literal '=== $header_key' shape, so a single substring check covers both. Reword to match what's actually asserted. --- plugins/faustwp/tests/integration/RestCallbacksTests.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugins/faustwp/tests/integration/RestCallbacksTests.php b/plugins/faustwp/tests/integration/RestCallbacksTests.php index d5540ed43..112faad7b 100644 --- a/plugins/faustwp/tests/integration/RestCallbacksTests.php +++ b/plugins/faustwp/tests/integration/RestCallbacksTests.php @@ -150,7 +150,10 @@ public function test_secret_comparisons_use_constant_time_hash_equals(): void { $this->assertNotFalse( $rest_callbacks, 'Failed to read includes/rest/callbacks.php for regression guard.' ); $this->assertNotFalse( $graphql_callbacks, 'Failed to read includes/graphql/callbacks.php for regression guard.' ); - // The three bad patterns this PR replaces: + // The bad comparison shapes this PR replaces (two distinct patterns, three + // call sites: '=== $header_key' covers both rest_authorize_permission_callback + // and wpac_authorize_permission_callback; '!== $_SERVER[...]' covers + // filter_introspection): $this->assertStringNotContainsString( '=== $header_key', $rest_callbacks, 'rest_authorize_permission_callback must use hash_equals(), not ===.' ); $this->assertStringNotContainsString( "!== \$_SERVER['HTTP_X_FAUST_SECRET']", $graphql_callbacks,