diff --git a/CHANGELOG.md b/CHANGELOG.md index 58612ad..ebd2ab7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,33 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +Measured on 103 pinned projects (24 new open-source ones of kinds not tried before, among them intentionally vulnerable Rails, Node, GraphQL, C# and Java apps, a Deno framework, a WordPress plugin, a cookiecutter template and projects in Kotlin, Swift, Elixir and C, and 8 more of the maintainer's own), with findings labeled by hand: on the 70 labeled projects JevGate was tuned on, 75% of reviews were right against 69% with 0.20.0 (136 wrong reviews against 192), and 72% of considers against 65% (254 wrong considers against 354); on 11 held-out projects, 61% of reviews against 57%, and 56% of considers against 54%. Undecided units went from 2.2% to 1.5% of judged units. + +- Server templates: ERB, EJS, JSP, Handlebars, Mustache, Nunjucks, Twig, Jinja and Go templates, and HTML under `templates/`, `views/`, `layouts/`, `partials/` or `includes/`, are judged. Their inline `\n" + )); + assert!(!inline_scripts( + "

Hi

\n\n" + )); + assert!(!inline_scripts( + "" + )); + assert!(!inline_scripts("

{{ name }}

")); + } +} diff --git a/src/docs/discover.rs b/src/docs/discover.rs index 25d9c27..30b5a6c 100644 --- a/src/docs/discover.rs +++ b/src/docs/discover.rs @@ -125,6 +125,19 @@ fn project_doc(path: &Path) -> bool { && !RECORD_STEMS.contains(&stem.as_str()) } +/// Claude Code skills, commands and subagent definitions: Markdown under +/// `.claude/skills`, `.claude/commands` or `.claude/agents`. A session loads +/// only their descriptions and reads the rest when one is used, so they are +/// project documentation, checked for stale paths, repetition and size: one +/// project's fifteen skills cited documentation paths a rename had removed. +fn claude_doc(path: &Path) -> bool { + let parts: Vec<&str> = path.iter().filter_map(|p| p.to_str()).collect(); + parts + .windows(2) + .any(|w| w[0] == ".claude" && matches!(w[1], "skills" | "commands" | "agents")) + && path.extension().and_then(|e| e.to_str()) == Some("md") +} + fn file_name(path: &Path) -> &str { path.file_name().and_then(|n| n.to_str()).unwrap_or("") } @@ -152,23 +165,14 @@ pub fn discover(root: &Path) -> Result { found.directories.insert(relative); } else if agent_file(&relative) { add_agent(root, relative, &mut found); - } else if project_doc(&relative) { + } else if project_doc(&relative) || claude_doc(&relative) { found.project.insert(relative); } } // Ignored instruction files next to visible ones still load, so probe for // them by name and walk the agent directories without ignore files. for directory in found.directories.clone() { - for name in AGENT_NAMES { - let path = directory.join(name); - if root - .join(&path) - .symlink_metadata() - .is_ok_and(|m| !m.is_dir()) - { - add_agent(root, path, &mut found); - } - } + probe_agents(root, &directory, &mut found); for name in AGENT_DIRS { let path = root.join(&directory).join(name); if path.is_dir() { @@ -186,6 +190,30 @@ pub fn discover(root: &Path) -> Result { Ok(found) } +/// The instruction files of one directory, found by name even when ignore +/// files hide them. Only an entry of exactly that name counts: on a +/// case-insensitive file system, `AGENTS.md` also opens refined-github's +/// `agents.md`, whose read then failed and left the whole run incomplete. +fn probe_agents(root: &Path, directory: &Path, found: &mut Found) { + let names: BTreeSet = std::fs::read_dir(root.join(directory)) + .into_iter() + .flatten() + .flatten() + .map(|entry| entry.file_name()) + .collect(); + for name in AGENT_NAMES { + let path = directory.join(name); + if names.contains(std::ffi::OsStr::new(name)) + && root + .join(&path) + .symlink_metadata() + .is_ok_and(|m| !m.is_dir()) + { + add_agent(root, path, found); + } + } +} + /// Documentation folders read even when ignored. const DOC_DIRS: &[&str] = &["docs", "doc"]; @@ -222,8 +250,13 @@ fn walk_agent_dir(root: &Path, dir: &Path, found: &mut Found) -> Result<()> { { let entry = entry.context("Failed while discovering agent instructions")?; let relative = crate::discovery::relative(entry.path(), root)?; - if !entry.file_type().is_some_and(|t| t.is_dir()) && agent_file(&relative) { + if entry.file_type().is_some_and(|t| t.is_dir()) { + continue; + } + if agent_file(&relative) { add_agent(root, relative, found); + } else if claude_doc(&relative) { + found.project.insert(relative); } } Ok(()) @@ -303,6 +336,20 @@ mod tests { } } + #[test] + fn an_instruction_file_is_found_by_its_exact_name() { + let project = crate::tests::Project::new(); + project.write("agents.md", "# Agents\n"); + project.write("docs/CLAUDE.md", "# C\n"); + let found = discover(&project.0).unwrap(); + let agent: Vec<_> = found.agent.iter().map(|p| p.to_str().unwrap()).collect(); + assert_eq!( + agent, + ["docs/CLAUDE.md"], + "a lowercase agents.md is not AGENTS.md" + ); + } + #[test] fn ignored_and_hidden_agent_files_are_found() { let project = crate::tests::Project::new(); @@ -312,6 +359,12 @@ mod tests { ("AGENTS.md", "# A\n"), ("README.md", "# R\n"), (".claude/rules/x.md", "# X\n"), + ( + ".claude/skills/deploy/SKILL.md", + "---\nname: deploy\n---\n# Deploy\n", + ), + (".claude/skills/deploy/run.sh", "echo\n"), + (".claude/commands/review.md", "# Review\n"), ("notes/n.md", "# N\n"), ("src/CLAUDE.md", "# C\n"), ("node_modules/p/README.md", "# P\n"), @@ -322,6 +375,14 @@ mod tests { let agent: Vec<_> = found.agent.iter().map(|p| p.to_str().unwrap()).collect(); assert_eq!(agent, [".claude/rules/x.md", "AGENTS.md", "src/CLAUDE.md"]); let docs: Vec<_> = found.project.iter().map(|p| p.to_str().unwrap()).collect(); - assert_eq!(docs, ["README.md", "docs/plan.md"]); + assert_eq!( + docs, + [ + ".claude/commands/review.md", + ".claude/skills/deploy/SKILL.md", + "README.md", + "docs/plan.md" + ] + ); } } diff --git a/src/docs/load.rs b/src/docs/load.rs index e36dbb8..1859aff 100644 --- a/src/docs/load.rs +++ b/src/docs/load.rs @@ -278,13 +278,25 @@ pub fn files( source: source.clone(), }) .collect(); - // A link loads its target's text under its own name. + // A link loads its target's text under its own name. A target that no + // harness reads by its own name, such as refined-github's `agents.md` + // behind its `CLAUDE.md`, is read through the link instead: it takes the + // link's readers, so its findings name the file itself and its text is + // counted once. for (link, target) in links { - let Some(source) = target.as_ref().and_then(|t| sources.get(t)) else { + let Some((target, source)) = target.as_ref().and_then(|t| sources.get_key_value(t)) else { continue; }; + let linked = readers(link, &markdown::parse(source), &exists); + if let Some(file) = files + .iter_mut() + .find(|f| f.path == *target && f.readers.is_empty()) + { + file.readers = linked; + continue; + } files.push(File { - readers: readers(link, &markdown::parse(source), &exists), + readers: linked, path: link.clone(), source: source.clone(), }); @@ -729,23 +741,32 @@ mod tests { )); } - #[test] - fn a_linked_claude_file_reads_the_same_instructions() { + /// `CLAUDE.md` linked to `target`: the files Claude Code reads, and + /// the load facts. + fn linked_claude(target: &str) -> (Vec, usize) { let project = crate::tests::Project::new(); let text = "# Build\nRun `make`.\n"; - project.write("AGENTS.md", text); - let links = [(PathBuf::from("CLAUDE.md"), Some(PathBuf::from("AGENTS.md")))]; - let files = files( - [(PathBuf::from("AGENTS.md"), text.to_string())].into(), - &links, - ); - let claude: Vec<_> = files + project.write(target, text); + let links = [(PathBuf::from("CLAUDE.md"), Some(PathBuf::from(target)))]; + let files = files([(PathBuf::from(target), text.to_string())].into(), &links); + let claude = files .iter() - .flat_map(|f| f.readers.iter().map(move |r| (&f.path, r))) - .filter(|(_, r)| r.harness == CLAUDE) - .map(|(p, _)| p.to_str().unwrap()) + .filter(|f| f.readers.iter().any(|r| r.harness == CLAUDE)) + .map(|f| f.path.to_string_lossy().into_owned()) .collect(); - assert_eq!(claude, ["CLAUDE.md"]); - assert!(context_load(&files, &links, &project.0).facts.is_empty()); + (claude, context_load(&files, &links, &project.0).facts.len()) + } + + #[test] + fn a_linked_claude_file_reads_the_same_instructions() { + assert_eq!( + linked_claude("AGENTS.md"), + (vec!["CLAUDE.md".to_string()], 0) + ); + // A target no harness names is read once, under its own name. + assert_eq!( + linked_claude("agents.md"), + (vec!["agents.md".to_string()], 0) + ); } } diff --git a/src/docs/mod.rs b/src/docs/mod.rs index 77a3537..efe3d4e 100644 --- a/src/docs/mod.rs +++ b/src/docs/mod.rs @@ -47,7 +47,11 @@ impl Repository { pub fn scan(root: &Path) -> Result { let found = discover::discover(root)?; - let sources = agent_sources(root, &found.agent); + // A link's target is read even when its own name is no instruction + // file's (`load::files`). + let mut read = found.agent.clone(); + read.extend(found.links.iter().filter_map(|(_, target)| target.clone())); + let sources = agent_sources(root, &read); let generated_files: BTreeSet = sources .iter() .filter(|(_, source)| generated(source)) diff --git a/src/docs/references.rs b/src/docs/references.rs index faea021..3789468 100644 --- a/src/docs/references.rs +++ b/src/docs/references.rs @@ -247,7 +247,8 @@ fn nearby(base: &Path, name: &str, history: &History) -> Option { } /// A token naming a repository path: a file extension, or a first directory -/// the repository has. Routes, URLs, globs and placeholders are not. +/// the repository has. Routes, URLs, globs and placeholders are not, such as +/// a Claude command's `.kiro/specs/$1/spec.json`, whose `$1` is its argument. fn path_like(token: &str, top: &BTreeSet<&str>) -> Option { let mut t = token .trim() @@ -261,7 +262,7 @@ fn path_like(token: &str, top: &BTreeSet<&str>) -> Option { let excluded = ["http", "mailto:", "#", "$", "-", "@", "~", "/"] .iter() .any(|p| t.starts_with(p)) - || t.chars().any(|c| "*<>{}|=()[] ,'\"".contains(c)) + || t.chars().any(|c| "*<>{}|=()[] ,'\"$".contains(c)) || t.is_empty(); if excluded { return None; @@ -585,7 +586,7 @@ mod tests { fn routes_branches_urls_and_globs_are_not_paths() { assert!( found( - "Open `/login`, merge `origin/main`, fetch `https://x.io/a.ts`, match `src/*.ts`." + "Open `/login`, merge `origin/main`, fetch `https://x.io/a.ts`, match `src/*.ts`, read `specs/$1/spec.json`." ) .is_empty() ); diff --git a/src/file_kind.rs b/src/file_kind.rs index e2fae04..04a70f0 100644 --- a/src/file_kind.rs +++ b/src/file_kind.rs @@ -132,6 +132,10 @@ pub fn unsent(path: &Path, named_source: &str, detail: &str) -> Classification { } pub fn language(path: &Path) -> &'static str { + if crate::components::server_template(path) { + // Only its inline scripts are parsed and judged. + return "JavaScript"; + } match extension(path).as_str() { "rs" => "Rust", "py" => "Python", diff --git a/src/inventory/django.rs b/src/inventory/django.rs index ba9ff28..fe5a979 100644 --- a/src/inventory/django.rs +++ b/src/inventory/django.rs @@ -64,12 +64,15 @@ pub(super) fn unescaped_templates( boundary: &Boundary, inputs: &mut [Input], ) { + // Python views name templates as `blog/post.html`; a Node handler + // renders a view by name, as in `res.render('app/products', …)`. let candidate = |input: &Input| { - input.result.path.extension().is_some_and(|e| e == "py") - && input - .source - .as_deref() - .is_some_and(|source| source.contains(".html")) + let source = input.source.as_deref().unwrap_or(""); + match input.result.path.extension().and_then(|e| e.to_str()) { + Some("py") => source.contains(".html"), + Some("js" | "mjs" | "cjs" | "ts" | "mts" | "cts") => source.contains(".render("), + _ => false, + } }; if !inputs.iter().any(candidate) { return; @@ -80,9 +83,10 @@ pub(super) fn unescaped_templates( let Ok(relative) = &crate::discovery::relative(path, &context.root) else { continue; }; - let Some(name) = crate::analysis::django::template_name(relative) else { + let django = crate::analysis::django::template_name(relative); + if django.is_none() && crate::analysis::views::view_name(relative).is_none() { continue; - }; + } if !entry.file_type().is_some_and(|t| t.is_file()) || !boundary.permits(relative) || std::fs::metadata(path) @@ -93,13 +97,18 @@ pub(super) fn unescaped_templates( let Ok(text) = std::fs::read_to_string(path) else { continue; }; - let unescaped = crate::analysis::django::unescaped_lines(&text); - if !unescaped.is_empty() { - templates.push(crate::analysis::django::Template { - name, - path: relative.to_path_buf(), - unescaped, - }); + match django { + Some(name) => { + let unescaped = crate::analysis::django::unescaped_lines(&text); + if !unescaped.is_empty() { + templates.push(crate::analysis::django::Template { + name, + path: relative.to_path_buf(), + unescaped, + }); + } + } + None => templates.extend(crate::analysis::views::view(relative, &text)), } } templates.sort_by(|a, b| a.path.cmp(&b.path)); diff --git a/src/inventory/documents.rs b/src/inventory/documents.rs index 51486e9..fc557b6 100644 --- a/src/inventory/documents.rs +++ b/src/inventory/documents.rs @@ -62,7 +62,8 @@ pub(super) fn load_document( templates: Vec::new(), } } - Err(error) => error_input(result, error), + // dvja's docs hold a Markdown file with NUL bytes, which made the run incomplete. + Err(error) => unread(result, error), }) } diff --git a/src/inventory/mod.rs b/src/inventory/mod.rs index 33a98f3..56a6664 100644 --- a/src/inventory/mod.rs +++ b/src/inventory/mod.rs @@ -143,7 +143,12 @@ fn source_paths( continue; } let relative = &discovery::relative(path, &context.root)?; - if discovery::source(relative, &args.source_extension) + // A server template counts only for its inline scripts and the code + // that reads client data. + let template = crate::components::server_template(relative) + && std::fs::read_to_string(path) + .is_ok_and(|text| crate::components::judged(relative, &text)); + if (discovery::source(relative, &args.source_extension) || template) && selected(relative) && boundary.permits(relative) && super::context::ensure_visible_path(relative).is_ok() @@ -236,7 +241,7 @@ fn load( extra: &[super::context::ContextInput], ) -> Result { let relative = &discovery::relative(&path, &context.root).context("Source outside root")?; - let mut result = pending_result(relative, &role, args, extra); + let result = pending_result(relative, &role, args, extra); if !matches!(role.as_str(), "source" | "test") { return Ok(excluded(result, &role, relative)); } @@ -258,14 +263,19 @@ fn load( Some(kind) => recast(result, kind, relative), None => source_input(result, source, (&path, relative), args, context, extra), }), - // Binary and non-UTF-8 files are reported and skipped; they never make a run incomplete. - Err(error) if not_text(&error) => { - result.status = Status::Skipped; - result.error = Some(format!("{error}; this file was not judged.")); - Ok(bare_input(result)) - } - Err(error) => Ok(error_input(result, error)), + Err(error) => Ok(unread(result, error)), + } +} + +/// A file that could not be read. Binary and non-UTF-8 files are reported and +/// skipped; they never make a run incomplete. +fn unread(mut result: FileResult, error: anyhow::Error) -> Input { + if not_text(&error) { + result.status = Status::Skipped; + result.error = Some(format!("{error}; this file was not judged.")); + return bare_input(result); } + error_input(result, error) } /// Application source or a test read whole, at `path` and `relative` to the diff --git a/src/options/mod.rs b/src/options/mod.rs index e9b6b60..1f443d8 100644 --- a/src/options/mod.rs +++ b/src/options/mod.rs @@ -100,9 +100,11 @@ pub struct CheckArgs { /// /// Without paths, JevGate walks the repository (respecting .gitignore) and /// selects application source in Rust, Python, JavaScript, TypeScript, Go, - /// C#, Ruby, PHP and Java. Tests, generated code and vendored files are - /// classified and skipped with a reason. `upload_allow`/`upload_deny` in - /// jevgate.toml still bound what is sent. + /// C#, Ruby, PHP and Java, the scripts of Astro, Vue and Svelte files, and + /// server templates (ERB, EJS, JSP, Handlebars, Jinja and others) that + /// hold inline scripts or code reading the request. Tests, generated code + /// and vendored files are classified and skipped with a reason. + /// `upload_allow`/`upload_deny` in jevgate.toml still bound what is sent. pub paths: Vec, /// Review only files changed against this Git revision (commit, branch or tag) /// diff --git a/src/packages.rs b/src/packages.rs index 0414550..4deb61d 100644 --- a/src/packages.rs +++ b/src/packages.rs @@ -19,7 +19,7 @@ pub struct Package { const MANIFEST_BYTES: u64 = 1_048_576; /// The package of a file: the nearest directory at or above it, up to the -/// root, with a `package.json`, `Cargo.toml` or `pyproject.toml`. +/// root, with a `package.json`, `Cargo.toml`, `pyproject.toml` or `go.mod`. pub fn package(root: &Path, relative: &Path) -> Option { relative.ancestors().skip(1).find_map(|dir| { let read = |name: &str| { @@ -32,6 +32,7 @@ pub fn package(root: &Path, relative: &Path) -> Option { read("package.json").map(|t| node_manifest(&t)), read("Cargo.toml").map(|t| cargo_manifest(&t)), read("pyproject.toml").map(|t| python_manifest(&t)), + read("go.mod").map(|t| go_manifest(&t)), ]; let mut package = Package { dir: dir.to_path_buf(), @@ -79,6 +80,48 @@ fn cargo_manifest(text: &str) -> Manifest { (name, dependencies) } +/// A Go module's path and the modules it requires, on `require` lines and +/// in `require ( … )` blocks. Without it, Online Boutique's Go services, +/// each its own module, read as one package, and 9 of 10 copies found +/// between them were wrong: each service is built on its own. +fn go_manifest(text: &str) -> Manifest { + let lines: Vec<&str> = text + .lines() + .map(|line| line.split("//").next().unwrap_or("").trim()) + .collect(); + let name = lines + .iter() + .find_map(|line| line.strip_prefix("module ")) + .map(|path| path.trim_matches('"').to_string()); + (name, go_requirements(&lines)) +} + +/// The module paths of `require` lines and of `require ( … )` blocks. +fn go_requirements(lines: &[&str]) -> Vec { + let mut required = Vec::new(); + let mut block = false; + for line in lines { + let requirement = match (block, *line) { + (true, ")") => { + block = false; + None + } + (true, entry) => Some(entry), + (false, line) if line.starts_with("require (") => { + block = true; + None + } + (false, line) => line.strip_prefix("require "), + }; + required.extend( + requirement + .and_then(|r| r.split_whitespace().next()) + .map(str::to_string), + ); + } + required +} + fn python_manifest(text: &str) -> Manifest { let Ok(table) = text.parse::() else { return (None, Vec::new()); @@ -173,6 +216,34 @@ mod tests { assert!(linked(web.as_ref(), shared.as_ref(), &local)); assert!(!linked(web.as_ref(), template.as_ref(), &local)); assert!(!linked(template.as_ref(), rust.as_ref(), &local)); + // Go modules: separate services, and two that share a local module. + project.write( + "src/frontend/go.mod", + "module example.com/shop/frontend // the web tier\n\ngo 1.22\n\nrequire (\n\tgithub.com/gorilla/mux v1.8.1\n\texample.com/shop/common v0.0.0\n)\n", + ); + project.write( + "src/checkout/go.mod", + "module example.com/shop/checkout\n\nrequire example.com/shop/common v0.0.0\n", + ); + project.write( + "src/shipping/go.mod", + "module example.com/shop/shipping\n\nrequire github.com/gorilla/mux v1.8.1\n", + ); + project.write("src/common/go.mod", "module example.com/shop/common\n"); + let frontend = at("src/frontend/main.go"); + let checkout = at("src/checkout/main.go"); + let shipping = at("src/shipping/main.go"); + let common = at("src/common/log.go"); + let local: BTreeSet = [&frontend, &checkout, &shipping, &common] + .iter() + .filter_map(|p| p.as_ref()?.name.clone()) + .collect(); + assert_eq!( + frontend.as_ref().unwrap().name.as_deref(), + Some("example.com/shop/frontend") + ); + assert!(linked(frontend.as_ref(), checkout.as_ref(), &local)); + assert!(!linked(frontend.as_ref(), shipping.as_ref(), &local)); assert!(linked( at("scripts/x.ts").as_ref(), template.as_ref(), diff --git a/src/syntax.rs b/src/syntax.rs index 553630f..41aae6c 100644 --- a/src/syntax.rs +++ b/src/syntax.rs @@ -98,20 +98,45 @@ fn extension(path: &Path) -> &str { /// Whether a parser supports this file's language. pub(crate) fn supported(path: &Path) -> bool { - grammar(path).is_some() || crate::components::FORMATS.contains(&extension(path)) + grammar(path).is_some() + || crate::components::FORMATS.contains(&extension(path)) + || crate::components::server_template(path) } -pub(crate) fn parse(path: &Path, source: &str) -> Result> { +/// The grammar a file is parsed with, and the text parsed in place of its +/// source when that is not its code as written: a component's or server +/// template's scripts, or a project template without its Jinja tags. +fn parsed_text(path: &Path, source: &str) -> Option<(tree_sitter::Language, Option)> { let extension = extension(path); - let (language, scripts) = if crate::components::FORMATS.contains(&extension) { + if crate::components::FORMATS.contains(&extension) { let (scripts, language) = crate::components::scripts(extension, source); - (language, Some(scripts)) - } else if let Some(language) = grammar(path) { - (language, None) + Some((language, Some(scripts))) + } else if crate::components::server_template(path) { + let (scripts, language) = crate::components::scripts("html", source); + Some((language, Some(without_tags(&scripts, true)))) } else { + let language = grammar(path)?; + Some(( + language, + project_template(path).then(|| without_jinja(source)), + )) + } +} + +pub(crate) fn parse(path: &Path, source: &str) -> Result> { + let extension = extension(path); + let server_template = crate::components::server_template(path); + let Some((language, scripts)) = parsed_text(path, source) else { return Ok(None); }; - let key = (extension.to_owned(), crate::schema::hash(source.as_bytes())); + // A template's tree is of its code without the Jinja tags, apart from + // the same text's tree elsewhere. + let kind = if scripts.is_some() && grammar(path).is_some() { + format!("{extension}+jinja") + } else { + extension.to_owned() + }; + let key = (kind, crate::schema::hash(source.as_bytes())); let tree = match PARSES.with(|cache| cache.borrow_mut().get(&key, source)) { Some(tree) => tree, None => { @@ -126,7 +151,7 @@ pub(crate) fn parse(path: &Path, source: &str) -> Result> { }; // Whether errors are tolerable depends on the path, not only the source. ensure!( - if template(path, source) { + if template(path, source) && !server_template { !tree.root_node().has_error() } else { tolerable(tree.root_node(), source.len()) @@ -151,6 +176,67 @@ fn template(path: &Path, source: &str) -> bool { || source.contains("//#if") } +/// A file of a project template such as a cookiecutter's, under a directory +/// whose name holds a `{{ … }}` placeholder: its Jinja tags are no syntax of +/// its language, and 31 of cookiecutter-django's Python and JavaScript +/// files, the generated application's settings, models, views and tests, +/// were skipped for syntax errors. +fn project_template(path: &Path) -> bool { + path.iter().any(|part| { + part.to_str() + .is_some_and(|p| p.contains("{{") && p.contains("}}")) + }) +} + +/// The source with its Jinja statements and comments blanked and each +/// `{{ … }}` placeholder turned into an identifier of the same length, so +/// that byte offsets and lines stay the file's: `from {{ slug }}.users +/// import User` reads as an import, and both branches of an `{% if %}` stay. +fn without_jinja(source: &str) -> String { + without_tags(source, false) +} + +/// Jinja's tags blanked as `without_jinja` does, and with `server`, a server +/// template's as well: Handlebars' `{{{ … }}}` and each ERB, EJS or JSP +/// `<%= … %>` or `<%- … %>` read as a name, other `<% … %>` tags blanked. +fn without_tags(source: &str, server: bool) -> String { + let bytes = source.as_bytes(); + let mut out = bytes.to_vec(); + let mut at = 0; + while at + 1 < bytes.len() { + let next = bytes.get(at + 2).copied(); + let (close, fill) = match (bytes[at], bytes[at + 1]) { + (b'{', b'%') => ("%}", b' '), + (b'{', b'#') => ("#}", b' '), + (b'{', b'{') if server && next == Some(b'{') => ("}}}", b'_'), + (b'{', b'{') => ("}}", b'_'), + (b'<', b'%') if server => ( + "%>", + if matches!(next, Some(b'=' | b'-')) { + b'_' + } else { + b' ' + }, + ), + _ => { + at += 1; + continue; + } + }; + let Some(length) = source[at + 2..].find(close) else { + break; + }; + let end = at + 2 + length + close.len(); + for byte in &mut out[at..end] { + if *byte != b'\n' { + *byte = fill; + } + } + at = end; + } + String::from_utf8(out).unwrap_or_else(|_| source.to_string()) +} + /// Whether a tree's syntax errors are few and small enough to judge the /// rest of the file. Grammars miss some valid code: tree-sitter-typescript /// reads a call signature that starts with `` on the line after another @@ -193,6 +279,27 @@ mod tests { use crate::locations::collect; use tree_sitter::{InputEdit, Point}; + #[test] + fn jinja_tags_of_a_project_template_are_not_its_syntax() { + let source = "{% if cookiecutter.use_celery == 'y' %}\nfrom celery import shared_task\n{% endif %}\nfrom {{ cookiecutter.project_slug }}.users.models import User\n\n\ndef total(values):\n {# the café's sum #}\n return sum(values)\n"; + let blanked = without_jinja(source); + assert_eq!(blanked.len(), source.len()); + assert_eq!(blanked.lines().count(), source.lines().count()); + let placeholder = "_".repeat("{{ cookiecutter.project_slug }}".len()); + assert!(blanked.contains(&format!("from {placeholder}.users.models import User"))); + let tree = parse( + Path::new("{{cookiecutter.project_slug}}/app/tasks.py"), + source, + ) + .unwrap() + .unwrap(); + assert!(!tree.root_node().has_error()); + assert!( + parse(Path::new("app/tasks.py"), source).is_err(), + "outside a template its tags are syntax errors" + ); + } + #[test] fn identical_source_reuses_a_tree_across_paths() { let source = "function before() { return 1; }"; @@ -223,6 +330,39 @@ mod tests { ); } + #[test] + fn a_server_template_parses_as_its_inline_scripts_with_its_tags_blanked() { + let erb = "

<%= @title %>

\n<% if admin? %>

Admin

<% end %>\n\n"; + let (masked, _) = crate::components::scripts("html", erb); + let blanked = without_tags(&masked, true); + assert_eq!(blanked.len(), erb.len()); + assert_eq!(blanked.lines().count(), erb.lines().count()); + assert!(!blanked.contains("

") && !blanked.contains("<%")); + assert!(blanked.contains(&format!( + "var tags = {};", + "_".repeat("<%== @tags.to_json %>".len()) + ))); + // Handlebars' triple stash and Jinja's tags, in a template directory. + let jinja = "{% extends 'base.html' %}\n\n"; + for (path, source, function) in [ + ("app/views/sessions/new.html.erb", erb, ("greet", 6)), + ("server/templates/profile.html", jinja, ("show", 5)), + ] { + let path = Path::new(path); + assert!( + !parse(path, source) + .unwrap() + .unwrap() + .root_node() + .has_error() + ); + assert_eq!( + collect(path, source, Path::new(".")).unwrap().1, + vec![(function.0.into(), function.1)] + ); + } + } + #[test] fn component_scripts_parse_in_place() { let astro = "---\nimport Layout from '../layouts/Layout.astro'\nconst posts = await getPosts()\nfunction title(p) { return p.data.title }\n---\n{posts.map(p => {title(p)})}\n\n"; diff --git a/src/test_locations/javascript.rs b/src/test_locations/javascript.rs index 8a8d2c4..be00a15 100644 --- a/src/test_locations/javascript.rs +++ b/src/test_locations/javascript.rs @@ -1,5 +1,5 @@ -//! JavaScript and TypeScript tests: `describe`, `it` and `test` calls and -//! their hooks, as statements. +//! JavaScript and TypeScript tests: `describe`, `it`, `test` and +//! `Deno.test` calls and their hooks, as statements. use super::child_text; use tree_sitter::Node; @@ -25,6 +25,7 @@ fn is_test_call(name: &str) -> bool { "afterEach", "beforeAll", "afterAll", + "Deno.test", ]; NAMES .iter() diff --git a/src/units/client_app.rs b/src/units/client_app.rs new file mode 100644 index 0000000..6e6fa83 --- /dev/null +++ b/src/units/client_app.rs @@ -0,0 +1,62 @@ +//! What a program people run on their own machine does with the text it +//! shows, sent beside the file's path and language like the web framework +//! roles: a game client that hands the server's error text to its own +//! window over a channel of `Response` messages read as a server answering +//! a remote client, in fifteen reviews for sending internal error details. +//! Facts come from the package's dependencies on a desktop, game or +//! terminal interface toolkit. +use crate::packages::Package; + +const CLIENT: &str = "Client application: this package is a desktop, game or terminal program that runs on its user's own machine, so the errors and messages it shows or passes to its own screens go to that user, not in a response to a remote client."; + +/// Dependencies that make a package a program people run on their own machine. +const INTERFACES: [&str; 16] = [ + "ratatui", + "cursive", + "egui", + "eframe", + "iced", + "bevy", + "macroquad", + "ggez", + "slint", + "druid", + "fltk", + "gtk4", + "relm4", + "tauri", + "spacetimedb-sdk", + "electron", +]; + +/// The client facts of a file whose package depends on an interface toolkit. +pub(super) fn describe(package: Option<&Package>) -> Option<&'static str> { + package + .filter(|p| INTERFACES.iter().any(|d| p.dependencies.contains(*d))) + .map(|_| CLIENT) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn package(dependencies: &[&str]) -> Package { + Package { + dir: PathBuf::from("client"), + name: None, + dependencies: dependencies.iter().map(|d| d.to_string()).collect(), + } + } + + #[test] + fn packages_with_an_interface_toolkit_are_client_applications() { + assert_eq!( + describe(Some(&package(&["ratatui", "spacetimedb-sdk"]))), + Some(CLIENT) + ); + assert!(describe(Some(&package(&["tauri", "serde"]))).is_some()); + assert!(describe(Some(&package(&["axum", "tokio"]))).is_none()); + assert!(describe(None).is_none()); + } +} diff --git a/src/units/comments.rs b/src/units/comments.rs index a5cc340..7cd3466 100644 --- a/src/units/comments.rs +++ b/src/units/comments.rs @@ -82,9 +82,9 @@ pub(super) fn plan( documentation: teaching || matches!(comment.placement, Placement::Declaration | Placement::File) || crate::analysis::comments::banner(&comment.text), - kind, + kind: kind.map(Into::into), }, - recheck, + recheck: recheck.map(Into::into), }); let entry = Entry { id, diff --git a/src/units/compose.rs b/src/units/compose.rs index 1e9b488..19f61eb 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, at_most_note, benefit, checks, choice, lowered, noul, open, - origin_outcome, score, several_kind, unit_outcome, value_signals, + Answers, Outcome, at_most_note, benefit, checks, choice, document_split, lowered, noul, + open, origin_outcome, score, settled_checks, several_kind, unit_outcome, value_signals, }, wording::{Wording, comment_reason, comment_wording}, wording::{ @@ -196,6 +196,8 @@ fn test_value_answers<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> Answers merged.insert(question, answer); } } + // What its assertions read, asked after an internal-details consider. + merged.extend(answers(judgments, &unit.id, Pass::Locate)); merged } @@ -244,12 +246,18 @@ pub fn unlocated_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet matches!(outcome, Outcome::Review(_)), + // Only the internal-details check raises a test's consider. + Detail::Test { confirm: Some(_) } => matches!(outcome, Outcome::Consider(_)), Detail::Function { locate: Some(_), .. - } - | Detail::Document { - locate: Some(_), .. } => raised(resolved.get("split").map(|a| benefit(a))), + Detail::Document { + locate: Some(_), .. + } => raised( + resolved + .get("split") + .map(|a| document_split(a, resolved.get("kind").copied())), + ), _ => false, } }) @@ -324,7 +332,8 @@ pub fn unkinded_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet bool { if ![catalog::FILE_ORGANIZATION, catalog::LARGE_DOCS].contains(&unit.rule) || !answers(judgments, &unit.id, Pass::Trace).is_empty() @@ -336,9 +345,14 @@ fn unkinded_split(unit: &UnitPlan, judgments: &[Judgment]) -> bool { } else { Pass::First }; + let document = unit.rule == catalog::LARGE_DOCS; answers(judgments, &unit.id, pass) .get("split") - .is_some_and(|a| matches!(benefit(a), Outcome::Uncertain(_))) + .is_some_and(|a| match benefit(a) { + Outcome::Uncertain(_) => true, + Outcome::Consider(_) | Outcome::Review(_) => document, + _ => false, + }) } /// A section pair or stale section whose checks were asked, stayed @@ -509,15 +523,7 @@ impl<'p> Tally<'p> { return; } 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 { - outcome - }; + let outcome = capped(unit, judgments, few, outcome); // Two tests that check one behavior with different inputs are a note // on their own; three or more linked by such pairs are grouped into a // consider below. Labeled by hand on just, express, gson and @@ -1011,7 +1017,7 @@ fn finding( let reason = comment_reason(answers, documented(unit)); comment_wording(name, &[(&unit.locations[0], reason)], strength, p) } - Detail::Test => test_wording(name, strength, p, answers), + Detail::Test { .. } => test_wording(name, strength, p, answers), Detail::TestPair { .. } => { symbol = None; test_pair_wording(name, strength == Strength::Review, p) @@ -1056,6 +1062,33 @@ fn finding( } } +/// A unit's outcome under the caps its rule and facts put on it: an +/// unnamed or single-use value, a value that only needs a name, security +/// code at a test path or resting on what lies outside the function, a +/// short outline or section, an outline naming no group, and comments too +/// few to act on. +fn capped( + unit: &UnitPlan, + judgments: &[Judgment], + few: &BTreeSet<&str>, + outcome: Outcome, +) -> Outcome { + if unnamed_value(unit, judgments) { + return lowered(lowered(outcome)); + } + if single_use_value(unit, judgments) || short_outline(unit) || small_section(unit) { + return at_most_note(outcome); + } + if named_value_only(unit, judgments) { + return at_most_consider(outcome); + } + let lower = test_path_security(unit) + || outside_function(unit, judgments) + || unnamed_outline(unit, judgments) + || few.contains(unit.id.as_str()); + if lower { lowered(outcome) } else { outcome } +} + /// A function's hardcoded-value review or consider whose value was not /// named: the locate Choice picked none clearly, or there were too many /// values to offer. Its finding is a note, since a reader cannot tell what @@ -1069,6 +1102,141 @@ fn unnamed_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { ) } +/// A hardcoded-value review or consider that rests only on whether a value +/// needs a name. Naming a value is a cleanup, so it is at most a consider: +/// labeled by hand, 17 such reviews were right and 18 wrong, most of the +/// wrong ones tuning in game, audio and animation code (a scheduler's +/// 500 ms, a hash seed, a mix gain, a float epsilon). +fn named_value_only(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + if !matches!(unit.detail, Detail::Values { .. }) { + return false; + } + let (outcome, answers) = resolved(unit, judgments); + if !matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) { + return false; + } + let get = |q: &str| answers.get(q).copied(); + crate::units::outcome::value_signals(&get, &unit.detail, true) + .unwrap_or_default() + .iter() + .filter(|(_, o, _)| matches!(o, Outcome::Review(_) | Outcome::Consider(_))) + .all(|(question, ..)| *question == "magic") +} + +/// Such a finding about a value its file writes once is a note: labeled by +/// hand on 35 projects, those considers were right 19 times in 52, against +/// 34 in 49 for a value its file repeats. A delay given to `setTimeout`, a +/// size given to an attribute or a CSS class reads where it is used; a value +/// written twice can drift apart. +fn single_use_value(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let Detail::Values { repeated, .. } = &unit.detail else { + return false; + }; + named_value_only(unit, judgments) + && located_option(unit, judgments, ("value", 'v')) + .is_some_and(|i| repeated.get(i) == Some(&false)) +} + +/// Weak-setting checks whose review needs its settle Choice to name what +/// the function itself does, and the option that does: whether a token was +/// verified before the function reads it, or whether a callee or model hook +/// hashes the password it saves, lies outside the function. +const SHOWN_IN_FUNCTION: [(&str, &str, &str); 2] = [ + ("token", "token_use", "turned_off"), + ("hash", "password_handling", "fast_hash"), +]; + +/// An unsafe-settings review named only by checks of `SHOWN_IN_FUNCTION` +/// whose Choice does not name what the function itself does. Labeled by +/// hand, reviews that decoded a token to decide access were right in +/// intentionally vulnerable apps and wrong in three others (a SpacetimeDB +/// module whose host verifies tokens, a SvelteKit hook whose API verifies +/// them, an identity provider's token read over TLS), and reviews for +/// passwords saved as plain text were wrong where a service or an entity's +/// `@BeforeInsert` hook hashed them; turning `verify_signature` off and +/// hashing with MD5 in the function were right. It is one level lower. +fn outside_function(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + if unit.rule != catalog::UNSAFE_SETTINGS { + return false; + } + let (outcome, answers) = resolved(unit, judgments); + if !matches!(outcome, Outcome::Review(_)) { + return false; + } + let get = |q: &str| answers.get(q).copied(); + let named: Vec<&str> = settled_checks(unit.rule, &get) + .into_iter() + .filter(|(_, o)| matches!(o, Outcome::Review(_))) + .map(|(id, _)| id) + .collect(); + let shown = |check: &str| { + SHOWN_IN_FUNCTION + .iter() + .find(|(id, ..)| *id == check) + .is_none_or(|(_, question, option)| { + matches!( + choice(get(question)), + Some((chosen, p)) if chosen == *option + && crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY) + ) + }) + }; + !named.is_empty() && !named.iter().any(|check| shown(check)) +} + +/// Instruction sections of fewer tokens than this cost a session too little +/// to be worth a consider. +const SECTION_NOTE_TOKENS: usize = 15; + +/// An instruction section of fewer than 15 tokens is a note: labeled by +/// hand, 1 of 10 findings on such sections was right, most of them a title +/// and a "Last updated" line read as a record of past work, against 64 of +/// 68 on larger ones. +fn small_section(unit: &UnitPlan) -> bool { + matches!(unit.detail, Detail::Section { tokens, .. } if tokens < SECTION_NOTE_TOKENS) +} + +/// A security unit of a file at a test path, judged as application code +/// because it holds no tests, such as a test app's settings or a model only +/// tests use: like code that runs only in development, it is one level +/// lower. The dummy apps of devise and clearance and a test model hashing +/// with `password.reverse` were three wrong reviews, the only security +/// reviews or considers at test paths across 103 projects. +fn test_path_security(unit: &UnitPlan) -> bool { + matches!( + unit.detail, + Detail::Security { + test_path: true, + .. + } + ) +} + +/// Why a hardcoded-value finding is below the level its answers reached, +/// with that level. +fn lowered_value(unit: &UnitPlan, judgments: &[Judgment]) -> Option<(Strength, &'static str)> { + let why = if unnamed_value(unit, judgments) { + "No single value stood out, so it is a note." + } else if single_use_value(unit, judgments) { + "It is written once in its file, so it is a note." + } else if named_value_only(unit, judgments) + && matches!(resolved(unit, judgments).0, Outcome::Review(_)) + { + "" + } else { + return None; + }; + strength_of(resolved(unit, judgments).0).map(|(s, _)| (s, why)) +} + +/// A review lowered to a consider; other outcomes as they are. +fn at_most_consider(outcome: Outcome) -> Outcome { + match outcome { + Outcome::Review(p) => Outcome::Consider(p), + other => other, + } +} + /// A file-organization consider that says only that some members could /// move, naming no group: the module Choice was not asked (one group or /// none) or spread wider than two groups, and no kind of file decided it. @@ -1155,21 +1323,27 @@ fn values_finding( answers: &Answers<'_>, judgments: &[Judgment], ) -> (Wording, Option) { - let unnamed = unnamed_value(unit, judgments) - .then(|| strength_of(resolved(unit, judgments).0).map(|(s, _)| s)) - .flatten(); - let (message, action) = - values_wording(&unit.name, &unit.detail, (strength, unnamed), p, answers); + let lowered = lowered_value(unit, judgments); + let (message, action) = values_wording( + &unit.name, + &unit.detail, + (strength, lowered.map(|(reached, _)| reached)), + p, + answers, + ); + let why = lowered + .filter(|(_, why)| !why.is_empty()) + .map_or(String::new(), |(_, why)| format!(" {why}")); if let Some(index) = located_constant(unit, judgments) { // The finding points at the constant the Choice named. let location = unit.locations[index].clone(); let constant = location.symbol.as_deref().unwrap_or(""); - let message = format!("{message} The constant is `{constant}`."); + let message = format!("{message} The constant is `{constant}`.{why}"); return ((message, action), Some(location)); } let wording = match located_value(unit, judgments) { - Some(value) => (format!("{message} The value is {value}."), action), - None => (message, action), + Some(value) => (format!("{message} The value is {value}.{why}"), action), + None => (format!("{message}{why}"), action), }; (wording, None) } diff --git a/src/units/documents.rs b/src/units/documents.rs index 9823993..c02d798 100644 --- a/src/units/documents.rs +++ b/src/units/documents.rs @@ -62,8 +62,8 @@ pub(super) fn plan(file: &FileContext<'_>, out: &mut FilePlan, requests: &mut Ve let kind = Some(kind_request(file, &shown)).filter(|(request, _)| file.budget.fits(request)); unit.detail = Detail::Document { parts, - locate, - kind, + locate: locate.map(Into::into), + kind: kind.map(Into::into), }; out.units.push(unit); requests.push(Planned { diff --git a/src/units/drift.rs b/src/units/drift.rs index 197644c..95129e1 100644 --- a/src/units/drift.rs +++ b/src/units/drift.rs @@ -261,8 +261,8 @@ impl<'a> Shared<'a> { identity: identity(&[&compact(§ion.text), &compact(&other_section.text)]), detail: Detail::DocPair { other, - check: fits.then_some((request, asked)), - settle: fits.then_some(settle), + check: fits.then(|| (request, asked).into()), + settle: fits.then(|| settle.into()), }, recheck: None, } @@ -441,8 +441,8 @@ fn stale_section( identity: identity(&[§ion.heading, &compact(§ion.text)]), detail: Detail::Stale { missing: missing.iter().map(describe).collect(), - check: fits.then_some((request, asked)), - settle: fits.then_some(settle), + check: fits.then(|| (request, asked).into()), + settle: fits.then(|| settle.into()), }, recheck: None, } diff --git a/src/units/duplicates.rs b/src/units/duplicates.rs index 535a733..506e6dd 100644 --- a/src/units/duplicates.rs +++ b/src/units/duplicates.rs @@ -109,7 +109,7 @@ pub(super) fn plan( .chain(&pair.copies) .all(in_case), }, - recheck, + recheck: recheck.map(Into::into), }); } } diff --git a/src/units/follow_ups.rs b/src/units/follow_ups.rs index b6a88f2..196f95c 100644 --- a/src/units/follow_ups.rs +++ b/src/units/follow_ups.rs @@ -1,13 +1,13 @@ //! Follow-up requests that recorded answers call for: traces of security //! units, rechecks of undecided units, the kind of an outline still undecided //! and locating split findings. -use super::{Detail, Plan, Planned, UnitPlan, compose}; +use super::{Detail, FollowUp, Plan, Planned, UnitPlan, compose}; use crate::schema::{FileResult, Judgment, Status}; -use serde_json::Value; use std::collections::BTreeSet; /// One locate follow-up per function whose split raised a review or consider, -/// and per hardcoded-value function raised to a review or consider. +/// per hardcoded-value function raised to a review or consider, per redundant +/// test pair raised to a review, and per test that asserts internal details. pub fn locates(plan: &Plan, files: &[FileResult]) -> Vec { follow_ups(plan, files, compose::unlocated_units, |unit| { match &unit.detail { @@ -15,7 +15,7 @@ pub fn locates(plan: &Plan, files: &[FileResult]) -> Vec { | Detail::Document { locate, .. } | Detail::Values { locate, .. } | Detail::Constants { locate, .. } => locate.as_ref(), - Detail::TestPair { confirm, .. } => confirm.as_ref(), + Detail::TestPair { confirm, .. } | Detail::Test { confirm } => confirm.as_ref(), _ => None, } }) @@ -40,15 +40,11 @@ pub fn doc_checks(plan: &Plan, files: &[FileResult]) -> Vec { .judgments .iter() .any(|j| j.unit == unit.id && j.pass == crate::schema::Pass::Trace); - if let Some((request, questions)) = check + if let Some(check) = check && !asked && other.is_none_or(|p| !finished.contains(p)) { - planned.push(Planned { - owner, - request: request.clone(), - asked: questions.clone(), - }); + planned.push(check.planned(owner)); } } } @@ -87,11 +83,7 @@ pub fn settles(plan: &Plan, files: &[FileResult]) -> Vec { }; let open = compose::unsettled(unit, &file.judgments); for settle in settles.iter().filter(|s| open.contains(s.question)) { - planned.push(Planned { - owner, - request: settle.request.0.clone(), - asked: settle.request.1.clone(), - }); + planned.push(settle.request.planned(owner)); } } } @@ -117,7 +109,7 @@ fn follow_ups( plan: &Plan, files: &[FileResult], select: fn(&super::FilePlan, &[Judgment]) -> BTreeSet, - follow_up: fn(&UnitPlan) -> Option<&(Value, super::Asked)>, + follow_up: fn(&UnitPlan) -> Option<&FollowUp>, ) -> Vec { let mut planned = Vec::new(); for (&owner, file_plan) in &plan.files { @@ -127,14 +119,10 @@ fn follow_ups( } let selected = select(file_plan, &file.judgments); for unit in &file_plan.units { - if let Some((request, asked)) = follow_up(unit) + if let Some(follow_up) = follow_up(unit) && selected.contains(&unit.id) { - planned.push(Planned { - owner, - request: request.clone(), - asked: asked.clone(), - }); + planned.push(follow_up.planned(owner)); } } } diff --git a/src/units/functions.rs b/src/units/functions.rs index 8824a83..49cf591 100644 --- a/src/units/functions.rs +++ b/src/units/functions.rs @@ -43,8 +43,11 @@ pub(super) fn plan( quote: None, lines: unit.lines(), identity: identity(&[&unit.name, &compact(source)]), - detail: Detail::Function { blocks, locate }, - recheck, + detail: Detail::Function { + blocks, + locate: locate.map(Into::into), + }, + recheck: recheck.map(Into::into), }); if presence == Presence::Judged { judged.push(Item { diff --git a/src/units/graphql.rs b/src/units/graphql.rs new file mode 100644 index 0000000..0b0dcdb --- /dev/null +++ b/src/units/graphql.rs @@ -0,0 +1,58 @@ +//! What a Python GraphQL schema's resolvers receive, sent beside the file's +//! path and language like the web framework roles: a graphene resolver's +//! arguments read as parameters any caller could pass, so DVGA's SSRF, +//! command and SQL injections through `resolve_*` and `mutate` arguments +//! were considers "if a caller passes outside input" instead of reviews. +use std::path::Path; + +const RESOLVERS: &str = "GraphQL server code: its resolvers (graphene `resolve_*` methods and `mutate`, strawberry fields and mutations, ariadne `@query.field` and `@mutation.field` functions) receive the arguments of a client's query or mutation, so those arguments are client input, and `info.context` holds the request."; + +/// The GraphQL facts of a Python file that imports a GraphQL server +/// library, or none. +pub(super) fn describe(path: &Path, source: &str) -> Option<&'static str> { + if path.extension().and_then(|e| e.to_str()) != Some("py") { + return None; + } + source + .lines() + .map(str::trim_start) + .any(|line| { + ["graphene", "strawberry", "ariadne"].iter().any(|library| { + line.starts_with(&format!("import {library}")) + || line.starts_with(&format!("from {library}")) + }) + }) + .then_some(RESOLVERS) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn python_files_importing_a_graphql_server_library_have_resolvers() { + let described = |path: &str, source: &str| describe(Path::new(path), source); + assert_eq!( + described( + "core/views.py", + "import graphene\n\nclass Query(graphene.ObjectType):\n pass\n" + ), + Some(RESOLVERS) + ); + assert!( + described( + "api/schema.py", + "from strawberry.fastapi import GraphQLRouter\n" + ) + .is_some() + ); + assert!( + described( + "core/views.py", + "import requests\n# uses graphene elsewhere\n" + ) + .is_none() + ); + assert!(described("schema.js", "import graphene\n").is_none()); + } +} diff --git a/src/units/handlers/mod.rs b/src/units/handlers/mod.rs index 6914061..a5958c2 100644 --- a/src/units/handlers/mod.rs +++ b/src/units/handlers/mod.rs @@ -16,7 +16,7 @@ use super::{ plan::Scope, questions, }; use crate::{ - analysis::{imports::Imports, units::Unit}, + analysis::{imports::Links, units::Unit}, catalog::SENSITIVE_DATA, options::CheckArgs, schema::Pass, @@ -30,7 +30,7 @@ use std::{collections::BTreeMap, path::PathBuf}; /// What handler lookups need from the whole scope. pub(super) struct Evidence<'a> { - pub imports: &'a BTreeMap, + pub links: &'a Links, pub hashes: &'a BTreeMap, } @@ -42,7 +42,7 @@ pub(super) fn plan( budget: &TokenBudget, result: &mut Plan, ) { - let handlers = error_handlers(scope, evidence.imports); + let handlers = error_handlers(scope, evidence.links); let classes = error_classes(scope, evidence.hashes); for handler in &handlers { let input = &scope.inputs[handler.owner]; @@ -75,16 +75,16 @@ pub(super) fn plan( } /// Error handlers registered in application code outside tests, once each. -fn error_handlers(scope: &Scope<'_>, imports: &BTreeMap) -> Vec { +fn error_handlers(scope: &Scope<'_>, links: &Links) -> Vec { let mut found: Vec = Vec::new(); for &owner in &scope.owners { if !scope.views[&owner].application { continue; } - let file = registered(scope, imports, owner) + let file = registered(scope, links, owner) .into_iter() .chain(decorated(scope, owner)) - .chain(django_views(scope, imports, owner)) + .chain(django_views(scope, links, owner)) .chain(implemented(scope, owner)); for handler in file { if !found @@ -143,7 +143,7 @@ fn handler_helpers(scope: &Scope<'_>, handler: &Handler) -> Vec { /// module, so a unique name is enough. fn named_handler( scope: &Scope<'_>, - imports: &BTreeMap, + links: &Links, owner: usize, name: &str, ) -> Option<(usize, String, String, (usize, usize))> { @@ -165,11 +165,7 @@ fn named_handler( .iter() .find(|(o, _)| *o == owner) .or_else(|| (definitions.len() == 1).then(|| &definitions[0])) - .or_else(|| { - definitions - .iter() - .find(|(o, _)| imports[&owner].reach(&scope.inputs[*o].result.path)) - })?; + .or_else(|| definitions.iter().find(|(o, _)| links.reach(owner, *o)))?; let source = scope.inputs[*found].source.as_deref().unwrap_or(""); Some(( *found, diff --git a/src/units/handlers/registered.rs b/src/units/handlers/registered.rs index 440566a..941799e 100644 --- a/src/units/handlers/registered.rs +++ b/src/units/handlers/registered.rs @@ -4,8 +4,7 @@ //! Python function under an error-handler decorator, and the views a Django //! URLconf or Django REST framework's `EXCEPTION_HANDLER` names. use super::{Handler, Scope, named_handler}; -use crate::analysis::imports::Imports; -use std::collections::BTreeMap; +use crate::analysis::imports::Links; /// Calls that register a web framework's error handler, by the method that /// takes it; the handler is the function named or written in the call. @@ -26,11 +25,7 @@ const HANDLER_DECORATORS: [&str; 2] = [".exception_handler(", ".errorhandler("]; /// Handlers one file passes to a registration call: a function named there, /// or the function written inside the call. -pub(super) fn registered( - scope: &Scope<'_>, - imports: &BTreeMap, - owner: usize, -) -> Vec { +pub(super) fn registered(scope: &Scope<'_>, links: &Links, owner: usize) -> Vec { let input = &scope.inputs[owner]; let source = input.source.as_deref().unwrap_or(""); let lines = scope.test_lines(owner); @@ -51,7 +46,7 @@ pub(super) fn registered( { continue; } - found.extend(registration(scope, imports, owner, needle, at)); + found.extend(registration(scope, links, owner, needle, at)); } } found @@ -62,7 +57,7 @@ pub(super) fn registered( /// counts only with the error-middleware parameter count. fn registration( scope: &Scope<'_>, - imports: &BTreeMap, + links: &Links, owner: usize, needle: &str, at: usize, @@ -90,7 +85,7 @@ fn registration( return None; } let (owner, name, source, lines) = if named { - named_handler(scope, imports, owner, quoted)? + named_handler(scope, links, owner, quoted)? } else { let first = crate::analysis::line_of(source, open); let last = crate::analysis::line_of(source, open + argument.len()); @@ -165,11 +160,7 @@ const DRF_EXCEPTION_HANDLER: &str = "EXCEPTION_HANDLER"; /// Views a Django URLconf names for errors (`handler500 = views.server_error` /// or a dotted path in a string), and the function Django REST framework's /// `EXCEPTION_HANDLER` setting names, found by their last name segment. -pub(super) fn django_views( - scope: &Scope<'_>, - imports: &BTreeMap, - owner: usize, -) -> Vec { +pub(super) fn django_views(scope: &Scope<'_>, links: &Links, owner: usize) -> Vec { let input = &scope.inputs[owner]; if input.result.path.extension().is_none_or(|e| e != "py") { return Vec::new(); @@ -214,7 +205,7 @@ pub(super) fn django_views( continue; } let Some((handler_owner, handler_name, handler_source, lines)) = - named_handler(scope, imports, owner, name) + named_handler(scope, links, owner, name) else { continue; }; diff --git a/src/units/hardcoded.rs b/src/units/hardcoded.rs index 7d9ae11..b204c9e 100644 --- a/src/units/hardcoded.rs +++ b/src/units/hardcoded.rs @@ -52,11 +52,16 @@ pub(super) fn plan( .then(|| locate(file, &unit.name, source, &id, &choices)); Detail::Values { values: unit.literals.iter().map(|l| l.text.clone()).collect(), + repeated: choices + .iter() + .map(|c| occurrences(file.source, c) != 1) + .collect(), choices, - locate, + locate: locate.map(Into::into), } }, - recheck: benign_request(file, &id, json!({"functions": [state.clone()]}), true), + recheck: benign_request(file, &id, json!({"functions": [state.clone()]}), true) + .map(Into::into), }); items.push((out.units.len() - 1, id, state)); } @@ -77,6 +82,26 @@ pub(super) fn plan( /// Most distinct values a locate Choice offers; a unit with more is not located. const LOCATE_CHOICES: usize = 24; +/// How often a literal is written in `source`: a number as a whole token (not +/// part of `100` or `10.5` for `10`), other text wherever it appears without +/// its quotes. +fn occurrences(source: &str, literal: &str) -> usize { + let text = literal.trim_matches(['"', '\'', '`']); + if text.is_empty() { + return 0; + } + let number = text.starts_with(|c: char| c.is_ascii_digit() || c == '-' || c == '.'); + let word = |c: char| c.is_alphanumeric() || c == '_' || c == '.'; + source + .match_indices(text) + .filter(|(at, _)| { + !number + || !(source[..*at].chars().next_back().is_some_and(word) + || source[at + text.len()..].chars().next().is_some_and(word)) + }) + .count() +} + /// Which value a finding is about: the function's source and its distinct values. fn locate( file: &FileContext<'_>, @@ -257,9 +282,9 @@ fn plan_constants( identity: identity(&names), detail: Detail::Constants { values: constants.iter().flat_map(|c| c.values.clone()).collect(), - locate, + locate: locate.map(Into::into), }, - recheck: recheck.filter(|_| fits), + recheck: recheck.filter(|_| fits).map(Into::into), }); if fits { requests.push(Planned { diff --git a/src/units/instructions.rs b/src/units/instructions.rs index e7723cd..2b452db 100644 --- a/src/units/instructions.rs +++ b/src/units/instructions.rs @@ -74,7 +74,7 @@ pub(super) fn plan( let item = (*index, id.clone(), state.clone()); let (request, asked) = kind_request(file, &evidence, &item); if file.budget.fits(&request) { - out.units[*index].recheck = Some((request, asked)); + out.units[*index].recheck = Some((request, asked).into()); } } // Runs end after headings, never after a unit's name, which names a diff --git a/src/units/mod.rs b/src/units/mod.rs index f448c36..a4e9ef3 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -3,6 +3,7 @@ //! Jev answers short literal questions, and `compose` turns answers into results. mod access; mod answers; +mod client_app; mod comments; pub mod compose; mod documents; @@ -11,6 +12,7 @@ mod duplicates; mod evidence; mod follow_ups; mod functions; +mod graphql; pub mod grouping; mod handlers; mod hardcoded; @@ -79,7 +81,42 @@ pub struct Settle { /// The Choice's question id, which `security::SETTLES` maps to the checks /// it settles and the options that clear them. pub question: &'static str, - pub request: (Value, Asked), + pub request: FollowUp, +} + +/// A follow-up request, kept as its JSON text until it is asked: most are +/// never sent, and held as JSON values, the traces, rechecks and settles of +/// laravel/framework's security units took about 2 GB while planning. +/// `serde_json` reads the text back to the same value, so the request, and +/// the answer cache it keys, do not change. +#[derive(Clone, Debug)] +pub struct FollowUp { + request: Box, + pub asked: Asked, +} + +impl FollowUp { + pub fn request(&self) -> Value { + serde_json::from_str(&self.request).expect("a follow-up is JSON it wrote itself") + } + + /// The request, planned for the file at `owner`. + pub fn planned(&self, owner: usize) -> Planned { + Planned { + owner, + request: self.request(), + asked: self.asked.clone(), + } + } +} + +impl From<(Value, Asked)> for FollowUp { + fn from((request, asked): (Value, Asked)) -> Self { + Self { + request: request.to_string().into_boxed_str(), + asked, + } + } } #[derive(Clone, Debug)] @@ -88,7 +125,7 @@ pub enum Detail { blocks: Vec, /// The follow-up that asks which block to extract, sent only after the /// split question raises a review or consider. - locate: Option<(Value, Asked)>, + locate: Option, }, Outline { /// A test file's cases rather than application members. @@ -97,7 +134,7 @@ pub enum Detail { /// 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)>, + kind: Option, }, Pair { differences: Vec, @@ -115,7 +152,9 @@ pub enum Detail { /// Its distinct values, whose ids `v0`, `v1`, ... the locate follow-up /// chooses among; that follow-up is sent only after a review or consider. choices: Vec, - locate: Option<(Value, Asked)>, + /// Whether each choice's text is not written exactly once in the file. + repeated: Vec, + locate: Option, }, /// A comment of application code and the unit it documents or sits in. Comment { @@ -126,14 +165,14 @@ pub enum Detail { /// tool may render even when it repeats the signature. documentation: bool, /// What kind of comment it is, asked when its questions stay undecided. - kind: Option<(Value, Asked)>, + kind: Option, }, /// A file's module-level constants and the literal values they hold. Constants { values: Vec, /// Which constant a review or consider is about, asked after it; /// its options are the unit's locations, one per constant, in order. - locate: Option<(Value, Asked)>, + locate: Option, }, /// A security unit: its statements as sites for locating a finding, and /// the trace follow-up sent when presence is not clear. @@ -142,7 +181,7 @@ pub enum Detail { /// The message argument of each error it creates, by position (`m0`…), /// for sensitive-data units. messages: Vec, - trace: Option<(Value, Asked)>, + trace: Option, /// One Choice per kind of check that can stay undecided after the /// trace and recheck, such as where its URLs come from or its output /// goes; each is asked only while its checks are undecided. @@ -150,36 +189,36 @@ pub enum Detail { /// Django code, asked the Django checks: a weak setting must be /// named by one of them. django: bool, + /// Code at a test path, such as a test app's settings or models. + test_path: bool, }, /// A large document judged by its outline, with its top-level parts /// and the follow-up that locates a split. Document { parts: Vec, - locate: Option<(Value, Asked)>, + locate: Option, /// What kind of document it is, asked when the split stays undecided. - kind: Option<(Value, Asked)>, + kind: Option, }, /// A document whose release is tagged or whose named paths were deleted: /// the facts a finished plan finding cites. - Plan { - facts: Vec, - }, + Plan { facts: Vec }, /// A section naming paths or scripts the repository lacks, and the check /// sent unless its document is a finished plan. Stale { missing: Vec, - check: Option<(Value, Asked)>, + check: Option, /// What the section treats the missing names as, asked when the /// check stays undecided. - settle: Option<(Value, Asked)>, + settle: Option, }, /// A candidate pair of sections, the other in `other`, and the check /// sent unless either document is a finished plan. DocPair { other: Location, - check: Option<(Value, Asked)>, + check: Option, /// How the two sections relate, asked when the check stays undecided. - settle: Option<(Value, Asked)>, + settle: Option, }, /// A heading section of an agent instruction file. Section { @@ -189,16 +228,16 @@ pub enum Detail { loaded: String, }, /// A web framework's error handler and how the program registers it. - Handler { - registered: String, - }, + Handler { registered: String }, /// A policy, SECURITY DEFINER function or grant in its final state. Access(Access), /// A workflow job and the expressions its `run` scripts hold. - Job { - expressions: Vec, + Job { expressions: Vec }, + Test { + /// What its assertions read, asked with the code under test after its + /// first answer says it asserts internal details. + confirm: Option, }, - Test, TestPair { names: [String; 2], subject: String, @@ -212,7 +251,7 @@ pub enum Detail { unseen_setup: bool, /// Outside Ruby, whether each test checks something the other does /// not, asked only of a pair that reached a review. - confirm: Option<(Value, Asked)>, + confirm: Option, }, } @@ -245,7 +284,7 @@ pub struct UnitPlan { /// Identity for the finding fingerprint: survives moves and unrelated edits. pub identity: String, pub detail: Detail, - pub recheck: Option<(Value, Asked)>, + pub recheck: Option, } #[derive(Clone, Debug, Default)] diff --git a/src/units/outcome/comments.rs b/src/units/outcome/comments.rs index 933a3f3..7c3cb7e 100644 --- a/src/units/outcome/comments.rs +++ b/src/units/outcome/comments.rs @@ -34,8 +34,12 @@ pub(in crate::units) fn comment_signals<'a>( /// 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 -/// declaration), the others reaching it clear it. Comments are cleanups, -/// never defects: at most a consider. +/// declaration), the others reaching it clear it. The kind is the last ask, +/// so a kind that only leans decides too: toward a kind a reader could do +/// without, a note, else clear. Step headings such as `// update any single +/// tag` above `this.addTag()` stayed between the thresholds on every ask: +/// 1,153 comments on the corpus, each leaving its file uncertain. Comments +/// are cleanups, never defects: at most a consider. pub(in crate::units) fn comment_outcome<'a>( get: &impl Fn(&str) -> Option<&'a Answer>, documentation: bool, @@ -48,7 +52,10 @@ pub(in crate::units) fn comment_outcome<'a>( Outcome::Note(p) } (Outcome::Uncertain(_), Some((_, p))) if at_least(p) => Outcome::Consider(p), - (Outcome::Uncertain(_), Some((_, p))) if at_least(1.0 - p) => Outcome::Clear, + (Outcome::Uncertain(_), Some((_, p))) if probability_at_least(p, LEADING_PROBABILITY) => { + Outcome::Note(p) + } + (Outcome::Uncertain(_), Some(_)) => Outcome::Clear, _ => outcome, })) } diff --git a/src/units/outcome/documentation.rs b/src/units/outcome/documentation.rs index 549f563..785f883 100644 --- a/src/units/outcome/documentation.rs +++ b/src/units/outcome/documentation.rs @@ -48,12 +48,16 @@ pub(in crate::units) fn document_outcome<'a>( Some(strongest(&[split, past])) } -/// The split Score, or when it stays undecided, the kind of document: the -/// kinds that serve one subject reaching the threshold clear it, a -/// collection of unrelated subjects reaching it raises a consider. +/// The split Score, weighed with the kind of document once it is asked: the +/// kinds that serve one subject reaching the threshold clear an undecided +/// split or a finding, and a collection of unrelated subjects reaching it +/// raises an undecided split to a consider. pub(in crate::units) fn document_split(split: &Answer, kind: Option<&Answer>) -> Outcome { let outcome = benefit(split); - let (Outcome::Uncertain(_), Some(Answer::Choice { probabilities, .. })) = (outcome, kind) + let ( + Outcome::Uncertain(_) | Outcome::Consider(_) | Outcome::Review(_), + Some(Answer::Choice { probabilities, .. }), + ) = (outcome, kind) else { return outcome; }; @@ -70,7 +74,7 @@ pub(in crate::units) fn document_split(split: &Answer, kind: Option<&Answer>) -> .sum(); if at_least(1.0 - several) { Outcome::Clear - } else if at_least(several) { + } else if at_least(several) && matches!(outcome, Outcome::Uncertain(_)) { Outcome::Consider(several) } else { outcome diff --git a/src/units/outcome/exposure.rs b/src/units/outcome/exposure.rs index 129dc04..a7d06d5 100644 --- a/src/units/outcome/exposure.rs +++ b/src/units/outcome/exposure.rs @@ -79,7 +79,9 @@ pub(in crate::units) fn exposure_outcome<'a>( /// The presence answers of `questions` and the rule's specific checks, each /// judged with its lean; an undecided check its settle Choice clears is -/// clear. None until every presence question is answered. +/// clear, and so is one that found a concern a Choice asked whenever the +/// check is not clear rules out. None until every presence question is +/// answered. fn exposure_signals<'a>( rule: &str, get: &impl Fn(&str) -> Option<&'a Answer>, @@ -90,9 +92,18 @@ fn exposure_signals<'a>( .flatten(); let away = rule == catalog::SENSITIVE_DATA && away_from_clients(get); let judge = |question: &str, answer: &Answer| exposure_signal(question, answer, own, away); + // A Choice asked whenever a presence signal is not clear rules it out + // too: what a function's logs write clears an audit line that names who + // signed in, which the presence question found as personal data. let presence: Vec = questions .iter() - .map(|q| get(q).map(|a| judge(q, a))) + .map(|q| { + get(q).map(|a| match judge(q, a) { + (Outcome::Clear, lean) => (Outcome::Clear, lean), + _ if settled(rule, q, get, true) => (Outcome::Clear, 0.0), + signal => signal, + }) + }) .collect::>()?; let specific: Vec = crate::units::security::checks(rule) .iter() @@ -102,6 +113,12 @@ fn exposure_signals<'a>( Outcome::Uncertain(_) if settled(rule, check.id, get, false) => { (Outcome::Clear, 0.0) } + // A Choice asked whenever its check is not clear clears a + // concern it rules out, such as HMAC signing read as a + // password hash. + Outcome::Review(_) | Outcome::Consider(_) if settled(rule, check.id, get, true) => { + (Outcome::Clear, 0.0) + } _ => (outcome, lean), }) }) diff --git a/src/units/outcome/test_rules.rs b/src/units/outcome/test_rules.rs index 61ac44a..71bffe4 100644 --- a/src/units/outcome/test_rules.rs +++ b/src/units/outcome/test_rules.rs @@ -23,7 +23,7 @@ pub(in crate::units) fn test_value_outcome<'a>( Some(if let Some(p) = strongest(&hollow) { Outcome::Review(p) } else if let Some(p) = strongest(&weak) { - Outcome::Consider(p) + internal_outcome(p, get("reads")) } else if let Outcome::Review(p) = several { Outcome::Note(p) } else if hollow.iter().all(|o| *o == Outcome::Clear) { @@ -33,6 +33,32 @@ pub(in crate::units) fn test_value_outcome<'a>( }) } +/// An internal-details consider, weighed with what the test's assertions +/// read once that is asked: results, state or effects a caller observes at +/// the threshold clear it, stored input or the program's own calls leading +/// keep it, and otherwise it is a note. +fn internal_outcome(p: f64, reads: Option<&Answer>) -> Outcome { + let Some(Answer::Choice { probabilities, .. }) = reads else { + return Outcome::Consider(p); + }; + let mass: f64 = probabilities.values().sum(); + if mass <= 0.0 { + return Outcome::Consider(p); + } + let observed: f64 = probabilities + .iter() + .filter(|(kind, _)| questions::OBSERVED_READS.contains(&kind.as_str())) + .map(|(_, q)| q / mass) + .sum(); + if at_least(observed) { + Outcome::Clear + } else if probability_at_least(1.0 - observed, LEADING_PROBABILITY) { + Outcome::Consider(p) + } else { + Outcome::Note(p) + } +} + /// Two tests that check the same behavior with equivalent inputs make a /// review: one of them adds nothing. A review also needs both tests to /// exercise the same input case and expect the same outcome, each at the diff --git a/src/units/outline.rs b/src/units/outline.rs index c030985..6d35d3f 100644 --- a/src/units/outline.rs +++ b/src/units/outline.rs @@ -174,7 +174,8 @@ fn plan_outline( .into_iter() .filter(|_| judged) .map(|source| outline.request(file, Ask::Kind(source))) - .find(|(request, _)| file.budget.fits(request)), + .find(|(request, _)| file.budget.fits(request)) + .map(Into::into), groups: ids .into_iter() .zip(&sets) @@ -193,7 +194,8 @@ fn plan_outline( }, recheck: judged .then(|| outline.request(file, Ask::Recheck(source.clone()))) - .filter(|(request, _)| file.budget.fits(request)), + .filter(|(request, _)| file.budget.fits(request)) + .map(Into::into), }); if judged { requests.push(Planned { diff --git a/src/units/plan/file.rs b/src/units/plan/file.rs index c51d214..18a7788 100644 --- a/src/units/plan/file.rs +++ b/src/units/plan/file.rs @@ -3,7 +3,7 @@ use super::{Scope, Shared, plan_security}; use crate::{ analysis::{ - imports::Imports, + imports::Links, test_map::{self, TestCase}, units::{FileUnits, Unit}, }, @@ -125,12 +125,26 @@ fn file_context<'a>( source_hash: &input.result.source_hash, model: args.model(), budget, - framework: crate::units::nextjs::describe( - &input.result.path, - input.source.as_deref().unwrap_or(""), - input.package.as_ref(), - ) - .or_else(|| crate::units::sveltekit::describe(&input.result.path, input.package.as_ref())), + framework: crate::components::server_template(&input.result.path) + .then(|| crate::components::TEMPLATE_SCRIPT.to_string()) + .or_else(|| { + crate::units::nextjs::describe( + &input.result.path, + input.source.as_deref().unwrap_or(""), + input.package.as_ref(), + ) + }) + .or_else(|| { + crate::units::sveltekit::describe(&input.result.path, input.package.as_ref()) + }) + .or_else(|| { + crate::units::graphql::describe( + &input.result.path, + input.source.as_deref().unwrap_or(""), + ) + .or_else(|| crate::units::client_app::describe(input.package.as_ref())) + .map(str::to_string) + }), } } @@ -152,7 +166,7 @@ fn plan_outline( .collect(); file.rules.insert(catalog::FILE_ORGANIZATION, 0); if members.len() >= 2 { - let callers = callers(scope, &shared.imports, owner); + let callers = callers(scope, &shared.links, owner); let parsed = &scope.units[&owner]; outline::plan(context, parsed, &members, &callers, file, requests); } @@ -328,17 +342,9 @@ fn plan_tests( } /// Short callee name to the other selected files that import `target` and call it. -fn callers( - scope: &Scope<'_>, - imports: &BTreeMap, - target: usize, -) -> BTreeMap> { - let path = &scope.inputs[target].result.path; +fn callers(scope: &Scope<'_>, links: &Links, target: usize) -> BTreeMap> { let mut callers = BTreeMap::>::new(); - for &owner in &scope.owners { - if owner == target || !imports[&owner].reach(path) { - continue; - } + for &owner in links.importers(target).iter() { // Tests exercise a group; only application code makes it a dependency. let tests = scope.test_lines(owner); let units = scope.units[&owner].units.iter(); diff --git a/src/units/plan/mod.rs b/src/units/plan/mod.rs index 3995e9a..7a25f7c 100644 --- a/src/units/plan/mod.rs +++ b/src/units/plan/mod.rs @@ -99,7 +99,7 @@ pub fn plan( } if shared.enabled(catalog::SENSITIVE_DATA) { let evidence = handlers::Evidence { - imports: &shared.imports, + links: &shared.links, hashes: &shared.hashes, }; handlers::plan(&scope, &evidence, args, budget, &mut result); diff --git a/src/units/plan/security_units.rs b/src/units/plan/security_units.rs index 5bd2737..5afb45d 100644 --- a/src/units/plan/security_units.rs +++ b/src/units/plan/security_units.rs @@ -9,13 +9,13 @@ use super::{ }; use crate::{ analysis::{ - imports::Imports, + imports::Links, units::{FileUnits, Unit}, }, catalog, units::{FileContext, FilePlan, Planned, security}, }; -use std::{collections::BTreeMap, ops::Range}; +use std::ops::Range; /// Application functions outside tests and the file's setup statements; the /// injection recheck shows up to three callers of each function. @@ -32,33 +32,13 @@ pub(super) fn plan_security( file.rules.insert(rule, 0); } let parsed = &scope.units[&context.owner]; + let test_path = scope.inputs[context.owner].result.role == "test"; let outside_tests = |line: usize| !lines.iter().any(|l| l.contains(&line)); let subjects: Vec> = parsed .units .iter() .filter(|u| u.callable() && outside_tests(u.line)) - .map(|unit| { - let callers = if rules.contains(&catalog::INJECTION) { - callers_of(scope, &shared.imports, context.owner, unit) - } else { - Vec::new() - }; - let mut subject = security::function_subject( - context, - unit, - callers, - &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); - } - subject - }) + .map(|unit| function_subject(scope, shared, context, unit, rules)) .collect(); let mut setup = security::setup_subject(context, &parsed.setup, &shared.constants) .filter(|_| parsed.setup.statements.iter().all(|s| outside_tests(s.1))); @@ -67,6 +47,7 @@ pub(super) fn plan_security( // settings module, keeps the common questions. if let Some(setup) = setup.as_mut() { setup.django = parsed.setup.settings; + setup.test_path = test_path; } if let Some(setup) = setup.as_mut().filter(|_| parsed.setup.settings) { let selected = selections(&scope.inputs[context.owner].settings_selected_by); @@ -93,6 +74,56 @@ pub(super) fn plan_security( file, requests, ); + // A server template's code that reads client data: a JSP page's + // scriptlets are judged like a PHP page script, by every rule; other + // templates' code is tags that write a value unescaped, which injection + // judges by where the value comes from. Asked whether they turn off + // escaping, each `raw` or `html_safe` tag of RailsGoat's views said yes, + // even around a user's numeric id. + if let Some(code) = security::template_subject(context, &parsed.template_code) { + let jsp = matches!( + context.path.extension().and_then(|e| e.to_str()), + Some("jsp" | "jspf") + ); + let judged: Vec<&'static str> = rules + .iter() + .copied() + .filter(|rule| jsp || *rule == catalog::INJECTION) + .collect(); + security::plan(context, &[code], None, &judged, false, file, requests); + } +} + +/// A function with the evidence its enabled rules need: callers for +/// injection, the errors its callees create for sensitive data, Django's +/// facts, and the templates it renders. +fn function_subject<'a>( + scope: &'a Scope<'_>, + shared: &'a Shared<'_>, + context: &FileContext<'_>, + unit: &'a Unit, + rules: &[&'static str], +) -> security::Subject<'a> { + let parsed = &scope.units[&context.owner]; + let callers = if rules.contains(&catalog::INJECTION) { + callers_of(scope, &shared.links, context.owner, unit) + } else { + Vec::new() + }; + let mut subject = + security::function_subject(context, unit, callers, &shared.enums, &shared.constants); + if rules.contains(&catalog::SENSITIVE_DATA) { + subject.callee_errors = callee_errors(scope, &shared.links, context.owner, unit); + } + subject.django = parsed.django; + subject.test_path = scope.inputs[context.owner].result.role == "test"; + if parsed.django { + django_evidence(scope, context, parsed, unit, &mut subject); + } + if rules.contains(&catalog::INJECTION) { + rendered_templates(scope, context, &mut subject); + } + subject } /// Module constants shown with one function, at most. @@ -108,7 +139,6 @@ fn django_evidence( context: &FileContext<'_>, parsed: &FileUnits, unit: &Unit, - rules: &[&'static str], subject: &mut security::Subject<'_>, ) { if let Some(command) = crate::analysis::django::management_command(context.path) { @@ -133,6 +163,16 @@ fn django_evidence( serde_json::json!(routes), ); } +} + +/// The templates a function renders by name that write values without +/// escaping, with those lines: the markup a Django view's or a Node +/// handler's values reach is written there, not in the function. +fn rendered_templates( + scope: &Scope<'_>, + context: &FileContext<'_>, + subject: &mut security::Subject<'_>, +) { let templates: Vec = crate::analysis::django::rendered( subject.source.as_str(), &scope.inputs[context.owner].templates, @@ -146,11 +186,10 @@ fn django_evidence( }) }) .collect(); - if rules.contains(&catalog::INJECTION) && !templates.is_empty() { - subject.evidence.insert( - "templates_it_renders_that_write_values_without_escaping".into(), - serde_json::json!(templates), - ); + if !templates.is_empty() { + subject + .evidence + .insert(security::RENDERED.into(), serde_json::json!(templates)); } } @@ -219,7 +258,7 @@ const CALLEE_ERRORS: usize = 8; /// library's: twelve such handlers of one FastAPI project were reviews. fn callee_errors( scope: &Scope<'_>, - imports: &BTreeMap, + links: &Links, owner: usize, unit: &Unit, ) -> Vec { @@ -229,7 +268,7 @@ fn callee_errors( for _ in 0..2 { let mut next = Vec::new(); for (file, caller) in callers { - for (other, callee) in callees(scope, imports, file, caller) { + for (other, callee) in callees(scope, links, file, caller) { if visited.contains(&callee.name) { continue; } @@ -254,16 +293,12 @@ fn callee_errors( /// imports, with the file each is in. fn callees<'s>( scope: &'s Scope<'_>, - imports: &BTreeMap, + links: &Links, 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 { + for &other in links.reachable_from(file).iter() { let lines = scope.test_lines(other); found.extend( scope.units[&other] @@ -282,17 +317,13 @@ fn callees<'s>( fn callers_of( scope: &Scope<'_>, - imports: &BTreeMap, + links: &Links, owner: usize, unit: &Unit, ) -> Vec<(String, String)> { - let path = &scope.inputs[owner].result.path; - let others = scope.owners.iter().filter(|&&o| o != owner); + let importers = links.importers(owner); let mut found = Vec::new(); - for &other in std::iter::once(&owner).chain(others) { - if other != owner && !imports[&other].reach(path) { - continue; - } + for &other in std::iter::once(&owner).chain(importers.iter()) { let source = scope.inputs[other].source.as_deref().unwrap_or(""); let lines = scope.test_lines(other); for caller in &scope.units[&other].units { diff --git a/src/units/plan/shared.rs b/src/units/plan/shared.rs index 9d81856..a09e819 100644 --- a/src/units/plan/shared.rs +++ b/src/units/plan/shared.rs @@ -7,7 +7,7 @@ use super::{ use crate::{ analysis::{ clones::{self, SourceFile}, - imports::Imports, + imports::Links, routes::Route, test_map::{self, TestCase}, units::Unit, @@ -21,11 +21,11 @@ use std::{ path::{Path, PathBuf}, }; -/// Facts that span files: clone groups, imports, callable subjects and hashes. +/// Facts that span files: clone groups, links, callable subjects and hashes. pub(super) struct Shared<'a> { pub(super) rules: &'a [String], pub(super) pairs: clones::Candidates, - pub(super) imports: BTreeMap, + pub(super) links: Links, /// Callable short names to their signatures, for test subjects. pub(super) subjects: BTreeMap, /// Java method short names to the Java types that own a method of that name. @@ -102,7 +102,7 @@ impl<'a> Shared<'a> { let mut shared = Self { rules: &args.rules, pairs: clones::Candidates::default(), - imports: imports(scope), + links: links(scope), subjects: BTreeMap::new(), subject_owners: BTreeMap::new(), subject_sources: BTreeMap::new(), @@ -311,15 +311,11 @@ fn test_cases(scope: &Scope<'_>) -> BTreeMap> { .collect() } -/// Import lines of every selected file, for caller lookups. -fn imports(scope: &Scope<'_>) -> BTreeMap { - scope - .owners - .iter() - .map(|&owner| { - let input = &scope.inputs[owner]; - let source = input.source.as_deref().unwrap_or(""); - (owner, Imports::new(&input.result.path, source)) - }) - .collect() +/// Which selected files import which, for caller lookups. +fn links(scope: &Scope<'_>) -> Links { + Links::new(scope.owners.iter().map(|&owner| { + let input = &scope.inputs[owner]; + let source = input.source.as_deref().unwrap_or(""); + (owner, input.result.path.as_path(), source) + })) } diff --git a/src/units/questions/documentation/documents.rs b/src/units/questions/documentation/documents.rs index 4104836..2459711 100644 --- a/src/units/questions/documentation/documents.rs +++ b/src/units/questions/documentation/documents.rs @@ -37,11 +37,14 @@ pub fn document_history() -> Value { /// Kinds of documents that hold several subjects; the others serve one. pub const SEVERAL_DOCUMENT_KINDS: [&str; 1] = ["collection"]; -/// What a large document is, asked when its split Score stays undecided: -/// on long guides, references and migration guides the split stayed near a -/// third per level, while naming the kind of document is decisive. +/// What a large document is, asked when its split Score stays undecided or +/// raises a finding: on long guides, references and migration guides the +/// split stayed near a third per level, while naming the kind of document +/// is decisive. Read from headings alone, a plan for one release, a README +/// and a list of business rules held "several unrelated subjects" in every +/// labeled split finding. pub fn document_kind() -> Value { - let kinds: [(&str, &str); 5] = [ + let kinds: [(&str, &str); 7] = [ ( "guide", "One guide, tutorial or quickstart that walks a reader through one product, tool or task, even across many steps or topics.", @@ -58,6 +61,14 @@ pub fn document_kind() -> Value { "introduction", "An introduction to one project, package or example: what it is, how to install, configure and use it, and where to learn more.", ), + ( + "plan", + "A plan, design or proposal for one change, feature or release, with a section for each part of the work, even when the parts touch different areas.", + ), + ( + "requirements", + "Requirements, rules or a specification of one product or feature, with a section for each requirement or rule.", + ), ( "collection", "Several unrelated subjects with different readers or purposes, such as deployment, onboarding and API rules in one file.", diff --git a/src/units/questions/maintainability.rs b/src/units/questions/maintainability.rs index f25ca55..14b4a95 100644 --- a/src/units/questions/maintainability.rs +++ b/src/units/questions/maintainability.rs @@ -108,8 +108,11 @@ pub fn outline_module(tests: bool, groups: &[String]) -> Value { /// Kinds of files whose members serve several features; every other kind /// serves one. Asked with the recheck: when the split Score stays undecided, /// the kind decides, since naming what a file holds was decisive where -/// weighing a split was not. -pub const SEVERAL_KINDS: [&str; 2] = ["per_feature", "several"]; +/// weighing a split was not. The same kind of code written out per feature +/// is one job: the three such files a consider named, a mailer's function +/// per template, a game's admin reducers per kind of map content and +/// JevGate's own follow-up questions per security check, read well whole. +pub const SEVERAL_KINDS: [&str; 1] = ["several"]; /// What the members of a file hold, as a Choice among kinds of files. pub fn outline_kind(tests: bool) -> Value { diff --git a/src/units/questions/mod.rs b/src/units/questions/mod.rs index 4966ed2..7f061c8 100644 --- a/src/units/questions/mod.rs +++ b/src/units/questions/mod.rs @@ -16,6 +16,7 @@ mod maintainability; mod php; mod privilege; mod security; +mod settle; mod spacetimedb; mod test_rules; pub use comments::*; @@ -27,6 +28,7 @@ pub use maintainability::*; pub use php::*; pub use privilege::*; pub use security::*; +pub use settle::*; pub use spacetimedb::*; pub use test_rules::*; @@ -141,6 +143,8 @@ mod tests { test_mock_only("tests[0].source", TestEvidence::Recheck), test_mock_only("tests[0].source", TestEvidence::RecheckGroups), test_several("tests[0].source"), + test_reads("tests[0].source", TestEvidence::Recheck), + test_reads("tests[0].source", TestEvidence::RecheckGroups), test_pair_overlap(false), test_pair_overlap(true), test_pair_distinct(), @@ -162,8 +166,9 @@ mod tests { security_url_parts("function.source", true), security_redirect_target("function.source", false), security_redirect_target("function.source", true), - security_markup_output("function.source", false), - security_markup_output("function.source", true), + security_markup_output("function.source", false, false), + security_markup_output("function.source", true, false), + security_markup_output("function.source", false, true), security_markup_parts("function.source", false), security_markup_parts("function.source", true), security_path_parts("function.source"), @@ -183,9 +188,11 @@ mod tests { all.extend(security_checks()); for django in [false, true] { all.extend([ - security_interpreted("function.source", django, None, false), - security_interpreted("function.source", django, Some("pickle"), false), - security_interpreted("function.source", django, Some("pickle"), true), + security_interpreted("function.source", django, false, None, false), + security_interpreted("function.source", django, false, Some("pickle"), false), + security_interpreted("function.source", django, false, Some("pickle"), true), + security_interpreted("function.source", django, true, None, false), + security_interpreted("function.source", django, true, Some("pickle"), false), security_resource("function.source", django), security_error_details("function.source", django), security_weakened("function.source", django), @@ -197,7 +204,7 @@ mod tests { for (id, mut body) in [ ( "interpreted", - security_interpreted("function.source", false, None, false), + security_interpreted("function.source", false, false, None, false), ), ("resource", security_resource("function.source", false)), ( @@ -218,6 +225,7 @@ mod tests { .chain(&WEAK_SETTINGS) .chain(&EXPOSURES) .chain(&DJANGO_VARIANTS) + .chain([&VIEW_MARKUP]) .chain(&DJANGO_UNHANDLED) .chain(&DJANGO_SETTINGS) .chain(&DJANGO_EXPOSURES) diff --git a/src/units/questions/php.rs b/src/units/questions/php.rs index 1581b70..75fa689 100644 --- a/src/units/questions/php.rs +++ b/src/units/questions/php.rs @@ -121,23 +121,23 @@ const WORDING: [Wording; 17] = [ }, Wording { id: "hash", - question: "Does `{code}` hash passwords or derive keys from them with a fast or broken hash, or with few iterations?", - yes: "It hashes passwords or derives keys from them with md5, sha1, crypt with a weak salt, a single round of SHA-256 through hash, or hash_pbkdf2 with few iterations.", - no: "It uses password_hash and password_verify, or hash_pbkdf2 with many iterations, or it does not handle passwords.", + question: "Does `{code}` keep passwords as plain text, or hash them or derive keys from them with a fast or broken hash, or with few iterations?", + yes: "It saves passwords, or checks a login against saved passwords, as plain text, or hashes passwords or derives keys from them with md5, sha1, crypt with a weak salt, a single round of SHA-256 through hash, or hash_pbkdf2 with few iterations.", + no: "It uses password_hash and password_verify, or hash_pbkdf2 with many iterations; it hands passwords to a framework that hashes them, such as WordPress's wp_hash_password or Laravel's Hash; or it does not handle passwords.", no_examples: CALLED, }, Wording { id: "random", question: "Does `{code}` make a token, code, password or identifier that must be unguessable with a non-cryptographic generator or from a predictable value?", yes: "It makes a secret value, such as a session id, token, reset or verification code, or random password, with rand, mt_rand, uniqid or lcg_value, or from a counter, the time or a hash of such values.", - no: "It uses random_bytes, random_int, openssl_random_pseudo_bytes or Laravel's Str::random, which is built on random_bytes, or the value is not a secret.", + no: "It uses random_bytes, random_int, openssl_random_pseudo_bytes, Laravel's Str::random, which is built on random_bytes, or WordPress's wp_rand and wp_generate_password, which are built on random_int, or the value is not a secret.", no_examples: CALLED, }, Wording { id: "cookie", question: "Does `{code}` set or configure a session or authentication cookie without the Secure or HttpOnly flag?", yes: "It sets a cookie that holds a session or token with setcookie or session_set_cookie_params, or starts a session after setting session.cookie_httponly or session.cookie_secure off, without Secure or without HttpOnly.", - no: "Such cookies have both flags, the cookie holds no session or token, or the code sets no cookie.", + no: "Such cookies have both flags, the cookie holds no session or token, it is set empty and already expired to delete it, or the code sets no cookie.", no_examples: CALLED, }, Wording { diff --git a/src/units/questions/privilege.rs b/src/units/questions/privilege.rs index fdadff2..ddcccd8 100644 --- a/src/units/questions/privilege.rs +++ b/src/units/questions/privilege.rs @@ -17,11 +17,14 @@ fn sql_noul(question: &str, yes: &str, no: &str, context: &str) -> Value { /// "Let a user read other users' rows" was read literally and flagged role /// checks, admin policies and restrictive policies; the criteria name them. +/// So was reading rows their owners chose to share: chatbot-ui's ten +/// `using (sharing <> 'private')` policies, the read side of its sharing, +/// were considers. pub fn policy_others() -> Value { sql_noul( "Does the policy in `policy.source` let every user it applies to read or change rows that belong to other users or accounts?", "Its condition admits other people's rows for every user it applies to, such as `using (true)` on private data, a check only that the user is signed in, or a write without a `with check` that ties the row to the user.", - "Its condition ties the rows it admits to the user, their account or membership, or to a role or permission check; it applies only to administrative or service roles; it is restrictive, so it only narrows other policies; or the table holds data meant for everyone to read.", + "Its condition ties the rows it admits to the user, their account or membership, or to a role or permission check; it applies only to administrative or service roles; it is restrictive, so it only narrows other policies; it lets others read only rows their owners marked as shared or public, such as `sharing <> 'private'` or `is_public`; or the table holds data meant for everyone to read.", POLICY_CONTEXT, ) } @@ -49,12 +52,17 @@ pub fn definer_search_path() -> Value { /// A secret token the function looks up is the caller's capability: /// basejump's `accept_invitation` and `lookup_invitation`, which find an /// invitation by its token, were reviews for checking no `auth.uid()`. +/// The note says who may call a function nothing revokes: chatbot-ui's +/// `delete_storage_object`, which deletes any stored file with the service +/// role key and is callable by anyone, stayed at 0.68 with an empty +/// `function.privileges`, and at 0.75 with the note while the question +/// named only rows. pub fn definer_unchecked() -> Value { sql_noul( - "Does the SECURITY DEFINER function in `function.source` read or change rows of other users without checking who the caller is?", - "It runs with its owner's privileges and returns or changes rows chosen by its arguments, without comparing them to `auth.uid()` or checking a role, and clients can call it.", + "Does the SECURITY DEFINER function in `function.source` read or change other users' rows or stored files without checking who the caller is?", + "It runs with its owner's privileges and returns or changes rows or stored files chosen by its arguments, without comparing them to `auth.uid()` or checking a role, and clients can call it.", "It checks the caller, touches only the caller's rows, acts only for whoever holds a secret token it looks up by value, such as an invitation or reset token, only returns data meant for everyone, is a trigger function that runs on table events, or `function.privileges` revokes EXECUTE from public, anon and authenticated so only roles clients do not use, such as `supabase_auth_admin` or `service_role`, may call it.", - "`function.privileges` lists the grants and revokes of EXECUTE on it, when found.", + "`function.privileges` lists the grants and revokes of EXECUTE on it, when found. PostgreSQL lets every role execute a new function, so unless a revoke there takes EXECUTE from public, clients can call it, anon included.", ) } diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index 52240c2..a2ec26d 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -16,6 +16,7 @@ use serde_json::{Value, json}; pub fn security_interpreted( code: &str, django: bool, + rendered: bool, deserializers: Option<&str>, xml: bool, ) -> Value { @@ -33,11 +34,16 @@ pub fn security_interpreted( 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 { + if rendered { + yes.push_str(", or passed to a template that writes it unescaped"); + } 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;"); + } else if rendered { + yes.push_str(", or passed to a template that writes it unescaped"); } if xml { // Both clauses would make the question too long to read as one. @@ -179,237 +185,6 @@ pub fn security_origin(code: &str, callers: bool, django: bool) -> Value { ) } -/// Options of the URL-parts Choice that rule a URL concern out: a host of the -/// program's own, or no request. -pub const OWN_PARTS: [&str; 2] = ["own", "none"]; - -/// Where the URLs a function requests come from, asked when the URL check -/// stays undecided: on clients of a fixed or configured service the check -/// split on a variable path or query, while naming the host decided them. A -/// host that is sent another URL to fetch is its own option, since internal -/// proxies fetched what users sent. The same question about paths cleared -/// real traversals, reading names stored in an index as the program's own, -/// so paths are not settled this way. -pub fn security_url_parts(code: &str, callers: bool) -> Value { - let shown = if callers { - ", in the function or in what `callers` pass it" - } else { - "" - }; - let note = if callers { - format!("{CALLERS} {EVIDENCE}") - } else { - EVIDENCE.to_string() - }; - json!({ - "type": "choice", - "instructions": { - "question": format!("Where do the URLs that `{code}` requests come from?"), - "note": note, - }, - "criteria": { - "own": format!("A host written in the code or set in the program's configuration or environment, with only ids, names, numbers or search terms from variables in its path or query{shown}."), - "forwards": "A host from the code or configuration, with another URL or host from a variable passed in its path or query for that service to fetch.", - "given": "A whole URL or host handed to the function as a parameter or field.", - "outside": "A URL or host from outside the program, such as a request, message, uploaded file or a record users can edit.", - "none": "It requests no URL.", - }, - }) -} - -/// The option of the runs-in Choice that rules a forged request out. -pub const BROWSER: &str = "browser"; - -/// Where a function runs, asked when the URL check stays undecided, since a -/// request from the user's browser reaches only what that user can. Offered -/// beside the URL's parts, the browser lost to "a whole URL handed to it" -/// for a client component's fetch helper. -pub fn security_runs_in(code: &str) -> Value { - json!({ - "type": "choice", - "instructions": { - "question": format!("Where does `{code}` run once the program is deployed?"), - "note": EVIDENCE, - }, - "criteria": { - "browser": "Only in the user's web browser: in a client component, in a web page script, or in a component or hook that only client code uses.", - "server": "On a server or in a backend process: a route handler, server component, Server Action, API, job, or command-line tool.", - "either": "Either side may run it, such as shared code that both server and browser code import, or the code does not show which.", - }, - }) -} - -/// Options of the redirect-target Choice that rule an open redirect out. -pub const OWN_TARGETS: [&str; 3] = ["own", "checked", "none"]; - -/// Where the targets a function redirects clients to come from, asked when -/// the redirect check stays undecided: client components that navigate to -/// fixed paths or to a checkout URL their server returns, and helpers that -/// build a path their callers name, split on "a URL or path taken from a -/// variable". Offered "a whole path handed to it" beside "what callers -/// pass", helpers whose callers pass fixed paths took the first, which is -/// true as well; with callers shown, that option is only for paths the -/// callers do not explain. -pub fn security_redirect_target(code: &str, callers: bool) -> Value { - let (own, given, note) = if callers { - ( - "A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page, in the function or in what `callers` pass it; variables fill only ids, names, numbers or messages in its segments or query.", - "A whole path or URL handed to the function as a parameter, where `callers` does not show where it comes from.", - format!("{CALLERS} {EVIDENCE}"), - ) - } else { - ( - "A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page; variables fill only ids, names, numbers or messages in its segments or query.", - "A whole path or URL handed to the function as a parameter or field.", - EVIDENCE.to_string(), - ) - }; - json!({ - "type": "choice", - "instructions": { - "question": format!("Where do the paths or URLs that `{code}` redirects or navigates the client to come from?"), - "note": note, - }, - "criteria": { - "own": own, - "checked": "A path or URL from a variable that is checked before the redirect to be a path on the program's own site or on a host from an allowed list.", - "given": given, - "outside": "A whole path or URL that a request carries, such as a query parameter, form field, header or cookie, or an argument of a function clients call directly, without such a check.", - "none": "It redirects or navigates nowhere; it only builds or returns a path, or it has no redirect.", - }, - }) -} - -/// Options of the markup Choice that rule a markup injection out. -pub const INERT_MARKUP: [&str; 3] = ["escaped", "text", "none"]; - -/// How the markup a function builds with variables is rendered, asked when -/// the markup check stays undecided: React components with values in -/// attributes, and snippets shown in a text field, split on "a variable put -/// into markup without escaping". A Django view is asked what it sends -/// back: views that only redirect or render a template split on the markup -/// check, since the variables they pass on end up in a page, and a template -/// escapes them unless it writes one with `|safe`. -pub fn security_markup_output(code: &str, django: bool) -> Value { - if django { - return json!({ - "type": "choice", - "instructions": { - "question": format!("What does `{code}` send back to the client, and how are the variables in it rendered?"), - "note": EVIDENCE, - }, - "criteria": { - "escaped": "A page rendered from a template that writes each value it is given without a safe filter or autoescaping off, which Django escapes, or HTML built with format_html or escape.", - "text": "It is never rendered as HTML: JSON, a file download or plain text.", - "raw": "HTML it builds from variables as text itself, text it marks safe with mark_safe, or a template that writes a value it is given with a safe filter or with autoescaping off.", - "none": "No markup with variables: it only redirects, or sends nothing to a client itself.", - }, - }); - } - json!({ - "type": "choice", - "instructions": { - "question": format!("How is the markup that `{code}` builds with variables rendered?"), - "note": EVIDENCE, - }, - "criteria": { - "escaped": "By JSX or a template engine that escapes each value: variables appear only as element children, attribute values or component props, or go through an escaping or sanitizing function first.", - "text": "It is never rendered as HTML: it is shown as plain text, such as a code snippet in a text field, or sent as text.", - "raw": "As raw HTML with a variable inside, unescaped: through dangerouslySetInnerHTML, innerHTML, insertAdjacentHTML, document.write, an iframe srcdoc, or an HTML response built as text.", - "none": "It builds no HTML or SVG markup with variables.", - }, - }) -} - -/// Options of the logging Choice that rule a logged secret out. -pub const PLAIN_LOGS: [&str; 2] = ["plain", "none"]; - -/// What a function's log statements write, asked when the check for a -/// logged object that holds a secret stays undecided: an error caught from a -/// payment or database call, logged with a message, split on it. -pub fn security_logged(code: &str) -> Value { - json!({ - "type": "choice", - "instructions": { - "question": format!("What do the log and console statements of `{code}` write?"), - "note": EVIDENCE, - }, - "criteria": { - "plain": "Only messages, ids, counts, statuses, or an error caught from a failed call, none of which holds a password, token or key.", - "secret": "A password, token, API key or other secret, or a whole object, configuration, request or argument list that holds one.", - "personal": "Personal data about a person, such as an email address, name, address or document number.", - "none": "It logs or prints nothing.", - }, - }) -} - -/// Options of the CORS Choice that rule a credentialed-origin concern out. -pub const SAFE_ORIGINS: [&str; 3] = ["unset", "listed", "public"]; - -/// Which other sites a function lets send credentialed requests, asked when -/// the CORS check stays undecided: route handlers that set cookies or answer -/// preflights with `*` and no credentials split on "any origin allowed". -pub fn security_cors_origins(code: &str) -> Value { - json!({ - "type": "choice", - "instructions": { - "question": format!("Which other sites does `{code}` let send requests that carry a user's cookies or credentials?"), - "note": EVIDENCE, - }, - "criteria": { - "unset": "None: it sets no CORS header or option.", - "listed": "Only origins written in the code or configuration, or the program's own origin.", - "public": "Any origin, but without allowing credentials: no `Access-Control-Allow-Credentials: true` or credentials option, as for a public or token-authenticated API.", - "any": "Any origin, or whatever origin a request names reflected back, with credentials allowed.", - }, - }) -} - -/// The options of the cookie Choice that clear the cookie check. -pub const FLAGGED_COOKIES: [&str; 2] = ["unset", "flagged"]; - -/// What a function leaves a session cookie's flags as, asked when the cookie -/// check stays undecided: a SvelteKit form action's `cookies.set` without -/// options, whose defaults set both flags, stayed at 0.21. -pub fn security_cookie_flags(code: &str) -> Value { - json!({ - "type": "choice", - "instructions": { - "question": format!("How are the Secure and HttpOnly flags set on the cookies `{code}` sets?"), - "note": EVIDENCE, - }, - "criteria": { - "unset": "It sets no cookie, or only cookies that hold no session, token or sign-in state, such as a theme or language preference.", - "flagged": "Session or token cookies get both flags: in the options it passes, or from a framework whose defaults set them, such as SvelteKit's `cookies.set`.", - "missing": "A session or token cookie is set with Secure or HttpOnly turned off, or through an API whose defaults leave them off, such as Express `res.cookie`, `document.cookie` or PHP `setcookie` without them.", - }, - }) -} - -/// The options of the destination Choice that rule error details out: every -/// place but a remote client. -pub const AWAY_FROM_CLIENTS: [&str; 4] = ["local", "logs", "caller", "stored"]; - -/// Where a function's text goes, asked when an error-detail signal stays -/// undecided. An error or body shaped for a response counts as the client: -/// helpers that format errors for a server's callers return them. -pub fn security_destination(code: &str) -> Value { - json!({ - "type": "choice", - "instructions": { - "question": format!("Where does the text that `{code}` produces or passes on go?"), - "note": EVIDENCE, - }, - "criteria": { - "client": "Into a response to a request from another computer: an HTTP, API or RPC response, a message to a connected client, or an error, status or body shaped for such a response that it builds or returns.", - "local": "To the person running a local program: a terminal, console, window, or a report or file on their own machine.", - "logs": "To logs, or to the program's own error reporting or monitoring.", - "caller": "Back to the code that called it as an ordinary error or value, such as a parse, lookup or validation failure, not shaped as a response.", - "stored": "Into a database, queue, cache or job record.", - }, - }) -} - /// Asked in the sensitive-data trace: whether every error message is the /// program's own. It can only clear the error-detail signals; functions that /// throw the program's typed errors otherwise stayed undecided, since the @@ -545,7 +320,7 @@ impl Check { } } -const CALLERS: &str = "`callers` holds functions that call it."; +pub(super) const CALLERS: &str = "`callers` holds functions that call it."; /// Whether a variable reaches each kind of interpreted text unhandled. The /// markup check names text shown as a JSX child and CSS values as escaped or @@ -646,8 +421,24 @@ const DJANGO_MARKUP: Check = Check { ], }; +/// The markup check of a function outside Django that renders a template +/// writing values without escaping: DVNA's product search hands the +/// request's search term to `views/app/products.ejs`, which writes it with +/// `<%- … %>`, and the function alone read as building no markup. +pub const VIEW_MARKUP: Check = Check { + id: "markup", + question: "Does `{code}` put a variable into HTML or SVG markup without escaping it, itself or through a template it renders?", + yes: "A variable is joined into HTML or SVG text, or passed to a template that writes it without escaping, such as with EJS `<%- … %>`, Handlebars `{{{ … }}}` or a `|safe` filter, without an escaping function.", + no: "Values go through an escaping function or a template that escapes them, or it builds no markup.", + no_examples: &[ + "A template rendered with the variable, when the template writes that value with an escaping tag, such as EJS `<%= … %>` or Handlebars `{{ … }}`", + ], +}; + /// Specific weak settings, asked when the broad presence question is not clear. -pub const WEAK_SETTINGS: [Check; 6] = [ +/// Turning off output escaping had no check: NodeGoat's `autoescape: false` +/// and RailsGoat's `escape_html_entities_in_json = false` were at most notes. +pub const WEAK_SETTINGS: [Check; 7] = [ Check { id: "tls", question: "Does `{code}` turn off certificate or host name verification?", @@ -657,9 +448,9 @@ pub const WEAK_SETTINGS: [Check; 6] = [ }, Check { id: "hash", - question: "Does `{code}` hash passwords or derive keys from them with a fast or broken hash, or with few iterations?", - yes: "It hashes passwords or derives keys from them with MD5, SHA-1, a single round of SHA-256, or a key derivation function with few iterations.", - no: "It uses bcrypt, scrypt, Argon2 or a key derivation function with many iterations, or it does not handle passwords.", + question: "Does `{code}` keep passwords as plain text, or hash them or derive keys from them with a fast or broken hash, or with few iterations?", + yes: "It saves passwords, or checks a login against saved passwords, as plain text, or hashes passwords or derives keys from them with MD5, SHA-1, a single round of SHA-256, or a key derivation function with few iterations.", + no: "It uses bcrypt, scrypt, Argon2 or a key derivation function with many iterations; it hands passwords to a library, framework or model hook that hashes them before saving; or it does not handle passwords.", no_examples: &[], }, Check { @@ -681,7 +472,10 @@ pub const WEAK_SETTINGS: [Check; 6] = [ question: "Does `{code}` set or configure a session or authentication cookie without the Secure or HttpOnly flag?", yes: "A cookie that holds a session or token is set or configured without Secure or without HttpOnly.", no: "Such cookies have both flags, the cookie holds no session or token, or the code sets no cookie.", - no_examples: &[], + no_examples: &[ + "A cookie added to a request, such as Go's `r.AddCookie`, rather than set on a response", + "A cookie set empty and already expired, which deletes it", + ], }, Check { id: "public_secret", @@ -690,8 +484,38 @@ pub const WEAK_SETTINGS: [Check; 6] = [ no: "Such variables hold only values meant for browsers, such as publishable or anonymous keys, public URLs and site ids; secrets come from variables without such a prefix; or it reads no such variable.", no_examples: &[], }, + Check { + id: "escape", + question: "Does `{code}` turn off the automatic escaping of values written into HTML?", + yes: "It turns off a template engine's or serializer's escaping of HTML for output that browsers render, such as autoescape set to false or escape_html_entities_in_json set to false.", + no: "Escaping stays on; the output is not HTML that browsers render, such as Markdown, plain-text email or source code; or it configures no escaping.", + no_examples: &[], + }, ]; +/// The token and key checks of code outside C# and Django, which ask their +/// own: a JWT decoded without verifying its signature found the broad +/// question at 0.93 to 0.98 in DVGA and JavaVulnerableLab, and with no check +/// to name the setting it was only a note; DVNA's session secret +/// `'keyboard cat'` and RailsGoat's encryption key were missed. +pub const TOKEN_AND_KEY: [Check; 2] = [TOKEN, KEY]; + +const KEY: Check = Check { + id: "key", + question: "Does `{code}` sign or encrypt with a key or secret written in the code?", + yes: "A signing or encryption key, such as the secret that signs session cookies or JSON Web Tokens, or a key that encrypts stored data, is a string or bytes written in the code or a constant of the program.", + no: "Keys are read from configuration, the environment or a secret store; the literal is only a placeholder, or is used only in tests or local development; or it uses no key.", + no_examples: &[], +}; + +const TOKEN: Check = Check { + id: "token", + question: "Does `{code}` accept security tokens without verifying their signature or expiry?", + yes: "It trusts a JSON Web Token or other signed token without verifying its signature, such as a decode call with signature verification turned off, a decode used where a verify is needed, or an algorithm list that allows none, or it turns off the expiry check.", + no: "Signatures and expiry are checked where tokens are accepted, or it accepts none: it only creates, stores or sends a token, reads the claims of a token already verified before it, or checks that one is present while a server verifies it.", + no_examples: &[], +}; + const DJANGO_HASH: Check = Check { id: "hash", question: "Does `{code}` hash passwords or derive keys from them with a fast or broken hash, or with few iterations?", diff --git a/src/units/questions/settle.rs b/src/units/questions/settle.rs new file mode 100644 index 0000000..9e4db02 --- /dev/null +++ b/src/units/questions/settle.rs @@ -0,0 +1,320 @@ +//! The settle Choices of security units: one literal Choice per kind of +//! check, naming what the code does (where its URLs, redirect targets and +//! text go, how it renders markup and handles tokens and passwords, what its +//! logs write, which origins and cookies it allows), asked apart from the +//! checks; some options clear the check they settle. +use super::{EVIDENCE, security::CALLERS}; +use serde_json::{Value, json}; + +/// Options of the URL-parts Choice that rule a URL concern out: a host of the +/// program's own, or no request. +pub const OWN_PARTS: [&str; 2] = ["own", "none"]; + +/// Where the URLs a function requests come from, asked when the URL check +/// stays undecided: on clients of a fixed or configured service the check +/// split on a variable path or query, while naming the host decided them. A +/// host that is sent another URL to fetch is its own option, since internal +/// proxies fetched what users sent. The same question about paths cleared +/// real traversals, reading names stored in an index as the program's own, +/// so paths are not settled this way. +pub fn security_url_parts(code: &str, callers: bool) -> Value { + let shown = if callers { + ", in the function or in what `callers` pass it" + } else { + "" + }; + let note = if callers { + format!("{CALLERS} {EVIDENCE}") + } else { + EVIDENCE.to_string() + }; + json!({ + "type": "choice", + "instructions": { + "question": format!("Where do the URLs that `{code}` requests come from?"), + "note": note, + }, + "criteria": { + "own": format!("A host written in the code or set in the program's configuration or environment, with only ids, names, numbers or search terms from variables in its path or query{shown}."), + "forwards": "A host from the code or configuration, with another URL or host from a variable passed in its path or query for that service to fetch.", + "given": "A whole URL or host handed to the function as a parameter or field.", + "outside": "A URL or host from outside the program, such as a request, message, uploaded file or a record users can edit.", + "none": "It requests no URL.", + }, + }) +} + +/// The option of the runs-in Choice that rules a forged request out. +pub const BROWSER: &str = "browser"; + +/// Where a function runs, asked when the URL check stays undecided, since a +/// request from the user's browser reaches only what that user can. Offered +/// beside the URL's parts, the browser lost to "a whole URL handed to it" +/// for a client component's fetch helper. +pub fn security_runs_in(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("Where does `{code}` run once the program is deployed?"), + "note": EVIDENCE, + }, + "criteria": { + "browser": "Only in the user's web browser: in a client component, in a web page script, or in a component or hook that only client code uses.", + "server": "On a server or in a backend process: a route handler, server component, Server Action, API, job, or command-line tool.", + "either": "Either side may run it, such as shared code that both server and browser code import, or the code does not show which.", + }, + }) +} + +/// Options of the redirect-target Choice that rule an open redirect out. +pub const OWN_TARGETS: [&str; 3] = ["own", "checked", "none"]; + +/// Where the targets a function redirects clients to come from, asked when +/// the redirect check stays undecided: client components that navigate to +/// fixed paths or to a checkout URL their server returns, and helpers that +/// build a path their callers name, split on "a URL or path taken from a +/// variable". Offered "a whole path handed to it" beside "what callers +/// pass", helpers whose callers pass fixed paths took the first, which is +/// true as well; with callers shown, that option is only for paths the +/// callers do not explain. +pub fn security_redirect_target(code: &str, callers: bool) -> Value { + let (own, given, note) = if callers { + ( + "A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page, in the function or in what `callers` pass it; variables fill only ids, names, numbers or messages in its segments or query.", + "A whole path or URL handed to the function as a parameter, where `callers` does not show where it comes from.", + format!("{CALLERS} {EVIDENCE}"), + ) + } else { + ( + "A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page; variables fill only ids, names, numbers or messages in its segments or query.", + "A whole path or URL handed to the function as a parameter or field.", + EVIDENCE.to_string(), + ) + }; + json!({ + "type": "choice", + "instructions": { + "question": format!("Where do the paths or URLs that `{code}` redirects or navigates the client to come from?"), + "note": note, + }, + "criteria": { + "own": own, + "checked": "A path or URL from a variable that is checked before the redirect to be a path on the program's own site or on a host from an allowed list.", + "given": given, + "outside": "A whole path or URL that a request carries, such as a query parameter, form field, header or cookie, or an argument of a function clients call directly, without such a check.", + "none": "It redirects or navigates nowhere; it only builds or returns a path, or it has no redirect.", + }, + }) +} + +/// Options of the markup Choice that rule a markup injection out. +pub const INERT_MARKUP: [&str; 3] = ["escaped", "text", "none"]; + +/// How the markup a function builds with variables is rendered, asked when +/// the markup check stays undecided: React components with values in +/// attributes, and snippets shown in a text field, split on "a variable put +/// into markup without escaping". A Django view is asked what it sends +/// back: views that only redirect or render a template split on the markup +/// check, since the variables they pass on end up in a page, and a template +/// escapes them unless it writes one with `|safe`. A function elsewhere +/// that renders a template writing values unescaped is asked the same way. +pub fn security_markup_output(code: &str, django: bool, rendered: bool) -> Value { + if rendered && !django { + return json!({ + "type": "choice", + "instructions": { + "question": format!("What does `{code}` send back to the client, and how are the variables in it rendered?"), + "note": EVIDENCE, + }, + "criteria": { + "escaped": "A page rendered from a template that writes each value it is given with an escaping tag, such as EJS `<%= … %>` or Handlebars `{{ … }}`, or HTML built with an escaping function.", + "text": "It is never rendered as HTML: JSON, a file download or plain text.", + "raw": "HTML it builds from variables as text itself, or a template that writes a value it is given without escaping, such as with EJS `<%- … %>`, Handlebars `{{{ … }}}` or a `|safe` filter.", + "none": "No markup with variables: it only redirects, or sends nothing to a client itself.", + }, + }); + } + if django { + return json!({ + "type": "choice", + "instructions": { + "question": format!("What does `{code}` send back to the client, and how are the variables in it rendered?"), + "note": EVIDENCE, + }, + "criteria": { + "escaped": "A page rendered from a template that writes each value it is given without a safe filter or autoescaping off, which Django escapes, or HTML built with format_html or escape.", + "text": "It is never rendered as HTML: JSON, a file download or plain text.", + "raw": "HTML it builds from variables as text itself, text it marks safe with mark_safe, or a template that writes a value it is given with a safe filter or with autoescaping off.", + "none": "No markup with variables: it only redirects, or sends nothing to a client itself.", + }, + }); + } + json!({ + "type": "choice", + "instructions": { + "question": format!("How is the markup that `{code}` builds with variables rendered?"), + "note": EVIDENCE, + }, + "criteria": { + "escaped": "By JSX or a template engine that escapes each value: variables appear only as element children, attribute values or component props, or go through an escaping or sanitizing function first.", + "text": "It is never rendered as HTML: it is shown as plain text, such as a code snippet in a text field, or sent as text.", + "raw": "As raw HTML with a variable inside, unescaped: through dangerouslySetInnerHTML, innerHTML, insertAdjacentHTML, document.write, an iframe srcdoc, or an HTML response built as text.", + "none": "It builds no HTML or SVG markup with variables.", + }, + }) +} + +/// Options of the logging Choice that rule a logged secret out. +pub const PLAIN_LOGS: [&str; 4] = ["plain", "identity", "operator", "none"]; + +/// What a function's log statements write, asked whenever a logging signal +/// is not clear: an error caught from a payment or database call, logged +/// with a message, split on the check for a logged object; and the question +/// whether it logs personal data found an audit line naming who signed in +/// (vaultwarden's "User {email} logged in successfully. IP: {ip}") and a +/// command printing recovery codes for the admin who ran it: 10 of 19 +/// labeled logging reviews were such lines. +pub fn security_logged(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("What do the log and console statements of `{code}` write?"), + "note": EVIDENCE, + }, + "criteria": { + "plain": "Only messages, ids, counts, statuses, or an error caught from a failed call, none of which holds a password, token or key.", + "identity": "Who did what: a user's id, name, email address or IP address beside the action they took, as an audit or access log records, and no secret.", + "operator": "Values it shows on purpose to the person running a command-line tool, such as recovery codes or credentials a command prints for that person.", + "secret": "A password, token, API key or other secret, or a whole object, configuration, request or argument list that holds one.", + "personal": "Other personal data about a person, such as a home address, document number, or health or payment details.", + "none": "It logs or prints nothing.", + }, + }) +} + +/// Options of the token Choice that rule an unverified-token concern out. +pub const VERIFIED_TOKENS: [&str; 5] = [ + "verifies", + "passes", + "verified_before", + "reads_claims", + "none", +]; + +/// What a function does with security tokens, asked whenever the token check +/// is not clear: front-end hooks that read their own token to send it and +/// middleware that looks a session up stayed between 0.2 and 0.5 on the +/// check, while naming what the code does with tokens decides. Reading a +/// token's claims is apart from deciding access with them: code that read +/// the expiry of a token its identity provider had just sent, or the +/// character id of an access token, was chosen as trusting it unverified. +pub fn security_token_use(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("What does `{code}` do with security tokens, such as JSON Web Tokens or session tokens?"), + "note": EVIDENCE, + }, + "criteria": { + "verifies": "It verifies each token's signature and expiry, or looks the token up in its own store, before trusting what it holds.", + "passes": "It only creates, signs, stores, sends or forwards tokens, or checks that one is present, while a server verifies them.", + "verified_before": "It reads the claims of a token verified before it runs, such as by middleware, or of a token it has just received from an identity provider over TLS.", + "reads_claims": "It decodes a token only to read or show what it says, such as a user id, a name or its expiry, while other code or a server decides what the caller may do.", + "decides_access": "It decides what the caller may do, such as signing them in, granting a role or accepting a reset, from a token it has not verified.", + "turned_off": "It turns off a check a library makes by default, such as verify_signature=False, verify=False, an algorithm list that allows none, or ignoreExpiration.", + "none": "It handles no security tokens.", + }, + }) +} + +/// Options of the password Choice that rule a weak-password concern out. +pub const HASHED_PASSWORDS: [&str; 2] = ["slow_hash", "none"]; + +/// How a function treats users' passwords, asked whenever the password +/// check is not clear: HMAC signing, key loading and a demo login form were +/// reviews or stayed between 0.2 and 0.4 on it. +pub fn security_password_handling(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("How does `{code}` handle users' passwords?"), + "note": EVIDENCE, + }, + "criteria": { + "slow_hash": "It hashes them with bcrypt, scrypt, Argon2 or a key derivation function with many iterations, or hands them to a library, framework or model hook that does.", + "plain": "It saves them, or checks a login against saved ones, as plain text.", + "fast_hash": "It hashes them with MD5, SHA-1, a single round of SHA-256 or another fast hash, or derives keys from them with few iterations.", + "none": "It stores and checks no users' passwords: what it hashes, signs or encrypts is other data, such as tokens, messages, files or keys, or it only fills in or sends a password someone types.", + }, + }) +} + +/// Options of the CORS Choice that rule a credentialed-origin concern out. +pub const SAFE_ORIGINS: [&str; 3] = ["unset", "listed", "public"]; + +/// Which other sites a function lets send credentialed requests, asked when +/// the CORS check stays undecided: route handlers that set cookies or answer +/// preflights with `*` and no credentials split on "any origin allowed". +pub fn security_cors_origins(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("Which other sites does `{code}` let send requests that carry a user's cookies or credentials?"), + "note": EVIDENCE, + }, + "criteria": { + "unset": "None: it sets no CORS header or option.", + "listed": "Only origins written in the code or configuration, or the program's own origin.", + "public": "Any origin, but without allowing credentials: no `Access-Control-Allow-Credentials: true` or credentials option, as for a public or token-authenticated API.", + "any": "Any origin, or whatever origin a request names reflected back, with credentials allowed.", + }, + }) +} + +/// The options of the cookie Choice that clear the cookie check. +pub const FLAGGED_COOKIES: [&str; 2] = ["unset", "flagged"]; + +/// What a function leaves a session cookie's flags as, asked when the cookie +/// check stays undecided: a SvelteKit form action's `cookies.set` without +/// options, whose defaults set both flags, stayed at 0.21. +pub fn security_cookie_flags(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("How are the Secure and HttpOnly flags set on the cookies `{code}` sets?"), + "note": EVIDENCE, + }, + "criteria": { + "unset": "It sets no cookie, or only cookies that hold no session, token or sign-in state, such as a theme or language preference.", + "flagged": "Session or token cookies get both flags: in the options it passes, or from a framework whose defaults set them, such as SvelteKit's `cookies.set`.", + "missing": "A session or token cookie is set with Secure or HttpOnly turned off, or through an API whose defaults leave them off, such as Express `res.cookie`, `document.cookie` or PHP `setcookie` without them.", + }, + }) +} + +/// The options of the destination Choice that rule error details out: every +/// place but a remote client. +pub const AWAY_FROM_CLIENTS: [&str; 4] = ["local", "logs", "caller", "stored"]; + +/// Where a function's text goes, asked when an error-detail signal stays +/// undecided. An error or body shaped for a response counts as the client: +/// helpers that format errors for a server's callers return them. A game +/// client that hands the server's error text to its own window over a +/// channel of `Response` messages was answered as sending it to a client, +/// so the local option names the program's own screens. +pub fn security_destination(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("Where does the text that `{code}` produces or passes on go?"), + "note": EVIDENCE, + }, + "criteria": { + "client": "Into a response to a request from another computer: an HTTP, API or RPC response, a message to a connected remote client, or an error, status or body shaped for such a response that it builds or returns.", + "local": "To the person running a local program: a terminal, console or window, the program's own screens that a desktop, game or mobile app reaches through a channel, event or IPC call, or a report or file on their own machine.", + "logs": "To logs, or to the program's own error reporting or monitoring.", + "caller": "Back to the code that called it as an ordinary error or value, such as a parse, lookup or validation failure, not shaped as a response.", + "stored": "Into a database, queue, cache or job record.", + }, + }) +} diff --git a/src/units/questions/test_rules.rs b/src/units/questions/test_rules.rs index a77d568..ba184f3 100644 --- a/src/units/questions/test_rules.rs +++ b/src/units/questions/test_rules.rs @@ -137,6 +137,34 @@ pub fn test_mock_only(path: &str, evidence: TestEvidence) -> Value { }) } +/// The `reads` options that name what a caller can observe. +pub const OBSERVED_READS: [&str; 3] = ["effects", "result", "state"]; + +/// What a test's assertions read, asked with the code under test once its +/// first answer said it asserts internal details. Asked of the test and the +/// signatures it calls, that check read a debug panel's recorded queries +/// (`panel._queries`, which the panel renders), a framework's documented +/// hooks and an app's state after an action as internals: of 66 such +/// considers labeled by hand, 49 were wrong, and all 44 whose assertions +/// read state or effects were among them, while 16 of the 19 reading stored +/// input or the program's own calls were right. +pub fn test_reads(path: &str, evidence: TestEvidence) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("What do the assertions of the test in `{path}` read where they use private names, mocks or spies?"), + "note": test_note(evidence), + }, + "criteria": { + "result": "What the code under test returns or builds, read through its fields, private ones included.", + "state": "The state an action leaves the object under test in, when the program shows that state or acts on it next, such as records a panel collects and renders or a flag a later call reads.", + "effects": "Effects a caller or another system can observe: rows written, responses, files, rendered output, requests a stand-in received, or calls to hooks and callbacks that the caller or a framework supplies.", + "stored": "That a constructor or setter kept what it was given, or private fields that no behavior of the code depends on.", + "own_calls": "Which of the program's own functions were called, how often or in what order, through spies or mocks on its own helpers.", + }, + }) +} + pub fn test_several(path: &str) -> Value { noul( format!("Does the test in `{path}` check several unrelated behaviors?"), diff --git a/src/units/security.rs b/src/units/security.rs index a2569cf..90097ef 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -57,13 +57,25 @@ pub(super) struct Subject<'a> { /// 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, + /// Whether its file sits at a test path, such as a test app's settings. + pub test_path: bool, } +/// The evidence key of the templates a function renders that write values +/// without escaping. +pub(super) const RENDERED: &str = "templates_it_renders_that_write_values_without_escaping"; + impl Subject<'_> { fn code(&self) -> String { format!("{}.source", self.kind) } + /// Whether it renders a template that writes values without escaping, + /// outside Django, whose questions name its templates already. + fn renders(&self) -> bool { + !self.django && self.evidence.contains_key(RENDERED) + } + /// Its name, source and framework evidence, as sent. fn state(&self) -> Value { let mut state = serde_json::Map::new(); @@ -95,6 +107,7 @@ pub(super) fn function_subject<'a>( evidence: serde_json::Map::new(), django: false, callee_errors: Vec::new(), + test_path: false, } } @@ -181,9 +194,43 @@ pub(super) fn setup_subject<'a>( evidence: serde_json::Map::new(), django: false, callee_errors: Vec::new(), + test_path: false, + }) +} + +/// A server template's code that reads client data, judged like a function +/// by every security rule. +pub(super) fn template_subject<'a>( + file: &FileContext<'_>, + code: &'a crate::analysis::sites::Setup, +) -> Option> { + let first = code.statements.first()?; + let last = code.statements.last()?; + let source: Vec<&str> = code + .statements + .iter() + .map(|(range, ..)| &file.source[range.clone()]) + .collect(); + Some(Subject { + name: TEMPLATE_CODE.into(), + kind: "function", + source: source.join("\n"), + sites: &code.sites, + errors: &[], + lines: (first.1, last.2), + callers: Vec::new(), + enums: Vec::new(), + constants: Vec::new(), + evidence: serde_json::Map::new(), + django: false, + callee_errors: Vec::new(), + test_path: false, }) } +/// The name of the unit that holds a server template's code. +pub(super) const TEMPLATE_CODE: &str = "template code"; + /// The name of the unit that holds a file's top-level setup statements. pub(super) const MODULE_SETUP: &str = "module setup"; /// The name of that unit in a Django settings module, whose statements @@ -295,11 +342,12 @@ fn push_unit( } else { Vec::new() }, - trace, + trace: trace.map(Into::into), settles, django: subject.django, + test_path: subject.test_path, }, - recheck, + recheck: recheck.map(Into::into), }); (rule, out.units.len() - 1, id.to_string()) } @@ -363,11 +411,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); + let rendered = !django && items[index].1.get(RENDERED).is_some(); for (rule, _, id) in units { for question in presence_questions(rule) { questions.ask( format!("{}{index}_{question}", &key[..1]), - presence_body(question, &code, django, (deserializers, xml)), + presence_body(question, &code, (django, rendered), (deserializers, xml)), id, rule, question, @@ -399,11 +448,13 @@ pub(super) fn presence_questions(rule: &str) -> &'static [&'static str] { fn presence_body( question: &str, code: &str, - django: bool, + (django, rendered): (bool, bool), (deserializers, xml): (Option<&str>, bool), ) -> Value { match question { - "interpreted" => questions::security_interpreted(code, django, deserializers, xml), + "interpreted" => { + questions::security_interpreted(code, django, rendered, deserializers, xml) + } "resource" => questions::security_resource(code, django), "logs_secret" => questions::security_logs_secret(code), "error_details" => questions::security_error_details(code, django), @@ -469,7 +520,8 @@ fn rule_checks( /// 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. Code that parses XML with a parser able to -/// resolve external entities (`xml`) is asked the XML check. +/// resolve external entities (`xml`) is asked the XML check. The token and +/// key checks are asked of code outside C# and Django, which ask their own. fn asked_checks( rule: &str, language: &str, @@ -504,6 +556,12 @@ fn asked_checks( .flatten(), ) .chain((rule == INJECTION && xml).then_some(&questions::XXE)) + .chain( + (rule == UNSAFE_SETTINGS && language != questions::CSHARP && !django) + .then_some(&questions::TOKEN_AND_KEY) + .into_iter() + .flatten(), + ) .collect() } @@ -567,21 +625,57 @@ fn trace( questions::security_message_origin(&ids, from_callees), ); } - 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 { - check - }; + for check in trace_checks(file, subject, rule, from_callees) { ask(check.id, check.body(&code)); } + file.request( + "trace", + trace_state(file, subject, rule, messages), + questions, + ) +} + +/// The checks a unit's trace and recheck ask, in the variants its evidence +/// calls for: the exception check of text its callees create, and the +/// markup check of a function that renders unescaped templates. +fn trace_checks( + file: &FileContext<'_>, + subject: &Subject<'_>, + rule: &'static str, + from_callees: bool, +) -> Vec<&'static questions::Check> { + let xml = questions::parses_xml(file.source, &subject.source); + asked_checks(rule, file.language, subject.django, &subject.source, xml) + .into_iter() + // A template's code writes its values unescaped by construction, + // which injection judges; it turns no escaping setting off. Asked + // anyway, a JSP page's `<%= … %>` read as one. + .filter(|check| !(subject.name == TEMPLATE_CODE && check.id == "escape")) + .map(|check| { + if from_callees && check.id == "exception_to_client" { + &questions::EXCEPTION_TO_CLIENT_FROM_CALLEES + } else if subject.renders() && check.id == "markup" { + &questions::VIEW_MARKUP + } else { + check + } + }) + .collect() +} + +/// A trace's state: the unit's source with its sites, and the evidence its +/// rule's checks read. +fn trace_state( + file: &FileContext<'_>, + subject: &Subject<'_>, + rule: &str, + messages: Vec, +) -> Value { let mut state = json!({ "file": file.file_state(), subject.kind: subject.state(), "sites": subject.sites.iter().map(|s| json!({"id": s.id, "source": s.text})).collect::>(), }); - if rule == SENSITIVE_DATA && !messages.is_empty() { state["messages"] = json!(messages); } @@ -594,8 +688,7 @@ fn trace( if rule == UNSAFE_SETTINGS && !subject.constants.is_empty() { state["constants_named"] = json!(subject.constants); } - - file.request("trace", state, questions) + state } /// The origin question and the injection checks again, with the functions @@ -615,14 +708,7 @@ fn recheck(file: &FileContext<'_>, subject: &Subject<'_>, id: &str) -> Option<(V "origin", Pass::Recheck, ); - let xml = questions::parses_xml(file.source, &subject.source); - for check in asked_checks( - INJECTION, - file.language, - subject.django, - &subject.source, - xml, - ) { + for check in trace_checks(file, subject, INJECTION, false) { questions.ask( check.id.into(), check.with_callers(&code), @@ -703,8 +789,13 @@ pub(in crate::units) enum SettleWhen { /// may send credentials settle theirs, which split on client components that /// navigate to fixed paths or render values as attributes, and on route /// handlers that answer preflights for any origin without credentials. What -/// its logs write settles a logged object, which split on errors caught from -/// a payment or database call. Where a function's text goes settles error +/// its logs write settles its logging signals whenever they are not clear: +/// a logged object split on errors caught from a payment or database call, +/// and audit lines naming who signed in were logged personal data. Where a +/// function's text goes is asked whenever its error-detail signals are not +/// clear, too: a game client handing the server's error text to its own +/// window over a channel whose messages are named `Response` was fifteen +/// reviews for sending details to a remote client. Where a function's text goes settles error /// details (see `exposure_signal`), also under a finding that claims the text /// likely reaches a client. /// @@ -717,8 +808,11 @@ pub(in crate::units) enum SettleWhen { /// its undecided path Choice was never asked, and once the markup Choice /// cleared the markup it was left uncertain. What their command lines hold /// settles the shell check the same way: a page that checks each octet of -/// an address with is_numeric was a command injection at 0.88. -pub(in crate::units) const SETTLES: [SettleKind; 11] = [ +/// an address with is_numeric was a command injection at 0.88. What code +/// does with tokens and how it handles passwords settle those checks +/// whenever they are not clear: front ends that send their own token and +/// HMAC signing split on them or were reviews. +pub(in crate::units) const SETTLES: [SettleKind; 13] = [ SettleKind { rule: INJECTION, question: "url_parts", @@ -788,16 +882,16 @@ pub(in crate::units) const SETTLES: [SettleKind; 11] = [ checks: &["error_details", "exception_to_client"], clears: &questions::AWAY_FROM_CLIENTS, callers: false, - when: SettleWhen::UndecidedOrFinding, + when: SettleWhen::NotClear, files: SettleFiles::All, }, SettleKind { rule: SENSITIVE_DATA, question: "logged", - checks: &["logs_object_secret"], + checks: &["logs_object_secret", "logs_secret"], clears: &questions::PLAIN_LOGS, callers: false, - when: SettleWhen::Undecided, + when: SettleWhen::NotClear, files: SettleFiles::All, }, SettleKind { @@ -818,6 +912,24 @@ pub(in crate::units) const SETTLES: [SettleKind; 11] = [ when: SettleWhen::Undecided, files: SettleFiles::All, }, + SettleKind { + rule: UNSAFE_SETTINGS, + question: "token_use", + checks: &["token"], + clears: &questions::VERIFIED_TOKENS, + callers: false, + when: SettleWhen::NotClear, + files: SettleFiles::All, + }, + SettleKind { + rule: UNSAFE_SETTINGS, + question: "password_handling", + checks: &["hash"], + clears: &questions::HASHED_PASSWORDS, + callers: false, + when: SettleWhen::NotClear, + files: SettleFiles::All, + }, ]; /// The settle follow-ups of one unit, one per Choice of its rule, each sent @@ -835,7 +947,7 @@ fn settles( let request = settle(file, subject, kind, id); file.budget.fits(&request.0).then_some(Settle { question: kind.question, - request, + request: request.into(), }) }) .collect() @@ -853,13 +965,17 @@ fn settle( "url_parts" => questions::security_url_parts(&code, callers), "runs_in" => questions::security_runs_in(&code), "redirect_target" => questions::security_redirect_target(&code, callers), - "markup_output" => questions::security_markup_output(&code, subject.django), + "markup_output" => { + questions::security_markup_output(&code, subject.django, subject.renders()) + } "markup_parts" => questions::security_markup_parts(&code, callers), "path_parts" => questions::security_path_parts(&code), "shell_parts" => questions::security_shell_parts(&code), "destination" => questions::security_destination(&code), "logged" => questions::security_logged(&code), "cookie_flags" => questions::security_cookie_flags(&code), + "token_use" => questions::security_token_use(&code), + "password_handling" => questions::security_password_handling(&code), _ => questions::security_cors_origins(&code), }; let mut questions = Questions::default(); diff --git a/src/units/test_units.rs b/src/units/test_units.rs index 884b6be..4b989d1 100644 --- a/src/units/test_units.rs +++ b/src/units/test_units.rs @@ -99,7 +99,11 @@ pub(super) fn plan_values( } else { (setup.clone(), Vec::new()) }; - let recheck = value_recheck(file, case, &id, subjects, &own, &helper_paths); + let evidence = value_evidence(file, case, subjects, &own, &helper_paths); + let recheck = value_recheck(file, &id, &evidence); + let confirm = (!reaches_past_visibility(source)) + .then(|| value_confirm(file, &id, &evidence)) + .flatten(); out.units.push(UnitPlan { rule: TEST_VALUE, id: id.clone(), @@ -109,8 +113,10 @@ pub(super) fn plan_values( quote: None, lines: case.end_line + 1 - case.line, identity: identity(&[&case.name, &compact(source)]), - detail: Detail::Test, - recheck, + detail: Detail::Test { + confirm: confirm.map(Into::into), + }, + recheck: recheck.map(Into::into), }); items.push((out.units.len() - 1, id, case, test_item(case, source, ruby))); } @@ -126,21 +132,40 @@ pub(super) fn plan_values( for (unit, ..) in group { out.units[unit].presence = Presence::NeedsContext; out.units[unit].recheck = None; + out.units[unit].detail = Detail::Test { confirm: None }; } } } } -/// The hollow-test questions again for one test, with the bodies of the -/// functions it calls and its file's setup; none when there is nothing to add. -fn value_recheck( +/// One test with the bodies of the functions it calls and its file's setup, +/// and the files they come from: the evidence of its recheck and confirm. +struct Evidence { + state: Value, + sources: Vec<(PathBuf, String)>, + /// Whether it adds a body or setup to what the first pass showed. + adds: bool, + ruby: bool, +} + +impl Evidence { + fn request(&self, file: &FileContext<'_>, stage: &str, questions: Questions) -> (Value, Asked) { + let paths: Vec<(&Path, &str)> = self + .sources + .iter() + .map(|(path, hash)| (path.as_path(), hash.as_str())) + .collect(); + super::request(file.model, stage, &paths, self.state.clone(), questions) + } +} + +fn value_evidence( file: &FileContext<'_>, case: &TestCase, - id: &str, subjects: &Subjects<'_>, setup: &str, setup_paths: &[PathBuf], -) -> Option<(Value, Asked)> { +) -> Evidence { let mut sources = vec![(file.path.to_path_buf(), file.source_hash.to_string())]; for path in setup_paths { if let Some(hash) = subjects.hashes.get(path) @@ -151,9 +176,6 @@ fn value_recheck( } let listed = sourced_subjects(case, subjects, &mut sources); let sourced = listed.iter().any(|s| s.get("source").is_some()); - if !sourced && setup.is_empty() { - return None; - } let ruby = file.path.extension().is_some_and(|e| e == "rb"); let state = json!({ "file": file.plain_state(), @@ -161,12 +183,67 @@ fn value_recheck( "subjects": listed, "setup": setup, }); - let paths: Vec<(&Path, &str)> = sources - .iter() - .map(|(path, hash)| (path.as_path(), hash.as_str())) - .collect(); - let questions = recheck_questions(id, ruby); - let (request, asked) = super::request(file.model, "recheck", &paths, state, questions); + Evidence { + state, + sources, + adds: sourced || !setup.is_empty(), + ruby, + } +} + +/// The hollow-test questions again for one test, with the bodies of the +/// functions it calls and its file's setup; none when there is nothing to add. +fn value_recheck(file: &FileContext<'_>, id: &str, evidence: &Evidence) -> Option<(Value, Asked)> { + if !evidence.adds { + return None; + } + let (request, asked) = evidence.request(file, "recheck", recheck_questions(id, evidence.ruby)); + file.budget.fits(&request).then_some((request, asked)) +} + +/// Calls that reach past a language's visibility: reflection, a cast to +/// `any`, Ruby's `send(:…)` and `instance_variable_get`. +const BYPASSES: [&str; 12] = [ + "ReflectionClass", + "ReflectionProperty", + "ReflectionMethod", + "setAccessible(", + "getDeclaredField(", + "getDeclaredMethod(", + "BindingFlags.NonPublic", + "Whitebox.", + "ReflectionTestUtils.", + "as any)", + "instance_variable_get", + ".send(:", +]; + +/// Whether a test reads or calls members past its language's visibility. It +/// reads internals by the language's own definition, so an internal-details +/// consider on it is not asked what its assertions read: 4 of the 6 labeled +/// tests that did so were right, and the question read two reflected private +/// properties and two `(service as any)` fields as results or state. +fn reaches_past_visibility(source: &str) -> bool { + BYPASSES.iter().any(|b| source.contains(b)) +} + +/// What the test's assertions read, with the same evidence as its recheck. +fn value_confirm(file: &FileContext<'_>, id: &str, evidence: &Evidence) -> Option<(Value, Asked)> { + let mut questions = Questions::default(); + let kind = if evidence.ruby { + TestEvidence::RecheckGroups + } else { + TestEvidence::Recheck + }; + questions.ask( + "reads".into(), + questions::test_reads("tests[0].source", kind), + id, + TEST_VALUE, + "reads", + Pass::Locate, + ); + let (request, asked) = evidence.request(file, "locate", questions); file.budget.fits(&request).then_some((request, asked)) } @@ -626,9 +703,11 @@ pub(super) fn plan_pairs( table, unseen_setup: !ruby && a.suite != b.suite, identical, - confirm: confirm.filter(|(request, _)| fits && file.budget.fits(request)), + confirm: confirm + .filter(|(request, _)| fits && file.budget.fits(request)) + .map(Into::into), }, - recheck: recheck.filter(|_| fits), + recheck: recheck.filter(|_| fits).map(Into::into), }); if fits { requests.push(Planned { diff --git a/src/units/tests/comments.rs b/src/units/tests/comments.rs index a81e257..26a7a9e 100644 --- a/src/units/tests/comments.rs +++ b/src/units/tests/comments.rs @@ -170,6 +170,26 @@ fn an_undecided_comment_is_rechecked_then_settled_by_its_kind() { "{}", finding.message ); + // A kind that only leans decides as well: toward restating, a note. + let leaning = |restates: f64| { + let mut probabilities: serde_json::Map = + kinds.iter().map(|k| (k.to_string(), json!(0.0))).collect(); + probabilities.insert("restates".into(), json!(restates)); + probabilities.insert("summary".into(), json!(1.0 - restates)); + let choice = if restates > 0.5 { + "restates" + } else { + "summary" + }; + json!({"type":"choice","choice":choice,"confidence":0.5,"probabilities":probabilities}) + }; + eval.overrides = vec![("kind", leaning(0.6))]; + let report = run(&project, &options, &mut eval); + assert_eq!(report.files[0].findings[0].strength, Strength::Note); + eval.overrides = vec![("kind", leaning(0.4))]; + let report = run(&project, &options, &mut eval); + let dimension = &report.files[0].dimensions[catalog::COMMENTS]; + assert_eq!((dimension.units.clear, dimension.units.uncertain), (1, 0)); } #[test] diff --git a/src/units/tests/django.rs b/src/units/tests/django.rs index 20daa56..3b860ab 100644 --- a/src/units/tests/django.rs +++ b/src/units/tests/django.rs @@ -25,15 +25,14 @@ fn django_project() -> (Project, CheckArgs) { } /// The trace request of the injection unit of the function `name`. -fn injection_trace<'p>(plan: &'p Plan, name: &str) -> &'p Value { +fn injection_trace(plan: &Plan, name: &str) -> Value { plan.files .values() .flat_map(|f| &f.units) .find_map(|u| match &u.detail { Detail::Security { - trace: Some((request, _)), - .. - } if u.rule == catalog::INJECTION && u.name == name => Some(request), + trace: Some(trace), .. + } if u.rule == catalog::INJECTION && u.name == name => Some(trace.request()), _ => None, }) .unwrap_or_else(|| panic!("an injection trace for {name}")) @@ -53,7 +52,7 @@ fn django_views_are_asked_the_django_checks_and_other_python_the_common_ones() { let (project, options) = django_project(); let (_, plan) = planned(&project, &options); let view = injection_trace(&plan, "search"); - let questions = asked(view); + let questions = asked(&view); for check in ["redirect", "deserialize", "sql", "markup"] { assert!(questions.contains(&check.to_string()), "{questions:?}"); } @@ -64,7 +63,7 @@ fn django_views_are_asked_the_django_checks_and_other_python_the_common_ones() { .contains("RawSQL") ); let plain = injection_trace(&plan, "archive"); - let questions = asked(plain); + let questions = asked(&plain); assert!( !questions.contains(&"deserialize".to_string()), "{questions:?}" diff --git a/src/units/tests/documentation.rs b/src/units/tests/documentation.rs index b2a686d..e0ac8ec 100644 --- a/src/units/tests/documentation.rs +++ b/src/units/tests/documentation.rs @@ -9,7 +9,7 @@ fn instruction_sections_are_at_most_consider_and_name_their_harnesses() { project.write("web/app.ts", ""); project.write( "AGENTS.md", - "# Stack\nThis is a Rust project.\n\n# Web\nUse the design tokens in `web/theme.ts`.\n\n# Release\nTag with `v` then push.\n", + "# Stack\nThis is a Rust project: the library lives in `src/lib.rs` and the web client in `web/app.ts`.\n\n# Web\nUse the design tokens in `web/theme.ts`.\n\n# Release\nTag with `v` then push.\n", ); let mut options = args(); only(&mut options, catalog::AGENT_CONTEXT); @@ -56,6 +56,23 @@ fn instruction_sections_are_at_most_consider_and_name_their_harnesses() { assert!(file.findings[1].action.starts_with("Optional: move it")); let load = report.context_load.as_ref().unwrap(); assert!(load.harnesses.iter().any(|h| h.harness == "Codex")); + // A section of fewer than 15 tokens costs a session too little for a + // consider, whatever its answers. + let mut eval = scripted(0); + eval.overrides = vec![( + "s2_inferable", + json!({"type":"score","score":2.0,"confidence":1.0, + "probabilities":{"0":0.0,"1":0.0,"2":1.0}}), + )]; + options.refresh = true; + let report = run(&project, &options, &mut eval); + let release = report + .files + .iter() + .flat_map(|f| &f.findings) + .find(|f| f.symbol.as_deref() == Some("Release")) + .unwrap(); + assert_eq!(release.strength, Strength::Note, "{}", release.message); } fn git(project: &Project, args: &[&str]) { @@ -581,14 +598,34 @@ fn an_undecided_instruction_section_settles_by_its_kind() { assert_eq!(decided.units.consider, 1, "{}", decided.decision_basis); } -const DOCUMENT_KINDS: [&str; 5] = [ +const DOCUMENT_KINDS: [&str; 7] = [ "collection", "guide", "introduction", "migration", + "plan", "reference", + "requirements", ]; +#[test] +fn a_binary_document_is_skipped_without_blocking_the_run() { + let project = Project::new(); + project.write("docs/guide.md", "# Guide\n\nRun the app.\n"); + std::fs::write(project.0.join("docs/bootstrap.md"), b"# Bootstrap\n\0\0\n").unwrap(); + let mut options = args(); + only(&mut options, catalog::LARGE_DOCS); + let report = run(&project, &options, &mut scripted(0)); + assert!(report.complete, "{:?}", report.files); + let binary = report + .files + .iter() + .find(|f| f.path == std::path::Path::new("docs/bootstrap.md")) + .unwrap(); + assert_eq!(binary.status, Status::Skipped); + assert!(binary.error.as_ref().unwrap().contains("not judged")); +} + /// A long guide checked for large docs, its split answered with `split` /// and its kind, when asked, with `kind`. fn large_doc(split: Value, kind: Value) -> crate::schema::FileResult { @@ -627,7 +664,21 @@ fn an_undecided_large_document_is_asked_its_kind() { "{}", collection.findings[0].message ); - // A decided split is not asked its kind. + // A split finding is asked its kind: a plan for one release clears it, + // and a collection keeps it. + let found = || spread(0.1, 0.25, 0.65); + let plan = large_doc(found(), choice_of("plan", &DOCUMENT_KINDS)); + let dimension = &plan.dimensions[catalog::LARGE_DOCS]; + assert_eq!(dimension.units.clear, 1, "{}", dimension.decision_basis); + assert!( + plan.judgments.iter().all(|j| j.question != "part"), + "a cleared split is not located: {:?}", + plan.judgments + ); + let kept = large_doc(found(), choice_of("collection", &DOCUMENT_KINDS)); + assert_eq!(kept.findings.len(), 1); + assert_eq!(kept.findings[0].strength, Strength::Consider); + // A split that clears is not asked its kind. let decided = large_doc( spread(0.9, 0.1, 0.0), choice_of("collection", &DOCUMENT_KINDS), diff --git a/src/units/tests/functions.rs b/src/units/tests/functions.rs index b93442e..1e10640 100644 --- a/src/units/tests/functions.rs +++ b/src/units/tests/functions.rs @@ -17,6 +17,19 @@ fn a_review_function_carries_a_located_finding() { assert_eq!(crate::gate::exit_code(&report), 1); } +#[test] +fn a_block_ending_inside_a_character_is_located_by_its_line() { + // vnpy: a Python block holds its trailing comment, and the block's last + // byte fell inside the `线` that closes it. + let source = "def record(contract, engine):\n total = 0\n if contract.ready:\n total += 1\n engine.add(contract) # 录制分钟K线\n engine.flush()\n total *= 2\n return total\n"; + let (project, options) = project_with( + &[("record.py", source)], + &[catalog::FUNCTION_SIMPLIFICATION], + ); + let report = run(&project, &options, &mut scripted(2)); + assert_eq!(report.files[0].status, Status::Review); +} + const NESTED: &str = "fn nested(rows: &[Vec]) -> i32 {\n let mut total = 0;\n for row in rows {\n if !row.is_empty() {\n for value in row {\n if *value > 0 {\n total += value;\n }\n }\n }\n }\n total\n}\n"; #[test] diff --git a/src/units/tests/hardcoded.rs b/src/units/tests/hardcoded.rs index 7d71905..9bf07ed 100644 --- a/src/units/tests/hardcoded.rs +++ b/src/units/tests/hardcoded.rs @@ -103,6 +103,65 @@ fn a_local_default_is_a_note_and_a_special_case_is_a_review() { ); } +#[test] +fn a_value_that_needs_a_name_but_is_written_once_is_a_note() { + let findings = |source: &str| { + let (project, options) = rule_project(source, catalog::HARDCODED_VALUES); + let mut eval = scripted(0); + eval.overrides = vec![ + ("magic", spread(0.1, 0.35, 0.55)), + ("value", choice_of("v1", &["v0", "v1", "none"])), + ]; + run(&project, &options, &mut eval).files[0].findings.clone() + }; + // `300_000` holds `30_000` only as part of a longer number. + let once = findings(&format!("{HARDCODED}\nconst CAP: u64 = 300_000;\n")); + let connect = once + .iter() + .find(|f| f.symbol.as_deref() == Some("connect")) + .unwrap(); + assert_eq!(connect.strength, Strength::Note); + assert!( + connect + .message + .ends_with("It is written once in its file, so it is a note."), + "{}", + connect.message + ); + let twice = findings(&format!( + "{HARDCODED}\nfn backup() -> Client {{\n Client::new(\"db.backup:5432\", 30_000)\n}}\n" + )); + let connect = twice + .iter() + .find(|f| f.symbol.as_deref() == Some("connect")) + .unwrap(); + assert_eq!(connect.strength, Strength::Consider, "{}", connect.message); + // Naming a value is a cleanup: a review-level answer is at most a consider. + let (project, options) = rule_project( + &format!( + "{HARDCODED}\nfn backup() -> Client {{\n Client::new(\"db.backup:5432\", 30_000)\n}}\n" + ), + catalog::HARDCODED_VALUES, + ); + let mut eval = scripted(0); + eval.overrides = vec![ + ("magic", spread(0.0, 0.05, 0.95)), + ("value", choice_of("v1", &["v0", "v1", "none"])), + ]; + let report = run(&project, &options, &mut eval); + let connect = report.files[0] + .findings + .iter() + .find(|f| f.symbol.as_deref() == Some("connect")) + .unwrap(); + assert_eq!(connect.strength, Strength::Consider, "{}", connect.message); + assert!( + connect.message.contains("a reader must guess"), + "{}", + connect.message + ); +} + #[test] fn undecided_units_are_listed_with_the_questions_left_undecided() { let (project, mut options) = function_rule_project(&function("borderline")); diff --git a/src/units/tests/mod.rs b/src/units/tests/mod.rs index def8bf9..54643eb 100644 --- a/src/units/tests/mod.rs +++ b/src/units/tests/mod.rs @@ -315,9 +315,18 @@ fn run_with_nouls(project: &Project, options: &CheckArgs, nouls: &[(&'static str let mut eval = scripted(0); eval.overrides = nouls.iter().map(|&(q, p)| (q, noul_at(p))).collect(); eval.overrides.push(to_client()); + eval.overrides.push(logs_a_secret()); run(project, options, &mut eval) } +/// The settle Choice naming a secret among what a unit logs. +fn logs_a_secret() -> (&'static str, Value) { + let options = [ + "plain", "identity", "operator", "secret", "personal", "none", + ]; + ("logged", choice_of("secret", &options)) +} + /// The settle Choice sending a unit's text to a remote client. fn to_client() -> (&'static str, Value) { let probabilities = diff --git a/src/units/tests/organization.rs b/src/units/tests/organization.rs index b827c40..d9b0f14 100644 --- a/src/units/tests/organization.rs +++ b/src/units/tests/organization.rs @@ -47,10 +47,11 @@ fn an_uncertain_outline_is_rechecked_once_with_the_application_source() { Status::Clear ); let (_, plan) = planned(&project, &options); - let (request, _) = plan.files.values().next().unwrap().units[0] + let request = plan.files.values().next().unwrap().units[0] .recheck .as_ref() - .unwrap(); + .unwrap() + .request(); let sent = request["state"]["file"]["source"].as_str().unwrap(); assert!(sent.contains("fn warm0") && !sent.contains("hidden_check")); } @@ -71,26 +72,32 @@ fn an_undecided_recheck_is_decided_by_the_kind_of_file() { "the kind is its own request" ); options.refresh = true; - let mut eval = scripted(3); - let mut probabilities: serde_json::Map = - questions::outline_kind(false)["criteria"] - .as_object() - .unwrap() - .keys() - .map(|k| (k.clone(), json!(0.0))) - .collect(); - probabilities.insert("per_feature".into(), json!(0.85)); - probabilities.insert("algorithm".into(), json!(0.15)); - eval.recheck_overrides = vec![( - "kind", - json!({"type":"choice","choice":"per_feature","confidence":0.8,"probabilities":probabilities}), - )]; - let finding = &first_finding(&project, &options, &mut eval); + let kind = |choice: &str| { + let mut probabilities: serde_json::Map = + questions::outline_kind(false)["criteria"] + .as_object() + .unwrap() + .keys() + .map(|k| (k.clone(), json!(0.0))) + .collect(); + probabilities.insert(choice.into(), json!(0.85)); + probabilities.insert("algorithm".into(), json!(0.15)); + let mut eval = scripted(3); + eval.recheck_overrides = vec![( + "kind", + json!({"type":"choice","choice":choice,"confidence":0.8,"probabilities":probabilities}), + )]; + run(&project, &options, &mut eval) + }; + assert_eq!( + kind("per_feature").files[0].dimensions["file_organization"].status, + Status::Clear, + "the same kind of code written out per feature is one job" + ); + let finding = &kind("several").files[0].findings[0]; assert_eq!(finding.strength, Strength::Consider); assert!( - finding - .message - .contains("same kind of code for several features"), + finding.message.contains("several unrelated features"), "{}", finding.message ); @@ -119,13 +126,12 @@ fn an_outline_too_long_for_a_recheck_is_decided_by_its_kind_alone() { let unit = &plan.files.values().next().unwrap().units[0]; assert!(unit.recheck.is_none()); let Detail::Outline { - kind: Some((kind, _)), - .. + kind: Some(kind), .. } = &unit.detail else { panic!("the kind is asked from the outline"); }; - assert!(kind["state"]["file"]["source"].is_null()); + assert!(kind.request()["state"]["file"]["source"].is_null()); } #[test] @@ -227,10 +233,11 @@ fn test_files_are_outlined_by_suite_without_include_tests() { assert_eq!(report.files[0].findings[0].strength, Strength::Note); options.refresh = false; let (_, plan) = planned(&project, &options); - let (request, _) = plan.files.values().next().unwrap().units[0] + let request = plan.files.values().next().unwrap().units[0] .recheck .as_ref() - .unwrap(); + .unwrap() + .request(); let state = &request["state"]; assert_eq!(state["members"][0]["kind"], "test"); assert_eq!(state["members"][0]["suite"], "parse"); diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index c2e7ed8..fa921ba 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -392,13 +392,12 @@ 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, _)), - .. + trace: Some(trace), .. } = &trace.detail else { panic!("no trace planned"); }; - request["questions"].as_object().unwrap().clone() + trace.request()["questions"].as_object().unwrap().clone() } #[test] @@ -444,7 +443,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, false), + questions::security_interpreted("functions[0].source", false, false, None, false), "code that names no deserializer keeps its question and cached answer" ); assert!(traced_checks("shop/cart.py", PICKLED).contains_key("deserialize")); @@ -859,12 +858,12 @@ fn an_error_trace_shows_the_errors_the_called_functions_raise() { .position(|i| i.result.path.ends_with("api.py")) .unwrap(); let Detail::Security { - trace: Some((trace, _)), - .. + trace: Some(trace), .. } = &plan.files[&owner].units[0].detail else { panic!("a traced security unit"); }; + let trace = trace.request(); assert_eq!( trace["state"]["errors_created_by_functions_it_calls"], json!([{"function": "find_asset", "error": "LookupError", "message": "\"Asset not found\""}]) @@ -879,18 +878,18 @@ fn an_error_trace_shows_the_errors_the_called_functions_raise() { .contains("errors_created_by_functions_it_calls") }; assert!( - names_callees(trace), + names_callees(&trace), "passing on a callee's own error text is the program's own" ); let alone = project_with(&[("app/api.py", HANDLER)], &[catalog::SENSITIVE_DATA]); let (_, plan) = planned(&alone.0, &alone.1); let Detail::Security { - trace: Some((trace, _)), - .. + trace: Some(trace), .. } = &plan.files[&0].units[0].detail else { panic!("a traced security unit"); }; + let trace = trace.request(); assert!( trace["state"] .get("errors_created_by_functions_it_calls") @@ -902,7 +901,7 @@ fn an_error_trace_shows_the_errors_the_called_functions_raise() { .contains("errors_created_by_functions_it_calls"), "without callee errors the check is asked as before" ); - assert!(trace["questions"].get("messages").is_some() && !names_callees(trace)); + assert!(trace["questions"].get("messages").is_some() && !names_callees(&trace)); } 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"; @@ -915,13 +914,14 @@ fn each_created_error_message_is_asked_about_and_names_the_foreign_one() { options.rules = vec![catalog::SENSITIVE_DATA.into()]; let (_, plan) = planned(&project, &options); let Detail::Security { - trace: Some((trace, _)), + trace: Some(trace), messages, .. } = &plan.files[&0].units[0].detail else { panic!("a traced security unit"); }; + let trace = trace.request(); assert_eq!( messages, &["`Query failed: ${error.message}`", "'Thing not found'"] @@ -979,15 +979,14 @@ fn an_injection_trace_shows_the_enums_its_sites_name() { let mut options = args(); options.rules = vec![catalog::INJECTION.into()]; let (_, plan) = planned(&project, &options); - let traces: Vec<&Value> = plan + let traces: Vec = plan .files .values() .flat_map(|f| &f.units) .filter_map(|u| match &u.detail { Detail::Security { - trace: Some((request, _)), - .. - } => Some(request), + trace: Some(trace), .. + } => Some(trace.request()), _ => None, }) .collect(); @@ -1110,6 +1109,127 @@ fn a_csharp_setup_trace_shows_the_constants_it_names_and_finds_a_key_written_in_ ); } +/// An unsafe-settings run over one file, with `nouls` answered and an +/// optional settle Choice: its report and the requests sent. +fn settings_run( + path: &str, + source: &str, + nouls: &[(&'static str, f64)], + settle: Option<(&'static str, Value)>, +) -> (Report, Vec) { + let project = Project::new(); + project.write(path, source); + let mut options = args(); + options.rules = vec![catalog::UNSAFE_SETTINGS.into()]; + let mut eval = recording(nouls); + eval.inner.overrides.extend(settle); + let report = run(&project, &options, &mut eval); + (report, eval.requests) +} + +/// The questions of the trace among `requests`. +fn trace_questions(requests: &[Value]) -> serde_json::Map { + requests + .iter() + .find(|r| r["jevgate"]["stage"] == "trace") + .unwrap()["questions"] + .as_object() + .unwrap() + .clone() +} + +#[test] +fn code_outside_csharp_and_django_is_asked_about_tokens_keys_and_escaping() { + let (report, requests) = settings_run( + "server.js", + "const session = require('express-session');\nconst app = require('express')();\napp.use(session({ secret: 'keyboard cat', resave: true, saveUninitialized: true }));\napp.listen(9090);\n", + &[("weakened", 0.95), ("key", 0.95)], + None, + ); + let questions = trace_questions(&requests); + for check in ["token", "key", "escape", "hash", "cookie"] { + assert!(questions[check].is_object(), "{check}"); + } + let finding = &report.files[0].findings[0]; + assert_eq!(finding.strength, Strength::Review); + assert_eq!( + finding.category.as_deref(), + Some("CWE-321 hard-coded cryptographic key") + ); + // C# asks its own wording of the token check, once. + let (_, requests) = settings_run( + "Program.cs", + "var builder = WebApplication.CreateBuilder(args);\nbuilder.Services.AddCors(o => o.AddDefaultPolicy(p => p.AllowAnyOrigin()));\nvar app = builder.Build();\napp.Run();\n", + &[("weakened", 0.95)], + None, + ); + assert!( + trace_questions(&requests)["token"] + .to_string() + .contains("ValidateIssuerSigningKey") + ); +} + +#[test] +fn a_token_the_code_only_passes_on_is_no_review() { + const USES: [&str; 7] = [ + "verifies", + "passes", + "verified_before", + "reads_claims", + "decides_access", + "turned_off", + "none", + ]; + let strength = |choice: &str| { + let (report, requests) = settings_run( + "src/useAuth.ts", + "export function useAuth() {\n const token = localStorage.getItem('access_token');\n return fetch('/api/me', { headers: { Authorization: `Bearer ${token}` } });\n}\n", + &[("weakened", 0.95), ("token", 0.9)], + Some(("token_use", choice_of(choice, &USES))), + ); + assert!( + requests + .iter() + .any(|r| r["questions"]["token_use"].is_object()), + "asked although the check found a concern" + ); + report.files[0].findings.first().map(|f| f.strength) + }; + assert_eq!(strength("turned_off"), Some(Strength::Review)); + assert_eq!( + strength("decides_access"), + Some(Strength::Consider), + "whether a token was verified before lies outside the function" + ); + assert_eq!(strength("reads_claims"), Some(Strength::Note)); + assert_eq!( + strength("passes"), + Some(Strength::Note), + "the broad answer alone names no setting" + ); +} + +#[test] +fn a_password_saved_as_plain_text_is_a_consider_and_one_hashed_fast_a_review() { + const HANDLING: [&str; 4] = ["slow_hash", "plain", "fast_hash", "none"]; + let strength = |choice: &str| { + let (report, _) = settings_run( + "src/users.ts", + "export async function register(repo, name, password) {\n const user = repo.create({ name, password });\n await repo.save(user);\n return user;\n}\n", + &[("weakened", 0.95), ("hash", 0.9)], + Some(("password_handling", choice_of(choice, &HANDLING))), + ); + report.files[0].findings.first().map(|f| f.strength) + }; + assert_eq!(strength("fast_hash"), Some(Strength::Review)); + assert_eq!( + strength("plain"), + Some(Strength::Consider), + "a callee or model hook may hash what the function saves" + ); +} + #[test] fn a_csharp_type_named_by_input_is_an_injection_named_by_its_own_check() { let project = Project::new(); @@ -1224,7 +1344,9 @@ fn undecided_markup_cors_cookies_and_logged_objects_are_settled_by_their_choices "{chosen}" ); } - let logs = ["plain", "secret", "personal", "none"]; + let logs = [ + "plain", "identity", "operator", "secret", "personal", "none", + ]; let undecided_logs = [("logs_secret", 0.4), ("logs_object_secret", 0.4)]; for (chosen, status) in [("plain", Status::Clear), ("secret", Status::Uncertain)] { options.refresh = true; @@ -1244,6 +1366,26 @@ fn undecided_markup_cors_cookies_and_logged_objects_are_settled_by_their_choices } } +#[test] +fn an_audit_line_naming_who_signed_in_is_no_logged_personal_data() { + let (project, mut options) = security_project(QUERY); + let logs = [ + "plain", "identity", "operator", "secret", "personal", "none", + ]; + let found = [("logs_secret", 0.92)]; + for (chosen, status) in [("identity", Status::Clear), ("secret", Status::Review)] { + options.refresh = true; + let settle = ("logged", choice_of(chosen, &logs)); + let (outcome, settles) = + settled_status(&project, &options, catalog::SENSITIVE_DATA, &found, settle); + assert_eq!( + settles, 1, + "asked although the presence question found a concern" + ); + assert_eq!(outcome, status, "{chosen}"); + } +} + #[test] fn a_decided_check_asks_no_settle_choice() { let (project, options) = security_project(QUERY); @@ -1285,3 +1427,113 @@ fn a_function_added_to_one_run_is_the_only_security_request_asked_again() { assert_eq!(sizes, [3, 3, 4, 5]); only_changed(&before, &after, 1); } + +#[test] +fn a_server_template_is_judged_by_its_inline_scripts_only() { + let project = Project::new(); + project.write( + "app/views/sessions/new.html.erb", + "

<%= t('login') %>

\n\n", + ); + project.write( + "app/views/users/show.html.erb", + "

<%= raw @user.bio %>

\n", + ); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into()]; + let (inputs, plan) = planned(&project, &options); + let paths: Vec<_> = inputs.iter().map(|i| i.result.path.clone()).collect(); + assert_eq!( + paths, + [std::path::PathBuf::from("app/views/sessions/new.html.erb")], + "a template without inline scripts is not selected" + ); + let request = &plan.requests[0].request; + assert_eq!(request["state"]["file"]["language"], "JavaScript"); + assert!( + request["state"]["file"]["framework"] + .as_str() + .unwrap() + .contains("runs in the visitor's browser") + ); + let page = &request["state"]["functions"][0]; + assert_eq!(page["name"], "top-level code"); + assert!( + page["source"].as_str().unwrap().contains("document.write("), + "{page}" + ); +} + +#[test] +fn a_node_handler_is_sent_the_unescaped_lines_of_the_view_it_renders() { + let project = Project::new(); + project.write( + "app.js", + "const express = require('express');\nconst app = express();\n\nfunction search(req, res) {\n const term = req.query.q;\n res.render('shop/products', { term });\n}\n\napp.get('/search', search);\n", + ); + project.write( + "views/shop/products.ejs", + "<%- include('../head') %>\n

Results for <%- term %>

\n

<%= term %>

\n", + ); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into()]; + let (_, plan) = planned(&project, &options); + let first = &plan.requests[0].request; + let search = &first["state"]["functions"][0]; + assert_eq!( + search["templates_it_renders_that_write_values_without_escaping"], + json!([{"template": "views/shop/products.ejs", "unescaped_output": ["2:

Results for <%- term %>

"]}]) + ); + let interpreted = first["questions"]["f0_interpreted"]["criteria"]["true"] + .as_str() + .unwrap(); + assert!( + interpreted.contains("passed to a template that writes it unescaped"), + "{interpreted}" + ); + let unit = plan.files[&0] + .units + .iter() + .find(|u| u.rule == catalog::INJECTION) + .unwrap(); + let Detail::Security { + trace: Some(trace), .. + } = &unit.detail + else { + panic!("a traced injection unit"); + }; + let markup = trace.request()["questions"]["markup"]["instructions"]["question"].clone(); + assert!( + markup + .as_str() + .unwrap() + .contains("through a template it renders"), + "{markup}" + ); +} + +#[test] +fn a_template_writing_client_data_unescaped_is_judged_as_template_code() { + let project = Project::new(); + project.write( + "app/views/layouts/application.html.erb", + "\n\n

<%= @title %>

\n\n", + ); + let mut options = args(); + options.rules = vec![catalog::INJECTION.into(), catalog::SENSITIVE_DATA.into()]; + let (inputs, plan) = planned(&project, &options); + assert_eq!(inputs.len(), 1, "selected for its template code alone"); + let names: Vec<(&str, &str)> = plan.files[&0] + .units + .iter() + .map(|u| (u.rule, u.name.as_str())) + .collect(); + assert_eq!( + names, + [(catalog::INJECTION, "template code")], + "an ERB tag is judged for what it writes" + ); + let code = &plan.requests[0].request["state"]["functions"][0]; + assert_eq!(code["source"], "<%= raw cookies[:font] %>"); + assert_eq!(plan.files[&0].units[0].locations[0].start_line, 2); +} diff --git a/src/units/tests/test_rules.rs b/src/units/tests/test_rules.rs index 07b733e..4bdbe2e 100644 --- a/src/units/tests/test_rules.rs +++ b/src/units/tests/test_rules.rs @@ -58,7 +58,7 @@ fn an_undecided_test_pair_is_asked_again_with_the_body_of_its_subject() { .iter() .find(|u| u.rule == catalog::TEST_REDUNDANCY) .unwrap(); - let (request, _) = pair.recheck.as_ref().expect("a recheck"); + let request = pair.recheck.as_ref().expect("a recheck").request(); assert_eq!(request["jevgate"]["stage"], "recheck"); assert!( request["state"]["subject"]["source"] @@ -119,7 +119,7 @@ fn an_undecided_test_is_asked_again_with_its_subjects_and_setup() { ); let (_, plan) = planned(&project, &options); let file = file_plan(&plan, "profile.test.ts"); - let (request, _) = file.units[0].recheck.as_ref().expect("a recheck"); + let request = file.units[0].recheck.as_ref().expect("a recheck").request(); let state = &request["state"]; assert!( state["subjects"][0]["source"] @@ -200,7 +200,7 @@ fn a_ruby_test_is_rechecked_with_its_groups_the_setup_it_reads_and_its_helpers() let file = file_plan(&plan, "invoice_spec.rb"); let first = first_request(&plan, "tests"); assert_eq!(first["state"]["tests"][0]["suite"], "Invoice"); - let (request, _) = file.units[0].recheck.as_ref().expect("a recheck"); + let request = file.units[0].recheck.as_ref().expect("a recheck").request(); let setup = request["state"]["setup"].as_str().unwrap(); assert_eq!( setup, @@ -339,7 +339,7 @@ fn a_mockmvc_test_is_rechecked_with_the_controller_method_its_request_reaches() .flat_map(|f| &f.units) .find(|u| u.name == "showsOwner") .and_then(|u| u.recheck.as_ref()) - .map(|(request, _)| request["state"]["subjects"].clone()) + .map(|recheck| recheck.request()["state"]["subjects"].clone()) .unwrap(); let subjects = recheck.as_array().unwrap(); assert_eq!(subjects.len(), 1, "{subjects:?}"); @@ -459,3 +459,65 @@ fn copies_inside_tests_a_redundancy_finding_names_are_reported_once() { .collect(); assert_eq!(rules, [catalog::id(catalog::TEST_REDUNDANCY)], "{rules:?}"); } + +#[test] +fn an_internal_details_consider_is_confirmed_by_what_its_assertions_read() { + let (project, mut options) = tests_project(&[("lib.rs", TESTS)], catalog::TEST_VALUE); + let reads = ["effects", "own_calls", "result", "state", "stored"]; + let mut judged = |reads: Value| { + let mut eval = scripted(0); + eval.overrides + .push(("internal", json!({"type":"noul","noul":0.9}))); + eval.overrides.push(("reads", reads)); + let report = run(&project, &options, &mut eval); + options.refresh = true; + report.files[0].dimensions["test_value"].clone() + }; + // Spies on the program's own helpers keep the consider. + let own = judged(choice_of("own_calls", &reads)); + assert_eq!(own.units.consider, 3, "{}", own.decision_basis); + // State the program shows or acts on next is what a caller observes. + let state = judged(choice_of("state", &reads)); + assert_eq!(state.units.clear, 3, "{}", state.decision_basis); + // Leaning toward what a caller observes, without reaching it: a note. + let mut split: serde_json::Map = + reads.iter().map(|k| (k.to_string(), json!(0.0))).collect(); + split.insert("state".into(), json!(0.6)); + split.insert("stored".into(), json!(0.4)); + let leaning = + judged(json!({"type":"choice","choice":"state","confidence":0.5,"probabilities":split})); + assert_eq!(leaning.units.note, 3, "{}", leaning.decision_basis); +} + +#[test] +fn a_test_that_reaches_past_visibility_keeps_its_internal_details_consider() { + let reflected = TESTS.replace( + " assert_eq!(total(&values), 3);\n", + " let field = ReflectionClass::new(\"Totals\");\n assert_eq!(total(&values), 3);\n", + ); + let (project, options) = tests_project(&[("lib.rs", &reflected)], catalog::TEST_VALUE); + let (_, plan) = planned(&project, &options); + let confirmed: Vec = plan.files[&0] + .units + .iter() + .map(|u| matches!(u.detail, Detail::Test { confirm: Some(_) })) + .collect(); + assert_eq!( + confirmed, + [false, true, true], + "only the reflecting test skips it" + ); + let mut eval = scripted(0); + eval.overrides + .push(("internal", json!({"type":"noul","noul":0.9}))); + let reads = ["effects", "own_calls", "result", "state", "stored"]; + eval.overrides.push(("reads", choice_of("state", &reads))); + let report = run(&project, &options, &mut eval); + let dimension = &report.files[0].dimensions["test_value"]; + assert_eq!( + (dimension.units.consider, dimension.units.clear), + (1, 2), + "{}", + dimension.decision_basis + ); +} diff --git a/src/units/wording/documentation.rs b/src/units/wording/documentation.rs index 6f00385..544e442 100644 --- a/src/units/wording/documentation.rs +++ b/src/units/wording/documentation.rs @@ -155,14 +155,14 @@ pub(in crate::units) fn document_wording( ) }; } - let likely = if strength == Strength::Note { - " may" + let records = if strength == Strength::Note { + "may mainly record" } else { - "" + "mainly records" }; ( format!( - "`{name}`{likely} mainly records past work, such as dated plans, completed tasks or logs{}.", + "`{name}` {records} past work, such as dated plans, completed tasks or logs{}.", shown(strength, p) ), "Remove finished plans and logs, or move them out of the living documentation", diff --git a/src/units/wording/maintainability.rs b/src/units/wording/maintainability.rs index fc0a30f..4d854cf 100644 --- a/src/units/wording/maintainability.rs +++ b/src/units/wording/maintainability.rs @@ -124,9 +124,6 @@ pub(in crate::units) fn outline_wording( ), Strength::Consider => ( match several { - Some("per_feature") => format!( - "This file writes out the same kind of code for several features ({p:.2}); each feature's part would be easier to find in its own {kind}.{detail}" - ), Some(_) => format!("This file holds several unrelated features ({p:.2}).{detail}"), None => format!( "Some {parts} of this file could move to a separate {kind} ({p:.2}).{detail}" @@ -244,15 +241,15 @@ const VALUE_SIGNALS: [ValueSignal; 3] = [ pub(in crate::units) fn values_wording( name: &str, detail: &Detail, - (strength, unnamed): (Strength, Option), + (strength, lowered): (Strength, Option), p: f64, answers: &Answers<'_>, ) -> Wording { let get = |q: &str| answers.get(q).copied(); let signals = value_signals(&get, detail, true).unwrap_or_default(); - // An unnamed value's finding is lower than the strength its signals reached. - let reached_at = unnamed.unwrap_or(strength); - let unnamed = unnamed.is_some(); + // A lowered finding, such as one whose value was not named, is lower + // than the strength its signals reached. + let reached_at = lowered.unwrap_or(strength); let reached: Vec<(&ValueSignal, bool)> = signals .iter() .filter(|(_, outcome, _)| { @@ -290,14 +287,9 @@ pub(in crate::units) fn values_wording( } else { "" }; - let unnamed = if unnamed { - " No single value stood out, so it is a note." - } else { - "" - }; ( format!( - "{subject}{likely} {}{}.{unnamed}", + "{subject}{likely} {}{}.", reasons.join("; "), shown(strength, p) ), diff --git a/src/units/wording/security.rs b/src/units/wording/security.rs index 133cb43..0fe98fb 100644 --- a/src/units/wording/security.rs +++ b/src/units/wording/security.rs @@ -18,7 +18,7 @@ const PRIVILEGE: [(&str, &str, &str, &str); 7] = [ ), ( "unchecked", - "reads or changes other users' rows without checking the caller", + "reads or changes other users' rows or files without checking the caller", "CWE-862 missing authorization", "Check `auth.uid()` or a role in the function, or make it SECURITY INVOKER", ), @@ -146,7 +146,7 @@ const INJECTIONS: [(&str, &str, &str, &str); 12] = [ ]; /// Weak settings: what the code does, its weakness and remedy. -const SETTINGS: [(&str, &str, &str, &str); 12] = [ +const SETTINGS: [(&str, &str, &str, &str); 13] = [ ( "tls", "turns off certificate or signature verification", @@ -155,8 +155,8 @@ const SETTINGS: [(&str, &str, &str, &str); 12] = [ ), ( "hash", - "hashes passwords with a fast or broken hash", - "CWE-916 weak password hash", + "keeps passwords as plain text or hashes them with a fast or broken hash", + "CWE-256 plaintext password or CWE-916 weak password hash", "Hash passwords with Argon2, bcrypt or scrypt", ), ( @@ -201,6 +201,12 @@ const SETTINGS: [(&str, &str, &str, &str); 12] = [ "CWE-200 secret exposed to browsers", "Read the secret from a variable without the public prefix, only in server code, and rotate it", ), + ( + "escape", + "turns off the escaping of values written into HTML", + "CWE-79 cross-site scripting", + "Keep automatic escaping on and mark only values that are already safe HTML as raw", + ), ( "csrf", "turns off cross-site request forgery protection for requests that change data", diff --git a/src/units/workflows.rs b/src/units/workflows.rs index 4539a73..e6602d9 100644 --- a/src/units/workflows.rs +++ b/src/units/workflows.rs @@ -54,7 +54,9 @@ pub(super) fn plan(file: &FileContext<'_>, out: &mut FilePlan, requests: &mut Ve lines: job.end_line + 1 - job.start_line, identity: identity(&[&id, &compact(&job.source)]), detail: Detail::Job { expressions }, - recheck: recheck.filter(|(request, _)| file.budget.fits(request)), + recheck: recheck + .filter(|(request, _)| file.budget.fits(request)) + .map(Into::into), }); if fits { requests.push(Planned {