fix(auth): trim bearer tokens in rate-limit keys - #65
Conversation
There was a problem hiding this comment.
@jony376 the underlying fix is valid, but this is not merge-ready yet.
Required:
- Strengthen the test so it exercises the actual rate-limit key/enforcement path with padded and unpadded
Authorization: Bearer ...headers, not onlyextractBearerToken()/hashToken()directly. - Sync/rebase with current
mainand fix the failingvalidaterun. Current failure is in readiness/SLO integration assertions. - Keep the fix scoped to
src/auth/rate-limit.tsand auth/rate-limit tests.
3935e3d to
de39013
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
de39013 to
3ab540a
Compare
…er-trim # Conflicts: # test/unit/auth.test.ts
|
Hi, @JSONbored I fixed conflict and CI error. Please check this PR. Thanks. |
JSONbored
left a comment
There was a problem hiding this comment.
@jony376 The implementation direction is right, but the test still does not cover the actual rate-limit identity/enforcement path. Please add a behavioral test proving padded and unpadded Authorization: Bearer ... headers produce the same limiter identity through the rate-limit path itself, not only through extractBearerToken()/hashToken() directly.
JSONbored
left a comment
There was a problem hiding this comment.
@jony376 the implementation direction is right: routing bearer parsing through extractBearerToken() removes the whitespace mismatch in both rateLimitKey() and actorHint().
A few notes:
- The current added test proves trimmed helper output hashes the same.
- The maintainer concern is still the runtime path:
enforceRateLimit()should produce the same rate-limit identity for padded and unpadded bearer headers. - CI is green, so this is now a coverage/behavior proof issue rather than a build issue.
Required changes:
- Add a behavioral test through
enforceRateLimit()usingAuthorization: Bearer session-tokenandAuthorization: Bearer session-token. - Assert they hit the same limiter key/bucket, for example by capturing the Durable Object name/key or by proving the second request is rate-limited when the limit is 1.
Validation expected:
- Re-run
npm run test:coverage. - Keep the auth tests green without weakening existing rate-limit coverage.
|
@jony376 - this was a valid issue, however it was already resolved through a security advisory yesterday, so I'm going to close this. Getting it fixed was more important than waiting for an update on this PR, unfortunately, and the advisory was more extensive in its resolution. A few notes:
|
Summary
Use
extractBearerTokenwhen building rate-limit keys and audit actor hints so bearer credentials match private API authentication.Closes #63
Validation
npm run test:ci(auth unit tests)Safety