Enable cookies in wasm32

[jkelleyrtp]

This PR adds cookie support for wasm targets.

[seanmonstar]

Thanks for the PR! Just some minor comments inline.

[happysalada]

hey I'm interested in this, is there anything I can do to help ?

[jkelleyrtp]

I've updated this branch and disabled my toml autoformatter. Hopefully it should clear CI now. Sorry for the wait :) ping @seanmonstar

[mitchelldyer01]

+1 on this -- hope to see it merged soon.

[jkelleyrtp]

@seanmonstar Hey - would love to get your re-review of this since a handful of people have reached out to me about this. Thanks!

[jkelleyrtp]

Bumping @seanmonstar

[JeanMertz]

Are we certain this is working as expected? It might be something else on my side that is not working correctly, but I have an authentication-related test case that works when I run the program in a non-wasm context, but when I compile to wasm, and run it in a browser, the authentication fails. I haven't dug deep yet to figure out what is going on, but the first thing that jumps out at me is that if the cookie jar isn't working as expected (e.g. cookies returned from request 1 need to be set in request 2), the authentication request fails.

I hope to find some more time this week to verify if it is indeed the cookie jar that is behaving differently between non-wasm and wasm, or if it's something else entirely unrelated to this PR.

edit Actually, I did quickly validate that indeed, the cookie jar is empty on the Wasm target, but not on the native local target. I don't know why that's the case yet, it might still be that the first request actually didn't return any cookies to store in the jar, but I find that highly unlikely, since both targets do the same query to the same endpoint.

edit 2 I just validated that indeed, both on the wasm and non-wasm target, the first response has a set-cookie header, but on the wasm target, that cookie is not added to the cookie jar, while that does happen on the non-wasm target.

[JeanMertz]

I might be wrong, but I suspect this is the actual issue that causes this feature not to work as expected.

That is, the current PR adds support for adding cookies to requests that are already present in the cookie jar. However, there's no support yet to actually fill the cookie jar with cookies from the previous response by looking at the set-cookie header.

If we look at the non-wasm implementation here:

reqwest/src/async_impl/client.rs

Lines 1891 to 1900 in e9ba0a9

	#[cfg(feature = "cookies")]
	{
	if let Some(ref cookie_store) = self.client.cookie_store {
	let mut cookies =
	cookie::extract_response_cookie_headers(&res.headers()).peekable();
	if cookies.peek().is_some() {
	cookie_store.set_cookies(&mut cookies, &self.url);
	}
	}
	}

This is the logic that we need to implement for Wasm, which, from cursory reading, would need to happen in the fetch method in this module.

Does that make sense?

[JeanMertz]

I did a bit more digging and tried to implement this myself quickly. I got relatively far, until I got panics because std::time isn't available on wasm32-unknown-unknown.

The offending code starts at line 1897 above (set_cookies), which then goes into the cookie-store crate, which goes to the cookies crate, which then ends up here:

https://github.com/pfernie/cookie_store/blob/579b91247a4096c69a3bb94abd86de5bd6295bef/src/cookie.rs#L199-L207

Which then calls into Cookie::max_age, which returns std::time::Duration, which panics on wasm:

https://github.com/SergioBenitez/cookie-rs/blob/ba60a065ef18af3949966369e4448653c9d25a60/src/lib.rs#L462-L478

This is where I stopped digging, for now, as I ran out of time. There are ways to resolve this (using js-sys::Date, et al.) but I haven't dug into that yet, and it does seem like a fairly large chunk of work to get all the plumbing to work together here, potentially having to update external crates, or re-implementing their logic internally for the wasm target.

For anyone interested in picking this up, here's where I left off, based on the existing changes in this PR:

JeanMertz@7d238da

[pfernie]

Thanks for the detailed summary of your investigation; as you point out the rabbit hole goes down a few levels on this one. I have opened an issue on cookie_store to investigate this, although I do not have time to look at this immediately.

