fix(vgs/http): preserve duplicate response headers such as Set-Cookie - #703
Open
ekluev wants to merge 1 commit into
Open
fix(vgs/http): preserve duplicate response headers such as Set-Cookie#703ekluev wants to merge 1 commit into
ekluev wants to merge 1 commit into
Conversation
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
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.
Problem
VGSHttpResponsekeeps headers in a plain Starlarkdict:Any header that legitimately appears more than once in a response is collapsed to a single
value before a Larky script is ever invoked.
Set-Cookieis the case that matters in practice. RFC 6265 §3 requires oneSet-Cookieheader per cookie and explicitly forbids folding them into one comma-separated line, so an
origin that sets N cookies emits N headers. A RESPONSE-phase filter that reads and writes
input.headers— the normal way to rewriteDomain=/Path=/Securewhen proxying anapplication onto a different hostname — silently drops N-1 of them and the end user is
logged out.
VGSHttpRequestdoes not have this problem: it stores headers inVGSCIMultiDict(
vgs/http/request.starL52-58). Sotype(input.headers)isVGSCIMultiDictin theREQUEST phase and
dictin the RESPONSE phase, and only the request phase can round-triprepeated headers.
This is #292. #497 fixed the request side;
response.starhas not been touched since it wasintroduced in #244, so the issue is still open for responses.
Reproducer
Fix
Use
VGSCIMultiDictforVGSHttpResponse.headerstoo, behind a property, exactly asVGSHttpRequestdoes. The container accepts a mapping, another multidict, or a sequence of(key, value)pairs, so the caller can hand over repeated headers as pairs without anyfurther change.
Two follow-on changes come with it:
remove_header()is added._set_body()has always called it to drop a staleContent-length(issue 16464, inherited fromurllib), but the method was never definedon the response object — the call would have raised. It went unnoticed because the guard
ran against a case-sensitive
dictand the literal key"Content-length"practicallynever matches what an origin actually sends (
Content-Length). With a case-insensitivecontainer the branch becomes reachable, so the method has to exist. Side effect: rewriting
input.bodynow really does drop the staleContent-Length, which is what the commentalways said it should do.
append_header(),has_header(),get_header(),get_all_headers()are added so afilter can read and write multi-valued headers without reaching into the multidict
directly.
add_header()keeps replace-all semantics — same as before this change, and same asVGSHttpRequest.add_header().Compatibility
input.headers = {...}still works: the setter wraps whatever it is given.input.headers.items()/.keys()/.values()/in/[key]all keep working.str(input.headers)now produces the same JSON-ish shape as the request phase.for key in input.headersnow yields a repeated key once pervalue, and
input.headers[key]returns the first value. A filter that rebuilds headerswith the naive
for key in headers: new[key] = headers[key]loop will now emit N copies ofthe first value instead of one copy of the last. That loop was already lossy; it is now
lossy in a way that is visible and fixable from the script (
getall/items), and itbehaves identically to what the REQUEST phase has done since Implement the Starlark index expression assignment operation #497.
Tests
larky/src/test/resources/vgs_tests/http/test_default_response.stargains coverage for:duplicate
Set-Cookiepreservation and ordering, case-insensitive lookup,append_header,add_headerreplace-all,remove_header(including the missing-key no-op), and_set_body()dropping a staleContent-Length.Run with:
Note for the proxy side
This fixes the loss that happens inside Starlark. If the component that constructs
VGSHttpResponsealready flattens the origin's headers into a single-valued map beforehanding them over, it needs a matching one-line change to pass a list of
(key, value)pairs instead — which
VGSCIMultiDictaccepts as-is.Closes #292 for the RESPONSE phase.