fix(icaptcha): harden mock-consumed guard with proxy observer and blackhole tripwire - #214
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds RAII proxy-environment helpers, strengthens mock-consumed egress verification with loopback connection observation, and introduces a tripwire test covering direct success versus blackhole-induced connection failure. ChangesiCaptcha proxy interception tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TripwireTest
participant obtain_proof
participant LocalICaptchaServer
participant BlackholeProxy
TripwireTest->>LocalICaptchaServer: Start local server on 127.0.0.2
TripwireTest->>obtain_proof: Run with proxy disabled
obtain_proof->>LocalICaptchaServer: Request challenge and answer
LocalICaptchaServer-->>obtain_proof: Return JSON responses
TripwireTest->>obtain_proof: Run with blackhole enabled
obtain_proof->>BlackholeProxy: Attempt proxied connection
BlackholeProxy-->>obtain_proof: Return connection failure
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The observer half does what #211 asked. I verified it by mutation at 9c731c8: injecting a swallowed fire-and-forget leak (let _ = client.get("http://leak.example.com/exfil").send();, discarded error) into obtain_proof makes the mock-consumed test go RED with the observer counting one connection, exactly the case the old closed-port blackhole missed. That's the real win here.
The tripwire, though, is vacuous as committed: it passes whether or not the blackhole is armed. Details below, each checked by running it.
Findings
-
[P1] Fix the tripwire fixture's Content-Length so the test actually locks the blackhole
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:54
serve_icaptchadeclaresContent-Length: 78for an 86-byte challenge body and42for a 40-byte answer body. Both are wrong, and it makes the tripwire test nothing. I ran it three ways: with the blackhole disarmed (proxy env cleared) and the committed lengths, obtain_proof returnsErr("parse iCaptcha challenge")because the truncated body fails to deserialize, soassert!(result.is_err())passes with the blackhole off. Correct the lengths to 40/86 and disarmed now returnsOk("PROOF-TRIP"); armed returns a connect error. So the fix is load-bearing and the current test is not. Derive the length from the body instead of hardcoding: bind each body to a variable and usebody.len()in the format string. -
[P2] Assert the tripwire error is a connect/proxy failure, not bare
is_err()
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:109
Even with the lengths fixed,result.is_err()accepts any failure, so a future fixture bug can re-vacuate the test the same way this one did. I confirmed the armed-blackhole error downcasts toreqwest::Errorwithis_connect() == true("tcp connect error: Connection refused"), so narrowing the assertion to that shape is both accurate and self-defending:err.chain().any(|c| c.downcast_ref::<reqwest::Error>().is_some_and(|e| e.is_connect())). As a bonus it fails today's parse-error vacuity directly. -
[P2] State the observer's coverage scope, or drive the branches it can't see
crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:14
The observer only witnesses leaks issued on the one path the test drives: a single passing round, no PoW, no Continue/Failed, no non-2xx. A leak on any of those branches makes no connection during the run and the zero-count assertion stays green, yet the test is named..._makes_no_live_calland the header claims "no leak reached the network" without qualification. It also can't see a transport that ignores proxy env (.no_proxy()or a raw TcpStream). #211 asked for exactly this: either drive those branches or state that the guarantee covers only the exercised path and proxy-honoring transports. Add that caveat to the header (and ideally an observer-armed test driving a Continue round and a non-2xx response). -
[P3] Retitle to
test(icaptcha): ...
The diff is test-only (twotests/files plus amockitodev-dependency; nosrc/change), sofix(would have release-please cut a patch release for it. Test-only changes here usetest(...)(ad7c2b2, c296e23); CONTRIBUTING.md line 55 notes prefixes drive the bump. -
[P3]
arm_blackholeis near-duplicated across the two test files
They differ only in the proxy target (the observer port vs the closed:1). Atests/support/mod.rsexporting onearm_blackhole(target: &str)both files pull in viamod support;removes the duplication. Minor; fine to leave for two small binaries if you'd rather.
One note, not a change request: the Cargo.lock version bumps (0.5.0 -> 0.5.1 across five crates) are not churn from this PR. Those crates' manifests are already 0.5.1 on main; only the lockfile was stale, and regenerating it to add mockito correctly synced them. Leave them as they are. A one-line mention in the PR description that the lockfile picked up an unrelated sync would be tidy, nothing more.
The observer work is a genuine improvement; once the tripwire actually exercises the blackhole it'll be a solid pair.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the blackhole tripwire’s successful fixture valid
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:50
The challenge body is 86 bytes but advertisesContent-Length: 78(and the 40-byte answer advertises 42). If a future client bypasses the proxy, it reaches the local listener but receives truncated JSON, soobtain_proofreturns an error and theis_err()assertion still passes. That leaves the test green with the blackhole disarmed—the exact regression this PR is intended to catch. Derive the header frombody.len()(or otherwise make the direct fixture valid) so an unblocked flow succeeds. -
[P2] Assert that the tripwire failed at the proxy connection
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:109
Even with valid response lengths,result.is_err()accepts any unrelated failure: a malformed fixture, changed request protocol, or solver error would again leave the test green after the proxy guard was bypassed. The test should require the expected connect/proxy failure (for example by finding areqwest::Errorwithis_connect()in the error chain), or include an explicit unarmed positive control.
9c731c8 to
87c716e
Compare
Superseded — re-reviewed the current head 87c716e.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed 87c716e3. The tripwire rework is load-bearing: disarmed, the flow returns Ok("PROOF-TRIP") (a real positive control); armed, it fails with a reqwest connect error. Injecting a swallowed fire-and-forget leak into obtain_proof makes the observer count 1 and the mock-consumed test go red. Content-Length now derives from body.len(), the assertion narrows to a connect error, the observer-scope caveat is stated, and arm_blackhole is deduped into support/mod.rs. Prior findings resolved.
@kevincodex1 ready to merge.
|
hi @Gravirei thaks for this contribution please rebase to main and fix conflicts |
87c716e to
e45bf31
Compare
jatmn
left a comment
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] Rebase this branch onto the current
mainbefore merge
e45bf312033f9a51cba239dcd9cdc383838266d7
The PR head is based one14c2d3f3f4269c61ac8f8ae228223f46d22c078, whilemainis nowb484a24955548257bb4f3fa92f460ba8fd00eafe; GitHub currently marks the PR blocked, and a maintainer has requested a rebase. The current merge tree is clean, so this is branch-state drift rather than a code conflict, but please rebase before merging so the final change can be reviewed against the current base. -
[P2] Synchronize the proxy observer before asserting that no egress occurred
crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:63
The counter is incremented only after the detached accept thread is scheduled, but the test reads it immediately afterobtain_proofreturns. A future fire-and-forget/background request can connect to the listener backlog after that read (or before the accept thread runs), so the test exits green even though the leak this guard is meant to catch occurred. Add a bounded drain/acknowledgement or another lifecycle synchronization before asserting a zero count.
926f978 to
aa0056e
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
[P1] arm_blackhole does not clear REQUEST_METHOD, which disables all proxies
Severity: High
Files: crates/icaptcha-client/tests/support/mod.rs:3-15
Also affects: crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:115, crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:74
In the pinned hyper-util 0.1.20 (src/client/proxy/matcher.rs:228-236), Builder::from_env() sets:
is_cgi: std::env::var_os("REQUEST_METHOD").is_some(),and build() (matcher.rs:304-311) returns an empty matcher when is_cgi is true, ignoring every proxy variable. arm_blackhole sets ALL_PROXY, HTTPS_PROXY, HTTP_PROXY, and NO_PROXY in both cases, but it does not remove REQUEST_METHOD.
Impact: If the test runner, CI wrapper, or developer environment happens to set REQUEST_METHOD, the blackhole is silently disarmed. icaptcha_mock_consumed.rs still passes because its mock lives on 127.0.0.1 and direct loopback access satisfies the mock-consumed assertions; the proxy observer merely sees zero connections, which is indistinguishable from success. Only the tripwire test fails, and then with a confusing "obtain_proof succeeded" message rather than a clear "blackhole disabled" signal.
Fix: Clear REQUEST_METHOD in arm_blackhole:
std::env::remove_var("REQUEST_METHOD");or save/restore it the same way InsecureEnv in src/lib.rs handles GITLAWB_ICAPTCHA_INSECURE.
[P2] ProxyObserver::flush() can under-count leaked connections
Severity: Medium–High
File: crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:96-107
The flush is supposed to guarantee that every leaked connection initiated during obtain_proof has been counted before the zero-leak assertion. It does not provide that guarantee:
- Sentinel-ordering race.
flush()opens a sentinel self-connection and assumes that once the sentinel is accepted, all prior connections have been counted. TCP backlogs are not FIFO; a leaked connection whose handshake completes after the sentinel's can remain in the backlog while the sentinel is accepted first.flush()then readscount == 1, subtracts one, and reports0. - Unconditional
saturating_sub(1). If the self-connect fails (backlog saturated, accept thread stalled), no sentinel is counted, yet line 106 still subtracts one, turning a single real leak into a reported0. - Ignored 100 ms timeout.
recv_timeouterrors are discarded; if the accept thread has not processed the sentinel within 100 ms on a loaded runner, the subtraction again under-counts.
This is particularly problematic because the test is explicitly designed to catch "fire-and-forget leaks whose transport error is discarded" (icaptcha_mock_consumed.rs:16-17). Those leaks are exactly the kind that can still be handshaking when obtain_proof returns, so the flush race undermines the guard's purpose.
Fix direction: Make flush() wait until the count is stable (e.g., repeatedly flush and verify the non-flush count does not change across a bounded interval), or close the listener and drain the backlog before reading the final count. Also subtract the sentinel only when its ack is actually received.
[P3] Blackhole tripwire is not portable to macOS
Severity: Medium
File: crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:69-70
The tripwire binds a listener on 127.0.0.2. The file's own comment notes that macOS may need an explicit loopback alias, but the test uses .expect(...) rather than skipping or failing fast with a helpful message. On a stock macOS runner the test panics before it exercises the blackhole.
Worse, if the bind succeeds but the address is unreachable (or if a future runner configuration changes), the assertion only checks reqwest::Error::is_connect() (icaptcha_blackhole_tripwire.rs:97-102). A direct connect failure to an unreachable 127.0.0.2 is indistinguishable from a proxy-blocked connect failure, so the test can pass for the wrong reason.
Fix direction: Gate the test to Linux, mark it #[ignore = "requires 127.0.0.2 loopback alias"], or add a reachability/positive-control check that verifies 127.0.0.2 is reachable before asserting the blackhole behavior.
[P4] Tripwire lacks a disarmed-blackhole positive control
Severity: Medium
File: crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:66-103
The tripwire asserts that obtain_proof fails when the blackhole is armed, but it never asserts the opposite: that the same request succeeds when the blackhole is disarmed. Without that positive control, a platform where 127.0.0.2 is unreachable (see P3) or a future change that makes obtain_proof fail for unrelated reasons will keep the test green while the guard is not actually being exercised.
Fix direction: Add a companion test (or an initial phase in the same test) that clears proxy variables, verifies 127.0.0.2 is reachable, and asserts obtain_proof returns Ok("PROOF-TRIP").
[P5] arm_blackhole mutates process-global env vars without restoration
Severity: Low–Medium
File: crates/icaptcha-client/tests/support/mod.rs:3-15
The helper overwrites eight proxy-related environment variables and never restores them. This is safe today because each integration-test file contains a single #[test] and Cargo compiles each file as a separate binary. It is fragile, however: adding a second test to either file would leak the mutated env into that test, and any child process or library code that inspects proxy vars inside the test will see the blackhole values.
Fix direction: Convert arm_blackhole into a drop-guard type that captures prior values (or None if absent) and restores them on drop, mirroring InsecureEnv in crates/icaptcha-client/src/lib.rs:392-413.
[P6] Tripwire fixture reads full buffer instead of bytes returned
Severity: Low
File: crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:47-51
let mut buf = [0; 4096];
if stream.read(&mut buf).is_err() { continue; }
let request = String::from_utf8_lossy(&buf);The code discards the byte count returned by read and converts the entire 4096-byte buffer, including trailing NULs. contains("/v1/answer") still works because NUL is valid UTF-8, but the server is technically parsing uninitialized padding. This is brittle if the fixture ever needs to inspect headers or body length.
Fix direction: Use the count returned by read:
let n = stream.read(&mut buf).unwrap_or(0);
let request = String::from_utf8_lossy(&buf[..n]);[P7] Branch is one commit behind main
Severity: Process / Low
Evidence: git log --oneline HEAD..main shows 111cff7 infra: add production fly configs for gitlawb-node-2 (sjc) and gitlawb-node-3 (nrt).
GitHub reports the PR as mergeable, so this is branch-state drift rather than a code conflict. Rebasing onto the current main before merge keeps the final change reviewed against the latest base.
…ackhole tripwire (Gitlawb#211) Replace the closed-port proxy with a ProxyObserver (TCP listener with acknowledged accept-loop flushing) so fire-and-forget leaks whose error is discarded are caught and the count is synchronized before assertion. Add a blackhole tripwire asserting obtain_proof fails against 127.0.0.2 with the proxy armed, narrowed to a connect-level error to prevent vacuous passes. Share arm_blackhole across both test files via a tests/support module. Document the observer's coverage scope: it only witnesses leaks on the single exercised happy path; error/retry branches and non-proxy transports are outside its guarantee.
… positive control, portability, read size (Gitlawb#211)
096c9c1 to
53091ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/icaptcha-client/tests/support/mod.rs (1)
40-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal env-var guards are fragile if a second
#[test]is later added to a binary using them.
EnvGuard/arm_blackhole/disarm_proxy_envmutate process-wide state. Today each consuming test file has exactly one#[test], so there's no race, butcargo testruns multiple tests within a binary concurrently by default — a future second test in the same file calling either helper would race on these env vars. Consider a doc comment noting this constraint, or guarding withserial_test, to prevent a silent regression later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/icaptcha-client/tests/support/mod.rs` around lines 40 - 79, Add an explicit synchronization safeguard around the process-wide environment mutations in EnvGuard, arm_blackhole, and disarm_proxy_env, using the project’s established serial-test mechanism if available. Ensure any tests invoking these helpers cannot run concurrently within the same binary; otherwise document the single-test-per-binary constraint directly on the helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/icaptcha-client/tests/support/mod.rs`:
- Around line 40-79: Add an explicit synchronization safeguard around the
process-wide environment mutations in EnvGuard, arm_blackhole, and
disarm_proxy_env, using the project’s established serial-test mechanism if
available. Ensure any tests invoking these helpers cannot run concurrently
within the same binary; otherwise document the single-test-per-binary constraint
directly on the helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9638444-b11b-415e-b33c-d3fbbe31d202
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rscrates/icaptcha-client/tests/icaptcha_mock_consumed.rscrates/icaptcha-client/tests/support/mod.rs
jatmn
left a comment
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] Parse the complete HTTP request before selecting the fixture response
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:51
A TCP read is not message-framed, so it can return before the complete request line (including/v1/answer) arrives. The fixture then treats an answer request as a challenge request and returns challenge JSON; the direct positive control fails to deserializeAnswerResulteven though the proxy behavior is correct. Read through the HTTP headers/request line before dispatching, or use an HTTP test server, so normal TCP segmentation cannot make this tripwire intermittently fail.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one low-priority test-coverage issue.
Findings
- [P3] Narrow the observer's fire-and-forget guarantee
crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:82
stabilisedeclares success after the first 100 ms interval in which the detached accept loop has not incrementedcount. A regression can spawn a fire-and-forget proxy request on the exercised path just beforeobtain_proofreturns, but schedule its connection after that interval; both samples are zero, the test passes, and the leaked DID/answer/API-key request is never observed. Conversely, a repeating leak that keeps changing the counter never lets this unbounded loop return. That makes the new test non-load-bearing for the discarded-error/background egress it says it detects. Use a bounded acknowledgement/lifecycle barrier for the request work being tested (or narrow the assertion to connections already accepted), rather than treating one quiet polling interval as proof that the flow has no outstanding egress.
…re/after obtain_proof The stabilise() polling loop cannot prove no more connections are coming, only that none arrived in the last 100ms. A carefully timed background leak could schedule its connection after stabilise stabilises and the test would pass despite a live egress. Replace with a simple counter read before and after obtain_proof. Connections accepted during the synchronous call are counted; a leak whose TCP handshake completes after the counter is read is outside the assertion window — this is now documented as a known limitation of the single-threaded test harness. Also narrow the fire-and-forget claim: the observer catches leaks that a closed-port blackhole would miss (because it counts connections regardless of error propagation), but the timing race inherent in a detached accept thread means the guarantee is best-effort for post-return handshakes.
jatmn
left a comment
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] Do not let the blackhole tripwire report success when it never ran
crates/icaptcha-client/tests/icaptcha_blackhole_tripwire.rs:96
WhenTcpListener::bind("127.0.0.2:0")fails (the documented macOS / missing-loopback-alias case), the test printsSKIPand returns. Cargo still reports the case as passed, so a platform that cannot bind127.0.0.2gets zero coverage of the negative control while CI stays green. That recreates the original gap this tripwire was added to close: a future.no_proxy()or connector bypass can ship undetected anywhere the test silently no-ops. Please fail fast with a clear message, gate the test with#[cfg(...)]/#[ignore = "..."], or otherwise make skipped platforms visible instead of counting them as success. -
[P2] Synchronize the proxy observer before asserting zero egress
crates/icaptcha-client/tests/icaptcha_mock_consumed.rs:124
The latest head removesstabilise()and reads the counter immediately afterobtain_proofreturns, while the accept loop still runs on a detached thread. A fire-and-forget leak whose TCP handshake is still in the listener backlog—or completes just after the synchronous call returns—can therefore pass withleak_count == 0, which is exactly the class #211 asked this guard to catch. The module comment now documents a post-return window, but that narrows the guarantee rather than replacing the lifecycle barrier previously requested. Please add a bounded drain/acknowledgement (or join/barrier with the accept thread) before the zero-count assertion, or narrow the test name/docs to match what is actually proved. -
[P3] Document the single-test-per-binary constraint on env helpers
crates/icaptcha-client/tests/support/mod.rs:48
arm_blackholeanddisarm_proxy_envmutate process-wide proxy variables and restore them on drop. Each consuming integration test binary currently has exactly one#[test], so this is safe today, butcargo testruns multiple tests in the same binary concurrently by default. A future second test in either file would race on these vars withoutserial_testor an explicit comment on the helpers. Please document that constraint on the public helpers (or add the project's serial-test guard if one exists). -
[P3] Retitle to
test(icaptcha): ...
PR title / lead commit
The diff is test-only (tests/plus lockfile sync; nosrc/change). This repo uses Conventional Commits prefixes for release-please versioning, and prior test-only icaptcha work here usestest(...).fix(...)can trigger an unnecessary product patch release for harness-only changes.
Summary
Hardens the iCaptcha mock-consumed guard with a proxy observer (catches fire-and-forget leaks) and a blackhole tripwire (detects if the proxy guard is silently disarmed).
Motivation & context
Closes #211
Kind of change
What changed
ProxyObserver(TCP listener counting connections) so fire-and-forget leaks whose errors are discarded are caughtobtain_prooffails against127.0.0.2while the proxy is armed; goes RED if.no_proxy()or a custom connector is addedmockitodev-dependencyHow a reviewer can verify
cargo test -p icaptcha-client cargo clippy -p icaptcha-client --all-targets -- -D warnings cargo fmt --check -p icaptcha-clientBefore you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare clean.env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
N/A - test-only crate change
Summary by CodeRabbit