[GFZSPAM-14] Replace static anti-spam key with signed HMAC tokens#52
Open
[GFZSPAM-14] Replace static anti-spam key with signed HMAC tokens#52
Conversation
Replace the static key mechanism with stateless HMAC-SHA256 signed tokens that expire and cannot be replayed. Tokens are fetched at submit time via a three-tier fallback chain (REST API > admin-ajax > server-rendered token), making the system compatible with page caching.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds a signed token anti-spam system: new GF_Zero_Spam_Token (mint/validate HMAC-SHA256), a GF_Zero_Spam_Token_Endpoint exposing REST and admin-ajax token minting with per-IP rate limiting, and integrates token validation and migration fallback into GF_Zero_Spam. Updates docs and phpstan ignore rules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as Token Endpoint
participant Token as Token Service
participant Response
Client->>Endpoint: GET /gf-zero-spam/v1/token?form_id=ID
Endpoint->>Endpoint: validate form_id, check feature enabled, check rate limit
alt Rate limited
Endpoint->>Response: 429 {error}
else Allowed
Endpoint->>Token: mint(form_id, ttl=600)
Token->>Token: create payload, sign with HMAC-SHA256
Token->>Endpoint: {token, expires}
Endpoint->>Response: 200 {token, expires} + no-cache headers
end
Response->>Client: deliver token
sequenceDiagram
participant User
participant Form as Form Handler
participant Validator as Token Validator
participant Legacy as Legacy Key Handler
User->>Form: Submit with gf_zero_spam_token (or legacy key)
Form->>Validator: validate(token, form_id)
alt Token valid
Validator->>Form: allow submission
else Token invalid/absent
Form->>Legacy: validate_legacy_key(submitted_key)
alt Within migration window and valid
Legacy->>Form: allow submission
else
Legacy->>Form: reject submission
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
includes/class-gf-zero-spam-token-endpoint.php (1)
193-199: Clamp rate limit filter output to a positive minimum.If
gf_zero_spam_rate_limitreturns0or negative values, every request gets blocked.Suggested fix
- $limit = (int) apply_filters( 'gf_zero_spam_rate_limit', self::RATE_LIMIT ); + $limit = max( 1, (int) apply_filters( 'gf_zero_spam_rate_limit', self::RATE_LIMIT ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/class-gf-zero-spam-token-endpoint.php` around lines 193 - 199, The rate limit obtained from the gf_zero_spam_rate_limit filter can be zero or negative which blocks all requests; change the assignment of $limit so it's clamped to a sensible positive minimum (e.g., 1) by taking the max of 1 and the filtered integer value (use the existing self::RATE_LIMIT fallback), i.e., update the $limit calculation where gf_zero_spam_rate_limit is applied to ensure $limit = max(1, (int) apply_filters( 'gf_zero_spam_rate_limit', self::RATE_LIMIT )); so subsequent checks against $count behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/class-gf-zero-spam-token-endpoint.php`:
- Around line 99-101: The WP_Error status is being passed as a raw integer to
wp_send_json_error which REST ignores; update the error handling in
class-gf-zero-spam-token-endpoint (where $result is a WP_Error and methods like
get_error_data() / get_error_message() are used) to extract status from
get_error_data() when it's an array with a 'status' key (e.g., $data =
$result->get_error_data(); $status = is_array($data) && isset($data['status']) ?
(int)$data['status'] : (int)$data;) and pass that status into
wp_send_json_error, and also update the AJAX handler branch(s) that read
get_error_data() so they likewise handle array['status'] format instead of
assuming a raw int; apply the same change to the other occurrences referenced
(around the other lines).
In `@includes/class-gf-zero-spam.php`:
- Around line 91-93: Sanitize the incoming token before persisting it: replace
the raw rgpost('gf_zero_spam_token') assignment with a cleaned value using
WordPress sanitization (e.g., $token = sanitize_text_field(
rgpost('gf_zero_spam_token') );) and then store
$submission['partial_entry']['gf_zero_spam_token'] = $token; — update the code
in includes/class-gf-zero-spam.php where the partial_entry is populated so only
the sanitized token is written.
- Around line 196-197: The fallback token TTL is too long: replace the
DAY_IN_SECONDS used in GF_Zero_Spam_Token::mint (where $fallback_token is
created) with a much shorter lifetime (e.g., MINUTE_IN_SECONDS or a
config-driven short constant like FALLBACK_TOKEN_TTL) so the fallback window for
replay protection is minimal; update the call site that mints $fallback_token
and any related tests/config to use the new short TTL and ensure it remains
shorter than the primary token lifetime.
- Around line 461-468: Migration deadline is being set lazily on first legacy
submission in class-gf-zero-spam.php which allows legacy keys to remain valid
indefinitely; instead, add a method (e.g., maybe_initialize_legacy_deadline)
that runs during plugin init/upgrade to set gf_zero_spam_legacy_deadline to
time() + 14 * DAY_IN_SECONDS if the option is missing and gf_zero_spam_key
exists, remove the current lazy initialization block from the legacy handling
code, and call the new method from the plugin's initialization/upgrade routine
(reference the class name and the new maybe_initialize_legacy_deadline method
and the gf_zero_spam_legacy_deadline and gf_zero_spam_key options to locate
where to make the changes).
In `@readme.txt`:
- Line 115: The README's absolute security claim ("tokens can't be copied from
the page source") is incorrect because includes/class-gf-zero-spam.php exposes a
client-side fallbackToken (reference fallbackToken in that file); update the
README wording to soften the claim and accurately describe the protection: state
that tokens are signed/validated server-side and tied to request/context (or
expiry) rather than being impossible to read from page source, and mention that
client-side exposure does not by itself allow successful reuse without the
server-side checks.
---
Nitpick comments:
In `@includes/class-gf-zero-spam-token-endpoint.php`:
- Around line 193-199: The rate limit obtained from the gf_zero_spam_rate_limit
filter can be zero or negative which blocks all requests; change the assignment
of $limit so it's clamped to a sensible positive minimum (e.g., 1) by taking the
max of 1 and the filtered integer value (use the existing self::RATE_LIMIT
fallback), i.e., update the $limit calculation where gf_zero_spam_rate_limit is
applied to ensure $limit = max(1, (int) apply_filters(
'gf_zero_spam_rate_limit', self::RATE_LIMIT )); so subsequent checks against
$count behave correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
includes/class-gf-zero-spam-token-endpoint.phpincludes/class-gf-zero-spam-token.phpincludes/class-gf-zero-spam.phpphpstan.dist.neonreadme.txt
Comment on lines
+99
to
+101
| $status = (int) $result->get_error_data(); | ||
|
|
||
| wp_send_json_error( $result->get_error_message(), $status ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress REST API WP_Error status code format ['status' => code] vs integer data parameter
💡 Result:
In WordPress REST API callbacks, the HTTP status code for a WP_Error is not taken from an integer passed as the error “data”. The REST server determines the response status by looking for an array with a status key in the error’s data:
- Use:
new WP_Error( 'my_code', 'Message', [ 'status' => 404 ] )(or set error data similarly). - If you pass an integer as the data (e.g.
new WP_Error(..., 404)), it’s treated as payload data, and the REST API will not use it as the HTTP status—so the response status falls back to a default (commonly 500) unless a['status' => ...]array is present. This is exactly howrest_convert_error_to_response()extracts the status. [1] - The REST API docs explicitly describe customizing
WP_ErrorHTTP status via thestatusoption in the error data array. [2]
If you don’t want WP_Error, you can return a WP_REST_Response( $data, $status_code ) to set status directly. [3]
Sources:
[1] rest_convert_error_to_response() status extraction logic (checks is_array($error_data) and isset($error_data['status'])) https://developer.wordpress.org/reference/functions/rest_convert_error_to_response/
[2] REST API “Adding Custom Endpoints” docs showing WP_Error(..., array('status' => 400)) https://ja.wp-api.org/extending/adding/
[3] WP_REST_Response supports setting HTTP status code https://developer.wordpress.org/plugins/rest-api/responses-2/
🏁 Script executed:
# First, locate and examine the target file
fd "class-gf-zero-spam-token-endpoint.php" --type fRepository: GravityKit/gravity-forms-zero-spam
Length of output: 124
🏁 Script executed:
# Examine the target file, focusing on the specified line ranges
head -210 "includes/class-gf-zero-spam-token-endpoint.php" | tail -125Repository: GravityKit/gravity-forms-zero-spam
Length of output: 3499
Use REST-compatible WP_Error status data.
When this endpoint returns WP_Error to the REST API, raw integers as the third parameter are not recognized as HTTP status codes—the REST server only extracts status from ['status' => code] array format, otherwise defaulting to HTTP 500. The AJAX handler also needs updating to extract status from the array format.
Suggested fix
- $status = (int) $result->get_error_data();
+ $error_data = $result->get_error_data();
+ $status = (int) ( is_array( $error_data ) && isset( $error_data['status'] ) ? $error_data['status'] : 500 );
wp_send_json_error( $result->get_error_message(), $status );- return new WP_Error( 'missing_form_id', __( 'A valid form_id is required.', 'gravity-forms-zero-spam' ), 400 );
+ return new WP_Error( 'missing_form_id', __( 'A valid form_id is required.', 'gravity-forms-zero-spam' ), [ 'status' => 400 ] );- return new WP_Error( 'invalid_form', __( 'Form not found.', 'gravity-forms-zero-spam' ), 400 );
+ return new WP_Error( 'invalid_form', __( 'Form not found.', 'gravity-forms-zero-spam' ), [ 'status' => 400 ] );- return new WP_Error( 'zero_spam_disabled', __( 'Zero Spam is not enabled for this form.', 'gravity-forms-zero-spam' ), 400 );
+ return new WP_Error( 'zero_spam_disabled', __( 'Zero Spam is not enabled for this form.', 'gravity-forms-zero-spam' ), [ 'status' => 400 ] );- return new WP_Error( 'rate_limited', __( 'Too many requests. Please try again later.', 'gravity-forms-zero-spam' ), 429 );
+ return new WP_Error( 'rate_limited', __( 'Too many requests. Please try again later.', 'gravity-forms-zero-spam' ), [ 'status' => 429 ] );Also applies to: 118-119, 124-125, 131-132, 196-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/class-gf-zero-spam-token-endpoint.php` around lines 99 - 101, The
WP_Error status is being passed as a raw integer to wp_send_json_error which
REST ignores; update the error handling in class-gf-zero-spam-token-endpoint
(where $result is a WP_Error and methods like get_error_data() /
get_error_message() are used) to extract status from get_error_data() when it's
an array with a 'status' key (e.g., $data = $result->get_error_data(); $status =
is_array($data) && isset($data['status']) ? (int)$data['status'] : (int)$data;)
and pass that status into wp_send_json_error, and also update the AJAX handler
branch(s) that read get_error_data() so they likewise handle array['status']
format instead of assuming a raw int; apply the same change to the other
occurrences referenced (around the other lines).
Comment on lines
+91
to
93
| // Store the token in the draft JSON for audit purposes only; not re-validated on resume. | ||
| $submission['partial_entry']['gf_zero_spam_token'] = rgpost( 'gf_zero_spam_token' ); | ||
|
|
There was a problem hiding this comment.
Sanitize token before persisting draft JSON.
Line 92 stores raw request data; sanitize before writing it into partial_entry.
Suggested fix
- $submission['partial_entry']['gf_zero_spam_token'] = rgpost( 'gf_zero_spam_token' );
+ $submission['partial_entry']['gf_zero_spam_token'] = sanitize_text_field( (string) rgpost( 'gf_zero_spam_token' ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Store the token in the draft JSON for audit purposes only; not re-validated on resume. | |
| $submission['partial_entry']['gf_zero_spam_token'] = rgpost( 'gf_zero_spam_token' ); | |
| // Store the token in the draft JSON for audit purposes only; not re-validated on resume. | |
| $submission['partial_entry']['gf_zero_spam_token'] = sanitize_text_field( (string) rgpost( 'gf_zero_spam_token' ) ); | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/class-gf-zero-spam.php` around lines 91 - 93, Sanitize the incoming
token before persisting it: replace the raw rgpost('gf_zero_spam_token')
assignment with a cleaned value using WordPress sanitization (e.g., $token =
sanitize_text_field( rgpost('gf_zero_spam_token') );) and then store
$submission['partial_entry']['gf_zero_spam_token'] = $token; — update the code
in includes/class-gf-zero-spam.php where the partial_entry is populated so only
the sanitized token is written.
Comment on lines
+196
to
+197
| $fallback_token = GF_Zero_Spam_Token::mint( $form_id, DAY_IN_SECONDS ); | ||
| $rest_url = esc_url( rest_url( 'gf-zero-spam/v1/token' ) ); |
There was a problem hiding this comment.
Fallback token TTL is too long for anti-replay goals.
Line 196 mints a fallback token for DAY_IN_SECONDS, which re-opens a long replay window when endpoint fetches fail.
Suggested fix
- $fallback_token = GF_Zero_Spam_Token::mint( $form_id, DAY_IN_SECONDS );
+ $fallback_ttl = 600;
+ $fallback_token = GF_Zero_Spam_Token::mint( $form_id, $fallback_ttl );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $fallback_token = GF_Zero_Spam_Token::mint( $form_id, DAY_IN_SECONDS ); | |
| $rest_url = esc_url( rest_url( 'gf-zero-spam/v1/token' ) ); | |
| $fallback_ttl = 600; | |
| $fallback_token = GF_Zero_Spam_Token::mint( $form_id, $fallback_ttl ); | |
| $rest_url = esc_url( rest_url( 'gf-zero-spam/v1/token' ) ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/class-gf-zero-spam.php` around lines 196 - 197, The fallback token
TTL is too long: replace the DAY_IN_SECONDS used in GF_Zero_Spam_Token::mint
(where $fallback_token is created) with a much shorter lifetime (e.g.,
MINUTE_IN_SECONDS or a config-driven short constant like FALLBACK_TOKEN_TTL) so
the fallback window for replay protection is minimal; update the call site that
mints $fallback_token and any related tests/config to use the new short TTL and
ensure it remains shorter than the primary token lifetime.
Comment on lines
+461
to
+468
| $deadline = get_option( 'gf_zero_spam_legacy_deadline' ); | ||
|
|
||
| // Set the migration deadline on first encounter. | ||
| if ( false === $deadline ) { | ||
| $deadline = time() + ( 14 * DAY_IN_SECONDS ); | ||
|
|
||
| update_option( 'gf_zero_spam_legacy_deadline', $deadline, false ); | ||
| } |
There was a problem hiding this comment.
Migration window starts too late (first legacy hit).
The deadline is initialized on first legacy submission, so legacy keys can remain acceptable indefinitely until that first hit.
Suggested direction
- // Set the migration deadline on first encounter.
- if ( false === $deadline ) {
- $deadline = time() + ( 14 * DAY_IN_SECONDS );
-
- update_option( 'gf_zero_spam_legacy_deadline', $deadline, false );
- }
+ // Deadline should be initialized during migration bootstrap, not on first legacy request.
+ if ( false === $deadline ) {
+ return __( 'Legacy spam prevention key no longer accepted. Please clear your page cache.', 'gravity-forms-zero-spam' );
+ }// Add once during plugin init/upgrade flow (outside this method).
private function maybe_initialize_legacy_deadline(): void {
$deadline = get_option( 'gf_zero_spam_legacy_deadline' );
$key = get_option( 'gf_zero_spam_key' );
if ( false === $deadline && ! empty( $key ) ) {
update_option( 'gf_zero_spam_legacy_deadline', time() + ( 14 * DAY_IN_SECONDS ), false );
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/class-gf-zero-spam.php` around lines 461 - 468, Migration deadline
is being set lazily on first legacy submission in class-gf-zero-spam.php which
allows legacy keys to remain valid indefinitely; instead, add a method (e.g.,
maybe_initialize_legacy_deadline) that runs during plugin init/upgrade to set
gf_zero_spam_legacy_deadline to time() + 14 * DAY_IN_SECONDS if the option is
missing and gf_zero_spam_key exists, remove the current lazy initialization
block from the legacy handling code, and call the new method from the plugin's
initialization/upgrade routine (reference the class name and the new
maybe_initialize_legacy_deadline method and the gf_zero_spam_legacy_deadline and
gf_zero_spam_key options to locate where to make the changes).
|
|
||
| = develop = | ||
|
|
||
| * Added: Stronger spam prevention using signed tokens that can't be copied from the page source by bots |
There was a problem hiding this comment.
Security claim is currently too absolute.
Line 115 says tokens can’t be copied from page source, but includes/class-gf-zero-spam.php embeds fallbackToken client-side (Line 214), so this should be softened.
Suggested wording
-* Added: Stronger spam prevention using signed tokens that can't be copied from the page source by bots
+* Added: Stronger spam prevention using short-lived signed tokens fetched at submit time, with compatibility fallbacks for restricted environments📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Added: Stronger spam prevention using signed tokens that can't be copied from the page source by bots | |
| * Added: Stronger spam prevention using short-lived signed tokens fetched at submit time, with compatibility fallbacks for restricted environments |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@readme.txt` at line 115, The README's absolute security claim ("tokens can't
be copied from the page source") is incorrect because
includes/class-gf-zero-spam.php exposes a client-side fallbackToken (reference
fallbackToken in that file); update the README wording to soften the claim and
accurately describe the protection: state that tokens are signed/validated
server-side and tied to request/context (or expiry) rather than being impossible
to read from page source, and mention that client-side exposure does not by
itself allow successful reuse without the server-side checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gf_zero_spam_key(visible in page source, replayable by bots) with HMAC-SHA256 signed tokens fetched at submit timeAUTH_KEY/SECURE_AUTH_KEY, scoped per form, expire in 10 minutes, and include a random noncegf_zero_spam_client_ip,gf_zero_spam_rate_limitCloses GFZSPAM-14
💾 Build file (78592b7).
Summary by CodeRabbit
New Features
Documentation