Skip to content

Clarify servlet response sendRedirect() #275

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

Closed
pmd1nh opened this issue Dec 10, 2019 · 10 comments
Closed

Clarify servlet response sendRedirect() #275

pmd1nh opened this issue Dec 10, 2019 · 10 comments
Labels
Question Further information is requested

Comments

@pmd1nh
Copy link

pmd1nh commented Dec 10, 2019

Hi,

https://jakarta.ee/specifications/servlet/4.0/apidocs/javax/servlet/http/HttpServletResponse.html#sendRedirect-java.lang.String-

The API doc states that "the servlet container must convert the relative URL to an absolute URL before sending the response to the client."

Currently, different server returns different Location header.

Tomcat returns a relative path, for example : Location: /reDirectPath

Jetty, WebSphere Liberty return an absolute URL, for example: Location: http://x.x.x.x:9080/reDirectPath

Can you please clarify the expecting behavior? Perhaps, clearly state it in the next servlet specification/doc.

Thanks,

@pmd1nh pmd1nh changed the title Clarification for servlet response sendRedirect() Clarify servlet response sendRedirect() Dec 10, 2019
@stuartwdouglas
Copy link
Contributor

"the servlet container must convert the relative URL to an absolute URL before sending the response to the client"

The spec is already very clear here. From the linked issue I gather you are actually asking to allow relative URL's as well?

@pmd1nh
Copy link
Author

pmd1nh commented Dec 10, 2019

yes, since all the browsers can support the relative URL in the Location header. If it is allowed, what should be the default? or it is up to the server's implementation.

@stuartwdouglas
Copy link
Contributor

We can't change the default, as it would affect a lot of apps and break backwards compatibility (there are lots of other HTTP clients that are not browsers).

I don't think we want to change the spec here, if an implementation wants to support relative URL's that is fine, they can do it with a config option. If we were going to do it in the spec IMHO it would require a setting in web.xml to enable it, and I don't know if we really need it.

@bshannon
Copy link
Contributor

Does "absolute" just apply to the path portion of the URL? Or does it include the protocol/host/port portion as well? As described in the referenced bug, the latter would be a problem for reverse proxies. Is the reverse proxy intended to "undo" the host mapping and rewrite the header before returning it? Or is the Servlet container supposed to be aware of the host mapping so that it can create a URL with the appropriate protocol/host/port portion that will work with the reverse proxy?

@gregw
Copy link
Contributor

gregw commented Dec 11, 2019

Containers should use the host header (or equivalent headers if Forwarded or X-forwarded support is configured) to determine the scheme, host and port to which are URL should be written.

@stuartwdouglas
Copy link
Contributor

I think think can be closed, I think the current behaviour of the spec is correct and has been working behind reverse proxies for a long time. If containers want to support an option to allow relative redirects that is fine but I don't think it needs to be in the spec.

@markt-asf
Copy link
Contributor

I'll note that Tomcat switched to relative URLs by default (it is configurable and is one of the settings that changes if you put Tomcat into strict spec compliance mode) over 4 years ago and I don't recall any reports of client breakage because of using relative rather than absolute redirects.

Generally, relative redirects work better with reverse proxies. The issues occur when the proxy tries to change the context path but that is generally going to cause a bunch of problems.

I don't think relative vs absolute should be an application concern. I don't think adding sendRedirect(String path, boolean isAbsolute) would be helpful. The application doesn't have the information necessary to make the best decision as to what to pass for isAbsolute.

Could we relax the language in the spec a little? Change that must into a may and add a note that containers may make the behaviour configurable?

See also #100 but I suggest we stick to discussing absolute vs relative here and focus #100 on different redirect codes.

@stuartwdouglas
Copy link
Contributor

stuartwdouglas commented Dec 11, 2019

I am ok with relaxing the spec language, but this will likely also require TCK tests to be updated so this is likely a little bit more involved than just modifying the Javadoc.

@markt-asf
Copy link
Contributor

Indeed. That would mean revisiting this for 5.1/6.0 (depending on what we call it). Do we want to create a label to mark issues we need to defer?

@gregw
Copy link
Contributor

gregw commented Dec 12, 2019

@markt-asf The labels on this project are a bit of a mess and need a good cleanup. The projects are all EE related and while useful I'd also like to see some 5.0, 5.1, 6.0 style projects for clarity. Cleaning up the labels and then assessing all the open issues is a task we should do sooner rather than later... so let's not label this issue just yet and instead let these comments direct actions when that uber task comes along... I'll open an issue to discuss what the labels and projects should be...

@gregw gregw added the Question Further information is requested label Jan 18, 2020
markt-asf added a commit to markt-asf/servlet-api that referenced this issue Nov 22, 2022
markt-asf added a commit to markt-asf/servlet-api that referenced this issue Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants