test(gl): add tests for iCaptcha transparent retry flow (#167)#168
Conversation
Gitlawb#126) The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating only reachable blobs), so a dangling/unreachable blob was absent from the set and served in cleartext to anonymous callers. Flip to an allowed-set (allowed_blob_set_for_caller) that enumerates reachable blobs the caller may read: a dangling blob has no path, is never in the set, and 404s.
Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object.
…tion ✓ P3 blocker fixed: cargo fmt applied — the format gate will pass. ✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays. • P2 follow-up: Tree/commit disclosure tracked in Gitlawb#135 — out of scope here.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR tightens iCaptcha solver trust to exact origins, including ports, and adds NodeClient tests for configuration, signing, proof retries, and retry exhaustion. It also removes a redundant borrow from profile pin-name formatting. ChangesiCaptcha Trust and Client Behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GlClient as gl NodeClient
participant Node as gitlawb-node
participant Icaptcha as iCaptcha Server
GlClient->>Node: send_signed(request)
Node-->>GlClient: 403 iCaptcha challenge
GlClient->>Icaptcha: request challenge
Icaptcha-->>GlClient: challenge
GlClient->>Icaptcha: submit answer
Icaptcha-->>GlClient: proof
GlClient->>Node: retry with x-icaptcha-proof
Node-->>GlClient: successful response
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
added
crate:gl
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Thanks for adding coverage here. The blocker is that the two "full integration" tests don't exercise the iCaptcha retry path they're named for — they fall through to the real production service — so the transparent-retry flow has no real coverage.
I verified by execution: the mockito iCaptcha server is never contacted, and the solve resolves to DEFAULT_URL (https://icaptcha.gitlawb.com) and makes a live network call. Root cause: is_https gates the localhost allowance behind #[cfg(test)], but cfg(test) is not set for icaptcha-client when it's compiled as gl's dependency, so that branch is compiled out during cargo test -p gl. With is_https(http://127.0.0.1:port) false, resolve_solver_url drops both the operator env URL and the advertised header URL and falls back to DEFAULT_URL. Pointing DEFAULT_URL at a bogus host makes both tests fail with a DNS error on {DEFAULT_URL}/v1/challenge; removing the relaxation entirely leaves the tests passing, confirming it's inert.
Findings
-
[P1] Make the integration tests exercise the mock, not the live service.
crates/gl/src/http.rs(send_signed_solves_icaptcha_and_retries_to_success, send_signed_returns_403_after_icaptcha_retries_exhausted). Both resolve the solve to https://icaptcha.gitlawb.com and make a real network call — the mock's /v1/challenge and /v1/answer are never hit, and with DEFAULT_URL pointed at a bogus host both fail with a DNS error. So they're network-dependent (fail in a no-egress CI, hit production on every run) and the retry flow is uncovered; the exhaustion test even panics on the live-call error before its assert_eq!(403), so retry-exhaustion is untested. The #[cfg(test)] approach can't work because cfg(test) doesn't propagate to a dependency. Test the localhost solve inside icaptcha-client's own #[cfg(test)] module instead, where the relaxation is live — I confirmed the localhost path is reachable there; alternatively inject the base URL / reqwest client into gl's solve path. -
[P2] Assert the mocks and the retry count.
crates/gl/src/http.rs. Both tests bind their mocks with_and never call .assert(); the exhaustion test comments "3 node calls" but sets no .expect(3). That silence is exactly why the live-service fallthrough passes unnoticed — add .assert()/.expect(N) so an unreached mock fails loudly. -
[P2] Drop or rework the is_https #[cfg(test)] relaxation.
crates/icaptcha-client/src/lib.rs. It's inert where it's meant to help (removing it changes no test) and nothing depends on it. It does not reach release builds — I confirmed a non-test release build rejects a localhost URL and falls back to DEFAULT_URL — so it's not a security hole, just dead code that implies coverage which isn't there. -
[P3] The clippy borrow fix in
crates/gitlawb-node/src/api/profiles.rsis unrelated to a test(gl) PR and duplicates the identical one-liner already in #156 and #134; drop it here to avoid a conflict/no-op once one of those merges.
Happy to re-review once the tests drive the mock.
…g(test), add mock assertions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/icaptcha-client/src/lib.rs (1)
84-105: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDoc comment overstates the safety of this gate; the insecure branch is compiled into production.
Two small points on
is_https:
- The doc says this avoids "shipping the backdoor to production", but the check is runtime env-gated (
GITLAWB_ICAPTCHA_INSECURE), notcfg(test)-gated, so the code path is present in release binaries and merely dormant. Consider rewording so maintainers don't assume it's compiled out.- The insecure branch matches on host only and never asserts
scheme() == "http", so any non-https scheme (e.g.ftp://localhost) would also be trusted when the flag is set. Tightening tohttpcloses that gap.♻️ Optional hardening
if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some() && parsed .as_ref() - .map(|p| p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) + .map(|p| { + p.scheme() == "http" + && (p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) + }) .unwrap_or(false) { return true; }🤖 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/src/lib.rs` around lines 84 - 105, Update the `is_https` doc comment to avoid implying the insecure path is compiled out of production; clarify that `GITLAWB_ICAPTCHA_INSECURE` only enables a runtime escape hatch. Also tighten the insecure branch in `is_https` so it only trusts `http://localhost` and `http://127.0.0.1`, not any non-https scheme, by explicitly checking the parsed URL’s scheme alongside the host.
🤖 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.
Inline comments:
In `@crates/gl/src/http.rs`:
- Around line 537-538: The fixed hit count is being asserted on the existing
mock after registration, but mockito expectations are set when the mock is
created. Update MockIcaptcha::new to register the challenge and answer mocks
with an expectation of 2 hits, or recreate the mock with the correct count
before the two requests are made, so the assertion matches the actual request
sequence.
---
Nitpick comments:
In `@crates/icaptcha-client/src/lib.rs`:
- Around line 84-105: Update the `is_https` doc comment to avoid implying the
insecure path is compiled out of production; clarify that
`GITLAWB_ICAPTCHA_INSECURE` only enables a runtime escape hatch. Also tighten
the insecure branch in `is_https` so it only trusts `http://localhost` and
`http://127.0.0.1`, not any non-https scheme, by explicitly checking the parsed
URL’s scheme alongside the host.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f6f6b73-1d8f-4622-841e-f4183d09cc89
📒 Files selected for processing (3)
crates/gitlawb-node/src/api/profiles.rscrates/gl/src/http.rscrates/icaptcha-client/src/lib.rs
…prevent env-var race
beardthelion
dismissed
their stale review
Superseded by 5698c2e: the two blocking findings (tests hitting the live iCaptcha service, no mock assertions) are resolved. Re-reviewing the new head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving. The blocker from the last round is genuinely fixed: the two integration tests now exercise the iCaptcha retry flow against their mock instead of falling through to the live service. I confirmed it by poisoning DEFAULT_URL to a bogus host and re-running — both tests stay green, so the mock is actually consumed and no live call happens; removing the new GITLAWB_ICAPTCHA_INSECURE branch makes them fail with a DNS error on that poisoned host, so the flag is load-bearing. The .expect(N) + .assert() on every mock closes the earlier no-assertions gap. The runtime flag is contained: with it set, a hostile x-icaptcha-url: http://127.0.0.1 (or 169.254.169.254 / [::1] / a userinfo trick) is still rejected and falls back, and the API key stays with the operator origin — I drove those cases through resolve_solver_url and IcaptchaCfg::new.
Three small non-blocking follow-ups:
Findings
- [P3] Tighten the flag's presence check —
crates/icaptcha-client/src/lib.rs:97.std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some()treats any value as enabled, soGITLAWB_ICAPTCHA_INSECURE=0or=falsestill turns the insecure loopback path on. A truthy check (== "1"/ parse a bool) would honor disable-intent. - [P3] The relaxation is now reachable in a non-test build, not just tests —
crates/icaptcha-client/src/lib.rs:90-104. I verified it's inert without a localhost operator URL (a hostile node can't reach it, no key leaks), so this is fine as a default-off escape hatch. If you'd rather keep insecure code out of the shipped binary entirely, injecting the base URL/client at a test seam inglwould avoid the runtime flag. - [P3]
crates/gitlawb-node/src/api/profiles.rs— the clippy borrow tweak is unrelated to this PR and is the same one-liner as #169; it'll no-op once either merges.
Core is solid. @kevincodex1 good to merge once you're happy with it.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found two issues that need to be addressed before this is ready.
Findings
-
[P2] Keep a loopback test origin from trusting a different loopback port
crates/icaptcha-client/src/lib.rs:97
WithGITLAWB_ICAPTCHA_INSECURE=1, an operator URL ofhttp://localhost:3000, andGITLAWB_ICAPTCHA_API_KEYconfigured, a hostile node can advertisex-icaptcha-url: http://localhost:9000. The new branch accepts both URLs, whileresolve_solver_urlallowlists and marks the selected endpoint key-trusted using onlyhost_str; it therefore chooses:9000and sends the operator's bearer key there. Compare normalized origins (including the effective port) before honoring an insecure advertised URL, or keep advertised HTTP URLs untrusted, and add the distinct-port regression case. -
[P2] Assert that the retry uses the proof produced by the solver
crates/gl/src/http.rs:503
The success-path node mock accepts anyx-icaptcha-proof, so this test remains green ifsend_signedsubstitutes a stale or unrelated nonempty token instead of themock.proofreturned by the iCaptcha answer mock. Matchx-icaptcha-proof: mock.proofhere so the new full-flow test makes the solve-to-retry value transfer load-bearing.
…rl; tighten proof assertion in test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/icaptcha-client/src/lib.rs (1)
432-453: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize this env-var test. Mirror the
ICAPTCHA_ENV_LOCK/IcaptchaEnvpattern here:GITLAWB_ICAPTCHA_INSECUREis process-global, so this test can race with other parallel unit tests that callis_httpsand flake.🤖 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/src/lib.rs` around lines 432 - 453, Serialize this environment-variable test using the existing ICAPTCHA_ENV_LOCK/IcaptchaEnv pattern. Update rejects_insecure_advertised_url_on_different_port to acquire the shared lock and use the guard for setting and restoring GITLAWB_ICAPTCHA_INSECURE, removing the local _EnvGuard implementation while preserving the test assertions.
🤖 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/src/lib.rs`:
- Around line 432-453: Serialize this environment-variable test using the
existing ICAPTCHA_ENV_LOCK/IcaptchaEnv pattern. Update
rejects_insecure_advertised_url_on_different_port to acquire the shared lock and
use the guard for setting and restoring GITLAWB_ICAPTCHA_INSECURE, removing the
local _EnvGuard implementation while preserving the test assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39dfdd7c-4690-4df1-94d4-2f903a36a319
📒 Files selected for processing (2)
crates/gl/src/http.rscrates/icaptcha-client/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gl/src/http.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P3] Restore the inherited iCaptcha test environment after each mock test
crates/gl/src/http.rs:428
crates/icaptcha-client/src/lib.rs:437
Both new guards replace process-wide iCaptcha variables and then unconditionally remove them on drop. Acargo testrun launched withGITLAWB_ICAPTCHA_URLorGITLAWB_ICAPTCHA_INSECUREset therefore loses its original configuration after these tests, which can make later in-process tests (including consumers of the configured URL) order-dependent. Save the priorOsStringvalues before installing the mock settings, then restore each value on drop (removing it only when it was originally absent).
…itional remove; share lock in icaptcha-client tests
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Make the negative
send_oncechecks reject the headers they prohibit
crates/gl/src/http.rs:305
send_once_omits_proof_header_when_not_providedandsend_once_does_not_sign_when_no_keypaircreate unconstrained Mockito handlers, so both tests still receive 200 ifsend_oncestarts attachingx-icaptcha-proofwithNoneor emitsSignature,Signature-Input, andContent-Digestwithout a keypair. Match those headers withmockito::Matcher::Missingso the newly added negative cases actually protect the unsigned/no-proof contract.
…o they actually protect the contract
beardthelion
dismissed
their stale review
Stale: head advanced to 58769e7 after this approval, and jatmn found issues on the intervening heads. Dismissing and re-reviewing the current head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving at 58769e7. jatmn's two open findings are both addressed here, and I confirmed each holds under a break rather than just that the suite is green.
- The negative
send_oncetests now protect the contract: attachingx-icaptcha-proofwhen none was provided, or emitting the signature headers with no keypair, makes each test fail (theMatcher::Missingmatchers reject the request). Before this they passed regardless — that was the P2. - The retry integration tests exercise the real path: disabling the retry drops
send_signed_solves_icaptcha_and_retries_to_successto a 403, and raising the retry cap breaks the exhaustion test's call count. A retry-exhausted 403 is returned to the caller (register reads!status.is_success()and bails), and an unsolvable challenge surfaces as anErr, not a success. - The env guards now restore an inherited
GITLAWB_ICAPTCHA_URL/INSECUREon drop instead of deleting it (the P3 from the prior round).
INV-8 holds across the denial paths: retry-exhausted → Ok(403) surfaced, unsolvable → Err, no iCaptcha headers → Ok(403) surfaced. The loop is capped and origin trust compares scheme+host+port, so a hostile advert or a different loopback port can't redirect the proof or key.
Three optional follow-ups, none blocking:
- [P3] The status half of the "only a 403 retries" gate isn't pinned — the header gate short-circuits it, so a non-403 carrying
x-icaptcha-*isn't exercised. - [P3] No end-to-end test for the level-only advert (
x-icaptcha-levelpresent,x-icaptcha-urlabsent). - [P3] A panic while holding the env lock surfaces a sibling test as a poison error rather than its real assertion;
lock().unwrap_or_else(|e| e.into_inner())fixes the ergonomics.
One housekeeping item: the base is behind main and the one-line profiles.rs change is already identical in main, so please rebase before merge — it resolves to a no-op, no gate dropped.
@kevincodex1 good to merge once rebased.
Summary
Add comprehensive tests for the iCaptcha transparent solve-and-retry flow in the
glCLI HTTP client, covering the critical registration path that was blocking new users.Motivation & context
Closes #167
Node v0.5.0 requires iCaptcha proof for registration, but the released gl v0.3.8 predates the icaptcha-client code. The fix existed on
main(transparent solve-and-retry incrates/gl/src/http.rs,crates/icaptcha-client) but lacked test coverage for the retry flow.Kind of change
What changed
crates/icaptcha-client/src/lib.rs: Added#[cfg(test)]support inis_httpsto allow localhost/127.0.0.1 URLs in test builds, enabling mockito-based integration tests for the full iCaptcha retry path.crates/gl/src/http.rs: Added 16 tests:icaptcha_cfg(header parsing, defaults, keypair required)send_once(proof header attachment, signing)send_signed(non-iCaptcha 403 passthrough, success, 405 handling)How a reviewer can verify
cargo test -p gl -p icaptcha-client cargo clippy -p gl -p icaptcha-client cargo fmt -p gl -p icaptcha-client --checkAll 266 tests in
gland 12 tests inicaptcha-clientpass with no warnings.Before you request review
cargo test --workspacepasses locally (node DB-dependent tests are pre-existing PG auth failures)cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleantest(gl): ...).env.exampleupdated if behavior or config changed (or N/A)Notes for reviewers
The two
send_signedintegration tests (send_signed_solves_icaptcha_and_retries_to_success,send_signed_returns_403_after_icaptcha_retries_exhausted) usestd::env::set_varforGITLAWB_ICAPTCHA_URL. An RAII guard (IcaptchaEnv) ensures cleanup. Tests use#[tokio::test]default single-threaded runtime, so env var races are not a concern.Summary by CodeRabbit
localhost/127.0.0.1) are supported, with stricter origin matching (including port) to avoid trusting an unintended solver.