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

Jump to bottom

ForwardedRequestCustomizer behavior should not be applied to requests without forwarding headers #5445

Merged
merged 7 commits into from
Oct 13, 2020
Merged

ForwardedRequestCustomizer behavior should not be applied to requests without forwarding headers #5445

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b646ee
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
f0681b3
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
abdada0
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
ea11030
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
457025b
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
0721178
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
89dc16a
Issue #5443 - Forwarding Headers are optional
joakime Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
113 changes: 60 additions & 53 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 @@ -461,81 +461,88 @@ public void customize(Connector connector, HttpConfiguration config, Request req

// Do a single pass through the header fields as it is a more efficient single iteration.
Forwarded forwarded = new Forwarded(request, config);
boolean match = false;
for (HttpField field : httpFields)
{
try
{
MethodHandle handle = _handles.get(field.getName());
if (handle != null)
{
match = true;
handle.invoke(forwarded, field);
}
}
catch (Throwable t)
{
onError(field, t);
}
}

String proto = "http";

// Is secure status configured from headers?
if (forwarded.isSecure())
if (match)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
// set default to https
proto = config.getSecureScheme();
}
String proto = "http";

// Set Scheme from configured protocol
if (forwarded._proto != null)
{
proto = forwarded._proto;
request.setScheme(proto);
}
// Is secure status configured from headers?
if (forwarded.isSecure())
{
// set default to https
proto = config.getSecureScheme();
}

// Set authority
String host = null;
int port = -1;
// Set Scheme from configured protocol
if (forwarded._proto != null)
{
proto = forwarded._proto;
request.setScheme(proto);
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

// Use authority from headers, if configured.
if (forwarded._authority != null)
{
host = forwarded._authority._host;
port = forwarded._authority._port;
}
// Set authority
String host = null;
int port = -1;

// Fall back to request metadata if needed.
HttpURI requestURI = request.getMetaData().getURI();
if (host == null)
{
host = requestURI.getHost();
}
if (port == MutableHostPort.UNSET) // is unset by headers
{
port = requestURI.getPort();
}
// Don't change port if port == IMPLIED.
// Use authority from headers, if configured.
if (forwarded._authority != null)
{
host = forwarded._authority._host;
port = forwarded._authority._port;
}

// Update authority if different from metadata
if (!host.equalsIgnoreCase(requestURI.getHost()) ||
port != requestURI.getPort())
{
httpFields.put(new HostPortHttpField(host, port));
request.setAuthority(host, port);
}
// Fall back to request metadata if needed.
HttpURI requestURI = request.getMetaData().getURI();
if (host == null)
{
host = requestURI.getHost();
}
if (port == MutableHostPort.UNSET) // is unset by headers
{
port = requestURI.getPort();
}
// Don't change port if port == IMPLIED.

// Set secure status
if (forwarded.isSecure() ||
proto.equalsIgnoreCase(config.getSecureScheme()) ||
port == getSecurePort(config))
{
request.setSecure(true);
request.setScheme(proto);
}
// Update authority if different from metadata
if (!host.equalsIgnoreCase(requestURI.getHost()) ||
port != requestURI.getPort())
{
httpFields.put(new HostPortHttpField(host, port));
request.setAuthority(host, port);
}

// Set Remote Address
if (forwarded.hasFor())
{
int forPort = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort();
request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, forPort));
// Set secure status
if (forwarded.isSecure() ||
proto.equalsIgnoreCase(config.getSecureScheme()) ||
port == getSecurePort(config))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm don't think that just because it arrives on the secure port is sufficient to make it secure. Some ports might do both secure and insecure requests.

{
request.setSecure(true);
request.setScheme(proto);
}

// Set Remote Address
if (forwarded.hasFor())
{
int forPort = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort();
request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, forPort));
}
}
}

Expand Down
28 changes: 24 additions & 4 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 @@ -135,6 +135,26 @@ public void destroy() throws Exception
public static Stream<Arguments> cases()
{
return Stream.of(
// HTTP 1.0
Arguments.of(
new Request("HTTP/1.0 - no Host header")
.headers(
"GET /example HTTP/1.0"
),
new Expectations()
.scheme("http").serverName("0.0.0.0").serverPort(80)
.requestURL("http://0.0.0.0/example")
),
Arguments.of(
new Request("HTTP/1.0 - Empty Host header")
.headers(
"GET /example HTTP/1.0",
"Host:"
),
new Expectations()
.scheme("http").serverName("0.0.0.0").serverPort(80)
.requestURL("http://0.0.0.0/example")
),
// Host IPv4
Arguments.of(
new Request("IPv4 Host Only")
Expand All @@ -157,12 +177,12 @@ public static Stream<Arguments> cases()
),
Arguments.of(new Request("IPv4 in Request Line")
.headers(
"GET http://1.2.3.4:2222/ HTTP/1.1",
"GET https://1.2.3.4:2222/ HTTP/1.1",
"Host: wrong"
),
new Expectations()
.scheme("http").serverName("1.2.3.4").serverPort(2222)
.requestURL("http://1.2.3.4:2222/")
.scheme("https").serverName("1.2.3.4").serverPort(2222)
.requestURL("https://1.2.3.4:2222/")
),
Arguments.of(new Request("IPv6 in Request Line")
.headers(
Expand Down Expand Up @@ -931,7 +951,7 @@ private static class Expectations implements Consumer<Actual>
public void accept(Actual actual)
{
assertThat("scheme", actual.scheme.get(), is(expectedScheme));
if (actual.scheme.get().equals("https"))
if (expectedScheme.equals("https"))
{
assertTrue(actual.wasSecure.get(), "wasSecure");
}
Expand Down