Skip to content

Commit 5f25cae

Browse files
josephfuscolatenighthackathon
josephfusco
and
latenighthackathon
committed
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 <latenighthackathon@users.noreply.github.com>
1 parent 5d73ec8 commit 5f25cae

1 file changed

Lines changed: 167 additions & 0 deletions

File tree

‎plugins/faustwp/tests/integration/RestCallbacksTests.php‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
<?php
2+
/**
3+
* Class RestCallbacksTests
4+
*
5+
* @package FaustWP
6+
*/
7+
8+
namespace WPE\FaustWP\Tests\Integration;
9+
10+
use function WPE\FaustWP\Settings\faustwp_update_setting;
11+
use function WPE\FaustWP\REST\rest_authorize_permission_callback;
12+
use function WPE\FaustWP\REST\wpac_authorize_permission_callback;
13+
14+
/**
15+
* Regression tests for the REST permission callbacks in
16+
* plugins/faustwp/includes/rest/callbacks.php.
17+
*
18+
* Specifically guards the timing-safe secret-key comparison fix for #2310,
19+
* plus provides behavioural coverage for the wider contract (no coverage
20+
* existed previously for either callback).
21+
*
22+
* @group rest
23+
* @group auth
24+
*/
25+
class RestCallbacksTests extends \WP_UnitTestCase {
26+
27+
const VALID_SECRET = '00000000-0000-4000-8000-000000000001';
28+
const WRONG_SECRET = 'definitely-not-the-right-key';
29+
30+
public function setUp(): void {
31+
parent::setUp();
32+
faustwp_update_setting( 'secret_key', self::VALID_SECRET );
33+
}
34+
35+
private function make_request_with_faust_header( $value ): \WP_REST_Request {
36+
$request = new \WP_REST_Request( 'POST', '/faustwp/v1/authorize' );
37+
if ( null !== $value ) {
38+
$request->set_header( 'x-faustwp-secret', $value );
39+
}
40+
return $request;
41+
}
42+
43+
private function make_request_with_wpe_header( $value ): \WP_REST_Request {
44+
$request = new \WP_REST_Request( 'POST', '/wpac/v1/authorize' );
45+
if ( null !== $value ) {
46+
$request->set_header( 'x-wpe-headless-secret', $value );
47+
}
48+
return $request;
49+
}
50+
51+
// ----- rest_authorize_permission_callback (the current x-faustwp-secret route) -----
52+
53+
/**
54+
* Happy path: configured secret matches the request header → authorized.
55+
*/
56+
public function test_rest_authorize_returns_true_when_header_matches_secret(): void {
57+
$request = $this->make_request_with_faust_header( self::VALID_SECRET );
58+
59+
$this->assertTrue( rest_authorize_permission_callback( $request ) );
60+
}
61+
62+
/**
63+
* Wrong header value must be rejected. With hash_equals() this is a
64+
* constant-time false; the previous `===` check would have returned
65+
* the same boolean but via a fast short-circuit (the bug fixed by #2310).
66+
*/
67+
public function test_rest_authorize_returns_false_when_header_value_differs(): void {
68+
$request = $this->make_request_with_faust_header( self::WRONG_SECRET );
69+
70+
$this->assertFalse( rest_authorize_permission_callback( $request ) );
71+
}
72+
73+
/**
74+
* Missing x-faustwp-secret header → unauthorized. Guards the early return
75+
* at `if ( $secret_key && $header_key )`.
76+
*/
77+
public function test_rest_authorize_returns_false_when_header_is_missing(): void {
78+
$request = $this->make_request_with_faust_header( null );
79+
80+
$this->assertFalse( rest_authorize_permission_callback( $request ) );
81+
}
82+
83+
/**
84+
* Empty-string header → unauthorized. Guards against a downstream caller
85+
* sending `x-faustwp-secret: ` and accidentally being treated as authorized.
86+
*/
87+
public function test_rest_authorize_returns_false_when_header_is_empty_string(): void {
88+
$request = $this->make_request_with_faust_header( '' );
89+
90+
$this->assertFalse( rest_authorize_permission_callback( $request ) );
91+
}
92+
93+
/**
94+
* Server-side secret unset → unauthorized regardless of what the client sends.
95+
* Guards the early return for unconfigured installs.
96+
*
97+
* Note: we delete the option directly rather than calling
98+
* faustwp_update_setting('secret_key', '') because the
99+
* sanitize_option_faustwp_settings filter (at includes/settings/callbacks.php)
100+
* silently restores the previous value when the new one isn't a valid UUID,
101+
* so a string clear wouldn't actually clear it.
102+
*/
103+
public function test_rest_authorize_returns_false_when_secret_is_unset(): void {
104+
delete_option( 'faustwp_settings' );
105+
$request = $this->make_request_with_faust_header( self::VALID_SECRET );
106+
107+
$this->assertFalse( rest_authorize_permission_callback( $request ) );
108+
}
109+
110+
// ----- wpac_authorize_permission_callback (the deprecated x-wpe-headless-secret route) -----
111+
112+
/**
113+
* The deprecated route still authorizes on a matching x-wpe-headless-secret.
114+
* Kept under test until the route is removed so the deprecation period
115+
* doesn't accidentally break older clients.
116+
*/
117+
public function test_wpac_authorize_returns_true_when_header_matches_secret(): void {
118+
$request = $this->make_request_with_wpe_header( self::VALID_SECRET );
119+
120+
$this->assertTrue( wpac_authorize_permission_callback( $request ) );
121+
}
122+
123+
/**
124+
* Wrong header value rejected by the deprecated route too.
125+
*/
126+
public function test_wpac_authorize_returns_false_when_header_value_differs(): void {
127+
$request = $this->make_request_with_wpe_header( self::WRONG_SECRET );
128+
129+
$this->assertFalse( wpac_authorize_permission_callback( $request ) );
130+
}
131+
132+
// ----- timing-safe-comparison regression guard (source-level) -----
133+
134+
/**
135+
* Source-level guard for #2310: the three secret-key comparisons in this
136+
* codebase must use hash_equals(), not `===`/`!==`. Behavioural tests above
137+
* cannot distinguish hash_equals() from `===` (both return the same boolean
138+
* for valid/invalid inputs); the distinction is timing-safety, which can't
139+
* be reliably asserted in unit tests. So we assert at the source level that
140+
* the known-bad shapes are not present and hash_equals() is.
141+
*
142+
* Narrowly-scoped to the exact patterns that the #2310 fix removed — false
143+
* positives are bounded to a future contributor reintroducing those exact
144+
* literal expressions, which is exactly the regression we want to catch.
145+
*/
146+
public function test_secret_comparisons_use_constant_time_hash_equals(): void {
147+
$rest_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/rest/callbacks.php' );
148+
$graphql_callbacks = file_get_contents( dirname( __DIR__, 2 ) . '/includes/graphql/callbacks.php' );
149+
150+
// The three bad patterns this PR replaces:
151+
$this->assertStringNotContainsString( '=== $header_key', $rest_callbacks,
152+
'rest_authorize_permission_callback must use hash_equals(), not ===.' );
153+
$this->assertStringNotContainsString( "!== \$_SERVER['HTTP_X_FAUST_SECRET']", $graphql_callbacks,
154+
'filter_introspection must use hash_equals(), not !==.' );
155+
156+
// And the positive: both files must contain hash_equals(). Use >= 1 rather
157+
// than a hardcoded count so a future contributor adding a legitimate third
158+
// hash_equals call (e.g. for a new permission callback) doesn't trip a
159+
// false-positive "security regression". The not-contains assertions above
160+
// are the real revert guards; these are affirmative checks that the timing-
161+
// safe primitive is still present somewhere.
162+
$this->assertGreaterThanOrEqual( 1, substr_count( $rest_callbacks, 'hash_equals' ),
163+
'rest/callbacks.php must contain at least one hash_equals call.' );
164+
$this->assertStringContainsString( 'hash_equals', $graphql_callbacks,
165+
'filter_introspection must use hash_equals.' );
166+
}
167+
}

0 commit comments

Comments
 (0)