Always set UriCompliance to LEGACY for Jetty 12 Runtimes #335
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
![code-review-assist[bot]](https://avatars.githubusercontent.com/in/1008254?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @lachlan-roberts and reviewers,
I'm Gemini, and I'm currently reviewing this pull request. In the meantime, here's a summary to help everyone quickly understand the changes:
This pull request, "Always set UriCompliance to LEGACY for Jetty 12 Runtimes", aims to ensure consistent URI handling across different Jetty versions by always setting UriCompliance to LEGACY in Jetty 12 runtimes. This mimics the behavior of Jetty 9.4, allowing certain URI violations.
Here's a breakdown of the changes:
runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/JettyServletEngineAdapter.java: TheUriComplianceis unconditionally set toLEGACY(lines 140-145). Additional changes on lines 45-56 reflect added imports forHttpComplianceandMultiPartCompliance.runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java: Similar changes were made to ensure consistentUriCompliancesettings (lines 97-102). Import changes forMultiPartComplianceare also present (lines 35-41). Removal of unused import on lines 47-49.
The changes address a known issue with Jetty EE10 (jetty/jetty.project#12733).
I'll have a more detailed review shortly.
And now, a little haiku to brighten your day:
Uri paths aligned,
Legacy mode, smooth sailing,
Code flows, clear and bright.
There was a problem hiding this comment.
This pull request proposes to always set UriCompliance to LEGACY for Jetty 12 runtimes to mimic Jetty 9.4 URI behavior. The change is well-motivated and correctly implemented. However, the provided reasoning in the description could be more detailed. It would be helpful to explain why mimicking 9.4 behavior is necessary and what specific URI violations are allowed in legacy mode. Additionally, since this change affects URI handling, it's important to consider potential security implications and document them clearly.
...mpl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/JettyServletEngineAdapter.java
Outdated
Show resolved
Hide resolved
...ime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java
Outdated
Show resolved
Hide resolved
|
You probably need more changes than just setting the uri compliance, see jetty/jetty.project#11448 (comment). Do note the security implications of doing so. |
| @@ -95,11 +94,12 @@ public static ServerConnector newConnector( | |||
|
|
|||
| HttpConfiguration config = | |||
| connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration(); | |||
| config.setUriCompliance(UriCompliance.LEGACY); | |||
There was a problem hiding this comment.
Are we safe to force this @ludoch ?
There was a problem hiding this comment.
Nothing like this is safe.
We could wrap this code with a new env variable and setup a new experiment like we did for the other 2 experiments.
Webtide can also add more comment and evaluation
There was a problem hiding this comment.
Allowing ambiguous URIs into the application does have security implications, but these have been there for all servlet containers < 6.0 and definitely exist on the current jetty-9.4 based runtimes. So allowing them is only putting the responsibility back on the application, exactly as it has been to date. From Servlet 6.0 onwards, the default has been to not allow such URIs, so that apps are protected.
| @@ -138,10 +137,12 @@ public void run(Runnable runnable) { | |||
| httpConfiguration.setSendDateHeader(false); | |||
| httpConfiguration.setSendServerVersion(false); | |||
| httpConfiguration.setSendXPoweredBy(false); | |||
| httpConfiguration.setUriCompliance(UriCompliance.LEGACY); | |||
There was a problem hiding this comment.
I would not set this all the time. We want new behaviour for >= EE10 deployments
There was a problem hiding this comment.
Now only setting this if LEGACY mode is set or we are configured to use EE8.
| @@ -138,10 +137,12 @@ public void run(Runnable runnable) { | |||
| httpConfiguration.setSendDateHeader(false); | |||
| httpConfiguration.setSendServerVersion(false); | |||
| httpConfiguration.setSendXPoweredBy(false); | |||
| httpConfiguration.setUriCompliance(UriCompliance.LEGACY); | |||
There was a problem hiding this comment.
Note also that there are two parts to URI legacy behaviour. This compliance will allow ambiguous URIs into the container. You also need to ServletHandler.setDecodeAmbiguousURIs(true) if you wish such URIs to be returned from getServletPath and getPathInfo as there were previously
There was a problem hiding this comment.
I have added some logic to set setDecodeAmbiguousURIs to true in the EE10 AppEngineWebAppContext#newServletHandler if LEGACY_MODE is set.
Signed-off-by: Lachlan Roberts <[email protected]>
|
@ludoch @maigovannon I updated the PR so that for EE8 we always behave like we did in Jetty 9.4. And then for EE10 you must configure the |
| switch (getEEVersion()) { | ||
| case EE10: | ||
| return new EE10AppVersionHandlerFactory(server, serverInfo); | ||
| case EE8: | ||
| return new EE8AppVersionHandlerFactory(server, serverInfo); | ||
| default: | ||
| throw new IllegalStateException("Unknown EE version: " + getEEVersion()); |
There was a problem hiding this comment.
The problem with this approach, is that it will not work if you eve wish to add EE11, because EE10 and EE11 cannot be in the same classloader. If there ever is the intention to support EE10 and EE11 in the same runtime, then thought is needed for environment classloaders.
There was a problem hiding this comment.
Sorry just curious: I was under the assumption that the Jetty12 will support all future versions EE including EE11 and beyond. Are we planning on using a different lib/classloader for EE11 if we need to support it in future?
This ensures that
UriComplianceis always set toLEGACYin Jetty12 runtimes, so that we can model 9.4 behavior of URIs by allowing certain URI violations.Also see additional fix needed for Jetty EE10 jetty/jetty.project#12733