test(gl): add tests for iCaptcha transparent retry flow (#167)#168
test(gl): add tests for iCaptcha transparent retry flow (#167)#168Gravirei wants to merge 18 commits into
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.
|
Warning Review limit reached
Next review available in: 19 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR relaxes the icaptcha-client trusted-URL check to allow http://127.0.0.1 and http://localhost when GITLAWB_ICAPTCHA_INSECURE is set, adds extensive unit and integration tests for NodeClient's iCaptcha configuration, signing, and retry logic in gl/src/http.rs, and removes a redundant borrow in a profiles.rs format string. ChangesiCaptcha Client Trust and Testing
Estimated code review effort: 3 (Moderate) | ~20 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_proof_required
GlClient->>Icaptcha: request challenge
Icaptcha-->>GlClient: challenge
GlClient->>Icaptcha: submit answer
Icaptcha-->>GlClient: proof
GlClient->>Node: retry request with x-icaptcha-proof header
Node-->>GlClient: 200 success
Possibly related PRs
🚥 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
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.
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
localhostand127.0.0.1when insecure mode is enabled, while still requiring secure connections elsewhere.