mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
Abhitej John
80baddd6c4
* Harden vally evaluation against command injection; source tokens from copilot-pat-pool Close a command-injection vector where fork-PR-controlled path segments flowed into a bash run: body via matrix.entry.plugin interpolation in the token-bearing job. - Allowlist-validate matrix entries (plugin/name/skills_path) before they enter the matrix in evaluation.yml, and re-validate defense-in-depth as the first step of the vally-evaluate job. - Pass all PR-controlled values through env: vars and reference them quoted; no raw expression interpolation remains in any run: body. - Build workflow_dispatch entries with jq --arg instead of string interpolation. - Replace secrets: inherit with explicit secret passing to reduce blast radius. - Source Copilot tokens from the copilot-pat-pool environment (COPILOT_PAT_0..9) via the job environment, with COPILOT_GITHUB_TOKEN as an empty-pool fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address Copilot review: reject empty skills_path and quote dirname - Fail the matrix-entry validation when skills_path is empty (never a legitimate state for vally entries) instead of silently skipping the loop. - Quote the inner $(dirname ...) in EVAL_NAME to avoid word-splitting. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address jeffhandley review: drop PAT fallback, explain regex anchors - Remove the COPILOT_GITHUB_TOKEN fallback secret from vally-evaluation.yml (workflow_call.secrets, the token-selection step, and evaluation.yml's caller-side secrets passing). An empty copilot-pat-pool is now a hard configuration failure instead of falling back to a separate secret. - Add short comments explaining why bash's ^/$ anchors are safe here without the \A/\z equivalents used in the PowerShell discover job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden regex validation against locale-dependent bracket ranges Add LC_ALL=C before the allowlist regex checks in both the prepare job and the Validate matrix entry step. POSIX bracket ranges like [A-Za-z] can behave unexpectedly under non-C locale collation; forcing the C locale makes the anchor/character-class behavior deterministic regardless of the runner's default locale. Found via cross-model (GPT-5.5) review of the prior fixup commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address review: constrain vally by skills_path, reject empty skills_path - vally: select eval specs per matrix leg's skills_path so sharded plugins run only their own skills (no N-times re-run / PAT-pool token waste) - evaluation.yml: hard-fail on empty skills_path in the matrix allowlist - vally: quote inner dirname substitution in the Write summary step Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reject whitespace-only skills_path in matrix validation A value like a single space passed the bash `-z` empty check and then word-split to zero loop iterations, letting an effectively-empty skills_path slip through. Count word-split segments instead so empty and whitespace-only both hard-fail. Mirror the intent on the PowerShell discover side with a trim-based emptiness filter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Bind skills_path segments to the matrix entry's plugin The skills_path allowlist previously validated only the generic shape (plugins/<any>/skills[/<skill>]); it did not require each segment to belong to the entry's own plugin. A mismatched entry (plugin=foo + skills_path=plugins/bar/skills) would pass, and downstream steps assume the plugins/$PLUGIN/skills prefix. Add a literal (non-regex) prefix binding in both validators: - vally-evaluation.yml: [[ seg != prefix && seg != prefix/* ]] with skills_prefix="plugins/$ENTRY_PLUGIN/skills". - evaluation.yml: case-sensitive -cne plus Ordinal StartsWith against "plugins/$($e.plugin)/skills". Literal comparison avoids regex-metachar pitfalls from '.'/'-' in plugin names; -cne/Ordinal keeps it case-sensitive to match Linux path semantics. Legitimate entries are always same-plugin, so this rejects no valid input. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>