Issue #5025 - wrong welcome file handling with dispatcher.include() a… #5026

grgrzybek · 2020-07-04T13:23:01Z

…nd non-default mapping

joakime · 2020-07-04T15:54:49Z

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

+        HttpTester.Response response;
+
+        // Test included alt default
+        rawResponse = connector.getResponse("GET /context/gateway HTTP/1.0\r\n\r\n");


This should be GET /context/gateway/ HTTP/1.0\r\n\r\n as you are requesting content belonging to the url-pattern of /gateway/* which the request to /gateway will not match.

I checked org.eclipse.jetty.http.pathmap.PathMappings#getMatch() and /p request is successfully matched for servlet mapped to /p/* - it's kept in org.eclipse.jetty.http.pathmap.PathMappings#_prefixMap:

_prefixMap = {org.eclipse.jetty.util.ArrayTernaryTrie@2636} "{/p=MappedResource[pathSpec=ServletPathSpec["/p/*",pathDepth=2,group=PREFIX_GLOB],resource=default-servlet@2c16bab9==org.ops4j.pax.web.service.jetty.internal.EmbeddedJettyTest$11,jsp=null,order=-1,inst=true,async=true]}" LO: int = 1 (0x1) EQ: int = 2 (0x2) HI: int = 3 (0x3) ROW_SIZE: int = 4 (0x4) _tree: char[] = {char[512]@2893} _key: java.lang.String[] = {java.lang.String[128]@2894} 2 = "/p" _value: java.lang.Object[] = {java.lang.Object[128]@2895} 2 = {org.eclipse.jetty.http.pathmap.MappedResource@2643} "MappedResource[pathSpec=ServletPathSpec["/p/*",pathDepth=2,group=PREFIX_GLOB],resource=default-servlet@2c16bab9==org.ops4j.pax.web.service.jetty.internal.EmbeddedJettyTest$11,jsp=null,order=-1,inst=true,async=true]" pathSpec: org.eclipse.jetty.http.pathmap.PathSpec = {org.eclipse.jetty.http.pathmap.ServletPathSpec@2897} "ServletPathSpec["/p/*",pathDepth=2,group=PREFIX_GLOB]" resource: java.lang.Object = {org.eclipse.jetty.servlet.ServletHolder@2656} "default-servlet@2c16bab9==org.ops4j.pax.web.service.jetty.internal.EmbeddedJettyTest$11,jsp=null,order=-1,inst=true,async=true"

I think you've got 2 differents tests to do here:

url is /context/gateway WITHOUT the trailing slash, which will go to line 377 of ResourceService that does a 302 redirect to url that adds the trailing slash

url is /context/gateway/ WITH the trailing slash, which will go to the code at line 402 of ResourceService

@grgrzybek I'd still like to see one request with GET /context/gateway and one with GET /context/gateway/, because the first one will do a redirect before doing the new include path handling: I just want to ensure that we've exercised both codepaths.

joakime · 2020-07-04T15:58:42Z

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

+                req.getRequestDispatcher("/alt/").include(req, resp);
+            }
+        });
+        context.addServlet(gwholder, "/gateway/*");


This addServlet should be done before Server.start()

I don't think it's the case in other tests inside org.eclipse.jetty.servlet.DefaultServletTest...

joakime · 2020-07-04T15:58:48Z

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

+        FS.ensureDirExists(altRoot);
+        Path altIndex = altRoot.resolve("index.html");
+
+        ServletHolder defholder = context.addServlet(DefaultServlet.class, "/alt/*");


This addServlet should be done before Server.start()

joakime · 2020-07-04T16:00:18Z

I'll let others look at this too, but I feel this should be done against branch jetty-9.4.x (not jetty-10.0.x)

grgrzybek · 2020-07-04T16:09:13Z

I'm not sure about the branch. I'm mostly interested in version 9.4.x, but I see that 10.x is the "current" version being developed.

janbartel

If you can get rid of the unnecessary "if" statement, and make 2 tests instead of 1, we're golden.

janbartel · 2020-07-09T14:30:07Z

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

@@ -401,8 +401,13 @@ protected void sendWelcome(HttpContent content, String pathInContext, boolean en

        if (welcome != null)
        {
+            String servletPath = included ? (String)request.getAttribute(RequestDispatcher.INCLUDE_SERVLET_PATH)
+                    : request.getServletPath();
+            if (servletPath == null)


I think this is a good solution. I would delete the extra "if" at line 406 because that can never happen, unless there's a bug in jetty :)

janbartel · 2020-07-09T14:33:51Z

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

+        HttpTester.Response response;
+
+        // Test included alt default
+        rawResponse = connector.getResponse("GET /context/gateway HTTP/1.0\r\n\r\n");


I think you've got 2 differents tests to do here:

url is /context/gateway WITHOUT the trailing slash, which will go to line 377 of ResourceService that does a 302 redirect to url that adds the trailing slash

url is /context/gateway/ WITH the trailing slash, which will go to the code at line 402 of ResourceService

…e() and non-default mapping Signed-off-by: Grzegorz Grzybek <gr.grzybek@gmail.com>

grgrzybek · 2020-07-10T12:20:41Z

I've just removed the extra if and changed the test, so it uses "strict" mapping for gateway servlet. What's important is not the mapping (and even a request to) for the gateway servlet, but the fact that the servlet calls dispatcher.include()`.

janbartel · 2020-07-15T09:37:28Z

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

+        HttpTester.Response response;
+
+        // Test included alt default
+        rawResponse = connector.getResponse("GET /context/gateway HTTP/1.0\r\n\r\n");


@grgrzybek I'd still like to see one request with GET /context/gateway and one with GET /context/gateway/, because the first one will do a redirect before doing the new include path handling: I just want to ensure that we've exercised both codepaths.

grgrzybek · 2020-07-15T09:50:39Z

I've changed the mapping of the gateway servlet from prefix mapping to strict mapping (from /gateway/* which also handles /gateway requests to plain /gateway). According to Servlet spec, /context/gateway URL is handled by (mapped to) gateway servlet - there's no redirect at all. Trying /context/gateway/ request URL simply gives me 404 error.

My point with this test was to check how /alt/ request works with dispatcher.include() - it wasn't meant to test redirect/lack of redirect for the servlet that acts as a gateway.

grgrzybek · 2020-07-15T09:53:11Z

When I checked:

context.addServlet(gwholder, "/gateway/*");

again, according to my expectations (and Servlet specification), all these requests:

/context/gateway
/context/gateway/
/context/gateway/xxx

are mapped to gateway servlet without redirect and the servlet invokes dispatcher with include for /alt/ path.
But as I said - mapping and checking the gateway servlet itself was not a point of this test - I wanted to test one line:

req.getRequestDispatcher("/alt/").include(req, resp);

:)

janbartel · 2020-07-15T12:28:45Z

@grgrzybek oh, I think we're talking about different things. I was wanting to check the behaviour at line 377 of ResourceService (which sends a redirect if the request didn't end in '/'). But, as I've confirmed via a test, the redirect wouldn't work from an include anyway, as include is not permitted to mess with the response headers.

So, to sum up: we're good! Thanks for the PR.

grgrzybek · 2020-07-15T13:31:04Z

Thanks @janbartel for review! yes - I went through include + redirect attempts myself in pax-web 8 ;)

grgrzybek · 2020-07-15T13:31:41Z

@janbartel May I kindly ask for a backport to Jetty 9.4.x too?

…nd non-default mapping (#5026) Signed-off-by: Grzegorz Grzybek <gr.grzybek@gmail.com> Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel · 2020-07-16T10:29:45Z

@grgrzybek I knew you were going to ask me to do that :). Done, cherrypicked back to 9.4.x branch just now.

grgrzybek · 2020-07-16T10:31:48Z

Ha :) Thanks - much appreciated!

grgrzybek · 2020-07-16T10:34:47Z

By the way - I don't have test yet, but when checking Jetty / Tomcat / Undertow, I saw that Jetty sets wrong value to javax.servlet.include.context_path attribute when using root context. should be empty string, but is "/". I can think about PR later, but maybe already you have idea if it's wrong? (Undertow and Tomcat set it to "").

joakime · 2020-07-16T11:36:51Z

@grgrzybek opened issue #5057 about that

grgrzybek force-pushed the 5025-include-welcome branch 2 times, most recently from c7d114f to 72d3d1d Compare July 4, 2020 13:52

joakime reviewed Jul 4, 2020

View reviewed changes

grgrzybek force-pushed the 5025-include-welcome branch from 72d3d1d to a14a665 Compare July 4, 2020 15:57

joakime reviewed Jul 4, 2020

View reviewed changes

joakime changed the base branch from jetty-10.0.x to jetty-9.4.x July 4, 2020 15:59

joakime changed the base branch from jetty-9.4.x to jetty-10.0.x July 4, 2020 15:59

janbartel requested changes Jul 9, 2020

View reviewed changes

grgrzybek force-pushed the 5025-include-welcome branch from a14a665 to 8439595 Compare July 10, 2020 12:17

Issue jetty#5025 - wrong welcome file handling with dispatcher.includ…

93fa21b

…e() and non-default mapping Signed-off-by: Grzegorz Grzybek <gr.grzybek@gmail.com>

grgrzybek force-pushed the 5025-include-welcome branch from 8439595 to 93fa21b Compare July 10, 2020 12:19

janbartel requested changes Jul 15, 2020

View reviewed changes

janbartel approved these changes Jul 15, 2020

View reviewed changes

janbartel merged commit b08fd18 into jetty:jetty-10.0.x Jul 15, 2020

janbartel added a commit that referenced this pull request Jul 16, 2020

Issue #5025 - wrong welcome file handling with dispatcher.include() a…

668174d

…nd non-default mapping (#5026) Signed-off-by: Grzegorz Grzybek <gr.grzybek@gmail.com> Signed-off-by: Jan Bartel <janb@webtide.com>

joakime mentioned this pull request Jul 16, 2020

javax.servlet.include.context_path attribute on root context. should be empty string, but is "/" #5057

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #5025 - wrong welcome file handling with dispatcher.include() a… #5026

Issue #5025 - wrong welcome file handling with dispatcher.include() a… #5026

grgrzybek commented Jul 4, 2020

joakime Jul 4, 2020

grgrzybek Jul 4, 2020

janbartel Jul 9, 2020

janbartel Jul 15, 2020

joakime Jul 4, 2020

grgrzybek Jul 4, 2020

joakime Jul 4, 2020

joakime commented Jul 4, 2020

grgrzybek commented Jul 4, 2020

janbartel left a comment

janbartel Jul 9, 2020

janbartel Jul 9, 2020

grgrzybek commented Jul 10, 2020

janbartel Jul 15, 2020

grgrzybek commented Jul 15, 2020

grgrzybek commented Jul 15, 2020 •

edited

Loading

janbartel commented Jul 15, 2020

grgrzybek commented Jul 15, 2020

grgrzybek commented Jul 15, 2020

janbartel commented Jul 16, 2020

grgrzybek commented Jul 16, 2020

grgrzybek commented Jul 16, 2020

joakime commented Jul 16, 2020

Issue #5025 - wrong welcome file handling with dispatcher.include() a… #5026

Issue #5025 - wrong welcome file handling with dispatcher.include() a… #5026

Conversation

grgrzybek commented Jul 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime commented Jul 4, 2020

grgrzybek commented Jul 4, 2020

janbartel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgrzybek commented Jul 10, 2020

Choose a reason for hiding this comment

grgrzybek commented Jul 15, 2020

grgrzybek commented Jul 15, 2020 • edited Loading

janbartel commented Jul 15, 2020

grgrzybek commented Jul 15, 2020

grgrzybek commented Jul 15, 2020

janbartel commented Jul 16, 2020

grgrzybek commented Jul 16, 2020

grgrzybek commented Jul 16, 2020

joakime commented Jul 16, 2020

grgrzybek commented Jul 15, 2020 •

edited

Loading