Skip to content

test[faustwp]: regression test for hash_equals secret-key comparison (follow-up to #2312)#2362

Merged
josephfusco merged 3 commits into
canaryfrom
fix/faustwp-security-hardening-with-tests
May 15, 2026
Merged

test[faustwp]: regression test for hash_equals secret-key comparison (follow-up to #2312)#2362
josephfusco merged 3 commits into
canaryfrom
fix/faustwp-security-hardening-with-tests

Conversation

@josephfusco
Copy link
Copy Markdown
Member

@josephfusco josephfusco commented May 13, 2026

Summary

Follow-up to #2312 (merged as 5d73ec86), which landed the hash_equals() fix for #2310 without regression test coverage. canary now has the timing-safe secret-key comparison in production code, but no automated guard against a future revert.

This PR adds plugins/faustwp/tests/integration/RestCallbacksTests.php — tests-only, no production code changes. Both REST permission callbacks (rest_authorize_permission_callback and the deprecated wpac_authorize_permission_callback) previously had zero direct test coverage; this brings coverage up plus a source-level guard against the specific revert that would reintroduce the timing-attack vulnerability.

Why this is a separate PR

The original #2312 had maintainerCanModify: false, so I couldn't push the test commit onto the contributor's fork branch. I'd opened this PR (#2362) with the same security fix plus tests, but #2312 merged first — so this PR's production-code commits are now empty against canary. Resetting the branch to a single tests-only commit keeps the safety net we built without re-shipping the already-merged code.

Scope

File Status
plugins/faustwp/tests/integration/RestCallbacksTests.php New — 8 tests, 12 assertions
No production code changes
No changeset — (tests aren't customer-facing)

Tests

cd plugins/faustwp
composer install

WP_VERSION=6.8 composer run docker:start
docker compose exec wordpress init-testing-environment.sh
docker compose exec -w /var/www/html/wp-content/plugins/faustwp wordpress \
  wp plugin install wp-graphql --activate --allow-root

docker compose exec -w /var/www/html/wp-content/plugins/faustwp wordpress \
  ./vendor/bin/phpunit --filter=RestCallbacks
# Expected: OK (8 tests, 12 assertions)

# Full suite (single-site + multisite)
docker compose exec -w /var/www/html/wp-content/plugins/faustwp wordpress \
  composer test

Test inventory

Behavioural (rest_authorize_permission_callback):

  • match → true
  • mismatch → false
  • missing header → false
  • empty-string header → false
  • server-side secret unset → false (deletes the faustwp_settings option directly, because sanitize_option_faustwp_settings would otherwise restore a UUID-shaped value)

Behavioural (deprecated wpac_authorize_permission_callback):

  • match → true
  • mismatch → false

Source-level regression guard (test_secret_comparisons_use_constant_time_hash_equals):

  • Asserts === $header_key and !== $_SERVER['HTTP_X_FAUST_SECRET'] are NOT present in the callbacks files.
  • Asserts hash_equals IS present (assertGreaterThanOrEqual(1, ...) to avoid false positives if a future contributor adds a legitimate third hash_equals call).
  • Behavioural tests cannot distinguish hash_equals from === for valid/invalid inputs — both return the same boolean. The security difference is timing, which is impractical to assert in unit tests. The source-level check is the only way to catch a future revert of the fix[faustwp]: secret key comparisons use timing-vulnerable === instead of hash_equals() #2310 fix.

Verified red-on-revert against today's canary-based branch (HEAD 5f25cae7, 2026-05-14): with hash_equals reverted to === / !== in rest/callbacks.php and graphql/callbacks.php, the source guard fails cleanly (Failed asserting that ... does not contain "=== $header_key"). Restoring hash_equals greens the suite. Full plugin PHPUnit run on WP 6.8: 134 tests, 303 assertions on both single-site and multisite.

Related

Together these three test files give handle_generate_endpoint, process_and_replace_blocks, and the REST permission callbacks their first dedicated regression coverage. (filter_introspection is already covered by four pre-existing tests in GraphQLCallbacksTests.php.)

Co-authored-by: latenighthackathon [email protected]

@josephfusco josephfusco requested a review from a team as a code owner May 13, 2026 16:20
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 13, 2026

⚠️ No Changeset found

Latest commit: df186fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

📦 Next.js Bundle Analysis for @faustwp/getting-started-example

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

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 <[email protected]>
@josephfusco josephfusco force-pushed the fix/faustwp-security-hardening-with-tests branch from 5991f12 to 5f25cae Compare May 14, 2026 14:16
@josephfusco josephfusco changed the title fix[faustwp]: (#2310) use hash_equals for secret key comparison, with tests test[faustwp]: regression test for hash_equals secret-key comparison (follow-up to #2312) May 14, 2026
@josephfusco josephfusco requested a review from Copilot May 14, 2026 14:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds integration/regression coverage for FaustWP’s REST permission callbacks, specifically guarding against regressions of the timing-safe hash_equals() secret comparison introduced in #2312/#2310.

Changes:

  • Add a new integration test suite covering rest_authorize_permission_callback() behavior across match/mismatch/missing/empty/unset-secret scenarios.
  • Add behavioral coverage for the deprecated wpac_authorize_permission_callback().
  • Add a source-level regression guard intended to fail if comparisons revert away from hash_equals().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/faustwp/tests/integration/RestCallbacksTests.php
Comment thread plugins/faustwp/tests/integration/RestCallbacksTests.php Outdated
Comment thread plugins/faustwp/tests/integration/RestCallbacksTests.php
josephfusco and others added 2 commits May 14, 2026 10:42
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
…uard

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.
@josephfusco josephfusco enabled auto-merge (squash) May 14, 2026 15:03
@josephfusco josephfusco merged commit 973c9f9 into canary May 15, 2026
16 checks passed
@josephfusco josephfusco deleted the fix/faustwp-security-hardening-with-tests branch May 15, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix[faustwp]: secret key comparisons use timing-vulnerable === instead of hash_equals()

3 participants