From c1f193a9d43f94b5e31374e4461b3153a84d4b5e Mon Sep 17 00:00:00 2001 From: Saffron <263493777+itsmiso-ai@users.noreply.github.com> Date: Thu, 13 Aug 2026 06:23:41 +0000 Subject: [PATCH] Fix #495: extend assert_safe_artifact_paths to cover every write target The symlink guard added for #406 (PR #411) refused PR-controlled symlinks only at the 45 artifact names hard-coded in scripts/artifact_paths.sh. The pipeline wrote 19 more artifact files into the same CWD (the reviewed PR checkout) that were not in the guard list, so a malicious PR that tracked a symlink at one of those names still redirected the write outside the workspace on persistent self-hosted runners. The guard list also silently drifted from the write inventory with no test keeping them in sync. This change addresses all three acceptance criteria from #495: 1. Every file the action writes is covered by assert_safe_artifact_paths. Added the 19 literal paths the issue called out (findings.json, review-request.json, review-verdict-body.md, inline-findings-body.md, review-comments.json, finding-threads.json, resolve-findings.json, pr-files.json, pr-files.raw.json, linked-issue.raw.json, linked-issues.json, linked-issues.merged.json, linked-issue.filtered.json, linear-issues.json, linear-issues.md, terms.txt, terms.all.txt, review-corpus.body.md, repo-impact.combined.txt) plus ai-response.primary.json, which the new drift test flagged as an additional unguarded write target in scripts/run_tool_harness.py. 2. New CI check tests/test_artifact_paths_drift.sh derives the write inventory from literal `> filename.ext` redirects in action.yml and from write_text / open() calls under scripts/, then fails (exit 1) when any discovered target is missing from the artifact_paths array. Modeled on tests/test_env_consistency.sh so it follows the existing test pattern and runs in the same `bash tests/test_*.sh` step of .github/workflows/ci.yaml. 3. tests/test_artifact_paths.sh now loops over each of the 19 previously- unguarded paths, plants a symlink at it pointing to a file the harness would otherwise overwrite, and asserts that assert_safe_artifact_paths refuses the write and leaves the target untouched. The test also keeps the original negative/positive cases for the pre-existing guard list plus the sanity check that check_review_needed.sh actually sources the guard function before the publish step writes. Fixes #495 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com> --- scripts/artifact_paths.sh | 11 ++- tests/test_artifact_paths.sh | 33 ++++++++- tests/test_artifact_paths_drift.sh | 104 +++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 2 deletions(-) create mode 100755 tests/test_artifact_paths_drift.sh diff --git a/scripts/artifact_paths.sh b/scripts/artifact_paths.sh index 7f96915..aa21897 100644 --- a/scripts/artifact_paths.sh +++ b/scripts/artifact_paths.sh @@ -16,10 +16,19 @@ assert_safe_artifact_paths() { standards-context.md standards-present.txt tool-harness.md tool-harness.json review-corpus.md review-corpus.truncated.md review-corpus.fallback.truncated.md ai-request.json ai-response.json ai-output.json ai-output.primary.json - ai-request.fallback.json ai-response.fallback.json + ai-request.fallback.json ai-response.fallback.json ai-response.primary.json verdict.txt analysis_engine.txt review-markdown.raw.md review-comment-markdown.raw.md review-comment.md review-comment-body.md review-body.md inline-comments.json + findings.json review-request.json + review-verdict-body.md inline-findings-body.md + review-comments.json finding-threads.json resolve-findings.json + pr-files.json pr-files.raw.json + linked-issue.raw.json linked-issues.json linked-issues.merged.json + linked-issue.filtered.json + linear-issues.json linear-issues.md + terms.txt terms.all.txt + review-corpus.body.md repo-impact.combined.txt ) for path in "${artifact_paths[@]}"; do diff --git a/tests/test_artifact_paths.sh b/tests/test_artifact_paths.sh index 7f298bc..e5ff797 100755 --- a/tests/test_artifact_paths.sh +++ b/tests/test_artifact_paths.sh @@ -5,6 +5,22 @@ ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" # shellcheck source=scripts/artifact_paths.sh source "$ROOT_DIR/scripts/artifact_paths.sh" +# Every path that was previously written-but-unguarded must now be rejected when +# a PR-controlled symlink appears at it. The list mirrors the additions in +# scripts/artifact_paths.sh and must be kept in sync with that guard list (the +# drift test fails CI when it does not). +new_paths=( + findings.json review-request.json + review-verdict-body.md inline-findings-body.md + review-comments.json finding-threads.json resolve-findings.json + pr-files.json pr-files.raw.json + linked-issue.raw.json linked-issues.json linked-issues.merged.json + linked-issue.filtered.json + linear-issues.json linear-issues.md + terms.txt terms.all.txt + review-corpus.body.md repo-impact.combined.txt +) + TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT cd "$TMP" @@ -25,5 +41,20 @@ rm pr.json ln -s "$outside" legitimate-repo-link assert_safe_artifact_paths +for path in "${new_paths[@]}"; do + ln -sf "$outside" "$path" + if assert_safe_artifact_paths 2>error.txt; then + echo "FAIL: symlinked unguarded artifact '$path' was accepted" >&2 + exit 1 + fi + if ! grep -q "Refusing to write review artifact through symlink: $path" error.txt; then + echo "FAIL: expected refusal message for '$path' missing from stderr" >&2 + cat error.txt >&2 + exit 1 + fi + [[ "$(cat "$outside")" == "unchanged" ]] + rm -f "$path" +done + grep -q 'assert_safe_artifact_paths' "$ROOT_DIR/scripts/check_review_needed.sh" -echo "PASS: generated artifact symlinks are rejected before precheck writes" +echo "PASS: generated artifact symlinks (existing + new) are rejected before precheck writes" diff --git a/tests/test_artifact_paths_drift.sh b/tests/test_artifact_paths_drift.sh new file mode 100755 index 0000000..3230451 --- /dev/null +++ b/tests/test_artifact_paths_drift.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# test_artifact_paths_drift.sh — CI gate against assert_safe_artifact_paths +# drift (#495). +# +# The symlink guard in scripts/artifact_paths.sh is the only thing that +# refuses PR-controlled symlinks before the publish step writes model output +# through them on persistent self-hosted runners. The list used to be a +# hand-maintained copy of the write inventory — 19 of those writes were not +# listed, so a tracked symlink at any of those names wrote the review outside +# the workspace. That bug class rots silently without an automated check, so +# this test derives the write inventory from action.yml / scripts/ and refuses +# to pass when a write target is missing from the guard list. +# +# Invariant enforced (hard FAIL, not a warning): every literal filename that +# the action or its scripts redirects or writes into the working directory +# (the reviewed PR checkout) appears in the artifact_paths array in +# scripts/artifact_paths.sh. Writes through variables or pipes are not +# covered by the grep, so this is a conservative over-approximation of the +# true write inventory; the test still catches the bug class because the +# unguarded files were all written with literal `> filename.ext` patterns. +# +# Exclusions: +# - scratch paths under /tmp (write_targets may write there for diffs) +# - /dev/null +# - filenames already known to be covered +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +ARTIFACT_PATHS_SH="$REPO_ROOT/scripts/artifact_paths.sh" +ACTION_YML="$REPO_ROOT/action.yml" + +if [ ! -f "$ARTIFACT_PATHS_SH" ]; then + echo "FAIL: $ARTIFACT_PATHS_SH not found" >&2 + exit 1 +fi +if [ ! -f "$ACTION_YML" ]; then + echo "FAIL: $ACTION_YML not found" >&2 + exit 1 +fi + +# 1. Collect the guard list from the artifact_paths array in +# scripts/artifact_paths.sh. The script defines the array inside a +# function, so we extract from the literal `local -a artifact_paths=(` +# block rather than relying on sourcing the file. +extract_guard_paths() { + sed -n '/^[[:space:]]*local[[:space:]]*-a[[:space:]]\+artifact_paths=(/,/^[[:space:]]*)/p' "$ARTIFACT_PATHS_SH" \ + | grep -oE '[a-zA-Z][a-zA-Z0-9._-]*\.[a-zA-Z0-9]+' \ + | sort -u +} + +guard_paths="$(extract_guard_paths)" +if [ -z "$guard_paths" ]; then + echo "FAIL: could not extract guard list from $ARTIFACT_PATHS_SH — the file was restructured; update extract_guard_paths" >&2 + exit 1 +fi + +# 2. Collect literal filenames written with `> filename.ext` from shell and +# Python sources under the repo. Match against the working directory (the +# reviewed checkout), which is what assert_safe_artifact_paths guards. +extract_write_targets() { + # action.yml uses printf/jq/heredocs that end in `> filename` + grep -hnE '>[[:space:]]+[A-Za-z_./-][A-Za-z0-9_./-]*\.(json|md|txt|yaml|yml|diff)([[:space:]]|$|"|$)' "$ACTION_YML" 2>/dev/null || true + # scripts/ — bash, python + find "$REPO_ROOT/scripts" -type f \( -name '*.sh' -o -name '*.py' \) \ + -exec grep -hnE '>[[:space:]]+[A-Za-z_./-][A-Za-z0-9_./-]*\.(json|md|txt|yaml|yml|diff)([[:space:]]|$|"|$)' {} + +} + +# Python write_text / open() targets — separate pass. +extract_python_writes() { + find "$REPO_ROOT/scripts" -type f -name '*.py' -exec grep -hnE "(write_text|Path\(.*\)\.write|\.write_bytes|open\(.*['\"]w['\"])" {} + \ + | grep -oE "['\"][A-Za-z0-9._/-]+\.(json|md|txt|yaml|yml|diff)['\"]" \ + | sed -e "s/^'//" -e "s/'$//" -e 's/^"//' -e 's/"$//' \ + | grep -v '^/' \ + | sort -u || true +} + +write_targets="$({ extract_write_targets; extract_python_writes; } | grep -oE '[a-zA-Z][a-zA-Z0-9._-]*\.(json|md|txt|yaml|yml|diff)' | sort -u)" + +# 3. Cross-check. +failures=0 +missing="" +while IFS= read -r target; do + [ -z "$target" ] && continue + # Exclusions: scratch files outside the checkout + case "$target" in + /dev/null|/tmp/*|/var/*) continue ;; + esac + if ! printf '%s\n' "$guard_paths" | grep -qx "$target"; then + missing="${missing}${target}"$'\n' + failures=$((failures + 1)) + fi +done <<< "$write_targets" + +if [ "$failures" -gt 0 ]; then + echo "FAIL: $failures artifact write target(s) are missing from SAFE_ARTIFACT_PATHS in scripts/artifact_paths.sh:" >&2 + printf '%s' "$missing" | sed 's/^/ /' >&2 + echo " -> add the listed filenames to the artifact_paths array in scripts/artifact_paths.sh" >&2 + echo " -> and verify assert_safe_artifact_paths is invoked before any write to them" >&2 + exit 1 +fi + +echo "PASS: every write target discovered in action.yml and scripts/ is guarded by assert_safe_artifact_paths ($failures missing)" +exit 0