fix: strip every credential header on a cross-origin redirect - #8
Merged
Merged
Conversation
Three places decide what counts as a credential header: the trace redactor, the response cache key, and the cross-origin redirect strip. They kept separate lists and drifted. The redirect strip named five headers where the redactor named seven, so X-Access-Token survived a redirect and was sent to whatever host the Location header pointed at. The strip now erases by predicate over the single list rather than matching a second hand-written copy of it, which also fixes the case sensitivity: it compared names verbatim, so a header spelled "x-api-key" rather than "X-API-Key" was kept. x-csrf-token, apikey and ocp-apim-subscription-key are added to that one list, so all three uses gain them together. The test sends six credential headers through a 302 to a second local server and asserts none of them arrive. Before this change X-Access-Token did.
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
1 participant
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.
Three places decide what counts as a credential header — the trace redactor, the response cache key, and the cross-origin redirect strip. They kept separate lists and drifted.
The redirect strip named five headers where the redactor named seven, so
X-Access-Tokensurvived a redirect and was sent to whatever host theLocationheader pointed at.Reproduced
Six credential headers through a 302 to a second local server:
Change
x-api-keywould have been kept whereX-API-Keywas stripped.x-csrf-token,apikeyandocp-apim-subscription-keyadded to that one list, so all three uses gain them together.Tests live in erpl-web (
test/cpp/test_http_redirect_credentials.cpp) becausehttp_client.cppneeds DuckDB's vendored httplib. erpl-web: 430 cases / 2171 assertions green.Found by an agent-crew review.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.