-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Issue #8296 and #8259 - AllowedResourceAliasChecker improvements #8315
Conversation
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…Handler.doStart() Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few niggles
{ | ||
// We may have symlink to baseResource, try to resolve symlink if possible. | ||
File file = getBaseResource().getFile(); | ||
if (file != null) | ||
_baseResource = Resource.newResource(file.toPath().toRealPath()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the WebAppProvider in the deployer does something similar. Perhaps wrap up in a utlility static method on Resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a static method on Resource
called resolveAlias
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but it needs javadoc!
...-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerMultipleResourceBasesTest.java
Outdated
Show resolved
Hide resolved
...-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerMultipleResourceBasesTest.java
Show resolved
Hide resolved
...-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerMultipleResourceBasesTest.java
Outdated
Show resolved
Hide resolved
.../test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerWebRootIsSymlinkTest.java
Show resolved
Hide resolved
.../test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerWebRootIsSymlinkTest.java
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved... but add some javadoc to the new method before merging.
{ | ||
// We may have symlink to baseResource, try to resolve symlink if possible. | ||
File file = getBaseResource().getFile(); | ||
if (file != null) | ||
_baseResource = Resource.newResource(file.toPath().toRealPath()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but it needs javadoc!
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Issue #8296 and #8259
AllowedResourceAliasChecker
andSymlinkAllowedResourceAliasChecker
to allow custom resource base directory, so we can more easily support use case of multiple default servlets with different resource bases from Symlinks cause 404 with DefaultServlet when its "resourceBase" is different from ContextHandler's #8259AllowedResourceAliasChecker
to be used whenContextHandler
is still starting, by always extracting resource base when needed beforelifeCycleStarted
is called.baseResource
duringContextHandler.doStart()
so that not every path from it is also an alias.