Skip to content

Follow every redirect status and method in passthrough_redirect_handler - #1247

Open
chrikrah wants to merge 1 commit into
prometheus:masterfrom
chrikrah:fix/passthrough-redirect-307-308-delete
Open

chrikrah wants to merge 1 commit into
prometheus:masterfrom
chrikrah:fix/passthrough-redirect-307-308-delete

Conversation

@chrikrah

Copy link
Copy Markdown
Contributor

passthrough_redirect_handler is documented to follow redirects for all HTTP methods, and its docstring says "any request method". It repeats the request, method and data included, but only for 301, 302 and 303 on POST and PUT. A 307 or 308 raises HTTPError, and any redirect of a DELETE does too.

# base ff9a2d3, python 3.10.21 and 3.12.6 print the same; repro_redirect.py redirects
# /metrics/ to /final and calls push_to_gateway, pushadd_to_gateway and delete_from_gateway
$ python repro_redirect.py
301 PUT    -> ok
301 DELETE -> HTTPError: HTTP Error 301: Moved Permanently
307 PUT    -> HTTPError: HTTP Error 307: Temporary Redirect
308 POST   -> HTTPError: HTTP Error 308: Permanent Redirect
$ python repro_redirect.py | grep -c ok   # this branch, 5 statuses x 3 methods, both interpreters
15

The check copies the urllib rule that #622 added the handler to get around ("for all method types"). An nginx return 307 in front of a pushgateway sends a 307. Before Python 3.11 urllib never routes a 308 here, so the class sets http_error_308 itself.

$ python -m pytest -q   # 3.10.21 and 3.12.6, same counts
430 passed, 14 skipped, 28 subtests passed   # this branch
427 passed, 14 skipped, 28 subtests passed   # base ff9a2d3
$ python -m pytest tests/test_exposition.py -q   # 3.10, exposition.py reverted, tests kept
3 failed, 98 passed, 1 skipped, 3 subtests passed
$ python -m pytest -q tests/test_exposition.py -k redirect   # 3.10, without http_error_308
1 failed, 3 passed, 98 deselected
$ flake8 prometheus_client/ tests/   # flake8 7.5.0
$ isort --check-only prometheus_client tests   # isort 9.0.2
$ mypy prometheus_client   # mypy 2.4.1
Found 5 errors in 2 files   # identical at base
# not run: 3.9 and pypy3.9

No issue exists for this. @csmarchbanks, DELETE now follows on 301 to 303 too, because the docs say all methods. Want it narrowed to 307 and 308?

The handler is documented to follow redirects for all HTTP methods and to repeat the original request, method and data included. It only did that for 301, 302 and 303 on POST and PUT, so a 307 or 308, the statuses that exist to keep the method, raised HTTPError, and so did every redirect of a DELETE.

Signed-off-by: Christopher Krah <48338417+chrikrah@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

1 participant