test(icaptcha): assert obtain_proof consumes the mocked service#184
test(icaptcha): assert obtain_proof consumes the mocked service#184beardthelion wants to merge 3 commits into
Conversation
Add an integration test that runs obtain_proof against a mockito server and asserts both /v1/challenge and /v1/answer were consumed, so the flow is proven to call the service rather than short-circuit or make a live call. Adds mockito as a dev-dependency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesiCaptcha mock coverage
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jatmn
left a comment
There was a problem hiding this comment.
These assertions only verify that the local mock received one request for each route. A regression can make those two requests and also send the challenge, answer, or configured API key to DEFAULT_URL (or another origin), while this test still passes. That leaves the stated guard against unintended live iCaptcha traffic and token exposure untested. Please either constrain the test's HTTP transport so every non-mock destination fails, or narrow the test name/comments and PR claim to endpoint consumption only.
…al (#184) The mock-consumed assertions only prove the two mock endpoints were hit; they cannot see a request that also leaks the DID, answer, or API key to DEFAULT_URL or any other origin, so the test's stated "no live call" guarantee was untested. Blackhole every non-loopback destination for the flow: NO_PROXY lets the loopback mock through while ALL_PROXY routes any other host to a closed port, so the client contacting anything but the injected mock fails the request. Verified load-bearing: pointing cfg.url at DEFAULT_URL makes obtain_proof fail with a proxy tunnel error (RED), and the loopback-mock path still passes (GREEN).
|
Addressed via your option (a) on |
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Make the non-mock egress block override scheme-specific proxies
crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:26
This still does not guarantee the requested no-live-call behavior on a proxied runner.obtain_proofuses reqwest's default system-proxy configuration, and itsHTTPS_PROXY/https_proxy(or HTTP equivalents) take precedence over theALL_PROXYset here. Therefore an environment with a working HTTPS proxy can let a regression additionally send the DID, answer, or API key toDEFAULT_URLwhile both loopback mock requests are consumed and this test remains green. Override or clear the scheme-specific proxy variables too (restoring the prior environment safely), or enforce the transport policy directly, so the test actually detects that egress.
…all guard covers https The mock-consumed guard blackholed non-loopback egress with ALL_PROXY only, but reqwest honors HTTPS_PROXY/https_proxy (and the http equivalents) OVER ALL_PROXY for https URLs. On a runner whose environment has a working HTTPS_PROXY, a regression could still leak the DID/answer/API key to the https DEFAULT_URL while both loopback mock calls are consumed and the test stayed green. Override the scheme-specific proxy vars (both cases) to the closed port too; NO_PROXY keeps the loopback mock direct. Single #[test] per process, so the override needs no restore. Verified: a throwaway repro showed reqwest routing an https request to HTTPS_PROXY past the ALL_PROXY blackhole (the gap); with the fix the override sends that egress to the closed port even when a working HTTPS_PROXY is pre-set; the guard still goes RED when cfg.url points off-loopback.
|
Confirmed and fixed at The finding is real. I reproduced the precedence directly: with Fix: the test now blackholes the scheme-specific variables too ( Vetted both ways:
|
Adds an integration test that runs
obtain_proofagainst amockitoserver and asserts both/v1/challengeand/v1/answerare consumed. This proves the iCaptcha client actually calls the service rather than short-circuiting or making a live call: a regression that skipped the network would fail the test, since the mock-consumed assertions go red when the endpoints are not hit.Adds
mockitoas a dev-dependency.Verified: the test is load-bearing (red when the mock is under-consumed or the client points elsewhere, green when the service is consumed), and
cargo build --workspace,fmt --check, andclippy -D warningsare clean.Summary by CodeRabbit