From 0de986a022a6a9f2b8a8f1da88fb7a95be23d958 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Fri, 29 May 2026 11:05:49 +0800 Subject: [PATCH 1/2] Follow Makefile includes when completing make targets The `make` `list_targets` generator ran `cat [Mm]akefile`, which reads only the top-level Makefile, so targets defined in files pulled in via `include`/`-include`/`sinclude` were never suggested (warpdotdev/warp#11705). Replace the command with a POSIX-sh `visit()` walker over the include graph: it `cat`s a file, uses a small awk program to extract that file's include paths, and recurses into each. awk only extracts paths (it never opens them) and every path is passed to the shell as a quoted argument, never interpolated, so a Makefile cannot inject commands. The extractor only matches `include` directives indented with spaces, not a leading tab -- a tab-indented line is a make recipe (a shell command), so a tab-indented `include`-looking line is deliberately not followed -- and strips a trailing `# comment` before splitting so comment words are not mistaken for include paths. Confine include-following to the project subtree so a hostile repository cannot make tab-completion read files outside the working tree (e.g. `include /etc/passwd`) before the user runs `make`, using two layers: - a lexical `safe()` guard that skips absolute, home-relative, and `..`-escaping include paths; and - `realpath`, which canonicalizes each remaining candidate so it is followed only when it resolves under `$(pwd -P)`. This stops escapes a lexical check cannot, e.g. an in-tree symlink (`evil -> /etc`) used as `include evil/passwd`. GNU make resolves include paths relative to its working directory, so legitimate project-local includes are unaffected. A `seen` set of canonical paths guards against include cycles. If `realpath` is unavailable the command falls back to `cat [Mm]akefile` (top-level only) rather than following includes unsafely. Glob (`include dir/*.mk`), variable (`include $(VAR)`), and whitespace-containing include paths are intentionally left unresolved. Add end-to-end tests run against temp projects: nested includes plus a missing optional include (asserting `##` descriptions survive and a trailing-comment operand is not followed), a directory include, an absolute and a `..`-escaping include pointing at real out-of-tree sentinels, a tab-indented (recipe) `include`-looking line, and an in-tree symlink resolving out of tree -- asserting only in-tree targets surface in each. --- command-signatures/src/generators/make.rs | 322 +++++++++++++++++++++- 1 file changed, 321 insertions(+), 1 deletion(-) diff --git a/command-signatures/src/generators/make.rs b/command-signatures/src/generators/make.rs index 327b554d..8a022a8e 100644 --- a/command-signatures/src/generators/make.rs +++ b/command-signatures/src/generators/make.rs @@ -80,11 +80,48 @@ fn list_targets_post_process(output: &str) -> GeneratorResults { .collect_unordered_results() } +/// Shell command that prints the root Makefile plus every file it pulls in through `include`, +/// `-include`, and `sinclude` directives, so that targets split across included files are +/// surfaced (GNU make treats them as one unified ruleset). +/// +/// A POSIX-sh `visit()` function walks the include graph: it `cat`s a file, then uses a small +/// `awk` program to extract that file's include paths and recurses into each. `awk` only extracts +/// the paths (it never opens them), and every path is passed to the shell as a quoted argument — +/// never interpolated into a command — so a malicious Makefile cannot inject commands. The awk +/// extractor only matches `include`/`-include`/`sinclude` directives indented with **spaces**, not +/// a leading tab: in make a tab-indented line is a recipe (a shell command), so a tab-indented +/// `include`-looking line is deliberately not followed. A trailing `# comment` on the directive is +/// stripped before the operands are split, so comment words are not mistaken for include paths. +/// +/// Security boundary: completion runs against whatever repository the user has `cd`'d into, which +/// may be untrusted. Containment is enforced in two layers so a hostile Makefile cannot make +/// tab-completion read files outside the working tree before the user ever runs `make`: +/// 1. `safe()` (lexical) skips absolute (`/etc/...`), home-relative (`~/...`), and `..`-escaping +/// include paths outright. +/// 2. `realpath` (symlink-resolving) canonicalizes each remaining candidate and the `case` guard +/// follows it only when it stays under `$root` (`pwd -P`). This stops escapes a lexical check +/// cannot — e.g. a repo that ships `evil -> /etc` and does `include evil/passwd`, where the +/// path is lexically in-tree but resolves outside it. +/// +/// GNU make resolves include paths relative to its working directory (not the including file), so a +/// legitimate project-local include is always a relative, non-`..` path resolving under the root +/// and is unaffected. A `seen` set of canonical paths guards against include cycles. +/// +/// `realpath` is required for the symlink-resolving layer; if it is unavailable the command falls +/// back to `cat [Mm]akefile` (top-level targets only) rather than following includes unsafely. +/// +/// Known limitation: include paths that rely on globbing (`include dir/*.mk`), make variables +/// (`include $(VAR)`), or that contain whitespace are not expanded/followed — resolving them +/// safely would require a shell (injection risk) or evaluating the Makefile. Absolute, +/// home-relative, `..`-escaping, and symlink-escaping includes are intentionally not followed (see +/// the security boundary above); their targets are simply not surfaced in completion. +const LIST_TARGETS_COMMAND: &str = r##"root=$(pwd -P)||exit 0;command -v realpath >/dev/null 2>&1||{ cat [Mm]akefile 2>/dev/null;exit 0;};seen="|";visit(){ rp=$(realpath -- "$1" 2>/dev/null)||return 0;case "$rp" in "$root"|"$root"/*) ;; *) return 0;; esac;case "$seen" in *"|$rp|"*) return 0;; esac;seen="$seen$rp|";cat -- "$rp" 2>/dev/null;set -f;for inc in $(awk 'function safe(p){return p!~/^\//&&p!~/^~/&&p!~/(^|\/)\.\.(\/|$)/} /^ *[-s]?include[ \t]+/{match($0,/^ *[-s]?include[ \t]+/);rest=substr($0,RLENGTH+1);sub(/#.*/,"",rest);n=split(rest,parts,/[ \t]+/);for(i=1;i<=n;i++)if(parts[i]!=""&&safe(parts[i]))print parts[i]}' "$rp" 2>/dev/null);do set +f;visit "$inc";set -f;done;set +f;};for f in [Mm]akefile;do [ -e "$f" ]&&visit "$f";done"##; + pub fn generator() -> CommandSignatureGenerators { CommandSignatureGenerators::new("make").add_generator( "list_targets", Generator::script( - CommandBuilder::single_command("cat [Mm]akefile"), + CommandBuilder::single_command(LIST_TARGETS_COMMAND), list_targets_post_process, ), ) @@ -162,4 +199,287 @@ dmg: $(DMG_NAME)-native"#; .collect::>() ); } + + /// Runs the real generator command via `sh -c` against a temp project and returns the + /// resulting `(target, description)` pairs. + fn run_list_targets_in(files: &[(&str, &str)]) -> Vec<(String, Option)> { + use super::LIST_TARGETS_COMMAND; + use std::process::Command; + use std::sync::atomic::{AtomicU64, Ordering}; + use std::time::{SystemTime, UNIX_EPOCH}; + + // Unique per call so the tests can run in parallel without sharing a temp dir. + static COUNTER: AtomicU64 = AtomicU64::new(0); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("cs_make_include_test_{nanos}_{seq}")); + for (path, contents) in files { + let full = dir.join(path); + std::fs::create_dir_all(full.parent().unwrap()).unwrap(); + std::fs::write(full, contents).unwrap(); + } + + let output = Command::new("sh") + .arg("-c") + .arg(LIST_TARGETS_COMMAND) + .current_dir(&dir) + .output() + .expect("failed to run list_targets command"); + + std::fs::remove_dir_all(&dir).ok(); + + assert!( + output.status.success(), + "command failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8(output.stdout).unwrap(); + list_targets_post_process(&stdout) + .suggestions + .into_iter() + .map(|suggestion| (suggestion.exact_string, suggestion.description)) + .collect() + } + + /// End-to-end test of the `list_targets` command: builds a temp project whose targets are + /// split across `include`d files (one reached via a nested include, plus a missing optional + /// `-include`), runs the actual generator command, and feeds its output through + /// `list_targets_post_process`. Guards against the regression in #11705 where only top-level + /// Makefile targets were suggested, and confirms `##` descriptions survive the include path. + #[test] + fn test_list_targets_command_follows_includes() { + let results = run_list_targets_in(&[ + ( + // The trailing `# makefiles/leak.mk` is a Make comment, not a second operand: it + // must be stripped before splitting so the comment word is not followed as an + // include path (the file exists below precisely to catch that regression). + "Makefile", + "include makefiles/main.mk # makefiles/leak.mk\n-include does-not-exist.mk\n\nhelp: ## root help\n\t@echo root\n", + ), + ( + "makefiles/main.mk", + "include makefiles/nested.mk\n\nup-main: ## start\n\tdocker compose up -d\ndown-main:\n\tdocker compose down\n", + ), + ( + "makefiles/nested.mk", + "nested-target: ## from a nested include\n\t@echo nested\n", + ), + ( + "makefiles/leak.mk", + "comment-leaked: ## must not surface\n\t@echo bad\n", + ), + ]); + + for expected in ["help", "up-main", "down-main", "nested-target"] { + assert!( + results.iter().any(|(target, _)| target == expected), + "expected target `{expected}` reached through includes, got {results:?}" + ); + } + + // The trailing-comment operand must not have been followed. + assert!( + !results.iter().any(|(target, _)| target == "comment-leaked"), + "a word in a trailing `#` comment must not be followed as an include, got {results:?}" + ); + + // A `##` description defined in an included file must survive the include expansion. + assert!( + results + .iter() + .any(|(target, description)| target == "nested-target" + && description.as_deref() == Some("from a nested include")), + "expected `nested-target` to keep its included `##` description, got {results:?}" + ); + } + + /// An `include` directive pointing at a real directory is an invalid Makefile that `make` + /// itself rejects. The generator resolves it (it is in-tree) but `cat`/`awk` on a directory + /// produce no targets and no error reaches stdout, so the command must still exit cleanly and + /// surface the top-level targets rather than breaking or returning nothing. + #[test] + fn test_list_targets_command_survives_directory_include() { + let results = run_list_targets_in(&[ + ( + "Makefile", + "include some_dir\n\nhelp: ## root help\n\t@echo root\nbuild:\n\t@echo build\n", + ), + ("some_dir/.keep", ""), + ]); + + let targets: Vec<&str> = results.iter().map(|(target, _)| target.as_str()).collect(); + assert!( + targets.contains(&"help") && targets.contains(&"build"), + "expected top-level targets to survive a directory include, got {targets:?}" + ); + } + + /// Security: completion may run inside an untrusted repository. A hostile Makefile must not be + /// able to make tab-completion read and parse files outside the working tree via an absolute, + /// home-relative, or `..`-escaping `include`. This builds a project whose Makefile tries the + /// absolute and parent-traversal escape vectors against real sentinel files placed *outside* + /// the project dir, and asserts none of their targets surface while the in-tree target does. + #[test] + fn test_list_targets_command_ignores_includes_outside_project() { + use std::time::{SystemTime, UNIX_EPOCH}; + + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + // Sentinels outside the project dir: one reached by absolute path, one via `..`. They are + // real, readable files so the test proves the generator *chose* not to follow them rather + // than them merely being absent. Unique names keep parallel test runs from colliding. + let abs_sentinel = std::env::temp_dir().join(format!("cs_make_secret_abs_{nanos}.mk")); + let parent_name = format!("cs_make_secret_parent_{nanos}.mk"); + let parent_sentinel = std::env::temp_dir().join(&parent_name); + std::fs::write( + &abs_sentinel, + "leaked-abs: ## must not surface\n\t@echo x\n", + ) + .unwrap(); + std::fs::write( + &parent_sentinel, + "leaked-parent: ## must not surface\n\t@echo y\n", + ) + .unwrap(); + + // cwd at runtime is the project dir under temp_dir, so `../` resolves to the parent + // sentinel sitting beside it in temp_dir. + let makefile = format!( + "include {abs}\ninclude ../{parent}\n\nhelp: ## root help\n\t@echo root\n", + abs = abs_sentinel.display(), + parent = parent_name, + ); + let results = run_list_targets_in(&[("Makefile", makefile.as_str())]); + + std::fs::remove_file(&abs_sentinel).ok(); + std::fs::remove_file(&parent_sentinel).ok(); + + let targets: Vec<&str> = results.iter().map(|(target, _)| target.as_str()).collect(); + assert!( + targets.contains(&"help"), + "in-tree target must still surface, got {targets:?}" + ); + assert!( + !targets.contains(&"leaked-abs"), + "absolute include outside the tree must not be followed, got {targets:?}" + ); + assert!( + !targets.contains(&"leaked-parent"), + "`..`-escaping include must not be followed, got {targets:?}" + ); + } + + /// A line beginning with a tab is a recipe line (a shell command), not an `include` directive: + /// GNU make only treats `include` as a directive when it is not recipe-indented. A hostile + /// Makefile could otherwise hide an `include`-looking recipe line inside a target body to make + /// completion follow a path make itself would never include. Assert tab-indented + /// `include`-looking lines are not followed, while a genuine space-indented directive still is. + #[test] + fn test_list_targets_command_ignores_recipe_indented_include() { + let results = run_list_targets_in(&[ + ( + "Makefile", + // `\tinclude sneaky.mk` is a recipe line of `help`, not a directive. + "help: ## ok\n\tinclude sneaky.mk\nbuild:\n\t@echo build\n include real.mk\n", + ), + ( + "sneaky.mk", + "recipe-leaked: ## must not surface\n\t@echo bad\n", + ), + ("real.mk", "real-target: ## from a space-indented include\n"), + ]); + + let targets: Vec<&str> = results.iter().map(|(target, _)| target.as_str()).collect(); + assert!( + targets.contains(&"help") && targets.contains(&"build"), + "top-level targets must surface, got {targets:?}" + ); + assert!( + targets.contains(&"real-target"), + "a genuine space-indented `include` must still be followed, got {targets:?}" + ); + assert!( + !targets.contains(&"recipe-leaked"), + "a tab-indented (recipe) `include` line must not be followed, got {targets:?}" + ); + } + + /// The lexical `safe()` guard cannot catch an include whose path is in-tree *textually* but + /// resolves out of tree through a symlink (e.g. a repo shipping `escape -> /some/outside/dir` + /// and `include escape/secret.mk`). This is what the `realpath` containment layer is for. Build + /// a project with exactly that shape — a real out-of-tree sentinel reached only via an in-tree + /// symlink — and assert its target never surfaces while an ordinary in-tree include still does. + #[cfg(unix)] + #[test] + fn test_list_targets_command_blocks_symlink_escape() { + use super::LIST_TARGETS_COMMAND; + use std::process::Command; + use std::sync::atomic::{AtomicU64, Ordering}; + use std::time::{SystemTime, UNIX_EPOCH}; + + static COUNTER: AtomicU64 = AtomicU64::new(0); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + + let base = std::env::temp_dir(); + let project = base.join(format!("cs_make_symlink_proj_{nanos}_{seq}")); + // The sentinel lives OUTSIDE the project, reachable only by resolving the symlink. + let outside = base.join(format!("cs_make_symlink_outside_{nanos}_{seq}")); + std::fs::create_dir_all(&project).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write( + outside.join("secret.mk"), + "leaked-symlink: ## must not surface\n\t@echo bad\n", + ) + .unwrap(); + std::fs::write(project.join("ok.mk"), "in-tree-target: ## ok\n").unwrap(); + std::fs::write( + project.join("Makefile"), + "include escape/secret.mk\ninclude ok.mk\n\nhelp: ## root\n\t@echo root\n", + ) + .unwrap(); + // `escape` is an in-tree name pointing at the out-of-tree dir. + std::os::unix::fs::symlink(&outside, project.join("escape")).unwrap(); + + let output = Command::new("sh") + .arg("-c") + .arg(LIST_TARGETS_COMMAND) + .current_dir(&project) + .output() + .expect("failed to run list_targets command"); + + std::fs::remove_dir_all(&project).ok(); + std::fs::remove_dir_all(&outside).ok(); + + assert!( + output.status.success(), + "command failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let targets: Vec<(String, Option)> = + list_targets_post_process(&String::from_utf8_lossy(&output.stdout)) + .suggestions + .into_iter() + .map(|s| (s.exact_string, s.description)) + .collect(); + let names: Vec<&str> = targets.iter().map(|(t, _)| t.as_str()).collect(); + + assert!( + names.contains(&"help") && names.contains(&"in-tree-target"), + "in-tree targets must surface, got {names:?}" + ); + assert!( + !names.contains(&"leaked-symlink"), + "an include resolving out of tree through a symlink must not be followed, got {names:?}" + ); + } } From f4ceae0f07720631c4c9a8831e3ed0817f679d04 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Fri, 19 Jun 2026 17:02:34 +0800 Subject: [PATCH 2/2] Separate included makefiles with a newline `visit()` cat'd each makefile in the include graph back-to-back with no separator, so a file without a trailing newline merged its last line into the first line of the next included file -- a target at the top of an included file was swallowed into the preceding recipe line and dropped from completion. Emit an explicit newline (`echo`) after each `cat`. The extra blank line between files is inert to the target parser. Add a regression test: an include reached from a newline-less root Makefile whose last line is a recipe must still surface its leading target. --- command-signatures/src/generators/make.rs | 40 +++++++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/command-signatures/src/generators/make.rs b/command-signatures/src/generators/make.rs index 8a022a8e..eaa6311e 100644 --- a/command-signatures/src/generators/make.rs +++ b/command-signatures/src/generators/make.rs @@ -84,8 +84,10 @@ fn list_targets_post_process(output: &str) -> GeneratorResults { /// `-include`, and `sinclude` directives, so that targets split across included files are /// surfaced (GNU make treats them as one unified ruleset). /// -/// A POSIX-sh `visit()` function walks the include graph: it `cat`s a file, then uses a small -/// `awk` program to extract that file's include paths and recurses into each. `awk` only extracts +/// A POSIX-sh `visit()` function walks the include graph: it `cat`s a file (followed by an explicit +/// newline, so a file without a trailing newline cannot merge its last line into the next included +/// file's first line), then uses a small `awk` program to extract that file's include paths and +/// recurses into each. `awk` only extracts /// the paths (it never opens them), and every path is passed to the shell as a quoted argument — /// never interpolated into a command — so a malicious Makefile cannot inject commands. The awk /// extractor only matches `include`/`-include`/`sinclude` directives indented with **spaces**, not @@ -115,7 +117,7 @@ fn list_targets_post_process(output: &str) -> GeneratorResults { /// safely would require a shell (injection risk) or evaluating the Makefile. Absolute, /// home-relative, `..`-escaping, and symlink-escaping includes are intentionally not followed (see /// the security boundary above); their targets are simply not surfaced in completion. -const LIST_TARGETS_COMMAND: &str = r##"root=$(pwd -P)||exit 0;command -v realpath >/dev/null 2>&1||{ cat [Mm]akefile 2>/dev/null;exit 0;};seen="|";visit(){ rp=$(realpath -- "$1" 2>/dev/null)||return 0;case "$rp" in "$root"|"$root"/*) ;; *) return 0;; esac;case "$seen" in *"|$rp|"*) return 0;; esac;seen="$seen$rp|";cat -- "$rp" 2>/dev/null;set -f;for inc in $(awk 'function safe(p){return p!~/^\//&&p!~/^~/&&p!~/(^|\/)\.\.(\/|$)/} /^ *[-s]?include[ \t]+/{match($0,/^ *[-s]?include[ \t]+/);rest=substr($0,RLENGTH+1);sub(/#.*/,"",rest);n=split(rest,parts,/[ \t]+/);for(i=1;i<=n;i++)if(parts[i]!=""&&safe(parts[i]))print parts[i]}' "$rp" 2>/dev/null);do set +f;visit "$inc";set -f;done;set +f;};for f in [Mm]akefile;do [ -e "$f" ]&&visit "$f";done"##; +const LIST_TARGETS_COMMAND: &str = r##"root=$(pwd -P)||exit 0;command -v realpath >/dev/null 2>&1||{ cat [Mm]akefile 2>/dev/null;exit 0;};seen="|";visit(){ rp=$(realpath -- "$1" 2>/dev/null)||return 0;case "$rp" in "$root"|"$root"/*) ;; *) return 0;; esac;case "$seen" in *"|$rp|"*) return 0;; esac;seen="$seen$rp|";cat -- "$rp" 2>/dev/null;echo;set -f;for inc in $(awk 'function safe(p){return p!~/^\//&&p!~/^~/&&p!~/(^|\/)\.\.(\/|$)/} /^ *[-s]?include[ \t]+/{match($0,/^ *[-s]?include[ \t]+/);rest=substr($0,RLENGTH+1);sub(/#.*/,"",rest);n=split(rest,parts,/[ \t]+/);for(i=1;i<=n;i++)if(parts[i]!=""&&safe(parts[i]))print parts[i]}' "$rp" 2>/dev/null);do set +f;visit "$inc";set -f;done;set +f;};for f in [Mm]akefile;do [ -e "$f" ]&&visit "$f";done"##; pub fn generator() -> CommandSignatureGenerators { CommandSignatureGenerators::new("make").add_generator( @@ -482,4 +484,36 @@ dmg: $(DMG_NAME)-native"#; "an include resolving out of tree through a symlink must not be followed, got {names:?}" ); } + + /// `visit()` concatenates each makefile in the include graph back-to-back. A makefile that lacks + /// a final newline would otherwise merge its last line into the first line of the next included + /// file, so a target at the top of an included file is swallowed into the preceding line and + /// disappears from completion. Build a newline-less root Makefile whose last line is a recipe and + /// assert the included file's leading target still surfaces (the walker emits an explicit + /// separator after each file). + #[test] + fn test_list_targets_command_separates_files_without_trailing_newline() { + let results = run_list_targets_in(&[ + ( + // No trailing newline: the last line is a recipe, so without a separator `inc.mk`'s + // first line is appended to `\t@echo root` and parsed as part of that recipe line. + "Makefile", + "include inc.mk\nhelp: ## root help\n\t@echo root", + ), + ( + "inc.mk", + "included-target: ## from an include\n\t@echo inc\n", + ), + ]); + + let targets: Vec<&str> = results.iter().map(|(target, _)| target.as_str()).collect(); + assert!( + targets.contains(&"help"), + "root target must surface, got {targets:?}" + ); + assert!( + targets.contains(&"included-target"), + "a target at the top of an include reached from a newline-less Makefile must still surface, got {targets:?}" + ); + } }