ForwardedRequestCustomizer.setForwardedHostHeader(null) has no effect in in Jetty 10.0.x

[fbuechler]

Jetty version(s)
10.0.6

Java version/vendor (use: java -version)
openjdk 11.0.10 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.10+9)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)

OS type/version
Debian GNU/Linux 10

Description
We use an embedded Jetty with a ForwardedRequestCustomizer. Because we want to ignore the X-Forwarded-Host header, we used setForwardedHostHeader(null). After updating from Jetty 9.4.x to 10.0.x, we noticed that this doesn't work anymore and the request host is updated with the host from X-Forwarded-Host.

How to reproduce?
Start a server with a ForwardedRequestCustomizer and setForwardedHostHeader(null):

Server server = new Server();

ForwardedRequestCustomizer forwardedCustomizer = new ForwardedRequestCustomizer();
forwardedCustomizer.setForwardedHostHeader(null);

HttpConfiguration httpConf = new HttpConfiguration();
httpConf.addCustomizer(forwardedCustomizer);

ServerConnector connector = new ServerConnector(server, new HttpConnectionFactory(httpConf));
connector.setPort(8080);
server.addConnector(connector);

server.start();

Send a request with X-Forwarded-Host: curl http://localhost:8080/ -H 'X-Forwarded-Host: example.org'
The ForwardedRequestCustomizer will set the request URI to http://example.org/ (see https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java#L534).

[joakime]

That is an unexpected use case.

I hesitate to support this, as the ForwardedRequestCustomizer is one of most delicate piece of code in the project due to how messed up the non-standard X-Forwarded-* headers are in the real world.

Why do you do this?
Do you not trust your load balancer? (why is it either not sending you the correct X-Forwarded-Host header, or not stripping it before sending it to you?)
Is it conflicting with a different header?
If a malicious user discovers this behavior, it could result in some interesting exploits.

Anyway, there are 2 solutions I can think of for you to use today.

1. Exploiting HTTP Validation

ForwardedRequestCustomizer forwardedCustomizer = new ForwardedRequestCustomizer();
forwardedCustomizer.setForwardedHostHeader("ignore : me");

This sets the header to something impossible to match.
This is because an HTTP field is not valid if it contains a space or a colon : and would be rejected at HttpParser, way before it reaches the ForwardedRequestCustomizer.

2. Customized Solution

Extend from the ForwardedRequestCustomizer ...

        Server server = new Server();

        HttpConfiguration httpConfiguration = new HttpConfiguration();
        httpConfiguration.addCustomizer(new ForwardedRequestCustomizer() {
            @Override
            public String getForwardedHostHeader() {
                return null;
            }
        });

        ServerConnector connector = new ServerConnector(server, new HttpConnectionFactory(httpConfiguration));
        connector.setPort(9999);

        server.addConnector(connector);

This breaks the binding of header field names to actions for that one header.

---

I still want to know why you are ignoring the X-Forwarded-Host header coming from your load balancer.

[fbuechler]

Thanks for the detailed answer. Extending the ForwardedRequestCustomizer should work for us.

We chose to ignore the header because we wanted to prevent Host header attacks without having to rely on the load balancer being configured to remove an external X-Forwarded-Host header.

While I agree that this is an unexpected use case, I still think that the current behavior is a bug.
For one thing, setForwardedOnly() also sets _forwardedHostHeader etc. to null to ignore the headers which currently doesn't work.
For another, setting _forwardedHostHeader to a different value, as proposed in your first solution, results in that value being added as an alternative header name but not in replacing X-Forwarded-Host.
Both are probably caused by aa8bd5d which changed updateHandles() to not remove the previous handles before adding the new ones.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[joakime]

Addressed in Issue #7975