Issue #6302 - Treat empty path segments as ambiguous. #6304

lachlan-roberts · 2021-05-20T14:11:03Z

Signed-off-by: Lachlan Roberts lachlan@webtide.com

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

gregw · 2021-05-21T00:17:55Z

I think we also need to add something in the documentation regarding ambiguous URIs, how to configure and alternatives if they are not exactly correct. @lachlan-roberts do you want to have a go at the doco, or should I push an attempt to this branch?

lachlan-roberts · 2021-05-21T00:37:53Z

@gregw you can push an attempt to this branch if you want.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw · 2021-05-21T22:48:17Z

I have moved the doco work to #6312 , so this PR can proceed without waiting for it.

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

…, '?' or end of string Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts · 2021-05-26T04:47:36Z

@gregw DefaultServletTest.testListingWithSession() is failing because it is trying a request to /context/;JSESSIONID=1234567890 which is now classified as having an ambiguous segment.

What do you think about this, should I fix this test to remove the ; from the URI?

gregw · 2021-05-26T05:52:18Z

@lachlan-roberts Ouch! that's a tough one. There definitely is an empty segment there, but we don't care because it is the last segment and we are happy to ignore empty segments when they are the last ones.... but at the time we check it, we don't know if it is the last one or not.

So how do we make /foo/;param/bar ambiguous, but /foo/;param not ? ideas???
Perhaps an empty segment is only ambiguous if we have see another segment after it?
So the first time checkSegment is called with an empty segment, we just remember we have seen an empty segment, then if checkSegment is ever called again we say we are ambiguous. That might be better than checking for the termination character???

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw

I've had enough input into this PR that it needs another reviewer.
However, other than some niggles, it LGTM

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

I've contributed too much to this. We need another reviewer

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts · 2021-06-03T06:22:50Z

@sbordet / @joakime can you please review this PR

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

sbordet · 2021-06-07T17:32:14Z

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

+        _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986);
+        assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
+        _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.UNSAFE);
+        assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));


Can you please add other combinations?

//ambiguous/initial
/ambiguous//middle
/double//ambiguous//
//triple//ambiguous//

I don't think this would be the right place for these extra tests. This is just testing how the compliance modes handle and empty segment.

What combinations cause an empty segment is tested by HttpUriTest. We already cover these cases, but I have also added these ones exactly to be sure.

gregw

There is at least one negation error in the comments.
Also here are some suggested improvements to the wording of them

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

better examples

Issue #6302 - Treat empty path segments are ambiguous.

36332ab

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw reviewed May 20, 2021

View reviewed changes

Fix false empty segments being reported + changes from review.

c30875e

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw added this to In progress in Jetty 10.0.4/11.0.4 FROZEN via automation May 21, 2021

gregw reviewed May 21, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Fix broken test + improve comments.

3206dab

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts marked this pull request as ready for review May 24, 2021 00:01

lachlan-roberts changed the title ~~Issue #6302 - Treat empty path segments are ambiguous.~~ Issue #6302 - Treat empty path segments as ambiguous. May 24, 2021

lachlan-roberts requested a review from gregw May 24, 2021 04:51

gregw reviewed May 24, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

gregw requested changes May 24, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Jetty 10.0.4/11.0.4 FROZEN automation moved this from In progress to Review in progress May 24, 2021

lachlan-roberts added 2 commits May 25, 2021 12:06

Add HttpUriTests for the empty segment as ambiguous

3aec963

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #63002 - Only call checkSegment() for real empty segments.

0cae7bf

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw previously requested changes May 25, 2021

View reviewed changes

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java Show resolved Hide resolved

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Issue #63002 - Empty segments are only ambiguous if followed by a '#'…

2649182

…, '?' or end of string Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested review from sbordet and gregw May 25, 2021 07:10

If only the last segment is empty it is not ambiguous.

2792217

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw reviewed May 27, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java Outdated Show resolved Hide resolved

lachlan-roberts added 2 commits May 28, 2021 11:48

improve testing for path HttpUri pathQuery()

3dd5460

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Add javadoc for HttpURI.Ambiguous

93eaffd

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from joakime May 31, 2021 01:59

gregw reviewed May 31, 2021

View reviewed changes

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java Outdated Show resolved Hide resolved

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java Outdated Show resolved Hide resolved

Add decodedPath to testPathQuery() expectations.

918f377

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

sbordet removed this from Review in progress in Jetty 10.0.4/11.0.4 FROZEN Jun 4, 2021

sbordet added this to In progress in Jetty 10.0.5/11.0.5 FROZEN via automation Jun 4, 2021

sbordet linked an issue Jun 4, 2021 that may be closed by this pull request

Treat empty path segments are ambiguous. #6302

Closed

sbordet requested changes Jun 7, 2021

View reviewed changes

Jetty 10.0.5/11.0.5 FROZEN automation moved this from In progress to Review in progress Jun 7, 2021

gregw requested changes Jun 8, 2021

View reviewed changes

Change comments, add some extra tests.

339e245

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw previously approved these changes Jun 10, 2021

View reviewed changes

sbordet self-requested a review June 10, 2021 10:32

gregw reviewed Jun 10, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

26d1646

gregw dismissed their stale review via 26d1646 June 10, 2021 10:52

gregw reviewed Jun 10, 2021

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Outdated Show resolved Hide resolved

Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

ceccab9

better examples

Jetty 10.0.5/11.0.5 FROZEN automation moved this from Review in progress to Reviewer approved Jun 10, 2021

sbordet approved these changes Jun 10, 2021

View reviewed changes

lachlan-roberts mentioned this pull request Jun 10, 2021

Treat empty path segments are ambiguous. #6302

Closed

sbordet merged commit b4d7e51 into jetty-10.0.x Jun 10, 2021

Jetty 10.0.5/11.0.5 FROZEN automation moved this from Reviewer approved to Done Jun 10, 2021

sbordet deleted the jetty-10.0.x-6302-EmptyPathSegments branch June 10, 2021 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #6302 - Treat empty path segments as ambiguous. #6304

Issue #6302 - Treat empty path segments as ambiguous. #6304

lachlan-roberts commented May 20, 2021

gregw commented May 21, 2021

lachlan-roberts commented May 21, 2021

gregw commented May 21, 2021

lachlan-roberts commented May 26, 2021

gregw commented May 26, 2021

gregw left a comment

lachlan-roberts commented Jun 3, 2021

sbordet Jun 7, 2021

lachlan-roberts Jun 9, 2021

gregw left a comment

Issue #6302 - Treat empty path segments as ambiguous. #6304

Issue #6302 - Treat empty path segments as ambiguous. #6304

Conversation

lachlan-roberts commented May 20, 2021

gregw commented May 21, 2021

lachlan-roberts commented May 21, 2021

gregw commented May 21, 2021

lachlan-roberts commented May 26, 2021

gregw commented May 26, 2021

gregw left a comment

Choose a reason for hiding this comment

lachlan-roberts commented Jun 3, 2021

sbordet Jun 7, 2021

Choose a reason for hiding this comment

lachlan-roberts Jun 9, 2021

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment