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

Add cookie support? #645

Closed
mre opened this issue Jun 8, 2022 · 11 comments · Fixed by #1146
Closed

Add cookie support? #645

mre opened this issue Jun 8, 2022 · 11 comments · Fixed by #1146
Labels
enhancement New feature or request

Comments

@mre
Copy link
Member

mre commented Jun 8, 2022

Over at #574 I noted that checking https://sms-digital.com would not work without cookie support. curl has the same problem, but they have cookie support with --cookie. We could do the same in lychee. reqwest supports that.

We'd have to enable the "cookies" crate feature, construct a Client with cookie_store(true) and then place a cookie into its cookie jar using https://docs.rs/reqwest/latest/reqwest/cookie/struct.Jar.html#method.add_cookie_str (source).

Should this feature be enabled by default to avoid confusion on failed requests?

@mre mre added enhancement New feature or request waiting-for-feedback labels Jun 8, 2022
@egrieco
Copy link

egrieco commented Jun 17, 2022

I don't know about enabled by default, but maybe provide some guidance to the user that --cookie is an option if they encounter authorization errors.

Probably a good idea to give them some guidance on how to get cookies from their browser into the lychee cookie store as well. There are some extensions that can make this easier.

@mre
Copy link
Member Author

mre commented Jun 21, 2022

You mean something like https://addons.mozilla.org/en-US/firefox/addon/cookies-txt/ and https://chrome.google.com/webstore/detail/get-cookiestxt/bgaddhkoddajcdgocldbbfleckgcbcid?

@egrieco
Copy link

egrieco commented Jul 16, 2022

Yes. I think trying to handle auth and such within lychee, especially with modern all JS sites will be far more trouble than it's worth.

Having the ability to extract the necessary cookies from the browser and pass them to lychee via a file or config option should be adequate.

@lebensterben
Copy link
Member

getting cookies from browser is a very bad idea.

There might not be browser, for example in CI.

How do you deal with multiple browser or multiple user accounts of a browser?

How do you prevent accessing potential private information in cookies?

@mre
Copy link
Member Author

mre commented Jul 16, 2022

Where the cookies come from might be irrelevant to lychee. The scenarios you mention are out of scope for lychee itself and should be the user's responsibility.

At least curl has ways to set cookies through the cli and their cookiejar file. We could support the same behavior and even the same file format.

We also need to separate between storing cookies we receive and setting cookies provided by a user. Right now we don't handle cookies we receive. There could be a setting for allowing server-provided cookies, e.g. --accept-cookies or --store-cookies. To set a cookie we could provide --cookie similar to curl.

@lebensterben
Copy link
Member

the way curl handles cookies has nothing to d with browser. It simply takes a given cookie file.

what I commented above is regarding

Having the ability to extract the necessary cookies from the browser

I don't think lychee should ever need to talk to the browser.

@mre
Copy link
Member Author

mre commented Jul 16, 2022

Oh, yeah I agree with that. Not sure if @egrieco meant that.
lychee's cookie support will be very basic: manually set cookies, optionally accept cookies from websites. That's it. 👍

What do you think about the parameter names I proposed? Shall we change/remove any of them or do they sound okay to you? Is anything missing?

@lebensterben
Copy link
Member

I don't have any opinion on that right now.

@egrieco
Copy link

egrieco commented Jul 25, 2022

No, I don't think lychee should talk to the browser. Though I do think that it should be able to accept cookies that the user has exported from the browser (by some means or other) and provide them to hosts it contacts. This is necessary to receive correct statuses when testing links that require the user to be logged in.

The --cookie option is what I had in mind.

As far as --accept-cookies or --store-cookies I guess I tend slightly toward the latter, though it might make sense to mirror curl with the --cookie-jar option. Would make it easier to remember and understand for those already familiar with curl.

@lebensterben
Copy link
Member

yes --cookie-jar is a good name. First it mirrors curl. second, various language has http.cookiejar, e.g. python, ruby, perl, etc.

@mre
Copy link
Member Author

mre commented Nov 6, 2022

https://github.com/pfernie/reqwest_cookie_store is an implementation of a cookie store for reqwest.

@mre mre mentioned this issue Jul 8, 2023
@mre mre closed this as completed in #1146 Jul 13, 2023
mre added a commit that referenced this issue Jul 13, 2023
This is a very conservative and limited implementation of cookie support.

The goal is to ship an MVP, which covers 80% of the use-cases.
When you run lychee with --cookie-jar cookies.json, all cookies will be stored in cookies.json, one cookie per line.
This makes cookies easy to edit by hand if needed, although this is an advanced use-case and the API for the format is not guaranteed to be stable.

Fixes: #645, #715
Partially fixes: #1108
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants