Skip to content

Commit cf70ed2

Browse files
jamesadevineCopilot
andcommitted
fix(compile): block path-traversal in runtime-import resolver and guard fixture helper
Two follow-up findings from the 2026-05-19 review on #625: 1. Reject `..` path components in both resolvers. `resolve_imports_inline` (compile-time, inlined-imports: true) and `import.js` (runtime, inlined-imports: false) both accepted `../`-style paths without restriction. A malicious markdown body on an untrusted PR branch could therefore embed host files (e.g. `{{#runtime-import ../../../../etc/passwd}}`) into the compiled YAML or, at runtime, into the agent prompt. The new guard rejects any path whose `/` or `\\`-split segments include `..`, regardless of whether the path is absolute or relative. Literal `..` characters inside a filename (e.g. `name..md`) are still allowed because they are not segments. 2. `compile_fixture_with_inlined_imports` now refuses fixtures that already declare `inlined-imports:`. The helper used to inject `inlined-imports: true` by raw string substitution before the closing `---`. If a future fixture hard-coded `inlined-imports: false`, the rewritten front matter would have two `inlined-imports:` keys; serde_yaml silently uses the last one so the test would still pass, but the duplicate-key fixture is confusing and the helper would silently flip the author's intent. The guard panics with an actionable message. Tests: * src/compile/extensions/ado_script.rs: 5 new unit tests covering relative/embedded/absolute/backslash `..` rejection and the literal `name..md` allow case. * scripts/ado-script/src/import/__tests__/path-traversal.test.ts: 4 new vitest cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e6f6ff1 commit cf70ed2

4 files changed

Lines changed: 200 additions & 0 deletions

File tree

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { describe, expect, it } from "vitest";
2+
import { readText, runImportSource, withScratchDir, writeFixture } from "./helpers.js";
3+
4+
describe("runtime-import path traversal", () => {
5+
it("rejects a required marker whose path contains a '..' segment", () => {
6+
withScratchDir("traversal-required", (dir) => {
7+
const original = "before\n{{#runtime-import ../escape.md}}\nafter\n";
8+
const target = writeFixture(dir, "prompt.md", original);
9+
10+
const result = runImportSource(target);
11+
12+
expect(result.status).toBe(1);
13+
expect(result.stderr).toBe("");
14+
expect(result.stdout).toBe(
15+
"##vso[task.logissue type=error]runtime-import: invalid path '../escape.md': '..' path components are not allowed\n",
16+
);
17+
// Target file must NOT be overwritten on error.
18+
expect(readText(target)).toBe(original);
19+
});
20+
});
21+
22+
it("rejects '..' segments even when the marker is optional", () => {
23+
withScratchDir("traversal-optional", (dir) => {
24+
const original = "before\n{{#runtime-import? ../escape.md}}\nafter\n";
25+
const target = writeFixture(dir, "prompt.md", original);
26+
27+
const result = runImportSource(target);
28+
29+
// The traversal guard fires regardless of optional-marker form:
30+
// path traversal is structurally invalid, not a missing-file case.
31+
expect(result.status).toBe(1);
32+
expect(result.stdout).toContain("'..' path components are not allowed");
33+
expect(readText(target)).toBe(original);
34+
});
35+
});
36+
37+
it("rejects backslash-style '..' segments on Windows-shaped paths", () => {
38+
withScratchDir("traversal-backslash", (dir) => {
39+
const target = writeFixture(
40+
dir,
41+
"prompt.md",
42+
"{{#runtime-import sub\\..\\..\\escape.md}}\n",
43+
);
44+
45+
const result = runImportSource(target);
46+
47+
expect(result.status).toBe(1);
48+
expect(result.stdout).toContain("'..' path components are not allowed");
49+
});
50+
});
51+
52+
it("allows literal '..' inside a filename (not a segment)", () => {
53+
withScratchDir("traversal-literal", (dir) => {
54+
writeFixture(dir, "name..md", "DOUBLE_DOT_FILE");
55+
const target = writeFixture(
56+
dir,
57+
"prompt.md",
58+
"{{#runtime-import ./name..md}}\n",
59+
);
60+
61+
const result = runImportSource(target);
62+
63+
expect(result.status).toBe(0);
64+
expect(readText(target)).toBe("DOUBLE_DOT_FILE\n");
65+
});
66+
});
67+
});

