Always set UriCompliance to LEGACY for Jetty 12 Runtimes by lachlan-roberts · Pull Request #335 · GoogleCloudPlatform/appengine-java-standard

lachlan-roberts · 2025-01-24T08:52:19Z

This ensures that UriCompliance is always set to LEGACY in 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~~

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

ghost

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: The UriCompliance is unconditionally set to LEGACY (lines 140-145). Additional changes on lines 45-56 reflect added imports for HttpCompliance and MultiPartCompliance.
runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java: Similar changes were made to ensure consistent UriCompliance settings (lines 97-102). Import changes for MultiPartCompliance are 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.

ghost

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.

paulrutter · 2025-01-24T09:03:42Z

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.

maigovannon · 2025-01-27T05:00:16Z


    HttpConfiguration config =
        connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration();
+    config.setUriCompliance(UriCompliance.LEGACY);


Are we safe to force this @ludoch ?

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

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.

gregw · 2025-01-28T08:25:51Z

      httpConfiguration.setSendDateHeader(false);
      httpConfiguration.setSendServerVersion(false);
      httpConfiguration.setSendXPoweredBy(false);
+      httpConfiguration.setUriCompliance(UriCompliance.LEGACY);


I would not set this all the time. We want new behaviour for >= EE10 deployments

Now only setting this if LEGACY mode is set or we are configured to use EE8.

gregw · 2025-01-28T08:27:48Z

      httpConfiguration.setSendDateHeader(false);
      httpConfiguration.setSendServerVersion(false);
      httpConfiguration.setSendXPoweredBy(false);
+      httpConfiguration.setUriCompliance(UriCompliance.LEGACY);


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

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 <lachlan.p.roberts@gmail.com>

lachlan-roberts · 2025-01-29T05:56:32Z

@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 LEGACY flag to allow these ambiguous URIs like in Jetty 9.4.

gregw

cautious approval

gregw · 2025-01-31T07:06:24Z

+    switch (getEEVersion()) {
+      case EE10:
+        return new EE10AppVersionHandlerFactory(server, serverInfo);
+      case EE8:
+        return new EE8AppVersionHandlerFactory(server, serverInfo);
+      default:
+        throw new IllegalStateException("Unknown EE version: " + getEEVersion());


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.

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?

Always set UriCompliance to LEGACY for Jetty 12 Runtimes

f079dea

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

ghost reviewed Jan 24, 2025

View reviewed changes

Comment thread ...mpl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/JettyServletEngineAdapter.java Outdated

Comment thread ...ime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java Outdated

maigovannon reviewed Jan 27, 2025

View reviewed changes

gregw suggested changes Jan 28, 2025

View reviewed changes

Only set LEGACY UriCompliance for EE8 runtime by default

99dd82f

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>

lachlan-roberts requested review from gregw, ludoch and maigovannon January 29, 2025 05:59

ludoch approved these changes Jan 29, 2025

View reviewed changes

maigovannon approved these changes Jan 30, 2025

View reviewed changes

gregw approved these changes Jan 31, 2025

View reviewed changes

maigovannon added the ready to pull label Jan 31, 2025

copybara-service Bot merged commit 54e94d9 into main Jan 31, 2025

copybara-service Bot deleted the UriComplianceLegacy branch January 31, 2025 10:16

Uh oh!

Conversation

lachlan-roberts commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paulrutter commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lachlan-roberts commented Jan 29, 2025

Uh oh!

gregw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lachlan-roberts commented Jan 24, 2025 •

edited

Loading