fix: Middleware route checks do not match routing-equivalent path variants (trailing slash, case)#10501
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds exported matchesExactRoute(path, route) and replaces strict path equality checks in middlewares.js and src/batch.js; includes tests ensuring rate-limit windows are shared across trailing-slash and casing variants for /login and /sessions/me, and adds revocable-upgrade URL-variant tests. ChangesRoute normalization for exact static route matching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.09][ERROR]: Error: exception Unix_error: No such file or directory stat spec/RevocableSessionsUpgrade.spec.js 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 |
…lash or case path variants
mtrezza
force-pushed
the
fix/rate-limit-login-path-variants
branch
from
f5421c5 to
c5349e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10501 +/- ##
==========================================
- Coverage 92.62% 92.61% -0.01%
==========================================
Files 193 193
Lines 16941 16947 +6
Branches 240 240
==========================================
+ Hits 15691 15696 +5
- Misses 1227 1228 +1
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middlewares.js (1)
323-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the exact-route helper to
/upgradeToRevocableSessionas well.The new
/sessions/meguard is normalized, but the legacy-token branch at Line 330 still usesreq.url === '/upgradeToRevocableSession'./upgradeToRevocableSession/or case variants will hit the same handler yet skipgetAuthForLegacySessionToken, so legacy-session upgrades can still diverge on routing-equivalent paths.♻️ Proposed fix
- if ( - info.sessionToken && - req.url === '/upgradeToRevocableSession' && - info.sessionToken.indexOf('r:') != 0 - ) { + if ( + info.sessionToken && + matchesExactRoute(req.path, '/upgradeToRevocableSession') && + info.sessionToken.indexOf('r:') != 0 + ) {Based on the PR context describing Express-equivalent path variants and the adjacent
handleParseSessionlogic.🤖 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 `@src/middlewares.js` around lines 323 - 324, Replace the strict string comparison req.url === '/upgradeToRevocableSession' with the normalized helper matchesExactRoute(req.path, '/upgradeToRevocableSession') wherever legacy-token logic branches (the branch that calls getAuthForLegacySessionToken) so path variants like trailing slashes or casing are treated equivalently; update the condition that gates calling getAuthForLegacySessionToken to use matchesExactRoute(req.path, '/upgradeToRevocableSession') (and preserve any method checks) so legacy-session upgrade requests follow the same normalized routing as the /sessions/me guard.
🤖 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.
Outside diff comments:
In `@src/middlewares.js`:
- Around line 323-324: Replace the strict string comparison req.url ===
'/upgradeToRevocableSession' with the normalized helper
matchesExactRoute(req.path, '/upgradeToRevocableSession') wherever legacy-token
logic branches (the branch that calls getAuthForLegacySessionToken) so path
variants like trailing slashes or casing are treated equivalently; update the
condition that gates calling getAuthForLegacySessionToken to use
matchesExactRoute(req.path, '/upgradeToRevocableSession') (and preserve any
method checks) so legacy-session upgrade requests follow the same normalized
routing as the /sessions/me guard.
…sion legacy branch
mtrezza
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/RevocableSessionsUpgrade.spec.js (1)
73-73: ⚡ Quick winStrengthen success assertions in both new tests.
Both lines 73 and 88 use
expect(response.status).not.toBe(400), which would pass for any non-400 status, including other error codes (404, 500, etc.). Explicitly assert the success status code (200) for clarity and to catch unexpected failures.✅ Proposed fix
Apply to both lines 73 and 88:
- expect(response.status).not.toBe(400); + expect(response.status).toBe(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 `@spec/RevocableSessionsUpgrade.spec.js` at line 73, Replace the weak negative assertion expect(response.status).not.toBe(400) with an explicit success assertion expect(response.status).toBe(200) in both new tests; locate the two occurrences of response.status in the spec (the expectations using expect(...) with .not.toBe(400)) and change them to use .toBe(200) so failures like 404/500 are caught.
🤖 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.
Inline comments:
In `@spec/RevocableSessionsUpgrade.spec.js`:
- Around line 78-91: The existing test verifies that query strings are ignored
by route matching (handles '/upgradeToRevocableSession?foo=bar') but to extend
coverage add a sibling test in RevocableSessionsUpgrade.spec.js that asserts
case-insensitive route matching: call the same request flow used in the current
test but target a differently-cased path (e.g.
'/UpgradeToRevocableSession?foo=bar'), reuse the same headers and assertions
(response.status not 400, response.data.sessionToken defined and starts with
'r:'), and mention handleParseSession/matchesExactRoute in the test comment so
future readers know this is exercising case-insensitive matching.
---
Nitpick comments:
In `@spec/RevocableSessionsUpgrade.spec.js`:
- Line 73: Replace the weak negative assertion
expect(response.status).not.toBe(400) with an explicit success assertion
expect(response.status).toBe(200) in both new tests; locate the two occurrences
of response.status in the spec (the expectations using expect(...) with
.not.toBe(400)) and change them to use .toBe(200) so failures like 404/500 are
caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e9b4a0e-4c32-424c-af68-c8f4d86b7bc5
📒 Files selected for processing (2)
spec/RevocableSessionsUpgrade.spec.jssrc/middlewares.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/middlewares.js
## [9.9.1-alpha.10](9.9.1-alpha.9...9.9.1-alpha.10) (2026-06-12) ### Bug Fixes * Middleware route checks do not match routing-equivalent path variants (trailing slash, case) ([#10501](#10501)) ([f861210](f861210))
|
🎉 This change has been released in version 9.9.1-alpha.10 |
2 participants
Issue
Express routes case-insensitively and tolerates a trailing slash by default, so variants like
/login/,/LOGIN,/sessions/me/, and/upgradeToRevocableSession/reach the same handlers as their canonical paths. Several middleware checks compared the request path against a route with strict string equality (and some still readreq.url, which includes the query string), so they did not recognize these routing-equivalent variants and diverged from how the request is actually dispatched:/login— the inbound session token was not dropped on variant paths, so asession/user-zone rate limit could be keyed by the token or user id instead of the client IP, splitting or evading the configured limit (also in/batchsub-requests)./sessions/me— theGETfast-path was skipped on variant paths, splitting auser-zone rate-limit window./upgradeToRevocableSession— the legacy-token branch was skipped on variant paths, so a legacy session token was sent to the revocable-session lookup and the upgrade failed.Approach
Added a shared
matchesExactRoute(path, route)helper that matches a path the same way the Express router and the rate limiter do, viapath-to-regexp(compiled once per route and cached), and used it at the affected checks instead of strict string comparison.Tasks
Summary by CodeRabbit
Bug Fixes
/login,/login/,/LOGIN, and/sessions/mevariants share the same limiter behavior.Tests