Jetty 12.1.x fix aliascheckertests

[janbartel]

Review ContextHandlerGetResourceTest, and AllowSymLinkAliasTest.

Most of ContextHandlerGetResourceTest is no longer applicable because the ContextHandler no longer implements the ServletContext.getResource(String) method. I've ported the tests that are checking the retrieval of resources to the ContextHandlerTest class instead.
However, some of them are failing. Not sure yet if this is due to the old test expecting that the resource would be alias checked (which the new ContextHandler.APIContext.getResource(String)` method does not do.

[janbartel]

@lorban @joakime please look at https://github.com/jetty/jetty.project/blob/jetty-12.1.x-fix-aliascheckertests/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java#L2699. This is a test that passes on ee9, but fails on ee10/11. This is because in ee9, the URL getResource(path) method calls URIUtil.canonicalPath(path), but in ee10/11 we are calling path = URIUtil.normalizePath(path);. One of these is incorrect, but which one? I'm leaning towards ee10/11 as incorrect, as the ee9 one is implementing previous jetty behaviour.

@lachlan-roberts can you confirm that alias checking is not performed on resources obtained from calls to o.e.j.ee9.nested.ContextHandler.getResource(path), nor on the ee10/11 implementations of ServletContextHandler.getServletContext().getResource(path) ?

[lachlan-roberts]

@janbartel yes, the alias check is now only performed in ResourceService.getContent, so you can call ContextHandler.getResource(path) and there will be no alias check.

[lorban]

@joakime ee9 uses canonical path while ee10 uses normalized path, but I don't know why. Can you please shed some light on here?

[joakime]

@joakime ee9 uses canonical path while ee10 uses normalized path, but I don't know why. Can you please shed some light on here?

Looks like that change comes from commit d369adf

Looks like both canonicalPath and normalizePath removes . and .. path segments.
But canonicalPath(String) also ...

• decodes pct-encoded things like %25, which we want to preserve.
• changes (space) to %20.

Which normalizePath doesn't do.

If you want a similar (but not 100% same) result to canonicalPath, but with normalizePath you would then follow up with a URIUtil.decodePath() (or similar call).

When looking at the implementations of both, and the testcases, and comparing them to Jetty 11.
Looks like Jetty 11's canonicalPath is the Jetty 12 normalizePath (which jives with the comments in commit d369adf)

At this point, it looks like ee9 is where the bug is, as it's not following the Jetty 11 behaviors.

[janbartel]

At this point, it looks like ee9 is where the bug is, as it's not following the Jetty 11 behaviors.

@joakime except that the tests in ee9 are the same as the tests in older versions of jetty, aren't they? Which would imply that it's ee11 that is wrong.

[janbartel]

@gregw @joakime the difference in the tests between prior versions of jetty and jetty-12 ee8/9/10/11 is these tests:

        assertThat(context.getServletContext().getResource("//subdir/data.txt"), notNullValue());
        assertThat(context.getServletContext().getResource("/subdir//data.txt"), notNullValue());
        assertThat(context.getServletContext().getResource("/subdir%2Fdata.txt"), notNullValue()); //encoded slash

In prior versions, they all returned null. In jetty-12 they return a resource.

Please review and indicate if you are happy with this behaviour change or not.

[gregw]

I need to check what that return value is and if it is an alias before I can OK this.
Note that we don't need comments like the above in the code. Put it as a PR comment instead.

[gregw]

I think this is good as the javadoc for this method now says in Servlet 6.1:

This method bypasses both implicit (no direct access to WEB-INF or META-INF) and explicit (defined by the web application) security constraints. Care should be taken both when constructing the path (e. g. avoid unsanitized user provided data) and when using the result not to create a security vulnerability in the application.

The provided path parameter is canonicalized as per Servlet 6.0, 3.5.2 before being used to match resources.

So it is explicitly warning against aliases.

I think we no longer use this pathway for our own resource serving, so we are no longer reliant on the alias checking here.

[gregw]

I think this is good as the javadoc for this method now says in Servlet 6.1:

This method bypasses both implicit (no direct access to WEB-INF or META-INF) and explicit (defined by the web application) security constraints. Care should be taken both when constructing the path (e. g. avoid unsanitized user provided data) and when using the result not to create a security vulnerability in the application.

The provided path parameter is canonicalized as per Servlet 6.0, 3.5.2 before being used to match resources.

So it is explicitly warning against aliases.

I think we no longer use this pathway for our own resource serving, so we are no longer reliant on the alias checking here.