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.

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

Fix wording in context root mapping in section 12.2 #300

Closed
markt-asf opened this issue Jan 31, 2020 · 13 comments
Closed

Fix wording in context root mapping in section 12.2 #300

markt-asf opened this issue Jan 31, 2020 · 13 comments

Comments

@markt-asf
Copy link
Contributor

The current wording is:

The empty string ("") is a special URL pattern that exactly maps to the application's context root, i.e., requests of the form http://host:port/<context-root>/. In this case the path info is ’ / ’ and the servlet path and context path is empty string (““).

The requirement that this makes the context path the empty string makes no sense. I propose we strike that text to give:

The empty string ("") is a special URL pattern that exactly maps to the application's context root, i.e., requests of the form http://host:port/<context-root>/. In this case the path info is ’ / ’ and the servlet path is empty string (““).

@grgrzybek
Copy link

Context: https://bz.apache.org/bugzilla/show_bug.cgi?id=64109

I agree. Context Path should always be the context path of given application / war / org.eclipse.jetty.server.handler.ContextHandler / org.apache.catalina.core.StandardContext.

@grgrzybek
Copy link

grgrzybek commented Feb 4, 2020

For documentation purpose, here's my summary for test of this special "" mapping rule from Servlet API 4.0, 12.2 "Specification of Mappings" chapter.

For three container implementations, I've created two contexts - one for / context path and one for /c1 context path.

Jetty

ServletContextHandler rootHandler = new ServletContextHandler(chc, "", ServletContextHandler.NO_SESSIONS);
rootHandler.setAllowNullPathInfo(true);

ServletContextHandler otherHandler = new ServletContextHandler(chc, "/c1", ServletContextHandler.NO_SESSIONS);
// with "false", "/c1" request will be redirected (HTTP 302) to "/c1/"
otherHandler.setAllowNullPathInfo(true);

Tomcat

Context rootContext = new StandardContext();
rootContext.setPath("");
rootContext.setMapperContextRootRedirectEnabled(false);

Context otherContext = new StandardContext();
otherContext.setPath("/c1");
// with "true", "/c1" request will be redirected (HTTP 302) to "/c1/"
otherContext.setMapperContextRootRedirectEnabled(false);

Undertow

DeploymentInfo rootContext = Servlets.deployment().setContextPath("/");

DeploymentInfo otherContext = Servlets.deployment().setContextPath("/c1");

