infra/tickets

Fork 1

Wrong redirects on elections.fedoraproject.org #12997

New issue

Open

opened 2025-12-18 17:27:15 +00:00 by adamwill · 15 comments

adamwill commented

2025-12-18 17:27:15 +00:00

Member

When logging in or out of elections.fedoraproject.org , you get redirected to https://elections.fedoraproject.org,elections.fedoraproject.org/, which is obviously wrong. It's the same on staging.

I've been poking into this and it's quite weird. One thing I noticed is that if you log out and go to https://elections.stg.fedoraproject.org/vote/32 , you get redirected to https://elections.stg.fedoraproject.org/login?next=https://elections.stg.fedoraproject.org,elections.stg.fedoraproject.org/vote/32. This redirect is, I'm pretty sure, produced by the login_required decorator, and all that does is:

        if not is_authenticated():
            return flask.redirect(
                flask.url_for("oidc_auth.login", next=flask.request.url)
            )

so the wrong next parameter in the URL is coming from flask.request.url. I've no idea how flask.request.url is coming up wrong, but it seems like it's as likely to be a deployment issue as it is a bug in the elections code, so I'm filing it here.

I'll keep poking at it for a bit, but I'm rather baffled right now, can't see any reason it would be like this.

Initially reported by @stefw , thanks.

