Cookie Support #1146
Merged
Cookie Support #1146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mre
force-pushed
the
cookie-support-v2
branch
12 times, most recently
from
9d1968b
to
2ae9ac6
Compare
mre
commented
Jul 8, 2023
| @@ -46,7 +46,7 @@ where | |||
| let cache = params.cache; | |||
| let accept = params.cfg.accept; | |||
|
|
|||
| let pb = if params.cfg.no_progress { | |||
| let pb = if params.cfg.no_progress || params.cfg.verbose.log_level() >= log::Level::Info { | |||
There was a problem hiding this comment.
This is here, because showing the progress bar in verbose mode makes the output harder to read. It's not related to the other changes in this PR, but I got annoyed by the output while troubleshooting, so I fixed it in one go.
mre
commented
Jul 8, 2023
|
I am wondering if we should enable cookies by default. So if the user didn't set a custom cookie jar file, we still enable cookie support for this "link check session". After that, we throw the jar away. This would handle more cases, like #715. Opinions? |
stefankreutz
added a commit
to stefankreutz/lychee
that referenced
this pull request
Aug 2, 2023
Commit 14e7487 (cookie support lycheeverse#1146) re-introduced an unconditional dependency on the openssl-sys crate. That is, building Lychee with the Rustls TLS backend now requires OpenSSL. I suppose this change was unintended, maybe due to automatic conflict resolution. If not, please let me know. You can review the re-introduced dependency like so: ``` cargo tree --no-default-features --features rustls-tls -i openssl-sys ``` This commit puts the OpenSSL dependency behind the native-tls feature flag again. You can check the TLS features like so: ``` cargo check --workspace --all-targets --features vendored-openssl cargo check --workspace --all-targets --all-features cargo check --workspace --all-targets --no-default-features --features rustls-tls ``` Maybe this should be added to CI. But I don't want to waste anybody's time.
Merged
mre
pushed a commit
that referenced
this pull request
Aug 4, 2023
* Fix rustls-tls feature Commit 14e7487 (cookie support #1146) re-introduced an unconditional dependency on the openssl-sys crate. That is, building Lychee with the Rustls TLS backend now requires OpenSSL. I suppose this change was unintended, maybe due to automatic conflict resolution. If not, please let me know. You can review the re-introduced dependency like so: ``` cargo tree --no-default-features --features rustls-tls -i openssl-sys ``` This commit puts the OpenSSL dependency behind the native-tls feature flag again. You can check the TLS features like so: ``` cargo check --workspace --all-targets --features vendored-openssl cargo check --workspace --all-targets --all-features cargo check --workspace --all-targets --no-default-features --features rustls-tls ``` Maybe this should be added to CI. But I don't want to waste anybody's time. * Check feature flags during CI Adds a new CI job 'check-feature-flags' to verify the following: - Lychee with rustls-tls feature only doesn't depend on OpenSSL - Cargo check passes with default features - Cargo check passes with all features - Cargo check passes with rustls-tls feature only
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 incookies.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.
My initial goal was to use the same cookie file format that curl uses, but I backed away from this thought for a few reasons:
So, at least for now, we use reqwest_cookie_store under the hood.
Fixes: #645, #715
Partially fixes: #1108 (comment)
Reviews/feedback welcome!
Note: I had to change an unrelated unit test for invalid status codes, because it was flaky, and I wanted the CI pipeline to be green before merging.