fix(#2476): bind the HTTP method and path into the keeper request HMAC - #2534
Merged
Conversation
The signed string was `"<timestamp>.<rawBody>"` — no method, no path. Every route
sharing KEEPER_REGISTER_SECRET therefore accepted every other route's signatures:
the credential authenticated "someone who knows the secret", not "this request".
A signature minted for one endpoint verified on another with the same body.
Now signs `[timestamp, METHOD, path, rawBody].join("\n")`.
Newline-separated rather than dot-separated: a dot can appear in a path, so
"POST./a.b" and "POST./a" + ".b" would be ambiguous under the old delimiter. A
newline cannot appear in a method or a URL path. Method is upper-cased and path
excludes origin and query, so the two ends cannot disagree over casing or a host.
ONE ENDPOINT IS DELIBERATELY TRANSITIONAL, and this is the part worth reading.
`PATCH /api/markets/[slab]` verifies signatures from a caller that is NOT in this
repo — the keeper service has no createHmac at all, so nothing I can see signs
it. Making that verifier strict could break a live hop I cannot inspect, so it
accepts the legacy unbound form as well AND LOGS EVERY ACCEPTANCE. That is not a
fix; an unbound signature is still valid for any endpoint sharing the secret,
which is the whole of #2476. The log exists so the transition can be finished
rather than forgotten — once it goes quiet, drop the flag.
The internal markets -> oracle-keeper hop has both ends in this repo and both are
updated here, so that verifier is STRICT immediately. A test pins that it rejects
the legacy form.
WHAT REMAINS, exactly as #2476 says: there is still no nonce, so a captured
signature is replayable against the SAME endpoint within MAX_SIGNATURE_AGE_MS.
A nonce needs a store shared across serverless instances — the same constraint
that shaped the keeper-register proof in #2505 — so it is a separate design
problem, not a line change.
FOUND WHILE DOING THIS, filed as #2533: the launch app sends HMAC headers to the
keeper's /register, but that endpoint requires `x-shared-secret`, which the app
never sends. Hot-registration 401s and fails silently behind "Keeper unreachable
— market will auto-discover on next cycle". LAUNCH-16 migrated the sender and
never the receiver, which is also why #2233 is still accurate.
An existing test signed the unbound message; split into sign() and signUnbound()
so the legacy form is still constructible and is now used to assert REJECTION.
Negative control: removing the binding fails 2 of the 9 tests in that file.
Launch suite: 3138 passed / 16 skipped / 0 failed.
Refs: #2476
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Closes #2476.
The signed string was
"<timestamp>.<rawBody>"— no method, no path. Every route sharingKEEPER_REGISTER_SECRETtherefore accepted every other route's signatures: the credential authenticated "someone who knows the secret", not "this request". A signature minted for one endpoint verified on another with the same body.Now:
[timestamp, METHOD, path, rawBody].join("\n").Newline-separated, not dot-separated. A dot can appear in a path, so
"POST./a.b"and"POST./a"+".b"would be ambiguous under the old delimiter. A newline cannot appear in a method or a URL path. Method is upper-cased, path excludes origin and query — so the two ends cannot disagree over casing or a trailing host.One endpoint is deliberately transitional
This is the part worth reading before approving.
PATCH /api/markets/[slab]verifies signatures from a caller that is not in this repo — the keeper service has nocreateHmacat all, so nothing I can inspect signs it. Making that verifier strict could break a live hop I cannot see.So it accepts the legacy unbound form and logs every acceptance:
That is not a fix — an unbound signature is still valid for any endpoint sharing the secret, which is the whole of #2476. The log exists so the transition can be finished rather than forgotten.
The internal
markets → oracle-keeperhop has both ends in this repo, both updated here, so that verifier is strict immediately. A test pins that it rejects the legacy form.What remains, exactly as the issue says
No nonce. A captured signature is still replayable against the same endpoint within
MAX_SIGNATURE_AGE_MS. A nonce needs a store shared across serverless instances — the same constraint that shaped the keeper-register proof in #2505 — so it is a separate design problem, not a line change.Found while doing this — filed as #2533
The launch app sends HMAC headers to the keeper's
/register, but that endpoint requiresx-shared-secret, which the app never sends. Hot-registration 401s and fails silently behind "Keeper unreachable — market will auto-discover on next cycle".LAUNCH-16 migrated the sender and never the receiver — which is also why #2233 is still accurate.
Verification
sign()/signUnbound()so the legacy form is still constructible and now asserts rejectiontscclean🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D