scripts/ado-script/src/import/index.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,18 @@ function main(): void {
3939
// nested runtime-import markers inside them are not expanded. This matches
4040
// gh-aw's runtime-import behaviour.
4141
const expanded = original.replace(MARKER, (_whole, optional: string | undefined, rawPath: string) => {
42+
// Reject `..` segments — a malicious or compromised agent body could
43+
// otherwise reach files outside `base` (which on the agent VM is
44+
// `$(Build.SourcesDirectory)`, the trigger-repo checkout). Mirrors
45+
// `resolve_imports_inline` in src/compile/extensions/ado_script.rs.
46+
const hasDotDotSegment = rawPath.split(/[\/\\]/).some((segment) => segment === "..");
47+
if (hasDotDotSegment) {
48+
errors.push(
49+
`invalid path '${sanitizeForVsoMessage(rawPath)}': '..' path components are not allowed`,
50+
);
51+
return "";
52+
}
53+
4254
const absPath = isAbsolute(rawPath) ? rawPath : resolve(base, rawPath);
4355

4456
if (!existsSync(absPath)) {

src/compile/extensions/ado_script.rs

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,20 @@ pub fn resolve_imports_inline(body: &str, base_dir: &std::path::Path) -> Result<
249249
"runtime-import: invalid path '{}': whitespace is not allowed",
250250
path_str
251251
);
252+
// Reject any path whose segments contain `..`. A malicious agent
253+
// body could otherwise reach files outside `base_dir` and embed
254+
// them verbatim into the compiled YAML — e.g.
255+
// `{{#runtime-import ../../../../etc/passwd}}` if `ado-aw compile`
256+
// is run on an untrusted PR branch. This guard applies to both
257+
// relative and absolute paths because `..` segments make any
258+
// path-confinement check unsound.
259+
anyhow::ensure!(
260+
!path_str
261+
.split(['/', '\\'])
262+
.any(|component| component == ".."),
263+
"runtime-import: invalid path '{}': '..' path components are not allowed",
264+
path_str
265+
);
252266

253267
let abs = if std::path::Path::new(path_str).is_absolute() {
254268
std::path::PathBuf::from(path_str)
@@ -477,4 +491,92 @@ mod tests {
477491

478492
assert_eq!(result, "A ONE B TWO C");
479493
}
494+
495+
/// Path traversal: `..` segments would let a malicious agent body
496+
/// reach files outside `base_dir` (e.g. `../../../../etc/passwd` when
497+
/// `ado-aw compile` runs over an untrusted PR branch). Reject at
498+
/// resolution time regardless of whether the file actually exists.
499+
#[test]
500+
fn rejects_relative_path_with_dotdot_segment() {
501+
let workspace = TestWorkspace::new();
502+
let err = resolve_imports_inline(
503+
"{{#runtime-import ../escape.md}}",
504+
&workspace.path,
505+
)
506+
.unwrap_err();
507+
assert!(
508+
err.to_string().contains("'..' path components are not allowed"),
509+
"expected '..' rejection, got: {err}"
510+
);
511+
}
512+
513+
#[test]
514+
fn rejects_path_with_embedded_dotdot_segment() {
515+
let workspace = TestWorkspace::new();
516+
let err = resolve_imports_inline(
517+
"{{#runtime-import sub/../../escape.md}}",
518+
&workspace.path,
519+
)
520+
.unwrap_err();
521+
assert!(
522+
err.to_string().contains("'..' path components are not allowed"),
523+
"expected '..' rejection, got: {err}"
524+
);
525+
}
526+
527+
#[test]
528+
fn rejects_absolute_path_with_dotdot_segment() {
529+
let workspace = TestWorkspace::new();
530+
// Absolute paths are otherwise accepted (see
531+
// `supports_relative_and_absolute_paths`), but `..` segments
532+
// make path-confinement reasoning unsound and must still be
533+
// rejected.
534+
let err = resolve_imports_inline(
535+
"{{#runtime-import /tmp/agents/../../etc/passwd}}",
536+
&workspace.path,
537+
)
538+
.unwrap_err();
539+
assert!(
540+
err.to_string().contains("'..' path components are not allowed"),
541+
"expected '..' rejection, got: {err}"
542+
);
543+
}
544+
545+
#[test]
546+
fn rejects_backslash_dotdot_segment_on_windows_style_paths() {
547+
let workspace = TestWorkspace::new();
548+
let err = resolve_imports_inline(
549+
r"{{#runtime-import sub\..\..\escape.md}}",
550+
&workspace.path,
551+
)
552+
.unwrap_err();
553+
assert!(
554+
err.to_string().contains("'..' path components are not allowed"),
555+
"expected '..' rejection, got: {err}"
556+
);
557+
}
558+
559+
/// `..filename.md` and `name..md` are not path-traversal — they're
560+
/// literal filenames where `..` is part of the name, not a segment.
561+
/// Make sure the segment-aware check doesn't false-positive on these.
562+
#[test]
563+
fn allows_literal_double_dot_in_filename() {
564+
let workspace = TestWorkspace::new();
565+
workspace.write("..hidden.md", "DOTHIDDEN");
566+
workspace.write("name..md", "DOUBLE");
567+
568+
let a = resolve_imports_inline(
569+
"{{#runtime-import ..hidden.md}}",
570+
&workspace.path,
571+
)
572+
.unwrap();
573+
let b = resolve_imports_inline(
574+
"{{#runtime-import name..md}}",
575+
&workspace.path,
576+
)
577+
.unwrap();
578+
579+
assert_eq!(a, "DOTHIDDEN");
580+
assert_eq!(b, "DOUBLE");
581+
}
480582
}

tests/compiler_tests.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3697,6 +3697,25 @@ fn assert_valid_yaml(compiled: &str, fixture_name: &str) {
36973697

36983698
fn compile_fixture_with_inlined_imports(fixture_name: &str) -> String {
36993699
compile_fixture_tree_with_flags(fixture_name, &[], &[], |contents| {
3700+
// If the fixture already declares `inlined-imports:` (either
3701+
// value), don't inject a second key. serde_yaml silently uses the
3702+
// last key on duplicates, so the test would still pass — but the
3703+
// rewritten fixture would have a confusing duplicate and a
3704+
// future fixture that hard-codes `inlined-imports: false` would
3705+
// silently get flipped to `true` by this helper. Detect line-
3706+
// starting `inlined-imports:` so we don't false-positive on the
3707+
// string appearing inside body content.
3708+
let already_present = contents.lines().any(|line| {
3709+
let trimmed = line.trim_start();
3710+
trimmed.starts_with("inlined-imports:")
3711+
});
3712+
if already_present {
3713+
panic!(
3714+
"Fixture {fixture_name} already declares `inlined-imports:`; \
3715+
`compile_fixture_with_inlined_imports` would produce a duplicate key. \
3716+
Use `compile_fixture` directly, or remove the existing key from the fixture."
3717+
);
3718+
}
37003719
if let Some((front_matter, body)) = contents.split_once("\r\n---\r\n") {
37013720
format!("{front_matter}\r\ninlined-imports: true\r\n---\r\n{body}")
37023721
} else if let Some((front_matter, body)) = contents.split_once("\n---\n") {

0 commit comments

Comments
 (0)