fix(safe-outputs): make create-work-item body field configurable - #1926
Conversation
|
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. |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
/review |
|
✅ PR Security Reviewer completed the security review.
|
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed for ado-aw's compiler-contract invariants (front-matter grammar, safe-output schema/newtype rules, generated-artifact drift, docs sync).
- Front-matter grammar:
description_field: Option<AdoWorkItemFieldRef>with#[serde(default)]is backward compatible — no codemod needed. - Newtype rule: correctly followed —
description-fielduses the newAdoWorkItemFieldRefvalidated newtype (src/secure.rs) backed byvalidate_ado_work_item_field_ref(src/validate.rs), not a rawString. This also protects the/multilineFieldsFormat/{description_field}JSON Patch path from Pointer-syntax corruption. - Docs sync:
docs/front-matter.mdanddocs/safe-outputs.mdwere both updated with the newdescription-fieldoption and defaulting behavior — good. - No generated-artifact drift: this PR only touches
src/anddocs/; noado-scriptbundles, IR/gate schema, or.lock.ymlfiles are implicated, so no drift check applies here.
No merge-blocking compiler-contract issues found. This is a well-scoped, correctly-typed change.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 41.4 AIC · ⌖ 2.3 AIC · ⊞ 12.8K
Comment /review to run again
There was a problem hiding this comment.
Solid test coverage overall for the new description-field config and the HTML-preserving sanitize_markdown path (defaults, Bug override, invalid value rejection, executor E2E, sanitizer preserve/neutralize). Two gaps worth addressing before merge:
- No test covers
description-fieldcolliding withcustom-fieldsorSystem.Title, which produces a duplicate JSON Patch path that ADO will likely reject at runtime — this failure mode is currently undetectable except in production. test_sanitize_markdown_neutralizes_active_htmlbundles ~15 distinct attack vectors into one test; a regression in any single vector is masked by the rest passing, making CI failures hard to localize.
Neither is merge-blocking on its own, but the first is a real correctness gap worth at least a regression test.
🧪 Test quality analysis by Test Quality Sentinel · auto · 60.8 AIC · ⌖ 2.84 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
Rust code quality review
No merge-blocking Rust defects found in src/mcp.rs, src/safe_outputs/create_work_item.rs, src/sanitize.rs, src/secure.rs, src/validate.rs.
💡 Notes
description_field_forcorrectly derives its fallback to the validatedAdoWorkItemFieldReftype, andvalidate_ado_work_item_field_refrejects.///~characters that would otherwise corrupt the JSON Patch pointer path built infield_op— good defensive design.- Minor, non-blocking:
strip_dangerous_html_tagsandstrip_event_handler_attrs_in_tagsinsrc/sanitize.rseach re-implement an almost identical tag-scanning loop aroundhtml_tag_end; a shared helper that takes a per-tag transform closure would remove the duplication, but it's not a correctness issue and isn't worth a blocking comment. - The
rust-criticsub-agent returned no output for this diff after repeated polling; I completed the review from my own pass only. - Error handling, casts, path handling, and concurrency all looked sound in the changed lines —
anyhow::Result/.context()used consistently, no newunwrap()/expect()on agent-input paths, no lossy casts introduced.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 79.3 AIC · ⌖ 2.23 AIC · ⊞ 11.4K
Comment /review to run again
There was a problem hiding this comment.
One merge-blocking finding: the new strip_dangerous_html_tags HTML sanitizer in src/sanitize.rs is single-pass and vulnerable to the classic nested-tag bypass (e.g. <scr<script>ipt>alert(1)</scr</script>ipt> reconstructs a live <script> tag after stripping). I verified this against a standalone extraction of the exact diff algorithm. Since sanitize_markdown is now used for work-item descriptions, and description-field routes Bug items to a Markdown-rendered field, this is a stored-XSS-class regression introduced by this PR (the old sanitize path fully HTML-escaped, so this preserving-HTML replacement needs to close this gap before merge). See inline comment for details and a suggested fix (loop the tag-stripping pass to a fixed point, or use a real tokenizer).
🔒 Security review by PR Security Reviewer · auto · 108.4 AIC · ⌖ 7.28 AIC · ⊞ 11.7K
Comment /review to run again
|
@copilot implement suggestions and resolve blocking findings. |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Addressed in 48b6a6f through a287f33. The follow-up strips slash-separated event-handler attributes, pre-caps Markdown before the expensive HTML pass, makes custom-field patch ordering deterministic, and adds the requested collision/folded-tag sanitizer coverage. |
|
/review |
|
✅ PR Security Reviewer completed the security review. Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Solid test coverage overall: the new description-field config, patch-field collision validation, and the HTML-preserving markdown sanitizer all have targeted unit tests plus one mocked-HTTP executor test. Found a few gaps that would let regressions ship silently — none are blocking, all are advisory additions to widen coverage on the case-insensitive Bug-type match and the full collision matrix.
🧪 Test quality analysis by Test Quality Sentinel · auto · 65.7 AIC · ⌖ 2.65 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
Rust review — no blocking issues found
Reviewed src/mcp.rs, src/safe_outputs/create_work_item.rs, src/sanitize.rs, src/secure.rs, src/validate.rs.
AdoWorkItemFieldRefvalidation is strict and correctly guards against JSON Pointer path corruption in/fields/<ref>.sorted_custom_fieldsfixes what would otherwise be nondeterministicHashMapiteration in generated JSON Patch output — good catch, and it's covered bytest_sorted_custom_fields_orders_case_insensitively.validate_patch_fieldscorrectly detects field collisions (title/description/custom-fields) before use, avoiding silent data loss on the ADO patch document.- The new
sanitize_markdown/dangerous-tag stripping logic insanitize.rs(fixed-point loop overstrip_dangerous_html_tags, event-handler-attribute stripping, protected fenced-code ranges) is thorough and has strong adversarial test coverage (nested/folded tag reconstruction, attribute-span folding, unclosed tags). - No
unwrap()/expect()on user-reachable paths; error handling usesanyhow::ensure!/bail!with actionable messages throughout. cargo buildsucceeds cleanly.
The Compiler Contract reviewer owns the front-matter/schema/docs side of this change; nothing further from a pure Rust-engineering standpoint.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
spsprodeus21.vssps.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "spsprodeus21.vssps.visualstudio.com"See Network Configuration for more information.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 66.5 AIC · ⌖ 5.98 AIC · ⊞ 11.4K
Comment /review to run again
There was a problem hiding this comment.
Compiler-contract review: front-matter grammar, safe-output docs, and the new AdoWorkItemFieldRef newtype all look correctly wired (properly uses validated_string!/src/validate.rs, not a raw String). Tests are thorough and pass locally (cargo test --bin ado-aw create_work_item). One finding posted inline: the new validate_patch_fields collision check only runs in the Stage 3 executor, not in the compile-time validate_work_item_assignment_outputs_config, so a misconfigured description-field/custom-fields collision is discovered only after a pipeline runs, not at ado-aw compile/lint time.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
spsprodeus21.vssps.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "spsprodeus21.vssps.visualstudio.com"See Network Configuration for more information.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 107.6 AIC · ⌖ 2.45 AIC · ⊞ 12.8K
Comment /review to run again
There was a problem hiding this comment.
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Reviewed the sanitize/validate/create-work-item changes. The new AdoWorkItemFieldRef typed newtype, validate_ado_work_item_field_ref, and the config sanitization (SanitizeConfig impl covering all string fields including custom-field keys/values and artifact-link) look correctly scoped and don't weaken any existing control.
One finding worth blocking on: sanitize_markdown (new src/sanitize.rs function used by create-work-item descriptions) preserves raw HTML while still relying on the pre-existing sanitize_url_protocols, which does a literal substring search for (redacted) That check is bypassable with an embedded whitespace/control character in the scheme (java\tscript:), and because the surrounding HTML is no longer escaped to inert entities (unlike the original sanitize()path), this bypass is now live rather than latent — a malicious agent-authored work item description could smuggle a(redacted) URI into a rendered <a href>/<img src>. See inline comment for a reproduction and suggested fix (normalize/strip whitespace from the scheme before matching, or operate on parsed attribute values).
Everything else — token handling, custom-field key typing, patch-field duplicate-field validation, and the dangerous-tag/event-handler stripping logic — is sound.
🔒 Security review by PR Security Reviewer · auto · 87.8 AIC · ⌖ 11 AIC · ⊞ 11.7K
Comment /review to run again
Superseded by updated review from same workflow.
|
@copilot perform a comprehensive review of santization. Identify if there are crates we can leverage to reduce complexity and corner cases. Submit a full report comment on this pr. |
Sanitization review report: Scope reviewed
Current state
Main concern Residual risk areas
Crates to leverage
Recommended follow-up design
Recommendation for this PR |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ PR Security Reviewer completed the security review.
|
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
|
✅ Test Quality Sentinel completed the test quality analysis.
|
|
✅ Compiler Contract Reviewer completed the compiler contract review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "spsprodeus21.vssps.visualstudio.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Compiler Contract Review
No compiler-contract issues found. Highlights:
description-fieldisOption<AdoWorkItemFieldRef>with a sane default fallback (description_field_for), so existing.mdsources without the new key keep parsing — no codemod needed.custom-fieldskeys were switched from rawStringto the newAdoWorkItemFieldRefvalidated newtype (src/secure.rs), which is exactly the pattern this reviewer enforces for identifier-like config fields, andvalidate_ado_work_item_field_refis unit-tested with both valid and invalid cases.- Since
custom_fieldsis nowHashMap<AdoWorkItemFieldRef, String>,#[derive(SanitizeConfig)]can no longer auto-generate the impl (its codegen only recognizesHashMap<String, String>), so the PR correctly drops the derive and hand-writesSanitizeConfig for CreateWorkItemConfig— consistent with the existing hand-written pattern inassign_github_issue_milestone.rs. docs/front-matter.mdanddocs/safe-outputs.mdwere both updated for the newdescription-fieldoption and description-format semantics.sanitize_markdownis correctly scopedpub(crate)and used consistently from bothmcp.rs(proposal write) andcreate_work_item.rs'sSanitizeContentimpl (pre-execution).
No lock-file, codegen, or documentation drift detected for this change.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
spsprodeus21.vssps.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "spsprodeus21.vssps.visualstudio.com"See Network Configuration for more information.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 61 AIC · ⌖ 2.7 AIC · ⊞ 12.7K
Comment /review to run again
There was a problem hiding this comment.
Solid test coverage overall for the new description-field config and the sanitize_markdown HTML-preserving sanitizer — happy paths, defaults, validation errors, and several adversarial HTML/attribute-splitting cases are all exercised, and the executor test correctly asserts on the outgoing JSON patch body rather than internals.
Two gaps worth closing (see inline comments):
- No test covers a
custom-fieldscollision against an explicitly configureddescription-field(only the two default-field collision cases are tested), so the"description-field"vs"default description-field"label branch invalidate_patch_fieldsis unprotected. - The new HTML-tag/event-handler stripping logic is only tested with lowercase tag/attribute names; no test asserts case-insensitive matching (
<SCRIPT>,ONCLICK=), despite the implementation relying onto_ascii_lowercase()/(?i)for it.
Neither is blocking, but both are plausible regression vectors that current tests wouldn't catch.
🧪 Test quality analysis by Test Quality Sentinel · auto · 85.9 AIC · ⌖ 1.91 AIC · ⊞ 11.2K
Comment /review to run again
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_rejects_duplicate_description_custom_field() { |
There was a problem hiding this comment.
No test exercises description_field collisions with custom-fields when the field is explicitly configured (not the Bug/default case) — only the two default cases (test_patch_field_validation_rejects_custom_field_description_collision for Bug's default, and ..._default_system_description_collision for Task's default) are covered.
💡 Why this matters
description_field_for prefers config.description_field when set. If a user sets description-field: Custom.Body and also adds Custom.Body to custom-fields, the collision path uses the "description-field" label (line ~1046: let description_label = if config.description_field.is_some() { "description-field" } else { "default description-field" };). None of the existing tests hit this branch — a regression there (e.g. always reporting "default description-field") would ship silently, and the error-message wording is user-facing.
Suggested addition:
#[test]
fn test_patch_field_validation_rejects_custom_field_explicit_description_field_collision() {
let mut config = CreateWorkItemConfig {
description_field: Some(AdoWorkItemFieldRef::parse("Custom.Body").unwrap()),
..Default::default()
};
config.custom_fields.insert(
AdoWorkItemFieldRef::parse("Custom.Body").unwrap(),
"overridden".to_string(),
);
let error = validate_patch_fields(&config, description_field_for(&config), false)
.unwrap_err()
.to_string();
assert_eq!(
error,
"custom-fields field 'Custom.Body' duplicates description-field field 'Custom.Body'"
);
}| let output = sanitize_markdown("<style>body{display:none}</style><p>safe</p>"); | ||
|
|
||
| assert_eq!(output, "body{display:none}<p>safe</p>"); | ||
| assert!(!output.contains("<style")); |
There was a problem hiding this comment.
The neutralize_dangerous_html/sanitize_url_protocols combo is only tested for the happy path (HTML preserved, dangerous tags stripped) — no test covers the case-insensitivity of tag/attribute matching (e.g. <SCRIPT>, <ScRiPt data-x=...>, ONCLICK=), even though is_dangerous_html_tag lowercases and the event-handler regexes use (?i).
💡 Why this matters
This is new, security-sensitive sanitization logic (XSS-style payload stripping) shipping in this PR. Case-insensitive tag/attribute matching is exactly the kind of thing that regresses silently if someone "simplifies" to_ascii_lowercase() out of is_dangerous_html_tag, since none of the current tests would catch it (they all use lowercase tag names). A single test asserting <SCRIPT> and onCLICK= are still stripped would close this gap.
There was a problem hiding this comment.
This PR's sanitize_markdown preserves raw HTML instead of escaping it wholesale (as sanitize() did), which is the correct intent for readable work-item Markdown bodies, but it makes the pre-existing sanitize_url_protocols substring-match weakness directly exploitable via whitespace/control-char/entity obfuscation of dangerous URL schemes ((redacted) (redacted) etc.) inside preserved href/src attributes. See inline comment for a reproducible bypass verified against this branch. The rest of the diff (field-ref validation, dangerous-tag stripping, event-handler stripping, duplicate patch-field detection) looks sound.
🔒 Security review by PR Security Reviewer · auto · 95.3 AIC · ⌖ 2.32 AIC · ⊞ 11.7K
Comment /review to run again
| /// Sanitize untrusted Markdown content while preserving ordinary inline HTML for | ||
| /// renderers that accept it natively. | ||
| pub(crate) fn sanitize_markdown(input: &str) -> String { | ||
| let mut s = remove_control_characters(input); | ||
| s = neutralize_pipeline_commands(&s); | ||
| s = neutralize_mentions(&s); | ||
| s = neutralize_bot_triggers(&s); | ||
| s = remove_xml_comments(&s); | ||
| s = enforce_content_limits(&s); | ||
| s = neutralize_dangerous_html(&s); | ||
| s = sanitize_url_protocols(&s); |
There was a problem hiding this comment.
High severity, high confidence: sanitize_url_protocols only does a literal, unescaped substring match for (redacted) which sanitize_markdownnow relies on as its *only* URL-protocol defense while simultaneously preserving raw HTML tags/attributes it previously escaped wholesale viaescape_html_tags`.
💡 Bypass details and fix
Because sanitize() used to run escape_html_tags first, an attacker-controlled <a href="(redacted) would render as harmless escaped text () regardless of whether the protocol string was caught. sanitize_markdownremoves that escaping safety net for tags not in theis_dangerous_html_tagdenylist (e.g., ), so the substring-match weakness in sanitize_url_protocolsbecomes directly exploitable: a work-item description authored by an untrusted agent can render a clickable/executing(redacted) (or `(redacted) URI in the ADO work item UI.
Verified locally (built from this PR's create_work_item.rs/sanitize.rs) that all of these bypass sanitize_markdown unmodified:
<a href="java\tscript:alert(1)">x</a> // tab inside scheme
<a href="java script:alert(1)">x</a> // space inside scheme
<a href="(redacted) // HTML entity-encoded 'j'
Only the literal (unescaped) `(redacted) case is redacted; whitespace/control-char obfuscation and entity-encoding both evade the check.
Fix: normalize/decode the URL scheme before matching (strip control chars/whitespace within the scheme portion, and reject on href/src attribute values whose decoded scheme matches a denylist) rather than doing a naive substring search on the raw markup, since this function is now the last line of defense for HTML that renders live instead of being escaped.
There was a problem hiding this comment.
Verdict: Request changes
One merge-blocking defect found; the rest of the change (typed AdoWorkItemFieldRef, patch-field collision validation, deterministic sorted_custom_fields, work-item description-field selection) is solid, well-tested, and handled error paths appropriately with anyhow/Result rather than panics.
Themes reviewed
- Error handling:
validate_ado_work_item_field_refandAdoWorkItemFieldRefuseanyhow::ensure!/Resultcorrectly; nounwrap()/expect()on attacker-reachable paths increate_work_item.rsorvalidate.rs. - Correctness:
sorted_custom_fieldsfixes priorHashMapiteration non-determinism for generated patch ordering — good.html_tag_end/strip_dangerous_html_tags/strip_event_handler_attrs_in_tagscorrectly operate on byte offsets validated as char boundaries (viachar_indices/.len_utf8()), so no UTF-8 panics observed in testing. - Performance/DoS (blocking): see inline comment on
src/sanitize.rs— the new HTML-neutralization fixed-point loop is quadratic on adversarial nested-tag input and can hang well within the existing byte-size cap. - Did not flag the
rust-criticsub-agent's output separately since it produced no response in the available window; findings above are from my own pass plus a runtime probe.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 88.5 AIC · ⌖ 2.89 AIC · ⊞ 11.4K
Comment /review to run again
| apply_outside_markdown_protected_ranges(input, neutralize_dangerous_html_fragment) | ||
| } | ||
|
|
||
| fn neutralize_dangerous_html_fragment(input: &str) -> String { |
There was a problem hiding this comment.
neutralize_dangerous_html_fragment's fixed-point loop is quadratic (or worse) on adversarial input, giving agent-controlled work-item descriptions a cheap DoS vector against the compiler/executor process.
💡 Reproduction and fix
I ran a local probe (sanitize_markdown on a string of 5,000 repetitions of <scr followed by ipt>tail, i.e. "<scr".repeat(5000) + "ipt>tail", ~20KB — well under MAX_CONTENT_BYTES). The call did not return within 90 seconds. strip_dangerous_html_tags does a full O(n) scan per call, but folded/nested dangerous tags (e.g. <scr<script>...) only get one dangerous tag removed per iteration of the loop { ... } in neutralize_dangerous_html_fragment, so an input can force O(n) iterations of an O(n) scan — O(n2) total, and the existing test test_sanitize_markdown_strips_nested_dangerous_tags_to_fixed_point only exercises this at trivial size (<scr<script>ipt>...), so it never catches the blow-up.
Since sanitize_markdown runs on agent-provided description text at MCP-tool-call time (src/mcp.rs) before any size truncation from enforce_content_limits has capped realistic-but-large inputs, an agent (or content it echoes) can trigger multi-minute (or worse) hangs in the sanitizer with a payload well inside MAX_CONTENT_BYTES (512 KiB).
Fix options:
- Bound the number of fixed-point iterations (e.g. cap at a small constant, or proportional to nesting depth actually observed) and bail out to conservative full-escaping if the cap is hit.
- Replace the repeated full-string rescans with a single properly nested tag-boundary scan (track open
<and dangerous-tag state without needing multiple whole-string passes). - Move
enforce_content_limits(byte truncation) to run beforeneutralize_dangerous_html, and additionally impose a stricter pre-HTML-stripping length cap tuned to the O(n2) cost, so a single MCP call can't spend unbounded CPU regardless of the 512 KiB ceiling.
Summary
create-work-itemalways wrote agent-generated body content toSystem.Description, which makes Bug work items appear blank in processes whose Bug form rendersMicrosoft.VSTS.TCM.ReproStepsinstead. It also escaped body HTML before storing the field as Markdown, producing unreadable content.safe-outputs.create-work-item.description-field.BugtoMicrosoft.VSTS.TCM.ReproSteps.System.Description.Body sanitization
Docs and coverage
description-fieldand Markdown body semantics.Test plan
cargo test create_work_itemcargo test sanitize_markdowncargo test validate_ado_work_item_field_refcreate-work-itemhardcodesSystem.Description, so Bug work items are filed blank #1924