fix(icaptcha-client): parse GITLAWB_ICAPTCHA_INSECURE as truthy only (#227) - #246
fix(icaptcha-client): parse GITLAWB_ICAPTCHA_INSECURE as truthy only (#227)#246Ayush7614 wants to merge 5 commits into
Conversation
Presence checks treated =0/=false/empty as enabled. Only explicit 1/true now enable loopback HTTP trust relaxation (Gitlawb#227).
|
Warning Review limit reached
Next review available in: 21 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe iCaptcha client now enables loopback HTTP trust only for trimmed, case-insensitive ChangesiCaptcha insecure flag handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 — this PR addresses the issue you filed in #227. Happy to adjust if you’d like a different truthy set or to drop the runtime flag entirely in favor of a test-only seam. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The parse fix is right, and I verified the regression test is load-bearing: reverting is_https to the old var_os(...).is_some() turns insecure_env_only_truthy_enables_loopback_http red on its first assertion, and it also resists a partial regression to "any non-empty value enables". Five things below, the first two worth doing before merge.
Findings
-
[P2] Run
cargo fmtbefore this merges
crates/icaptcha-client/src/lib.rs:489
cargo fmt --all -- --checkis a required gate in.github/workflows/pr-checks.yml, andcargo fmt -p icaptcha-client --checkexits 1 on this head. The single hunk collapses the"=1 must enable loopback HTTP"assert onto one line. Only the quality-signal job has run against the PR so far, so the failure shows up once the full check set does. -
[P2] Pin the trim with a test, or drop it
crates/icaptcha-client/src/lib.rs:101
let t = v.trim();is the only production line in this change that no test holds. I ran the mutations: reverting the guard, swapping the truthy match for!t.is_empty(), narrowing to"1"only, and flippingErr(_)totrueeach turn the suite red, but deleting.trim()leaves all 22 lib tests green. One more block in your existing style closes it:InsecureEnv::with_value(" 1 ")assertingis_https("http://127.0.0.1")is true. -
[P3] Say in the doc comment and the description that the value is trimmed
crates/icaptcha-client/src/lib.rs:95
Both state the contract as1/true, case-insensitive, but the value is trimmed first, so" 1 "and"\t1\n"enable the relaxation too. That behavior is fine; it just is not what the stated contract says. -
[P3] Keep a failing assert from poisoning the env lock
crates/icaptcha-client/src/lib.rs:431
When I reverted the production line to prove the test bites, two tests failed rather than one: the real assertion, plusrejects_insecure_advertised_url_on_different_portpanicking withcalled Result::unwrap() on an Err value: PoisonErroratlib.rs:431:49. So the guard degrades an unrelated test at exactly the moment it does its job.ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner())inwith_valueandunsetavoids that. The pattern predates this PR; the new helpers are where it starts to matter. -
[P3] The new warning does not reach operators by default
crates/icaptcha-client/src/lib.rs:106
The description says a non-truthy value warns so operators notice the misconfiguration.glbuilds its subscriber asEnvFilter::from_default_env().add_directive("gl=info"), and withRUST_LOGunset anicaptcha_clientWARN is not printed; I saw it only underRUST_LOG=icaptcha_client=warn. That gap is pre-existing and already swallows the advertised-URL warning next to it, so either add the directive incrates/gl/src/main.rsor soften the claim.
The rest checked out. is_https has only the two call sites in resolve_solver_url, the one other reader of the variable (crates/gl/src/http.rs) sets "1" and is unaffected, and every value class that diverges from the old presence check now fails closed, so this is a strict narrowing rather than a behavior swap. Host matching under the relaxation is unchanged.
Pin trim with a whitespace truthy case, recover from a poisoned ICAPTCHA_ENV_LOCK, document trim + warn visibility, and cargo fmt.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/icaptcha-client/src/lib.rs`:
- Around line 432-444: Refactor is_https() and resolve_solver_url() to accept
the insecure-mode flag as an explicit parameter, removing their reads of
GITLAWB_ICAPTCHA_INSECURE. Update all production and test callers to obtain or
provide the flag without mutating process-global environment state, and remove
the InsecureEnv helpers and related environment locking from the parallel
resolver tests.
- Around line 101-118: Update the warning in the GITLAWB_ICAPTCHA_INSECURE
environment-variable parsing branch to avoid logging the raw trimmed value
through the tracing field value = %t. Log only a safe bounded representation or
a fixed message indicating the value is non-truthy, while preserving the
existing warning condition and disabled fallback.
🪄 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 Plus
Run ID: e08d9fa1-7ce8-4985-be90-45732de8a285
📒 Files selected for processing (1)
crates/icaptcha-client/src/lib.rs
|
@beardthelion — addressed your review findings:
|
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 emit the raw insecure-mode environment value
crates/icaptcha-client/src/lib.rs:109
The new non-truthy path records the complete trimmedGITLAWB_ICAPTCHA_INSECUREvalue withvalue = %t. That value is arbitrary deployment input, so a mistakenly pasted secret or terminal-control-containing value is written to the tracing sink whenever an HTTP URL is evaluated;git-remote-gitlawbenables WARN logging to stderr by default. The warning only needs to report that the setting is invalid—remove the value field or replace it with a bounded, control-safe non-secret classification.
The non-truthy warn path previously recorded the full trimmed env value. That is arbitrary deployment input; report only that the setting is invalid (Gitlawb#246 review).
|
@jatmn — addressed your review: [P2] raw env value in warn — removed
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/icaptcha-client/src/lib.rs (1)
100-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWarn for present non-Unicode values instead of treating them as unset.
std::env::varcan returnVarError::NotUnicodewhenGITLAWB_ICAPTCHA_INSECUREis set but contains invalid UTF-8;Err(_) => falsefails closed but skips the warning intended for non-truthy values. Usevar_osor distinguishNotPresentfromNotUnicode, emitting the same fixed warning for the latter while remaining fail-closed.🤖 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 100 - 119, Update icaptcha_insecure_enabled to distinguish an unset GITLAWB_ICAPTCHA_INSECURE from a present non-Unicode value, using var_os or explicit VarError handling. Preserve fail-closed behavior for invalid UTF-8, but emit the existing fixed warning for present non-Unicode values without logging raw input.
♻️ Duplicate comments (1)
crates/icaptcha-client/src/lib.rs (1)
423-445:⚠️ Potential issue | 🟠 MajorThe process-global environment race remains unresolved.
InsecureEnvserializes only environment writers, whileis_https()/icaptcha_insecure_enabled()read the same process-global variable without taking_lock. Concurrent resolver tests can therefore observe values while another test sets or removes them. Pass the parsed flag into pure helpers or otherwise synchronize all readers.🤖 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 423 - 445, Synchronize environment reads with the writes guarded by ICAPTCHA_ENV_LOCK, or parse the insecure flag once and pass it into pure helpers used by is_https() and icaptcha_insecure_enabled(). Ensure all resolver tests cannot observe the process-global GITLAWB_ICAPTCHA_INSECURE value while InsecureEnv is mutating it.
🤖 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.
Outside diff comments:
In `@crates/icaptcha-client/src/lib.rs`:
- Around line 100-119: Update icaptcha_insecure_enabled to distinguish an unset
GITLAWB_ICAPTCHA_INSECURE from a present non-Unicode value, using var_os or
explicit VarError handling. Preserve fail-closed behavior for invalid UTF-8, but
emit the existing fixed warning for present non-Unicode values without logging
raw input.
---
Duplicate comments:
In `@crates/icaptcha-client/src/lib.rs`:
- Around line 423-445: Synchronize environment reads with the writes guarded by
ICAPTCHA_ENV_LOCK, or parse the insecure flag once and pass it into pure helpers
used by is_https() and icaptcha_insecure_enabled(). Ensure all resolver tests
cannot observe the process-global GITLAWB_ICAPTCHA_INSECURE value while
InsecureEnv is mutating it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76f52a4b-a120-46b1-9beb-7e60b5d0c933
📒 Files selected for processing (1)
crates/icaptcha-client/src/lib.rs
Warn on present non-Unicode GITLAWB_ICAPTCHA_INSECURE (fail closed). Pass insecure mode into is_https_with / resolve_solver_url_with so tests no longer mutate process-global env (removes InsecureEnv race).
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
@beardthelion LGTM but requires your evaluation
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The parse is right and containment holds: with the flag set I drove the host matrix through the real check and only loopback-equivalent hosts are trusted, while 127.0.0.1.evil.com, localhost.evil.com, 169.254.169.254 and [::1] are all rejected. cargo fmt is clean, the trim is pinned, the doc contract now matches the code, and both the raw-value warning fix and the pure-helper refactor landed. One thing regressed between rounds.
Findings
-
[P2] Cover the line that actually reads the environment
crates/icaptcha-client/src/lib.rs:145
icaptcha_insecure_enabled()is the only production line namingGITLAWB_ICAPTCHA_INSECURE, and after the harness was removed nothing exercises it. I mutated it three ways and ran the suite each time: reverting tostd::env::var_os(..).is_some(), which is exactly the bug #227 is about, 22 passed; hardcodingtrue, so loopback HTTP trust is on unconditionally in release, 22 passed; typo'ing the variable name, so the flag silently never works, 22 passed. At the earlier head the test did bite, because it drove the real env read. The same gap covers the warning: I deleted both call sites ofwarn_icaptcha_insecure_non_truthyand the suite stayed at 22 passed, so the message two reviewers worked on has no guard either. Keep the pure helpers, since they are what fixed the race, and add one serialized test alongside them: take aMutexlocked withunwrap_or_else(|e| e.into_inner()), save the prior value, asserticaptcha_insecure_enabled()is false for unset /0/false/ empty and true for1/true/TRUE/" 1 ", then restore. I wrote that and ran it: 23 passed clean, and it fails on all three mutations above. It does not cover the warning emission, and I am not asking you to add a tracing capture harness in this PR. -
[P3] Collapse the space run in the warning
crates/icaptcha-client/src/lib.rs:96
The literal reads"...(expected 1 or true); loopback HTTP trust relaxation remains disabled", with ten spaces mid-sentence. It looks like a line-join artifact and it reaches operators verbatim.
Nothing else is outstanding on my side. Unrelated to this PR but worth knowing while you are in here: a test harness bound to ::1 rather than 127.0.0.1 is not trusted even with the flag on, and would quietly fall back to the public default.
Superseded by my review on 16eaa3e; dismissing so the state reflects the current head.
Add a serialized test that drives icaptcha_insecure_enabled() against the process env so presence-check / typo / always-true mutations fail. Collapse the mid-sentence space run in the non-truthy warning (Gitlawb#246).
|
@beardthelion — addressed your latest review:
|
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
- [P3] Collapse the space run in the non-truthy warning
crates/icaptcha-client/src/lib.rs:95-98
Thetracing::warn!literal still uses a\line continuation, so the emitted message contains a run of spaces between;andloopback(...(expected 1 or true); loopback HTTP trust relaxation remains disabled). That is the same line-join artifact from the prior review round; the latest commit message claims spacing was collapsed but the continuation indent is still in the string. Please put the sentence on one line (or otherwise join without embedded whitespace) so operators see normal prose.
Superseded: re-reviewed on 45f3de1, the blocking coverage gap is closed.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The parse is right, and I re-verified it on this head rather than carrying the last round forward. Seven mutations of the changed lines each turn the suite red: reverting icaptcha_insecure_enabled to var_os(..).is_some(), hardcoding it true, typo'ing the variable name, dropping the .trim(), widening the match to any non-empty value, narrowing it to exactly "1", and flipping the NotUnicode arm to fail open. That is the gap from my last round closed.
I also drove the release-reachable path with a cfg(test)-off example through IcaptchaCfg::new: 0, false, FALSE, no, empty and unset all fall back to the public default with the API key withheld, and 1 / true / TRUE / " 1 " all enable. With the flag on, every hostile advert I tried falls back to the operator origin: 127.0.0.1.evil.com, localhost.evil.com, 169.254.169.254, [::1], http://user@evil.com, https://evil.com, and a different loopback port. fmt and clippy are clean and the gl consumer suite passes.
On the warning spacing: that one is already correct, so please leave it as it is. Rust's \ line continuation strips the newline and the continuation line's leading whitespace, so the emitted message has a single space after the semicolon. I confirmed it twice by execution rather than by eye: compiling the exact literal (length 117, no double space anywhere), and pulling the string back out of the built rlib. Changing it now would be a change with no observable effect.
One thing is still open, and it is mine to land rather than a fifth round for you. resolve_solver_url passes icaptcha_insecure_enabled() into resolve_solver_url_with, and that seam has no coverage in the fail-open direction: hardcoding the argument true keeps this crate at 23 passed and gl at 11 passed, which is the #227 behaviour restored with nothing going red. The opposite direction is caught, false fails two of gl's mockito tests. It pairs with tightening the test lock, since the six resolver tests still read the environment while EnvGuard writes it. I ran the suite thirty times and saw no flake, so nothing is wrong today. I will send both as a follow-up.
Thanks for staying with this one through four rounds.
3 participants
Summary
GITLAWB_ICAPTCHA_INSECUREas enabled only for explicit truthy values (1/true, case-insensitive).=0,=false, empty, and unset leave the loopback HTTP trust relaxation disabled (fixes the presence-check footgun in Parse GITLAWB_ICAPTCHA_INSECURE value so =0/=false disables the loopback relaxation #227).Test plan
cargo test -p icaptcha-client— all 23 tests passedinsecure_env_only_truthy_enables_loopback_httpcovers0/false/ empty / unset /1/true/TRUEFixes #227
Summary by CodeRabbit
1ortrue(case-insensitive, whitespace-tolerant).0,false, or random text) no longer enables insecure behavior and now emits a warning.