Skip to content

Commit 50ab7f4

Browse files
hyperpolymathclaude
andcommitted
fix(aletheia): SHA-pinning check could never fail; make it real
`check_workflow_pins` decided a line was unpinned with: if line.contains("@v") && !line.contains("@") { `contains("@v")` implies `contains("@")`, so the second clause is always false and `has_unpinned` could never be set. Every repository passed the check unconditionally, including ones with `@v4` or `@master` refs. Verified with a standalone binary before changing anything: uses: actions/checkout@v4 old_detects_unpinned=false This is aletheia's Silver-level "GitHub Actions SHA pinning" check — a gate that has never been able to fail. It is also, pointedly, the exact check that would have caught `SonarSource/sonarqube-scan-action@master`, the unpinned action that broke this repo's Governance and CodeQL in #139. Replaced with `uses_line_is_pinned`, which requires the ref after the final `@` to be 40 hex characters, and: - accepts both `uses:` and list form `- uses:` (the estate's existing workflow linter anchors to line-start and misses the inline form) - ignores a trailing `# v7.0.1` provenance comment, so a correctly pinned line with a stale version comment is not a false positive - exempts `./…` local actions/reusable workflows and `docker://` refs, which cannot carry a Git SHA Also replaced `assert!(true)` in test_file_exists with a real assertion. It had given up on CWD-dependence; anchoring on `env!("CARGO_MANIFEST_DIR")` makes it deterministic, and it now covers the negative case and the is_file()-not- exists() distinction. Verified: 29 unit tests pass (was 26), `cargo fmt --check` clean, clippy 25 -> 23 findings. End-to-end `cargo run -- .` against this repo now reports `[FAIL] GitHub Actions SHA pinning`, correctly identifying the one genuinely unpinned action, where before the fix it reported a pass. Refs #125. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent bb151f9 commit 50ab7f4

1 file changed

Lines changed: 103 additions & 17 deletions

File tree

aletheia/src/checks.rs

Lines changed: 103 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -141,17 +141,8 @@ pub fn check_workflow_pins(report: &mut ComplianceReport, repo_path: &Path) {
141141
{
142142
checked_files += 1;
143143
if let Ok(content) = fs::read_to_string(&path) {
144-
// Check if all 'uses:' lines have SHA pinning (40 hex chars)
145-
let mut has_unpinned = false;
146-
for line in content.lines() {
147-
if line.trim().starts_with("uses:") {
148-
// Simple check: if it contains @v (like @v4), it's unpinned
149-
if line.contains("@v") && !line.contains("@") {
150-
has_unpinned = true;
151-
break;
152-
}
153-
}
154-
}
144+
let has_unpinned =
145+
content.lines().any(|line| !uses_line_is_pinned(line));
155146
if !has_unpinned {
156147
valid_files += 1;
157148
}
@@ -176,6 +167,49 @@ fn file_exists(repo_path: &Path, filename: &str) -> bool {
176167
repo_path.join(filename).is_file()
177168
}
178169

170+
/// SECURITY: Decide whether one workflow line satisfies SHA pinning.
171+
///
172+
/// Returns `true` for any line that is not a `uses:` line, so callers can apply
173+
/// this with `.any(|l| !uses_line_is_pinned(l))` over a whole file.
174+
///
175+
/// A `uses:` value is pinned only when the ref after the final `@` is exactly
176+
/// 40 hexadecimal characters (a full-length Git SHA-1). `@v4`, `@main`,
177+
/// `@master` and a bare action with no `@` at all are all unpinned.
178+
///
179+
/// Exempt (not pinnable, so treated as pinned):
180+
/// - local actions and local reusable workflows — `./…`
181+
/// - `docker://` image references, which use a different digest syntax
182+
///
183+
/// A trailing `# v4` provenance comment is ignored, so
184+
/// `uses: actions/checkout@3d3c42e5… # v7.0.1` is correctly seen as pinned.
185+
fn uses_line_is_pinned(line: &str) -> bool {
186+
let trimmed = line.trim();
187+
// Accept both `uses:` and list form `- uses:`.
188+
let rest = match trimmed
189+
.strip_prefix("uses:")
190+
.or_else(|| trimmed.strip_prefix("- uses:"))
191+
{
192+
Some(r) => r,
193+
None => return true, // not a uses: line — nothing to judge
194+
};
195+
196+
// Strip the trailing provenance comment, then surrounding quotes.
197+
let value = rest.split('#').next().unwrap_or("").trim();
198+
let value = value.trim_matches(|c| c == '"' || c == '\'');
199+
200+
if value.is_empty() {
201+
return true;
202+
}
203+
if value.starts_with("./") || value.starts_with(".\\") || value.starts_with("docker://") {
204+
return true;
205+
}
206+
207+
match value.rsplit_once('@') {
208+
Some((_, git_ref)) => git_ref.len() == 40 && git_ref.chars().all(|c| c.is_ascii_hexdigit()),
209+
None => false, // no ref at all — unpinned
210+
}
211+
}
212+
179213
/// Helper: Recursive implementation of glob_match.
180214
fn glob_match_recursive(pattern: &[char], text: &[char], pi: usize, ti: usize) -> bool {
181215
if pi >= pattern.len() && ti >= text.len() {
@@ -300,12 +334,64 @@ mod tests {
300334

301335
#[test]
302336
fn test_file_exists() {
303-
// file_exists checks if path.join(filename).is_file()
304-
// Since we're testing with temp dir, check something that exists
305-
let current_dir = std::env::current_dir().expect("TODO: handle error");
306-
let exists = file_exists(&current_dir, "Cargo.toml");
307-
// May or may not exist depending on CWD, so just check it doesn't panic
308-
assert!(true); // Test passed if no panic
337+
// Anchor on CARGO_MANIFEST_DIR rather than the process working directory:
338+
// the crate root is fixed at compile time, so this is deterministic no
339+
// matter where the test binary is invoked from.
340+
let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR"));
341+
342+
assert!(
343+
file_exists(root, "Cargo.toml"),
344+
"Cargo.toml exists at the crate root"
345+
);
346+
assert!(!file_exists(root, "no-such-file.does-not-exist"));
347+
// `is_file()`, not `exists()` — a directory must not count as a file.
348+
assert!(!file_exists(root, "src"), "a directory is not a file");
349+
}
350+
351+
#[test]
352+
fn test_uses_line_is_pinned_accepts_full_sha() {
353+
// Real pins taken from this repository's own workflows.
354+
assert!(uses_line_is_pinned(
355+
" - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1"
356+
));
357+
// A trailing provenance comment must not defeat the check.
358+
assert!(uses_line_is_pinned(
359+
" uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0"
360+
));
361+
}
362+
363+
#[test]
364+
fn test_uses_line_is_pinned_rejects_tags_and_branches() {
365+
// REGRESSION: the previous implementation was `line.contains("@v")
366+
// && !line.contains("@")`, which is unsatisfiable — every one of these
367+
// was silently reported as pinned.
368+
assert!(!uses_line_is_pinned(" uses: actions/checkout@v4"));
369+
assert!(!uses_line_is_pinned(
370+
" - uses: actions/checkout@v7.0.1"
371+
));
372+
assert!(!uses_line_is_pinned(" uses: some/action@main"));
373+
// The exact line that broke Governance and CodeQL on this repo.
374+
assert!(!uses_line_is_pinned(
375+
" uses: SonarSource/sonarqube-scan-action@master"
376+
));
377+
// A short/abbreviated SHA is not a full-length pin.
378+
assert!(!uses_line_is_pinned(" uses: foo/bar@3d3c42e"));
379+
// No ref at all.
380+
assert!(!uses_line_is_pinned(" uses: foo/bar"));
381+
}
382+
383+
#[test]
384+
fn test_uses_line_is_pinned_ignores_non_uses_and_exempt_forms() {
385+
assert!(uses_line_is_pinned(" - name: Checkout"));
386+
assert!(uses_line_is_pinned(" run: cargo test"));
387+
assert!(uses_line_is_pinned(""));
388+
// Local actions and local reusable workflows cannot be SHA-pinned.
389+
assert!(uses_line_is_pinned(" - uses: ./.github/actions/setup"));
390+
assert!(uses_line_is_pinned(
391+
" uses: ./.github/workflows/reusable.yml"
392+
));
393+
// Docker refs use a different digest syntax; out of scope.
394+
assert!(uses_line_is_pinned(" uses: docker://alpine:3.20"));
309395
}
310396

311397
#[test]

0 commit comments

Comments
 (0)