Skip to content
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.

Sign up for GitHub

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

Issue #5224 X-Forwarded-Host support for port #5226

Merged
merged 3 commits into from
Sep 9, 2020
Merged

Issue #5224 X-Forwarded-Host support for port #5226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4ca153
Issue #5224 - Test to replicate reported issue
joakime Sep 2, 2020
e2134b1
Issue #5224 - X-Forwarded-Host support for port
joakime Sep 2, 2020
2896ed3
Issue #5224 - Updating ForwardedRequestCustomizerTest expectations
joakime Sep 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,16 +598,18 @@ public void handleSslSessionId(HttpField field)
@SuppressWarnings("unused")
public void handleHost(HttpField field)
{
HostPort hostField = new HostPort(getLeftMost(field.getValue()));

if (getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader()))
{
if (_host == null)
_host = new PossiblyPartialHostPort(getLeftMost(field.getValue()));
_host = new PossiblyPartialHostPort(hostField.getHost(), hostField.getPort());
else if (_host instanceof PortSetHostPort)
_host = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _host.getPort());
_host = new HostPort(hostField.getHost(), hostField.getPort() > 0 ? hostField.getPort() : _host.getPort());
}
else if (_host == null)
{
_host = new HostPort(getLeftMost(field.getValue()));
_host = hostField;
}
}

Expand Down
64 changes: 61 additions & 3 deletions jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,67 @@ public static Stream<Arguments> cases()
.requestURL("http://fw.example.com:4333/")
.remoteAddr("8.5.4.3").remotePort(2222)
),

Arguments.of(new Request("X-Forwarded-* (Multiple Ports)")
.headers(
"GET / HTTP/1.1",
"Host: myhost:10001",
"X-Forwarded-For: 127.0.0.1:8888,127.0.0.2:9999",
"X-Forwarded-Port: 10002",
"X-Forwarded-Proto: https",
"X-Forwarded-Host: sub1.example.com:10003",
"X-Forwarded-Server: sub2.example.com"
),
new Expectations()
.scheme("https").serverName("sub1.example.com").serverPort(10003)
.requestURL("https://sub1.example.com:10003/")
.remoteAddr("127.0.0.1").remotePort(8888)
),
Arguments.of(new Request("X-Forwarded-* (Multiple Ports - Server First)")
.headers(
"GET / HTTP/1.1",
"X-Forwarded-Server: sub2.example.com:10007",
"Host: myhost:10001",
"X-Forwarded-For: 127.0.0.1:8888,127.0.0.2:9999",
"X-Forwarded-Proto: https",
"X-Forwarded-Port: 10002",
"X-Forwarded-Host: sub1.example.com:10003"
),
new Expectations()
.scheme("https").serverName("sub1.example.com").serverPort(10003)
.requestURL("https://sub1.example.com:10003/")
.remoteAddr("127.0.0.1").remotePort(8888)
),
Arguments.of(new Request("X-Forwarded-* (Multiple Ports - setForwardedPortAsAuthority = false)")
.configureCustomizer((customizer) -> customizer.setForwardedPortAsAuthority(false))
.headers(
"GET / HTTP/1.1",
"Host: myhost:10001",
"X-Forwarded-For: 127.0.0.1:8888,127.0.0.2:9999",
"X-Forwarded-Port: 10002",
"X-Forwarded-Proto: https",
"X-Forwarded-Host: sub1.example.com:10003",
"X-Forwarded-Server: sub2.example.com"
),
new Expectations()
.scheme("https").serverName("sub1.example.com").serverPort(10003)
.requestURL("https://sub1.example.com:10003/")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened Issue #5247, proposing a better way.

.remoteAddr("127.0.0.1").remotePort(8888)
),
Arguments.of(new Request("X-Forwarded-* (Multiple Ports Alt Order)")
.headers(
"GET / HTTP/1.1",
"Host: myhost:10001",
"X-Forwarded-For: 127.0.0.1:8888,127.0.0.2:9999",
"X-Forwarded-Proto: https",
"X-Forwarded-Host: sub1.example.com:10003",
"X-Forwarded-Port: 10002",
"X-Forwarded-Server: sub2.example.com"
),
new Expectations()
.scheme("https").serverName("sub1.example.com").serverPort(10003)
.requestURL("https://sub1.example.com:10003/")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened Issue #5247, proposing a better way.

.remoteAddr("127.0.0.1").remotePort(8888)
),
// =================================================================
// Mixed Behavior
Arguments.of(new Request("RFC7239 mixed with X-Forwarded-* headers")
Expand Down Expand Up @@ -585,7 +645,6 @@ public static Stream<Arguments> cases()

@ParameterizedTest(name = "{0}")
@MethodSource("cases")
@SuppressWarnings("unused")
public void testDefaultBehavior(Request request, Expectations expectations) throws Exception
{
request.configure(customizer);
Expand All @@ -601,7 +660,6 @@ public void testDefaultBehavior(Request request, Expectations expectations) thro

@ParameterizedTest(name = "{0}")
@MethodSource("cases")
@SuppressWarnings("unused")
public void testConfiguredBehavior(Request request, Expectations expectations) throws Exception
{
request.configure(customizerConfigured);
Expand Down