test(safe-outputs): verify work item rendering fidelity end to end

[Copilot]

Summary

The Markdown sanitizer rewrite changes what ends up in a work item description, but nothing asserted the rendered result — scenarios/work-item.ts only checked title and assignee, so a rendering regression would ship silently.

sanitize_markdown reaches ADO through exactly one path: create-work-item's description, written to System.Description (or Microsoft.VSTS.TCM.ReproSteps for Bugs) with a /multilineFieldsFormat/<field> = Markdown patch. This adds coverage for that path at both the sub-second and against-real-ADO levels.

Shared corpus (single source of truth)

• scripts/ado-script/src/executor-e2e/scenarios/markdown-rendering-corpus.json holds input (headings, nested lists, GFM + HTML tables with colspan, <details>/<summary>, <kbd>/<sub>/<sup>, autolink, image, and denied constructs interleaved with their fenced-code twins) and expected (the sanitized golden), as line arrays.
• Rust include_str!s it; the harness imports it. A deliberate rendering change means editing expected in one file — the two layers cannot drift.

Rust

• src/sanitize/markdown.rs: rendering_corpus test module + golden test asserting sanitize_markdown(input) == expected.
• src/safe_outputs/create_work_item.rs: wiremock test pinning the default path — the patch carries the sanitized golden and/multilineFieldsFormat/System.Description = Markdown.

Executor E2E

• create-work-item-rendering (Task) and create-work-item-rendering-bug (Bug) propose the raw corpus, then assert on the stored field:
  1. no <script/onerror=/<iframe/javascript: outside fenced code, while the fenced twins survive verbatim;
  2. multilineFieldsFormat: Markdown;
  3. byte-for-byte equality with the golden.
• Security runs before the golden so a leak reports as a leak, not a generic mismatch.
• AdoRest.getWorkItem now surfaces multilineFieldsFormat; new workItemTypeExists lets the Bug scenario SkipError where the type is undefined.

Deviation worth reviewing: ADO does not reliably surface multilineFieldsFormat on read and I could not probe AgentPlayground from the sandbox, so assertion (2) is conditional — it fails when the value is present and not Markdown, and logs a note when absent, with the patch itself pinned by the new Rust test. This is the fallback the plan allowed; if the field does come back on AgentPlayground, the conditional should be tightened to a hard requirement. The optional comment-based renderer probe was skipped, since comments go through sanitize_text, not the changed code.

Test plan

• cargo test --all-targets (new golden + executor patch tests included)
• npx vitest run in scripts/ado-script — 1149 tests, including new coverage for golden mismatch, non-Markdown format, leaked construct, mangled fenced code, and the Bug-type skip
• npm run typecheck; cargo clippy --all-targets shows only a pre-existing warning
• The E2E scenarios themselves run on the existing PR-validation and daily lanes (allScenarios); no pipeline change needed and they have not been exercised against ADO from here

[azure-pipelines]

Azure Pipelines:
2 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
2 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ TypeScript Code Quality Reviewer completed the TypeScript code quality review.

No scripts/ado-script/ TypeScript files were changed in this PR — the diff is Rust-only (sanitize/markdown.rs, safe_outputs/create_work_item.rs) plus a shared JSON test fixture (markdown-rendering-corpus.json) and an update to Rust-side executor-e2e test infra. Nothing falls under the TypeScript Code Quality Reviewer's remit, so no review comments were posted.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer

[github-actions]

Compiler contract review

No compiler-contract concerns here. This PR is test/coverage-only:

