Skip to content

requests: Fix protocol-relative redirect Locations.#1141

Open
pablogventura wants to merge 1 commit into
micropython:masterfrom
pablogventura:requests-protocol-relative-redirect
Open

requests: Fix protocol-relative redirect Locations.#1141
pablogventura wants to merge 1 commit into
micropython:masterfrom
pablogventura:requests-protocol-relative-redirect

Conversation

@pablogventura

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #931 / commit 8b7a02e, which already rewrites absolute-path
Location values such as /index onto the current host.

That rewrite used startswith("/"), which also matches protocol-relative
URLs (//host/path) and incorrectly prepends the original host. Handle
// first by prefixing only the current scheme so the redirect host is
preserved.

Path-relative Locations without a leading / (for example index) are
still not resolved.

Fixes #931.

Testing

  • Unix port: micropython python-ecosys/requests/test_requests.py passes,
    including a new test_redirect_protocol_relative case.
  • __init__.mpy size with mpy-cross vs upstream/master: 2987 -> 3028
    bytes (+41).

Trade-offs and Alternatives

  • Only protocol-relative (//) Locations are added here. Absolute-path
    (/) support remains as in 8b7a02e.
  • Path-relative Locations without / stay unsupported to keep the diff
    and bytecode small.

Generative AI

I used generative AI tools when creating this PR, but a human has checked the
code and is responsible for the code and the description above.

Handle Location values that start with "//" before absolute-path
rewrites so they keep the redirect host. Fixes micropython#931.

Signed-off-by: Pablo Ventura <pablogventura@gmail.com>
@dpgeorge

Copy link
Copy Markdown
Member

Did you find you needed this in a real use case? Apparently protocol-relative redirects are discouraged these days. So might not be worth supporting.

See https://datatracker.ietf.org/doc/html/rfc3986#section-4.2, which says they are "rarely used".

@pablogventura

Copy link
Copy Markdown
Contributor Author

I did hit this once with a legacy HTTP service that returned Location: //host/.... I do not see it on modern HTTPS setups.

You are right that protocol-relative redirects are rare and discouraged. I am happy to close this PR if you would rather not grow requests for that edge case. Absolute-path redirects (/path) from 8b7a02e already cover the useful part of #931.

@dpgeorge

Copy link
Copy Markdown
Member

I would prefer to close this. If there are more reports about protocol-relative redirects not working, then we can reconsider.

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

Successfully merging this pull request may close these issues.

"requests" does not handle redirection with relative path correctly

2 participants