Enforce gh-aw bash blocklist across workflow, prompt, and skill surfaces#2090
Enforce gh-aw bash blocklist across workflow, prompt, and skill surfaces#2090
Conversation
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
🏷️ Automatic Labeling SummaryThis PR has been automatically labeled based on the files changed and PR metadata. Applied Labels: size-xs Label Categories
For more information, see |
…ents across workflow/prompt/skill surface - Add tests/agentic-workflow-bash-safety.test.ts (118 tests) scanning .github/workflows/news-*.md, .github/prompts/*.md, and .github/skills/**/*.md fenced code blocks for the 4 banned patterns documented in 01-bash-and-shell-safety.md (parameter transformations, indirect expansion, nested $(...) , eval / bash -c "$var" / source /dev/stdin) - Verify the safety doc still names the 3 allowed eval replacements (arrays, case, explicit branches) - Fix nested $(...$(...)...) staged-injection shape in gh-aw-github-actions-integration/SKILL.md PAT-expiration example by pre-capturing each substitution into EXP_EPOCH / NOW_EPOCH variables Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/7d5e5371-c642-485d-b5a0-ff7494826890 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
- Document zsh fence in extractShellBlocks JSDoc (matches the actual fence list) - Handle already-expired PAT case in skill example (negative DAYS_UNTIL_EXPIRATION) so the example does not silently mis-trigger when a token is already expired Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/7d5e5371-c642-485d-b5a0-ff7494826890 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
Hardens agent-visible shell snippets against the gh-aw AWF sandbox blocklist by updating a PAT-expiration example and adding a Vitest suite that scans workflow/prompt/skill markdown for banned bash patterns.
Changes:
- Updates the PAT-expiration snippet to avoid nested command substitutions and adds handling for already-expired tokens.
- Adds a new Vitest suite that extracts fenced shell blocks from agent-visible markdown and fails CI on banned patterns, with guard tests to detect drift in the safety doc wording.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/skills/gh-aw-github-actions-integration/SKILL.md |
Refactors the PAT expiration calculation to avoid nested substitutions and adds an “already expired” branch. |
tests/agentic-workflow-bash-safety.test.ts |
New test that scans fenced shell code blocks in workflows/prompts/skills for sandbox-blocked bash patterns and validates safety-doc consistency. |
| // Match eval followed by a string- or variable-bearing argument. | ||
| // Eval on plain-literal flag-only invocations is also banned in our | ||
| // workflows -- there is no safe eval use in this repo. | ||
| regex: | ||
| /(^|[^A-Za-z_./])(eval\s+["'$\\]|bash\s+-c\s+["'][^"']*\$|source\s+\/dev\/stdin)/, |
There was a problem hiding this comment.
The eval banned-pattern regex only matches eval when followed by a quote/$/backslash, so cases like eval $var or eval -- "$var" may not be attributed to the eval-on-variable pattern (they’d only be caught by the later “eval outside safety doc” guard). If the intent is “no eval anywhere”, consider widening this regex so the per-file violation report reliably points to the eval rule and safe rewrite.
| // Match eval followed by a string- or variable-bearing argument. | |
| // Eval on plain-literal flag-only invocations is also banned in our | |
| // workflows -- there is no safe eval use in this repo. | |
| regex: | |
| /(^|[^A-Za-z_./])(eval\s+["'$\\]|bash\s+-c\s+["'][^"']*\$|source\s+\/dev\/stdin)/, | |
| // Match any eval token, including cases such as `eval $var` and | |
| // `eval -- "$var"`, plus the existing bash -c / source /dev/stdin forms. | |
| // There is no safe eval use in this repo. | |
| regex: | |
| /(^|[^A-Za-z_./])(eval(?:\s|$)|bash\s+-c\s+["'][^"']*\$|source\s+\/dev\/stdin)/, |
| .readdirSync(PROMPTS_DIR) | ||
| .filter((f) => f.endsWith('.md') && f !== SAFETY_DOC) | ||
| .map((f) => path.join(PROMPTS_DIR, f)), | ||
| ...listMarkdown(SKILLS_DIR), |
There was a problem hiding this comment.
listMarkdown(SKILLS_DIR) collects every .md under .github/skills/ (including README.md and any non-skill docs), but the header comment and PR description scope this to **/SKILL.md. Scanning extra markdown may create noisy failures unrelated to agent-visible skill bodies. Consider restricting the skills scan to **/SKILL.md files (or update the documented scope accordingly).
| ...listMarkdown(SKILLS_DIR), | |
| ...listMarkdown(SKILLS_DIR).filter((file) => path.basename(file) === 'SKILL.md'), |
| echo "❌ Token already expired ($DAYS_UNTIL_EXPIRATION days ago) — rotate immediately" | ||
| gh issue create \ | ||
| --title "🔐 GitHub PAT Expired" \ | ||
| --body "Personal Access Token expired $((-DAYS_UNTIL_EXPIRATION)) days ago." \ |
There was a problem hiding this comment.
In the expired-token branch, the log message prints a negative day count (e.g. “expired (-3 days ago)”), while the issue body correctly converts it to a positive number. Consider echoing the absolute day count as well to avoid confusing operators reading the workflow logs.
| echo "❌ Token already expired ($DAYS_UNTIL_EXPIRATION days ago) — rotate immediately" | |
| gh issue create \ | |
| --title "🔐 GitHub PAT Expired" \ | |
| --body "Personal Access Token expired $((-DAYS_UNTIL_EXPIRATION)) days ago." \ | |
| EXPIRED_DAYS=$((-DAYS_UNTIL_EXPIRATION)) | |
| echo "❌ Token already expired ($EXPIRED_DAYS days ago) — rotate immediately" | |
| gh issue create \ | |
| --title "🔐 GitHub PAT Expired" \ | |
| --body "Personal Access Token expired $EXPIRED_DAYS days ago." \ |
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i] ?? ''; | ||
| const fenceMatch = line.match(/^```([A-Za-z0-9_+-]*)\s*$/); | ||
| if (fenceMatch) { | ||
| if (!inBlock) { | ||
| inBlock = true; | ||
| lang = (fenceMatch[1] ?? '').toLowerCase(); | ||
| bodyStart = i + 1; | ||
| bodyLines = []; | ||
| } else { | ||
| if (lang === '' || ['bash', 'sh', 'shell', 'console', 'zsh'].includes(lang)) { | ||
| blocks.push({ startLine: bodyStart + 1, body: bodyLines.join('\n') }); | ||
| } | ||
| inBlock = false; |
There was a problem hiding this comment.
extractShellBlocks only recognizes fences that start at column 0 (`^````), but many markdown files in this repo indent fenced blocks (e.g. under lists with leading spaces). Those indented blocks won’t be scanned, so banned patterns can slip in undetected. Consider allowing leading whitespace on both opening and closing fences (while still capturing the correct start line).
| /** | ||
| * The blocklist -- kept in sync with the table in | ||
| * .github/prompts/01-bash-and-shell-safety.md (Banned expansion patterns). | ||
| */ | ||
| const BANNED_PATTERNS: readonly BannedPattern[] = [ |
There was a problem hiding this comment.
The comment says the blocklist is “kept in sync” with .github/prompts/01-bash-and-shell-safety.md, but the safety doc currently lists additional sandbox-blocked shapes (e.g. chained builder assignments, the echo "…$(cmd)…" + another $(…) false-positive pattern, and the inline array+[@] case). As written, this test won’t prevent reintroducing those documented blocked patterns. Either expand BANNED_PATTERNS to cover all rows in the safety doc, or adjust the doc/test wording so they match.
|
@copilot apply changes based on the comments in this thread |
…ten scope - Expand BANNED_PATTERNS to all 7 rows of `01-bash-and-shell-safety.md`: add chained-builder $(\$var), echo/printf with two $(...), and inline array+[@] shapes - Widen eval regex to catch `eval $var`, `eval -- "$var"`, plain `eval cmd` so per-file diagnostics name the eval-on-variable rule - Allow indented fenced shell blocks (e.g. nested under list items) — opening/closing fence tolerate matching leading whitespace - Restrict skill scan to `**/SKILL.md` only — README.md and other skill docs are not in the agent context - Use EXPIRED_DAYS variable in expired-token branch so the log line shows a positive day count Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/38522090-0594-47e1-bbad-f3276a3bdda8 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all five review suggestions in e15ced1:
Verified all 7 patterns by poison-injection, including the indented fence. |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
Recent threat-detection warnings on issue #1889 surfaced a request to harden agent-visible bash against the AWF sandbox blocklist documented in
.github/prompts/01-bash-and-shell-safety.mdand to test that only the three allowedevalreplacements (arrays /case/ explicit branches) are used.Audit
Scanned every fenced shell block (including indented fences nested under list items) under
.github/workflows/news-*.md,.github/prompts/*.md, and.github/skills/**/SKILL.mdagainst every banned pattern row in the safety doc. One offender outside the safety doc itself: nested$(…$(…)…)inside$((…))in the PAT-expiration example.Changes
.github/skills/gh-aw-github-actions-integration/SKILL.md— pre-capture each$(date …)into its own variable; also handle the already-expired (negative-days) branch with anEXPIRED_DAYS=$((-DAYS_UNTIL_EXPIRATION))helper so the workflow log line shows a positive day count.tests/agentic-workflow-bash-safety.test.ts(new, 114 tests) — extracts every fenced shell block (column-0 or indented fences with matching open/close indentation) and matches against the full 7-row blocklist from01-bash-and-shell-safety.md:${var@P/Q/E/A/a}parameter transformations${!var}indirect expansion$(…$(…)…)(incl. inside$((…)))eval(any form:eval $var,eval -- "$var", plaineval cmd) /bash -c "$var"/source /dev/stdin <<<c=$($b)(re-parsing a variable as a command)echo/printflines with two$(…)substitutions (AWF false-positive nested-cmd-subst)(…)expanded with${arr[@]}in the same command stringFailures emit
file:line + pattern + safe-rewrite. Two outer guards verify the safety doc still spells out all four base banned shapes and still names the three allowedevalreplacements, so future drift fails loudly. The safety doc itself is the only allowlisted source of pattern strings.Skill scan is restricted to
**/SKILL.md(the bodies actually mounted into the agent context) —README.mdand other non-skill docs under.github/skills/are out of scope.Verified by poison-injection that all 7 patterns trigger with the expected per-pattern diagnostics, including indented fences.