Surface X-Error-Message response header in HTTP error details#14021
Open
gineshkumar wants to merge 3 commits into
Open
Surface X-Error-Message response header in HTTP error details#14021gineshkumar wants to merge 3 commits into
gineshkumar wants to merge 3 commits into
Conversation
Member
|
Hi @gineshkumar, thank you for your contribution! |
When a server returns an X-Error-Message response header alongside a 4xx or 5xx error, raise_for_status now uses that value as the error detail instead of resp.reason. Servers that do not send the header are unaffected. Closes pypa#14020.
_download was calling response.raise_for_status() (the requests built-in) while the rest of lazy_wheel.py already used pip's helper. Switch to raise_for_status(response) so both call sites raise NetworkConnectionError and both honour the X-Error-Message header.
- Parametrize new tests over 4xx and 5xx status codes (matching existing pip test style) - Assert header value appears in error and reason phrase does not - Cover the lazy_wheel._download path via LazyZipOverHTTP.__new__ and patch.object(_stream_response)
3fdb425 to
2284350
Compare
Author
Tests look good after rebasing, thanks @sepehr-rs. |
Member
|
There is active discussions on using RFC 9457 for index error messages. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Surface
X-Error-Messageresponse header in HTTP error detailsCloses #14020
Problem
pip._internal.network.utils.raise_for_statusbuilds its error message fromresp.reason(the HTTP reason phrase). The reason phrase is an unreliablechannel for actionable error detail:
libraries synthesize a default ("Forbidden", "Not Found") from the
status code when none is present on the wire. In modern deployments
the client almost always speaks HTTP/2 to a CDN or edge proxy, so any
reason phrase set by the origin is lost at protocol translation.
(RFC 7230 §3.1.2).
The result is that users see
403 Client Error: Forbidden for url: ...with no explanation, even when the server has a useful explanation to
give (e.g. "blocked by policy", "package not in this index", "MFA
required").
Change
When a server returns an
X-Error-Messageresponse header on a 4xx or5xx response,
raise_for_statusnow uses that header's value as theerror detail in place of
resp.reason. Servers that don't send theheader see no change in behaviour.
This also aligns the two
raise_for_statuscall sites innetwork/lazy_wheel.py— one was using pip's helper, the other wasusing the
requestsbuilt-in. Both now raise the same exception typeand both honour
X-Error-Message.Alternative considered:
application/problem+json(RFC 7807)RFC 7807 defines a standardised JSON body format for HTTP error details
(
application/problem+jsonwithtype,title,status,detail,instancefields).We chose
X-Error-Messageover RFC 7807 for two reasons:than parsing a JSON body with a new content type — and a single
human-readable string is all
raise_for_statusneeds today.X-Error-Messagefor this; NuGet usesX-NuGet-Warning(samepattern). See raise_for_status relies on HTTP reason phrase, which is unreliable in practice #14020 for details. Matching the convention gives a
server one header that lights up actionable errors across multiple
clients.
If a future change wants richer structured error details, RFC 7807
remains a clean additive option — this header does not preclude it.
What is not in this PR
index/collector.pysimple-index 4xx responses are still logged atlogger.debuglevel via_handle_get_simple_fail. A usefulX-Error-Messageon a 403 from a simple index would still be silentat default verbosity. That's a pre-existing behaviour, separate from
this header change.
network/auth.py's 401 retry path does not readX-Error-Message.Out of scope here; worth a follow-up.
News entry
news/14020.feature.rst(towncrierfeaturefragment).