From 786794736f629edfa21fa3b15a5fd7dfe9e422fc Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Mon, 12 Oct 2026 19:11:57 +0200 Subject: [PATCH] test(config): pin typo-port refusal and key-file permission warn A written port that is not a u16 (host:, host:99999, host:abc) parses with port_u16() == None and would match any port; the entry.len() > host.len() arm refuses it at startup. > mangled to < or ==, and the : check flipped to !=, accept the typo instead (>= is identical, documented in DESIGN.md). The key-file mode warn fires exactly for group/other-readable files: & mangled to | or ^ warns on 0600 too, != mangled to == warns on 0600 only. DESIGN.md Testing records both pins plus the documented-not-tested dispositions (custom-fields contains(',') collapse, stdio delimiter advance). Guards touched: host validation strictness kept fail-closed, I12 (key never logged). --- crates/bugwarden/src/config.rs | 86 ++++++++++++++++++++++++++++++++++ docs/DESIGN.md | 20 ++++++-- 2 files changed, 102 insertions(+), 4 deletions(-) diff --git a/crates/bugwarden/src/config.rs b/crates/bugwarden/src/config.rs index 9517727..f54d466 100644 --- a/crates/bugwarden/src/config.rs +++ b/crates/bugwarden/src/config.rs @@ -502,6 +502,57 @@ mod tests { assert_eq!(server_key(custody), "stdio-file-key"); } + #[test] + #[cfg(unix)] + fn key_file_group_or_other_readable_warns_exactly_for_the_wide_files() { + use std::os::unix::fs::PermissionsExt as _; + // `& 0o077` mangled to `|` or `^` warns on all three reads, `!=` + // mangled to `==` warns on the safe one only — so one capture over + // three reads kills all three. The other-writable file pins the + // `0o002` bit the group-readable one cannot distinguish. + let dir = tempfile::tempdir().expect("a temp key dir"); + let safe = dir.path().join("600.key"); + let wide = dir.path().join("640.key"); + let other = dir.path().join("602.key"); + for path in [&safe, &wide, &other] { + std::fs::write(path, "test-key\n").expect("write key file"); + } + std::fs::set_permissions(&safe, std::fs::Permissions::from_mode(0o600)).unwrap(); + std::fs::set_permissions(&wide, std::fs::Permissions::from_mode(0o640)).unwrap(); + std::fs::set_permissions(&other, std::fs::Permissions::from_mode(0o602)).unwrap(); + + let (_, logs) = crate::testlog::capture_logs(|| { + for path in [&safe, &wide, &other] { + let key = read_key_file(path).expect("a permitted key file must read"); + assert_eq!(key, "test-key"); + } + }); + + crate::testlog::assert_logged(&logs, "accessible by group or others"); + let text = logs.as_str(); + assert_eq!( + text.matches("accessible by group or others").count(), + 2, + "exactly the two wide reads must warn: {text}" + ); + let wide_path = wide.display().to_string(); + let other_path = other.display().to_string(); + let safe_path = safe.display().to_string(); + assert!( + text.contains(&wide_path), + "one warning must name the 0o640 path: {text}" + ); + assert!( + text.contains(&other_path), + "one warning must name the 0o602 path: {text}" + ); + assert!( + !text.contains(&safe_path), + "no warning must name the 0o600 path: {text}" + ); + logs.assert_not_contains("test-key"); + } + #[test] fn http_with_startup_key_alone_stays_per_request() { // Warn-and-ignore, pinned: --api-key over http must NOT silently @@ -731,6 +782,41 @@ mod tests { ); } + #[test] + fn a_written_port_that_is_not_a_u16_is_a_startup_error() { + // `host:` and `host:99999` parse with `port_u16() == None` and + // would match any port; the `entry.len() > host.len()` arm refuses + // them as typos instead. `>` mangled to `<` or `==`, or the `:` + // check flipped to `!=`, accepts the typo (`>=` is identical — at + // equal lengths the byte lookup is `None` — so that one is + // documented, not tested). + for entry in ["example.com:", "example.com:99999", "example.com:abc"] { + let mut cli = base_cli("http"); + cli.allowed_hosts = vec![entry.to_owned()]; + let err = cli + .checked_allowed_hosts() + .expect_err("a written port that is not a u16 is a typo"); + let msg = format!("{err:#}"); + assert!( + msg.contains("--allowed-hosts"), + "the error must name the flag: {msg}" + ); + assert!(msg.contains(entry), "the error must name the entry: {msg}"); + } + + // Controls: a bare host and a valid u16 port stay matchable. + for entry in ["example.com", "example.com:8000", "[::1]:8000"] { + let mut cli = base_cli("http"); + cli.allowed_hosts = vec![entry.to_owned()]; + assert_eq!( + cli.checked_allowed_hosts() + .unwrap_or_else(|e| panic!("{entry} must be matchable: {e:#}")), + Some(vec![entry]), + "{entry}" + ); + } + } + #[test] fn unbracketed_ipv6_loopback_is_a_matchable_allowed_host() { // rmcp's default allowlist is `localhost`, `127.0.0.1`, `::1`. diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d17f209..64e8987 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -3949,10 +3949,16 @@ wired, `server.rs` and `main.rs` are the reference. --api-key alone stays `PerRequest` (warn-and-ignore pinned against a silent upgrade to server-held custody); an empty --api-key-file value counts as absent; the startup log line states mode and source, never key - material (I12); `Cli`'s Debug redacts the startup key (I12); and + material (I12); `Cli`'s Debug redacts the startup key (I12); server-held http custody under an identity-consulting policy warns at construction (crates/bugwarden/src/server.rs) that created_by_me now - describes the service account for every client. + describes the service account for every client; a written port that is not + a u16 (`host:`, `host:99999`, `host:abc`) is a startup error naming flag + and entry while a bare host and a u16 port stay matchable (`>=` for `>` is + identical — at equal lengths the byte lookup is `None` — so that one is + documented, not tested); and the key-file permission warn fires exactly + for group/other-readable files (0600 silent, 0640/0602 naming their paths, + key material never logged, I12). - Integration tests (crates/bugwarden-core/tests/guard_wiremock.rs, wiremock): quicksearch_window — pages stay full while visible bugs remain (no hole), consecutive pages tile the visible sequence disjointly (against a stable @@ -4049,7 +4055,10 @@ wired, `server.rs` and `main.rs` are the reference. tainting the field and the first offending key named, and skips an Other field whatever it holds, and `candidate_ids` counts every id a value could name under any kind — scalars, array items and comma pieces - — as the request-only upper bound the cap is judged on. + — as the request-only upper bound the cap is judged on; and the + `contains(',')` fast path is behaviorally invisible (a comma-less string + splits to its one scalar either way), so the guard collapsed to `true` is + documented, not tested. - Integration tests (crates/bugwarden-core/tests/discovery_wiremock.rs, wiremock): the discovery endpoints answer their documented envelopes, a field name travels as one percent-encoded path segment, the names @@ -4889,7 +4898,10 @@ wired, `server.rs` and `main.rs` are the reference. rmcp bump. `BugWarden::request_body_cap` is pinned equal to both `http_server_config()?.max_request_body_bytes` and the cap `stdio_transport()` hands the reader, across four policy values, so the - two transports cannot drift apart. + two transports cannot drift apart; and the delimiter advance (`newline + + 1`) has no committed test — either arithmetic mutant hangs the scan + instead of failing it, verified by hand with the multi-frame drain under + `timeout 150`. - Unit tests (#[cfg(test)] in crates/bugwarden/src/otel.rs): configuration resolution — an unset, emptied or blank endpoint resolves to NO configuration and a bad protocol beside it is therefore not an error,