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 for IcaptchaCfg::new to ensure the iCaptcha API key is attached only when the node-advertised iCaptcha origin matches the configured operator URL.
  • Verified URL/origin resolution across scenarios, including default fallback, operator-based resolution, and rejection of untrusted origins (including mismatched ports).

[Copilot]

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

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a44d0703-e462-4846-be53-9b9119112da2

📥 Commits

Reviewing files that changed from the base of the PR and between 82fec97 and e1e6e1c.

📒 Files selected for processing (1)

• crates/icaptcha-client/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/icaptcha-client/src/lib.rs

---

📝 Walkthrough

Walkthrough

Adds a unit test covering IcaptchaCfg::new API-key withholding and attachment across six origin-resolution scenarios, with serialized environment access, restoration, and URL assertions.

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 trusted, untrusted, default, and operator-fallback origins, including port differences, while restoring 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: Covers related environment-driven API-key handling in the same constructor.

Possibly related PRs

• Gitlawb/node#168: Covers related iCaptcha URL trust and API-key attachment behavior.

Suggested labels: kind:test, crate:icaptcha

Suggested reviewers: jatmn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the added test for IcaptchaCfg::new() and its key_trusted gating.
Description check	✅ Passed	The description follows the template and includes the required summary, context, change list, verification, and review checklist sections.
Linked Issues check	✅ Passed	The new test addresses #212 by exercising IcaptchaCfg::new() and verifying the API key is withheld for untrusted origins and attached for the operator's origin.
Out of Scope Changes check	✅ Passed	The PR appears scoped to a single new test in one file, with no unrelated code or public API changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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.

[jatmn]

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

Findings

• [P2] Exercise HTTPS port isolation through IcaptchaCfg::new
crates/icaptcha-client/src/lib.rs:527
  The current origin_key implementation correctly preserves ports, but the added constructor arms never combine the configured HTTPS operator with a same-host advertised URL on a different port; the only port test enables the insecure localhost escape hatch and calls resolve_solver_url directly. If a later change regresses HTTPS port handling, an advert such as https://icap.mynode.example:8443/evil matches configured https://icap.mynode.example, so new() preserves that node-selected URL and attaches the operator bearer. All current tests stay green, but both challenge and answer requests then send the bearer to :8443. Add a configured-operator constructor arm for that advert and assert fallback to the operator URL with the key attached.