[JENKINS-75278] Upgrade Jetty 12.0.17 which restore UriCompliance.LEGACY previous behaviour

[rsandell]

Testing done

Unit tests written. But I can't get the http2 connector test to work, it fails with whatever uri compliance I set. Is it perhaps not possible in the http2 spec?

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[basil]

AbstractWinstoneTest#makeRequest doesn't work with HTTP/2 tests. For HTTP/2 tests you'll need to use Http2ConnectorFactoryTest#request. Since that currently takes int port as its second argument, it isn't flexible enough to be used with a new servlet, so I would recommend changing the argument from int port to URI uri to allow you to call it with a new URI in your new tests. You'll also need to make sure to construct the URI with https rather than http like the existing Http2ConnectorFactoryTest#wildcard test is doing in order for everything to work.

[olamy]

AbstractWinstoneTest#makeRequest doesn't work with HTTP/2 tests. For HTTP/2 tests you'll need to use Http2ConnectorFactoryTest#request. Since that currently takes int port as its second argument, it isn't flexible enough to be used with a new servlet, so I would recommend changing the argument from int port to URI uri to allow you to call it with a new URI in your new tests. You'll also need to make sure to construct the URI with https rather than http like the existing Http2ConnectorFactoryTest#wildcard test is doing in order for everything to work.

Good catch. fixed with 09776fc

[olamy]

please note the last commit contains the correct setup for Jetty httpclient to use Http2 and simplification of Jetty httpclient configuration globally.
25034a2

can be part of a different PR if needed.

[basil]

When we first adopted Jetty 12 EE 8 in core in jenkinsci/jenkins#9590, we made these changes:

