test(icaptcha-client): add test that new() honors key_trusted gate (#212)

[Gravirei]

Summary

Adds a guard test that constructs IcaptchaCfg through new() and verifies the API key gate is honored — key withheld for untrusted origins, attached for the operator's own origin.

Motivation & context

Closes #212

All existing tests called resolve_solver_url directly; none exercised new()'s wiring that consumes key_trusted. A regression dropping the gate would ship green.

Kind of change

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

• crates/icaptcha-client/src/lib.rs: Added new_withholds_key_from_untrusted_origin_but_attaches_for_operator test

How a reviewer can verify

cargo test -p icaptcha-client

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo clippy --workspace --all-targets -- -D warnings is clean
• Commit titles use Conventional Commits (fix(...))
• Docs / .env.example updated if behavior or config changed (or N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

No protocol impact.

Notes for reviewers

This test saves/restores env vars and releases the lock before asserting, so a failure won't poison the shared lock or cascade into sibling tests.

Summary by CodeRabbit

• Tests
  • Added coverage validating IcaptchaCfg::new attaches the API key only when the node-advertised iCaptcha origin matches the configured operator URL.
  • Confirmed API keys are withheld for untrusted/unconfigured origins and verified iCaptcha URL resolution behavior, including default fallback and operator URL resolution.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a unit test covering IcaptchaCfg::new API-key withholding and attachment across untrusted, operator-configured, default, and operator-fallback origins, with serialized environment access and restoration.

Changes

iCaptcha API-key gate

Layer / File(s)	Summary
API-key attachment test crates/icaptcha-client/src/lib.rs	Tests key withholding and attachment across multiple solver URL configurations, verifies resolved URLs, and restores environment variables under the shared mutex.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• #212: Adds the requested regression coverage for IcaptchaCfg::new API-key attachment wiring.
• #216: Adds related regression coverage for environment-driven IcaptchaCfg::new API-key handling.

Possibly related PRs

• Gitlawb/node#168: Covers the iCaptcha URL trust logic exercised by this test.

Suggested labels: kind:test, crate:icaptcha

Suggested reviewers: jatmn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The added lib.rs test covers the issue’s required new() wiring, including withheld key for untrusted origins and attachment for the operator origin.
Out of Scope Changes check	✅ Passed	The change is scoped to a single test addition in icaptcha-client and introduces no unrelated code churn.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly matches the change: a new icaptcha-client test covering new() and the key_trusted gate.
Description check	✅ Passed	The description follows the template well, with summary, motivation, change type, verification, checklist, and protocol impact filled in.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[beardthelion]

Verified this load-bearing by mutation at 7c24cd7 before anything else: baseline 19/19; making new() attach the key unconditionally goes RED on exactly the untrusted-arm assertion (left: Some("secret-bearer"), right: None); making it never attach goes RED on the operator arm; restore returns 19/19. Both arms bind the gate, the restore path uses remove_var rather than an empty set_var, and both asserts run after env-restore plus lock-drop, so a failure can't poison ICAPTCHA_ENV_LOCK. Ambient GITLAWB_ICAPTCHA_INSECURE can't flip either arm (every URL here is https, and with no operator configured lib.rs:166 pins key_trusted false). CI is green including MSRV. Solid work, and exactly what #212 asked for.

The same mutation pass turned up one state the test doesn't drive, worth closing while you're in this block.

Findings

• [P2] Drive the third reachable key_trusted state: operator configured, default host advertised
  crates/icaptcha-client/src/lib.rs:517
  The untrusted arm removes GITLAWB_ICAPTCHA_URL, so no arm ever observes the key withheld while an operator IS configured. That state is reachable: with the operator var set and a node advertising https://icaptcha.gitlawb.com/v2, the advert is allowlisted and chosen while its origin differs from the operator's, so key_trusted must be false. I ran both regressions that live in this gap: if key_trusted -> if operator.is_some() at lib.rs:204, and dropping the origin comparison at lib.rs:166. Each passes the full crate suite 19/19 with the test as submitted, and either one hands the operator's bearer key to an origin the operator did not choose, steerable by any node via x-icaptcha-url. Fix: before the restore block, while the operator var is still set, construct once more with Some("https://icaptcha.gitlawb.com/v2".into()) and assert api_key is None. I ran that arm both ways: RED under both mutations, green on the current code.

• [P3] Pin cfg.url in both arms
  crates/icaptcha-client/src/lib.rs:519
  The test asserts only api_key, so new()'s URL wiring is unpinned: url: url.unwrap_or(resolved_url) at lib.rs:212 passes the whole crate suite yet routes the solve to the hostile advertised origin. Capturing the cfg and asserting url == DEFAULT_URL in the untrusted arm (and "https://icap.mynode.example/v1" in the operator arm) closes it; I ran those asserts both ways (RED under that mutation with left "https://evil.example", green on current code).

• [P3] Retitle to test(icaptcha-client): the change is test-only
  CONTRIBUTING.md line 55: commit prefixes drive the release-please version bump, so a fix: title on squash-merge would cut a patch release for a tests-only change. Recent test-only merges use test(...) (ad7c2b2, c296e23).

Out of scope here: the empty-string GITLAWB_ICAPTCHA_API_KEY path through new()'s filter (filing that one separately), and wire-level observation that the bearer actually rides both challenge and answer requests, which belongs with the proxy-observer machinery #214 is building rather than a new issue.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Exercise the configured-operator/default-host key gate
  crates/icaptcha-client/src/lib.rs:522
  With GITLAWB_ICAPTCHA_URL=https://icap.mynode.example, a node may advertise the separately allowlisted https://icaptcha.gitlawb.com/v2; resolve_solver_url selects that URL, but key_trusted must be false because it is not the operator origin. The two current arms both remain green if new() regresses from if key_trusted to merely testing whether an operator is configured (or if the origin comparison is removed), which sends the operator bearer token to the node-selectable default origin. Add that third construction while the operator environment variable is set and assert that api_key is None.

• [P2] Assert the resolved URL from the constructor
  crates/icaptcha-client/src/lib.rs:518
  This test keeps only api_key, so it does not bind new() to use resolved_url. A regression such as assigning the raw advertised URL to IcaptchaCfg::url still passes both assertions and the direct resolver tests, yet makes the untrusted https://evil.example advertisement the challenge/answer destination. Retain each constructed config and assert that the rejected advertisement resolves to DEFAULT_URL and the matching operator advertisement retains its allowed /v1 URL.

• [P3] Use the test Conventional Commit type for this test-only change
  7c24cd72943b0d02289ca1bdbbccc887a7d750d7
  The PR changes only a unit test, but its fix(icaptcha-client): title/commit will cause release-please to select a patch release: CONTRIBUTING.md states that Conventional Commit prefixes drive version bumps. Please retitle/recommit it as test(icaptcha-client): ... so merging this coverage-only change does not publish an unnecessary release.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Exercise the configured-operator fallback construction
  crates/icaptcha-client/src/lib.rs:522
  All key-attached constructions pass an advertised operator URL. A normal API-key deployment also reaches IcaptchaCfg::new with GITLAWB_ICAPTCHA_URL configured but no x-icaptcha-url header (for example, the client receives only x-icaptcha-level); resolve_solver_url(None, Some(operator)) selects that operator and marks the key trusted. Consequently, a regression such as additionally requiring url.is_some() when attaching the key leaves all 19 current crate tests green but makes these fallback challenge/answer requests omit Authorization and fail against the protected operator service. Construct this url = None case while the operator env var is set and assert both the operator URL and attached key.

[beardthelion]

Re-reviewed 6b2ec93d. My two prior findings are resolved, both confirmed by mutation: the operator-configured / default-host-advertised state now withholds the key, and each arm pins the resolved URL.

Concurring with jatmn's remaining finding: the operator-configured, no-advertised-URL fallback (resolve_solver_url(None, Some(operator))) attaches the key with no arm covering it, so a url.is_some()-style regression ships green. I ran it: with if key_trusted && url.is_some() the 19 submitted tests all stay green while the real url = None path drops the Authorization header. One construction with the operator var set and url = None, asserting the key attaches, closes it. Not approving over that.

[beardthelion]

Re-reviewed 9be5eac3. All three prior P2 asks are in, and I certified each by mutation at this head: if key_trusted -> if operator.is_some() and the dropped origin comparison both go RED on exactly the arm-3 assertion (left: Some("secret-bearer"), right: None); key_trusted && url.is_some() goes RED on arm 4 (left: None, right: Some("secret-bearer")); url.unwrap_or(resolved_url) goes RED on arm 1's URL pin. Restore returns 19/19. The Cargo.lock delta is a clean regeneration against main's 0.5.1 manifests (sha2 v0.10.9 was already declared and consumed by pow.rs; no new packages, no checksum churn).

One state is still unpinned, and it is the scenario in the test's own name.

Findings

• [P2] Pin the operator-configured hostile-advert state (the exfiltration direction)
  crates/icaptcha-client/src/lib.rs:517
  Arm 1 is the only hostile-advert arm and it removes GITLAWB_ICAPTCHA_URL first, so its api_key == None passes because no operator exists, not because a live key was withheld from an attacker origin. I ran the regression that lives in this gap: url: if operator.is_some() && url.is_some() { url.clone().unwrap() } else { resolved_url } at lib.rs:212 keeps all 19 tests green while (operator set, key set, advert https://evil.example) yields url=https://evil.example, api_key=Some("secret-bearer"): the operator bearer rides to a node-steered origin, and arm 1's URL pin cannot see it because the override only fires when an operator is configured. Fix: a fifth construction with the operator var and key still set and advert Some("https://evil.example".into()), asserting url == "https://icap.mynode.example" and api_key.as_deref() == Some("secret-bearer"). I ran that arm both ways: RED under the mutation (it observes the exfil state directly), GREEN on the current code.

• [P3] Retitle the PR itself to test(icaptcha-client)
  Commit 6b2ec93 carries the requested test( type, but this repo squash-merges with COMMIT_OR_PR_TITLE and the PR now has two commits, so the merge commit takes the PR title, which still says fix(icaptcha-client). Per CONTRIBUTING.md that cuts a patch release for a tests-only change. With the title fixed the squash result is right regardless of the second commit's fix( message.

• [P3] Optional: pin the (no operator, no advert, key set) state while you are in this block
  crates/icaptcha-client/src/lib.rs:204
if key_trusted || (url.is_none() && operator.is_none()) passes all 19 tests and would attach the bearer to the public default host whenever the node omits x-icaptcha-url. One construction in arm 1's env state with url = None, asserting api_key == None and url == DEFAULT_URL, closes it; I ran it both ways (RED under that mutation, GREEN on current code). Take it or leave it in this PR.

Out of scope for this round, deliberately: wire-level observation that the bearer actually rides (or is absent from) the challenge and answer requests stays with the proxy-observer machinery #214 is building, and the empty-string GITLAWB_ICAPTCHA_API_KEY filter is still mine to file separately. Everything else from the prior rounds is resolved and verified.

[beardthelion]

Re-certified the gate by mutation at 82fec97a: baseline 19/19, all six prior gate-regressions each go RED on their arm, restore returns 19/19. Arm 5 (operator + hostile advert) and the URL pins are in and correct. One security-relevant state is still unpinned, and it is the same shape as the arms you already added.

Findings

• [P2] Pin https port-isolation through new()
crates/icaptcha-client/src/lib.rs:116
  Every port test reaches origin_key through the GITLAWB_ICAPTCHA_INSECURE http-localhost escape hatch (rejects_insecure_advertised_url_on_different_port), so the production https path never pins that a differing-port advert is a distinct origin. I ran the regression: stripping the port from origin_key for scheme == "https" keeps all 19 tests green, and under it an advert of https://icap.mynode.example:8443/evil resolves as the operator origin, so the solve (and the trusted bearer) rides to the node-steered :8443 URL on the operator host. Production code is correct today; this is the missing regression fence. Add one arm with the operator var set: IcaptchaCfg::new("did:key:zTEST", Some("https://icap.mynode.example:8443/evil".into()), None), assert url == "https://icap.mynode.example" and api_key == Some("secret-bearer"). I ran that arm both ways: GREEN on current code, RED under the mutant (the url pin catches it).

• [P3] Collect arm failures instead of asserting sequentially
  crates/icaptcha-client/src/lib.rs:572
  The six arms assert in sequence, so the first failing arm masks every later one in a single run. Either split per-arm or push mismatches into a Vec and assert!(failures.is_empty(), ...) once at the end, so a multi-arm regression shows every affected arm.

• [P3] Add assert messages to the three bare assert_eq!s
  crates/icaptcha-client/src/lib.rs:590
operator_match.api_key, operator_match.url, and default_advert.url assert without the descriptive message every sibling arm carries. Match the convention so a failure names the arm.