🐛(y-provider) send X-Forwarded-Proto on internal backend calls - #2542
🐛(y-provider) send X-Forwarded-Proto on internal backend calls#2542lucletoffe wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe y-provider’s internal backend requests now always include Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
🤖 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 `@src/frontend/servers/y-provider/src/api/collaborationBackend.ts`:
- Line 74: Validate the X-Forwarded-Proto value used in the requestHeaders
mapping before forwarding it to Django. In the collaboration backend request
construction, only preserve the inbound value when it is exactly “http” or
“https”; otherwise use the existing “https” fallback, preventing arbitrary
protocol values from reaching Django.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65139690-e0aa-4d4b-be1b-38c1e54c62e4
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/servers/y-provider/__tests__/collaborationBackend.test.tssrc/frontend/servers/y-provider/src/api/collaborationBackend.ts
|
Good catch, fixed in e6f7692: the value is now restricted to |
e6f7692 to
70c9249
Compare
|
Correction to my earlier comment — the branch was squashed into a single commit after I wrote it, and the approach changed with it.
Why the change. An ingress configured with Since Apologies for the noise — the earlier comment describes code that is no longer in the PR. |
70c9249 to
1032886
Compare
The backend hardcodes SECURE_SSL_REDIRECT in its Production settings class
and relies on SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
to know that a TLS termination sits in front of it.
When COLLABORATION_BACKEND_BASE_URL points at the backend directly, for
instance over an internal docker network, the y-provider call never crosses
the reverse proxy, so nothing sets that header. Django answers a 301 to
https://<host>:8000 and the client then speaks TLS to a port serving plain
HTTP. Every onConnect fails with "[onConnect] Backend error: Unauthorized"
and collaborative editing never starts, whatever the credentials.
Measured on a self-hosted 5.4.1 deployment, from inside the y-provider
container:
wget http://docs-backend:8000/api/v1.0/config/
-> 301 Location: https://docs-backend:8000/...
-> SSL routines:tls_validate_record_header:wrong version number
the same call with "X-Forwarded-Proto: https" -> 200 OK
Rather than relaying the client's own X-Forwarded-Proto, always send https.
An ingress configured with use-forwarded-headers: true (required when a
load-balancer sits upstream) passes the client's header through unchanged. A
client that sends X-Forwarded-Proto: http on its WebSocket upgrade would then
cause Django to answer a 301, axios would follow the redirect to a plain-HTTP
port, and the connection would break — a vector this fix has no reason to
introduce. Since SECURE_SSL_REDIRECT is enabled in the Production class,
https is the safe default given the current Production settings.
Fixes suitenumerique#2541
Signed-off-by: lucletoffe <15689941+lucletoffe@users.noreply.github.com>
1032886 to
b1affa7
Compare
|
The workflow runs are waiting on maintainer approval ( |
2 participants
Purpose
Fixes #2541.
On a self-hosted deployment where
COLLABORATION_BACKEND_BASE_URLpoints at the backend directly (internal docker network, e.g.http://docs-backend:8000), collaboration can never connect. EveryonConnectfails with[onConnect] Backend error: Unauthorized, whatever the credentials.The cause is not authentication, it is
SECURE_SSL_REDIRECT:Production.SECURE_SSL_REDIRECT = Trueis hardcoded insrc/backend/impress/settings.pyand cannot be driven by an environment variable.SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https").301tohttps://<host>:8000, and the client follows it — speaking TLS to a port that serves plain HTTP (wrong version number).Reproduced from inside the y-provider container (v5.4.1):
The backend log showed 46 ×
301 Moved Permanentlyon/api/v1.0/documents/<id>/within the last 200 lines, all from the y-provider container IP.Proposal
collaborationBackend.tsalways sendsX-Forwarded-Proto: https, next to thecookieandoriginheaders it already relays.httpsis hardcoded rather than relayed from the incoming request. An ingress configured withuse-forwarded-headers: true— required as soon as a load-balancer sits upstream — passes the client's own header through unchanged. A client that sendsX-Forwarded-Proto: httpon its WebSocket upgrade would therefore cause Django to answer a 301, axios would follow the redirect to a plain-HTTP port, and the connection would break. That is a vector this fix has no reason to introduce. SinceSECURE_PROXY_SSL_HEADERis only declared in theProductionclass whereSECURE_SSL_REDIRECT = Truealready makes a plain-HTTP deployment unreachable,httpsis the safe default given the current Production settings.Test suite updated: the two cases that tested relay behaviour are removed; one test checks that
X-Forwarded-Proto: httpsis always sent to the backend even when the incoming request carriesX-Forwarded-Proto: http.Changelog entry.
Ran locally on this branch:
yarn COLLABORATION_SERVER run test(6 files, 56 tests passing),run lint,run build.Why not fix this on the backend instead?
A maintainer might reasonably suggest extending
SECURE_REDIRECT_EXEMPT(src/backend/impress/settings.py) with the internal API prefixes called by the y-provider, rather than touching the y-provider itself. That approach is legitimate and would fix the problem at its source in Python.The trade-off is maintenance surface: every internal path that could be called before the reverse proxy sets the header would need to be listed and kept in sync as the API evolves. The y-provider fix applies unconditionally for any topology where
COLLABORATION_BACKEND_BASE_URLpoints at the backend directly.It is worth noting that the topology recommended by the official Helm chart (
documentation/examples/helm/impress.values.yaml, whereCOLLABORATION_BACKEND_BASE_URLis set to the public HTTPS URL) does not trigger this bug at all — the call goes through the ingress, which sets the header correctly. Routing an in-cluster call out through the public ingress and back in adds unnecessary latency and a dependency on external DNS resolution from inside the cluster; the direct-backend URL is the natural choice for in-cluster deployments, and this fix makes it work without requiring any backend change.Alternative considered
Making
SECURE_SSL_REDIRECTconfigurable through an environment variable would also work, but it weakens a security default for every deployment in order to fix an internal call. Sendinghttpsunconditionally keeps the production posture untouched.External contributions
General requirements
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements