Skip to content

Comments

fix: make rate limiting auth-aware under shared proxy ips#412

Open
lc0rp wants to merge 5 commits intoopenclaw:mainfrom
lc0rp-contrib:fix/411-auth-aware-rate-limit
Open

fix: make rate limiting auth-aware under shared proxy ips#412
lc0rp wants to merge 5 commits intoopenclaw:mainfrom
lc0rp-contrib:fix/411-auth-aware-rate-limit

Conversation

@lc0rp
Copy link

@lc0rp lc0rp commented Feb 18, 2026

Summary

Today, API rate limiting blocks based on IP or key. In proxied/shared-egress environments, IP sharing can deny legitimate authenticated users.

Ref issues: #349, #390

  • Make API rate limiting prefer key for authenticated requests and keep IP enforcement for anonymous/invalid-token requests
  • Invert trusted fallback precedence to x-forwarded-for -> x-real-ip -> fly-client-ip
  • Add structured deny logs (auth, userAllowed, ipAllowed, ipSource, hasClientIp)
  • Update HTTP API docs to reflect auth-aware limiter semantics

Rationale

  • current ip OR key deny behavior can block valid authenticated users behind shared egress/proxy IPs
  • user-first enforcement for authenticated traffic reduces false positives while preserving anonymous IP controls
  • x-forwarded-for first hop is the more common client-IP signal in trusted proxy chains

Work done

  • convex/lib/httpRateLimit.ts
    • switched to auth-aware decision model
    • switched trusted fallback precedence to x-forwarded-for first
    • added structured rate-limit deny logging
  • convex/lib/httpRateLimit.test.ts
    • added auth-vs-anon enforcement regression coverage
    • added forwarded-header precedence coverage
  • convex/httpApiV1.handlers.test.ts
    • adjusted auth mocks for stars routes under new limiter path
  • docs/http-api.md, docs/api.md
    • documented auth-aware enforcement semantics

Testing

bun run lint
bunx vitest run convex/lib/httpRateLimit.test.ts convex/httpApiV1.handlers.test.ts
bun run test

Checklist

  • Added test coverage and tested
  • bun run lint
  • Tested and working locally

Tag-teamed w/ codex 5.3 on this 🤖

Fixes #411, #349, #390

Greptile Summary

Modified rate limiting from IP OR key enforcement to auth-aware enforcement: authenticated requests now consume only user buckets, while anonymous requests use IP buckets. This prevents legitimate authenticated users from being blocked by shared proxy IPs while preserving IP-based protection for anonymous traffic.

Key changes:

  • Switched IP header precedence to x-forwarded-forx-real-ipfly-client-ip (more common proxy pattern)
  • Authenticated requests skip IP bucket consumption entirely (addressed previous reviewer's concern)
  • Added structured deny logging with auth status, IP source, and bucket states
  • Fixed double header parsing in client SDK (addressed previous reviewer's concern)
  • Enhanced client retry logic to respect Retry-After headers with jitter
  • Updated rate limit headers to include both legacy (X-RateLimit-*) and standardized (RateLimit-*) formats
  • Comprehensive test coverage for auth vs anonymous enforcement, token validation fallback, and header precedence

Confidence Score: 5/5

  • This PR is safe to merge with no blocking issues
  • The implementation correctly addresses the shared proxy IP problem with clean auth-aware logic, comprehensive test coverage validates all critical paths including edge cases, previous review concerns about IP consumption and header parsing have been properly fixed, documentation is thorough, and the changes follow established patterns in the codebase
  • No files require special attention

Last reviewed commit: a176658

@vercel
Copy link
Contributor

vercel bot commented Feb 18, 2026

@lc0rp is attempting to deploy a commit to the Amantus Machina Team on Vercel.

A member of the Team first needs to authorize it.

@lc0rp lc0rp marked this pull request as ready for review February 18, 2026 15:24
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

10 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

lc0rp and others added 3 commits February 18, 2026 10:58
fix header precedence in docs to match code

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@lc0rp
Copy link
Author

lc0rp commented Feb 19, 2026

@greptileai I addressed the comments. Thoughts?

@greptile-apps
Copy link

greptile-apps bot commented Feb 19, 2026

Looks good! Both issues are resolved:

  1. IP bucket isolation - Authenticated requests now only touch the user bucket. The code cleanly branches at if (userId) and never calls checkRateLimit for the IP when authenticated, so shared proxy IPs won't starve anonymous users.

  2. Header precedence docs - docs/http-api.md now correctly lists x-forwarded-for before x-real-ip, matching the implementation.

The auth-aware enforcement logic is clear, the retry/header parsing improvements are solid, and test coverage looks thorough. Good to merge.

@lc0rp
Copy link
Author

lc0rp commented Feb 19, 2026

@greptileai Do you edit the score added to the description when comments are addressed?

@greptile-apps
Copy link

greptile-apps bot commented Feb 19, 2026

No, I don't automatically edit the score in the PR description when comments are addressed. The score reflects my assessment at the time of the last full review (commit 321bc9f in this case).

If you'd like an updated score after addressing comments, you would typically:

  1. Ask me to re-review the PR, or
  2. Push new commits, which would trigger a fresh review with an updated score

The score in the description is essentially a snapshot from that review moment, not a living counter that updates as discussion progresses.

@lc0rp
Copy link
Author

lc0rp commented Feb 19, 2026

Gotcha, @greptileai re-review the PR and update the score

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.

Bug: Authenticated users can be rate-limited by shared proxy IPs

1 participant