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.

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