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

Allow ResourceService sendWelcome to honor isRelativeRedirect #7512

Closed
joakime opened this issue Feb 2, 2022 · 6 comments
Closed

Allow ResourceService sendWelcome to honor isRelativeRedirect #7512

joakime opened this issue Feb 2, 2022 · 6 comments
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement Stale For auto-closed stale issues and pull requests

Comments

@joakime
Copy link
Contributor

joakime commented Feb 2, 2022

Jetty version(s)
9.4.42+

Java version/vendor (use: java -version)
All

OS type/version
All

Description
The changes introduced in issue #6883 made the call to HttpServletResponse.sendRedirect(String path) use relative paths in most static content cases (that don't happen in a dispatch forward/include, and session in url, path params, query merging, etc).

Then relying on the HttpConfiguration.isRelativeRedirectAllowed() to determine behavior (or not).

The creation of the welcome redirect is no longer being created as an absolute location, I think it should honor the HttpConfiguration.isRelativeRedirectAllowed() in this case and produce an absolute location when that configuration is set appropriately.

@joakime joakime added the Bug For general bugs on Jetty side label Feb 2, 2022
@gregw
Copy link
Contributor

gregw commented Feb 2, 2022

@joakime can you explain why? If the ResourceService (and other redirecting handlers (eg ContextHandler)) produce relative links, then the resulting location header will be absolute or not entirely depending on the HttpConfiguration.isRelativeRedirectAllowed(). Moreover, we will have the code to determine an absolute URL in only one place (within sendRedirect) rather than duplicated in several places.

@joakime joakime added the Sponsored This issue affects a user with a commercial support agreement label Feb 2, 2022
@jluehe
Copy link
Contributor

jluehe commented Feb 14, 2022

@gregw, the proposal was to move the logic of whether to make the redirect location full (based on HttpConfiguration.isRelativeRedirectAllowed) inside Response.encodeRedirectURL (instead of Response.sendRedirect), so that by the time Response.sendRedirect is called, it will already be in its final form, thereby making out response wrapper happy, which enforces that only full URLs be passed to sendRedirect ("Cause: java.lang.IllegalArgumentException: HttpServletResponse.sendRedirect must be given an absolute URL, never just a path.").

@gregw
Copy link
Contributor

gregw commented Feb 15, 2022

@jluehe What is the isue with allowing relative paths to be passed? Is it that when converted to absolute URIs within sendRedirect that the wrong port and/or host is sometimes used? If so, aren't the other changes we are making to allow authority overrides sufficient to circumvent this problem?

I'm concerned that is we scatter code that checks isRelativeRedirectAllowed() all over the code then it will be both fragile and incomplete as we will always be vulnerable to new code that doesn't do that. So best to come up with a solution that doesn't rely on forcing the argument to be absolute.

@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Feb 16, 2023
@gregw
Copy link
Contributor

gregw commented Feb 16, 2023

I'll double check this

@jluehe
Copy link
Contributor

jluehe commented Feb 16, 2023

Thanks, @gregw - feel free to close this! @joakime suggested a solution at the time which did exactly what we needed.

@gregw gregw closed this as completed Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

No branches or pull requests

3 participants