fix: 2.10.3 — regenerating the site token actually stores it (#67) - #68
Conversation
"Regenerate Token" revealed a fresh token and stored nothing. The option was
registered as a read-only setting whose sanitize_callback returned the existing
value, and register_setting() installs that callback as a
`sanitize_option_aura_worker_site_token` filter which update_option() applies on
EVERY write — not only on writes from the settings form. So the handler's write
was discarded while the reveal transient, the connect-user write and the
dashboard-url deletion all landed, and the screen faithfully displayed a token
that existed nowhere.
The guard only bites once a value is stored: with the option empty,
get_option($k, $new_value) returns the default, which is why activation and the
magic-link connect were unaffected and this survived a release unnoticed. The
filter is registered on admin_init — fired by admin-ajax.php, not by REST — so
the connect path kept working while regeneration and the legacy raw-to-hash
migration in check_aura_token() silently did not.
Two consequences, the first a security defect: an admin rotating a leaked token
was told it was revoked while the old token stayed valid; and a site
disconnected from the dashboard became unreconnectable, because reconnecting
needs a site token and every token the screen offered was rejected.
The token is no longer registered as a setting. It is display-only —
render_token_field() emits no input carrying its name — so nothing submits it,
and leaving it out of the group's allow-list is what stops options.php writing
it. Regeneration now reads the stored value back and refuses to reveal a token
it could not save, so this class of failure cannot be silent again.
Test harness: update_option() now applies the `sanitize_option_{$option}`
filter as core does, and register_setting() installs it. Without that, a
regression test for this bug passes against the bug.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47dda4449
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( ! hash_equals( $hashed, (string) get_option( 'aura_worker_site_token', '' ) ) ) { | ||
| wp_send_json_error( | ||
| array( | ||
| 'message' => __( 'The new site token could not be saved, so the current token is unchanged. Check for a plugin filtering this option, or for a database write error.', 'digitizer-site-worker' ), |
There was a problem hiding this comment.
When a sanitize_option_aura_worker_site_token or pre_update_option_aura_worker_site_token filter rewrites the proposed hash rather than rejecting it, update_option() persists that rewritten value and this comparison then returns an error without restoring the previous hash or revealing a usable token. In that explicitly handled scenario, the response incorrectly says the current token is unchanged even though the old token has already been revoked, potentially leaving the site inaccessible; retain the previous value and restore/verify it on mismatch, or prevent the incompatible transformed value from being committed.
Useful? React with 👍 / 👎.
| $this->assertArrayNotHasKey( | ||
| 'sanitize_option_aura_worker_site_token', | ||
| $GLOBALS['_filters'], |
There was a problem hiding this comment.
This test claims to verify that the token is not registered, but it only checks for a sanitize filter. A future register_setting( 'aura_worker_settings', 'aura_worker_site_token' ) call without a sanitize callback would add the token to the options.php allow-list while leaving this assertion green, permitting a crafted settings submission to overwrite the token. Assert against the newly recorded _registered_settings['aura_worker_settings'] list instead.
Useful? React with 👍 / 👎.
…e write (Codex round-1 P2) Two findings from the review, both real. A filter can REWRITE the proposed hash rather than refuse it. update_option() then persists the rewritten value, so the row matches neither the new token nor the old one and the site authenticates nothing at all — while the handler reported "the current token is unchanged", which was false, and left it that way. The handler now captures the previous value first, and on a mismatch puts it back and verifies the restore, reporting which of the two states it actually ended in: unchanged, or a store that may accept no token and needs setting by hand. A failed rotation can no longer lock a site out. The allow-list assertion tested the wrong mechanism. options.php writes only options registered to the group, so a register_setting() call with no sanitize callback would re-expose the token to a crafted submission while a filter-only assertion stayed green. The test now asserts the option is absent from _registered_settings['aura_worker_settings'] as well. Regression test: a rewriting filter must leave the previous token in place and reveal nothing. It fails against the previous commit, which stored strrev($hash) and reported success at restoring nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings were real and are fixed in Preserve the old token when verification fails — correct, and the distinction matters: a filter that rewrites the value persists it, so the row ends up matching neither token and the site authenticates nothing, while the handler claimed the current token was unchanged. It now captures the previous value first and, on a mismatch, restores it and verifies the restore, then reports whichever state it actually ended in — unchanged, or a store that may accept no token and needs setting by hand. Assert the option is absent from the settings allow-list — also correct: a New regression test covers the rewriting-filter case and fails against the previous commit. 838 tests passing, lint clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bc4be4362
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // rather than assuming the store was left untouched. | ||
| $restored = hash_equals( $previous, $stored ); | ||
| if ( ! $restored ) { | ||
| update_option( 'aura_worker_site_token', $previous ); |
There was a problem hiding this comment.
When two administrators rotate concurrently, this unconditional restoration can overwrite a token that the other request has already stored and reported as successful. For example, request A reads the previous hash and reaches this failure path, request B then completes a successful rotation, and A finally writes its stale $previous value here; B's revealed token no longer authenticates and the potentially compromised original token becomes valid again. The new restoration should be serialized or use a compare-and-swap that restores only if the database still contains the exact failed value written by A.
Useful? React with 👍 / 👎.
… rotation (Codex round-2 P1) The restore added in the previous commit was unconditional, which introduced a worse failure than the one it fixed. Two administrators rotating at once: A reads the previous hash and reaches the failure path, B completes a successful rotation, then A writes its stale value back. B's freshly revealed token stops authenticating and the token A was rotating away from — the compromised one, in the case this feature exists for — becomes valid again. The restore is now a single-statement compare-and-swap against the exact bytes this request observed, following swap_raw() in class-aura-worker-rules.php: it either lands on the value it decided against or does nothing. $wpdb->query() answering 0 (matched nothing) is a lost race, not a fault, and is distinguished from false (SQL error) — neither counts as restored. The error message is now derived from what the store actually holds rather than from what this request tried to do, because "unchanged" over a concurrent rotation's token would be exactly the kind of false claim this PR removes: previous value present → unchanged; our failed value still present → the site may accept no token, set it directly; anything else → another token was stored while this ran, and it is the current one. Two tests: the CAS must leave a concurrent rotation's token alone and report that it changed nothing, and the rewriting-filter case now asserts the row holds the previous value exactly — the raw restore bypasses the filter, which is the point of doing it in SQL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Real, and a worse bug than the one it was fixing — the unconditional restore I added in round 1 could revoke a concurrent rotation's token and resurrect the token the admin was rotating away from. Fixed in The restore is now a single-statement compare-and-swap against the exact bytes this request observed, following The error message is now derived from what the store actually holds rather than from what this request attempted, since claiming "unchanged" over a concurrent rotation's token would be the same class of false claim this PR exists to remove: previous value present → unchanged; our failed value still present → the site may accept no token, set it directly; anything else → another token was stored while this ran and is the current one. Two tests added: the CAS leaves a concurrent rotation's token alone and reports it changed nothing, and the rewriting-filter case now asserts the row holds the previous value exactly — the raw restore bypasses the filter, which is the point of doing it in SQL. 839 tests passing, lint clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff0f0588e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| // $wpdb->query() answers false for an SQL error and 0 for "matched | ||
| // nothing" — a lost race, not a fault. Neither restored the row. | ||
| wp_cache_delete( 'aura_worker_site_token', 'options' ); |
There was a problem hiding this comment.
On upgraded sites where aura_worker_site_token is autoloaded, WordPress serves it from the alloptions cache rather than the individual option key, so deleting only this key does not expose the value restored by the raw SQL. After a rewriting filter stores an unusable value and this CAS restores the previous hash in the database, the subsequent get_option() still sees the unusable cached value and reports that restoration failed; with a persistent object cache, authentication can continue using that value across requests and leave the site locked out despite the correct database row. Invalidate/update alloptions as appropriate and use an uncached database read when classifying the final state.
Useful? React with 👍 / 👎.
…e is judged from the row (Codex round-3 P1) `aura_worker_site_token` is autoloaded — nothing ever passed an explicit $autoload — so core serves it from the `alloptions` bucket and never from the per-key cache entry. The compare-and-swap writes the row directly, past both, and evicting only the key left every later get_option() answering with the value the restore had just replaced. Two consequences: the handler judged a correctly restored database as a failed restore, and on a site with a persistent object cache authentication kept running against a token no longer in the database — locked out despite a correct row. swap_raw() in class-aura-worker-rules.php evicts only the key, and is right to: the ruleset option is written with autoload 'no'. Copying that idiom to an autoloaded option is what carried the bug in. The restore now evicts `alloptions` as well, and every state decision this handler makes — the previous value, what the write actually stored, and the final classification — reads the row through Aura_Worker_Rules::site_token_uncached(), the same uncached read the connect path uses for its write verification. The cached read remains only as a fallback when the database itself errors. Regression test: with a stale cached copy in play, a rewriting filter must still leave the previous hash in the row and the message must describe the database. It fails against the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Real, and the mechanism is worth naming:
Fixed in Regression test added: with a stale cached copy in play, a rewriting filter must still leave the previous hash in the row and the message must describe the database. Verified it fails against the previous commit and passes on this one. 840 tests passing, lint clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b5e9a1940
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // site out, and say which of the two states we actually ended in | ||
| // rather than assuming the store was left untouched. | ||
| if ( ! hash_equals( $previous, $stored ) ) { | ||
| $this->restore_token_if_unchanged( $stored, $previous ); |
There was a problem hiding this comment.
When two administrators rotate concurrently, this CAS can still overwrite the successful request: A writes hash A, B writes and verifies hash B, then A's first read observes hash B and treats it as a rewritten version of A's write; because $stored is now exactly B's hash, this restore successfully replaces it with A's stale $previous hash. B then reveals a token that no longer authenticates, while the token both requests intended to revoke becomes valid again. Fresh evidence after the prior CAS fix is that the value used as $expected is merely the post-write observation and therefore does not prove that A wrote it; serialize the whole rotation or otherwise associate the restore with A's own write.
Useful? React with 👍 / 👎.
… round-4 P1) Three rounds produced three P1s on one mechanism — write, read back, repair on mismatch — each closing one interleaving and leaving another. The mechanism is unsound, not under-patched: a read-back proves what the row holds, never who wrote it. A request that lost a race to a concurrent rotation cannot distinguish that from its own write being rewritten, so "repairing" revokes the winner's fresh token and revives the one both requests were rotating away from. No amount of guarding the repair fixes a proof that cannot tell the two apart. So there is no repair path any more. The rotation is a single compare-and-swap against the value this request read: `UPDATE … WHERE option_name = … AND option_value = <previous>`, or a conditional INSERT when there is no row. Exactly one of two concurrent rotations matches; the loser writes nothing and is told which state the store is actually in. Raw SQL also puts the write out of reach of any option filter — the failure this whole change exists to remove — rather than trying to detect one after the fact. Both remaining error paths write nothing, so neither can leave the store worse than it found it. Tests reshaped around the new mechanism: a rewriting filter can no longer corrupt the stored token (the rotation now succeeds despite it), losing the swap writes nothing and reveals nothing, a site with no row yet gets one by INSERT, and a database error still reveals nothing. The first two fail against the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Correct, and this is the third P1 on the same mechanism — so I have replaced it rather than guard it again. Why patching could not converge. Write, read back, repair on mismatch is an unsound proof: a read-back establishes what the row holds, never who wrote it. A request that lost a race cannot distinguish that from its own write being rewritten, so the repair revokes the winner's fresh token and revives the token both requests were rotating away from. Each round I closed one interleaving and left another, which is the signature of the mechanism being wrong rather than under-guarded. What replaces it ( Tests reshaped: a rewriting filter can no longer corrupt the stored token (the rotation succeeds despite it), losing the swap writes nothing and reveals nothing, a site with no row gets one by INSERT, a database error reveals nothing. The first two verified to fail against the previous commit. 841 tests passing, lint clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f951058ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The swap reported a row changed; confirm from the row itself before a | ||
| // token is revealed to anyone. Nothing is written on this path either, so | ||
| // a failure here leaves the store exactly as the swap left it. | ||
| if ( ! hash_equals( $hashed, $this->stored_token() ) ) { |
There was a problem hiding this comment.
When the UPDATE/INSERT commits but the immediate readback encounters a transient database error or the fallback serves stale cached state, this branch returns an error even though the stored token has already changed. Because the raw token is neither revealed nor saved to the transient on this path, the previous token is revoked and the administrator has no usable replacement until another rotation succeeds; the affected-row result already proves this request performed the write, so a readback failure must not silently discard its token.
Useful? React with 👍 / 👎.
| if ( '' === (string) $expected ) { | ||
| $rows = $wpdb->query( |
There was a problem hiding this comment.
If wp_options already contains aura_worker_site_token with an empty value, the settings UI treats the site as unconfigured, but stored_token() returns the same '' used for an absent row and this branch attempts only the conditional INSERT. The existing row makes NOT EXISTS false, so regeneration always reports failure and can never configure the site; distinguish an absent row from an empty value or compare-and-swap the empty row.
Useful? React with 👍 / 👎.
…nd-5 P1+P2) Two findings, both on what happens around the compare-and-swap. P1 — the confirming read after a successful swap could lose the token. The swap matched a row and changed it, so the store already holds the new hash and this request is what put it there; the affected-row count is the only proof that exists, since a read answers what the row holds and never who wrote it. The read could therefore only fail spuriously — a transient database error, a stale autoloaded copy — and failing there revoked the previous token while revealing no replacement, leaving the administrator with nothing to reconnect with. The read is gone. This is the third round on the same mechanism: a read-back has now been wrong as a repair trigger, wrong as a race detector and wrong as a confirmation, so it is removed rather than narrowed again. P2 — a row that exists but holds an empty value could never be rotated. '' is two states, no row and an empty row, and neither the settings screen nor get_option() distinguishes them; choosing the statement from that read sent an empty row down the conditional INSERT, whose NOT EXISTS can never be satisfied, so the site reported a failed rotation forever and could not be configured. The UPDATE now runs first in every case — it matches an empty row and changes nothing when there is no row — and only a genuinely absent row falls through to the INSERT, so a racer's committed row is still never clobbered. Both are covered by regression tests that fail against the previous commit: one asserts no read of the token row follows the swap, the other rotates a site whose row is present and empty. The changelog, upgrade notice and README each claimed regeneration "reads the stored value back" / "verifies the write" — all three now describe the swap.
|
Round 5 addressed at P1 — do not lose the token after a successful swap. Correct, and it retires the mechanism rather than narrowing it. The swap's affected-row count is the only proof available that this request stored the token: a read answers what the row holds, never who wrote it. So the confirming read could only ever fail spuriously — a transient database error, a stale autoloaded copy — and failing there revoked the previous token while revealing no replacement. The read is gone. Regression test asserts no This is the third round on that same mechanism (repair trigger → race detector → confirmation), so it is deleted rather than fixed again. P2 — an existing empty row. Correct. The tests-line-139 P2 from the earlier round is already fixed at HEAD — 843 tests green, lint clean. The changelog, upgrade notice and README each claimed regeneration "reads the stored value back"; all three now describe the swap. @codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Fixes #67.
The bug
register_settings()declared the site token read-only:register_setting()installs that callback as asanitize_option_aura_worker_site_tokenfilter, andupdate_option()applies it on every write, from any caller. So inajax_regenerate_token()only the token write was discarded:update_option( 'aura_worker_site_token', hash_token( $raw ) )update_option( 'aura_worker_connect_user_id', … )delete_option( 'aura_worker_dashboard_url' )set_transient( 'aura_worker_token_reveal', $raw, … )wp_send_json_success( [ 'token' => $raw ] )Why it survived a release
get_option( $k, $new_value )returns the default — which is the new value — so activation's first-token write succeeds. Only rotation, which by definition runs against an existing value, is frozen.admin_init.admin-ajax.phpfires it; REST requests do not. So/wp-json/aura/v1/connectkept storing tokens normally, and once magic-link became the usual way to connect, nothing exercised the broken path. The legacy raw-to-hash migration incheck_aura_token()is silently frozen the same way on admin requests.Impact
wp option update aura_worker_site_token <sha256>over SSH.The fix
Stop registering the token as a setting. It is display-only —
render_token_field()emits no input carrying the option's name — so nothing can submit it, and leaving it out of the group's allow-list is what actually stopsoptions.phpfrom writing it. Theregister_setting()call was protecting against a submission that cannot happen, at the cost of freezing every legitimate writer.Verify the rotation before announcing it.
ajax_regenerate_token()now reads the stored value back and compares it to the hash it just wrote. On a mismatch it returns a 500 explaining that the current token is unchanged, touches nothing else, and reveals no token. A filter or a refusing database can no longer produce a silent half-rotation.Tests
Five new tests in
tests/unit/TokenRegenerateTest.php, all of which fail against the unfixed code:Two harness changes were needed to make those tests meaningful, and are worth noting because without them the suite would have passed against this bug forever:
update_option()now applies thesanitize_option_{$option}filter, as core does.register_setting()is stubbed to install that filter and record the group allow-list.Also added:
wp_send_json_success/wp_send_json_error(throwing a catchableSA_Json_Responseso a test can assert the handler terminated),check_ajax_referer,wp_generate_password,get_current_user_id,add_settings_section/add_settings_field.composer test→ 837 tests, all passing (832 before).composer lint→ clean.Note for operators on 2.10.2 or earlier
Any site whose token was "regenerated" is still using the token it had before. Rotate with
wp option update aura_worker_site_token "$(printf '%s' "$NEW" | sha256sum | cut -d' ' -f1)"after upgrading, or simply regenerate again once 2.10.3 is installed.🤖 Generated with Claude Code