Skip to content

fix: enforce IP bucket before identity rate limits#705

Merged
whoabuddy merged 1 commit into
mainfrom
fix/pr704-rate-limit-review-followup
May 1, 2026
Merged

fix: enforce IP bucket before identity rate limits#705
whoabuddy merged 1 commit into
mainfrom
fix/pr704-rate-limit-review-followup

Conversation

@whoabuddy
Copy link
Copy Markdown
Contributor

Summary

  • restore the previous safety property that every counted request checks an IP bucket before any identity bucket
  • only check identity buckets when a route explicitly opts into identityHeader
  • fail closed in staging/production when a rate-limit binding is missing
  • remove misleading maxRequests / windowSeconds fields from RateLimitOptions
  • add a regression that spoofed X-BTC-Address values cannot bypass an exhausted IP bucket

Context

Follow-up for Codex/Copilot review comments posted on merged PR #704.

Validation

  • npm run typecheck
  • npm test -- src/__tests__/rate-limit-skip.test.ts src/__tests__/signal-read-rate-limit.test.ts src/__tests__/signals.test.ts
  • npx biome lint src/middleware/rate-limit.ts src/routes/signal-review.ts src/routes/classified-review.ts src/routes/brief-compile.ts src/__tests__/rate-limit-skip.test.ts src/lib/constants.ts && git diff --check
  • npm run wrangler -- deploy --dry-run --env production

Copilot AI review requested due to automatic review settings May 1, 2026 05:00
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
agent-news 819795e May 01 2026, 05:00 AM

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

Preview deployed: https://agent-news-staging.hosting-962.workers.dev

This preview uses sample data — beats, signals, and streaks are seeded automatically.

@whoabuddy whoabuddy merged commit e2f2e86 into main May 1, 2026
9 checks passed
@whoabuddy whoabuddy deleted the fix/pr704-rate-limit-review-followup branch May 1, 2026 05:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens the rate-limit middleware’s safety properties by ensuring every counted request hits an IP-based bucket before any identity-based bucket, preventing spoofed identity headers from bypassing an exhausted IP quota. It also makes identity bucketing explicitly opt-in per route and hardens behavior when Cloudflare rate-limit bindings are misconfigured.

Changes:

  • Enforce IP bucket checks before optional identity bucket checks in checkRateLimit, and only check identity when identityHeader is explicitly set.
  • Fail closed (503) in staging/production when a configured rate-limit binding is missing.
  • Add a regression test ensuring spoofed X-BTC-Address cannot bypass an exhausted IP bucket; update publisher/editorial routes to opt into identity bucketing.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/middleware/rate-limit.ts Reworks bucket-check order (IP first, optional identity second) and changes missing-binding behavior to fail closed in staging/prod.
src/routes/signal-review.ts Opts publisher signal-review rate limiting into identity bucketing via X-BTC-Address.
src/routes/classified-review.ts Opts publisher classified-review rate limiting into identity bucketing via X-BTC-Address.
src/routes/brief-compile.ts Opts brief compile rate limiting into identity bucketing via X-BTC-Address.
src/lib/constants.ts Updates rate-limit comments to reflect Cloudflare bindings as the source of enforcement.
src/tests/rate-limit-skip.test.ts Adds regression coverage for spoofed identity headers not bypassing exhausted IP bucket.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +121 to +124
return c.json(
{ error: "Rate limit service unavailable", retry_after: config.periodSeconds },
503
);
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a rate-limit binding is missing in staging/production, the 503 response includes retry_after in the JSON body but does not set a Retry-After header. This is inconsistent with the 429 responses from this middleware and with other retryable 503 responses in the codebase, and makes it harder for clients/proxies to implement standard backoff behavior. Consider also setting Retry-After on this 503 path (using config.periodSeconds).

Suggested change
return c.json(
{ error: "Rate limit service unavailable", retry_after: config.periodSeconds },
503
);
const response = c.json(
{ error: "Rate limit service unavailable", retry_after: config.periodSeconds },
503
);
response.headers.set("Retry-After", String(config.periodSeconds));
return response;

Copilot uses AI. Check for mistakes.
whoabuddy added a commit that referenced this pull request May 3, 2026
* fix(agent-resolver): scope NEWS_KV writes to requested addresses

resolveAgentNames previously wrote every agent in the bulk fetch page
(~1000 entries) to NEWS_KV on every cache miss, even when the caller
asked about 1-2 addresses. With staggered 24h TTLs across ~1000 agents,
this single call site explains the bulk of the ~12.3K/h remaining
NEWS_KV writes after PR #704/#705 removed rate-limit counters (B1 in
cloudflare-bill-reduction-tracker-2026-05.md).

Keep the bulk fetch as the latency optimization (one HTTP page beats N
calls) but scope the KV puts to the originally-requested addresses
only; the pre-warm fan-out is dropped.

Validation: 24h post-deploy NEWS_KV writes/h via Cloudflare GraphQL,
plus smoke on /api/signals, /api/correspondents, /api/init for name
resolution still returning expected display names.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(agent-resolver): tighten cache-scoping comment

Drop the planning-doc reference per arc0btc review nit on PR #725 —
inline comments shouldn't depend on transient planning paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants