Skip to content

fix(safeoutputs): use sanitize_config for identifier fields instead of sanitize_text#433

Merged
jamesadevine merged 1 commit into
mainfrom
fix/sanitize-identifier-fields
May 7, 2026
Merged

fix(safeoutputs): use sanitize_config for identifier fields instead of sanitize_text#433
jamesadevine merged 1 commit into
mainfrom
fix/sanitize-identifier-fields

Conversation

@jamesadevine
Copy link
Copy Markdown
Collaborator

@jamesadevine jamesadevine commented May 7, 2026

Problem

sanitize_text() (the full sanitization pipeline) wraps @-signs in backticks to prevent ADO mentions, escapes HTML tags, neutralizes bot triggers, etc. These transforms are correct for rich-text content rendered as markdown/HTML in ADO UI, but they corrupt identifier fields like tags, branch names, labels, and repository names.

Example: A work item tag copilot:repo=msazuresphere/4x4/VsCodeExtension@main gets transformed to copilot:repo=msazuresphere/4x4/VsCodeExtension `@main` — which then fails allowed-tags validation because the backtick-wrapped form doesn't match the allowlist pattern.

[WARN] [ado_aw::execute] create-work-item failed: Agent-provided tags not in allowed-tags: copilot:repo=msazuresphere/4x4/VsCodeExtension`@main`

Fix

Switch all identifier fields from sanitize_text() → sanitize_config() across 10 safe-output files. sanitize_config() only strips control characters, neutralizes pipeline commands (##vso[/##[), and enforces size limits — safe for identifiers without corrupting them.

Fields changed to sanitize_config

File Fields
add_build_tag.rs tag
create_work_item.rs tags
update_work_item.rs state, area_path, iteration_path, assignee, tags
create_pr.rs labels
create_branch.rs branch_name
queue_build.rs branch, parameters (keys & values)
link_work_items.rs link_type
resolve_pr_thread.rs status, repository
submit_pr_review.rs event, repository
update_pr.rs repository, operation, reviewers, labels, vote

Fields kept on sanitize_text

Rich-text fields: title, description, body, content, comment, reason — these are rendered as markdown/HTML and need the full pipeline.

Test updates

Updated update_work_item::test_sanitize_fields to assert that tags are preserved as-is (no backtick wrapping of @), reflecting the corrected behavior.

Validation

  • cargo build ✅
  • cargo test ✅ (all pass)
  • cargo clippy --all-targets --all-features ✅

@jamesadevine
fix(safeoutputs): use sanitize_config for identifier fields instead o…
69aaa9b 
…f sanitize_text

sanitize_text() wraps @-signs in backticks and escapes HTML, which corrupts identifier fields like tags, branch names, labels, and repo names.

Switch all identifier fields across 10 safe-output files from sanitize_text() to sanitize_config(), which only strips control characters, neutralizes pipeline commands, and enforces size limits. Rich-text fields (title, description, body, content, comment, reason) remain on sanitize_text().

Co-authored-by: Copilot <[email protected]>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

🔍 Rust PR Review

Summary: Looks good — the fix is correct, well-scoped, and preserves all existing security guarantees.

Findings

✅ What Looks Good

  • Root cause is correctly diagnosed. sanitize_text wraps @ in backticks (IS-04 mention neutralization) which is right for markdown/HTML-rendered content but corrupts identifiers like [email protected], branch names, and tags containing @. Switching to sanitize_config is the right fix.

  • Rich-text fields are consistently preserved. Every field rendered as markdown/HTML (title, description, body, content, comment, reason) correctly keeps sanitize_text. The field classification is accurate across all 10 files.

  • ##vso[ / ##[ injection protection is retained. sanitize_config still calls neutralize_pipeline_commands, so the pipeline-command injection guard is not weakened.

  • queue_build.rs expression-syntax check is safe. The $( / ${{ / $[ rejection at lines 219–227 runs in execute_impl against the already-sanitized self.parameters. sanitize_config does not strip those patterns, so the check still fires correctly — no bypass introduced.

  • create_work_item.rs assignee is operator-controlled, not agent-supplied, so it correctly lives in CreateWorkItemConfig (covered by #[derive(SanitizeConfig)]) and is unaffected by this PR. Good scoping.

  • Test update in update_work_item::test_sanitize_fields correctly flips the assertion from `@two` to the literal "tag @two", directly exercising the corrected behavior.

  • Ordering is consistent: sanitize_content_fields is called before execute_impl, so the sanitized values are what reach both the allowlist checks and the API calls — no double-sanitization or order-of-operations issues.

Generated by Rust PR Reviewer for issue #433 · ● 300.9K · ◷

@jamesadevine jamesadevine merged commit ea43b11 into main May 7, 2026
14 checks passed
@github-actions github-actions Bot mentioned this pull request May 7, 2026
Merged
@jamesadevine jamesadevine deleted the fix/sanitize-identifier-fields branch May 7, 2026 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jamesadevine