Repository navigation
Normalize payload key order before hashing in IdempotencyStore - #75
Merged
Merged
Conversation
hash('sha256', json_encode($request->all())) was sensitive to JSON
field order, so a retry carrying the same logical payload but
serialized with fields in a different order (a different client/proxy
serialization, not a real payload change) was misclassified as a
conflicting payload and got a spurious 409 instead of replaying the
original response -- violating the class's own documented idempotency
contract. Flagged independently by the #42 and #44 backend PRs' code
reviews as an issue in this shared class rather than something either
should fix unilaterally.
normalizeForHash() recursively ksort()s associative keys before
hashing; sequential list arrays are unaffected since ksort() on
0..n-1 keys is a no-op, so element order within a list (which is
semantically meaningful, e.g. tags) is preserved.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011u3o4QCgpcqH51edYyRPMn
matbrgz
added a commit
that referenced
this pull request
Sep 11, 2026
…s PR #73 review round 2 Merges in #74 (similarity backend), #75 (IdempotencyStore payload-hash key ordering), and #76 (Contest::getContestTime() sign fix). Only conflict was routes/web.php's `require` lines (kept all of them). A. Removes BocaWebcastZipBuilder::timeFile()'s local workaround for Contest::getContestTime()'s Carbon 3 signed-diff bug, now that #76 fixes the root cause directly on the model. timeFile() just calls $contest->getContestTime() again -- no more parallel implementation of the same elapsed-time calculation. The value produced is unchanged (the workaround already computed the correct positive figure), so no test expectations needed correcting, just the redundant code removed. B. Ten further review findings, all fixed: 1. Webcast\ScoreboardController::show() now null-checks $credential-> contest before use -- a still-active credential whose contest was soft-deleted (Contest uses SoftDeletes; ContestController::destroy() never cascades) previously reached Leaderboard::getScoreboard(null) and threw a TypeError instead of a clean 401. 2. BocaWebcastZipBuilder::teams() uses ?? instead of ?: for the fallback team name -- ?: silently replaced a team literally named "0" with the placeholder, since "0" is falsy in PHP. 3. AuthenticateWebcastCredential now eager-loads `contest` on the credential lookup (the one route a credential is meant to be polled on repeatedly). 4. WebcastController::storeCredential() no longer runs a redundant Rule::exists('contests','id') validation query alongside the Contest::find() + null-check that already answers the same question (and already correctly excludes soft-deleted contests). 5. WebcastController::export() now uses response()->download($path)-> deleteFileAfterSend(true) instead of a hand-rolled streamDownload()+ readfile()+register_shutdown_function() -- Symfony's BinaryFileResponse already calls ignore_user_abort(true) and unlinks the temp file in a finally block around the transfer loop, so a client-cancelled download still gets cleaned up via the framework's own mechanism. 6. WebcastController::index() now uses Eloquent's paginate() instead of hand-rolled page/perPage/lastPage/forPage bookkeeping, matching the convention Api\Frontend\ManagedAccountsController (#47) already established for the same "session-admin JSON list" shape. 7. Removed the dead hash_equals() re-check in AuthenticateWebcastCredential -- a row can only be returned by `WHERE token_hash = $hash`, so comparing them again could never evaluate false; it was presented as a timing-safety measure but wasn't one. 8. Left a comment on the storeCredential() secret-redaction workaround describing the pattern and a possible shared fix (an optional redact closure on IdempotencyStore::handle()) as a follow-up, since that would touch the shared class #47's endpoint also depends on. 9. BocaWebcastZipBuilder::resultCode() simplified in_array($run->status, ['judged'], true) to the direct $run->status !== 'judged' comparison. Regression tests added for #1 (soft-deleted contest -> 401), #2 (team named "0"), and #3 (relationLoaded('contest') on the credential lookup, tested directly against the query shape rather than by counting queries over a full request, which would also pick up Leaderboard::getScoreboard() 's own unrelated internal Contest::findOrFail() call). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011u3o4QCgpcqH51edYyRPMn
13 tasks
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
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
IdempotencyStore::handle()hashed the rawjson_encode($request->all())to detect payload conflicts, which is sensitive to JSON field order. A retry carrying the same logical payload but serialized with fields in a different order (different client/proxy) was misclassified as a conflicting payload, returning a spurious 409 instead of replaying the original response.Independently flagged by the code-review passes on both #42 (similarity) and #44 (webcast) PRs, which build on this shared class from #47 -- correctly left unfixed there since it's shared infrastructure code, not theirs to change unilaterally.
Fix
normalizeForHash()recursivelyksort()s associative array keys before hashing. Sequential list arrays are unaffected (ksort()on0..n-1keys is a no-op), so element order within a list -- which is semantically meaningful, e.g. tags -- is preserved.Test plan
test_repeating_the_same_idempotency_key_with_differently_ordered_payload_fields_still_replays-- same logical payload, different field order, same key, expects a replay (same id) not a 409.🤖 Generated with Claude Code
https://claude.ai/code/session_011u3o4QCgpcqH51edYyRPMn