-
Notifications
You must be signed in to change notification settings - Fork 541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: get set-cookie
header with credentials: include
#1454
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1454 +/- ##
=======================================
Coverage 94.37% 94.37%
=======================================
Files 49 49
Lines 4231 4231
=======================================
Hits 3993 3993
Misses 238 238
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
IMHO this is wrong and does not follow the spec. Probably related: Lines 1806 to 1811 in 5478cdd
I think this landed too early and would prefer we properly investigate and follow the spec. This feels more like a hack. |
Ok I'll push a revert, seemed ok at first look. |
revert pushed. |
@ronag I tried implementing it but it wouldn't solve the use case. It only states to parse the cookies. In the rfc itself, it doesn't make any mention of headers/disposing of "invalid" cookies.
So I deemed it a bug with the proxy as it was incorrectly filtering out I'd like to add that the set-cookie header can still exist in the private header map, but the proxy will always see that it's a forbidden header name and return |
The logic added here is not part of the spec. So either we are doing something differently (not good) or there is a bug in the spec. The third option is that we have not implemented something in the spec or done it incorrectly. However this PR does not address that. |
Yep, the spec doesn't allow (note how the cookie(s) are being received because of the warning, but they aren't available to the user) |
Can we raise this to wintercg? |
What does deno and cloudflare do? |
This conversation is happening in two places now. Maybe continue in #1262. |
This reverts commit 5a580ed.
This reverts commit 5a580ed.
This reverts commit 5a580ed.
Fixes #1262
When
credentials
is equal toinclude
, theset-cookie
header(s) should not be stripped.