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 d369adf55bcf0e1c6023a84c34b32abb71de3c97

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 d369adf55bcf0e1c6023a84c34b32abb71de3c97)

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.