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 append_slash_redirect when PATH_INFO has internal slashes #2338

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Fix append_slash_redirect when PATH_INFO has internal slashes #2338

merged 2 commits into from
Mar 16, 2022

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Feb 12, 2022

(This seems to have been a contentious issue — please don't hate me.)

This addresses an issue — reported before in #1972 — namely that werkzeug.utils.append_slash_redirect does not work correctly if PATH_INFO contains internal slashes.

Suppose a request comes in for http://example.com/app/foo/bar. Assume this makes it to the WSGI app with environ["SCRIPT_NAME"] = "/app" and environ["PATH_INFO"] = "/foo/bar". The app, in order to append a slash to the URL returns append_slash_redirect(environ). As things stand, this issues a redirect to the relative path "foo/bar/". That path gets interpreted relative to the base URL implied by the original request ("http://example.com/app/foo/bar") resulting in a final location of "http://example.com/app/foo/foo/bar/". This is not quite what is wanted — one foo is enough for us.

The solution is to redirect to a relative path (as is currently done), but redirect to a relative path that contains only the trailing component PATH_INFO.

This PR fixes that and includes new tests that exercise the problem.

Relation to Prior Work

Note that this issue is subtly different than that purportedly "fixed" by PR #1538. PR #1538, as demonstrated by the tests included in that PR wants to redirect to an absolute path based on PATH_INFO. This results in (incorrectly) discarding any leading path components that may be in SCRIPT_NAME.

There is also #1842. Due to its brevity, it is unclear precisely what issue is being described therein, but the proposed fix is the same as the (incorrect) solution proposed in #1538.

Issues Fixed

Checklist

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • [n/a] Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • [n/a] Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism
Copy link
Member

davidism commented Mar 16, 2022

Thank you, this finally makes sense. The issue is that Request.autocorrect_location_header uses the urljoin behavior to join the relative redirect URL to the full request path. The full path includes both SCRIPT_NAME and PATH_INFO, but the redirect was also using the entire PATH_INFO. Relative URL join behavior is to replace only the last segment of the path with the new relative path, so append_slash_redirect needs to only return that last segment.

@davidism davidism added this to the 2.1.0 milestone Mar 16, 2022
@dairiki
Copy link
Contributor Author

dairiki commented Mar 16, 2022

@davidism Yes, I think you have found exactly where the urljoin happens. (That is the correct behavior — relative URLs should be interpreted relative to the full request path. Of note, it would also be correct to pass the relative URL in the Location header back to the browser unmolested — relative URLs are allowed in the Location header, and they are to be interpreted relative to the request URL. Ref: RFC 7231.)

Anyhow, I've just rebased this PR onto the current main.

@davidism davidism merged commit 4c3cde7 into pallets:main Mar 16, 2022
@dairiki dairiki deleted the bug.1972-append_slash_redirect branch March 16, 2022 23:36
dairiki added a commit to dairiki/lektor that referenced this pull request Mar 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

function append_slash_redirect misredirects
2 participants