apply cookies from redirect hops to the hops that follow - #75
Open
liquidsec wants to merge 8 commits into
Open
Conversation
When following redirects, a cookie set by one hop is now sent on the later hops of the same request, which is what a browser does. It's what lets a login or bot-check page resolve: those hand you a cookie along with the redirect, and the cookie has to be on the next request to count for anything. Without it you land back on the same page or loop. The jar is request-scoped. It's created in send_inner and dropped when the request returns, so nothing carries into the next request and two concurrent requests can never see each other's cookies. A batch of 500 URLs runs 500 independent jars, which keeps every result reproducible on its own. Every HTTP path shares send_inner, so request(), the batch and streaming-batch paths, and download() all get this. Cookie selection follows RFC 6265, which matters beyond correctness: a cookie with no Domain is host-only, a Domain that doesn't cover the host that set it is rejected, Path has to match on a segment boundary, and Secure cookies never go over plain HTTP. That's what stops a redirect from being used to walk a session cookie onto an unrelated host. Chain cookies are merged into a caller-supplied Cookie header rather than sent as a second one. Opt out with redirect_cookies=False or --no-redirect-cookies.
Every open PR has been failing Rust Tests at the clippy step, dependabot's
and ours alike, including PRs that touch nothing but a Python dev
dependency. The bumps aren't the cause.
CI installs whatever the latest stable is, with no pin. Stable moved to
1.97.1, whose clippy extended manual_filter to catch this in mock.rs:
files_obj.and_then(|f| if f.is_none() { None } else { Some(f) })
That code is unchanged and has been on dev for a while. It only started
failing because the lint is new and the workflow runs -D warnings, so a
fresh lint turns into a hard error everywhere at once. Locally we were on
1.95, which is why nobody saw it coming.
Rewritten as filter(|f| !f.is_none()), which is what clippy suggests and
is behavior-identical.
The pin is the actual fix for the class of problem. rust-toolchain.toml
rather than a workflow input, so local cargo resolves to the same version
CI uses and this gets caught before pushing instead of after. Keeping
-D warnings is fine once the version is pinned, since new lints then
arrive only when someone bumps it deliberately.
Verified with 1.97.1: fmt, clippy --all-targets --all-features --locked
-D warnings, and cargo test --locked all pass.
It was shipping in the packaged crate, overriding the toolchain for anyone building from source. Also fixes the comment typo.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
When following redirects, a cookie set by one hop is now sent on the later hops of the same request, the way a browser does.
This is what lets a login or bot-check page resolve. Those hand you a cookie along with the redirect, and the cookie has to be on the next request to count for anything. Without it you land back on the same page, or loop. It's a coverage fix: on those targets we were reporting whatever the redirect bounced us to instead of the actual response.
The jar is request-scoped. It's created in
send_innerand dropped when the request returns, so nothing carries into the next request and two concurrent requests can never see each other's cookies. A batch of 500 URLs runs 500 independent jars, so every result stays reproducible on its own. This is deliberately narrower than httpx and requests, where redirect forwarding and cross-request persistence are the same client-wide jar and you can't have one without the other.Every HTTP path funnels through
send_inner, so this coversrequest(),request_batch(),request_batch_stream(), anddownload()without per-path plumbing. It does not apply toraw_connect()(no HTTP layer to read or build) or to requests usingresolve_ip/request_target(that path doesn't follow redirects at all).Cookie selection follows RFC 6265, which matters beyond correctness. A cookie with no
Domainis host-only, aDomainthat doesn't cover the host that set it is rejected,Pathhas to match on a segment boundary, andSecurecookies never go over plain HTTP. That's what stops a redirect from being used to walk a session cookie onto an unrelated host. Chain cookies merge into a caller-suppliedCookieheader rather than being sent as a second one.Pass
redirect_cookies=False, or--no-redirect-cookieson the CLI, to revert to the previous behavior.Covered by 17 unit tests on the jar plus 7 end-to-end tests that drive a real server through a 302 and read the second hop off the socket.
Heads up for anyone diffing scan output: results will move on targets that set a cookie and redirect. That's the fix working.