NPE in AllowedResourceAliasChecker.getPath()

[tivervac]

Jetty version(s)
10.0.7

Java version/vendor (use: java -version)

openjdk 11.0.13 2021-10-19
OpenJDK Runtime Environment 18.9 (build 11.0.13+8)
OpenJDK 64-Bit Server VM 18.9 (build 11.0.13+8, mixed mode, sharing)

OS type/version

Fedora 34; kernel 5.14.13-200.fc34.x86_64

Description

How to reproduce?
Create a server with a ServletContextHandler without a baseResource (not sure if this minimal config is enough, but the important part is I don't configure a base resource for my ServletContextHandler).

Server server = new Server(new InetSocketAddress("localhost", 0));
server.setStopAtShutdown(true);

ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
contextHandler.setContextPath("/");

server.setHandler(contextHandler);

When you then start the server you'll hit the following function

org.eclipse.jetty.server.AllowedResourceAliasChecker.getPath(Resource)

This one tries to get the path of the contextHandler's base resource, which is null resource.getFile().toPath(). Since it's null the following line will throw an NPE.

It will be caught by the very generic catch block underneath, but I'm sure that a better error message can be thought of.

[joakime]

It's always a good idea to define a Base Resource.
Even if you all you do is point to an empty directory in your JAR file.

Having no Base Resource will break many APIs.

Not just this AllowedResourceAliasChecker.
You'll also break things in the servlet jar itself, so we cannot fix those broken things.

I know that if you, or any of your libraries, call things like ...

ServletContext.getRealPath(String)
ServletContext.getResource(String)
ServletContext.getResourceAsStream(String)
ServletContext.getResourcePaths(String)

Those APIs will break as well.

If you use Servlet Auth (authentication or authorization) the constraint mapping will even fail.

[lachlan-roberts]

@tivervac this exception is caught in getPath(Resource) and is being logged with LOG.ignore(t) and not rethrown, so you shouldn't be seeing this in the logs unless you've set your logging level to ALL.

What exactly do you see to be the issue here, is it just that you think this should be logged at a higher level like debug or warn with a more descriptive message?

[tivervac]

@lachlan-roberts I saw it by running my application as debug with an NPE breakpoint on.

My issue is that if you look at the code, it's very obvious org.eclipse.jetty.server.handler.ContextHandler.getBaseResource() that null is a valid return value. Using a catch(Throwable) to deal with NPEs is AFAIK bad practice.

IMO it would be better to handle this case specifically, checking for null before dereferencing the possibly null resource instead of just catching the NPE. Logging at a higher level with something like You should set a base resource to support blabla would indeed be much better.

[lachlan-roberts]

@tivervac I agree, I'll put up a PR for this.

I don't think a null base resource should produce a warning here as it is a valid configuration. I think it should just set some flag to completely disable the SymlinkAllowedResourceAliasChecker which is added by default.

[lachlan-roberts]

PRs #7081 and #7076 have been merged, so this will be fixed in upcoming releases 9.4.45, 10.0.8 & 11.0.8.