fix(relay): harden pair body limit, challenge rate-limit, allowlist parse (#61) - #64
Conversation
…arse (#61) Close P2 availability gaps from M3.2 audit: bound POST /pair body size, rate-limit GET inbox challenge issuance, and fail-closed allowlist JSON parse.
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRelay hardening adds a reusable rate-limit consumer, isolates inbox challenge issuance throttling, makes malformed allowlists fail closed, and limits ChangesRelay rate-limit and challenge flow
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/relay/src/routes/inbox.test.ts (1)
2639-2661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover authenticated-pull isolation directly.
This test exhausts challenge issuance and verifies POST remains available, but never performs a signed
GET /inbox/:agentId. Obtain Bob’s challenge, exhaust the shared challenge bucket using another agent ID, then assert Bob’s signed pull still returns 200.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/relay/src/routes/inbox.test.ts` around lines 2639 - 2661, Update the test “does not count authenticated inbox pulls against POST rate limit bucket” to obtain Bob’s challenge, exhaust the shared challenge bucket with requests for a different agent ID, then perform a signed GET /inbox/:agentId for Bob and assert it returns 200. Preserve the existing POST availability assertion while adding direct coverage that authenticated pulls are isolated from challenge rate limiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/relay/src/routes/inbox.test.ts`:
- Around line 2639-2661: Update the test “does not count authenticated inbox
pulls against POST rate limit bucket” to obtain Bob’s challenge, exhaust the
shared challenge bucket with requests for a different agent ID, then perform a
signed GET /inbox/:agentId for Bob and assert it returns 200. Preserve the
existing POST availability assertion while adding direct coverage that
authenticated pulls are isolated from challenge rate limiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fce355f-7cc3-42e5-85b1-7be200ca8167
📒 Files selected for processing (8)
packages/relay/src/middleware/rate-limit.tspackages/relay/src/routes/allowlist.test.tspackages/relay/src/routes/allowlist.tspackages/relay/src/routes/inbox.test.tspackages/relay/src/routes/inbox.tspackages/relay/src/routes/pair.test.tspackages/relay/src/routes/pair.tspackages/relay/src/server.ts
| expect(body.error).toBe("rate_limit_exceeded"); | ||
| }); | ||
|
|
||
| it("does not count authenticated inbox pulls against POST rate limit bucket", async () => { |
There was a problem hiding this comment.
[suggestion] The test is named "does not count authenticated inbox pulls against POST rate limit bucket" but never performs a signed GET /inbox/:agentId. It only exhausts the challenge-issue bucket and asserts POST is not 429. That covers challenge-vs-POST isolation (the e2e regression vector) but does not lock in the second-order fix: authenticated pulls must remain unthrottled after challenge minting is exhausted. A future change that re-applies middleware rate-limit to the whole GET handler could pass this unit test while re-breaking session_open.
Suggestion: Rename to reflect what is asserted (challenge bucket does not share state with POST), and add a case that: (1) obtains a valid challenge+sig for Bob, (2) exhausts challenge issuance (optionally via another agent id), (3) performs Bob’s signed GET and expects 200 (not 429).
| }); | ||
| const port = 13013; | ||
| const base = `http://127.0.0.1:${port}`; | ||
| const server = serve({ fetch: relay.app.fetch, port }); |
There was a problem hiding this comment.
[suggestion] This test calls serve({ fetch, port }) without waiting for the listen callback, then immediately fetches. Sibling suites (including the beforeAll in the same describe at line 2610–2612) wait on the listen resolve to avoid ECONNREFUSED races under load.
Suggestion: Mirror the beforeAll pattern: await new Promise<void>((resolve) => { server = serve({ fetch: relay.app.fetch, port }, resolve); }) before the first request.
| routes.post( | ||
| "/pair/:sessionId", | ||
| bodyLimit({ | ||
| maxSize: MAX_ENVELOPE_WIRE_BYTES, |
There was a problem hiding this comment.
[nit] Pair body limit correctly uses MAX_ENVELOPE_WIRE_BYTES (64 KiB) and payload_too_large, matching Appendix C.4 “other routes.” Inbox still keeps a second app-level UTF-8 length check after bodyLimit; pair relies solely on Hono’s middleware. Under Node’s Content-Length-framed body this is fine, and tests cover oversize/exact boundaries.
Suggestion: Optional defense-in-depth: after c.req.text(), reject if utf8ToBytes(messageJson).length > MAX_ENVELOPE_WIRE_BYTES (same pattern as inbox). Only worth it if you want parity with inbox’s double gate; not required for #61 acceptance.
#64) Address PR review: wait for server listen, exhaust challenge bucket via another agent id, assert signed GET still returns 200 and POST stays unblocked.
1 participant
Summary
Closes #61 — relay P2 hardening from M3.2 security audit:
bodyLimit(MAX_ENVELOPE_WIRE_BYTES, 64 KiB) toPOST /pair/:sessionIdwith413 payload_too_large(Appendix C.4).isSenderAllowedwhenallowed_jsonis malformed or non-array (deny instead of 500).Follow-up fix: initial REL-2 middleware on all
GET /inboxshared the POST rate-limit bucket, causing e2esession_opento fail with maskedrelay_unavailable(429). Scoped limit to challenge minting only.Type of Change
Test plan
pnpm test— 796 passedpnpm --filter @agentpair/relay typecheck— cleanbiome checkon changed files — cleanNew coverage:
pair.test.ts— oversize POST → 413; exact 64 KiB → 204inbox.test.ts— challenge issuance → 429; POST not blocked when challenge bucket exhaustedallowlist.test.ts— corrupt/non-array JSON fail-closed; POST inbox → 403 (not 500)Pre-flight Checklist
Summary by CodeRabbit
429responses when limits are exceeded.413.