-
Notifications
You must be signed in to change notification settings - Fork 248
handle cookies on redirection manually #948
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
Conversation
src/http/Client.zig
Outdated
|
||
// parse and set cookies for the redirection. | ||
redirectionCookies( | ||
transfer.client.arena.allocator(), |
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.
I'm not sure about using this allocator...
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.
Yes. I've had the need for a transfer.arena
a few times before, but I've always managed to work around it.
I'm not opposed to adding one. There's code like https://github.com/lightpanda-io/browser/pull/946/files#diff-897697f048d76304958bca64b03d999c1b5f27c29a1e6d23d9f9a96386bd3b43R261 that could use it
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.
I added an arena to Transfer
+ I removed the arena from the Client which was useless AFAIK
308a928
to
482ecd4
Compare
Using lightpanda-io/demo#57 test file on
Which have This branch sends correctly
new behavior
previous behavior
|
var cookies: std.ArrayListUnmanaged(u8) = .{}; | ||
try cookie_jar.forRequest(&uri, cookies.writer(arena), .{ | ||
.is_http = true, | ||
.is_navigation = true, |
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.
In the Request Interception PR (#946), the Transfer.Request now has a resource_type
enum. I think you would want to to is_navigation = req.resource_type == .document
, because I think it should be false
for scripts and xhr requests (the other 2 types of resource_types we currently have)
src/http/Client.zig
Outdated
|
||
// parse and set cookies for the redirection. | ||
redirectionCookies( | ||
transfer.client.arena.allocator(), |
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.
Yes. I've had the need for a transfer.arena
a few times before, but I've always managed to work around it.
I'm not opposed to adding one. There's code like https://github.com/lightpanda-io/browser/pull/946/files#diff-897697f048d76304958bca64b03d999c1b5f27c29a1e6d23d9f9a96386bd3b43R261 that could use it
482ecd4
to
7795916
Compare
LGTM. Things to do in RI PR after this is merged: |
This PR fixes the cookies handling on redirection.
The original problem was:
This PR de-activate the Curl's cookie engine.
We now set cookies manually in redirection to ensure only once
Cookie:
is set.relates with #940