Skip to content
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

WASM cookies #2360

Closed
wants to merge 11 commits into from
Closed

WASM cookies #2360

wants to merge 11 commits into from

Conversation

c-git
Copy link

@c-git c-git commented Jul 17, 2024

I got cookies working for WASM to the best of my knowledge. I tested it in my use case and it works. I'm not sure how to add tests for the cookie functionality I added for WASM.

I also have one part of the code that I'm unsure of because I didn't see a clear translation between the native version and the WASM version.

Please let me know what you need me to do or change anything (including if the code I mentioned that I was unsure of is ok).

Edit:

If you need this functionality before the PR lands you can add the following to the bottom of your Cargo.toml. If you need me to rebase to keep up with changes to reqwest let me know (replying here would work as well) as I may not update unless a reason comes up such as I'm doing an updates on my dependencies or a security fixed comes out.

# https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#testing-a-bugfix
[patch.crates-io]
reqwest = { git = "https://github.com/c-git/reqwest.git", branch = "wasm-cookies" }

@c-git c-git changed the title Wasm cookies WASM cookies Aug 20, 2024
@c-git
Copy link
Author

c-git commented Aug 20, 2024

I rebased the PR and made a few lint related commits. I reviewed the code again in light of #1449 and they are very similar. I put a lot more of the code in wasm/client.rs but for the most part they seem to have a lot of the same things. Before this PR lands I would appreciate some feedback on:

  • If there is a way I should add tests for this
  • If the fact that we do not capture cookies during redirect loops is a problem. This is the native code that does not have a WASM counterpart. TODO for follow up on this.
  • There is a clone of url that isn't needed but I left it to keep the API as similar as possible between WASM and Native. But the function is private so it's likely not a problem to make the change. I did a sample of the version without the clone here here.
  • There is a clone that I did here to get around a lifetime restriction that I couldn't find a way to remove without making the code less easy to read.

@c-git
Copy link
Author

c-git commented Aug 20, 2024

PS. Maybe somewhere we should add a warning that WASM cookies won't work for WebSockets because according to

https://devcenter.heroku.com/articles/websocket-security#authentication-authorization

Since you cannot customize WebSocket headers from JavaScript, you’re limited to the “implicit” auth (i.e. Basic or cookies) that’s sent from the browser. Further, it’s common to have the server that handles WebSockets be completely separate from the one handling “normal” HTTP requests. This can make shared authorization headers difficult or impossible.

The emphasis above is mine. The way cookies are implemented right now for WASM is very similar to how it is done for native which means in the browser it has to use JavaScript to set the cookies in the header which is not allowed for WebSocket requests.

Namely
- impl fmt::Debug for ClientBuilder
- impl fmt::Debug for Client
Didn't fit well with merging of headers as the headers needed to be
re-borrowed anyway to get url
Resolved dead_code warning and was useful even if not to me
clippy::redundant_pattern_matching
@c-git
Copy link
Author

c-git commented Dec 11, 2024

I think the current version PR as is still be helpful for more than just me.and the Websocket issue is kinda out of reach without trying to use the browsers cookie store. If we could merge this I'd appreciate it as I need it and don't really want to have to keep maintaining a fork.

@jlizen
Copy link
Contributor

jlizen commented Dec 30, 2024

@c-git i'd recommend reaching out to sean on the #reqwest channel in the tokio discord to discuss a review directly - https://discord.gg/PWCbbMcy

cc @seanmonstar as well

@c-git
Copy link
Author

c-git commented Dec 30, 2024

@c-git i'd recommend reaching out to sean on the #reqwest channel in the tokio discord to discuss a review directly - https://discord.gg/PWCbbMcy

cc @seanmonstar as well

Thank you I will do that.

@seanmonstar
Copy link
Owner

Thanks for working on this! I've wondered a bit, since reqwest on wasm is backed by fetch, can we really manage cookies separately? Doesn't the browser do all of that internally? It at least looks like we can't adjust the Cookie header...

@c-git
Copy link
Author

c-git commented Dec 31, 2024

Thanks for working on this! I've wondered a bit, since reqwest on wasm is backed by fetch, can we really manage cookies separately?

I don't think we have a choice. The browser does not do it automatically. I'm not sure if there is a way to get it to do it for fetch but from what I've seen in testing it doesn't. There may be a way to try to get the browser to do it but I don't know how. Even if we could find a way to do that non-browser environments may still have an issue. I don't know that to be the case because for me my use case is in the browser.

Doesn't the browser do all of that internally?

It doesn't do it automatically (as I said not sure if there is a way to get it to).

It at least looks like we can't adjust the Cookie header...

I see what you mean and we cannot for WebSocket requests but for POST and GET which I tested we can.

If it would help I can setup a test backend and client so we can see what happens.

@seanmonstar
Copy link
Owner

That's surprising. When I go to httpbin.org, open the browser console, and try out await (await fetch("/cookies/set/hello/world")).json(), and peek in the Network tab, I see that the cookies are sent in the follow up request headers.

@c-git
Copy link
Author

c-git commented Dec 31, 2024

Thank you, for that information. I'll try to replicate what you did and then try my code again without cookie support in WASM and see what happens with the cookies more than just, tried and it doesn't work.

That would actually be great. Feature flags in my code would be much easier to maintain than the fork I'm keeping going right now.

@c-git
Copy link
Author

c-git commented Jan 2, 2025

Got tied up with other tasks and haven't been able to finish more detailed testing yet but it does seem that the browser does automatically manage the cookies (Sorry I'd gotten that wrong). Thus, personally I suspect that I no longer need this PR and didn't realize because it was working as expected just not why I expected.

I'll still try to finish the testing as soon as I can and report back to see if this PR actually does anything at all because, based on the docs you referenced, setting cookie headers isn't allowed so maybe it doesn't do anything at all. I don't know if reqwest would even work in other WASM contexts and don't personally have a need to use any other WASM contexts right now.

So far I tested disabling cookies and just running my app and it still works and the cookies show up in the browser dev tools.

I want to test:

  • Attempting to set a different cookie and see if both are sent
  • If I can access the cookies set using the reqwest::Client

@c-git c-git marked this pull request as draft January 5, 2025 02:59
@c-git
Copy link
Author

c-git commented Jan 6, 2025

I setup a small test server and cookies are set, unset and sent but they are done by the browser. There is no cookie information in the incoming requests that reaches my code. If I set cookies manually in code they also do not reach the server. I'm sorry to have wasted your time with this. Have a good year.

@c-git c-git closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants