Skip to content

fix[faustwp]: (#2310, #2311) use hash_equals for secret key comparison, clean up failed blockset extraction#2312

Open
latenighthackathon wants to merge 2 commits intowpengine:canaryfrom
latenighthackathon:fix/faustwp-security-hardening
Open

fix[faustwp]: (#2310, #2311) use hash_equals for secret key comparison, clean up failed blockset extraction#2312
latenighthackathon wants to merge 2 commits intowpengine:canaryfrom
latenighthackathon:fix/faustwp-security-hardening

Conversation

@latenighthackathon
Copy link
Copy Markdown
Contributor

@latenighthackathon latenighthackathon commented Apr 3, 2026

Description

This PR addresses two defense-in-depth issues in the FaustWP WordPress plugin:

1. Timing-vulnerable secret key comparisons (#2310)

The shared secret key (x-faustwp-secret header) is compared using PHP's === operator in three permission callbacks. === short-circuits on the first byte mismatch, making it theoretically vulnerable to timing side-channel attacks (CWE-208).

Changes:

  • plugins/faustwp/includes/rest/callbacks.phprest_authorize_permission_callback() (line 422) and wpac_authorize_permission_callback() (line 447): === $header_keyhash_equals( $secret_key, $header_key )
  • plugins/faustwp/includes/graphql/callbacks.phpfilter_introspection() (line 70): !== $_SERVER[...]! hash_equals() with proper wp_unslash() + sanitize_text_field() per WordPress coding standards

The codebase already uses hash_equals() for HMAC validation in auth/functions.php:200 — these three spots were inconsistent.

2. Failed blockset extraction leaves file on disk (#2311)

process_and_replace_blocks() in plugins/faustwp/includes/blocks/functions.php moves an uploaded file to the target directory, then attempts extraction. If unzip_uploaded_file() fails, the moved file is left at a predictable path under wp-content/uploads/faustwp/blocks/ without cleanup.

Fix: Call $wp_filesystem->delete( $target_file ) before returning the WP_Error.

Related Issues

Closes #2310
Closes #2311

Testing

Confirm the issue (before the fix)

1. Verify === is used for secret key comparison:

grep -n '=== $header_key' plugins/faustwp/includes/rest/callbacks.php
# Expected on canary: matches at lines 422 and 447

2. Verify !== is used in the GraphQL introspection filter:

grep -n "!== \$_SERVER\['HTTP_X_FAUST_SECRET'\]" plugins/faustwp/includes/graphql/callbacks.php
# Expected on canary: match at line 70

3. Verify no cleanup on failed unzip:

grep -A 2 'is_wp_error( $unzip_result )' plugins/faustwp/includes/blocks/functions.php
# Expected on canary: "return $unzip_result;" with no delete() call before it

Confirm the fix

4. Verify all three comparisons now use hash_equals():

grep -n 'hash_equals' plugins/faustwp/includes/rest/callbacks.php plugins/faustwp/includes/graphql/callbacks.php
# Expected: rest/callbacks.php lines 422 and 447, graphql/callbacks.php line 70

5. Verify $_SERVER is properly sanitized before hash_equals():

grep -B 1 'hash_equals' plugins/faustwp/includes/graphql/callbacks.php
# Expected: sanitize_text_field( wp_unslash( ... ) ) on the line before

6. Verify the cleanup delete is in the correct function (process_and_replace_blocks, not unzip_uploaded_file):

grep -B 5 'wp_filesystem->delete( $target_file )' plugins/faustwp/includes/blocks/functions.php
# Expected: inside the is_wp_error( $unzip_result ) block of process_and_replace_blocks()
# NOT inside unzip_uploaded_file() which lacks $wp_filesystem in scope

7. No === remaining for secret key comparison:

grep '=== $header_key' plugins/faustwp/includes/rest/callbacks.php
# Expected: no output (all replaced with hash_equals)

Run the test suites

phpcs — confirms changes follow WordPress coding standards:

cd plugins/faustwp && composer install && composer phpcs

JS tests — confirms no regressions in frontend packages:

npm install && npm run build && npm run test

WordPress PHPUnit — confirms plugin functionality is intact:

cd plugins/faustwp
composer run docker:start
docker-compose exec -e COVERAGE=1 wordpress init-testing-environment.sh
docker exec --workdir=/var/www/html/wp-content/plugins/faustwp $(docker-compose ps -q wordpress) wp plugin install wp-graphql --activate --allow-root
docker exec --workdir=/var/www/html/wp-content/plugins/faustwp $(docker-compose ps -q wordpress) composer test
composer run docker:stop

…et key comparison, clean up failed blockset extraction

Replace timing-vulnerable === comparisons with hash_equals() for
the shared secret key in rest_authorize_permission_callback(),
wpac_authorize_permission_callback(), and filter_introspection().
The codebase already uses hash_equals() for HMAC validation in
auth/functions.php — these three spots were missed.

Also sanitize the $_SERVER['HTTP_X_FAUST_SECRET'] superglobal with
wp_unslash() and sanitize_text_field() per WordPress coding standards
before passing to hash_equals().

Additionally, delete the uploaded blockset file from the target
directory when unzip_uploaded_file() fails, preventing orphaned
files from accumulating on disk after failed uploads.

Closes wpengine#2310
Closes wpengine#2311
@latenighthackathon latenighthackathon requested a review from a team as a code owner April 3, 2026 04:36
@headless-platform-by-wp-engine
Copy link
Copy Markdown

Currently, we do not support the creation of preview deployments based on changes coming from forked repositories.
Learn more about preview environments in our documentation.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 3, 2026

🦋 Changeset detected

Latest commit: 2294d33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@faustwp/wordpress-plugin Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-project-automation github-project-automation bot moved this to 🆕 Backlog in Headless OSS Apr 3, 2026
@ahuseyn ahuseyn moved this from 🆕 Backlog to 👀 In review in Headless OSS Apr 3, 2026
@Fran-A-Dev Fran-A-Dev self-requested a review April 4, 2026 15:11
Copy link
Copy Markdown
Contributor

@Fran-A-Dev Fran-A-Dev left a comment

Choose a reason for hiding this comment

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

The REST calls look ok. The GraphQL path might need one other guard. $_SERVER['HTTP_X_FAUST_SECRET'] is sanitized before comparison, but it is still accessed directly without an isset() check. If that header is missing, this could still trigger an undefined index notice. @moonmeister @latenighthackathon do we default to an empty string before calling hash_equals() so the comparison fails cleanly when the header is absent?

@latenighthackathon
Copy link
Copy Markdown
Contributor Author

Good catch to flag this - the existing isset() guard on line 65 already handles this case:

if ( ! isset( $_SERVER['HTTP_X_FAUST_SECRET'] ) ) {
    return $value;
}

This should return early before we ever reach the sanitize_text_field( wp_unslash( $_SERVER['HTTP_X_FAUST_SECRET'] ) ) call on line 70, so there's no risk of an undefined index notice when the header is absent. That guard was already in the upstream code - our diff just changed the comparison below it from !== to hash_equals() and added the sanitization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

3 participants