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

Issue #5810 - Support bespoke PathSpec for servlets in embedded-jetty #5854

Closed
wants to merge 15 commits into from
Closed

Issue #5810 - Support bespoke PathSpec for servlets in embedded-jetty #5854

wants to merge 15 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jan 5, 2021 •
edited by gregw

Issue #5810
Allows users of embedded-jetty to use ServletContextHandler (and WebAppContext)
to add bespoke org.eclipse.jetty.http.pathmap.PathSpec mappings for Servlets.
Works with WEB-INF/jetty-web.xml too.

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

@joakime
Issue #5810 - Support bespoke PathSpec for servlets in embedded-jetty
725b858 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw January 5, 2021 21:01
@joakime joakime self-assigned this Jan 5, 2021
@joakime joakime added this to In progress in Jetty 9.4.36 via automation Jan 5, 2021
@joakime joakime added the Sponsored This issue affects a user with a commercial support agreement label Jan 6, 2021
Jetty 9.4.36 automation moved this from In progress to Review in progress Jan 11, 2021
@gregw
Copy link
Contributor

gregw commented Jan 13, 2021

On consideration, this is a lot more complex than at first look.
For this to go into a main branch it needs to be consistent and safe.... so that means that we need to consider how additional mapping types affect security. Eg, if a security constraint is put on a servlet, then it needs to apply to all it's mappings.

This is doable, but it will not be ready in time for 9.4.36. I believe we need to make a special branch to test this out for a period before merging with the main branch.

@gregw
Refactor of fix for #5810
77fe316 
 + Created a common `Mapping` base for `FilterMapping` and `ServletMapping`
 + `Mapping` contains an array of PathSpec.  The `String` based methods assume `ServletPathSpec`.
 + Convert most handling of string pathSpecs to `PathSpec` instances
 + Modified `ConstraintMapping` to also use a PathSpec
 + Modified JSP and annotation handling to cope with any existing non standard path specs
 + Made ServletHandler update mappings respect the order of the mappings

 TODO:
 + Should `ConstraintMapping` extend `Mapping` ?
 + Convert `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs
 + Review remaining calls to `Mapping.getPathSpecs` and `ConstrainMapping.getPathSpec`
 + Currently Quick Start operates on just the string versions of PathSpec, as it must be able to write a web.xml which can only contain standard path specs.  Should it fail when it sees a non standard pathspec, or assume that it will again be programmatically added during quick start?
 + Review how frequently we are creating new arrays of path spec and/or converting path specs to/from strings.  Best resolution for such things are ultility methods on Mapping (eg `#containsPathSpec`)
 + More tests
@gregw gregw requested a review from janbartel January 13, 2021 08:21
@gregw
Copy link
Contributor

gregw commented Jan 13, 2021

@janbartel Can you please review this closely. It is a lot more invasive than I hoped, but I still think that we can achieve zero behaviour change if only standard pathspecs are used. A lot of the changes are good cleanup. I'm not proposing an early merge for this branch, as I think it will need extensive testing first, specially on the security constraint implications of non standard mappings.

@gregw gregw marked this pull request as draft January 13, 2021 08:24
@gregw
Refactor of fix for #5810
f4e4a10 
 + Converted `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs
@gregw
Merge branch 'jetty-9.4.x' into jetty-9.4.x-5810-bespoke-pathspec-sup…
cba7b48 
…port-alt

Signed-off-by: gregw <gregw@webtide.com>
@gregw
Refactor of fix for #5810
30906f1 
 + Fixed test by reverting ServletMapping normalize
@gregw
Refactor of fix for #5810
eb66945 
 + More tests
@gregw
Updated naming after review
8b1e81d
janbartel
janbartel requested changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Some initial comments, I now need to do a deep dive on combining security constraints so I'll do another review when finished.

@gregw gregw removed this from Review in progress in Jetty 9.4.36 Jan 13, 2021
@gregw gregw marked this pull request as ready for review January 19, 2021 13:55
@gregw gregw requested a review from janbartel February 19, 2021 08:59
@gregw
updated file header
b93d0c3 
Signed-off-by: gregw <gregw@webtide.com>
@gregw gregw dismissed their stale review February 19, 2021 09:20

I've now become an author so I should not review

@gregw gregw requested review from gregw and removed request for gregw February 19, 2021 09:21
@gregw gregw self-assigned this Feb 19, 2021
@gregw gregw requested review from gregw and removed request for gregw February 19, 2021 09:24
janbartel
janbartel requested changes May 15, 2021
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

I still need to review some files - there's a lot of them. I don't see anywhere in this code where any specs are prefaced with "servlet|" or "regex|"? Where do they come from? Should this be when generating quickstart-web.xml?

jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathSpec.java
/**
* Test if the list of path specs contains a particular one.
*
* @param pathSpec the path spec
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an arbitrary type of path spec, or assumed to be a ServletPathSpec? It would be helpful to keep the language the same, as all the different path specs get a bit confusing.

jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java
private final PathSpecGroup _group;
private final int _pathDepth;
private final int _specLength;
private final Pattern _pattern;

private static String normalize(String regex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking below at ServletPathSpec, should this method be called "santiize" rather than normalize? It's not prefixing "/" for example ....

jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java
@@ -26,17 +26,16 @@
{
private static final Logger LOG = Log.getLogger(ServletPathSpec.class);

private final String _declaration;
private final PathSpecGroup _group;
private final int _pathDepth;
private final int _specLength;
private final String _prefix;
private final String _suffix;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should have javadoc that describes what a ServletPathSpec is: eg prefixed with "servlet|" etc etc

jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/RegexPathSpec.java
@@ -18,25 +18,29 @@

package org.eclipse.jetty.http.pathmap;

import java.util.Objects;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class RegexPathSpec extends AbstractPathSpec
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should have javadoc that describes what a RegexPathSpec is: eg prefixed by "regex|" etc etc

jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java
{
return getDeclaration().equals(normalize(sanitize(pathSpec)));
}

private static void assertValidServletPathSpec(String servletPathSpec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the servletPathSpec arg expected to be prefixed with "servlet|" or without?

jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/PathMappingsTest.java
@@ -293,7 +293,6 @@ public void testPrecidenceVsOrdering() throws Exception

@ParameterizedTest
@ValueSource(strings = {
"*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was * removed from the list?

@gregw
Copy link
Contributor

gregw commented May 24, 2021

I think we should test this PR on jetty-10 first and only once stable consider back-porting to 9 (or even just a sponsored branch of 9)

@stale
Copy link

stale bot commented Jul 9, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added Stale For auto-closed stale issues and pull requests and removed Stale For auto-closed stale issues and pull requests labels Jul 9, 2021
@gregw
Copy link
Contributor

gregw commented Sep 8, 2021

@joakime is there still a demand for this change?

@joakime
Copy link
Contributor Author

joakime commented Sep 8, 2021

I'll check with the sponsor

@joakime joakime mentioned this pull request Mar 1, 2022
Merged
@joakime
Copy link
Contributor Author

joakime commented Apr 7, 2022

Closed in favor of Issue #7748 and associated PRs (that have already been merged)

@joakime joakime closed this Apr 7, 2022
@gregw gregw deleted the jetty-9.4.x-5810-bespoke-pathspec-support-alt branch May 3, 2022 10:04
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

@janbartel janbartel janbartel requested changes

Assignees

@gregw gregw

@joakime joakime

Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@joakime @gregw @janbartel