[fix] Resolve API trailing slashes in place instead of redirecting - #5487
Conversation
…b app ApiPrefixStripMiddleware rewrites scope["path"] before routing, so Starlette builds its trailing-slash redirect from the stripped path. Behind a path-prefixed proxy the 307 for /api/<route> (no trailing slash) points at /<route>/, which is served by the web UI — the caller gets an HTML 404 instead of the API response. Re-prefix 307/308 Locations on the way out, only when this middleware actually stripped, and only for root-relative paths so off-host redirects (SuperTokens callback) are untouched.
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Agenta Team seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe API prefix middleware now strips repeated ChangesAPI Prefix and Trailing-Slash Normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiPrefixStripMiddleware
participant Routes
participant Application
Client->>ApiPrefixStripMiddleware: HTTP or WebSocket request
ApiPrefixStripMiddleware->>ApiPrefixStripMiddleware: Strip repeated /api prefixes
ApiPrefixStripMiddleware->>Routes: Check exact and slash-variant paths
Routes-->>ApiPrefixStripMiddleware: Return route match result
ApiPrefixStripMiddleware->>Application: Forward normalized scope
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/oss/src/middlewares/prefix.py (1)
33-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
API_PREFIXfor raw-path rewriting too.The
pathlogic usesAPI_PREFIX, but Lines [37-39] still hardcodeb"/api"and its length. If the shared constant changes,scope["path"]andscope["raw_path"]can diverge. Derive the byte prefix once and use it for both checks and slicing.Proposed fix
API_PREFIX = "/api" +_API_PREFIX_BYTES = API_PREFIX.encode("ascii") - raw[: len(API_PREFIX)] == b"/api" + raw[: len(_API_PREFIX_BYTES)] == _API_PREFIX_BYTES ... - raw = bytes(raw)[len(API_PREFIX) :] or b"/" + raw = bytes(raw)[len(_API_PREFIX_BYTES) :] or b"/"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97ca8bb4-9813-4f3f-80b8-879bbf0b6bc1
📒 Files selected for processing (1)
api/oss/src/middlewares/prefix.py
Railway Preview Environment
Updated at 2026-07-29T12:48:49.989Z |
|
Marking this draft. Verified in a real browser (fresh profile) that the fix behaves correctly: unslashed does one hop to slashed and returns 200. But deploying it to a self-hosted box surfaced a failure mode I missed. If a client already holds a cached 308 that rewrites the slashed URL back to the unslashed one, this fix closes the circle into Trading a silent 404 for a redirect loop is not an improvement. The better shape is to stop redirecting at all on these routes: answer the unslashed path directly (internal rewrite, no Location header). That fixes poisoned clients without them clearing anything, and removes the loop by construction. Reworking along those lines. |
Correcting the redirect's Location was the wrong shape. A client holding a cached slash-normalizing 308 gets a redirect loop: its cache rewrites slashed to unslashed, the corrected Location sends unslashed back to slashed. Observed on a self-hosted deployment as ERR_TOO_MANY_REDIRECTS. Match the trailing slash against the route table on the way in and disable redirect_slashes, so both shapes resolve directly and no Location is ever emitted. That removes the escape and the loop together.
mmabrouk
changed the title
|
Reworked. The earlier approach (correcting the redirect's Location) is replaced: it produced ERR_TOO_MANY_REDIRECTS against a client holding a cached slash-normalizing 308, which is worse than the silent 404 it replaced. This version resolves the trailing slash against the route table on the way in and disables redirect_slashes, so no Location is emitted at all and already-poisoned clients recover without clearing anything. Verified in headless Chrome against an isolated stack, not curl. Description rewritten. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d43165e6-10ea-4780-a38f-e5678da19e0f
📒 Files selected for processing (2)
api/entrypoints/routers.pyapi/oss/src/middlewares/prefix.py
The prefix strip and the trailing-slash alternate-form lookup both
mutated `path` but tried to mirror that onto `raw_path` with fragile
byte-level heuristics: a literal `b"/api"` check that a percent-encoded
prefix (`/%61pi/...`) never matches, and an ascii-encode("ignore") that
silently drops non-ASCII bytes. Either case left raw_path stale or
corrupted relative to the routed path.
Re-derive raw_path from the final normalized path instead of tracking
it in parallel, so the two can never diverge.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Context
Behind a path-prefixed proxy, an API request that omits its trailing slash never reaches the API. It gets bounced into the web UI and comes back as an HTML 404. Captured in a real browser against a self-hosted deployment:
ApiPrefixStripMiddlewarerewritesscope["path"]before routing, so Starlette builds itsredirect_slashesresponse from the stripped path and the prefix disappears from theLocation.The 308 in that chain is the expensive part. Chrome caches it permanently, so from then on the browser rewrites the slashed URL to the unslashed one before it hits the network. Every subsequent correct request from the app gets rewritten into a broken one, and the escape mints the 308 again, so the cache re-poisons itself. On the affected deployment this silently degraded the playground's model drawer to its pre-catalog layout, which reads as a stale frontend build. It cost hours to trace.
Some deployments carry hand-written no-slash route aliases for the tools catalog. They are the same bug, patched one route at a time.
Changes
The middleware now resolves the trailing slash against the route table on the way in, and
redirect_slashesis disabled in the composition root. Both URL shapes route directly to the handler and noLocationheader is emitted at all.Correcting the
Locationinstead was the obvious fix and it is wrong. Any client already holding that cached 308 gets a loop: its cache sends slashed to unslashed, the corrected redirect sends unslashed back to slashed. That was tried first and producedERR_TOO_MANY_REDIRECTSon a live deployment. Resolving the path in place removes the escape and the loop together, and it repairs already-poisoned clients without asking them to clear anything, because the URL their cache produces now returns 200.A method mismatch still answers 405 rather than being hidden behind a slash rewrite, and unknown paths still 404.
Tests / notes
Deployed to an isolated stack (separate compose project, own database, port 8390) behind the same Traefik and gunicorn/uvicorn config, and driven with headless Chrome rather than curl. curl cannot see this class of bug: it has no redirect cache.
Browser, unslashed URL, patched vs unpatched:
Regression sweep on the patched stack, all passing:
/api/health, both slash forms of the harness catalog, the double-prefixed/api/api/...path, both slash forms of a path-param route,POSTto aGETroute returning 405, an unknown path returning 404, and the tools catalog without its trailing slash. The API emitted zero 3xx responses across the whole sweep. No import or startup errors.Two things for a reviewer to weigh:
redirect_slashesis disabled app-wide. Route matching now covers what it did, minus the redirect, so a path that previously redirected resolves directly. A path neither form matches still 404s, as before.Not addressed here: the frontend degrades silently when a catalog request fails, which is what made this hard to diagnose. That is #5492.
What to QA
/api/workflows/catalog/harnesses. It returns JSON with no redirect, rather than an HTML page.GET-only route withPOST. It still returns 405, not 404.