fix: preserve HTTPS scheme in trailing-slash redirects - #3204
fix: preserve HTTPS scheme in trailing-slash redirects#3204Nicknamess96 wants to merge 1 commit into
Conversation
Enable uvicorn's proxy header support so that X-Forwarded-Proto from reverse proxies (Cloudflare Tunnels, nginx, Traefik, etc.) is respected when constructing redirect Location headers. Without this, trailing-slash redirects downgrade HTTPS to HTTP, causing clients that follow the Fetch spec to strip Authorization headers on the redirect. - Add proxy_headers=True and forwarded_allow_ips="*" to both uvicorn.run call sites (HTTP and LOCAL_HTTPS modes) - Add tests verifying redirect scheme preservation with ProxyHeadersMiddleware - Add tests verifying uvicorn.run receives correct proxy parameters Fixes letta-ai#3189
|
Friendly ping — this is a one-line fix to preserve the HTTPS scheme when the trailing-slash redirect middleware rewrites URLs. Happy to address any feedback. |
alvinttang
left a comment
There was a problem hiding this comment.
Code Review
Security: forwarded_allow_ips="*" trusts all proxy headers unconditionally
File: letta/server/rest_api/app.py, both uvicorn.run() call sites
proxy_headers=True,
forwarded_allow_ips="*",Setting forwarded_allow_ips="*" means any client can set X-Forwarded-Proto, X-Forwarded-For, etc., and uvicorn will trust them. This is a well-known security anti-pattern:
- IP spoofing: An attacker can set
X-Forwarded-For: 127.0.0.1to bypass IP-based access controls or rate limiting that relies on the client IP from the ASGI scope. - Scheme spoofing: An attacker can set
X-Forwarded-Proto: httpsto trick the application into thinking it's behind HTTPS when it isn't, potentially bypassing secure-cookie checks.
For deployments behind a known reverse proxy, forwarded_allow_ips should be set to the proxy's IP address(es) or subnet, not "*". If the deployment topology varies, this should at minimum be configurable via an environment variable with a secure default:
forwarded_allow_ips=os.environ.get("LETTA_FORWARDED_ALLOW_IPS", "127.0.0.1"),This way the default is secure (only trust localhost as a proxy), and operators behind Cloudflare/nginx can explicitly opt in to broader trust.
Minor: Tests don't test the actual app.py middleware chain
The tests in test_proxy_headers.py build a separate minimal FastAPI app and manually wrap it with ProxyHeadersMiddleware. This validates that uvicorn's middleware works correctly (which is uvicorn's responsibility), but doesn't test that the actual Letta application's middleware stack, route definitions, and startup sequence correctly integrate the proxy headers configuration. Consider adding an integration test that imports the real create_app() and verifies the middleware is present.
themavik
left a comment
There was a problem hiding this comment.
Enabling forwarded_allow_ips="*" means any client that can reach the socket can forge X-Forwarded-Proto/Host; fine if Letta only listens on loopback or behind a locked-down proxy hop, but worth calling out in docs or tightening to known proxy CIDRs for public binds.
Summary
Enable uvicorn's proxy header support so that
X-Forwarded-Protofrom reverse proxies is respected when constructing trailing-slash redirectLocationheaders, preventing unintended HTTPS-to-HTTP protocol downgrades.Problem
When Letta is deployed behind an HTTPS-terminating reverse proxy (Cloudflare Tunnels, nginx, Traefik, AWS ALB, etc.), endpoints that trigger trailing-slash redirects generate
Location: http://...URLs instead ofLocation: https://.... Per the Fetch specification, HTTP clients stripAuthorizationheaders on HTTPS-to-HTTP redirects, resulting in401 Unauthorizederrors.Root cause:
uvicorn.run()instart_server()is called withoutproxy_headers=True, so Starlette doesn't readX-Forwarded-Protowhen building redirect URLs.Changes
proxy_headers=Trueandforwarded_allow_ips="*"to bothuvicorn.run()call sites (HTTP andLOCAL_HTTPSmodes) inletta/server/rest_api/app.pyProxyHeadersMiddlewareuvicorn.run()receives the correct proxy parametersTesting
tests/test_proxy_headers.py, all passingruff format(line length 140)ruff check(all checks passed)Fixes #3189