Skip to content

icaptcha-client: no test binds new()'s empty-API-key filter, so a Some("") regression would ship green #216

Description

@beardthelion

IcaptchaCfg::new (crates/icaptcha-client/src/lib.rs:204-210) normalizes an empty
GITLAWB_ICAPTCHA_API_KEY to None via .filter(|s| !s.is_empty()) before storing
api_key. If that filter regresses, a trusted-origin solve carries an Authorization
header with an empty bearer token on both the challenge and answer requests
(req.bearer_auth(key) at lib.rs:310 and lib.rs:335): a malformed credential instead of
an absent one, and the "operator set the var but left it blank" state silently flips from
keyless to key-bearing. No test in the crate drives the empty-string value through
new(), so the regression ships green.

Verified by mutation at 7c24cd7 (#213's head; new() is identical on main, that head
adds only the #213 test):

  • Baseline: 19 lib tests green.
  • Filter removed (std::env::var("GITLAWB_ICAPTCHA_API_KEY").ok() with no emptiness
    filter): all 19 still green. That is the gap.

Suggested regression test, load-bearing both ways at the same head: RED under the
filter-removal mutation with exactly one failure (left: Some(""), right: None), and
20 passed 0 failed with the current code. It goes in lib.rs's #[cfg(test)] mod tests
so it shares ICAPTCHA_ENV_LOCK, same shape as the #213 test:

#[test]
fn new_treats_empty_api_key_as_absent() {
    let guard = ICAPTCHA_ENV_LOCK.lock().unwrap();
    let prev_key = std::env::var_os("GITLAWB_ICAPTCHA_API_KEY");
    let prev_op = std::env::var_os("GITLAWB_ICAPTCHA_URL");
    std::env::set_var("GITLAWB_ICAPTCHA_API_KEY", "");
    std::env::set_var("GITLAWB_ICAPTCHA_URL", "https://icap.mynode.example");
    let api_key = IcaptchaCfg::new(
        "did:key:zTEST",
        Some("https://icap.mynode.example/v1".into()),
        None,
    )
    .api_key;
    match prev_key {
        Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_API_KEY", v),
        None => std::env::remove_var("GITLAWB_ICAPTCHA_API_KEY"),
    }
    match prev_op {
        Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_URL", v),
        None => std::env::remove_var("GITLAWB_ICAPTCHA_URL"),
    }
    drop(guard);
    assert_eq!(api_key, None, "empty key must not become a bearer header");
}

Scope, to stay clear of the in-flight iCaptcha work: #213 covers the key_trusted gate
arms and this deliberately does not touch them; #214's proxy-observer and tripwire
rewrite owns wire-level observation (asserting the bearer actually rides the challenge
and answer requests fits there, not here); #212 is closed by #213. The sibling
empty-string filter on GITLAWB_ICAPTCHA_URL (lib.rs:200-202) is deliberately not asked
for: an empty operator already fails is_https and drops out inside
resolve_solver_url, so a test on it would not be load-bearing. Not blocking #213 or
#214; whoever picks this up may want to wait for #213 to land first since it sits in the
same test module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind:testTest coverage or harnesssev:lowCosmetic, cleanup, or nice-to-havesubsystem:apiNode REST API request/response surface

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions