ForwardedRequestCustomizer setters do not clear existing handlers

[amarshall45676]

Jetty version(s)
10.0.8

Java version/vendor (use: java -version)
openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.1+13)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.1+13, mixed mode)

OS type/version
macOS Monterey Version 12.1

Description
We use ForwardedRequestCustomizer to dynamically configure the header to use instead of "X-Forwarded-For" to determine the IP address for a clients request. We use ForwardedRequestCustomizer.setForwardedForHeader to set a custom header to use instead of X-Forwarded-For. After updating from Jetty version 9.4.44 to 10.0.8 we saw an issue where, depending on the ordering of headers, a request would have a different remote address.

In the code for 9.4.44, the updateHandles function in ForwardedRequestCustomizer would always reinstantiate _handles before setting the headers that mapped to each handler (code link). What this meant is that using setForwardedForHeader would mean that the only header that would cause the "handleForwardedFor" handler to run would be the one specified via setForwardedForHeader. However after the upgrade to 10.0.8 the existing handlers in _handles never get cleared. Instead the updateHandles function will just add a new header to handle mapping that will run "handleForwardedFor" (code). What this means is that both the custom header we specify and "X-Forwarded-For" can be used to calculate the remote address for a request. We even saw some confusing behavior where, based on the order of the headers in the request a different remote address would be calculated (likely due to HttpFields being process in the specified order)

This looks like what could have also caused the issue in this other bug: #7026. We can take a similar approach to what you suggested in that issue and override the getter for getForwardedForHeader, but the behavior of the setters is still confusing. It would be good if the setters on ForwardedRequestCustomizer would clear the existing header to handler mappings before applying the new ones (similar to what was done in version 9.4).

How to reproduce?

Server server = new Server();

ForwardedRequestCustomizer forwardedCustomizer = new ForwardedRequestCustomizer();
forwardedCustomizer.setForwardedForHeader("X-Custom");

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

server.setRequestLog(new CustomRequestLog(new Slf4jRequestLogWriter(), CustomRequestLog.EXTENDED_NCSA_FORMAT));

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

server.start();

Now depending on the order of headers, a different remote address will be calculated

curl http://localhost:8080/ -H 'X-Forwarded-For: 1.1.1.1' -H 'X-Custom: 2.2.2.2'

Log looks like the following, showing that the remote address is considered 1.1.1.1

INFO org.eclipse.jetty.server.RequestLog - 1.1.1.1 - - [10/May/2022:14:29:19 +0000] "GET / HTTP/1.1" 404 434 "-" "curl/7.77.0"

curl http://localhost:8080/ -H 'X-Custom: 2.2.2.2' -H 'X-Forwarded-For: 1.1.1.1' 

Log looks like the following, showing that the remote address is considered 2.2.2.2

INFO org.eclipse.jetty.server.RequestLog - 2.2.2.2 - - [10/May/2022:14:29:58 +0000] "GET / HTTP/1.1" 404 434 "-" "curl/7.77.0"

[gb6881]

we ran into the same issue with 10.0.9, i've added a Test, not sure
0001-ForwardedRequestCustomizer-removal-of-headers-e.g.-p.patch.zip

i'm not sure what would be the best approach:
-> first clear() on every update in org.eclipse.jetty.server.ForwardedRequestCustomizer.updateHandles()
(before in 9.x the handler list was replaced)
-> or remove the blank one, issue is the previous name is not known anymore ...

this is topic is potentially security relevant ..

[joakime]

Opened PR #8102