diff --git a/AGENTS.md b/AGENTS.md index ba1804a5..03a2f497 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,6 +200,8 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ ├── version.rs # Semver parsing/comparison shared across the CLI (pipeline header versions, release tags, codemod cutovers) │ ├── ndjson.rs # NDJSON parsing utilities │ ├── sanitize.rs # Input sanitization for safe outputs +│ ├── sanitize/ +│ │ └── markdown.rs # Markdown structure parsing plus HTML/URL allowlisting │ ├── secure.rs # Validated newtype value objects (parse-don't-validate path/identifier types) │ ├── validate.rs # Structural input validators (char allowlists, format checks, injection detectors) │ ├── agent_stats.rs # OTel-based agent statistics parsing (token usage, duration, turns) diff --git a/Cargo.lock b/Cargo.lock index c1f5303c..affe77d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13,6 +13,7 @@ name = "ado-aw" version = "0.50.0" dependencies = [ "ado-aw-derive", + "ammonia", "anyhow", "async-trait", "base64", @@ -26,6 +27,7 @@ dependencies = [ "inventory", "log", "percent-encoding", + "pulldown-cmark", "rand", "regex-lite", "reqwest", @@ -62,6 +64,18 @@ dependencies = [ "memchr", ] +[[package]] +name = "ammonia" +version = "4.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc6d763210e2eb7670d1a5183a08bebefa3f97db2a738a684f2ce00bd49f681d" +dependencies = [ + "cssparser", + "html5ever", + "maplit", + "url", +] + [[package]] name = "android_system_properties" version = "0.1.5" @@ -375,6 +389,17 @@ dependencies = [ "hybrid-array", ] +[[package]] +name = "cssparser" +version = "0.37.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8c9cdaae01d5ed7882b04d795e7f752f46ff52d2fa3b50a20d28c464510bba98" +dependencies = [ + "dtoa-short", + "itoa", + "smallvec", +] + [[package]] name = "darling" version = "0.23.0" @@ -501,6 +526,21 @@ dependencies = [ "litrs", ] +[[package]] +name = "dtoa" +version = "1.0.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c3cf4824e2d5f025c7b531afcb2325364084a16806f6d47fbc1f5fbd9960590" + +[[package]] +name = "dtoa-short" +version = "0.3.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cd1511a7b6a56299bd043a9c167a6d2bfb37bf84a6dfceaba651168adfb43c87" +dependencies = [ + "dtoa", +] + [[package]] name = "dyn-clone" version = "1.0.20" @@ -800,6 +840,16 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" +[[package]] +name = "html5ever" +version = "0.39.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46a1761807faccc9a19e86944bbf40610014066306f96edcdedc2fb714bcb7b8" +dependencies = [ + "log", + "markup5ever", +] + [[package]] name = "http" version = "1.4.0" @@ -1231,6 +1281,23 @@ version = "0.4.29" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" +[[package]] +name = "maplit" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e2e65a1a2e43cfcb47a895c4c8b10d1f4a61097f9f254f183aee60cad9c651d" + +[[package]] +name = "markup5ever" +version = "0.39.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7122d987ec5f704ee56f6e5b41a7d93722e9aae27ae07cafa4036c4d3f9757de" +dependencies = [ + "log", + "tendril", + "web_atoms", +] + [[package]] name = "memchr" version = "2.7.6" @@ -1282,6 +1349,12 @@ dependencies = [ "tempfile", ] +[[package]] +name = "new_debug_unreachable" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" + [[package]] name = "num-traits" version = "0.2.19" @@ -1397,6 +1470,45 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" +[[package]] +name = "phf" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c1562dc717473dbaa4c1f85a36410e03c047b2e7df7f45ee938fbef64ae7fadf" +dependencies = [ + "phf_shared", + "serde", +] + +[[package]] +name = "phf_codegen" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49aa7f9d80421bca176ca8dbfebe668cc7a2684708594ec9f3c0db0805d5d6e1" +dependencies = [ + "phf_generator", + "phf_shared", +] + +[[package]] +name = "phf_generator" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "135ace3a761e564ec88c03a77317a7c6b80bb7f7135ef2544dbe054243b89737" +dependencies = [ + "fastrand", + "phf_shared", +] + +[[package]] +name = "phf_shared" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e57fef6bc5981e38c2ce2d63bfa546861309f875b8a75f092d1d54ae2d64f266" +dependencies = [ + "siphasher", +] + [[package]] name = "pin-project-lite" version = "0.2.16" @@ -1439,6 +1551,12 @@ dependencies = [ "zerovec", ] +[[package]] +name = "precomputed-hash" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" + [[package]] name = "prettyplease" version = "0.2.37" @@ -1458,6 +1576,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "pulldown-cmark" +version = "0.13.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9f068eba8e7071c5f9511831b44f32c740d5adf574e990f946ddb53db2f314e" +dependencies = [ + "bitflags", + "memchr", + "unicase", +] + [[package]] name = "quote" version = "1.0.43" @@ -1941,6 +2070,12 @@ dependencies = [ "bstr", ] +[[package]] +name = "siphasher" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ee5873ec9cce0195efcb7a4e9507a04cd49aec9c83d0389df45b1ef7ba2e649" + [[package]] name = "slab" version = "0.4.11" @@ -1969,6 +2104,30 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" +[[package]] +name = "string_cache" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a18596f8c785a729f2819c0f6a7eae6ebeebdfffbfe4214ae6b087f690e31901" +dependencies = [ + "new_debug_unreachable", + "parking_lot", + "phf_shared", + "precomputed-hash", +] + +[[package]] +name = "string_cache_codegen" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "585635e46db231059f76c5849798146164652513eb9e8ab2685939dd90f29b69" +dependencies = [ + "phf_generator", + "phf_shared", + "proc-macro2", + "quote", +] + [[package]] name = "strsim" version = "0.11.1" @@ -2046,6 +2205,15 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "tendril" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fed54709c5b3a53d09bb1c113ea4f5ceafd1e772ddcb0030a82e1d56c087b08" +dependencies = [ + "new_debug_unreachable", +] + [[package]] name = "thiserror" version = "2.0.18" @@ -2240,6 +2408,12 @@ version = "1.20.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40ce102ab67701b8526c123c1bab5cbe42d7040ccfd0f64af1a385808d2f43de" +[[package]] +name = "unicase" +version = "2.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dbc4bc3a9f746d862c45cb89d705aa10f187bb96c76001afab07a0d35ce60142" + [[package]] name = "unicode-ident" version = "1.0.22" @@ -2442,6 +2616,18 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "web_atoms" +version = "0.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba8b815c1b593dc0baf78dd0f4fc8fdb2de53198fb1163738093e9a311c33fb3" +dependencies = [ + "phf", + "phf_codegen", + "string_cache", + "string_cache_codegen", +] + [[package]] name = "winapi" version = "0.3.9" diff --git a/Cargo.toml b/Cargo.toml index 3be0ece6..b87c009c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,8 @@ indexmap = "2" zip = { version = "8.6.0", default-features = false, features = ["deflate"] } semver = "1.0.28" inventory = "0.3.24" +ammonia = "4.1.4" +pulldown-cmark = { version = "0.13.4", default-features = false } [dev-dependencies] reqwest = { version = "0.12", features = ["blocking"] } diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 7c5b0167..a8b2aea8 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -233,6 +233,57 @@ section). It is additive and build-scoped: it appears as one extra section alongside any `task.uploadsummary` tabs your own steps publish (including under `target: job` / `target: stage`), and never collides with them. +### Markdown body sanitization + +Safe outputs whose body is stored as Markdown (today: `create-work-item`'s +`description`) go through a Markdown-aware sanitizer rather than blanket HTML +escaping, so headings, lists, tables and code fences survive. + +The policy is: + +- **Transport sanitization applies to the whole document.** Control characters + and ANSI escapes are removed, Azure DevOps logging commands (`##vso[`, `##[`) + are wrapped in backticks, HTML comments are removed, and the content size and + line count caps are enforced. +- **Code is left alone.** [pulldown-cmark](https://docs.rs/pulldown-cmark) + identifies code spans, fenced and indented code blocks; no rendering + transformation is applied inside them, so a fence showing `", /still contains '', /still contains 'onerror='/], + ["iframe", '', /still contains ' { + await expect( + createWorkItemRendering.assert( + renderingCtx({ + id: 42, + fields: { "System.Description": `${adoExpected}\n${leaked}` }, + }), + { title: "t" }, + record, + [record], + ), + ).rejects.toThrow(error); + }); + + it("accepts the Bug repro-steps rendering path", async () => { + const state: { title: string; createdId?: number } = { title: "t" }; + + await createBugWorkItemRendering.assert( + renderingCtx({ + id: 42, + fields: { "Microsoft.VSTS.TCM.ReproSteps": adoExpected }, + multilineFieldsFormat: { + "Microsoft.VSTS.TCM.ReproSteps": "markdown", + }, + }), + state, + record, + [record], + ); + + expect(state.createdId).toBe(42); + }); + + it("rejects a Bug response that omits the repro-steps field", async () => { + await expect( + createBugWorkItemRendering.assert( + renderingCtx({ id: 42, fields: { "System.Description": adoExpected } }), + { title: "t" }, + record, + [record], + ), + ).rejects.toThrow(/has no string 'Microsoft\.VSTS\.TCM\.ReproSteps'/); + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/ado-rest.ts b/scripts/ado-script/src/executor-e2e/ado-rest.ts index 183ac5b1..65dd24fa 100644 --- a/scripts/ado-script/src/executor-e2e/ado-rest.ts +++ b/scripts/ado-script/src/executor-e2e/ado-rest.ts @@ -132,13 +132,35 @@ export class AdoRest { return res; } - async getWorkItem(id: number): Promise<{ id: number; fields: Record }> { + async getWorkItem(id: number): Promise<{ + id: number; + fields: Record; + /** + * Per-field rendering format (`Markdown` | `Html`). Only present on + * organizations where the work-item Markdown feature is enabled, so + * callers must treat it as optional. + */ + multilineFieldsFormat?: Record; + }> { const path = this.projPath(`_apis/wit/workitems/${id}?api-version=7.1`); - const res = await this.request<{ id: number; fields: Record }>(path); + const res = await this.request<{ + id: number; + fields: Record; + multilineFieldsFormat?: Record; + }>(path); if (!res) throw new Error(`getWorkItem(${id}) returned no body`); return res; } + /** True when the project defines the given work item type (e.g. `Bug`). */ + async workItemTypeExists(type: string): Promise { + const path = this.projPath( + `_apis/wit/workitemtypes/${AdoRest.seg(type)}?api-version=7.1`, + ); + const res = await this.request<{ name?: string }>(path, { allow404: true }); + return res !== undefined; + } + /** Find the newest live work item with an exact title. Used for cleanup recovery. */ async findWorkItemByTitle(title: string): Promise { const escapedTitle = title.replaceAll("'", "''"); diff --git a/scripts/ado-script/src/executor-e2e/scenarios/markdown-rendering-corpus.json b/scripts/ado-script/src/executor-e2e/scenarios/markdown-rendering-corpus.json new file mode 100644 index 00000000..18c6f96e --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/scenarios/markdown-rendering-corpus.json @@ -0,0 +1,230 @@ +{ + "$comment": [ + "Shared Markdown rendering-fidelity corpus for the work-item description", + "sanitizer (src/sanitize/markdown.rs, applied to create-work-item's", + "description). This file is the SINGLE source of truth for both:", + " * the Rust golden test in src/sanitize/markdown.rs (include_str!), and", + " * the create-work-item-rendering executor E2E scenarios", + " (scripts/ado-script/src/executor-e2e/scenarios/work-item.ts).", + "`expected` pins the output of ado-aw's sanitizer before the request.", + "`ado_expected` pins Azure DevOps' additional server-side normalization.", + "`input` is what an agent proposes; `expected` is the sanitized text that", + "ado-aw sends. All three values are arrays of lines joined with \\n." + ], + "input": [ + "# Rendering fidelity corpus", + "", + "Paragraph with **bold**, _italic_, `inline code` and a bold tag.
", + "Line after the break.", + "", + "## Lists", + "", + "1. Ordered item", + "2. Ordered item with a nested list", + " - Nested bullet", + " - Nested bullet with [a link](https://example.test/docs)", + "", + "## Tables", + "", + "| Field | Value |", + "| --- | --- |", + "| Markdown table | plain cell |", + "", + "", + " ", + " ", + "
HTML header
leftright
", + "", + "## Disclosure and inline tags", + "", + "
", + "Collapsed section", + "", + "Body with Ctrl+C, H2O and x2.", + "", + "
", + "", + "## Links and images", + "", + "Autolink: ", + "", + "Image: ![diagram](https://example.test/diagram.png)", + "", + "Denied image scheme: ![bad](data:image/svg+xml;base64,AAAA)", + "", + "Denied link scheme: [click me](javascript:alert(1))", + "", + "Entity-encoded denied scheme: [encoded](javascript:alert(1))", + "", + "Escaped denied reference: [escaped][escaped-ref]", + "", + "[escaped-ref]: javascript\\:alert(1)", + "", + "Entity-encoded denied reference: [entity][entity-ref]", + "", + "[entity-ref]: javascript:alert(1)", + "", + "## Code", + "", + "```html", + "", + "fenced javascript link", + "```", + "", + "## Denied HTML", + "", + "", + "", + "\"onerror", + "", + "", + "", + "entity-encoded href", + "", + "" + ], + "expected": [ + "# Rendering fidelity corpus", + "", + "Paragraph with **bold**, _italic_, `inline code` and a bold tag.
", + "Line after the break.", + "", + "## Lists", + "", + "1. Ordered item", + "2. Ordered item with a nested list", + " - Nested bullet", + " - Nested bullet with [a link](https://example.test/docs)", + "", + "## Tables", + "", + "| Field | Value |", + "| --- | --- |", + "| Markdown table | plain cell |", + "", + "", + " ", + " ", + "
HTML header
leftright
", + "", + "## Disclosure and inline tags", + "", + "
", + "Collapsed section", + "", + "Body with Ctrl+C, H2O and x2.", + "", + "
", + "", + "## Links and images", + "", + "Autolink: ", + "", + "Image: ![diagram](https://example.test/diagram.png)", + "", + "Denied image scheme: ![bad]((redacted))", + "", + "Denied link scheme: [click me]((redacted))", + "", + "Entity-encoded denied scheme: (redacted)", + "", + "Escaped denied reference: [escaped][escaped-ref]", + "", + "(redacted)", + "", + "Entity-encoded denied reference: [entity][entity-ref]", + "", + "(redacted)", + "", + "## Code", + "", + "```html", + "", + "fenced javascript link", + "```", + "", + "## Denied HTML", + "", + "", + "", + "\"onerror", + "", + "", + "", + "entity-encoded href", + "", + "" + ], + "ado_expected": [ + "# Rendering fidelity corpus", + "", + "Paragraph with **bold**, _italic_, `inline code` and a bold tag.
", + "Line after the break.", + "", + "## Lists", + "", + "1. Ordered item", + "2. Ordered item with a nested list", + " - Nested bullet", + " - Nested bullet with [a link](https://example.test/docs)", + "", + "## Tables", + "", + "| Field | Value |", + "| --- | --- |", + "| Markdown table | plain cell |", + "", + "", + " ", + " ", + "
HTML header
left right
", + "", + "## Disclosure and inline tags", + "", + "
", + "Collapsed section", + "", + "Body with Ctrl+C, H2O and x2.", + "", + "
", + "", + "## Links and images", + "", + "Autolink: ", + "", + "Image: ![diagram](https://example.test/diagram.png)", + "", + "Denied image scheme: ![bad]((redacted))", + "", + "Denied link scheme: [click me]((redacted))", + "", + "Entity-encoded denied scheme: (redacted)", + "", + "Escaped denied reference: [escaped][escaped-ref]", + "", + "(redacted)", + "", + "Entity-encoded denied reference: [entity][entity-ref]", + "", + "(redacted)", + "", + "## Code", + "", + "```html", + "", + "fenced javascript link", + "```", + "", + "## Denied HTML", + "", + "", + "", + "\"onerror", + "", + "", + "", + "entity-encoded href", + "", + "" + ] +} diff --git a/scripts/ado-script/src/executor-e2e/scenarios/work-item.ts b/scripts/ado-script/src/executor-e2e/scenarios/work-item.ts index 0b9efaf5..73be0f63 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/work-item.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/work-item.ts @@ -7,11 +7,28 @@ import type { ExecutedRecord, PriorEntry, Scenario, ScenarioContext } from "../scenario.js"; import { SkipError } from "../scenario.js"; import { detBody, numResult, strResult, Teardown } from "./common.js"; +import renderingCorpus from "./markdown-rendering-corpus.json" with { type: "json" }; const WORK_ITEM_TYPE = "Task"; const CREATE_TEMPORARY_ID = "#aw_wicreate"; const ASSIGN_TEMPORARY_ID = "#aw_wiassign"; +/** + * Rendering-fidelity corpus shared with the Rust golden test in + * `src/sanitize/markdown.rs` (which `include_str!`s the same JSON). The Rust + * test proves the sanitizer produces `expected`; these scenarios prove Azure + * DevOps applies the separately pinned `ado_expected` normalization, so the + * two boundaries cannot be confused. + */ +const RENDERING_INPUT = renderingCorpus.input.join("\n"); +const ADO_RENDERING_EXPECTED = renderingCorpus.ado_expected.join("\n"); + +/** + * Constructs the sanitizer must never let reach a rendered work item. + * Compared case-insensitively, so a folded `
"#, ); - assert!(!output.contains("b")); - assert!(!output.contains("alert(1)\n\n```\ntail" + "```html\n\n\n```\n" ); } #[test] - fn test_sanitize_markdown_strips_nested_dangerous_tags_to_fixed_point() { + fn test_sanitize_markdown_strips_nested_dangerous_tags() { let output = sanitize_markdown("ipt>alert(1)ipt>"); - assert_eq!(output, "alert(1)"); - assert!(!output.contains("{suffix}>payload"); let output = sanitize_markdown(&input); @@ -898,16 +584,15 @@ mod tests { fn test_sanitize_markdown_strips_dangerous_tags_folded_into_attribute_span() { let output = sanitize_markdown(r#"
safe
"#); - assert_eq!(output, r#"
safe
"#); - assert!(!output.contains(""), "{output}"); + assert!(!output.contains("body{display:none}

safe

"); - assert_eq!(output, "body{display:none}

safe

"); - assert!(!output.contains("safe

"); } #[test] @@ -916,15 +601,16 @@ mod tests { r#"

Hi

link"#, ); - assert!(output.contains("

Hi

")); - assert!(output.contains(r#"link")); - assert!(output.contains("Hi"), "{output}"); + assert!( + output.contains(r#"link"#), + "{output}" + ); + assert!(output.contains(r#""#), "{output}"); + assert!(!output.contains("onclick"), "{output}"); + assert!(!output.contains("onmouseover"), "{output}"); + assert!(!output.contains("onerror"), "{output}"); + assert!(!output.contains("doWork"), "{output}"); } #[test] @@ -932,12 +618,10 @@ mod tests { let output = sanitize_markdown(r#"link"#); assert!(output.contains("link"), "{output}"); - assert!(!output.contains(""#); - assert_eq!(output, r#""#); - assert!(!output.contains("onerror")); - assert!(!output.contains("alert")); + assert_eq!(output, r#""#); } #[test] - fn test_sanitize_markdown_escapes_unclosed_dangerous_tag() { + fn test_sanitize_markdown_drops_unclosed_dangerous_tag() { let output = sanitize_markdown("` and ``a `b` [x](javascript:1)`` inline."; + assert_eq!(sanitize_markdown(input), input); + } + + #[test] + fn preserves_tilde_fenced_and_indented_code_blocks() { + let input = + "~~~\n\n~~~\n\ntext\n\n \n"; + assert_eq!(sanitize_markdown(input), input); + } + + #[test] + fn skips_mention_and_bot_trigger_neutralization_inside_code() { + let input = "```\n@user fixes #12 AB#34\n```\n"; + assert_eq!(sanitize_markdown(input), input); + } + + #[test] + fn neutralizes_mentions_and_bot_triggers_outside_code() { + let output = sanitize_markdown("@user fixes #12 AB#34"); + + assert!(output.contains("`@user`"), "{output}"); + assert!(output.contains("`fixes #12`"), "{output}"); + assert!(output.contains("`AB#34`"), "{output}"); + } + + #[test] + fn neutralizes_pipeline_commands_inside_code_blocks() { + // Pipeline command neutralization is a transport concern: the string is + // echoed by the agent job, where a fence is not a fence. + let output = sanitize_markdown("```\n##vso[task.setvariable variable=x]y\n```\n"); + + assert!(!output.contains("##vso[task"), "{output}"); + } + + #[test] + fn removes_html_comments_including_unclosed() { + assert_eq!(sanitize_markdown("ab"), "ab"); + assert_eq!(sanitize_markdown("a