• No front-matter grammar, typed IR, or CompilerExtension changes.
• No new safe-output tool or Params field — create-work-item's description stays a plain String, which is correct: it's agent-authored Markdown prose, not a path/ref/identifier that the src/secure.rs newtypes are meant for.
• Cargo.toml gains ammonia/pulldown-cmark; Cargo.lock moved with it (186 additions) — no drift.
• No .github/workflows/*.md changed, so no .lock.yml drift.
• No gate/fact IR touched (src/compile/filter_ir.rs, Fact enum untouched), so types.gen.ts / fact-catalog.gen.json correctly did not move.
• No new ADO definition or per-case orchestrator variable was added for the two new executor-e2e scenarios — they're appended to the existing scenario list in work-item.ts, consistent with the shared-corpus/lane model.
• Docs are in sync: docs/safe-outputs.md gained a full "Markdown body sanitization" section and the description field doc links to it; tests/executor-e2e/README.md documents the two new rendering scenarios.

The sanitizer rewrite itself (src/sanitize/markdown.rs, HTML/URL allowlisting) is security-relevant but outside this reviewer's contract-drift scope — deferring to the security reviewer for that logic.

No blocking findings from the compiler-contract perspective.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 38.2 AIC · ⌖ 2.9 AIC · ⊞ 12.7K
_{Comment /review to run again}

[github-actions]

Security review: no regressions found.

This PR is test-only — it adds end-to-end and golden-test coverage for the Markdown sanitizer's rendering fidelity in create-work-item. The allowlist-based markdown.rs sanitizer itself was already merged on main in a prior commit (eba900a refactor(sanitize): allowlist-based Markdown sanitizer); this diff does not change sanitization logic, URL-scheme allowlisting, HTML tag allowlisting, or token/credential handling.

Checked specifically for regressions and found none:

• No validated newtype (src/secure.rs) downgraded to raw String.
• No widening of ALLOWED_TAGS / ALLOWED_URL_SCHEMES in this diff.
• The new executor-e2e assertions (scenarios/work-item.ts) actively check that <script, onerror=, <iframe, and `(redacted) do not survive in rendered prose, while verifying fenced-code twins survive verbatim — this strengthens the safety net rather than weakening it.
• The wiremock test in src/safe_outputs/create_work_item.rs pins the existing /multilineFieldsFormat/System.Description = Markdown patch path; no new write path or token is introduced.
• AdoRest.getWorkItem/workItemTypeExists additions are read-only test-harness helpers using the existing segment-encoding helper (AdoRest.seg), consistent with existing usage elsewhere in the file.

Nothing merge-blocking. COMMENT only.

🔒 Security review by PR Security Reviewer · auto · 64.5 AIC · ⌖ 2.35 AIC · ⊞ 11.7K
_{Comment /review to run again}

[github-actions]

Verdict: request changes

Rust-quality pass over the sanitizer rewrite (src/sanitize.rs / src/sanitize/markdown.rs). One inline finding is merge-blocking: locate_destination() can match the destination string inside the link label instead of the actual destination when both are byte-identical (e.g. [(redacted) letting a denied URL scheme survive sanitization untouched — confirmed by running the sanitizer locally against this branch. Everything else in the rewrite (allowlist approach, code-span protection via pulldown_cmark`, region cursor logic in the non-colliding case) looks sound and is a real improvement over the previous blocklist/fixed-point approach.

Themes reviewed

• Error handling: no new unwrap/expect reachable from untrusted input; the new rendering_corpus helpers are #[cfg(test)]-only.
• Region/cursor overlap handling in sanitize_markdown_html is correct for genuinely nested constructs (code spans within code fences, etc.) but the mislocated-destination case above can produce an incorrectly ordered region that defeats the range.start < cursor skip.
• scheme_allowed() scheme-confusion handling (whitespace/entity-decoded schemes, relative refs) looked correct for the cases exercised by the new test suite.
• No cross-platform path or casting concerns in the diff.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 84.2 AIC · ⌖ 1.89 AIC · ⊞ 11.4K
_{Comment /review to run again}

[github-actions]

locate_destination finds the first byte-for-byte match of dest_url anywhere in the element's full source span, which includes the link/alt text, not just the destination — for a payload like `[(redacted) this locates the label occurrence instead of the actual destination, so the real dangerous URL is left in the output untouched.

💡 Proof and suggested fix

Confirmed by running sanitize_markdown("[(redacted) against this branch: it returns "[(redacted)]((redacted) — the label was redacted but the `(redacted) destination survives verbatim in the sanitized output that gets written to the work item description. This is a full sanitizer bypass for the deny-by-default URL-scheme policy this module documents.

Root cause: source.find(dest_url) searches the whole range (which pulldown_cmark gives as the entire [label](dest) span for inline links), and returns the first match — the label text, when it happens to contain the same bytes as the destination.

Fix direction: search from the end of the span backwards (destinations are the last non-title component before the closing )/> in inline links, or use rfind for standard inline syntax), or better, walk the source structurally (e.g. locate the last (...) pair, or the reference-definition span which doesn't have this ambiguity) rather than a naive forward substring search over the whole element. At minimum, prefer rfind over find for inline links/images, since the destination always follows the label in that syntax.

This also compounds with the sanitize_markdown_html region-overlap handling (the range.start < cursor skip at line 128), since a mislocated destination range that starts before the enclosing element's redact range can cause the outer (correct) region to be skipped entirely once regions are sorted only by start.

[Copilot]

Confirmed and fixed in 8d0f6f0. locate_destination now starts the search after the label terminator (]( for inline links/images, ]: for reference definitions), since the destination always follows the label in those syntaxes; an autolink has no label so the search still starts at 0. [javascript:alert(1)](javascript:alert(1)) now sanitizes to [javascript:alert(1)]((redacted)) instead of leaving the live destination. Added three regression tests: label-repeats-destination for inline links and for reference definitions, plus an allowed-scheme case asserting the destination stays out of the text transform while the label is still processed.

[github-actions]

Solid test coverage for this rewrite: the shared corpus (markdown-rendering-corpus.json) genuinely closes the gap called out in the PR description — a Rust golden test and an against-real-ADO executor scenario now both pin the same expected output, so the sanitizer and the ADO storage/rendering path can't silently drift apart. The vitest suite for work-item.ts covers happy path, skip-on-missing-type, format mismatch, security leak, fenced-code mangling, and the Bug repro-steps field — genuinely behavior-level assertions, not implementation mirroring. The sanitize.rs/markdown.rs test rewrite correctly loosens assertions that pinned exact escaping behavior that changed by design (allowlist vs. blocklist), while adding new coverage for autolinks, reference definitions, encoded schemes, and nested code — no unjustified weakening found.

One gap noted inline: AdoRest.workItemTypeExists has no direct unit test exercising its real request()/allow404 path.

🧪 Test quality analysis by Test Quality Sentinel · auto · 87.2 AIC · ⌖ 15.3 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

workItemTypeExists has no direct unit test — it's only reached indirectly through the renderingCtx mock in work-item-scenarios.test.ts, which stubs ctx.rest entirely and never exercises this method's real body (URL construction via AdoRest.seg, or the allow404 → undefined → false translation through the shared request() path). A bug in the 404 handling here (e.g. treating a non-404 error as "type doesn't exist") would ship silently.

💡 suggestion

Add a focused unit test for AdoRest.workItemTypeExists (mocking fetch/request directly) asserting: 404 → false, 200 → true, and a non-404 error still throws rather than being swallowed as false.

[Copilot]

Added scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts in 8d0f6f0, stubbing global fetch: 200 → true (also asserting the encoded project/type URL), 404 → false, and 403 → throws rather than being swallowed as a missing type.

[jamesadevine]

@copilot address feedback

[jamesadevine]

@copilot comprehensively review the new sanitization code for back compost breaks and security holes. Post your review as a comment.

[Copilot]

@copilot comprehensively review the new sanitization code for back compost breaks and security holes. Post your review as a comment.

Review: src/sanitize/markdown.rs (allowlist rewrite)

Every claim below was reproduced by running sanitize_markdown on this branch (b8a5087) via a temporary probe test; the probe was removed before this comment. Blast radius is small — sanitize_markdown has exactly two callers (src/mcp.rs:816, src/safe_outputs/create_work_item.rs:95), both the create-work-item description.

Verdict: the allowlist direction is the right call and it closes the whole folded-tag/encoded-scheme family the blocklist kept losing to. But there are two residual holes in the deny-by-default URL policy and one design-level gap, plus a set of content-fidelity regressions that are worse than the old escaping behaviour.

---

Security

S1 — Nested link/image destinations are never checked when the outer destination is denied.(medium; same class as the label bug fixed in b8a5087)

in : [![x](vbscript:inner)](vbscript:outer)
out: [![x](vbscript:inner)]((redacted))

In regions(), the denied-scheme branch sets skip_until = range.end, which suppresses every event nested inside the outer element — including the inner image's Tag::Image. The inner denied destination reaches the work item verbatim and a Markdown renderer emits <img src="vbscript:inner">. Same with data::

in : [![x](data:text/html,<script>alert(1)</script>)](javascript:o)
out: [![x](data:text/html,)]((redacted))

Confirmed by idempotency: a secondsanitize_markdown pass redacts the inner destination, which by itself proves the first pass is incomplete. skip_until is only needed to stop the enclosing span being re-walked; it should not suppress nested link/image destination checks. The allowed-scheme branch has the same shape for autolinks.

S2 — Protected ranges are copied verbatim, so raw un-allowlisted HTML can survive.(low–medium; parser-differential)

in : [x](https://e.test/a<img/onerror=alert(1)>)
out: [x](https://e.test/a<img/onerror=alert(1)>) # unchanged
in : [r]: https://e.test/<script>
out: [r]: https://e.test/<script> # unchanged

An allowed-scheme destination is marked Region::Protected and bypasses ammonia entirely, so the module's core guarantee — nothing outside the tag allowlist reaches the output — does not actually hold. Whether this is live depends on ADO's renderer agreeing with pulldown-cmark that those bytes are a link destination; markdown-it and pulldown-cmark do not agree on every edge of destination parsing, and the sanitizer should not be betting on that. Cheap hardening: refuse to mark a destination Protected when it contains < or > (fall through to the allowlist, which will escape it), which costs nothing for real URLs.

S3 — The protected surface got wider, which widens S2. The old hand-rolled detector was deliberately conservative (its comment said so): single-line inline spans only. pulldown-cmark now also protects multi-line code spans and indented code blocks:

in/out: `a\nb <script>alert(1)</script>` # verbatim, was escaped before

That is correct CommonMark and probably correct for ADO, but it is a security-relevant widening that isn't called out anywhere. Worth a line in the module docs stating the explicit assumption: the ADO renderer's code-span/fence detection matches pulldown-cmark's. If that ever drifts, verbatim <script> is the failure mode.

Things I tried that correctly held: folded/nested tag names, on* in every quoting style, entity- and tab-encoded javascript:, srcdoc, base, form/input, template, noscript, SVG/MathML mglyph mutation-XSS, <script> inside an HTML block, inside a table cell, and inside a lazy-continuation indented line (correctly not treated as code). No DoS: at the 0.5 MB cap the worst input I found (120k code spans, one ammonia invocation each) is ~2.3 s in a debug build; everything else is under 1 s, and output re-truncation after the ≤5× entity expansion is handled by the second enforce_content_limits.

---

Back-compat / rendering fidelity

These are all regressions against the old behaviour, which escaped unknown markup (<) rather than deleting it. Silent deletion of author text is the theme.

B1 — Text that merely looks like a tag is deleted, not escaped.(high impact for engineering descriptions)

Fix `Vec<String>` and Vec<String> and Map<K,V> -> Fix `Vec<String>` and Vec and Map
\<tag\> -> (deleted)
<www.example.test> -> (deleted)

Vec<String> outside a code span silently loses <String>. The old sanitizer produced Vec<String>, which renders correctly. Note this also contradicts the new docs, which state "A bare < in prose is stored as <" — that only holds when the <isn't followed by something html5ever will parse as a tag, which is the common case in code-adjacent prose.

B2 — Prose can be turned into markup, deleting text in between.

in : if a<b and b>c then
out: if a<b>c then</b>

<b and b> is parsed as a <b> tag, "and b" is destroyed, and a </b> that the author never wrote is synthesised.

B3 — A literal > at line start becomes a real blockquote.

in : > quoted? -> out: > quoted?

restore_blockquote_markers can't distinguish a > that ammonia escaped from one the author wrote as >, so author-escaped text is promoted to markup. Fixable by doing the blockquote restore only on ranges ammonia actually escaped (or by recording line-leading > positions before cleaning).

B4 — Inline HTML spanning a code span is rebalanced.

in : <b>bold `code` more</b>
out: <b>bold </b>`code` more

Each inter-region chunk is cleaned independently, so ammonia closes the open tag at the chunk boundary and drops the orphan </b> in the next chunk. Inherent to per-chunk cleaning; worth at least a documented limitation.

B5 — Smaller ones:& in prose and in bare URLs becomes & (https://e.test/x?a=1&b=2 → …a=1&b=2, which breaks ADO's bare-URL autolinking); [a](<https://e.test/a b>) → [a](<https://e.test/a b>), breaking pointy-bracket destinations with spaces; CRLF is normalised to LF; <noscript> content is entity-escaped; a leading newline inside <pre> is eaten.

---

Suggested priority

1. S1 — real policy bypass, small fix (don't let skip_until suppress nested destination checks), and a [![x](vbscript:i)](vbscript:o) regression test.
2. B1/B2 — the content-deletion regressions are the ones authors will actually hit. Consider pre-escaping < that isn't followed by a plausible allowlisted tag name before handing the chunk to ammonia, so unknown markup degrades to < like it used to.
3. S2/S3 — cheap hardening (</> in a destination ⇒ not protected) plus an explicit renderer-parity assumption in the module docs.
4. B3/B4/B5 — document or fix; B3 is the only one that manufactures markup.

None of these are in the shared rendering corpus, so the golden test and the executor E2E both pass today. Happy to land 1 and 3 now and split the fidelity work out — say the word.

[jamesadevine]

@copilot fixup all reviewer findings including low pri ones

[Copilot]

@copilot fixup all reviewer findings including low pri ones

All findings from my review are fixed in ce07dd3 (plus test wording in the follow-up commit). sanitize_markdown is unchanged from the caller's point of view; the shared rendering corpus golden did not move.

Security

• S1 — the denied-scheme branch no longer sets skip_until, so nested elements keep being walked. [![x](vbscript:inner)](vbscript:outer) → [![x]((redacted))]((redacted)). The whole-element fallback (destination not locatable) still suppresses nested regions, now via the existing overlap rule rather than a parser skip.
• S2 — a destination containing < or > is never marked Protected; it falls through to the allowlist ([x](https://e.test/a<img/onerror=alert(1)>) loses the handler, [r]: https://e.test/<script> loses the tag). While in there I fixed the pointy-bracket case B5 mentioned: the brackets are now inside the protected range, so [a](<https://e.test/a b>) survives intact.
• S3 — the renderer-parity assumption is now stated in the module docs, with the note that everything outside code is deliberately routed through the allowlist so the assumption stays confined to code.

Fidelity

• B1/B2 — a < is only handed to the allowlist when it starts a real HTML/SVG/MathML element name written with plausible attribute syntax; otherwise it is escaped, so unknown markup degrades to < as it used to. Vec<String> → Vec<String>, <tag> and <www.example.test> survive. For B2 the discriminator is valueless attributes: no attribute the allowlist keeps is valueless, so if a<b and b>c is prose while <details open> stays a tag. The name list deliberately includes the dangerous elements — escaping <svg onload=alert(1)> would resurrect a dropped payload as visible text. The scan is capped at 256 bytes per < so it cannot go quadratic on adversarial input (there is a test).
• B3 — blockquote markers are recorded before cleaning with a private-use sentinel, so only a > the author actually wrote as markup comes back; > quoted? stays text. Marking skips a > that closes a tag written across two lines, so <b\n>text</b> is not turned into an unterminated tag.
• B4 — fixed rather than documented: protected/redacted ranges become placeholders and the document goes through one ammonia pass, so <b>bold \code` more` is no longer rebalanced. The sentinels are stripped from the input first, so they cannot be forged.
• B5 — & is restored in text (R&D, …?a=1&b=2), but not inside a tag: decoding & in an attribute could turn an approved URL into a denied one, and there is a test pinning that. CRLF→LF, <br />→<br>, implied <tbody>, mid-line >, the <pre> leading newline and removed raw-text element content are inherent to cleaning and are now documented in the module and in docs/safe-outputs.md.

Each item has a regression test, plus an idempotency test over the corpus and the tricky inputs. cargo test --all-targets and cargo clippy --all-targets are green (one pre-existing warning in a create_work_item test), and CodeQL found nothing.