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

Jump to bottom

Jetty 12 : More testing for Resource alias #8436

Merged
merged 38 commits into from
Sep 7, 2022
Merged

Jetty 12 : More testing for Resource alias #8436

merged 38 commits into from
Sep 7, 2022

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Aug 9, 2022

Reworked the PathResource.checkAliasPath ...

If the URI is a jar:file: based, the URI only check is skipped and the path segment checks still apply.
Also, the symlink check is skipped (as it's not possible to have symlinks in zipfs)

@joakime joakime self-assigned this Aug 9, 2022
@joakime
Copy link
Contributor Author

joakime commented Aug 9, 2022

I need more test cases!

What is a combination of ..

  1. A zipfs PathResource
  2. A request path used in resolve(String) that resolves to a resource that exists, but can be viewed as an alias.

@joakime joakime mentioned this pull request Aug 9, 2022
Closed
@joakime
Testing updates
537e359 
+ Closing ResourceFactory where appropriate
+ new internal to zipfs Symlink test
@joakime joakime requested a review from gregw August 10, 2022 01:04
@joakime joakime changed the title Jetty 12 : Rework PathResource.checkAliasPath to handle jar:file: paths more elegantly Jetty 12 : More testing for Resource alias Aug 10, 2022
gregw
gregw previously requested changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

A few things need fixing.

Overall, I'm really concerned about the amount of garbage that we are going to create as we frequently convert between Strings, URIs and Paths, then make corrected versions of them then throw them away. But let's go for correctness first I guess.

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java Show resolved Hide resolved
lachlan-roberts
lachlan-roberts requested changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

Few tests are now failing.

If the resource is an alias using Resource.getPath() will throw NoSuchFileException when used, but the Resource.exists() returns true. So everywhere it is used you would have to use a pattern like resource.isAlias() ? resource.getAlias() : resource.getPath(), so maybe instead resource.getPath() should return the alias path?

...y-core/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecOrderTest.java Outdated Show resolved Hide resolved
@lachlan-roberts
Copy link
Contributor

@joakime Can you look at the disabled test org.eclipse.jetty.ee10.servlet.DefaultServletTest#testListingFilenamesOnlyUrlResource in relation to the changes in this PR.

Is using jar:file URIs and in 12.0.x was failing because it was being classified as an alias because of trailing slash. But now on this PR the test it is throwing UnsupportedOperationException.

java.lang.UnsupportedOperationException
	at jdk.zipfs/jdk.nio.zipfs.ZipPath.toFile(ZipPath.java:669)
	at org.eclipse.jetty.server/org.eclipse.jetty.server.ResourceListing.getAsHTML(ResourceListing.java:207)
	at org.eclipse.jetty.server/org.eclipse.jetty.server.ResourceService.sendDirectory(ResourceService.java:530)
	at org.eclipse.jetty.server/org.eclipse.jetty.server.ResourceService.sendWelcome(ResourceService.java:438)
	at org.eclipse.jetty.server/org.eclipse.jetty.server.ResourceService.doGet(ResourceService.java:166)
	at org.eclipse.jetty.ee10.servlet/org.eclipse.jetty.ee10.servlet.DefaultServlet.doGet(DefaultServlet.java:388)
	at [email protected]/jakarta.servlet.http.HttpServlet.service(HttpServlet.java:527)
	at [email protected]/jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614)
	at org.eclipse.jetty.ee10.servlet/org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:742)

@lorban
Copy link
Contributor

lorban commented Sep 2, 2022

This is related to #8462 as well.

@joakime joakime dismissed gregw’s stale review September 7, 2022 00:37

Out on sabatacal

@joakime joakime merged commit 4b6ed97 into jetty-12.0.x Sep 7, 2022
@joakime joakime deleted the fix/jetty-12-pathresource-alias-detection-in-jarfile branch September 7, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw left review comments

@lachlan-roberts lachlan-roberts lachlan-roberts approved these changes

@lorban lorban Awaiting requested review from lorban

Assignees

@joakime joakime

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joakime @lachlan-roberts @lorban @gregw