Issue #5224 X-Forwarded-Host support for port

[joakime]

No description provided.

[gregw]

I don't agree with this test. We should be order independent for such headers. If we decide that X-Forwarded-Port takes precedent over any port specified in X-Forwarded-Host then it should do so regardless of the order of those headers!

[joakime]

The code in ForwardedRequestCustomizer is most definitely order dependent, esp around host/port/authority. (IIRC, these are not the only tests that show this order dependency, i'll point out which other ones we have tomm.).
This order dependency was introduced when the code changed to iterating the fields from querying the fields.

If we want to change this code to not be order dependent, then we should go over all of the tests and expectations first, discuss them, then fix the code to conform to the expectations.

[joakime]

I opened Issue #5247, proposing a better way.

[gregw]

Ditto the port here should be 10002... in fact isn't that what the code already does? Wont the code at 👍 https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java#L662
always replace the port of the host header?

[joakime]

IIRC, it resolves as ...

• Host becomes authority
• X-Forwarded-Port changes authority
• X-Forwarded-Host changes host (but not port, as it was set by X-Forwarded-Port) to new authority

So the following headers ...

Host: myhost:10001
X-Forwarded-Port: 10002
X-Forwarded-Host: sub1.example.com:10003

becomes in order ....

1. authority is myhost:10001 (from Host)
2. authority is myhost:10002 (from X-Forwarded-Port)
3. authority is sub1.example.com:10002 (from X-Forwarded-Host)

but the headers in a different order ...

Host: myhost:10001
X-Forwarded-Host: sub1.example.com:10003
X-Forwarded-Port: 10002

Goes thru ...

1. authority is myhost:10001 (from Host)
2. authority is sub1.example.com:10003 (from X-Forwarded-Host)
3. authority is unchanged, and stays at sub1.example.com:10003 because it set "host" and "port" already (the X-Forwarded-Port doesn't change it again)

Lets chat about the test cases tomm, I'll take a stab at fixing them in a more sane way (keeping iteration).
I see authority going thru a state change for both host and port of (UNSET, LAZY, AUTHORITATIVE), where LAZY is only used for Host, but other headers can upgrade the state to AUTHORITATIVE, and then when it's all done parsing the final server/host authority is calculated. (and not using the overly complicated extended HostPort classes that have confusing meanings with no javadoc explaining them)

[gregw]

@joakime the code has been explicitly designed to be order independent for host and port. That is why we have PortSetHostPort and PossiblyPartialHostPort classes to record if a port or a host has been set before another header arrives that sets the other one.

So specifically the outcome for:

X-Forwarded-Host: sub1.example.com:10003
X-Forwarded-Port: 10002

should be no different from the outcome for:

X-Forwarded-Port: 10002
X-Forwarded-Host: sub1.example.com:10003

My preference is for the explicit port header to always win, so the result will be sub1.example.com:1002 in both cases. Let me find some time to play with your branch to see how your tests are even passing, then we can chat about it.

[gregw]

OK part of the problem is here:

        public void handleFor(HttpField field)
        {
            String authority = getLeftMost(field.getValue());
            if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader()))
            {
                if (_for == null)
                    _for = new PossiblyPartialHostPort(authority);
                else if (_for instanceof PortSetHostPort)
                    _for = new HostPort(HostPort.normalizeHost(authority), _for.getPort());
            }
            else if (_for == null)
            {
                _for = new HostPort(authority);
            }
        }

This code is not checking if in the case a PortSetHostPort exists if the For header also has a port. Thus in this case we are giving precedence to the port header.

But the code for the port header does the opposite:

        public void handlePort(HttpField field)
        {
            int port = HostPort.parsePort(getLeftMost(field.getValue()));
            if (!getForwardedPortAsAuthority())
            {
                if (_for == null)
                    _for = new PortSetHostPort(_request.getRemoteHost(), port);
                else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0)
                    _for = new HostPort(HostPort.normalizeHost(_for.getHost()), port);
            }
            else
            {
                if (_host == null)
                    _host = new PortSetHostPort(_request.getServerName(), port);
                else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0)
                    _host = new HostPort(HostPort.normalizeHost(_host.getHost()), port);
            }
        }

It is checking if there is actually a port set in the PossiblyPartiallyHostPort and if so it gives it precedence and ignores the port header.

So you need to make the same policy decision in both places - either the port header has precedence or it doesn't - I guess I don't mind either way, we just need to be consistent.

[joakime]

I opened Issue #5247, proposing a better way.