-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
middleware: Detect reverse proxy misconfigurations. #26046
Conversation
b1271f7
to
ab3c02b
Compare
ab3c02b
to
1cde046
Compare
This seems to reliably wedge the |
Oh, it apparently segfaults Python:
|
CZO thread for the crash. |
Previously, `X-Forwarded-Proto` did not need to be set, and failure to set `loadbalancer.ips` would merely result in bad IP-address rate-limiting and incorrect access logs; after 0935d38, however, failure to do either of those, if Zulip is deployed with `http_only`, will lead to infinite redirect loops after login. These are accompanied by a misleading error, from Tornado, of: Forbidden (Origin checking failed - https://zulip.example.com does not match any trusted origins.): /json/events This is most common with Docker deployments, where deployments use another docker container, such as nginx or Traefik, to do SSL termination. See zulip/docker-zulip#403. Update the documentation to reinforce that `loadbalancer.ips` also controls trust of `X-Forwarded-Proto`, and that failure to set it will cause the application to not function correctly.
8722517
to
1b73158
Compare
This lgtm; posted one small comment on readability. Given the intent to immediately backport this, it'd be great if @andersk did a read as well; I am not very good at reading the |
1b73158
to
c9586d0
Compare
Having exactly 17 or 18 middlewares, on Python 3.11.0 and above, causes python to segfault when running tests with coverage; see python/cpython#106092 Work around this by adding one or two no-op middlewares if we would hit those unlucky numbers. We only add them in testing, since coverage is a requirement to trigger it, and there is no reason to burden production with additional wrapping.
Combine nginx and Django middlware to stop putting misleading warnings about `CSRF_TRUSTED_ORIGINS` when the issue is untrusted proxies. This attempts to, in the error logs, diagnose and suggest next steps to fix common proxy misconfigurations. See also zulip#24599 and zulip/docker-zulip#403.
c9586d0
to
74f6a47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven’t tested this, but it looks reasonable to me.
Merged, thanks @alexmv! |
Combine nginx and Django middlware to stop putting misleading warnings
about
CSRF_TRUSTED_ORIGINS
when the issue is untrusted proxies.This attempts to, in the error logs, diagnose and suggest next steps
to fix common proxy misconfigurations.
See also #24599 and zulip/docker-zulip#403.
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: