From 97a9fa63d27f8c357547fd657e9fa52740ce1b7e Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:09:18 -0300 Subject: [PATCH 01/27] Plan cached file purposes in a dry run A dry run left files that need a file-purpose answer unplanned, even when the cache held that answer, so the units a run then sends for them were missing and requests that depend on them (tests, outlines) differed. On devise the estimate counted 106 of 1,532 first-pass requests as new while the run sent none. The preview now records a cached purpose and plans the file as a run does. --- CHANGELOG.md | 2 ++ src/evaluate.rs | 25 ++++++++++++++++++++----- src/file_kind.rs | 18 ++++++++++++++++++ src/requests.rs | 8 ++++---- src/tests/mod.rs | 5 ++++- 5 files changed, 48 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b135b2..258a95f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +- `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. + ## [0.19.0] - 2026-09-25 - Inline suppressions: a comment `jevgate: allow(RULE) reason` on a finding's line, or in the comments and attributes directly above it, accepts that finding as the baseline does. RULE is a rule ID, key or group, and the reason is required; the report keeps the finding with its reason (`suppressed`, and `gate.suppressed_findings`), and `jevgate baseline` leaves it out. diff --git a/src/evaluate.rs b/src/evaluate.rs index ed9b24d..c9d7c6e 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -123,9 +123,10 @@ fn empty_report(args: &CheckArgs, current: &SnapshotContext<'_>, files: Vec {} - Ok(Scheduled::Purpose(request)) => planned.push(request), + Ok(Scheduled::Purpose(request)) => { + if let Some(body) = crate::requests::answered(root, args, &request) { + let file = &mut report.files[owner]; + let view = crate::file_kind::record_purpose(file, &request, &body) + .and_then(|()| crate::file_kind::decide_after_purpose(input, args, file)); + match view { + Ok(Some(view)) => { + views.insert(owner, view); + } + Ok(None) => {} + Err(error) => report.errors.push(error.to_string()), + } + } + planned.push(request); + } Ok(Scheduled::Ready(view)) => { views.insert(owner, *view); } @@ -155,7 +170,7 @@ fn preview(inputs: &[Input], args: &CheckArgs, root: &std::path::Path, report: & .or_default(); stage.planned_requests += 1; stage.planned_evidence_bytes += crate::requests::evidence_bytes(&request); - if crate::requests::answered(root, args, &request) { + if crate::requests::answered(root, args, &request).is_some() { stage.planned_cached += 1; } else { stage.planned_tokens += budget.request_tokens(&request) as u64; diff --git a/src/file_kind.rs b/src/file_kind.rs index 650aff7..e2fae04 100644 --- a/src/file_kind.rs +++ b/src/file_kind.rs @@ -896,6 +896,24 @@ mod tests { } } + #[test] + fn a_dry_run_plans_the_units_of_a_file_whose_purpose_the_cache_answers() { + let project = Project::new(); + project.write("tests/support.rs", SUPPORT); + let mut options = args(); + options.rules = vec![crate::catalog::FUNCTION_SIMPLIFICATION.into()]; + run(&project, &options, &mut PurposeEval::new("mixed")); + options.dry_run = true; + let stages = crate::tests::snapshot(&project, &options).1.stages; + let planned = |stage: &str| (stages[stage].planned_requests, stages[stage].planned_cached); + assert_eq!(planned("file-purpose"), (1, 1)); + assert_eq!( + planned("functions"), + (1, 1), + "the units a run sends are planned" + ); + } + struct PurposeEval { mode: &'static str, calls: usize, diff --git a/src/requests.rs b/src/requests.rs index 099d901..3d6342a 100644 --- a/src/requests.rs +++ b/src/requests.rs @@ -92,17 +92,17 @@ fn cached_answer( .filter(|(b, _)| response::validate(b, request).is_ok()) } -/// Whether a dry run's planned request already has a cached answer, read -/// without opening the store. +/// A dry run's cached answer to a planned request, read without opening the +/// store. pub(super) fn answered( root: &std::path::Path, args: &crate::options::CheckArgs, request: &Value, -) -> bool { +) -> Option { cached_answer(args, request, |key, ttl| { crate::storage::peek(root, key, ttl) }) - .is_some() + .map(|(body, _)| body) } impl Session<'_> { diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 3dd85b1..3f738d4 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -167,7 +167,10 @@ pub(super) fn session<'a>( } /// The selected inputs and the first snapshot of a check, before evaluation. -fn snapshot(project: &Project, options: &CheckArgs) -> (Vec, schema::Report) { +pub(super) fn snapshot( + project: &Project, + options: &CheckArgs, +) -> (Vec, schema::Report) { let context = project.context(); let scope = inventory::scope(options, &context).unwrap(); let inputs = inventory::collect(options, &context, &scope).unwrap(); From b7af8e021bbe2ec47e12eabb5c08ea0a115b2408 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:23:52 -0300 Subject: [PATCH 02/27] Ask about deserializers in every language that names one Only Django views and PHP pages naming unserialize were asked whether they load data with a deserializer that can build any object. A Flask route passing pickle.loads(request.get_data()) was asked only about query, command, code and markup text, and was clear. Code whose source names such a deserializer (Python's pickle, marshal, shelve, jsonpickle and yaml.load; Ruby's Marshal.load and YAML.load; Java's ObjectInputStream, XMLDecoder, XStream and SnakeYAML; node-serialize) now gets the deserializer form of the presence question and its language's deserialize check in the trace. Other requests are byte-identical, so their cached answers stay valid: across 65 projects only gson (3) and pygoat (1) re-asked a request. pygoat's insecure deserialization lab went from a note to a review, and a seeded Flask canary's pickle route from clear to a review. --- CHANGELOG.md | 1 + docs/classification-cascade.md | 11 ++- src/catalog.rs | 2 +- src/units/questions/mod.rs | 15 +++- src/units/questions/security.rs | 126 +++++++++++++++++++++++++++++++- src/units/security.rs | 17 ++++- src/units/tests/security.rs | 61 ++++++++++++++++ 7 files changed, 222 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 258a95f..a7b017b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +- Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. ## [0.19.0] - 2026-09-25 diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index 9b99ac2..f3291fc 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -300,7 +300,16 @@ signatures, or one candidate pair. identifiers quoted by doubling embedded quotes as handled (identifiers cannot be bound), and the URL check excludes requests a web page sends from the user's browser; on fresh repositories both had flagged such code, while - the SQL and SSRF advisory functions kept their answers. + the SQL and SSRF advisory functions kept their answers. Code whose source + names a deserializer that can build any object (Python's `pickle`, + `marshal`, `shelve`, `jsonpickle` or `yaml.load`; Ruby's `Marshal.load` + or `YAML.load`; Java's `ObjectInputStream`, `XMLDecoder`, XStream or + SnakeYAML; node-serialize) is asked about loading data with it in the + presence question, and its trace asks that language's deserialize check, + as Django views and PHP pages naming `unserialize` are: a Flask route + passing `pickle.loads(request.get_data())` was asked only about query, + command, code and markup text, and was clear. Only the requests of such + functions change. PHP units read the presence questions and checks in PHP's own terms (`src/units/questions/php.rs`), naming its functions (`echo`, `shell_exec` and backticks, `mysqli_real_escape_string`, `password_hash`, diff --git a/src/catalog.rs b/src/catalog.rs index 203deaa..3fc6945 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -277,7 +277,7 @@ pub fn rule_version(key: &str) -> &'static str { SHARED_LOGIC => "19", TEST_VALUE => "5", TEST_REDUNDANCY => "3", - INJECTION => "6", + INJECTION => "7", SENSITIVE_DATA => "5", HARDCODED_VALUES | UNSAFE_SETTINGS => "4", AGENT_CONTEXT => "3", diff --git a/src/units/questions/mod.rs b/src/units/questions/mod.rs index f7d9b4d..832b4f1 100644 --- a/src/units/questions/mod.rs +++ b/src/units/questions/mod.rs @@ -162,7 +162,15 @@ mod tests { .chain(&DJANGO_UNHANDLED) .chain(&DJANGO_SETTINGS) .chain(&DJANGO_EXPOSURES) - .chain(&PHP_UNHANDLED); + .chain(&PHP_UNHANDLED) + .chain( + [ + ("Ruby", "Marshal.load"), + ("Java", "new ObjectInputStream(body)"), + ("JavaScript", "require('node-serialize')"), + ] + .map(|(language, source)| deserializer_check(language, source).unwrap()), + ); let mut all = vec![ security_logs_secret("function.source"), security_url_parts("function.source", false), @@ -195,7 +203,8 @@ mod tests { } for django in [false, true] { all.extend([ - security_interpreted("function.source", django), + security_interpreted("function.source", django, None), + security_interpreted("function.source", django, Some("pickle")), security_resource("function.source", django), security_error_details("function.source", django), security_weakened("function.source", django), @@ -207,7 +216,7 @@ mod tests { for (id, mut body) in [ ( "interpreted", - security_interpreted("function.source", false), + security_interpreted("function.source", false, None), ), ("resource", security_resource("function.source", false)), ( diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index fb35b2d..5f17873 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -8,8 +8,21 @@ use serde_json::{Value, json}; /// renders. Presence only: the trace questions decide whether it is a concern. /// In Django code it also names markup marked safe and deserializers, since /// `mark_safe`, `|safe` templates and `pickle.loads` of request data were -/// the injections its views held. -pub fn security_interpreted(code: &str, django: bool) -> Value { +/// the injections its views held. Other code whose source names a +/// deserializer that can build any object is asked about it too, naming +/// `deserializers` (see [`deserializers_named`]). +pub fn security_interpreted(code: &str, django: bool, deserializers: Option<&str>) -> Value { + if let (false, Some(names)) = (django, deserializers) { + return noul( + format!( + "Does `{code}` place a variable into the text of a database query, shell command, code to evaluate, or HTML markup, or load it with a deserializer that can build any object?" + ), + &format!( + "A variable is joined, formatted or interpolated into the text of a query, command, code or markup that is then run or rendered, or loaded with a deserializer that can build any object or run code, such as {names}." + ), + "Variables are passed only as bound parameters, separate arguments, or through a template or component that escapes them; the text is built only from fixed values; data is parsed only as JSON or another data-only format; or the function builds no such text.", + ); + } if django { return noul( format!( @@ -607,6 +620,115 @@ const DESERIALIZE: Check = Check { no_examples: &[], }; +/// The deserialize check in the terms of a language other than Python. +const fn deserialize(yes: &'static str, no: &'static str) -> Check { + Check { + id: "deserialize", + question: DESERIALIZE.question, + yes, + no, + no_examples: &[], + } +} + +/// Deserializers that can build any object or run code, per language, as +/// (language, what its source must name in any case, how the presence +/// question names them, the trace check). Django asks its own check of +/// every view and PHP of source that names `unserialize`; other code was +/// never asked, so a Flask route passing `pickle.loads(request.get_data())` +/// was clear. The question and check are added only to source that names +/// one, so every other request, and its cached answer, stays as it was. +const DESERIALIZERS: [(&str, &[&str], &str, Check); 5] = [ + ( + "Python", + &[ + "pickle.load", + "pickle.unpickler", + "marshal.load", + "shelve.open", + "jsonpickle.decode", + "dill.load", + "yaml.load(", + "yaml.unsafe_load", + "yaml.full_load", + "yaml.load_all(", + ], + "pickle, marshal, shelve, jsonpickle or yaml.load without a safe loader", + DESERIALIZE, + ), + ( + "Ruby", + &[ + "marshal.load", + "yaml.load", + "yaml.unsafe_load", + "psych.load", + "oj.load", + ], + "Marshal.load, YAML.unsafe_load or Oj.load in object mode", + deserialize( + "Request data, an uploaded file, a cookie, a message or a stored value users can set is passed to Marshal.load, YAML.unsafe_load, YAML.load with unsafe options, Oj.load in object mode or a similar deserializer.", + "It parses JSON, uses YAML.safe_load or another data-only format, or loads only data the program wrote and signed itself.", + ), + ), + ( + "Java", + &[ + "objectinputstream", + "xmldecoder", + "fromxml(", + "enabledefaulttyping", + "activatedefaulttyping", + "new yaml(", + ], + "ObjectInputStream, XMLDecoder, XStream or SnakeYAML's Yaml.load", + deserialize( + "Request data, an uploaded file, a cookie, a message or a stored value users can set is read with ObjectInputStream.readObject, XMLDecoder, XStream.fromXML, SnakeYAML's Yaml.load, Jackson with default typing enabled or a similar deserializer.", + "It parses JSON into types fixed in the code, uses a safe constructor or an allowed list of classes, or loads only data the program wrote and signed itself.", + ), + ), + ( + "JavaScript", + &["node-serialize", "unserialize(", "funcster", "cryo.parse"], + "node-serialize's unserialize, funcster or cryo", + NODE_DESERIALIZE, + ), + ( + "TypeScript", + &["node-serialize", "unserialize(", "funcster", "cryo.parse"], + "node-serialize's unserialize, funcster or cryo", + NODE_DESERIALIZE, + ), +]; + +const NODE_DESERIALIZE: Check = deserialize( + "Request data, a cookie, a message or a stored value users can set is passed to node-serialize's unserialize, funcster, cryo or a similar deserializer that can restore functions.", + "It parses JSON with JSON.parse or another data-only format, or loads only data the program wrote and signed itself.", +); + +/// The deserializer entry of `language` whose names `source` holds. +fn deserializer_entry( + language: &str, + source: &str, +) -> Option<&'static (&'static str, &'static [&'static str], &'static str, Check)> { + let source = source.to_ascii_lowercase(); + DESERIALIZERS.iter().find(|(lang, names, ..)| { + *lang == language && names.iter().any(|name| source.contains(name)) + }) +} + +/// How the presence question names `language`'s deserializers, when +/// `source` names one of them. +pub fn deserializers_named(language: &str, source: &str) -> Option<&'static str> { + deserializer_entry(language, source).map(|(_, _, names, _)| *names) +} + +/// The deserialize check asked of `source` in `language`, when it names one +/// of the language's deserializers. +pub fn deserializer_check(language: &str, source: &str) -> Option<&'static Check> { + deserializer_entry(language, source).map(|(.., check)| check) +} + /// Specific weak settings, asked when the broad presence question is not clear. pub const WEAK_SETTINGS: [Check; 6] = [ Check { diff --git a/src/units/security.rs b/src/units/security.rs index e50553f..fdbf3a7 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -354,11 +354,13 @@ fn presence_request( } else { format!("functions[{index}].source") }; + let source = items[index].1["source"].as_str().unwrap_or_default(); + let deserializers = questions::deserializers_named(file.language, source); for (rule, _, id) in units { for question in presence_questions(rule) { questions.ask( format!("{}{index}_{question}", &key[..1]), - presence_body(question, &code, django), + presence_body(question, &code, django, deserializers), id, rule, question, @@ -385,9 +387,9 @@ pub(super) fn presence_questions(rule: &str) -> &'static [&'static str] { .map_or(&[], |(_, questions)| questions) } -fn presence_body(question: &str, code: &str, django: bool) -> Value { +fn presence_body(question: &str, code: &str, django: bool, deserializers: Option<&str>) -> Value { match question { - "interpreted" => questions::security_interpreted(code, django), + "interpreted" => questions::security_interpreted(code, django, deserializers), "resource" => questions::security_resource(code, django), "logs_secret" => questions::security_logs_secret(code), "error_details" => questions::security_error_details(code, django), @@ -444,7 +446,9 @@ fn rule_checks( /// The checks a rule's trace asks about `source` in a file in `language`; /// Django code (`django`) is asked the Django variant of a check where one /// exists, and the Django checks besides; PHP files are asked PHP's own -/// checks only of source that names what they ask about. +/// checks only of source that names what they ask about, and other code +/// the deserialize check of its language when its source names one of the +/// language's deserializers. fn asked_checks( rule: &str, language: &str, @@ -472,6 +476,11 @@ fn asked_checks( .chain(csharp) .chain(framework) .chain(php) + .chain( + (rule == INJECTION && !django) + .then(|| questions::deserializer_check(language, source)) + .flatten(), + ) .collect() } diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index 6a0ac58..0c6eea4 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -425,6 +425,67 @@ fn php_checks_name_php_functions_and_its_own_kinds_only_where_the_source_names_t } } +const PICKLED: &str = "import pickle\n\nfrom flask import jsonify, request\n\n\ndef restore_cart():\n cart = pickle.loads(request.get_data())\n return jsonify(items=len(cart))\n"; + +#[test] +fn a_deserializer_is_asked_about_only_where_the_source_names_one() { + let first = |path: &str, source: &str| { + let project = Project::new(); + project.write(path, source); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into()]; + let (_, plan) = planned(&project, &options); + plan.requests[0].request["questions"]["f0_interpreted"].clone() + }; + let named = first("shop/cart.py", PICKLED); + assert!(named.to_string().contains("pickle, marshal"), "{named}"); + let parsed = PICKLED + .replace("import pickle", "import json") + .replace("pickle.loads", "json.loads"); + assert_eq!( + first("shop/cart.py", &parsed), + questions::security_interpreted("functions[0].source", false, None), + "code that names no deserializer keeps its question and cached answer" + ); + assert!(traced_checks("shop/cart.py", PICKLED).contains_key("deserialize")); + assert!(!traced_checks("shop/cart.py", &parsed).contains_key("deserialize")); + let ruby = traced_checks( + "app/models/cart.rb", + "class Cart\n def self.restore(params)\n Marshal.load(Base64.decode64(params[:cart]))\n end\nend\n", + ); + assert!(ruby["deserialize"].to_string().contains("Marshal.load")); + let java = traced_checks( + "src/main/java/shop/Cart.java", + "class Cart {\n Object restore(InputStream body) throws Exception {\n return new ObjectInputStream(body).readObject();\n }\n}\n", + ); + assert!( + java["deserialize"] + .to_string() + .contains("ObjectInputStream") + ); +} + +#[test] +fn request_data_given_to_pickle_is_a_deserialization_review() { + let project = Project::new(); + project.write("shop/cart.py", PICKLED); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into()]; + let mut eval = scripted(0); + eval.overrides = vec![ + ("interpreted", noul_at(0.95)), + ("deserialize", noul_at(0.95)), + ("origin", spread(0.0, 0.05, 0.95)), + ]; + let report = run(&project, &options, &mut eval); + let finding = &report.files[0].findings[0]; + assert_eq!( + finding.category.as_deref(), + Some("CWE-502 deserialization of untrusted data") + ); + assert_eq!(finding.strength, Strength::Review); +} + const MARKUP_PARTS: [&str; 8] = [ "request", "stored", From 3afc68eb7c908909262fc2740bb3623fa76f2c45 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:29:07 -0300 Subject: [PATCH 03/27] Write counts in the singular or plural instead of (s) "8 new review finding(s)" and "1 file(s) not judged" read as form letters. The gate's reasons, the notes line, the GitHub summary and the baseline commands now say "1 finding" or "2 findings". --- CHANGELOG.md | 1 + src/command.rs | 14 +++++++++----- src/gate.rs | 7 ++++--- src/github.rs | 3 ++- src/output.rs | 9 +++++++-- src/tests/gating.rs | 6 +----- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7b017b..3ceb6ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. +- Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. ## [0.19.0] - 2026-09-25 diff --git a/src/command.rs b/src/command.rs index 341c97e..1f5bcf5 100644 --- a/src/command.rs +++ b/src/command.rs @@ -92,12 +92,15 @@ fn accept( let path = written.path.display(); if merge { say!( - "Accepted {} finding(s) from the last check in {path}; kept {} earlier finding(s) for files it did not cover", - written.accepted, - written.kept + "Accepted {} from the last check in {path}; kept {} for files it did not cover", + output::count(written.accepted, "finding"), + output::count(written.kept, "earlier finding") ); } else { - say!("Accepted {} finding(s) in {path}", written.accepted); + say!( + "Accepted {} in {path}", + output::count(written.accepted, "finding") + ); } Ok(0) } @@ -119,7 +122,8 @@ fn baseline_action(context: &ConfigContext, action: options::BaselineAction) -> } let marked = baseline::mark(&context.root, reason, &targets, &keys)?; say!( - "Marked {marked} accepted finding(s) as {}", + "Marked {} as {}", + output::count(marked, "accepted finding"), output::label(&reason) ); } diff --git a/src/gate.rs b/src/gate.rs index 90ee6c9..860f24e 100644 --- a/src/gate.rs +++ b/src/gate.rs @@ -77,10 +77,10 @@ fn failures(report: &Report, new: &[(&Path, &Finding)], args: &CheckArgs) -> Vec .count(); let consider = failing.len() - review; if review > 0 { - reasons.push(format!("{review} new review finding(s)")); + reasons.push(crate::output::count(review, "new review finding")); } if consider > 0 { - reasons.push(format!("{consider} new consider finding(s)")); + reasons.push(crate::output::count(consider, "new consider finding")); } let uncertain = |rule: &str, path: &Path| args.levels_at(rule, path).contains(&FailOn::Uncertain); @@ -99,7 +99,8 @@ fn failures(report: &Report, new: &[(&Path, &Finding)], args: &CheckArgs) -> Vec .count(); if undecided > 0 { reasons.push(format!( - "{undecided} file(s) with uncertain or needs-context results" + "{} with uncertain or needs-context results", + crate::output::count(undecided, "file") )); } reasons diff --git a/src/github.rs b/src/github.rs index 7fe9ac8..f0fd098 100644 --- a/src/github.rs +++ b/src/github.rs @@ -84,7 +84,8 @@ fn summary(report: &Report, shown: &[(&Path, &Finding)], args: &CheckArgs) -> St .count(); if failed > 0 { text.push_str(&format!( - "- **{failed} file(s) not judged;** the annotations give each reason.\n\n" + "- **{} not judged;** the annotations give each reason.\n\n", + crate::output::count(failed, "file") )); } if shown.is_empty() { diff --git a/src/output.rs b/src/output.rs index 77b4ce1..18055c9 100644 --- a/src/output.rs +++ b/src/output.rs @@ -17,6 +17,11 @@ const TOP_CONSIDER: usize = 10; pub const INPUT_USD_PER_MILLION: f64 = 0.042; pub const PRICE_CHECKED: &str = "2026-09-18"; +/// `n` and a noun, plural unless `n` is one: "1 finding", "2 findings". +pub fn count(n: usize, noun: &str) -> String { + format!("{n} {noun}{}", if n == 1 { "" } else { "s" }) +} + /// Estimated dollars for this invocation's paid input tokens, for a priced model. pub fn estimated_usd(report: &Report) -> Option { usd(&report.requested_model, report.paid_input_tokens) @@ -250,8 +255,8 @@ fn emit_findings(out: &mut impl Write, report: &Report, verbose: bool, style: St if !verbose { writeln!( out, - "\n{} optional note(s) on code that reads well as it is; --verbose shows them.", - notes.len() + "\n{} on code that reads well as it is; --verbose shows them.", + count(notes.len(), "optional note") )?; return Ok(()); } diff --git a/src/tests/gating.rs b/src/tests/gating.rs index 649e1a4..0308b84 100644 --- a/src/tests/gating.rs +++ b/src/tests/gating.rs @@ -82,11 +82,7 @@ fn a_scope_makes_its_paths_report_only_while_other_files_gate() { options.path_fail_on = vec![scripts(vec![options::FailOn::None])]; let report = run(&project, &options, &mut mock); let gate = report.gate.as_ref().unwrap(); - assert_eq!( - gate.reasons, - ["1 new consider finding(s)"], - "only src/lib.rs" - ); + assert_eq!(gate.reasons, ["1 new consider finding"], "only src/lib.rs"); options.paths = vec!["scripts".into()]; let report = run(&project, &options, &mut mock); assert_eq!(gate::exit_code(&report), 0, "tooling findings only report"); From 465b9334dbdd78523b04bf5a6a68d1422d8b555e Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:36:24 -0300 Subject: [PATCH 04/27] Show the errors a function's callees raise in its error-detail trace A FastAPI handler that returns str(exc) for the LookupError its service raised with the program's own text was a review: the exception check asks about "an exception it did not raise itself", and the handler did not. Twelve handlers of one project were reviews this way. The sensitive-data trace now lists the errors the functions it calls create, two calls deep in its own file or files it imports, and asks the exception check about whose text a response carries: a library's, the database's or the runtime's, or the program's own. On 65 projects only error-detail traces were asked again (/bin/zsh.01): ten of the twelve reviews became notes or considers, two tools returning the text of every exception they catch became reviews, and nothing else changed. --- CHANGELOG.md | 1 + docs/classification-cascade.md | 10 +++++- src/catalog.rs | 2 +- src/units/plan/security_units.rs | 57 ++++++++++++++++++++++++++++++++ src/units/questions/security.rs | 12 +++++++ src/units/security.rs | 18 ++++++++++ src/units/tests/security.rs | 51 ++++++++++++++++++++++++++++ 7 files changed, 149 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ceb6ed..dbd6da4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. +- Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. - Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index f3291fc..5c2ce68 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -177,7 +177,15 @@ signatures, or one candidate pair. message argument of each error the function creates and asks which one, if any, carries another error's text: the response is often written by an error handler in another file, and adding the handler to every unit also - cleared real leaks. Each registered error handler (`.onError(…)`, + cleared real leaks. The trace also lists the errors that the functions it + calls create, two calls deep in its own file or files it imports, with + their messages, and then asks the exception check about whose text a + response carries rather than who raised it: FastAPI handlers returning + `str(exc)` for the `LookupError` their service raised with the program's + own text ("Imóvel não encontrado") were twelve reviews in one project, + since the handler "did not raise it itself"; with the service's raise in + view, ten became notes or considers, and tools that return the text of + every exception they catch became reviews. Each registered error handler (`.onError(…)`, `.setErrorHandler(…)`, Express four-parameter `.use(…)` middleware, Flask and FastAPI decorators, NestJS `@Catch` filters, axum `IntoResponse` and actix-web `ResponseError` for an error type, Rocket catchers, ASP.NET Core diff --git a/src/catalog.rs b/src/catalog.rs index 3fc6945..325810d 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -278,7 +278,7 @@ pub fn rule_version(key: &str) -> &'static str { TEST_VALUE => "5", TEST_REDUNDANCY => "3", INJECTION => "7", - SENSITIVE_DATA => "5", + SENSITIVE_DATA => "6", HARDCODED_VALUES | UNSAFE_SETTINGS => "4", AGENT_CONTEXT => "3", COMMENTS => "2", diff --git a/src/units/plan/security_units.rs b/src/units/plan/security_units.rs index 3ff4776..e92bf85 100644 --- a/src/units/plan/security_units.rs +++ b/src/units/plan/security_units.rs @@ -50,6 +50,9 @@ pub(super) fn plan_security( &shared.enums, &shared.constants, ); + if rules.contains(&catalog::SENSITIVE_DATA) { + subject.callee_errors = callee_errors(scope, &shared.imports, context.owner, unit); + } subject.django = parsed.django; if parsed.django { django_evidence(scope, context, parsed, unit, rules, &mut subject); @@ -205,6 +208,60 @@ fn routes_to( /// Functions outside tests, in this file and in selected files that import /// it, that call `unit`, as (name, source). +/// Errors listed with one function, at most. +const CALLEE_ERRORS: usize = 8; + +/// The errors that the functions a function calls create, two calls deep, +/// with their messages: functions of its own file or of files it imports. +/// A handler that returns `str(exc)` for the `LookupError` its service +/// raises with the program's own text ("Imóvel não encontrado") sends no +/// internal detail, but without the service's raise the text read as a +/// library's: twelve such handlers of one FastAPI project were reviews. +fn callee_errors( + scope: &Scope<'_>, + imports: &BTreeMap, + owner: usize, + unit: &Unit, +) -> Vec { + let mut found = Vec::new(); + let mut visited = vec![unit.name.clone()]; + let mut callers = vec![(owner, unit)]; + for _ in 0..2 { + let mut next = Vec::new(); + for (file, caller) in callers { + let reached = scope.owners.iter().filter(|&&other| { + other == file || imports[&file].reach(&scope.inputs[other].result.path) + }); + for &other in reached { + let lines = scope.test_lines(other); + for callee in &scope.units[&other].units { + if !callee.callable() + || visited.contains(&callee.name) + || !caller.calls.contains(&callee.short_name) + || lines.iter().any(|l| callee.overlaps(l)) + { + continue; + } + visited.push(callee.name.clone()); + for error in &callee.errors { + if found.len() == CALLEE_ERRORS { + return found; + } + found.push(serde_json::json!({ + "function": callee.name, + "error": error.error, + "message": error.message, + })); + } + next.push((other, callee)); + } + } + } + callers = next; + } + found +} + fn callers_of( scope: &Scope<'_>, imports: &BTreeMap, diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index 5f17873..8f8f86f 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -858,6 +858,18 @@ const EXCEPTION_TO_CLIENT: Check = Check { no_examples: &[], }; +/// The exception check of a unit sent with the errors the functions it +/// calls create: "an exception it did not raise itself" held for a handler +/// returning the message its service raised to explain a missing record, +/// which stayed a review with the service's raise in view. +pub const EXCEPTION_TO_CLIENT_FROM_CALLEES: Check = Check { + id: "exception_to_client", + question: EXCEPTION_TO_CLIENT.question, + yes: "The text of an exception that a library, the database or the runtime raised, or a stack trace, is put into the response to a request, such as the message of every exception it catches.", + no: "Responses carry fixed messages or codes, or only messages the program writes itself, such as those of the errors in `errors_created_by_functions_it_calls` that explain invalid input or a missing record; details stay in server logs.", + no_examples: &[], +}; + const DJANGO_EXCEPTION_TO_CLIENT: Check = Check { id: "exception_to_client", question: "Does `{code}` send an exception's message, stack trace or a database error to a remote client in a response?", diff --git a/src/units/security.rs b/src/units/security.rs index fdbf3a7..fd9f021 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -53,6 +53,10 @@ pub(super) struct Subject<'a> { /// Whether it is Django code, whose questions name Django's calls and /// settings and ask its extra checks. pub django: bool, + /// Errors the functions it calls create, with their messages, shown in + /// its sensitive-data trace: whether an error's text that it sends is + /// the program's own depends on where the error was raised. + pub callee_errors: Vec, } impl Subject<'_> { @@ -90,6 +94,7 @@ pub(super) fn function_subject<'a>( enums: named_enums(&unit.sites, enums), evidence: serde_json::Map::new(), django: false, + callee_errors: Vec::new(), } } @@ -175,6 +180,7 @@ pub(super) fn setup_subject<'a>( enums: Vec::new(), evidence: serde_json::Map::new(), django: false, + callee_errors: Vec::new(), }) } @@ -536,7 +542,16 @@ fn trace( let ids: Vec = (0..messages.len()).map(|i| format!("m{i}")).collect(); ask("messages", questions::security_message_origin(&ids)); } + // With the errors its callees create in view, the exception check asks + // whose text a response carries, not who raised it. + let from_callees = + rule == SENSITIVE_DATA && !subject.django && !subject.callee_errors.is_empty(); for check in asked_checks(rule, file.language, subject.django, &subject.source) { + let check = if from_callees && check.id == "exception_to_client" { + &questions::EXCEPTION_TO_CLIENT_FROM_CALLEES + } else { + check + }; ask(check.id, check.body(&code)); } let mut state = json!({ @@ -548,6 +563,9 @@ fn trace( if rule == SENSITIVE_DATA && !messages.is_empty() { state["messages"] = json!(messages); } + if rule == SENSITIVE_DATA && !subject.callee_errors.is_empty() { + state["errors_created_by_functions_it_calls"] = json!(subject.callee_errors); + } if rule == INJECTION && !subject.enums.is_empty() { state["enums_named_in_sites"] = json!(subject.enums); } diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index 0c6eea4..c474b99 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -804,6 +804,57 @@ fn an_undecided_caller_recheck_replaces_an_undecided_traced_lean() { ); } +const SERVICE: &str = "def find_asset(asset_id):\n asset = ASSETS.get(asset_id)\n if asset is None:\n raise LookupError(\"Asset not found\")\n return asset\n"; +const HANDLER: &str = "from fastapi import HTTPException\n\nfrom app.services import find_asset\n\n\ndef read_asset(asset_id: str):\n try:\n return find_asset(asset_id)\n except LookupError as exc:\n raise HTTPException(status_code=404, detail=str(exc)) from exc\n"; + +#[test] +fn an_error_trace_shows_the_errors_the_called_functions_raise() { + let (project, options) = project_with( + &[("app/services.py", SERVICE), ("app/api.py", HANDLER)], + &[catalog::SENSITIVE_DATA], + ); + let (inputs, plan) = planned(&project, &options); + let owner = inputs + .iter() + .position(|i| i.result.path.ends_with("api.py")) + .unwrap(); + let Detail::Security { + trace: Some((trace, _)), + .. + } = &plan.files[&owner].units[0].detail + else { + panic!("a traced security unit"); + }; + assert_eq!( + trace["state"]["errors_created_by_functions_it_calls"], + json!([{"function": "find_asset", "error": "LookupError", "message": "\"Asset not found\""}]) + ); + assert_eq!( + trace["questions"]["exception_to_client"], + questions::EXCEPTION_TO_CLIENT_FROM_CALLEES.body("function.source") + ); + let alone = project_with(&[("app/api.py", HANDLER)], &[catalog::SENSITIVE_DATA]); + let (_, plan) = planned(&alone.0, &alone.1); + let Detail::Security { + trace: Some((trace, _)), + .. + } = &plan.files[&0].units[0].detail + else { + panic!("a traced security unit"); + }; + assert!( + trace["state"] + .get("errors_created_by_functions_it_calls") + .is_none() + ); + assert!( + !trace["questions"]["exception_to_client"] + .to_string() + .contains("errors_created_by_functions_it_calls"), + "without callee errors the check is asked as before" + ); +} + const ROUTE: &str = "export async function loadThing(c: Context) {\n const { data, error } = await db.from('things').select('*').eq('id', c.req.param('id'))\n if (error) throw new InternalError(`Query failed: ${error.message}`, error)\n if (!data) throw new NotFoundError('Thing not found')\n return c.json(data)\n}\n"; #[test] From a587480abe85b3c9e290b71ba2c06f2fd4fc333c Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:37:25 -0300 Subject: [PATCH 05/27] Describe deserializer checks and callee errors on the site's how-it-works page --- site/src/how-it-works.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/site/src/how-it-works.md b/site/src/how-it-works.md index 374e66e..52a71cf 100644 --- a/site/src/how-it-works.md +++ b/site/src/how-it-works.md @@ -186,7 +186,15 @@ signatures, or one candidate pair. message argument of each error the function creates and asks which one, if any, carries another error's text: the response is often written by an error handler in another file, and adding the handler to every unit also - cleared real leaks. Each registered error handler (`.onError(…)`, + cleared real leaks. The trace also lists the errors that the functions it + calls create, two calls deep in its own file or files it imports, with + their messages, and then asks the exception check about whose text a + response carries rather than who raised it: FastAPI handlers returning + `str(exc)` for the `LookupError` their service raised with the program's + own text ("Imóvel não encontrado") were twelve reviews in one project, + since the handler "did not raise it itself"; with the service's raise in + view, ten became notes or considers, and tools that return the text of + every exception they catch became reviews. Each registered error handler (`.onError(…)`, `.setErrorHandler(…)`, Express four-parameter `.use(…)` middleware, Flask and FastAPI decorators, NestJS `@Catch` filters, axum `IntoResponse` and actix-web `ResponseError` for an error type, Rocket catchers, ASP.NET Core @@ -309,7 +317,16 @@ signatures, or one candidate pair. identifiers quoted by doubling embedded quotes as handled (identifiers cannot be bound), and the URL check excludes requests a web page sends from the user's browser; on fresh repositories both had flagged such code, while - the SQL and SSRF advisory functions kept their answers. + the SQL and SSRF advisory functions kept their answers. Code whose source + names a deserializer that can build any object (Python's `pickle`, + `marshal`, `shelve`, `jsonpickle` or `yaml.load`; Ruby's `Marshal.load` + or `YAML.load`; Java's `ObjectInputStream`, `XMLDecoder`, XStream or + SnakeYAML; node-serialize) is asked about loading data with it in the + presence question, and its trace asks that language's deserialize check, + as Django views and PHP pages naming `unserialize` are: a Flask route + passing `pickle.loads(request.get_data())` was asked only about query, + command, code and markup text, and was clear. Only the requests of such + functions change. PHP units read the presence questions and checks in PHP's own terms (`src/units/questions/php.rs`), naming its functions (`echo`, `shell_exec` and backticks, `mysqli_real_escape_string`, `password_hash`, From f551756ecf2f574c9da1fc855a077f1c9e6ba2cc Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:48:02 -0300 Subject: [PATCH 06/27] Keep file-organization findings for files long enough to split Labeled by hand on 25 projects, 3 of 32 file-organization findings on files under 250 lines were right (a 138-line module, a 175-line test helper file, a 134-line types file were told to split), against 21 of 29 on longer files. Such findings are now notes. A module Choice that picks a group holding three quarters or more of the outline's members no longer names it: moving 14 of a file's 15 members, or 9 of its 11 tests, moves the file rather than splitting it. A consider left naming no group is a note, as before, and a review says to split the whole file (a 754-line routes file whose group held 26 of 29 members). On the labeled projects, file-organization reviews went from 50% to 67% precision and considers from 46% to 71%; 18 wrong findings became notes and 3 right ones did. Nothing is asked again. --- CHANGELOG.md | 1 + docs/classification-cascade.md | 9 ++++- site/src/how-it-works.md | 9 ++++- src/catalog.rs | 2 +- src/units/compose.rs | 46 +++++++++++++++++++++---- src/units/mod.rs | 2 ++ src/units/outcome/comments.rs | 7 ---- src/units/outcome/mod.rs | 8 +++++ src/units/outline.rs | 1 + src/units/tests/organization.rs | 60 +++++++++++++++++++++++++++++---- 10 files changed, 122 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dbd6da4..09a03b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. +- File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. - Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index 5c2ce68..d2bb791 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -557,7 +557,14 @@ signatures, or one candidate pair. for three or more tests only when the pairs connect them: two pairs that share no test stay two pairs (a pair of redirect tests and a pair of deny tests of `get` are not four overlapping tests). A review always carries a - finding. + finding. A file-organization finding on a file of fewer than 250 lines is + a note: of 32 such findings labeled by hand on 25 projects, 3 were right, + and splitting a 138-line module or a 175-line test helper file only + scatters it, while 21 of 29 on longer files were right. A group that + holds three quarters or more of the outline's members is not named: + moving 14 of a file's 15 members, or 9 of its 11 tests, moves the file + rather than splitting it, so a consider left naming no group is a note + and a review says to split the whole file. 6. **Gate.** `--fail-on`, `[[scope]]` levels per path and the baseline act on composed findings only. Baseline entries can carry a reason (`intended`, `later`, `wrong`) that survives rewrites; `baseline stats` counts them. diff --git a/site/src/how-it-works.md b/site/src/how-it-works.md index 52a71cf..653ad6d 100644 --- a/site/src/how-it-works.md +++ b/site/src/how-it-works.md @@ -566,7 +566,14 @@ signatures, or one candidate pair. for three or more tests only when the pairs connect them: two pairs that share no test stay two pairs (a pair of redirect tests and a pair of deny tests of `get` are not four overlapping tests). A review always carries a - finding. + finding. A file-organization finding on a file of fewer than 250 lines is + a note: of 32 such findings labeled by hand on 25 projects, 3 were right, + and splitting a 138-line module or a 175-line test helper file only + scatters it, while 21 of 29 on longer files were right. A group that + holds three quarters or more of the outline's members is not named: + moving 14 of a file's 15 members, or 9 of its 11 tests, moves the file + rather than splitting it, so a consider left naming no group is a note + and a review says to split the whole file. 6. **Gate.** `--fail-on`, `[[scope]]` levels per path and the baseline act on composed findings only. Baseline entries can carry a reason (`intended`, `later`, `wrong`) that survives rewrites; `baseline stats` counts them. diff --git a/src/catalog.rs b/src/catalog.rs index 325810d..442ee95 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -272,7 +272,7 @@ pub fn rules() -> Vec { pub fn rule_version(key: &str) -> &'static str { match key { - FILE_ORGANIZATION => "18", + FILE_ORGANIZATION => "19", FUNCTION_SIMPLIFICATION => "14", SHARED_LOGIC => "19", TEST_VALUE => "5", diff --git a/src/units/compose.rs b/src/units/compose.rs index 01a6611..9ca464e 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -3,8 +3,8 @@ use super::{ Access, Block, Detail, FilePlan, Presence, UnitPlan, outcome::{ - Answers, Outcome, benefit, checks, choice, lowered, noul, open, origin_outcome, score, - several_kind, unit_outcome, value_signals, + Answers, Outcome, at_most_note, benefit, checks, choice, lowered, noul, open, + origin_outcome, score, several_kind, unit_outcome, value_signals, }, wording::{comment_reason, comment_wording}, wording::{ @@ -492,6 +492,8 @@ impl<'p> Tally<'p> { let (outcome, answers) = resolved(unit, judgments); let outcome = if unnamed_value(unit, judgments) { lowered(lowered(outcome)) + } else if short_outline(unit) { + at_most_note(outcome) } else if unnamed_outline(unit, judgments) || few.contains(unit.id.as_str()) { lowered(outcome) } else { @@ -899,8 +901,13 @@ fn finding( .filter(|b| !most_of(&b.location, &unit.locations)); function_wording(name, strength, p, answers, block) } - Detail::Outline { tests, groups, .. } => { - let chosen = outline_groups(answers.get("module").copied(), groups); + Detail::Outline { + tests, + groups, + members, + .. + } => { + let chosen = outline_groups(answers.get("module").copied(), groups, *members); symbol = chosen.first().map(|group| group.id.clone()); if !chosen.is_empty() { locations = chosen.iter().flat_map(|g| g.locations.clone()).collect(); @@ -1077,22 +1084,49 @@ fn unnamed_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { /// Its finding is a note, since a reader cannot tell which members to move. /// A review, or a consider the kind decided, says to split the whole file. fn unnamed_outline(unit: &UnitPlan, judgments: &[Judgment]) -> bool { - let Detail::Outline { groups, .. } = &unit.detail else { + let Detail::Outline { + groups, members, .. + } = &unit.detail + else { return false; }; let (outcome, answers) = resolved(unit, judgments); let get = |q: &str| answers.get(q).copied(); matches!(outcome, Outcome::Consider(_)) && several_kind(get("split"), get("kind")).is_none() - && outline_groups(get("module"), groups).is_empty() + && outline_groups(get("module"), groups, *members).is_empty() +} + +/// Files shorter than this many lines read easily whole. +const OUTLINE_NOTE_LINES: usize = 250; + +/// A file-organization finding on a file of fewer than 250 lines is a note: +/// of 32 such findings labeled by hand on 25 projects, 3 were right, while +/// splitting a 138-line module or a 175-line test helper file would only +/// scatter it; 21 of 29 on longer files were right. +fn short_outline(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Outline { .. }) && unit.lines < OUTLINE_NOTE_LINES } /// The group a module Choice picks clearly, or else the two it leans toward /// when together they reach the location probability: flask's `cli.py` /// split 0.45 and 0.23 over two of six groups. None when it spreads wider. +/// A group holding three quarters or more of the outline's `members` is +/// left out: moving 14 of a file's 15 members, or 9 of its 11 tests, moves +/// the file rather than splitting it. fn outline_groups<'g>( module: Option<&Answer>, groups: &'g [super::GroupInfo], + members: usize, +) -> Vec<&'g super::GroupInfo> { + let mut chosen = chosen_groups(module, groups); + chosen.retain(|g| g.names.len() * 4 < members * 3); + chosen +} + +fn chosen_groups<'g>( + module: Option<&Answer>, + groups: &'g [super::GroupInfo], ) -> Vec<&'g super::GroupInfo> { let find = |id: &str| groups.iter().find(|g| g.id == id); if let Some((id, _)) = choice(module) { diff --git a/src/units/mod.rs b/src/units/mod.rs index 0b5944c..f94ae55 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -94,6 +94,8 @@ pub enum Detail { /// A test file's cases rather than application members. tests: bool, groups: Vec, + /// How many members the outline lists. + members: usize, /// What kind of file it is, asked after a recheck that stays undecided. kind: Option<(Value, Asked)>, }, diff --git a/src/units/outcome/comments.rs b/src/units/outcome/comments.rs index 47152cb..933a3f3 100644 --- a/src/units/outcome/comments.rs +++ b/src/units/outcome/comments.rs @@ -31,13 +31,6 @@ pub(in crate::units) fn comment_signals<'a>( Some(signals) } -fn at_most_note(outcome: Outcome) -> Outcome { - match outcome { - Outcome::Review(p) | Outcome::Consider(p) => Outcome::Note(p), - other => other, - } -} - /// The strongest of a comment's signals, or when they stay undecided, the /// kind of comment: the kinds a reader could do without reaching the /// threshold raise a consider (a note for documentation that repeats its diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index c233a28..5c3e7d3 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -139,6 +139,14 @@ pub(super) fn open(unit: &UnitPlan, answers: &Answers<'_>, outcome: Outcome) -> } } +/// A review or consider as a note; other outcomes as they are. +pub(super) fn at_most_note(outcome: Outcome) -> Outcome { + match outcome { + Outcome::Review(p) | Outcome::Consider(p) => Outcome::Note(p), + other => other, + } +} + /// One level lower: review becomes consider, consider becomes note. pub(super) fn lowered(outcome: Outcome) -> Outcome { match outcome { diff --git a/src/units/outline.rs b/src/units/outline.rs index 440f3ed..c030985 100644 --- a/src/units/outline.rs +++ b/src/units/outline.rs @@ -167,6 +167,7 @@ fn plan_outline( identity: identity(&names), detail: Detail::Outline { tests, + members: listed.len(), // A file too long to send whole is asked its kind from the // outline alone, so its undecided split is not left open. kind: [Some(source.clone()), None] diff --git a/src/units/tests/organization.rs b/src/units/tests/organization.rs index c73e22b..e17f427 100644 --- a/src/units/tests/organization.rs +++ b/src/units/tests/organization.rs @@ -1,13 +1,15 @@ //! File organization: outlines of application and test files. use super::*; +/// Two concerns of sixteen functions each: 258 lines, long enough that a +/// split is weighed (shorter files are notes). fn two_concerns() -> String { let mut source = String::from("struct Cache { entries: Vec }\n"); - for i in 0..7 { + for i in 0..16 { source.push_str(&function(&format!("warm{i}"))); } source.push_str("struct Page { body: String }\n"); - for i in 0..7 { + for i in 0..16 { source.push_str(&function(&format!("render{i}"))); } source @@ -138,19 +140,63 @@ fn outlines_carry_member_and_file_sizes() { let mut mock = Mock::default(); run(&project, &options, &mut mock); let state = &mock.requests[0]["state"]; - assert_eq!(state["file"]["lines"], 16 + 14 * 7); + assert_eq!(state["file"]["lines"], 2 + 32 * 8); assert_eq!(state["members"][1]["lines"], 8); } -/// Two suites of cases, each calling its own subject. +#[test] +fn a_split_of_a_short_file_is_a_note() { + let project = Project::new(); + let short: String = (0..14).map(|i| function(&format!("warm{i}"))).collect(); + project.write("lib.rs", &short); + let mut options = args(); + only(&mut options, catalog::FILE_ORGANIZATION); + let report = run(&project, &options, &mut scripted(2)); + assert_eq!( + report.files[0].findings[0].strength, + Strength::Note, + "112 lines read easily whole" + ); +} + +#[test] +fn a_group_that_holds_most_of_the_file_is_not_named() { + let project = Project::new(); + project.write("tests/app.test.ts", &two_suites()); + let mut options = args(); + only(&mut options, catalog::FILE_ORGANIZATION); + let mut eval = scripted(2); + eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; + let named = run(&project, &options, &mut eval); + assert_eq!(named.files[0].findings[0].strength, Strength::Consider); + // Ten of twelve tests in the first suite: moving it would move the file. + project.write("tests/app.test.ts", &suites(10, 2)); + options.refresh = true; + let mut eval = scripted(2); + eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; + let unnamed = run(&project, &options, &mut eval); + assert_eq!( + unnamed.files[0].findings[0].strength, + Strength::Note, + "a consider that names no group is a note" + ); +} + +/// Two suites of six cases, each calling its own subject. fn two_suites() -> String { + suites(6, 6) +} + +/// A suite of `parse` cases and one of `render` cases. +fn suites(parse: usize, render: usize) -> String { let mut source = String::from("import { parse } from './parse';\nimport { render } from './render';\n\n"); - for subject in ["parse", "render"] { + for (subject, cases) in [("parse", parse), ("render", render)] { source.push_str(&format!("describe('{subject}', () => {{\n")); - for i in 0..6 { + for i in 0..cases { source.push_str(&format!( - " it('case {i}', () => {{\n const value = {subject}({{ id: {i} }});\n expect(value.id).toBe({i});\n expect(value).toBeDefined();\n expect(value).not.toBeNull();\n expect(typeof value).toBe('object');\n expect(Object.keys(value)).toContain('id');\n expect(value).toEqual({{ id: {i} }});\n }});\n" + " it('case {i}', () => {{\n const value = {subject}({{ id: {i} }});\n expect(value.id).toBe({i});\n expect(value).toBeDefined();\n expect(value).not.toBeNull();\n expect(typeof value).toBe('object');\n expect(Object.keys(value)).toContain('id');\n expect(value).toEqual({{ id: {i} }});\n{} }});\n", + " expect(value).toBeTruthy();\n".repeat(12) )); } source.push_str("});\n"); From 8bb9ffd5a90a5979b7e0ae9bf79e037078638b07 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:55:33 -0300 Subject: [PATCH 07/27] Name rules by their ID without the group A finding reads [maintainability/file-organization], but --rule file-organization was an unknown rule: only the full ID, the key (file_organization) and groups were accepted, and the error listed only the groups. The part of the ID after the group now names the rule everywhere a rule is named (--rule, --skip-rule, --fail-on, [rules], allow comments, the MCP tool), jevgate.schema.json lists it, and an unknown name points to `jevgate rules`. --- CHANGELOG.md | 1 + jevgate.schema.json | 39 +++++++++++++++++++++++++++++++++++++++ site/src/output.md | 2 +- src/catalog.rs | 23 ++++++++++++++++++----- src/command.rs | 9 +++++---- src/config.rs | 22 ++++++++++++++++++++-- src/config_schema.rs | 12 ++++++++++-- src/mcp.rs | 2 +- src/options/commands.rs | 2 +- src/options/mod.rs | 6 +++--- src/suppress.rs | 4 ++-- 11 files changed, 101 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09a03b0..8cb287d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. +- A rule can be named by its ID without the group (`--rule file-organization`, `jevgate: allow(sensitive-data) …`, `[rules]` in `jevgate.toml`), besides its full ID and its key; `jevgate.schema.json` lists these names, and an unknown name points to `jevgate rules`. - Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. diff --git a/jevgate.schema.json b/jevgate.schema.json index d1117e2..92eb6ff 100644 --- a/jevgate.schema.json +++ b/jevgate.schema.json @@ -35,34 +35,47 @@ "items": { "enum": [ "maintainability/file-organization", + "file-organization", "file_organization", "maintainability/function-simplification", + "function-simplification", "function_simplification", "maintainability/shared-logic", + "shared-logic", "shared_logic", "maintainability/hardcoded-values", + "hardcoded-values", "hardcoded_values", "security/injection", "injection", "security/sensitive-data", + "sensitive-data", "sensitive_data", "security/unsafe-settings", + "unsafe-settings", "unsafe_settings", "security/access-control", + "access-control", "access_control", "security/workflows", "workflows", "tests/value", + "value", "test_value", "tests/redundancy", + "redundancy", "test_redundancy", "documentation/agent-context", + "agent-context", "agent_context", "documentation/large-docs", + "large-docs", "large_docs", "documentation/staleness", + "staleness", "doc_staleness", "documentation/duplication", + "duplication", "doc_duplication", "documentation/comments", "comments", @@ -84,34 +97,47 @@ "propertyNames": { "enum": [ "maintainability/file-organization", + "file-organization", "file_organization", "maintainability/function-simplification", + "function-simplification", "function_simplification", "maintainability/shared-logic", + "shared-logic", "shared_logic", "maintainability/hardcoded-values", + "hardcoded-values", "hardcoded_values", "security/injection", "injection", "security/sensitive-data", + "sensitive-data", "sensitive_data", "security/unsafe-settings", + "unsafe-settings", "unsafe_settings", "security/access-control", + "access-control", "access_control", "security/workflows", "workflows", "tests/value", + "value", "test_value", "tests/redundancy", + "redundancy", "test_redundancy", "documentation/agent-context", + "agent-context", "agent_context", "documentation/large-docs", + "large-docs", "large_docs", "documentation/staleness", + "staleness", "doc_staleness", "documentation/duplication", + "duplication", "doc_duplication", "documentation/comments", "comments", @@ -161,34 +187,47 @@ "propertyNames": { "enum": [ "maintainability/file-organization", + "file-organization", "file_organization", "maintainability/function-simplification", + "function-simplification", "function_simplification", "maintainability/shared-logic", + "shared-logic", "shared_logic", "maintainability/hardcoded-values", + "hardcoded-values", "hardcoded_values", "security/injection", "injection", "security/sensitive-data", + "sensitive-data", "sensitive_data", "security/unsafe-settings", + "unsafe-settings", "unsafe_settings", "security/access-control", + "access-control", "access_control", "security/workflows", "workflows", "tests/value", + "value", "test_value", "tests/redundancy", + "redundancy", "test_redundancy", "documentation/agent-context", + "agent-context", "agent_context", "documentation/large-docs", + "large-docs", "large_docs", "documentation/staleness", + "staleness", "doc_staleness", "documentation/duplication", + "duplication", "doc_duplication", "documentation/comments", "comments", diff --git a/site/src/output.md b/site/src/output.md index baf908a..11e2382 100644 --- a/site/src/output.md +++ b/site/src/output.md @@ -21,7 +21,7 @@ Findings are `review` (act on it), `consider` (worth a look) or `note` (optional `--fail-on review|consider|uncertain|none` sets what fails the gate; `--fail-on security=consider` sets it for one group or rule. Baselined findings, findings allowed by a comment, and notes never fail it. -A single finding can also be accepted where it is, with a comment on its line or directly above it (doc comments and attributes may sit in between). The comment names a rule ID, key or group, and needs a reason; without one it is ignored and the finding says so: +A single finding can also be accepted where it is, with a comment on its line or directly above it (doc comments and attributes may sit in between). The comment names a rule ID (`security/injection`), its name (`injection`), its key or a group, and needs a reason; without one it is ignored and the finding says so: ```python # jevgate: allow(hardcoded_values) the protocol fixes this port diff --git a/src/catalog.rs b/src/catalog.rs index 442ee95..7340763 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -302,14 +302,27 @@ pub fn groups() -> Vec<&'static str> { groups } -/// The rule keys a rule ID, key or group names; `None` when it names nothing. +/// Whether `name` is the rule's ID (`maintainability/file-organization`), +/// its name (`file-organization`, the ID after its group) or its key +/// (`file_organization`). +pub fn names(rule: &Rule, name: &str) -> bool { + name == rule.key + || name == rule.id + || rule + .id + .rsplit_once('/') + .is_some_and(|(_, short)| short == name) +} + +/// The rule keys a rule ID, name, key or group names; `None` when it names +/// nothing. pub fn select(name: &str) -> Option> { let selected: Vec<&str> = rules() .into_iter() .filter(|r| match name { ALL_GROUP => true, DEFAULT_GROUP => r.default_enabled, - _ => r.key == name || r.id == name || r.group == name, + _ => names(r, name) || r.group == name, }) .map(|r| r.key) .collect(); @@ -319,7 +332,7 @@ pub fn select(name: &str) -> Option> { /// How specifically `name` addresses `rule`: 3 for the rule itself, 2 for its /// group, 1 for `default` or `all`, 0 when it does not address it. pub fn specificity(name: &str, rule: &Rule) -> u8 { - if name == rule.key || name == rule.id { + if names(rule, name) { 3 } else if name == rule.group { 2 @@ -330,9 +343,9 @@ pub fn specificity(name: &str, rule: &Rule) -> u8 { } } -/// A rule key or its catalog ID. +/// A rule by its key, catalog ID or name. pub fn find(name: &str) -> Option { - rules().into_iter().find(|r| r.key == name || r.id == name) + rules().into_iter().find(|r| names(r, name)) } pub fn id(key: &str) -> &'static str { diff --git a/src/command.rs b/src/command.rs index 1f5bcf5..9f14700 100644 --- a/src/command.rs +++ b/src/command.rs @@ -115,10 +115,11 @@ fn baseline_action(context: &ConfigContext, action: options::BaselineAction) -> } => { let mut keys = Vec::new(); for name in &rules { - keys.extend( - catalog::select(name) - .ok_or_else(|| anyhow::anyhow!("Unknown rule or group: {name}"))?, - ); + keys.extend(catalog::select(name).ok_or_else(|| { + anyhow::anyhow!( + "Unknown rule or group: {name}; `jevgate rules` lists the rules" + ) + })?); } let marked = baseline::mark(&context.root, reason, &targets, &keys)?; say!( diff --git a/src/config.rs b/src/config.rs index 63ad54f..83875eb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -350,13 +350,14 @@ impl Levels { } } -/// Rule keys named by rule IDs, keys or groups; an unknown name is an error. +/// Rule keys named by rule IDs, names, keys or groups; an unknown name is an +/// error. fn expand(names: &[String]) -> Result> { let mut keys = Vec::new(); for name in names { let selected = catalog::select(name).ok_or_else(|| { anyhow!( - "Unknown rule or group: {name} (groups: {}, {}, {})", + "Unknown rule or group: {name}; `jevgate rules` lists the rules (groups: {}, {}, {})", catalog::groups().join(", "), catalog::DEFAULT_GROUP, catalog::ALL_GROUP @@ -418,6 +419,23 @@ mod tests { Ok(args) } + #[test] + fn a_rule_is_named_by_its_id_its_name_or_its_key() { + for rule in catalog::rules() { + let (_, short) = rule.id.rsplit_once('/').unwrap(); + for name in [rule.id, short, rule.key] { + assert_eq!(catalog::select(name).unwrap(), [rule.key], "{name}"); + } + } + let args = configured("", &["file-organization", "redundancy"], &[]).unwrap(); + assert_eq!( + args.rules, + [catalog::FILE_ORGANIZATION, catalog::TEST_REDUNDANCY] + ); + let error = configured("", &["file-organisation"], &[]).unwrap_err(); + assert!(error.to_string().contains("`jevgate rules`"), "{error}"); + } + #[test] fn default_group_runs_when_nothing_is_configured() { let args = configured("", &[], &[]).unwrap(); diff --git a/src/config_schema.rs b/src/config_schema.rs index 0fce3c9..59a504c 100644 --- a/src/config_schema.rs +++ b/src/config_schema.rs @@ -47,10 +47,18 @@ pub fn schema() -> Value { schema } -/// Every rule ID, key and group, and the `default` and `all` groups. +/// Every rule ID, name, key and group, and the `default` and `all` groups. fn rule_names() -> Value { let rules = catalog::rules(); - let mut names: Vec<&str> = rules.iter().flat_map(|r| [r.id, r.key]).collect(); + let mut names: Vec<&str> = Vec::new(); + for rule in &rules { + let short = rule.id.rsplit_once('/').map_or(rule.id, |(_, short)| short); + for name in [rule.id, short, rule.key] { + if !names.contains(&name) { + names.push(name); + } + } + } names.extend(catalog::groups()); names.extend([catalog::DEFAULT_GROUP, catalog::ALL_GROUP]); json!(names) diff --git a/src/mcp.rs b/src/mcp.rs index c58c6b7..b2b9814 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -224,7 +224,7 @@ fn tools() -> Value { "properties": { "base": {"type": "string", "description": "Review only files changed since this Git revision, such as origin/main"}, "paths": {"type": "array", "items": {"type": "string"}, "description": "Files or directories to review instead of the discovered source"}, - "rules": {"type": "array", "items": {"type": "string"}, "description": "Rule IDs, keys or groups, such as security; replaces the configured selection"}, + "rules": {"type": "array", "items": {"type": "string"}, "description": "Rule IDs, names, keys or groups, such as security or file-organization; replaces the configured selection"}, "include_tests": {"type": "boolean", "description": "Also judge tests"}, "dry_run": {"type": "boolean", "description": "List the files and planned requests without sending anything"}, "verbose": {"type": "boolean", "description": "Also show optional notes and per-file detail"}, diff --git a/src/options/commands.rs b/src/options/commands.rs index a75143c..586fd7e 100644 --- a/src/options/commands.rs +++ b/src/options/commands.rs @@ -149,7 +149,7 @@ pub enum BaselineAction { reason: Disposition, #[arg(required = true, value_name = "TARGET")] targets: Vec, - /// Only findings of this rule ID, key or group (repeatable) + /// Only findings of this rule ID, name, key or group (repeatable) #[arg(long = "rule", value_name = "RULE")] rules: Vec, }, diff --git a/src/options/mod.rs b/src/options/mod.rs index f1c68fb..e9b6b60 100644 --- a/src/options/mod.rs +++ b/src/options/mod.rs @@ -65,7 +65,7 @@ impl FailOn { } /// A `--fail-on` value: a level for every rule, or `TARGET=LEVEL` for a rule -/// ID, key or group. +/// ID, name, key or group. #[derive(Clone, Debug, PartialEq, Eq)] pub struct FailOnSpec { pub target: Option, @@ -134,7 +134,7 @@ pub struct CheckArgs { /// edit. #[arg(long, value_name = "FILE", help_heading = SCOPE)] pub config: Option, - /// Select a rule ID, key or group (repeatable) [default: the `default` group] + /// Select a rule ID, name, key or group (repeatable) [default: the `default` group] /// /// Groups: maintainability, tests, security, documentation, default (every /// rule on by default) and all. Naming any rule replaces the configured @@ -142,7 +142,7 @@ pub struct CheckArgs { /// need --include-tests. `jevgate rules` lists every rule. #[arg(long = "rule", value_name = "RULE", help_heading = RULES)] pub rules: Vec, - /// Deselect a rule ID, key or group (repeatable); applied after --rule and jevgate.toml + /// Deselect a rule ID, name, key or group (repeatable); applied after --rule and jevgate.toml #[arg(long = "skip-rule", value_name = "RULE", help_heading = RULES)] pub skip_rules: Vec, /// What fails the gate: LEVEL for every rule, or TARGET=LEVEL (repeatable) [default: review] diff --git a/src/suppress.rs b/src/suppress.rs index fc00fe2..08d4ed0 100644 --- a/src/suppress.rs +++ b/src/suppress.rs @@ -1,6 +1,6 @@ //! Inline suppressions: a comment `jevgate: allow(RULE, …) reason` on a //! finding's line, or in the comments and attributes directly above it, -//! accepts that finding as the baseline does. RULE is a rule ID, key or group, +//! accepts that finding as the baseline does. RULE is a rule ID, name, key or group, //! and the reason is required: without one the comment is ignored and the //! finding says so. use crate::{catalog, schema::Report}; @@ -64,7 +64,7 @@ fn annotation(line: &str) -> bool { .any(|start| line.starts_with(start)) } -/// Whether `name` (an ID, key or group) selects the rule with ID `rule`. +/// Whether `name` (an ID, name, key or group) selects the rule with ID `rule`. fn names(name: &str, rule: &str) -> bool { let key = catalog::find(rule).map(|r| r.key); catalog::select(name).is_some_and(|keys| key.is_some_and(|key| keys.contains(&key))) From b194f8fb663bec679cdef5d277970b462e66d9fe Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:00:51 -0300 Subject: [PATCH 08/27] Read JVM packages named example or demo as source, not examples Any directory named example or demo made a file example code, including the packages below a JVM source root: Spring Initializr names a new project's package com.example.demo, so every finding of such a project was capped (injection at a consider, weak password hashing at a note) and its hardcoded values were not judged. A seeded Spring controller in com.example.shop had SQL injection, deserialization and path traversal at consider and MD5 password hashing and a Random reset token at note; it now has all five at review. Directories below src//java, kotlin, scala or groovy are packages and no longer mark example code; directories above the source root, such as examples/, still do. Across 65 projects only gson's test-shrinker module (sample classes in com.example) changed, with two hardcoded-value considers and one shared-logic review of its per-case harness. --- CHANGELOG.md | 1 + site/src/languages.md | 2 +- src/analysis/clones.rs | 39 ++++++++++++++++++++++++++++++++++----- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cb287d..f243062 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. +- Java, Kotlin, Scala and Groovy packages named `example` or `demo`, such as Spring Initializr's default `com.example.demo`, are source, not example code: every finding of such a project was capped (injection and sensitive data at a consider, the rest at a note) and its hardcoded values were not judged. Directories above the source root, such as `examples/`, still mark example code. - A rule can be named by its ID without the group (`--rule file-organization`, `jevgate: allow(sensitive-data) …`, `[rules]` in `jevgate.toml`), besides its full ID and its key; `jevgate.schema.json` lists these names, and an unknown name points to `jevgate rules`. - Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. - `--dry-run` plans the files whose purpose the cache already answers, as a run does, so a warm cache's estimate matches the run: on a Rails project it counted 106 of 1,532 requests as new while the run sent none. diff --git a/site/src/languages.md b/site/src/languages.md index da3a116..6785fe6 100644 --- a/site/src/languages.md +++ b/site/src/languages.md @@ -38,7 +38,7 @@ | React | JSX components and hooks as functions; text shown as a JSX child is not treated as markup injection | | RSpec, Minitest | Examples with the groups they are declared in, the `before` hooks and the `let`/`subject` definitions they read, and the helpers they call from support files such as `spec/support` and `test_helper.rb` | | Sinatra and other Ruby DSLs | Methods of classes and modules (`def`, `def self.`, `class << self`, `define_method`); blocks passed at class or file level as units named by their call (`get('/invoices')`); constants as hardcoded values | -| Monorepos and examples | Copies are compared within a package and across packages linked by a local dependency, not across separate example apps, templates or variants of one example (`examples/login/raw` and `examples/login/sdk`); copies inside example code are notes | +| Monorepos and examples | Copies are compared within a package and across packages linked by a local dependency, not across separate example apps, templates or variants of one example (`examples/login/raw` and `examples/login/sdk`); copies inside example code are notes; directories below a JVM source root (`src/main/java/com/example/demo`) are packages, not examples | | Java classes | Methods and constructors belong to their class, interface, enum constant or record; `static` fields are constants; `equals` and `hashCode` overrides, constructors storing fields and setters given literals are boilerplate or data, never copies; initial capacities and a number a method returns whole are not values to name; a class of the same package counts as imported | | Spring MVC | A MockMvc or RestTemplate test request reaches the controller method whose `@GetMapping`, `@PostMapping` or `@RequestMapping` route serves it, so the test is judged with that method as its code under test | | Bundlers and compilers | Minified and compiled output (a source map reference, very long lines) is skipped as generated | diff --git a/src/analysis/clones.rs b/src/analysis/clones.rs index 7e094f9..026ee82 100644 --- a/src/analysis/clones.rs +++ b/src/analysis/clones.rs @@ -193,6 +193,9 @@ fn example_directory(part: &str) -> bool { /// Also a top-level `samples` or `sample` directory (a Java package named /// `samples` is source), a .NET project named like `MediatR.Examples.Autofac`, /// and Go's `example_*_test.go` files, which show how to call a package. +/// Directories below a JVM source root (`src/main/java`) are packages, not +/// examples: Spring Initializr names a new project's package +/// `com.example.demo`, which made every finding of such a project a note. pub(crate) fn example_code(path: &Path) -> bool { let name = path .file_name() @@ -204,14 +207,31 @@ pub(crate) fn example_code(path: &Path) -> bool { .map(|p| p.to_string_lossy().to_ascii_lowercase()) .filter(|_| path.iter().count() > 1) .unwrap_or_default(); + let directories: Vec = path + .parent() + .map(|dir| { + dir.iter() + .map(|p| p.to_string_lossy().into_owned()) + .collect() + }) + .unwrap_or_default(); + let packages = jvm_source_root(&directories).unwrap_or(directories.len()); (name.starts_with("example_") && name.ends_with("_test.go")) || matches!(top.as_str(), "samples" | "sample") - || path.parent().is_some_and(|dir| { - dir.iter().any(|part| { - let part = part.to_string_lossy(); - example_directory(&part) || part.to_ascii_lowercase().contains(".examples") - }) + || directories[..packages] + .iter() + .any(|part| example_directory(part) || part.to_ascii_lowercase().contains(".examples")) +} + +/// Where the package directories of a JVM source root begin: after +/// `src//java` (or `kotlin`, `scala`, `groovy`). +fn jvm_source_root(directories: &[String]) -> Option { + directories + .windows(3) + .position(|w| { + w[0] == "src" && matches!(w[2].as_str(), "java" | "kotlin" | "scala" | "groovy") }) + .map(|at| at + 3) } /// Whether two files are separate variants of one example, kept side by @@ -1568,6 +1588,15 @@ mod tests { assert!(!super::example_code(Path::new( "src/main/java/org/springframework/samples/petclinic/Owner.java" ))); + for path in [ + "src/main/java/com/example/demo/OrderController.java", + "service/src/test/kotlin/com/example/OrderTest.kt", + ] { + assert!(!super::example_code(Path::new(path)), "{path}"); + } + assert!(super::example_code(Path::new( + "examples/spring/src/main/java/com/example/demo/Main.java" + ))); } #[test] From 62b314e194da2f56661bf87d469dbf09d5b88532 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:08:23 -0300 Subject: [PATCH 09/27] Ask about XML parsed with external entities XXE (CWE-611) was never asked: a pygoat view turning on external general entities before parsing the request body was a note found only by its CSRF check, and a Spring controller parsing its body with a default DocumentBuilderFactory was clear. Code that parses XML with a parser able to resolve external entities now gets that clause in its presence question and an xxe check in its trace: it names lxml, SAX, pulldom, DocumentBuilderFactory, SAXParserFactory, XMLInputFactory, XmlDocument, SimpleXML, libxmljs or Nokogiri, or its file imports one and it calls a parse method. Other requests are byte-identical: across 65 projects only lobsters (5) and pygoat (1) re-asked a request, the pygoat lab became a review, and seeded Python and Java canaries now report their XXE routes. --- CHANGELOG.md | 1 + docs/classification-cascade.md | 10 ++- site/src/how-it-works.md | 10 ++- src/units/compose.rs | 1 + src/units/questions/mod.rs | 10 ++- src/units/questions/security.rs | 132 ++++++++++++++++++++++++++------ src/units/security.rs | 41 ++++++++-- src/units/tests/security.rs | 38 ++++++++- src/units/wording/security.rs | 8 +- 9 files changed, 208 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f243062..41654f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. +- Injection: XML parsed with a parser that can resolve external entities (CWE-611) is asked about where the code names such a parser (lxml, SAX, pulldom, DocumentBuilderFactory, XmlDocument, SimpleXML, libxmljs, Nokogiri) or its file imports one and it calls a parse method. pygoat's XXE lab went from a note to a review; across 65 projects only 6 requests changed. - Java, Kotlin, Scala and Groovy packages named `example` or `demo`, such as Spring Initializr's default `com.example.demo`, are source, not example code: every finding of such a project was capped (injection and sensitive data at a consider, the rest at a note) and its hardcoded values were not judged. Directories above the source root, such as `examples/`, still mark example code. - A rule can be named by its ID without the group (`--rule file-organization`, `jevgate: allow(sensitive-data) …`, `[rules]` in `jevgate.toml`), besides its full ID and its key; `jevgate.schema.json` lists these names, and an unknown name points to `jevgate rules`. - Counts in messages are singular or plural ("1 new review finding", "2 files") instead of "finding(s)", including the gate's reasons in the JSON report. diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index d2bb791..bd52045 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -316,8 +316,14 @@ signatures, or one candidate pair. presence question, and its trace asks that language's deserialize check, as Django views and PHP pages naming `unserialize` are: a Flask route passing `pickle.loads(request.get_data())` was asked only about query, - command, code and markup text, and was clear. Only the requests of such - functions change. + command, code and markup text, and was clear. Code that parses XML with a + parser able to resolve external entities (it names lxml, SAX, pulldom, + DocumentBuilderFactory, XmlDocument, SimpleXML, libxmljs or Nokogiri, or + its file imports one and it calls a parse method) is asked the same way + about XML with external entities (CWE-611): pygoat's lab calling + `make_parser()` with external entities turned on, and a Spring controller + parsing its body with a default DocumentBuilderFactory, were clear. Only + the requests of such functions change. PHP units read the presence questions and checks in PHP's own terms (`src/units/questions/php.rs`), naming its functions (`echo`, `shell_exec` and backticks, `mysqli_real_escape_string`, `password_hash`, diff --git a/site/src/how-it-works.md b/site/src/how-it-works.md index 653ad6d..db31f00 100644 --- a/site/src/how-it-works.md +++ b/site/src/how-it-works.md @@ -325,8 +325,14 @@ signatures, or one candidate pair. presence question, and its trace asks that language's deserialize check, as Django views and PHP pages naming `unserialize` are: a Flask route passing `pickle.loads(request.get_data())` was asked only about query, - command, code and markup text, and was clear. Only the requests of such - functions change. + command, code and markup text, and was clear. Code that parses XML with a + parser able to resolve external entities (it names lxml, SAX, pulldom, + DocumentBuilderFactory, XmlDocument, SimpleXML, libxmljs or Nokogiri, or + its file imports one and it calls a parse method) is asked the same way + about XML with external entities (CWE-611): pygoat's lab calling + `make_parser()` with external entities turned on, and a Spring controller + parsing its body with a default DocumentBuilderFactory, were clear. Only + the requests of such functions change. PHP units read the presence questions and checks in PHP's own terms (`src/units/questions/php.rs`), naming its functions (`echo`, `shell_exec` and backticks, `mysqli_real_escape_string`, `password_hash`, diff --git a/src/units/compose.rs b/src/units/compose.rs index 9ca464e..a88a7fa 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -610,6 +610,7 @@ fn deciding_questions(rule: &str) -> &'static [&'static str] { "type", "redirect", "deserialize", + "xxe", ], catalog::SENSITIVE_DATA => &[ "logs_secret", diff --git a/src/units/questions/mod.rs b/src/units/questions/mod.rs index 832b4f1..4760b52 100644 --- a/src/units/questions/mod.rs +++ b/src/units/questions/mod.rs @@ -170,7 +170,8 @@ mod tests { ("JavaScript", "require('node-serialize')"), ] .map(|(language, source)| deserializer_check(language, source).unwrap()), - ); + ) + .chain([&XXE]); let mut all = vec![ security_logs_secret("function.source"), security_url_parts("function.source", false), @@ -203,8 +204,9 @@ mod tests { } for django in [false, true] { all.extend([ - security_interpreted("function.source", django, None), - security_interpreted("function.source", django, Some("pickle")), + security_interpreted("function.source", django, None, false), + security_interpreted("function.source", django, Some("pickle"), false), + security_interpreted("function.source", django, Some("pickle"), true), security_resource("function.source", django), security_error_details("function.source", django), security_weakened("function.source", django), @@ -216,7 +218,7 @@ mod tests { for (id, mut body) in [ ( "interpreted", - security_interpreted("function.source", false, None), + security_interpreted("function.source", false, None, false), ), ("resource", security_resource("function.source", false)), ( diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index 8f8f86f..96792e6 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -10,35 +10,55 @@ use serde_json::{Value, json}; /// `mark_safe`, `|safe` templates and `pickle.loads` of request data were /// the injections its views held. Other code whose source names a /// deserializer that can build any object is asked about it too, naming -/// `deserializers` (see [`deserializers_named`]). -pub fn security_interpreted(code: &str, django: bool, deserializers: Option<&str>) -> Value { - if let (false, Some(names)) = (django, deserializers) { - return noul( - format!( - "Does `{code}` place a variable into the text of a database query, shell command, code to evaluate, or HTML markup, or load it with a deserializer that can build any object?" - ), - &format!( - "A variable is joined, formatted or interpolated into the text of a query, command, code or markup that is then run or rendered, or loaded with a deserializer that can build any object or run code, such as {names}." - ), - "Variables are passed only as bound parameters, separate arguments, or through a template or component that escapes them; the text is built only from fixed values; data is parsed only as JSON or another data-only format; or the function builds no such text.", - ); - } +/// `deserializers` (see [`deserializers_named`]), and code that names an XML +/// parser able to resolve external entities (`xml`, see +/// [`xml_parser_named`]) about parsing with it. +pub fn security_interpreted( + code: &str, + django: bool, + deserializers: Option<&str>, + xml: bool, +) -> Value { + let mut question = format!( + "Does `{code}` place a variable into the text of a database query, shell command, code to evaluate, or HTML markup" + ); + let mut yes = String::from( + "A variable is joined, formatted or interpolated into the text of a query, command, code or markup that is then run or rendered", + ); + let mut no = String::from( + "Variables are passed only as bound parameters, separate arguments, or through a template or component that escapes them; the text is built only from fixed values;", + ); if django { - return noul( + question.push_str(", or load it with a deserializer that can build any object"); + yes.push_str(", marked as safe markup or passed to a template that writes it unescaped, or loaded with pickle or a similar deserializer"); + no.push_str(" data is parsed only as JSON or another data-only format;"); + } else if let Some(names) = deserializers { + question.push_str(", or load it with a deserializer that can build any object"); + yes.push_str(&format!( + ", or loaded with a deserializer that can build any object or run code, such as {names}" + )); + no.push_str(" data is parsed only as JSON or another data-only format;"); + } + if xml { + // Both clauses would make the question too long to read as one. + question = if django || deserializers.is_some() { format!( - "Does `{code}` place a variable into the text of a database query, shell command, code to evaluate, or HTML markup, or load it with a deserializer that can build any object?" - ), - "A variable is joined, formatted or interpolated into the text of a query, command, code or markup that is then run or rendered, marked as safe markup or passed to a template that writes it unescaped, or loaded with pickle or a similar deserializer.", - "Variables are passed only as bound parameters, separate arguments, or through a template or component that escapes them; the text is built only from fixed values; data is parsed only as JSON or another data-only format; or the function builds no such text.", + "Does `{code}` pass a variable into query, command, code or markup text, a deserializer that can build any object, or an XML parser that resolves external entities" + ) + } else { + question + ", or parse it as XML with a parser that resolves external entities" + }; + yes.push_str( + ", or parsed as XML with external entities or document type definitions enabled", + ); + no.push_str( + " XML is parsed with document type definitions and external entities turned off;", ); } - noul( - format!( - "Does `{code}` place a variable into the text of a database query, shell command, code to evaluate, or HTML markup?" - ), - "A variable is joined, formatted or interpolated into the text of a query, command, code or markup that is then run or rendered.", - "Variables are passed only as bound parameters, separate arguments, or through a template or component that escapes them; the text is built only from fixed values; or the function builds no such text.", - ) + question.push('?'); + yes.push('.'); + no.push_str(" or the function builds no such text."); + noul(question, &yes, &no) } /// In Django code, redirects count too: a view that redirects to a URL from @@ -729,6 +749,68 @@ pub fn deserializer_check(language: &str, source: &str) -> Option<&'static Check deserializer_entry(language, source).map(|(.., check)| check) } +/// XML parsers that can resolve external entities or load document type +/// definitions, as source names them in any case: lxml, Python's pulldom +/// and SAX, Java's DocumentBuilderFactory, SAXParserFactory, +/// XMLInputFactory, TransformerFactory, dom4j and JDOM, .NET's XmlDocument +/// and XmlTextReader, PHP's SimpleXML and DOMDocument, libxmljs and +/// Nokogiri. A pygoat lab parsing a request with lxml and a Spring +/// controller parsing its body with a default DocumentBuilderFactory were +/// never asked about entities. +const XML_PARSERS: [&str; 18] = [ + "lxml", + "resolve_entities", + "pulldom", + "xml.sax", + "documentbuilderfactory", + "saxparserfactory", + "xmlinputfactory", + "transformerfactory", + "saxreader", + "saxbuilder", + "xmldocument", + "xmltextreader", + "dtdprocessing", + "simplexml_load", + "domdocument", + "libxml_noent", + "libxmljs", + "nokogiri", +]; + +/// Whether `source` names an XML parser that can resolve external entities. +fn xml_parser_named(source: &str) -> bool { + let source = source.to_ascii_lowercase(); + XML_PARSERS.iter().any(|name| source.contains(name)) +} + +/// Calls that parse XML with a parser created or imported elsewhere in the +/// file: `make_parser()`, `parseString(…)`, `etree.fromstring(…)`. +const XML_CALLS: [&str; 5] = ["parse", "fromstring", "iterparse", "expandnode", "xml("]; + +/// Whether `code`, in a file whose whole source is `file`, parses XML with a +/// parser that can resolve external entities: it names one itself, or its +/// file imports one and it calls a parse method. Python modules import +/// lxml or `xml.sax` at the top, so pygoat's lab calling `make_parser()` and +/// `parseString(…)` named no parser in its own source. +pub fn parses_xml(file: &str, code: &str) -> bool { + xml_parser_named(code) + || (xml_parser_named(file) && { + let code = code.to_ascii_lowercase(); + XML_CALLS.iter().any(|call| code.contains(call)) + }) +} + +/// Whether XML from another party is parsed with external entities enabled, +/// asked only of source that names such a parser. +pub const XXE: Check = Check { + id: "xxe", + question: "Does `{code}` parse XML that another party can send with a parser that resolves external entities or loads document type definitions?", + yes: "Request data, an upload or a message is parsed as XML with external entities, DTD loading or entity substitution enabled, or with a parser whose defaults allow them, such as Java's DocumentBuilderFactory, SAXParserFactory or XMLInputFactory without disallowing DOCTYPE declarations, lxml with resolve_entities or load_dtd, .NET's XmlDocument with an XmlResolver or DtdProcessing.Parse, PHP's LIBXML_NOENT, or libxmljs with noent.", + no: "It turns DOCTYPE declarations and external entities off (disallow-doctype-decl, resolve_entities=False, DtdProcessing.Prohibit, a null XmlResolver), uses a parser that never resolves them such as Python's xml.etree or defusedxml, or parses only XML the program wrote itself.", + no_examples: &[], +}; + /// Specific weak settings, asked when the broad presence question is not clear. pub const WEAK_SETTINGS: [Check; 6] = [ Check { diff --git a/src/units/security.rs b/src/units/security.rs index fd9f021..868db71 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -362,11 +362,12 @@ fn presence_request( }; let source = items[index].1["source"].as_str().unwrap_or_default(); let deserializers = questions::deserializers_named(file.language, source); + let xml = questions::parses_xml(file.source, source); for (rule, _, id) in units { for question in presence_questions(rule) { questions.ask( format!("{}{index}_{question}", &key[..1]), - presence_body(question, &code, django, deserializers), + presence_body(question, &code, django, (deserializers, xml)), id, rule, question, @@ -393,9 +394,16 @@ pub(super) fn presence_questions(rule: &str) -> &'static [&'static str] { .map_or(&[], |(_, questions)| questions) } -fn presence_body(question: &str, code: &str, django: bool, deserializers: Option<&str>) -> Value { +/// `named` holds the deserializers and whether an XML parser that can +/// resolve entities appear in the source. +fn presence_body( + question: &str, + code: &str, + django: bool, + (deserializers, xml): (Option<&str>, bool), +) -> Value { match question { - "interpreted" => questions::security_interpreted(code, django, deserializers), + "interpreted" => questions::security_interpreted(code, django, deserializers, xml), "resource" => questions::security_resource(code, django), "logs_secret" => questions::security_logs_secret(code), "error_details" => questions::security_error_details(code, django), @@ -408,9 +416,15 @@ fn presence_body(question: &str, code: &str, django: bool, deserializers: Option /// answered only in its files. pub(super) fn checks(rule: &str) -> Vec<&'static questions::Check> { let (.., php) = rule_checks(rule); - let mut all = asked_checks(rule, questions::CSHARP, false, ""); + let mut all = asked_checks(rule, questions::CSHARP, false, "", false); // Django and PHP each ask a `deserialize` check of their own: one kind. - for check in asked_checks(rule, "", true, "").into_iter().chain(php) { + // The XML check is asked only of source that names an XML parser. + let xml = (rule == INJECTION).then_some(&questions::XXE); + for check in asked_checks(rule, "", true, "", false) + .into_iter() + .chain(php) + .chain(xml) + { if !all.iter().any(|c| c.id == check.id) { all.push(check); } @@ -454,12 +468,14 @@ fn rule_checks( /// exists, and the Django checks besides; PHP files are asked PHP's own /// checks only of source that names what they ask about, and other code /// the deserialize check of its language when its source names one of the -/// language's deserializers. +/// language's deserializers. Code that parses XML with a parser able to +/// resolve external entities (`xml`) is asked the XML check. fn asked_checks( rule: &str, language: &str, django: bool, source: &str, + xml: bool, ) -> Vec<&'static questions::Check> { let (general, csharp, framework, php) = rule_checks(rule); let csharp = if language == questions::CSHARP { @@ -487,6 +503,7 @@ fn asked_checks( .then(|| questions::deserializer_check(language, source)) .flatten(), ) + .chain((rule == INJECTION && xml).then_some(&questions::XXE)) .collect() } @@ -546,7 +563,8 @@ fn trace( // whose text a response carries, not who raised it. let from_callees = rule == SENSITIVE_DATA && !subject.django && !subject.callee_errors.is_empty(); - for check in asked_checks(rule, file.language, subject.django, &subject.source) { + let xml = questions::parses_xml(file.source, &subject.source); + for check in asked_checks(rule, file.language, subject.django, &subject.source, xml) { let check = if from_callees && check.id == "exception_to_client" { &questions::EXCEPTION_TO_CLIENT_FROM_CALLEES } else { @@ -593,7 +611,14 @@ fn recheck(file: &FileContext<'_>, subject: &Subject<'_>, id: &str) -> Option<(V "origin", Pass::Recheck, ); - for check in asked_checks(INJECTION, file.language, subject.django, &subject.source) { + let xml = questions::parses_xml(file.source, &subject.source); + for check in asked_checks( + INJECTION, + file.language, + subject.django, + &subject.source, + xml, + ) { questions.ask( check.id.into(), check.with_callers(&code), diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index c474b99..929bafa 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -444,7 +444,7 @@ fn a_deserializer_is_asked_about_only_where_the_source_names_one() { .replace("pickle.loads", "json.loads"); assert_eq!( first("shop/cart.py", &parsed), - questions::security_interpreted("functions[0].source", false, None), + questions::security_interpreted("functions[0].source", false, None, false), "code that names no deserializer keeps its question and cached answer" ); assert!(traced_checks("shop/cart.py", PICKLED).contains_key("deserialize")); @@ -486,6 +486,42 @@ fn request_data_given_to_pickle_is_a_deserialization_review() { assert_eq!(finding.strength, Strength::Review); } +const XML_IMPORT: &str = "import java.io.InputStream;\nimport javax.xml.parsers.DocumentBuilderFactory;\n\nclass Catalog {\n int count(InputStream body) throws Exception {\n var document = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(body);\n return document.getElementsByTagName(\"item\").getLength();\n }\n}\n"; + +#[test] +fn xml_parsed_with_entities_is_asked_about_only_where_a_parser_is_named() { + let path = "src/main/java/shop/Catalog.java"; + let named = traced_checks(path, XML_IMPORT); + assert!(named["xxe"].to_string().contains("DocumentBuilderFactory")); + let plain = XML_IMPORT + .replace("import javax.xml.parsers.DocumentBuilderFactory;\n", "") + .replace( + "DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(body)", + "Json.parse(body)", + ); + assert!(!traced_checks(path, &plain).contains_key("xxe")); + // A module that imports the parser at its top, as Python code does. + let module = "from lxml import etree\n\n\ndef count(body):\n root = etree.fromstring(body)\n return len(root.findall('item'))\n"; + assert!(traced_checks("shop/catalog.py", module).contains_key("xxe")); + let project = Project::new(); + project.write(path, XML_IMPORT); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into()]; + let mut eval = scripted(0); + eval.overrides = vec![ + ("interpreted", noul_at(0.95)), + ("xxe", noul_at(0.95)), + ("origin", spread(0.0, 0.05, 0.95)), + ]; + let report = run(&project, &options, &mut eval); + let finding = &report.files[0].findings[0]; + assert_eq!( + finding.category.as_deref(), + Some("CWE-611 XML external entity reference") + ); + assert_eq!(finding.strength, Strength::Review); +} + const MARKUP_PARTS: [&str; 8] = [ "request", "stored", diff --git a/src/units/wording/security.rs b/src/units/wording/security.rs index 851ad46..d3ba293 100644 --- a/src/units/wording/security.rs +++ b/src/units/wording/security.rs @@ -70,7 +70,7 @@ pub(in crate::units) fn privilege_wording( } /// Injection kinds: the text a variable is placed into, its weakness and remedy. -const INJECTIONS: [(&str, &str, &str, &str); 11] = [ +const INJECTIONS: [(&str, &str, &str, &str); 12] = [ ( "sql", "a database query", @@ -125,6 +125,12 @@ const INJECTIONS: [(&str, &str, &str, &str); 11] = [ "CWE-502 deserialization of untrusted data", "Parse the data as JSON or with a safe loader such as `yaml.safe_load`, or restrict the classes it may create", ), + ( + "xxe", + "an XML parser that resolves external entities", + "CWE-611 XML external entity reference", + "Turn off document type definitions and external entities in the parser, or parse with one that never resolves them", + ), ( "upload", "the name of a file it saves", From 2cf43a41f35c70a62f36708e5284494e570c5271 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:11:02 -0300 Subject: [PATCH 10/27] Share test setup and name the steps JevGate's review flagged JevGate reviewing this branch found a test repeating the steps of traced_checks, and considered preview's branching, callee_errors mixing two jobs, three-quarter checks written as arithmetic twice and three tests building the same scripted answers. The injection plan of a one-file project, a cached file purpose, a function's callees, the three-quarter comparison and the answers that move G1 are now named once each. Behavior is unchanged. --- src/evaluate.rs | 31 ++++++++++----- src/units/compose.rs | 9 ++++- src/units/plan/security_units.rs | 68 ++++++++++++++++++++------------ src/units/tests/organization.rs | 19 ++++----- src/units/tests/security.rs | 18 ++++----- 5 files changed, 90 insertions(+), 55 deletions(-) diff --git a/src/evaluate.rs b/src/evaluate.rs index c9d7c6e..d90e1d4 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -138,17 +138,12 @@ fn preview(inputs: &[Input], args: &CheckArgs, root: &std::path::Path, report: & match schedule(input, args, budget, &mut report.files[owner]) { Ok(Scheduled::None) => {} Ok(Scheduled::Purpose(request)) => { - if let Some(body) = crate::requests::answered(root, args, &request) { - let file = &mut report.files[owner]; - let view = crate::file_kind::record_purpose(file, &request, &body) - .and_then(|()| crate::file_kind::decide_after_purpose(input, args, file)); - match view { - Ok(Some(view)) => { - views.insert(owner, view); - } - Ok(None) => {} - Err(error) => report.errors.push(error.to_string()), + match cached_purpose(input, args, root, &request, &mut report.files[owner]) { + Ok(Some(view)) => { + views.insert(owner, view); } + Ok(None) => {} + Err(error) => report.errors.push(error.to_string()), } planned.push(request); } @@ -183,6 +178,22 @@ fn preview(inputs: &[Input], args: &CheckArgs, root: &std::path::Path, report: & } } +/// A file's view as a run decides it after its purpose request, when the +/// cache answers that request; none when it does not. +fn cached_purpose( + input: &Input, + args: &CheckArgs, + root: &std::path::Path, + request: &serde_json::Value, + file: &mut FileResult, +) -> Result> { + let Some(body) = crate::requests::answered(root, args, request) else { + return Ok(None); + }; + crate::file_kind::record_purpose(file, request, &body)?; + crate::file_kind::decide_after_purpose(input, args, file) +} + impl Session<'_> { pub fn evaluate(&mut self, inputs: &[Input], report: &mut Report) -> Result<()> { self.evaluator.begin_review(); diff --git a/src/units/compose.rs b/src/units/compose.rs index a88a7fa..cf899be 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -1121,7 +1121,7 @@ fn outline_groups<'g>( members: usize, ) -> Vec<&'g super::GroupInfo> { let mut chosen = chosen_groups(module, groups); - chosen.retain(|g| g.names.len() * 4 < members * 3); + chosen.retain(|g| !three_quarters(g.names.len(), members)); chosen } @@ -1174,7 +1174,12 @@ fn most_of(block: &crate::schema::Location, function: &[crate::schema::Location] let lines = |l: &crate::schema::Location| l.end_line + 1 - l.start_line; function .first() - .is_some_and(|f| lines(block) * 4 >= lines(f) * 3) + .is_some_and(|f| three_quarters(lines(block), lines(f))) +} + +/// Whether `part` is three quarters or more of `whole`. +fn three_quarters(part: usize, whole: usize) -> bool { + part * 4 >= whole * 3 } /// The block chosen by the locate follow-up `question`, when its choice is clear. diff --git a/src/units/plan/security_units.rs b/src/units/plan/security_units.rs index e92bf85..5bd2737 100644 --- a/src/units/plan/security_units.rs +++ b/src/units/plan/security_units.rs @@ -229,36 +229,54 @@ fn callee_errors( for _ in 0..2 { let mut next = Vec::new(); for (file, caller) in callers { - let reached = scope.owners.iter().filter(|&&other| { - other == file || imports[&file].reach(&scope.inputs[other].result.path) - }); - for &other in reached { - let lines = scope.test_lines(other); - for callee in &scope.units[&other].units { - if !callee.callable() - || visited.contains(&callee.name) - || !caller.calls.contains(&callee.short_name) - || lines.iter().any(|l| callee.overlaps(l)) - { - continue; - } - visited.push(callee.name.clone()); - for error in &callee.errors { - if found.len() == CALLEE_ERRORS { - return found; - } - found.push(serde_json::json!({ - "function": callee.name, - "error": error.error, - "message": error.message, - })); - } - next.push((other, callee)); + for (other, callee) in callees(scope, imports, file, caller) { + if visited.contains(&callee.name) { + continue; } + visited.push(callee.name.clone()); + found.extend(callee.errors.iter().map(|error| { + serde_json::json!({ + "function": callee.name, + "error": error.error, + "message": error.message, + }) + })); + next.push((other, callee)); } } callers = next; } + found.truncate(CALLEE_ERRORS); + found +} + +/// The application functions `caller` calls, in its own file or files it +/// imports, with the file each is in. +fn callees<'s>( + scope: &'s Scope<'_>, + imports: &BTreeMap, + file: usize, + caller: &Unit, +) -> Vec<(usize, &'s Unit)> { + let reached = + scope.owners.iter().copied().filter(|&other| { + other == file || imports[&file].reach(&scope.inputs[other].result.path) + }); + let mut found = Vec::new(); + for other in reached { + let lines = scope.test_lines(other); + found.extend( + scope.units[&other] + .units + .iter() + .filter(|callee| { + callee.callable() + && caller.calls.contains(&callee.short_name) + && !lines.iter().any(|l| callee.overlaps(l)) + }) + .map(|callee| (other, callee)), + ); + } found } diff --git a/src/units/tests/organization.rs b/src/units/tests/organization.rs index e17f427..7bbb7cf 100644 --- a/src/units/tests/organization.rs +++ b/src/units/tests/organization.rs @@ -165,16 +165,12 @@ fn a_group_that_holds_most_of_the_file_is_not_named() { project.write("tests/app.test.ts", &two_suites()); let mut options = args(); only(&mut options, catalog::FILE_ORGANIZATION); - let mut eval = scripted(2); - eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; - let named = run(&project, &options, &mut eval); + let named = run(&project, &options, &mut moving_g1()); assert_eq!(named.files[0].findings[0].strength, Strength::Consider); // Ten of twelve tests in the first suite: moving it would move the file. project.write("tests/app.test.ts", &suites(10, 2)); options.refresh = true; - let mut eval = scripted(2); - eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; - let unnamed = run(&project, &options, &mut eval); + let unnamed = run(&project, &options, &mut moving_g1()); assert_eq!( unnamed.files[0].findings[0].strength, Strength::Note, @@ -182,6 +178,13 @@ fn a_group_that_holds_most_of_the_file_is_not_named() { ); } +/// Answers at the top level that pick G1 as the group to move. +fn moving_g1() -> Scripted { + let mut eval = scripted(2); + eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; + eval +} + /// Two suites of six cases, each calling its own subject. fn two_suites() -> String { suites(6, 6) @@ -210,9 +213,7 @@ fn test_files_are_outlined_by_suite_without_include_tests() { project.write("tests/app.test.ts", &two_suites()); let mut options = args(); only(&mut options, catalog::FILE_ORGANIZATION); - let mut eval = scripted(2); - eval.overrides = vec![("module", choice_of("G1", &["G1", "G2", "none"]))]; - let report = run(&project, &options, &mut eval); + let report = run(&project, &options, &mut moving_g1()); let file = &report.files[0]; assert_eq!(file.classification.as_ref().unwrap().kind, "tests"); assert_eq!( diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index 929bafa..fc5fe53 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -378,13 +378,18 @@ fn redirects_deserializers_and_uploads_are_checked_kinds_with_their_weakness() { } } -/// The questions of the trace planned for the first unit of `path`. -fn traced_checks(path: &str, source: &str) -> serde_json::Map { +/// The injection plan of a project holding only `source` at `path`. +fn injection_plan(path: &str, source: &str) -> Plan { let project = Project::new(); project.write(path, source); let mut options = args(); options.rules = vec![catalog::INJECTION.into()]; - let (_, plan) = planned(&project, &options); + planned(&project, &options).1 +} + +/// The questions of the trace planned for the first unit of `path`. +fn traced_checks(path: &str, source: &str) -> serde_json::Map { + let plan = injection_plan(path, source); let trace = &plan.files[&0].units[0]; let Detail::Security { trace: Some((request, _)), @@ -430,12 +435,7 @@ const PICKLED: &str = "import pickle\n\nfrom flask import jsonify, request\n\n\n #[test] fn a_deserializer_is_asked_about_only_where_the_source_names_one() { let first = |path: &str, source: &str| { - let project = Project::new(); - project.write(path, source); - let mut options = args(); - options.rules = vec![catalog::INJECTION.into()]; - let (_, plan) = planned(&project, &options); - plan.requests[0].request["questions"]["f0_interpreted"].clone() + injection_plan(path, source).requests[0].request["questions"]["f0_interpreted"].clone() }; let named = first("shop/cart.py", PICKLED); assert!(named.to_string().contains("pickle, marshal"), "{named}"); From 83607d79f124b8c1cf3a190fa5993624f3934bfa Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:16:06 -0300 Subject: [PATCH 11/27] Let a Go file reach the files of its package Imports::reach looked for a target's file stem in import lines, which Go never writes: a Go import names a package directory, and files of one package use each other without importing. The import block's paths were not even read, since only lines starting with "import " were kept. So callers, callees and the files that use a Go file were never found: an injection's recheck with its callers was never asked, and govwa's SQL injection through a query helper that its handlers call with a cookie and a query parameter stayed a consider. A Go file now reaches every file of its directory and of the packages its import paths name. Across 65 projects only the six Go projects re-asked requests ($0.01): govwa's injection became a review, callers cleared one injection note in wtf and one error-detail note in wild-workouts, and nothing new was raised. --- CHANGELOG.md | 1 + site/src/languages.md | 1 + src/analysis/imports.rs | 54 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41654f2..44eaed4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. +- Go: a file reaches the other files of its package (its directory) and the packages its `import ( … )` block names, as Java files reach their package. Callers, callees and the files that use a file were missed in Go: an injection's recheck with its callers was never asked, so govwa's SQL injection through a query helper stayed a consider; it is now a review, and callers cleared two notes elsewhere. Only Go projects' requests change. - Injection: XML parsed with a parser that can resolve external entities (CWE-611) is asked about where the code names such a parser (lxml, SAX, pulldom, DocumentBuilderFactory, XmlDocument, SimpleXML, libxmljs, Nokogiri) or its file imports one and it calls a parse method. pygoat's XXE lab went from a note to a review; across 65 projects only 6 requests changed. - Java, Kotlin, Scala and Groovy packages named `example` or `demo`, such as Spring Initializr's default `com.example.demo`, are source, not example code: every finding of such a project was capped (injection and sensitive data at a consider, the rest at a note) and its hardcoded values were not judged. Directories above the source root, such as `examples/`, still mark example code. - A rule can be named by its ID without the group (`--rule file-organization`, `jevgate: allow(sensitive-data) …`, `[rules]` in `jevgate.toml`), besides its full ID and its key; `jevgate.schema.json` lists these names, and an unknown name points to `jevgate rules`. diff --git a/site/src/languages.md b/site/src/languages.md index 6785fe6..1ff4d89 100644 --- a/site/src/languages.md +++ b/site/src/languages.md @@ -39,6 +39,7 @@ | RSpec, Minitest | Examples with the groups they are declared in, the `before` hooks and the `let`/`subject` definitions they read, and the helpers they call from support files such as `spec/support` and `test_helper.rb` | | Sinatra and other Ruby DSLs | Methods of classes and modules (`def`, `def self.`, `class << self`, `define_method`); blocks passed at class or file level as units named by their call (`get('/invoices')`); constants as hardcoded values | | Monorepos and examples | Copies are compared within a package and across packages linked by a local dependency, not across separate example apps, templates or variants of one example (`examples/login/raw` and `examples/login/sdk`); copies inside example code are notes; directories below a JVM source root (`src/main/java/com/example/demo`) are packages, not examples | +| Go packages | A file's package is its directory: the files of its package and the packages its imports name are its callers and callees, so an injection is judged with the handlers that call its query helper | | Java classes | Methods and constructors belong to their class, interface, enum constant or record; `static` fields are constants; `equals` and `hashCode` overrides, constructors storing fields and setters given literals are boilerplate or data, never copies; initial capacities and a number a method returns whole are not values to name; a class of the same package counts as imported | | Spring MVC | A MockMvc or RestTemplate test request reaches the controller method whose `@GetMapping`, `@PostMapping` or `@RequestMapping` route serves it, so the test is judged with that method as its code under test | | Bundlers and compilers | Minified and compiled output (a source map reference, very long lines) is skipped as generated | diff --git a/src/analysis/imports.rs b/src/analysis/imports.rs index c0ae5f9..30a5b60 100644 --- a/src/analysis/imports.rs +++ b/src/analysis/imports.rs @@ -3,18 +3,26 @@ //! target's module. Matching calls by bare name alone linked unrelated files, //! such as a Python `update` to a JavaScript `decipher.update`. Java code //! uses the classes of its own package without importing them, so a Java file -//! also reaches a file of its directory whose class it names. +//! also reaches a file of its directory whose class it names. A Go package is +//! a directory: a Go file reaches every file of its own directory and of the +//! directories its import paths name. use std::{ collections::BTreeSet, path::{Path, PathBuf}, }; -/// Lines that import, load or declare another module. +/// Lines that import, load or declare another module, and for Go the import +/// paths of an `import ( … )` block. fn import_lines(source: &str, family: &str) -> Vec { + let mut block = false; source .lines() .map(str::trim) .filter(|line| { + if family == "go" && (block || line.starts_with("import (")) { + block = *line != ")"; + return true; + } [ "import ", "from ", "use ", "pub use ", "mod ", "pub mod ", "export ", ] @@ -50,6 +58,8 @@ pub struct Imports { /// For Java: the file's directory and the capitalized names its code /// mentions, the classes of its package it can use without an import. package: Option<(PathBuf, BTreeSet)>, + /// For Go: the file's directory, its package. + directory: Option, } impl Imports { @@ -60,12 +70,15 @@ impl Imports { family, lines: csharp_lines(source), package: None, + directory: None, }; } Self { family, lines: import_lines(source, family), package: (family == "java").then(|| java_package(path, source)), + directory: (family == "go") + .then(|| path.parent().unwrap_or(Path::new("")).to_path_buf()), } } @@ -77,6 +90,11 @@ impl Imports { if self.family.is_empty() || self.family != family(target) { return false; } + if let Some(directory) = &self.directory { + let package = target.parent().unwrap_or(Path::new("")); + return package == directory + || self.lines.iter().any(|line| imports_package(line, package)); + } let mut name = module_name(target); if self.family == "csharp" { // `Index.cshtml.cs` holds the page model of `Index.cshtml`. @@ -144,6 +162,23 @@ fn module_name(path: &Path) -> String { } } +/// A Go import line whose quoted path ends with the directory `package` +/// (`"example.com/shop/internal/orders"` for `internal/orders`). +fn imports_package(line: &str, package: &Path) -> bool { + let package: Vec = package + .iter() + .map(|part| part.to_string_lossy().into_owned()) + .collect(); + if package.is_empty() { + return false; + } + let Some(path) = line.split('"').nth(1) else { + return false; + }; + let segments: Vec<&str> = path.split('/').collect(); + segments.len() >= package.len() && segments[segments.len() - package.len()..] == package[..] +} + /// `name` appears as a whole path segment, as in `./name'`, `crate::name::x`, /// `from .name import` or `import name`. fn names_segment(line: &str, name: &str) -> bool { @@ -161,7 +196,7 @@ mod tests { #[test] fn callers_need_an_import_of_the_module_in_the_same_language() { - let cases: [(&str, &str, &[&str], &[&str]); 6] = [ + let cases: [(&str, &str, &[&str], &[&str]); 7] = [ ( "app/routes.php", " Date: Sat, 26 Sep 2026 01:23:25 -0300 Subject: [PATCH 12/27] Judge files with a few small syntax errors apart from them Any syntax error skipped a whole file, but tree-sitter's grammars miss some valid code: tree-sitter-typescript reads a call signature that starts with on the line after another as its continuation, which left four of zustand's store files unjudged, and tree-sitter-go flags a const group closed on a raw string's line, which hid govwa's MD5 password hashing in user/user.go. A file whose errors are at most three regions and an eighth of its source is now parsed; the definitions holding an error, and anything inside an error node, are left out. Generator templates (under a templates directory, or holding ERB tags or dotnet-new //#if conditions) keep the strict rule, since their placeholders are not the language's syntax: devise's and CleanArchitecture's templates stay skipped. Whether errors are tolerable depends on the path, so it is checked on cached trees too. Across 65 projects only zustand and govwa re-asked requests ($0.02): govwa's login now has its MD5 password hashing as a review, and zustand's tests resolve createStore as their code under test. --- CHANGELOG.md | 1 + docs/classification-cascade.md | 8 ++++ site/src/how-it-works.md | 8 ++++ src/analysis/units/mod.rs | 8 +++- src/analysis/units/tests/mod.rs | 28 ++++++++++++ src/syntax.rs | 79 ++++++++++++++++++++++++++++----- 6 files changed, 120 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44eaed4..a9f95e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver - Injection: code that names a deserializer that can build any object is asked whether it loads data another party sends, in Python, Ruby, Java, JavaScript and TypeScript: Python's `pickle`, `marshal`, `shelve`, `jsonpickle` and `yaml.load`, Ruby's `Marshal.load` and `YAML.load`, Java's `ObjectInputStream`, `XMLDecoder`, XStream and SnakeYAML, and node-serialize. Only Django views and PHP `unserialize` were asked before, so a Flask route passing `pickle.loads(request.get_data())` was clear; it is now a review (CWE-502). Other requests are unchanged, so cached answers stay valid. - File organization: a file of fewer than 250 lines gets at most a note, and a group holding three quarters or more of a file's members is no longer named as the part to move (a consider left naming no group is a note; a review says to split the whole file). Checked by hand on 25 projects, 3 of 32 findings on shorter files were right against 21 of 29 on longer ones; review precision went from 50% to 67% and consider precision from 46% to 71%. No request changes, so cached answers stay valid. - Sensitive data: an error-detail trace lists the errors the functions it calls create, with their messages, and asks whose text a response carries instead of who raised it. A handler that returns the message its own service raised to explain a missing record (`except LookupError as exc: raise HTTPException(404, detail=str(exc))`) is no longer a review, while one that returns the text of every exception it catches still is. On 65 projects, 10 of one FastAPI project's 12 wrong reviews became notes or considers; only error-detail traces are asked again. +- A file whose parse holds a few small syntax errors (at most three regions, an eighth of the source) is judged apart from the definitions that hold them, instead of being skipped: tree-sitter's grammars miss some valid code, which left four of zustand's store files and govwa's `user/user.go` unjudged (its MD5 password hashing is now a review). Generator templates (under `templates/`, or with ERB tags or `//#if` conditions) are still skipped. - Go: a file reaches the other files of its package (its directory) and the packages its `import ( … )` block names, as Java files reach their package. Callers, callees and the files that use a file were missed in Go: an injection's recheck with its callers was never asked, so govwa's SQL injection through a query helper stayed a consider; it is now a review, and callers cleared two notes elsewhere. Only Go projects' requests change. - Injection: XML parsed with a parser that can resolve external entities (CWE-611) is asked about where the code names such a parser (lxml, SAX, pulldom, DocumentBuilderFactory, XmlDocument, SimpleXML, libxmljs, Nokogiri) or its file imports one and it calls a parse method. pygoat's XXE lab went from a note to a review; across 65 projects only 6 requests changed. - Java, Kotlin, Scala and Groovy packages named `example` or `demo`, such as Spring Initializr's default `com.example.demo`, are source, not example code: every finding of such a project was capped (injection and sensitive data at a consider, the rest at a note) and its hardcoded values were not judged. Directories above the source root, such as `examples/`, still mark example code. diff --git a/docs/classification-cascade.md b/docs/classification-cascade.md index bd52045..611e16c 100644 --- a/docs/classification-cascade.md +++ b/docs/classification-cascade.md @@ -47,6 +47,14 @@ signatures, or one candidate pair. Astro, Vue and Svelte files are parsed as their scripts: Astro frontmatter and `