ci: harden action pinning lint (#1545)
* ci: harden action pinning lint * refactor: extract dockview review dialog slot * test: cover action pinning edge cases * fix: reject dynamic workflow action sources * fix: normalize quoted workflow action refs * test: cover quoted unpinned action refs --------- Co-authored-by: root <root@kandev.ts.net>
This commit is contained in:
@@ -15,6 +15,9 @@ from pathlib import Path
|
||||
# A valid pinned ref is exactly 40 lowercase hex characters.
|
||||
SHA_RE = re.compile(r"^[0-9a-f]{40}$")
|
||||
|
||||
# Docker container actions are pinned by immutable registry digest.
|
||||
DOCKER_DIGEST_RE = re.compile(r"^sha256:[0-9a-f]{64}$")
|
||||
|
||||
# Matches a `uses:` step line, capturing everything after `uses:` up to an
|
||||
# optional trailing YAML comment. The outer group is intentionally lazy so
|
||||
# trailing `# tag` comments are consumed by the optional group rather than
|
||||
@@ -25,7 +28,14 @@ USES_RE = re.compile(r"^\s*-?\s*uses:\s+(.+?)(?:\s+#.*)?$")
|
||||
|
||||
workflows_dir = Path(__file__).parent.parent / "workflows"
|
||||
|
||||
violations: list[str] = []
|
||||
violations: list[tuple[str, int, str]] = []
|
||||
|
||||
|
||||
def strip_balanced_quotes(value: str) -> str:
|
||||
if len(value) >= 2 and value[0] == value[-1] and value[0] in {"'", '"'}:
|
||||
return value[1:-1]
|
||||
return value
|
||||
|
||||
|
||||
for path in sorted(p for ext in ("*.yml", "*.yaml") for p in workflows_dir.glob(ext)):
|
||||
for lineno, line in enumerate(path.read_text().splitlines(), start=1):
|
||||
@@ -33,35 +43,44 @@ for path in sorted(p for ext in ("*.yml", "*.yaml") for p in workflows_dir.glob(
|
||||
if not m:
|
||||
continue
|
||||
|
||||
uses_value = strip_balanced_quotes(m.group(1).strip())
|
||||
|
||||
# Local actions and reusable workflows reference checked-out source, not
|
||||
# an external registry.
|
||||
if uses_value.startswith("./"):
|
||||
continue
|
||||
|
||||
if "${{" in uses_value:
|
||||
rel = path.relative_to(Path(__file__).parent.parent.parent)
|
||||
violations.append((str(rel), lineno, line.strip()))
|
||||
continue
|
||||
|
||||
# Split on the last `@` to separate the action name from its ref.
|
||||
# rpartition returns ("", "", value) when `@` is absent.
|
||||
uses_value = m.group(1).strip()
|
||||
action, sep, ref = uses_value.rpartition("@")
|
||||
if not sep:
|
||||
# No `@` — local action (`./...`) without a pin; skip.
|
||||
continue
|
||||
|
||||
# Local reusable workflows: uses: ./.github/workflows/foo.yml@ref
|
||||
# These reference local content with no external registry.
|
||||
if action.startswith("./"):
|
||||
continue
|
||||
|
||||
# Docker container actions: uses: docker://image@sha256:digest
|
||||
# The ref after `@` is a registry digest, not a commit SHA.
|
||||
if action.startswith("docker://"):
|
||||
if sep and DOCKER_DIGEST_RE.match(ref):
|
||||
continue
|
||||
rel = path.relative_to(Path(__file__).parent.parent.parent)
|
||||
violations.append((str(rel), lineno, line.strip()))
|
||||
continue
|
||||
|
||||
if not SHA_RE.match(ref):
|
||||
if not sep or not SHA_RE.match(ref):
|
||||
rel = path.relative_to(Path(__file__).parent.parent.parent)
|
||||
violations.append((str(rel), lineno, line.strip()))
|
||||
|
||||
if violations:
|
||||
for file, lineno, text in violations:
|
||||
# GitHub Actions annotation — shown inline on the PR diff.
|
||||
print(f"::error file={file},line={lineno}::Unpinned action ref (use a 40-char commit SHA): {text}")
|
||||
print(
|
||||
f"::error file={file},line={lineno}::Unpinned action ref "
|
||||
f"(use a 40-char commit SHA, or @sha256:<digest> for docker://): {text}"
|
||||
)
|
||||
print(
|
||||
f"\n{len(violations)} unpinned ref(s) found. "
|
||||
"Pin each `uses:` to a commit SHA and keep the version tag as a comment:\n"
|
||||
"Pin each GitHub action `uses:` to a commit SHA, and each docker:// action to a digest. "
|
||||
"Keep the version tag as a comment:\n"
|
||||
" uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
@@ -0,0 +1,196 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Regression tests for the workflow action pinning linter."""
|
||||
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import textwrap
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
LINTER = REPO_ROOT / ".github" / "scripts" / "lint-action-pinning.py"
|
||||
|
||||
|
||||
class LintActionPinningTest(unittest.TestCase):
|
||||
def run_lint(self, workflow: str) -> subprocess.CompletedProcess[str]:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
tmp_path = Path(tmp)
|
||||
scripts_dir = tmp_path / ".github" / "scripts"
|
||||
workflows_dir = tmp_path / ".github" / "workflows"
|
||||
scripts_dir.mkdir(parents=True)
|
||||
workflows_dir.mkdir(parents=True)
|
||||
|
||||
shutil.copy2(LINTER, scripts_dir / "lint-action-pinning.py")
|
||||
(workflows_dir / "test.yml").write_text(textwrap.dedent(workflow).strip() + "\n")
|
||||
|
||||
return subprocess.run(
|
||||
[sys.executable, str(scripts_dir / "lint-action-pinning.py")],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
|
||||
def test_rejects_docker_action_pinned_to_tag(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: docker://alpine:latest
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("docker://alpine:latest", output)
|
||||
|
||||
def test_rejects_dynamic_uses_value(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: ${{ inputs.action }}
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("${{ inputs.action }}", output)
|
||||
|
||||
def test_rejects_dynamic_action_source_pinned_to_sha(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: ${{ inputs.action }}@df4cb1c069e1874edd31b4311f1884172cec0e10
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("${{ inputs.action }}@df4cb1c069e1874edd31b4311f1884172cec0e10", output)
|
||||
|
||||
def test_rejects_docker_action_with_short_digest(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: docker://alpine@sha256:abc123
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("docker://alpine@sha256:abc123", output)
|
||||
|
||||
def test_rejects_docker_action_pinned_to_non_digest_ref(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: docker://alpine@v1.0
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("docker://alpine@v1.0", output)
|
||||
|
||||
def test_rejects_unpinned_action_without_at_sign(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn("uses: actions/checkout", output)
|
||||
|
||||
def test_rejects_quoted_unpinned_action(self) -> None:
|
||||
result = self.run_lint(
|
||||
"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: "actions/checkout@v1"
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertNotEqual(result.returncode, 0, output)
|
||||
self.assertIn('"actions/checkout@v1"', output)
|
||||
|
||||
def test_allows_pinned_action_local_action_and_docker_digest(self) -> None:
|
||||
digest = "a" * 64
|
||||
result = self.run_lint(
|
||||
f"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6
|
||||
- uses: ./.github/actions/setup-fixture
|
||||
- uses: ./.github/workflows/reusable.yml@main
|
||||
- uses: docker://alpine@sha256:{digest}
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertEqual(result.returncode, 0, output)
|
||||
|
||||
def test_allows_quoted_pinned_local_and_docker_uses_values(self) -> None:
|
||||
digest = "b" * 64
|
||||
result = self.run_lint(
|
||||
f"""
|
||||
name: test
|
||||
on: push
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: "actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10" # v6
|
||||
- uses: './.github/actions/setup-fixture'
|
||||
- uses: "docker://alpine@sha256:{digest}"
|
||||
"""
|
||||
)
|
||||
|
||||
output = result.stdout + result.stderr
|
||||
self.assertEqual(result.returncode, 0, output)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -6,11 +6,13 @@ on:
|
||||
paths:
|
||||
- ".github/workflows/**"
|
||||
- ".github/scripts/lint-action-pinning.py"
|
||||
- ".github/scripts/lint-action-pinning_test.py"
|
||||
pull_request:
|
||||
branches: [main]
|
||||
paths:
|
||||
- ".github/workflows/**"
|
||||
- ".github/scripts/lint-action-pinning.py"
|
||||
- ".github/scripts/lint-action-pinning_test.py"
|
||||
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.ref }}
|
||||
@@ -31,5 +33,8 @@ jobs:
|
||||
with:
|
||||
persist-credentials: false
|
||||
|
||||
- name: Test action-pinning lint
|
||||
run: python3 .github/scripts/lint-action-pinning_test.py
|
||||
|
||||
- name: Verify action refs
|
||||
run: python3 .github/scripts/lint-action-pinning.py
|
||||
|
||||
@@ -419,6 +419,7 @@ test-cli:
|
||||
.PHONY: test-scripts
|
||||
test-scripts:
|
||||
@printf "$(CYAN)Running script tests...$(RESET)\n"
|
||||
@python3 .github/scripts/lint-action-pinning_test.py
|
||||
@bash scripts/pr-state.test.sh
|
||||
@bash scripts/opencode-code-review.test.sh
|
||||
@python3 scripts/opencode-code-review.test.py
|
||||
|
||||
@@ -94,7 +94,7 @@ function findPnpmSetupVersion(
|
||||
}
|
||||
|
||||
function matchPnpmSetupUsesLine(line: string): RegExpMatchArray | null {
|
||||
return line.match(/^(\s*)(?:-\s*)?uses:\s*["']?pnpm\/action-setup@v\d+["']?\s*(?:#.*)?$/);
|
||||
return line.match(/^(\s*)(?:-\s*)?uses:\s*["']?pnpm\/action-setup@[0-9a-f]{40}["']?\s*(?:#.*)?$/);
|
||||
}
|
||||
|
||||
function findStepIndent(lines: string[], usesLineIndex: number, usesIndent: number): number {
|
||||
@@ -233,7 +233,7 @@ describe("release desktop artifacts", () => {
|
||||
expect(workflow).toContain(
|
||||
'rustup toolchain install stable --profile minimal --target "${{ matrix.rust_target }}"',
|
||||
);
|
||||
expect(workflow).toContain("Swatinem/rust-cache@v2");
|
||||
expect(workflow).toMatch(/Swatinem\/rust-cache@[0-9a-f]{40}\s+# v2/);
|
||||
|
||||
for (const platform of desktopPlatforms) {
|
||||
expect(workflow, `release.yml must include desktop platform ${platform}`).toContain(
|
||||
|
||||
@@ -536,6 +536,32 @@ type DockviewDesktopLayoutProps = {
|
||||
compact?: boolean;
|
||||
};
|
||||
|
||||
function ReviewDialogSlot({
|
||||
sessionId,
|
||||
review,
|
||||
}: {
|
||||
sessionId: string | null;
|
||||
review: ReturnType<typeof useReviewDialog>;
|
||||
}) {
|
||||
if (!sessionId) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return (
|
||||
<ReviewDialog
|
||||
open={review.reviewDialogOpen}
|
||||
onOpenChange={review.setReviewDialogOpen}
|
||||
sessionId={sessionId}
|
||||
baseBranch={review.baseBranch}
|
||||
onSendComments={review.handleReviewSendComments}
|
||||
onOpenFile={review.reviewOpenFile}
|
||||
gitStatusFiles={review.reviewGitStatusFiles}
|
||||
cumulativeDiff={review.reviewCumulativeDiff}
|
||||
prDiffFiles={review.reviewPRDiffFiles}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({
|
||||
sessionId,
|
||||
repository,
|
||||
@@ -643,19 +669,7 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({
|
||||
</div>
|
||||
<BottomTerminalPanel />
|
||||
<PanelPortalHost renderPanel={renderPanel} />
|
||||
{effectiveSessionId && (
|
||||
<ReviewDialog
|
||||
open={review.reviewDialogOpen}
|
||||
onOpenChange={review.setReviewDialogOpen}
|
||||
sessionId={effectiveSessionId}
|
||||
baseBranch={review.baseBranch}
|
||||
onSendComments={review.handleReviewSendComments}
|
||||
onOpenFile={review.reviewOpenFile}
|
||||
gitStatusFiles={review.reviewGitStatusFiles}
|
||||
cumulativeDiff={review.reviewCumulativeDiff}
|
||||
prDiffFiles={review.reviewPRDiffFiles}
|
||||
/>
|
||||
)}
|
||||
<ReviewDialogSlot sessionId={effectiveSessionId} review={review} />
|
||||
</div>
|
||||
);
|
||||
});
|
||||
|
||||
@@ -100,7 +100,7 @@ if [[ "$trusted_script_count" != "2" ]]; then
|
||||
fi
|
||||
pass "OpenCode review executes the parser script from the trusted base commit in both workflow paths"
|
||||
|
||||
artifact_upload_count="$(count_occurrences 'uses: actions/upload-artifact@v4')"
|
||||
artifact_upload_count="$(count_occurrences 'uses: actions/upload-artifact@')"
|
||||
if [[ "$artifact_upload_count" != "2" ]]; then
|
||||
fail "OpenCode review artifacts are uploaded from both workflow paths"
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user