When logging in or out of elections.fedoraproject.org , you get redirected to `https://elections.fedoraproject.org,elections.fedoraproject.org/`, which is obviously wrong. It's the same on staging. I've been poking into this and it's quite weird. One thing I noticed is that if you log out and go to https://elections.stg.fedoraproject.org/vote/32 , you get redirected to `https://elections.stg.fedoraproject.org/login?next=https://elections.stg.fedoraproject.org,elections.stg.fedoraproject.org/vote/32`. This redirect is, I'm pretty sure, produced by [the login_required decorator](https://pagure.io/elections/blob/2a8d6a2b460ecd42d39b9b61d84339a8c3f21c5f/f/fedora_elections/elections.py#_44), and all that does is: ``` if not is_authenticated(): return flask.redirect( flask.url_for("oidc_auth.login", next=flask.request.url) ) ``` so the wrong `next` parameter in the URL is coming from `flask.request.url`. I've no idea how `flask.request.url` is coming up wrong, but it seems like it's as likely to be a deployment issue as it is a bug in the elections code, so I'm filing it here. I'll keep poking at it for a bit, but I'm rather baffled right now, can't see any reason it would be like this. Initially reported by @stefw , thanks.

adamwill commented

2025-12-18 18:37:57 +00:00

Author

Member

@kevin , who is definitely enjoying his PTO, immediately knew (of course) what this was - it's openshift header forwarding behavior. Setting the haproxy.router.openshift.io/set-forwarded-headers annotation to replace (instead of the default append) seems to fix this.

If that's all there was to it, though, you'd think more stuff would be broken - using flask.request.url or similar is hardly unusual. I do wonder if elections' built-in reverse proxy thing is somehow related here. The openshift doc does specifically mention the "X-Forwarded-For" header, and the elections reverse proxy thingy seems to specifically care about that header, so it kinda adds up. I wonder if this might fix it:

diff --git a/fedora_elections/proxy.py b/fedora_elections/proxy.py
index 8839a58..e4f7bd2 100644
--- a/fedora_elections/proxy.py
+++ b/fedora_elections/proxy.py
@@ -58,7 +58,7 @@ class ReverseProxied(object):
 
         server = environ.get("HTTP_X_FORWARDED_HOST", "")
         if server:
-            environ["HTTP_HOST"] = server
+            environ["HTTP_HOST"] = server.split(",")[-1]
 
         scheme = environ.get("HTTP_X_SCHEME", "")
         if scheme:

or maybe server.split(",")[0], not sure if it's better to take the first value or the last.

@kevin , who is *definitely enjoying his PTO*, immediately knew (of course) what this was - it's openshift [header forwarding behavior](https://docs.redhat.com/en/documentation/openshift_container_platform/4.15/html/networking/configuring-routes). Setting the `haproxy.router.openshift.io/set-forwarded-headers` annotation to `replace` (instead of the default `append`) seems to fix this. If that's all there was to it, though, you'd think *more* stuff would be broken - using `flask.request.url` or similar is hardly unusual. I do wonder if elections' [built-in reverse proxy thing](https://pagure.io/elections/blob/develop/f/fedora_elections/proxy.py) is somehow related here. The openshift doc does specifically mention the "X-Forwarded-For" header, and the elections reverse proxy thingy seems to specifically care about that header, so it kinda adds up. I wonder if this might fix it: ``` diff --git a/fedora_elections/proxy.py b/fedora_elections/proxy.py index 8839a58..e4f7bd2 100644 --- a/fedora_elections/proxy.py +++ b/fedora_elections/proxy.py @@ -58,7 +58,7 @@ class ReverseProxied(object): server = environ.get("HTTP_X_FORWARDED_HOST", "") if server: - environ["HTTP_HOST"] = server + environ["HTTP_HOST"] = server.split(",")[-1] scheme = environ.get("HTTP_X_SCHEME", "") if scheme: ``` or maybe `server.split(",")[0]`, not sure if it's better to take the first value or the last.

adamwill commented

2025-12-18 18:49:17 +00:00

Author

Member

Hmm, or maybe...it looks like the ReverseProxied thing in elections is basically trying to do the same as werkzeug X-Forwarded-For Proxy Fix, which is documented by flask upstream. Maybe we should switch elections to use that, with appropriate configuration?

Hmm, or maybe...it looks like the `ReverseProxied` thing in elections is basically trying to do the same as [werkzeug X-Forwarded-For Proxy Fix](https://werkzeug.palletsprojects.com/en/stable/middleware/proxy_fix/), which is [documented by flask upstream](https://flask.palletsprojects.com/en/stable/deploying/proxy_fix/). Maybe we should switch elections to use that, with appropriate configuration?

adamwill commented

2025-12-18 19:08:36 +00:00

Author

Member

Filed https://pagure.io/elections/issue/106 to propose replacing ReverseProxied.

Filed https://pagure.io/elections/issue/106 to propose replacing `ReverseProxied`.

james commented

2025-12-18 19:44:08 +00:00

Member

From what I can remember, this looks a bit like a problem we had in fedocal that got fixed by...

github.com/fedora-infra/fedocal@3e00d4a425

...not sure if that helps you.

From what I can remember, this looks a bit like a problem we had in fedocal that got fixed by... https://github.com/fedora-infra/fedocal/commit/3e00d4a425efcce6b32c6969b5aa8fc5f7d0315f ...not sure if that helps you.

james commented

2025-12-18 19:44:09 +00:00

Member

Metadata Update from @james:

Issue tagged with: medium-gain, medium-trouble

**Metadata Update from @james**: - Issue tagged with: medium-gain, medium-trouble

adamwill commented

2025-12-18 20:00:27 +00:00

Author

Member

Yeah, that's pretty much exactly what I was suggesting. The tricky points to me are just:

How did you decide on the args x_proto=1, x_host=1 as the correct ones?
Do we make the invocation configurable so we can accommodate other deployments and easily adjust it if our own reverse proxying setup changes?

Yeah, that's pretty much exactly what I was suggesting. The tricky points to me are just: * How did you decide on the args `x_proto=1, x_host=1` as the correct ones? * Do we make the invocation configurable so we can accommodate other deployments and easily adjust it if our own reverse proxying setup changes?

james commented

2025-12-18 20:08:36 +00:00

Member

@abompard Ping on the above questions wrt using werkzeug.middleware.proxy_fix

abompard commented

2025-12-19 14:54:39 +00:00

Member

Yeah we should switch to Werkzeug's implementation instead of our own (manually copied blob from one project to another).

The argument values we set depend on what we know our proxy will set in the headers. I think it sets X-Forwarded-Host so x_host=1 seems correct.

Here's the reference docs.

Yeah we should switch to Werkzeug's implementation instead of our own (manually copied blob from one project to another). The argument values we set depend on what we know our proxy will set in the headers. I think it sets `X-Forwarded-Host` so `x_host=1` seems correct. Here's the [reference docs](https://werkzeug.palletsprojects.com/en/stable/middleware/proxy_fix/).

adamwill commented

2025-12-19 15:57:14 +00:00

Author

Member

Yeah, I read the doc, but I didn't know which headers the proxy sets or how many levels of proxying to consider.

phsmoura commented

2026-01-08 20:15:00 +00:00

Member

Metadata Update from @phsmoura:

Issue priority set to: Waiting on Assignee (was: Needs Review)

**Metadata Update from @phsmoura**: - Issue priority set to: Waiting on Assignee (was: Needs Review)

abompard commented

2026-01-29 11:00:43 +00:00

Member

Hey! Sorry I forgot to respond to this one.

* How did you decide on the args `x_proto=1, x_host=1` as the correct ones?
* Do we make the invocation configurable so we can accommodate other deployments and easily adjust it if our own reverse proxying setup changes?

Trusting only one level of proxying is a safer bet for something that goes in the code, so I went with that. But I also think this proxy-handling stuff should go into a deployment-specific WSGI file that would do the wrapping, because only the deployers know how many proxies to trust. But that's a little more work and I was in a rush to fix it.

For example, Noggin uses this WSGI file in Ansible: https://pagure.io/fedora-infra/ansible/blob/main/f/roles/openshift-apps/noggin/templates/wsgi.py

Hey! Sorry I forgot to respond to this one. > * How did you decide on the args `x_proto=1, x_host=1` as the correct ones? > * Do we make the invocation configurable so we can accommodate other deployments and easily adjust it if our own reverse proxying setup changes? Trusting only one level of proxying is a safer bet for something that goes in the code, so I went with that. But I also think this proxy-handling stuff should go into a deployment-specific WSGI file that would do the wrapping, because only the deployers know how many proxies to trust. But that's a little more work and I was in a rush to fix it. For example, Noggin uses this WSGI file in Ansible: https://pagure.io/fedora-infra/ansible/blob/main/f/roles/openshift-apps/noggin/templates/wsgi.py

kevin commented

2026-03-12 00:03:22 +00:00

Owner

So where are we here?

adamwill commented

2026-03-12 01:23:52 +00:00

Author

Member

Do you want to take it, @abompard , or should I?

adamwill commented

2026-03-27 19:18:28 +00:00

Author

Member

This is also affecting resultsdb. it's probably been the case for a while but didn't cause any obvious effects. but the latest resultsdb container now has werkzeug 3.1.7, which backported this hostname validity check, so now we're choking on it:

[Fri Mar 27 18:56:31.099117 2026] [wsgi:error] [pid 4:tid 34] [remote 10.131.0.2:33352] [2026-03-27 18:56:31,098] ERROR in __init__: Bad request: 400 Bad Request: Host 'resultsdb.stg.fedoraproject.org, resultsdb.stg.fedoraproject.org' is not trusted.

note that werkzeug is seeing the hostname as literally resultsdb.stg.fedoraproject.org, resultsdb.stg.fedoraproject.org and (not surprisingly) considering it invalid.

This is also affecting [resultsdb](https://github.com/release-engineering/resultsdb/blob/develop/resultsdb/proxy.py). it's probably been the case for a while but didn't cause any obvious effects. but the latest resultsdb container now has werkzeug 3.1.7, which backported [this hostname validity check](https://github.com/pallets/werkzeug/pull/3113), so now we're choking on it: ``` [Fri Mar 27 18:56:31.099117 2026] [wsgi:error] [pid 4:tid 34] [remote 10.131.0.2:33352] [2026-03-27 18:56:31,098] ERROR in __init__: Bad request: 400 Bad Request: Host 'resultsdb.stg.fedoraproject.org, resultsdb.stg.fedoraproject.org' is not trusted. ``` note that werkzeug is seeing the hostname as literally `resultsdb.stg.fedoraproject.org, resultsdb.stg.fedoraproject.org` and (not surprisingly) considering it invalid.

adamwill commented

2026-03-27 19:19:28 +00:00

Author

Member

Since this is affecting resultsdb, I particularly think the "this shouldn't be in code" thing is significant. We have no idea how many proxies the Red Hat internal resultsdb instance wants to / needs to trust.