Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-73129

Jetty 12 test failure DirectoryBrowserSupportTest

    • Icon: Task Task
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None

      On the prototype branch of Jenkins core, DirectoryBrowserSupportTest fails because the code under test now returns a 400 rather than 404. I have temporarily worked around the issue with the following hack:

      diff --git a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
      index 7b1bbdb7c9..e919223137 100644
      --- a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
      +++ b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
      @@ -89,6 +89,7 @@ import org.htmlunit.UnexpectedPage;
       import org.htmlunit.html.HtmlPage;
       import org.htmlunit.util.NameValuePair;
       import org.junit.Assume;
      +import org.junit.Ignore;
       import org.junit.Rule;
       import org.junit.Test;
       import org.jvnet.hudson.test.Email;
      @@ -150,7 +151,12 @@ public class DirectoryBrowserSupportTest {
               j.buildAndAssertSuccess(p);
       
               // can we see it?
      -        j.createWebClient().goTo("job/" + p.getName() + "/ws/abc%5Cdef.bin", "application/octet-stream");
      +        // TODO Why is this behavior changing?
      +        JenkinsRule.WebClient wc = j.createWebClient();
      +        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,13 +1114,16 @@ public class DirectoryBrowserSupportTest {
               String content = "random data provided as fixed value";
               Files.writeString(targetTmpPath, content, StandardCharsets.UTF_8);
       
      +        // TODO Why is this behavior changing?
               JenkinsRule.WebClient wc = j.createWebClient().withThrowExceptionOnFailingStatusCode(false);
      -        Page page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*", null);
      +        HtmlPage page = wc.goTo("userContent/" + targetTmpPath.toAbsolutePath() + "/*view*");
       
      -        MatcherAssert.assertThat(page.getWebResponse().getStatusCode(), equalTo(404));
      +        assertEquals(400, page.getWebResponse().getStatusCode());
      +        assertEquals("Error 400 Suspicious Path Character", page.getTitleText());
           }
       
           @Test
      +    @Ignore("TODO Escape hatch no longer works on Jetty 12")
           @Issue("SECURITY-2481")
           public void windows_canViewAbsolutePath_withEscapeHatch() throws Exception {
               Assume.assumeTrue("can only be tested this on Windows", Functions.isWindows());
      

      The root cause of this change in behavior should be understood. It may actually be a desirable Jetty security enhancement. If the behavior is expected, the test should be adapted (at the very least, to remove the TODO comment). If the behavior is unexpected, the bug should be fixed and the test change reverted. Before submitting the PR to the prototype branch to fix this ticket, ensure that mvn clean verify -Dtest=hudson.model.DirectoryBrowserSupportTest passes on both Linux and Windows. If we decide this is a desirable change and it obviates the escape hatch, then we should just remove the escape hatch in a PR to the default branch of Jenkins core.

          [JENKINS-73129] Jetty 12 test failure DirectoryBrowserSupportTest

          Mark Waite added a comment - - edited

          There is a rare behavioral difference for Jenkins that is a result of the Jakarta Servlet Spec 6.0 implementation of the suspicious path character check.

          Jenkins 2.461 with Jetty 10

          A label can be assigned to a node with the text "g\h". The label is displayed in the agent page as a valid label. When the label is clicked from the agent page, it reports page not found / HTTP 404 for the page with URL suffix '/label/g\h'.

          If the URL is converted from the literal "g\h" sequence to the URL escaped "g%5ch" sequence, then the label is shown on the web page for the page with URL suffix '/label/g%5ch/'.

          Jenkins 2.461-rc with Jetty 12

          A label can be assigned to a node with the text "g\h". The label is displayed in the agent page as a valid label. When the label is clicked from the agent page, it reports page not found / HTTP 404 for the page with URL suffix '/label/g\h'.

          If the URL is converted from the literal "g\h" sequence to the URL escaped "g%5ch" sequence, then the page reports HTTP 400 Suspicious Path Character for the page with URL suffix '/label/g%5ch/'.

          Mark Waite added a comment - - edited There is a rare behavioral difference for Jenkins that is a result of the Jakarta Servlet Spec 6.0 implementation of the suspicious path character check. Jenkins 2.461 with Jetty 10 A label can be assigned to a node with the text "g\h". The label is displayed in the agent page as a valid label. When the label is clicked from the agent page, it reports page not found / HTTP 404 for the page with URL suffix '/label/g\h'. If the URL is converted from the literal "g\h" sequence to the URL escaped "g%5ch" sequence, then the label is shown on the web page for the page with URL suffix '/label/g%5ch/'. Jenkins 2.461-rc with Jetty 12 A label can be assigned to a node with the text "g\h". The label is displayed in the agent page as a valid label. When the label is clicked from the agent page, it reports page not found / HTTP 404 for the page with URL suffix '/label/g\h'. If the URL is converted from the literal "g\h" sequence to the URL escaped "g%5ch" sequence, then the page reports HTTP 400 Suspicious Path Character for the page with URL suffix '/label/g%5ch/'.

          Mark Waite added a comment -

          I believe that the escape hatch will need an implementation that is aware of the Jetty 12 suspicious path character implementation. The servlet spec 6.0 allows that containers may modify the suspicious path character implementation, though I do not yet know how to perform that modification.

          Mark Waite added a comment - I believe that the escape hatch will need an implementation that is aware of the Jetty 12 suspicious path character implementation. The servlet spec 6.0 allows that containers may modify the suspicious path character implementation, though I do not yet know how to perform that modification.

          Mark Waite added a comment - - edited

          basil I think that the two comments added to my copy of the DirectoryBrowserSupportTest are ready for review. Changes in that file are not complete because the escape hatch still needs to be implemented for Jetty 12, but I believe the comments are ready for your review. I haven't changed this issue to "In review" because I would like to finish the escape hatch implementation and testing before the issue is put into review.

          I'm not sure how to handle the behavioral difference that is described in the earlier comment. The initial click of the label link reports HTTP 404 for both Jetty 10 and Jetty 12, but the URL encoded label link behaves differently with Jetty 10 (HTTP 200) than with Jetty 12 (HTTP 400). Do you have a recommendation for that case?

          It seems like a bug report for the HTTP 404 is appropriate but I'm not sure which is the more appropriate behavior for the URL encoded label link. Jetty 12 complies with servlet spec 6 but I don't think that users care much about servlet spec compliance.

          Mark Waite added a comment - - edited basil I think that the two comments added to my copy of the DirectoryBrowserSupportTest are ready for review. Changes in that file are not complete because the escape hatch still needs to be implemented for Jetty 12, but I believe the comments are ready for your review. I haven't changed this issue to "In review" because I would like to finish the escape hatch implementation and testing before the issue is put into review. I'm not sure how to handle the behavioral difference that is described in the earlier comment. The initial click of the label link reports HTTP 404 for both Jetty 10 and Jetty 12, but the URL encoded label link behaves differently with Jetty 10 (HTTP 200) than with Jetty 12 (HTTP 400). Do you have a recommendation for that case? It seems like a bug report for the HTTP 404 is appropriate but I'm not sure which is the more appropriate behavior for the URL encoded label link. Jetty 12 complies with servlet spec 6 but I don't think that users care much about servlet spec compliance.

          Basil Crow added a comment -

          Hi markewaite, I am not sure offhand what the behavior should be for URL encoded label links. This isn't an area I have any existing knowledge in, so I think your choices are to find someone else with knowledge in this security topic or to teach yourself by studying various other popular applications or reading online security resources about this topic. I don't believe it is worth the time to reimplement years-old escape hatches that are probably in use by 0 users, as I mentioned:

          If we decide this is a desirable change and it obviates the escape hatch, then we should just remove the escape hatch in a PR to the default branch of Jenkins core.

          Basil Crow added a comment - Hi markewaite , I am not sure offhand what the behavior should be for URL encoded label links. This isn't an area I have any existing knowledge in, so I think your choices are to find someone else with knowledge in this security topic or to teach yourself by studying various other popular applications or reading online security resources about this topic. I don't believe it is worth the time to reimplement years-old escape hatches that are probably in use by 0 users, as I mentioned: If we decide this is a desirable change and it obviates the escape hatch, then we should just remove the escape hatch in a PR to the default branch of Jenkins core.

          Daniel Beck added a comment -

          escape hatch still needs to be implemented for Jetty 12

          Ordinarily I would suggest updating and re-enabling https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/telemetry/impl/SecuritySystemProperties.java to understand whether this is even used. But I guess we're kind of late for that? If doing this properly would delay the Jetty project, just removing the escape hatch should be fine, someone will complain and then we can understand why it's needed, if anyone actually uses it.

          Based on the HTTP 400 error and message this looks like it comes from https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java anyway and I doubt we want to be more lax there unless really needed. (Wouldn't be the first time that we implemented a protection, and then upstream improvements ended up obsoleting it.)

          Daniel Beck added a comment - escape hatch still needs to be implemented for Jetty 12 Ordinarily I would suggest updating and re-enabling https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/telemetry/impl/SecuritySystemProperties.java to understand whether this is even used. But I guess we're kind of late for that? If doing this properly would delay the Jetty project, just removing the escape hatch should be fine, someone will complain and then we can understand why it's needed, if anyone actually uses it. Based on the HTTP 400 error and message this looks like it comes from https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java anyway and I doubt we want to be more lax there unless really needed. (Wouldn't be the first time that we implemented a protection, and then upstream improvements ended up obsoleting it.)

          Basil Crow added a comment -

          The prototype branch has been updated to expect a 400 "Suspicious Path Character" response with confidence. I have not included the references to the servlet specification since they are the wrong version (EE 10 rather than EE 9) and since the comments are duplicated all over the code. Such references are better suited in the PR description than in the source code I think.

          Basil Crow added a comment - The prototype branch has been updated to expect a 400 "Suspicious Path Character" response with confidence. I have not included the references to the servlet specification since they are the wrong version (EE 10 rather than EE 9) and since the comments are duplicated all over the code. Such references are better suited in the PR description than in the source code I think.

          Mark Waite added a comment -

          Jenkins 2.463 removes the escape hatch. It has also been removed from the documentation.

          Mark Waite added a comment - Jenkins 2.463 removes the escape hatch. It has also been removed from the documentation .

            markewaite Mark Waite
            basil Basil Crow
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: