feat(proxy): honour the per-request egress proxy end to end - #20
Merged
Conversation
Three of four proxy surfaces were wired to nothing. `POST /v1/solve`
deserialized `"proxy"` into SolveRequestDto and never read it, `px-cli
solve --proxy` sent it in the body for the server to drop, and the
published `pxsolver_core::SolveRequest::with_proxy()` had zero consumers
in the workspace. `HarvestRequest.proxy` was honoured by CamoufoxPool and
silently ignored by ChromiumoxidePool, which built its BrowserConfig with
no --proxy-server argument at all.
The cause was structural: SolveDispatcher::solve took `&str` and
ChallengeHandler::solve took `&PageHtml`, so no proxy could travel
inward, and both browser handlers built HarvestRequest::new(url), which
defaults proxy to None.
ChallengeHandler::solve now takes a SolveAction { page, proxy } built at
the HTTP edge; SolveDispatcher::solve takes px_core::SolveRequest, making
the published builder the type the edge maps into. Both browser handlers
forward it, ChromiumoxidePool gained --proxy-server, and the native
sensor path posts through a per-proxy reqwest client cached in
ProxyClients.
Solving deliberately does not fall back to the PX_PROXIES rotation: _px3
is bound to the IP that earned it, so a caller who named no egress could
not use a bundle harvested through a rotating one. Rotation stays with
/v1/fetch sessions. The proxy is hashed into the cache key so a bundle
earned through proxy A is never replayed for proxy B; a direct solve
keeps fp_key 0 and existing entries still resolve.
Browser engines cannot authenticate to a proxy — geckodriver's W3C proxy
capability has no credential field and Chromium ignores userinfo without
a CDP Fetch.authRequired handler — so strip_credentials removes
`user:pass@` and warns. reqwest does support it, so the native path is
exempt.
Docs corrected alongside: deployment.md claimed N x len(proxies) parallel
egress paths, but SessionPool::acquire only draws a proxy when spawning,
so distinct egress IPs per domain is min(PX_FETCH_MAX_PER_DOMAIN,
len(PX_PROXIES)). The native runbook told operators PX_PROXIES covered
the solve path, and its soak built a direct client while claiming to run
through their proxy.
Breaking: ChallengeHandler and SolveDispatcher signatures change. See
ADR-0025.
`bump` rewrote only `[workspace.package] version`, leaving the
`px-* = { path, version, package = "pxsolver-*" }` entries at whatever
they were last set to by hand — 1.4.0 since that release, while crates
were publishing at 1.8.0.
Minor bumps hid it: 1.8.0 still satisfies `^1.4.0`. A major bump does
not, so `cargo` fails to resolve the workspace the moment the version
crosses 2.0.0, with no obvious link back to the bump that caused it.
Every crate carries `version.workspace = true`, so the pins are meant to
move in lockstep; bump now rewrites them with the workspace version.
Post-1.0.0 architectural change per ADR-0017: ChallengeHandler::solve and SolveDispatcher::solve change signature so the per-request egress proxy reaches the browser and native paths (ADR-0025). Breaking for downstream users of pxsolver-pipeline, -harvester, -native and -core. Internal dependency pins move to 2.0.0 with the workspace version; they had been stale at 1.4.0 since that release.
…er files AppError::message() returns the payload verbatim as the whole user-facing message, so each one starts a sentence.
Chromium has no `socks5h` scheme and silently ignores a `--proxy-server` spec it cannot parse, so a `socks5h://` proxy — a scheme this API documents and geckodriver accepts — would have gone direct with no error. chromium_proxy_spec rewrites it to `socks5://`, and proxy_arg is now covered: the Chromium leg was the one part of the wiring with no test, and the part that had no proxy argument at all before. SolveRequest also serialized its absent optionals as `null`. The CLI's old private body struct skipped them, so switching it to the published type put `"fingerprint":null` on the wire — a field the server has no place for and silently discards, which is the same class of unkept promise this change set out to remove.
Supersedes the 2.0.0 bump earlier in this branch. Maintainer's call to ship the proxy contract as a minor, amending the ADR-0017 line that a post-1.0.0 architectural change takes a manual major. The break is real and stated rather than hidden: implementors of ChallengeHandler outside this workspace must add the SolveAction parameter, so a `pxsolver-* = "1"` pin fails to compile on cargo update. README carries the warning; ADR-0025 records the trade.
anyhow 1.0.102 → 1.0.104 (unsound Error::downcast_mut) and quinn-proto 0.11.14 → 0.11.16 (remote memory exhaustion from unbounded out-of-order stream reassembly). Both are transitive and predate this branch — main carries the same lock entries and fails the same audit, since the advisory database is fetched fresh on every run. Lock-only; no manifest requirement moves.
Merged
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.
Why
Four proxy surfaces shipped through v1.8.0; only one reached a browser.
PX_PROXIES→ProxyPool→SessionPool/v1/fetchsessions onlyPOST /v1/solvebody"proxy"SolveRequestDto, never readpx-cli solve --proxypxsolver_core::SolveRequest::with_proxy()HarvestRequest.proxyCamoufoxPool, ignored byChromiumoxidePoolThe cause was structural, not a missed call site:
SolveDispatcher::solvetook&strandChallengeHandler::solvetook&PageHtml, so no proxy could travel inward, and both browser handlers builtHarvestRequest::new(url)— which defaultsproxytoNone.ChromiumoxidePool::launch_browserhad no--proxy-serverargument at all.What changed
ChallengeHandler::solvetakes aSolveAction { page, proxy }built at the HTTP edge;SolveDispatcher::solvetakespx_core::SolveRequest, so the published builder is the type the edge maps into.HarvestRequest; Chromium gained--proxy-server; the native sensor path posts through a per-proxyreqwestclient cached inProxyClients.PX_PROXIESrotation._px3is bound to the IP that earned it, so a caller who named no egress could not use a rotating one. Rotation stays with/v1/fetchsessions.0for direct, so existing entries still resolve) — otherwise a bundle earned through proxy A gets replayed for proxy B.407.reqwestcan, so the native path is exempt.socks5h://is normalized for Chromium, which has no such scheme and silently ignores specs it cannot parse.Docs corrected
deployment.mdclaimedN × len(proxies)parallel egress paths.SessionPool::acquireonly draws a proxy when spawning, so it ismin(PX_FETCH_MAX_PER_DOMAIN, len(PX_PROXIES))— 10 proxies at the defaultN=2gives a domain 2 IPs, not 20.user:pass@for paths that cannot authenticate.runbook-native-bypass.mdtold operatorsPX_PROXIEScovered the solve path, and its soak built a direct client while claiming to run through their proxy.Release mechanics
xtask bumponly ever rewrote[workspace.package] version, leaving the internal dep pins at1.4.0since that release. Minor bumps hid it (1.8.0satisfies^1.4.0); the first major bump broke workspace resolution outright.bumpnow re-pins them.ChallengeHandleroutside this workspace must add theSolveActionparameter, so apxsolver-* = "1"pin fails to compile oncargo update. README carries the warning.Verification
cargo fmt --check·cargo clippy --workspace --all-targets --all-featuresunder the unwrap/expect/panic ban, zero warnings ·cargo test --workspace --all-features, 0 failures · 200-LOC gate green.New coverage: proxy reaches the handler (routing), the harvester (cloudflare), the native solver;
--proxy-serverargument shape andsocks5hnormalization; egress-scoped cache entries; absent optionals stay off the wire.Known deviation, unchanged here:
#[allow(clippy::expect_used, …)]remains on test modules becauselefthookruns those denies with--all-targets. The real fix is per-target lint config in the gate script — repo-wide, its own PR.