for all the above context I've added a servlet mapped to 4 URI patterns:

  • /p/* (path mapping)
  • *.action (extension mapping)
  • "" (empty string - this special rule from 12.2 chapter of Servlet API spec, meaning "[...] maps to the application's context root")
  • /x (exact mapping)

I didn't use "/" mapping to not hijack other request URIs I wanted to test.

The single servlet just prints 3 things:

  • javax.servlet.http.HttpServletRequest#getContextPath()
  • javax.servlet.http.HttpServletRequest#getServletPath()
  • javax.servlet.http.HttpServletRequest#getPathInfo()

And I send these requests destined for root context:

  • GET /p/anything
  • GET /anything.action
  • GET /
  • GET /x
  • GET /y

and these destined for /c1 context:

  • GET /c1/p/anything
  • GET /c1/anything.action
  • GET /c1
  • GET /c1/
  • GET /c1/x
  • GET /c1/y

Here are summaries (CP=context path, SP=servlet path, PI=path info):

request jetty tomcat undertow
/p/anything cp: ""
sp: /p
pi: /anything
cp: ""
sp: /p
pi: /anything
cp: ""
sp: /p
pi: /anything
/c1/p/anything cp: /c1
sp: /p
pi: /anything
cp: /c1
sp: /p
pi: /anything
cp: /c1
sp: /p
pi: /anything
/anything.action cp: ""
sp: /anything.action
pi: null
cp: ""
sp: /anything.action
pi: null
cp: ""
sp: /anything.action
pi: null
/c1/anything.action cp: /c1
sp: /anything.action
pi: null
cp: /c1
sp: /anything.action
pi: null
cp: /c1
sp: /anything.action
pi: null
/c1
(can't have "" case)
cp: /c1
sp: ""
pi: null
HTTP 404 HTTP 404
/ cp: ""
sp: ""
pi: null
cp: ""
sp: ""
pi: /
cp: ""
sp: /
pi: null
/c1/ cp: /c1
sp: ""
pi: null
cp: /c1
sp: ""
pi: /
cp: /c1
sp: /
pi: null
/x cp: ""
sp: /x
pi: null
cp: ""
sp: /x
pi: null
cp: ""
sp: /x
pi: null
/c1/x cp: /c1
sp: /x
pi: null
cp: /c1
sp: /x
pi: null
cp: /c1
sp: /x
pi: null
/y HTTP 404 HTTP 404 HTTP 404
/c1/y HTTP 404 HTTP 404 HTTP 404

Summarizing special "" mapping rule:

  • for GET /, Tomcat does it according to specification. Jetty returns wrong path info, Undertow returns bad servlet path and path info.
  • for GET /c1, Jetty (with setAllowNullPathInfo(true) preventing redirect to /c1/) behaves rationally but against specification, Tomcat (with setMapperContextRootRedirectEnabled(false) preventing redirect to /c1/) and Undertow return 404, which looks like "can't find mapping for "" == mapping for application's context root (as written in specification)".
  • for GET /c1/ (also with Jetty's setAllowNullPathInfo(false) and Tomcat's setMapperContextRootRedirectEnabled(true)), all containers return /c1 as context path (rational, but against spec) and servlet path + path info are inconsistent

I hope this summary will be a good inspiration for improving the specification ;).

@gregw
Copy link
Contributor

gregw commented Feb 4, 2020

@grgrzybek Great Work!!!

I hope this summary will be a good inspiration for improving the specification ;).
Well at the very least it is inspiration for improving Jetty :)

I think we also want to improve the spec to match jetty for /c1 and tomcat for / and /c1/

@bshannon
Copy link
Contributor

bshannon commented Feb 5, 2020

I hope this summary will be a good inspiration for improving the specification ;).

And the TCK!!!

@bshannon
Copy link
Contributor

bshannon commented Feb 5, 2020

I think we also want to improve the spec to match jetty for /c1 and tomcat for / and /c1/

I'd like to see a column for the "reference implementation" as well, before deciding
which interpretation is "correct". We may not have a reference implementation for
Jakarta EE, but we did for Java EE, and that's what we would've used to resolve
ambiguities or conflicts such as this in the Java EE 8 spec, which this spec is
supposed to be compatible with.

Hopefully, GlassFIsh and Tomcat behave the same due to their common ancestry.

@grgrzybek
Copy link

I'm not involved in a JCP process, so I should not suggest anything ;).
IMO, even if Tomcat (which was RI at some point, right?) handles root context + "" mapping according to spec, it does it with some bad aftertaste.

Also, knowing that:

  • Jetty (with org.eclipse.jetty.server.handler.ContextHandler#setAllowNullPathInfo()),
  • Tomcat (with org.apache.catalina.Context#setMapperContextRootRedirectEnabled())
  • and even Undertow (with proper handler setup)

treat "access root of the context" as something special (security concerns?), maybe the spec should be more clear about it? What does it mean (12.2 Specification of Mappings) "[...] a special URL pattern that exactly maps to the application's context root"? IMO the "context root" is more virtual concept, mapping is always from URI to servlets (or filters), even a "resource" ("default") servlet.

Even "12.1 Use of URL Paths" ends the checklist with default servlet. No way to know what "access context root" means.

What would be the problem specifying that access to "/c1" should always be redirected to "/c1/" and then normal servlet lookup will take place?

@grgrzybek
Copy link

I tried checking Glassfish, but entire eclipse-ee4j/glassfish history related to Tomcat is one entry - can't tell which version of Tomcat it is.
History of javaee/glassfish also looks more like cherry picking from official apache/tomcat repo than something more like alignment with upstream Tomcat version.

@gregw
Copy link
Contributor

gregw commented Feb 5, 2020

What would be the problem specifying that access to "/c1" should always be redirected to "/c1/" and then normal servlet lookup will take place?

I think something should be said about handling of such requests. However, while I think that redirection is probably the best behaviour, the fact that both jetty and tomcat have felt the need to make it configurable indicates that there are probably some users out there that see differently. However I'm not sure they are in numbers large enough to warrant official support in the spec to configure non redirect behaviour. So I'd be fine with either a statement that /context SHOULD or MAY be redirected to /context/.

But note that this is really just a special case of the redirection from /context/path to /context/path/ that occurs if relative URIs are to be used - eg. prior serving a welcome file from a static directory. I'm not aware of anything in the spec that indicates that behaviour, but I believe most servers do it. So perhaps we should have a separate issue to come up with wording for that kind of redirect and then make sure that the context root is handled as a edge case of that?

@bshannon
Copy link
Contributor

bshannon commented Feb 6, 2020

I tried checking Glassfish, but entire eclipse-ee4j/glassfish history related to Tomcat is one entry - can't tell which version of Tomcat it is.
History of javaee/glassfish also looks more like cherry picking from official apache/tomcat repo than something more like alignment with upstream Tomcat version.

Yes, the history there is ugly. My hope was that someone could actually test it
determine how the two behaved.

@arjantijms
Copy link
Contributor

arjantijms commented Feb 7, 2020 via email

@grgrzybek
Copy link

It's technically not "Tomcat", but Catalina and Jasper mostly. Coyote from
Tomcat has been replaced by Grizzly.

RIght. Then I don't think I should add 4th column to the above. the mapper (virtual host mapping, context mapping, servlet mapping) is done at Catalina level (and not Coyote/Grizzly) afaik. So it SHOULD behave as Tomcat. But you never know ;) And it'd really be good to know what Catalina version is used in GlassFish...

@olamy
Copy link
Contributor

olamy commented Feb 16, 2020

maybe would be good to add such test in the TCK? I cannot find any atm (or I missed it?)

markt-asf added a commit to markt-asf/servlet-api that referenced this issue Jan 20, 2023
… path

The context path does not change to "" just because the current request
matches a context root mapping.
markt-asf added a commit to markt-asf/servlet-api that referenced this issue Jan 20, 2023
markt-asf added a commit to markt-asf/servlet-api that referenced this issue Jan 20, 2023
… path

The context path does not change to "" just because the current request
matches a context root mapping.
markt-asf added a commit to markt-asf/servlet-api that referenced this issue Jan 20, 2023
markt-asf added a commit that referenced this issue Jan 27, 2023
The context path does not change to "" just because the current request
matches a context root mapping.
@markt-asf
Copy link
Contributor Author

Fixed via #497

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants