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 10.0.x 6497 alias checkers alt #6681

Merged
merged 7 commits into from
Sep 21, 2021
Merged

Jetty 10.0.x 6497 alias checkers alt #6681

merged 7 commits into from
Sep 21, 2021

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Aug 30, 2021

An alternative fix for #6497 that avoids using string prefix comparison and instead uses explicit File and Path methods

lachlan-roberts and others added 3 commits August 26, 2021 16:24
@lachlan-roberts
Issue #6497 - Replace the Alias checkers with new implementation.
c110648 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw
Alternate fix for #6497
8cb802f 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Alternate fix for #6497
a51e570 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Aug 30, 2021

@lachlan-roberts let me add some comments and javadoc to clarify my answers above....

gregw added 3 commits August 30, 2021 16:33
@gregw
Alternate fix for #6497
c29ba3e 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Alternate fix for #6497
497687d 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Alternate fix for #6497
48880c6 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from lachlan-roberts August 30, 2021 22:57
@gregw
Copy link
Contributor Author

gregw commented Aug 30, 2021

@lachlan-roberts new version pushed. I'm now handling symlinks by constructing the real URI to them and checking that with ContextHandler.isProtectedTarget(String).

@gregw
Fixed test for changed dump format
87c0f10 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Aug 31, 2021

@joakime I think @lachlan-roberts and I have closed our positions somewhat on this one. Would be good to get your review now.

@gregw gregw linked an issue Aug 31, 2021 that may be closed by this pull request
Replace SameFileAliasChecker #6497
Closed
@gregw gregw changed the base branch from jetty-10.0.x-6497-AliasCheckers to jetty-10.0.x August 31, 2021 06:31
@gregw gregw dismissed lachlan-roberts’s stale review August 31, 2021 06:31

The base branch was changed.

@gregw gregw requested a review from lachlan-roberts August 31, 2021 06:32
@gregw
Copy link
Contributor Author

gregw commented Aug 31, 2021

I've made this a direct PR to 10.0.x to make reviewing simpler for others. This has changes from both @lachlan-roberts and myself in it

@lachlan-roberts lachlan-roberts mentioned this pull request Sep 2, 2021
Closed
@gregw
Copy link
Contributor Author

gregw commented Sep 8, 2021

@joakime @lachlan-roberts nudge

@gregw
Copy link
Contributor Author

gregw commented Sep 9, 2021

@joakime your thoughts?

lachlan-roberts
lachlan-roberts reviewed Sep 16, 2021
View reviewed changes
jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java
protected boolean check(String pathInContext, Path path)
{
// do not allow any file separation characters in the URI, as we need to know exactly what are the segments
if (File.separatorChar != '/' && pathInContext.indexOf(File.separatorChar) >= 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be pathInContext.indexOf(File.separatorChar) != -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't that the same thing?

@gregw
Copy link
Contributor Author

gregw commented Sep 17, 2021

@joakime regardless of if this has been run on windows, do you have any comments on inspecting the code? Is this approach what you were advocating about using the Path class and associates more?

@lachlan-roberts
Copy link
Contributor

The AliasChecker tests passed when I ran them on windows.

I ran a full build with tests and had a whole bunch of failures which seem to be unrelated. I've never had a windows build pass all tests locally. But I will take a more detailed look at the windows failures soon for issue #6100.

@gregw
Copy link
Contributor Author

gregw commented Sep 17, 2021

Ok, I'll merge tonight unless I hear otherwise.

@gregw gregw merged commit aa793ee into jetty-10.0.x Sep 21, 2021
@gregw gregw deleted the jetty-10.0.x-6497-AliasCheckers-alt branch September 21, 2021 08:36
@lachlan-roberts lachlan-roberts mentioned this pull request Sep 22, 2021
Closed
lachlan-roberts added a commit that referenced this pull request Oct 12, 2021
@lachlan-roberts
Backport 6497 alias checkers alt (#6681)
7f94ec6 
* Issue #6497 - Replace the Alias checkers with new implementation.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Co-authored-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts mentioned this pull request Oct 12, 2021
Merged
lachlan-roberts added a commit that referenced this pull request Oct 17, 2021
@lachlan-roberts
Merge pull request #6979 from eclipse/jetty-9.4.x-6497-AliasCheckers
01fce6a 
Backport #6681 alias checker changes to Jetty 9.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@joakime joakime Awaiting requested review from joakime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace SameFileAliasChecker
2 participants
@gregw @lachlan-roberts