diff --git b/test/src/test/java/hudson/PluginTest.java a/test/src/test/java/hudson/PluginTest.java
index da30cf91a8..7328d36f93 100644
--- b/test/src/test/java/hudson/PluginTest.java
+++ a/test/src/test/java/hudson/PluginTest.java
@@ -54,7 +54,7 @@ public class PluginTest {
         r.createWebClient().assertFails("plugin/matrix-auth/images/%2e%2e%2fWEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST);
         r.createWebClient().assertFails("plugin/matrix-auth/images/%2e.%2fWEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST);
         r.createWebClient().assertFails("plugin/matrix-auth/images/..%2f..%2f..%2f" + r.jenkins.getRootDir().getName() + "%2fsecrets%2fmaster.key", HttpServletResponse.SC_BAD_REQUEST);
-        r.createWebClient().assertFails("plugin/matrix-auth/" + r.jenkins.getRootDir() + "/secrets/master.key", /* ./ prepended anyway */ HttpServletResponse.SC_NOT_FOUND);
+        r.createWebClient().assertFails("plugin/matrix-auth/" + r.jenkins.getRootDir() + "/secrets/master.key", /* ./ prepended anyway */ Functions.isWindows() ? HttpServletResponse.SC_BAD_REQUEST : HttpServletResponse.SC_NOT_FOUND);
         // SECURITY-155:
         r.createWebClient().assertFails("plugin/matrix-auth/WEB-INF/licenses.xml", HttpServletResponse.SC_BAD_REQUEST);
         r.createWebClient().assertFails("plugin/matrix-auth/META-INF/MANIFEST.MF", HttpServletResponse.SC_BAD_REQUEST);
diff --git b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
index ced47beead..fbd1173909 100644
--- b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
+++ a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
@@ -149,8 +149,16 @@ public class DirectoryBrowserSupportTest {
         p.getBuildersList().add(new Shell("mkdir abc; touch abc/def.bin"));
         j.buildAndAssertSuccess(p);
 
-        // can we see it?
-        j.createWebClient().goTo("job/" + p.getName() + "/ws/abc%5Cdef.bin", "application/octet-stream");
+        try (JenkinsRule.WebClient wc = j.createWebClient()) {
+            // normal path provided by the UI succeeds
+            wc.goTo("job/" + p.getName() + "/ws/abc/def.bin", "application/octet-stream");
+
+            // suspicious path is rejected with 400
+            wc.setThrowExceptionOnFailingStatusCode(false);
+            HtmlPage page = wc.goTo("job/" + p.getName() + "/ws/abc%5Cdef.bin");
+            assertEquals(400, page.getWebResponse().getStatusCode());
+            assertEquals("Error 400 Suspicious Path Character", page.getTitleText());
+        }
     }
 
     @Test
@@ -1108,10 +1116,13 @@ public class DirectoryBrowserSupportTest {
         String content = "random data provided as fixed value";
         Files.writeString(targetTmpPath, content, StandardCharsets.UTF_8);
 
-        JenkinsRule.WebClient wc = j.createWebClient().withThrowExceptionOnFailingStatusCode(false);
-        Page page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*", null);
-
-        MatcherAssert.assertThat(page.getWebResponse().getStatusCode(), equalTo(404));
+        try (JenkinsRule.WebClient wc = j.createWebClient()) {
+            // suspicious path is rejected with 400
+            wc.setThrowExceptionOnFailingStatusCode(false);
+            HtmlPage page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*");
+            assertEquals(400, page.getWebResponse().getStatusCode());
+            assertEquals("Error 400 Suspicious Path Character", page.getTitleText());
+        }
     }
 
     @Test

Are all of these changes still necessary after this PR, or can some or all of them be reverted?

[olamy]

yes. possibly might need to be reverted.
especially the one with explicit %5C, the others with file path may fail as well (windows will say \?)
but not so sure in this case (htmlunit), I need to search a bit.
I have created a PR jenkinsci/jenkins#10344 to have a build running with incremental of this PR.
So I will adjust there.

[olamy]

haha right. I was confused jenkins core PR not failing,
but yeah those tests are not using winstone :(
so core PR need jenkinsci/jenkins-test-harness#930

[basil]

jetty/jetty.project#12809 (comment) makes me nervous that this change might introduce a regression for other (not suspicious character) violations in LEGACY scope. I am not sure if that concern applies to EE 9 (as opposed to EE 10), but if it does, perhaps we should leave an escape hatch for the old behavior just in case?

[olamy]

jetty/jetty.project#12809 (comment) makes me nervous that this change might introduce a regression for other (not suspicious character) violations in LEGACY scope. I am not sure if that concern applies to EE 9 (as opposed to EE 10), but if it does, perhaps we should leave an escape hatch for the old behavior just in case?

Well, legacy should mean similar to Jetty9/10/11, and it's probably what we want here?
If you look in the other direction, you can see that the Winstone upgrade to Jetty 12 has introduced some non-backward compact changes.

[basil]

Well, legacy should mean similar to Jetty9/10/11, and it's probably what we want here?

Please read the linked comment more closely. The concern here is that what was previously allowed under legacy mode might (erroneously!) no longer be allowed under legacy mode if suspicious path characters are added to legacy mode.

[olamy]

Well, legacy should mean similar to Jetty9/10/11, and it's probably what we want here?

Please read the linked comment more closely. The concern here is that what was previously allowed under legacy mode might (erroneously!) no longer be allowed under legacy mode if suspicious path characters are added to legacy mode.

Both comments mention adding back SUSPICIOUS_PATH_CHARACTERS to LEGACY as we are doing here.

I am not sure to understand as the last comment is about jetty-start (only used when using the jetty-distribution) failing to understand parameter such jetty.httpConfig.uriCompliance=LEGACY,SUSPICIOUS_PATH_CHARACTERS in config file , which is something we are not using as we are using an embedded jetty and not sure if this applies in this case.

[basil]

I am not sure to understand

The point is that the embedded Jetty used in that example exhibited unexpected behavior (potentially a regression) when suspicious path characters were added to the legacy mode. Assuming the same is true for EE 9 as it is for EE 10, that could similarly affect our embedded Jetty instance in this PR, hence my suggestion to either confirm that we are not affected or to add an escape hatch.

[rsandell]

I have added an escape hatch where you can deny suspicious patch characters to go back to the "old" behaviour.
Here and in jenkins-test-harness. But I haven't yet updated the tests in core to use that escape hatch.

[olamy]

At this stage, we can hold off this for few days as Jetty 12.0.17 will integrate the fix and should be there by early next week (start of March)