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.

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

Jetty 10.0.x 6497 alias checkers alt #6681

Merged
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
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
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....

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@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).

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
@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
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

@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?

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 gregw added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Sep 17, 2021
@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.

@joakime joakime moved this from In progress to Reviewer approved in Jetty 10.0.7/11.0.7 FROZEN Sep 20, 2021
@gregw gregw merged commit aa793ee into jetty-10.0.x Sep 21, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Sep 21, 2021
@gregw gregw deleted the jetty-10.0.x-6497-AliasCheckers-alt branch September 21, 2021 08:36
lachlan-roberts added a commit that referenced this pull request Oct 12, 2021
* 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 added a commit that referenced this pull request Oct 17, 2021
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
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Replace SameFileAliasChecker
2 participants