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

Always set UriCompliance to LEGACY for Jetty 12 Runtimes #335

Merged
merged 2 commits into from
Jan 31, 2025
Merged

Always set UriCompliance to LEGACY for Jetty 12 Runtimes #335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f079dea
Always set UriCompliance to LEGACY for Jetty 12 Runtimes
lachlan-roberts Jan 24, 2025
99dd82f
Only set LEGACY UriCompliance for EE8 runtime by default
lachlan-roberts Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions ..._jetty12/src/main/java/com/google/apphosting/runtime/jetty/JettyServletEngineAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,13 @@
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import org.eclipse.jetty.http.CookieCompliance;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.MultiPartCompliance;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.SizeLimitHandler;
import org.eclipse.jetty.util.VirtualThreads;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.resource.ResourceFactory;
import org.eclipse.jetty.util.thread.QueuedThreadPool;

/**
Expand Down Expand Up @@ -138,10 +137,12 @@ public void run(Runnable runnable) {
httpConfiguration.setSendDateHeader(false);
httpConfiguration.setSendServerVersion(false);
httpConfiguration.setSendXPoweredBy(false);
httpConfiguration.setUriCompliance(UriCompliance.LEGACY);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added some logic to set setDecodeAmbiguousURIs to true in the EE10 AppEngineWebAppContext#newServletHandler if LEGACY_MODE is set.

if (LEGACY_MODE) {
httpConfiguration.setHttpCompliance(HttpCompliance.RFC7230_LEGACY);
httpConfiguration.setRequestCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setResponseCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setUriCompliance(UriCompliance.LEGACY);
httpConfiguration.setMultiPartCompliance(MultiPartCompliance.LEGACY);
}

server.addConnector(rpcConnector);
Expand Down
6 changes: 3 additions & 3 deletions ..._impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.logging.Level;
import org.eclipse.jetty.http.CookieCompliance;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.MultiPartCompliance;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.HttpConfiguration;
Expand All @@ -47,8 +48,6 @@
import org.eclipse.jetty.server.handler.gzip.GzipHandler;
import org.eclipse.jetty.util.Callback;

import static com.google.apphosting.runtime.AppEngineConstants.IGNORE_RESPONSE_SIZE_LIMIT;

/**
* A Jetty web server handling HTTP requests on a given port and forwarding them via gRPC to the
* Java8 App Engine runtime implementation. The deployed application is assumed to be located in a
Expand Down Expand Up @@ -95,11 +94,12 @@ public static ServerConnector newConnector(

HttpConfiguration config =
connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration();
config.setUriCompliance(UriCompliance.LEGACY);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we safe to force this @ludoch ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (JettyServletEngineAdapter.LEGACY_MODE) {
config.setHttpCompliance(HttpCompliance.RFC7230_LEGACY);
config.setRequestCookieCompliance(CookieCompliance.RFC2965);
config.setResponseCookieCompliance(CookieCompliance.RFC2965);
config.setUriCompliance(UriCompliance.LEGACY);
config.setMultiPartCompliance(MultiPartCompliance.LEGACY);
}

config.setRequestHeaderSize(runtimeOptions.jettyRequestHeaderSize());
Expand Down
Loading