[JeanMertz]

Incidentally, I just stumbled across time-rs/time#491, which might resolve all of these issues 🪄 magically 🪄, if we wait for that PR to land, and update both cookie-store and cookie-rs to use the latest version of time.

I might pull that PR and all relevant crates locally, and see what happens if I run my project with that change included.

[JeanMertz]

The good news is that with that PR in place, things seem to work. There are no panics, and the cookie store does get filled with data.

The potentially bad news is that my wasm test still doesn't pass, whereas the non-wasm one does, and they both do the same number of requests. I can see that the cookie store is way less full on the wasm test, so I still suspect that it might not be storing all set-cookie headers somehow, but this might also be something related to my code.

Regardless, no more panics, and the cookie jar is filled with set-header responses.

[JeanMertz]

So, I figured out the difference between the two platforms.

In the case of the non-wasm target, I get multiple set-cookie headers back, each are individually added to the cookie store. On the wasm targets, those multiple headers are merged into a single set-cookie header, which trips up the cookie store.

After doing down another rabbit hole, I ended up here: https://fetch.spec.whatwg.org/#headers-class.

The standard specifically calls out this erroneous behaviour of the Fetch API:

Unlike a header list, a Headers object cannot represent more than one Set-Cookie header. In a way this is problematic as unlike all other headers Set-Cookie headers cannot be combined, but since Set-Cookie headers are not exposed to client-side JavaScript this is deemed an acceptable compromise. Implementations could choose the more efficient Headers object representation even for a header list, as long as they also support an associated data structure for Set-Cookie headers.

Now, that call-out does mention that the concept of a "header list" does allow multiple Set-Cookie headers, but I'm not quite sure if this is exposed in any way. One way would be to get the actual HTTP message body, and parse the relevant headers from there.

I'll see what's possible here. I'm uncertain if we want to fix this in this PR, a follow-up, or not at all (and consider this "working as intended").

[JeanMertz]

FYI, I pushed a second commit to my branch, that resolves the above issues for me. It should be said that the implementation is specific to my needs (i.e. it uses node-fetch's custom raw() method on Headers).

This isn't something that can be supported in this library as-is, unless we add some way to optionally access raw cookies that result in None if the JS runtime doesn't support this feature (e.g. on the web, when using the native fetch API).

Given all of this, and from what I've come to understand, I assume that for this PR to move forward, we need to wait for the latest release on time, and then (some of) the changes I made in JeanMertz@7d238da.

The lack of support for multiple Set-Header cookies could be a separate issue, or "working as intended" (sadly).

[onweru]

I have tried 48ba15ad56ff8a9e5d65e89d0e3d09abbe2bda37. cargo run & cargo build run flawlessly and produce the desired results. cargo clippy and cargo test throw no method named fetch_credentials_include found for struct reqwest::RequestBuilder in the current scope.

Thank you @JeanMertz for linking your work. It has helped a lot.

[fee1-dead]

Any updates? The crates should have all been updated, right?

[seanmonstar]

I'm not certain what the status is, actually. It sounds like this needs something from cookie_store? Or from time? Or are all those things fine now? Is the rabbit hole @JeanMertz fell in the same thing?

[pfernie]

A new feature flag in time was introduced which (I believe) allows it to build w/ the wasm target. cookie_store has been updated to this version of time, and itself exposes a new feature flag wasm-bindgen for enabling the transitive time/wasm-bindgen feature.

Per #1674 , there is some impedance to making the upgrade; it may simply be the updated time dependency needing a higher MSRV that reqwest currently targets? I have followed up in that issue to see if there is a deeper problem.

I personally don't target wasm, so if there are further changes needed to support this in cookie_store that I am missing, users may comment at the existing pfernie/cookie_store#26

[onweru]

What is the blocker on this? Seems like most of the problems that hindered it being merged have been resolved.

[jkelleyrtp]

I'm still interested in getting this in provided I know we'll get a review.