Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
P1 — moving The hook moved from a plugin-scope
Splitting the two hooks keeps the auth-first short-circuit and gets the body back: onRequest: [claimedAgentAuthMiddleware],
preHandler: [agentContextHook],Worth a test that asserts through P1 — confirm intended: the card-reveal rate limit is now per-agent, not per-intent. - return `card-reveal:${intentId}`;
+ return `card-reveal:${req.authenticatedAgent?.id ?? 'unauthenticated'}`;Old rule: any caller gets 2 reveals per minute for a given intent. New rule: an agent gets 2 reveals per minute across every intent it owns. Keying on Whichever you pick, Note — the rate limiter runs after the credential lookup.
Nothing else. Auth is applied consistently across all seven agent routes with no gaps, the |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d14f6e74
ℹ️ 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".
78d14f6 to
a8511d5
Compare
|
Pushed P0 — recheck revocation before mutating an intent (@chatgpt-codex-connector). Confirmed and fixed. Five tests cover the window, and they are shaped so authentication sees the live credential and the change lands only in time for the recheck, which is the race an actually-slow request opens: revoked, unlinked, rotated, and expired all return 401 with the mutation never reached, plus one asserting a live credential still gets through. P1 — rate-limit before authentication (@chatgpt-codex-connector). Right, and worse than "the limiter runs late": on agent routes it never ran at all for rejected requests. The global limiter is back at P1 — intent creation without a linked agent (@chatgpt-codex-connector). Not intended. An intent stored with P1 — P1 — card-reveal rate limit key. Left keyed on the agent, since scoping it to agent + intent reopens the bypass that Unit suite is 431 green. |
|
To use Codex here, create a Codex account and connect to github. |
The Coinbase work lands as a stack: #202, #203, and #204 are all based on feat/coinbase-foundation rather than on main. The pull_request trigger filtered on branches: [main], so none of those three ran lint, type check, unit tests, integration tests, or CodeQL -- the only green checks on them were Dependabot and CodeRabbit, and the gap is invisible because a PR with no matching workflow looks the same as one with nothing to run. Dropping the filter costs nothing: a pull_request event still only fires for an open PR, and same-repo branches keep access to the Stripe secret the integration job needs.
The authentication hook runs once, early. A caller that has already sent its headers can hold the connection open and finish after the user unlinks it or the credential is revoked or rotated; request.authenticatedAgent still holds the identity captured beforehand, and requireOwnedIntent compared only that cached value against unchanged intent fields. A compromised agent could therefore post a quote or a checkout result, or reveal a card, after revocation. Every guarded mutation now re-reads the live PairingCode row and the user link before acting. It is a re-read by agent id, not a second bcrypt verification: the credential was already proven, and revocation, rotation, expiry, and the link are all observable without it. The global rate limiter ran at preHandler, behind each agent route's onRequest authentication hook, so a 401 terminated the request before the limiter counted it -- an invalid-key flood reached the PairingCode lookup and the bcrypt verifier unbounded, which is exactly the traffic the advertised 60/min ceiling exists to shed. It now runs at onRequest keyed on IP alone, ahead of authentication. Agent keying moves to the routes that need it, where it can run at preHandler with the identity established. agentContextHook was registered in the same onRequest array as authentication. onRequest fires before body parsing, so request.body was undefined and the two routes carrying intentId in the body -- quote and result, the two that move money -- logged intentId: null. Both hooks move to preHandler. The existing unit test passed a hand-built request object that cannot occur at that registration point; the new tests assert through app.inject and fail against the old wiring. POST /v1/intents accepted a user with no linked agent and stored agentId = null. That intent was enqueued, advanced to SEARCHING, and then failed the ownership check on every agent route for the rest of its life, with nothing able to attach an agent later. Creation now fails with 409 agent_not_linked. Also names the two route rate-limit budgets and records why card reveal is keyed on the agent: the intent-scoped key it replaced was bypassable by rotating the intent, and the cost is that it bounds an agent's concurrent checkouts.
a8511d5 to
7eaec9b
Compare
requireOwnedIntent now re-reads the live PairingCode row by agentId before a guarded mutation, but this suite's Prisma double answered only the credentialPrefix lookup used during authentication. The recheck therefore read null, treated the credential as revoked, and every ownership, card-reveal, and state-guard case returned 401 instead of its expected 403, 404, or 409. Found by CI, which started running on this PR once the pull_request trigger stopped filtering on branches: [main].
Summary
Closes #190.
This PR establishes the authenticated agent boundary required before Coinbase wallet or transaction routes can be enabled:
agk_...credential per registered agent and stores only a bcrypt verifier plus lookup prefixagent -> linked user -> intentthrough one shared ownership guard on quote, result, decision, and card routes401/403error codes, verified-agent audit attribution, and agent-keyed rate limitsStack
This PR is stacked on #201 and intentionally targets
feat/coinbase-foundationso the review contains only issue #190. After #201 merges, this PR should be retargeted tomain.Security properties
WORKER_API_KEYis accepted only as the server-side initial-registration bootstrap; operational routes requireX-Agent-KeyX-Agent-Idnever establishes identity and a mismatched value is rejectedclaimedAgentAuthMiddlewareandrequireOwnedIntent; this PR does not expose transaction executionAPI and migration
POST /v1/agent/registerwithX-Worker-Keyand{}X-Agent-KeyPOST /v1/agent/credential/rotatewith the currentX-Agent-KeyVerification
npm run buildnpm run lint(zero errors)npm run format:checknpx prisma validateandnpx prisma generateReviewer focus