From 8c25d46c170de6b2c2d82d777f456e52a4b746b8 Mon Sep 17 00:00:00 2001 From: James Soubry Date: Thu, 24 Sep 2026 11:31:37 +0000 Subject: [PATCH 1/3] [backlog] ast_guard: add crypto/TLS misconfiguration rules (v3.22.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds genuinely missing crypto/TLS AST rules for issue #264: Node crypto.createCipher/createDecipher (insecure-crypto), Python AES.MODE_ECB attribute access (insecure-crypto), and two more Python tls-verify-disabled shapes (ssl._create_unverified_context() call, check_hostname=False keyword arg). The existing Node rejectUnauthorized rule and existing Python verify=False rule are untouched since they already covered those cases. Deliberately skips the AES cipher-mode string-literal form and the NODE_TLS_REJECT_UNAUTHORIZED=0 env-var string form, per the issue's own scoping — those are string values, not code structure, and are a poor fit for AST matching. Closes jamessoubry/clawband#264 --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/ast_guard.rs | 254 ++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 251 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5a663d91..103a1ecf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.21.0" +version = "3.22.0" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index 6e71faa8..a4a72c8e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.21.0" +version = "3.22.0" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/src/ast_guard.rs b/src/ast_guard.rs index 2acc2217..299a8c10 100644 --- a/src/ast_guard.rs +++ b/src/ast_guard.rs @@ -55,7 +55,10 @@ pub enum Lang { /// external entities by default, so bare calls are routine and are not /// flagged — see the `python_xxe_findings` doc comment for the review /// finding this narrowed), `tls-verify-disabled` (any call with - /// keyword argument `verify=False`), `sql-string-interpolation` + /// keyword argument `verify=False`, `ssl._create_unverified_context()`, + /// or any call with keyword argument `check_hostname=False`), + /// `insecure-crypto` (`AES.MODE_ECB` attribute access, PyCryptodome/ + /// PyCrypto), `sql-string-interpolation` /// (`.execute`/`.executemany` with an f-string/`%`-format/`.format()`/ /// `+`-concatenated argument). Python, @@ -72,7 +75,8 @@ pub enum Lang { /// grammar parses JSX out of the box, even in a plain `.js` file — the /// `dangerouslySetInnerHTML` JSX attribute; see `Lang::Tsx`'s doc comment /// for why `.tsx` needs a separate grammar variant for the JSX form of - /// this rule but `.js`/`.jsx` do not). + /// this rule but `.js`/`.jsx` do not), `insecure-crypto` + /// (`crypto.createCipher`/`createDecipher`). JavaScript, /// `.ts` — `dynamic-eval` (`eval`/`Function`, including the /// `new Function(...)` constructor-call form), `shell-invoking-subprocess` @@ -86,7 +90,8 @@ pub enum Lang { /// — but NOT the `dangerouslySetInnerHTML` JSX-attribute form, since plain /// `.ts` files can't contain JSX syntax and `tree-sitter-typescript`'s /// `LANGUAGE_TYPESCRIPT` grammar has no JSX node kinds at all; see - /// `Lang::Tsx` for the `.tsx` variant that does parse JSX). + /// `Lang::Tsx` for the `.tsx` variant that does parse JSX), `insecure-crypto` + /// (`crypto.createCipher`/`createDecipher`). TypeScript, /// `.tsx` — same rule set as `Lang::TypeScript` (all of `.ts`'s rules /// apply verbatim, since `.tsx` is a superset of `.ts` syntax), PLUS the @@ -133,8 +138,10 @@ fn ts_language(lang: &Lang) -> TsLanguage { /// Rule set. `dynamic-eval` was ported as-is from treeband; /// `shell-invoking-subprocess` (issue #253), `insecure-deserialize` /// (issue #254), `tls-verify-disabled` (issue #255), `dynamic-module-load` -/// (issue #256), `sql-string-interpolation` (issue #257), and -/// `rust-unsafe-block` (issue #258) were added directly in clawband. Each +/// (issue #256), `sql-string-interpolation` (issue #257), +/// `rust-unsafe-block` (issue #258), and `insecure-crypto`/additional +/// `tls-verify-disabled` forms (issue #264) were added directly in +/// clawband. Each /// rule is a tree-sitter query, not a regex — it matches AST structure, so /// `// eval(x)` in a comment or `"eval(x)"` in a string literal never /// matches, unlike a naive text search. @@ -471,6 +478,30 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { "cross-site scripting (XSS) sink — React's dangerouslySetInnerHTML renders its __html value as raw HTML, executing attacker-controlled markup/script if the value isn't fully trusted; sanitize with a library like DOMPurify or avoid raw HTML rendering", )); } + // insecure-crypto (issue #264): `crypto.createCipher(...)` / + // `crypto.createDecipher(...)` (Node) — removed entirely in + // Node 22, and even where still available they derive the key + // from the passphrase with a single unsalted MD5 hash instead + // of a proper KDF, and (for createCipher) always use a fixed/ + // zero IV — both a weak-key-derivation and IV-reuse problem. + // Deliberately scoped to the `crypto.` member-expression form + // only (mirrors the `vm.runInNewContext` scoping above); the + // string-literal cipher-algorithm form (e.g. `"aes-128-ecb"` + // passed to `crypto.createCipheriv`) is a string *value*, not a + // code *structure*, and is intentionally left to clawband's + // existing regex layer rather than added as an AST rule here — + // see the issue #264 discussion for why AST matching is a poor + // fit for flagging string contents rather than syntax shapes. + rules.push(( + "insecure-crypto", + r#"(call_expression + function: (member_expression + object: (identifier) @obj + property: (property_identifier) @method) + (#eq? @obj "crypto") + (#match? @method "^(createCipher|createDecipher)$"))"#, + "insecure key derivation — crypto.createCipher()/createDecipher() derive the key from the passphrase with a single unsalted hash and were removed in Node 22; use crypto.createCipheriv()/createDecipheriv() with an explicit, properly-derived key and a random IV instead", + )); rules } Lang::Python => vec![ @@ -594,6 +625,70 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { (#eq? @kw "verify"))"#, tls_verify_disabled_reason, ), + ( + "tls-verify-disabled", + // `ssl._create_unverified_context()` — a call expression, a + // different AST shape from the `verify=False` keyword-arg + // rule above (issue #264). Scoped to the `ssl.` module + // qualifier and this exact function name, mirroring how + // `os.system`/`os.popen` above are scoped to `os.` — this is + // the documented, deliberate way to disable TLS verification + // via the stdlib `ssl` module (as opposed to a merely + // similarly-named function on an unrelated object). + r#"(call + function: (attribute + object: (identifier) @obj + attribute: (identifier) @method) + (#eq? @obj "ssl") + (#eq? @method "_create_unverified_context"))"#, + tls_verify_disabled_reason, + ), + ( + "tls-verify-disabled", + // `check_hostname=False` — same keyword-argument shape as + // `verify=False` above but a different keyword name; this + // disables hostname verification on an `ssl.SSLContext` + // (commonly paired with `_create_unverified_context`, but + // also settable directly on a context instance, hence not + // restricted to a specific callee — mirrors how the + // `verify=False` rule above intentionally doesn't restrict + // by callee either, since `requests`-style APIs are called + // in too many different ways to enumerate). + r#"(call + arguments: (argument_list + (keyword_argument + name: (identifier) @kw + value: (false))) + (#eq? @kw "check_hostname"))"#, + tls_verify_disabled_reason, + ), + ( + "insecure-crypto", + // `AES.MODE_ECB` (PyCryptodome/PyCrypto) — an `attribute` + // node (object/attribute fields), NOT a `call` — ECB mode is + // selected by passing this constant to `AES.new(...)`, not + // by calling anything itself, so this needs the different + // node shape used by python_yaml_load_findings's argument + // walk rather than the `call`-based shape every other rule + // in this vec uses. Deliberately requires the object to be + // exactly `AES` (`#eq? @obj "AES"`) rather than matching a + // bare `.MODE_ECB` on any object, mirroring how the JS + // `document.write` rule above requires the object to be + // exactly `document` — avoids flagging an unrelated + // `SomeOtherEnum.MODE_ECB`-shaped access. The string-literal + // form (e.g. `Cipher.new(key, AES.MODE_ECB)` is fine, but + // `"aes-128-ecb"` passed as a mode string to a different + // crypto library) is a string *value*, not code *structure*, + // and is intentionally left out of scope here — same + // reasoning as the `crypto.createCipher` string-literal + // exclusion above. + r#"(attribute + object: (identifier) @obj + attribute: (identifier) @attr + (#eq? @obj "AES") + (#eq? @attr "MODE_ECB"))"#, + "insecure block cipher mode — AES in ECB mode encrypts identical plaintext blocks to identical ciphertext blocks, leaking structural information about the data (the classic \"ECB penguin\" problem); use an authenticated mode like AES-GCM instead", + ), ], Lang::Rust => vec![ ( @@ -2123,6 +2218,49 @@ mod tests { assert!(!has_tls_verify_disabled_finding(&findings)); } + // ── tls-verify-disabled: ssl._create_unverified_context / check_hostname (issue #264) ── + + #[test] + fn python_flags_ssl_create_unverified_context() { + let findings = scan("ctx = ssl._create_unverified_context()", Lang::Python); + assert!(has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_create_unverified_context_on_unrelated_object() { + // Scoped to the `ssl.` qualifier specifically (issue #264) — a + // similarly-named method on an unrelated object must not flag. + let findings = scan("ctx = mymodule._create_unverified_context()", Lang::Python); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_ssl_create_unverified_context_mention_in_comment() { + let findings = scan( + "# ssl._create_unverified_context() is bad\nprint(1)", + Lang::Python, + ); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_flags_check_hostname_false() { + let findings = scan("ctx.wrap_socket(sock, check_hostname=False)", Lang::Python); + assert!(has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_check_hostname_true() { + let findings = scan("ctx.wrap_socket(sock, check_hostname=True)", Lang::Python); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_check_hostname_mention_in_string_literal() { + let findings = scan(r#"s = "check_hostname=False""#, Lang::Python); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + #[test] fn js_flags_reject_unauthorized_false() { let findings = scan( @@ -2180,6 +2318,112 @@ mod tests { assert!(has_tls_verify_disabled_finding(&findings)); } + // ── insecure-crypto (issue #264) ── + + fn has_insecure_crypto_finding(findings: &[Finding]) -> bool { + findings.iter().any(|f| f.rule == "insecure-crypto") + } + + #[test] + fn js_flags_crypto_create_cipher() { + let findings = scan( + r#"const c = crypto.createCipher("aes192", password);"#, + Lang::JavaScript, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_flags_crypto_create_decipher() { + let findings = scan( + r#"const c = crypto.createDecipher("aes192", password);"#, + Lang::JavaScript, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_ignores_create_cipheriv() { + // The safe replacement API must not be flagged — only the removed, + // insecure `createCipher`/`createDecipher` forms are in scope. + let findings = scan( + r#"const c = crypto.createCipheriv("aes-256-gcm", key, iv);"#, + Lang::JavaScript, + ); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_ignores_create_cipher_on_unrelated_object() { + // Scoped to the `crypto.` qualifier specifically (issue #264) — a + // similarly-named method on an unrelated object must not flag. + let findings = scan( + r#"const c = myLib.createCipher("aes192", password);"#, + Lang::JavaScript, + ); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_ignores_create_cipher_mention_in_comment() { + let findings = scan( + "// crypto.createCipher(...) is insecure\nfunction f(){}", + Lang::JavaScript, + ); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_ignores_create_cipher_mention_in_string_literal() { + let findings = scan( + r#"const s = "crypto.createCipher(algo, password)";"#, + Lang::JavaScript, + ); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn ts_flags_crypto_create_cipher() { + let findings = scan( + r#"const c = crypto.createCipher("aes192", password);"#, + Lang::TypeScript, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_flags_aes_mode_ecb() { + let findings = scan("cipher = AES.new(key, AES.MODE_ECB)", Lang::Python); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_ignores_aes_mode_cbc() { + let findings = scan("cipher = AES.new(key, AES.MODE_CBC, iv)", Lang::Python); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_ignores_mode_ecb_on_unrelated_object() { + // Scoped to the `AES` object specifically (issue #264) — a + // similarly-named `.MODE_ECB` attribute on an unrelated object must + // not flag. + let findings = scan("x = SomeOtherEnum.MODE_ECB", Lang::Python); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_ignores_aes_mode_ecb_mention_in_comment() { + let findings = scan("# AES.MODE_ECB is insecure\nprint(1)", Lang::Python); + assert!(!has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_ignores_aes_mode_ecb_mention_in_string_literal() { + let findings = scan(r#"s = "AES.MODE_ECB""#, Lang::Python); + assert!(!has_insecure_crypto_finding(&findings)); + } + #[test] fn rust_flags_danger_accept_invalid_certs_true() { let findings = scan( From 2ded148e84705b626f233ca489f6f80cbed8279c Mon Sep 17 00:00:00 2001 From: James Soubry Date: Fri, 25 Sep 2026 11:47:12 +0000 Subject: [PATCH 2/3] fix: catch check_hostname assignment form + missing e2e tests + inaccurate ECB comment (v3.22.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Greptile review findings on PR #305: - tls-verify-disabled now also matches the `ctx.check_hostname = False` attribute-assignment form (a Python `assignment` node), which previously escaped detection alongside the existing `check_hostname=False` keyword-argument rule. - Corrects a comment near the AES-ECB rule that inaccurately claimed the string-literal cipher-mode form (e.g. "aes-128-ecb") is covered by an existing regex layer; no such fallback exists, so the comment now states plainly that this is an accepted, out-of-scope gap. - Adds e2e tests in tests/cli.rs for all five crypto/TLS AST rules from this PR (insecure-crypto's createCipher/createDecipher and AES.MODE_ECB, tls-verify-disabled's ssl._create_unverified_context(), check_hostname keyword-arg, and the new check_hostname attribute-assignment form), plus their false-positive counterparts. - Dedupes identical [rule] reason lines in the ast-ask prompt so two rule matches sharing the same reason string (e.g. verify=False and ssl._create_unverified_context() in the same write) don't repeat the same warning twice. *— Claude (Sonnet 5), clawband backlog automation* --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/ast_guard.rs | 71 +++++++++++++++++++++++++-- src/main.rs | 18 +++++-- tests/cli.rs | 121 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 204 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 103a1ecf..ed8564f3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.22.0" +version = "3.22.1" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index a4a72c8e..3ba96063 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.22.0" +version = "3.22.1" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/src/ast_guard.rs b/src/ast_guard.rs index 299a8c10..c8165f16 100644 --- a/src/ast_guard.rs +++ b/src/ast_guard.rs @@ -488,10 +488,12 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { // only (mirrors the `vm.runInNewContext` scoping above); the // string-literal cipher-algorithm form (e.g. `"aes-128-ecb"` // passed to `crypto.createCipheriv`) is a string *value*, not a - // code *structure*, and is intentionally left to clawband's - // existing regex layer rather than added as an AST rule here — - // see the issue #264 discussion for why AST matching is a poor - // fit for flagging string contents rather than syntax shapes. + // code *structure*, and is intentionally left unflagged here — + // no other clawband layer (regex or otherwise) currently covers + // it either, so this is a known, accepted gap, not a + // fallback-covered one; see the issue #264 discussion for why + // AST matching is a poor fit for flagging string contents rather + // than syntax shapes. rules.push(( "insecure-crypto", r#"(call_expression @@ -662,6 +664,28 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { (#eq? @kw "check_hostname"))"#, tls_verify_disabled_reason, ), + ( + "tls-verify-disabled", + // `ctx.check_hostname = False` — attribute *assignment*, a + // different AST shape (Python's `assignment` node, with an + // `attribute` node as its `left` field) from the + // `check_hostname=False` *keyword-argument* rule immediately + // above (Greptile review round, PR #305/issue #264): setting + // the attribute directly on an already-constructed + // `ssl.SSLContext` instance is an equally common and equally + // dangerous way to disable hostname verification, and was + // passing through undetected. Left unscoped by object name + // (matches any `.check_hostname` attribute assignment), same + // reasoning as the keyword-arg rule above: SSLContext + // instances are constructed and named in too many different + // ways to enumerate a fixed set of object names. + r#"(assignment + left: (attribute + attribute: (identifier) @attr) + right: (false) + (#eq? @attr "check_hostname"))"#, + tls_verify_disabled_reason, + ), ( "insecure-crypto", // `AES.MODE_ECB` (PyCryptodome/PyCrypto) — an `attribute` @@ -2261,6 +2285,45 @@ mod tests { assert!(!has_tls_verify_disabled_finding(&findings)); } + // ── tls-verify-disabled: check_hostname attribute assignment + // (Greptile review, PR #305) ── + + #[test] + fn python_flags_check_hostname_attribute_assignment() { + let findings = scan("ctx.check_hostname = False", Lang::Python); + assert!(has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_flags_check_hostname_attribute_assignment_unscoped_object() { + // Unscoped by object name, mirroring the keyword-arg rule's own lack + // of a callee restriction — an unrelated object's `.check_hostname` + // attribute still flags. + let findings = scan("some_other_thing.check_hostname = False", Lang::Python); + assert!(has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_check_hostname_attribute_assignment_true() { + let findings = scan("ctx.check_hostname = True", Lang::Python); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_check_hostname_attribute_assignment_mention_in_comment() { + let findings = scan( + "# ctx.check_hostname = False is bad\nprint(1)", + Lang::Python, + ); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + + #[test] + fn python_ignores_check_hostname_attribute_assignment_mention_in_string_literal() { + let findings = scan(r#"s = "ctx.check_hostname = False""#, Lang::Python); + assert!(!has_tls_verify_disabled_finding(&findings)); + } + #[test] fn js_flags_reject_unauthorized_false() { let findings = scan( diff --git a/src/main.rs b/src/main.rs index 8e6519b1..994f9afe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -7249,10 +7249,20 @@ fn main() { if let Some(lang) = ast_guard::detect_language(raw_path) { let findings = ast_guard::scan(content, lang); if !findings.is_empty() { - let reasons: Vec = findings - .iter() - .map(|f| format!("[{}] {}", f.rule, f.reason)) - .collect(); + // Dedup identical `[rule] reason` lines before joining: + // a single Write can trip two different rule *matches* + // that happen to carry the same shared reason string + // (e.g. `verify=False` and `ssl._create_unverified_context()` + // both use `tls_verify_disabled_reason` — Greptile review, + // PR #305), which otherwise repeats the same warning + // verbatim in the prompt with no added information. + let mut reasons: Vec = Vec::with_capacity(findings.len()); + for f in &findings { + let line = format!("[{}] {}", f.rule, f.reason); + if !reasons.contains(&line) { + reasons.push(line); + } + } let reason = format!( "clawband: structural check flagged this write:\n{}", reasons.join("\n") diff --git a/tests/cli.rs b/tests/cli.rs index 3ce00a9d..bb60f8f1 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -914,6 +914,127 @@ fn e2e_ast_guard_ignores_rust_danger_accept_invalid_certs_false() { assert_eq!(decision(&out), None, "{out}"); } +// ── issue #264: insecure-crypto / additional tls-verify-disabled forms ──── + +#[test] +fn e2e_ast_guard_flags_js_crypto_create_cipher() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const c = crypto.createCipher('aes192', password);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("insecure-crypto"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_crypto_create_decipher() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const d = crypto.createDecipher('aes192', password);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("insecure-crypto"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_js_crypto_create_cipheriv() { + // Required false-positive test: the modern, non-deprecated createCipheriv + // (explicit IV, no unsalted single-hash KDF) must not be flagged. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const c = crypto.createCipheriv('aes-256-gcm', key, iv);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_js_crypto_create_cipher_mention_in_comment() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"// crypto.createCipher is insecure\nfunction f(){return 1;}"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_python_aes_mode_ecb() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"cipher = AES.new(key, AES.MODE_ECB)"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("insecure-crypto"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_python_aes_mode_cbc() { + // Required false-positive test: an authenticated/safer mode must not flag. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"cipher = AES.new(key, AES.MODE_GCM)"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_python_mode_ecb_on_unrelated_object() { + // Required false-positive test: scoped to `AES.` specifically. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"x = SomeOtherEnum.MODE_ECB"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_python_ssl_create_unverified_context() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx = ssl._create_unverified_context()"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("tls-verify-disabled"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_create_unverified_context_on_unrelated_object() { + // Required false-positive test: scoped to the `ssl.` qualifier. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx = mymodule._create_unverified_context()"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_python_check_hostname_false_keyword_arg() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx.wrap_socket(sock, check_hostname=False)"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("tls-verify-disabled"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_python_check_hostname_true_keyword_arg() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx.wrap_socket(sock, check_hostname=True)"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_python_check_hostname_false_attribute_assignment() { + // Greptile review, PR #305: `ctx.check_hostname = False` is an attribute + // *assignment*, a different AST shape from the keyword-argument form + // above, and was passing through undetected before this fix. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx.check_hostname = False"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("tls-verify-disabled"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_python_check_hostname_true_attribute_assignment() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"ctx.check_hostname = True"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_check_hostname_mention_in_comment() { + let json = r##"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"# ctx.check_hostname = False is bad\nprint(1)"}}"##; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_check_hostname_mention_in_string_literal() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.py","content":"s = \"ctx.check_hostname = False\""}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + // ── issue #256: dynamic-module-load ─────────────────────────────────────── #[test] From 26958f34575506be8cef0198a520758f57eba2a4 Mon Sep 17 00:00:00 2001 From: James Soubry Date: Sat, 26 Sep 2026 18:01:57 +0000 Subject: [PATCH 3/3] fix: widen insecure-crypto scope to DES/DES3/Blowfish MODE_ECB and crypto[] bracket notation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-opinion review on this PR flagged two gaps in the crypto/TLS rules that were never followed up on: - The AES.MODE_ECB rule was scoped to the AES object only, but PyCryptodome's DES, DES3, and Blowfish modules expose the identical MODE_ECB constant and were silently unflagged. - crypto["createCipher"](...) bracket notation (a subscript_expression, distinct from the member_expression the existing rule matched) bypassed the insecure-crypto rule entirely. Both fixed by widening the existing query patterns rather than adding new rules, mirroring the subscript_expression alternative already used by the innerHTML/outerHTML XSS rules in this same file. *— Claude (Sonnet 5), clawband backlog automation* --- src/ast_guard.rs | 86 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 9 deletions(-) diff --git a/src/ast_guard.rs b/src/ast_guard.rs index c8165f16..9f209045 100644 --- a/src/ast_guard.rs +++ b/src/ast_guard.rs @@ -496,10 +496,21 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { // than syntax shapes. rules.push(( "insecure-crypto", + // Covers both `crypto.createCipher(...)` (dot access, + // `member_expression`) and `crypto["createCipher"](...)` + // (computed/bracket access, `subscript_expression` — see the + // innerHTML/outerHTML XSS rules above for why this needs its + // own alternative rather than being covered by the dot-access + // pattern alone). r#"(call_expression - function: (member_expression - object: (identifier) @obj - property: (property_identifier) @method) + function: [ + (member_expression + object: (identifier) @obj + property: (property_identifier) @method) + (subscript_expression + object: (identifier) @obj + index: (string (string_fragment) @method)) + ] (#eq? @obj "crypto") (#match? @method "^(createCipher|createDecipher)$"))"#, "insecure key derivation — crypto.createCipher()/createDecipher() derive the key from the passphrase with a single unsalted hash and were removed in Node 22; use crypto.createCipheriv()/createDecipheriv() with an explicit, properly-derived key and a random IV instead", @@ -695,10 +706,13 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { // node shape used by python_yaml_load_findings's argument // walk rather than the `call`-based shape every other rule // in this vec uses. Deliberately requires the object to be - // exactly `AES` (`#eq? @obj "AES"`) rather than matching a - // bare `.MODE_ECB` on any object, mirroring how the JS - // `document.write` rule above requires the object to be - // exactly `document` — avoids flagging an unrelated + // one of PyCryptodome's block-cipher modules that expose the + // identical `MODE_ECB` constant (`AES`, `DES`, `DES3`, + // `Blowfish` — confirmed against PyCryptodome's docs, all four + // share the same `Crypto.Cipher._mode_ecb` constant) rather + // than matching a bare `.MODE_ECB` on any object, mirroring + // how the JS `document.write` rule above requires the object + // to be exactly `document` — avoids flagging an unrelated // `SomeOtherEnum.MODE_ECB`-shaped access. The string-literal // form (e.g. `Cipher.new(key, AES.MODE_ECB)` is fine, but // `"aes-128-ecb"` passed as a mode string to a different @@ -709,9 +723,9 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { r#"(attribute object: (identifier) @obj attribute: (identifier) @attr - (#eq? @obj "AES") + (#match? @obj "^(AES|DES|DES3|Blowfish)$") (#eq? @attr "MODE_ECB"))"#, - "insecure block cipher mode — AES in ECB mode encrypts identical plaintext blocks to identical ciphertext blocks, leaking structural information about the data (the classic \"ECB penguin\" problem); use an authenticated mode like AES-GCM instead", + "insecure block cipher mode — ECB mode encrypts identical plaintext blocks to identical ciphertext blocks, leaking structural information about the data (the classic \"ECB penguin\" problem); use an authenticated mode like AES-GCM instead", ), ], Lang::Rust => vec![ @@ -2454,12 +2468,66 @@ mod tests { assert!(has_insecure_crypto_finding(&findings)); } + #[test] + fn js_flags_crypto_create_cipher_bracket_notation() { + // Second-opinion review finding (PR #305, 2026-09-26): the + // dot-access-only pattern let `crypto["createCipher"](...)` (bracket + // notation, a `subscript_expression`) bypass detection entirely. + let findings = scan( + r#"const c = crypto["createCipher"]("aes192", password);"#, + Lang::JavaScript, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_flags_crypto_create_decipher_bracket_notation() { + let findings = scan( + r#"const c = crypto["createDecipher"]("aes192", password);"#, + Lang::JavaScript, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn js_ignores_create_cipher_bracket_notation_on_unrelated_object() { + let findings = scan( + r#"const c = myLib["createCipher"]("aes192", password);"#, + Lang::JavaScript, + ); + assert!(!has_insecure_crypto_finding(&findings)); + } + #[test] fn python_flags_aes_mode_ecb() { let findings = scan("cipher = AES.new(key, AES.MODE_ECB)", Lang::Python); assert!(has_insecure_crypto_finding(&findings)); } + #[test] + fn python_flags_des_mode_ecb() { + // Second-opinion review finding (PR #305, 2026-09-26): the rule was + // scoped to `AES` only, but PyCryptodome's `DES`, `DES3`, and + // `Blowfish` modules expose the identical `MODE_ECB` constant. + let findings = scan("cipher = DES.new(key, DES.MODE_ECB)", Lang::Python); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_flags_des3_mode_ecb() { + let findings = scan("cipher = DES3.new(key, DES3.MODE_ECB)", Lang::Python); + assert!(has_insecure_crypto_finding(&findings)); + } + + #[test] + fn python_flags_blowfish_mode_ecb() { + let findings = scan( + "cipher = Blowfish.new(key, Blowfish.MODE_ECB)", + Lang::Python, + ); + assert!(has_insecure_crypto_finding(&findings)); + } + #[test] fn python_ignores_aes_mode_cbc() { let findings = scan("cipher = AES.new(key, AES.MODE_CBC, iv)", Lang::Python);