Skip to content

feat: golden-file tests, ten examples, case studies, PR comment mode - #189

Merged
advaitpatel merged 4 commits into
mainfrom
feat/goldens-examples-pr-comments
Sep 20, 2026
Merged

advaitpatel merged 4 commits into
mainfrom
feat/goldens-examples-pr-comments

Conversation

@advaitpatel

Copy link
Copy Markdown
Collaborator

The three strategy items that were still outstanding: 1.5, 3.6 and 3.7. With these, Stages 0-3 are genuinely complete.

1.5 Golden-file tests

The terminal summary, the --json payload and the SARIF report are now compared against stored files. Each renders from a fixed findings set — no scanner, no network, no clock — so a diff means the output changed, not that the world did.

Verified they have teeth by reintroducing the fix-command ordering regression from #187: two tests fail, and pass again when reverted.

Re-record deliberately with DOCKSEC_UPDATE_GOLDEN=1 pytest tests/test_golden_output.py and review the diff.

This closes a real gap. The 1.2-1.4 output surfaces changed in #187 without golden coverage, which is exactly the drift this item exists to catch.

3.6 Ten examples + three case studies

Eight Dockerfiles (Node, Python, Java, Go, BuildKit secrets) plus the two compose stacks. Every count in examples/README.md was produced by running the scan.

Two are instructive beyond the insecure/hardened delta:

  • The distroless Go image reports a HEALTHCHECK finding that is correct to keep — there is no shell to run one.
  • The BuildKit example passes a build secret without tripping the secret rule that the ENV-based examples do.

Case studies use official images, so the numbers are reproducible and no third party is named unfavourably:

Study Image Findings Point
1 node:18 2,200 9 worth acting on; one at the 100th EPSS percentile
2 python:3.12-slim 44 none fixable — the honest "change your base or accept it" case
3 nginx:1.31.6-alpine 0 clean, and what that does not prove

3.7 PR comment mode

Two-stage, per the threat model in the strategy doc:

  • pr-scan.yml runs in the untrusted PR context with contents: read, no secrets, no commenting. Emits an artifact.
  • pr-comment.yml runs on workflow_run in the base repo. Reads only that artifact, never checks out the PR's code, and posts.

Because a fork controls the text of its own findings, everything the renderer prints is escaped — pipes, HTML, Markdown links, newline-injected rows, backticks — and the PR number is validated as an integer before reaching an API path. All covered by tests.

Found and fixed while testing: a PR touching many services rendered a 156KB comment, which GitHub's 65536-character limit would reject — the job would fail with nothing posted. The body is now budgeted to 60000 characters, dropping whole blocks (never splitting a table) and stating how many. A realistic 8-file comment renders byte-identical.

Verification

  • 534 tests pass (was 510), ruff check . clean
  • Collection script run against a real diff: detected and scanned all 8 changed Dockerfiles
  • Comment rendered from that real artifact end to end
  • All new doc links resolve

Advait Patel added 2 commits September 20, 2026 14:01
The three strategy items that were still outstanding (1.5, 3.6, 3.7).

Golden-file tests cover the terminal summary, the --json payload and the
SARIF report. Each renders from a fixed findings set - no scanner, no
network, no clock - and compares against a stored file, so an unintended
change fails CI instead of reaching a user. Verified by reintroducing the
fix-command ordering regression from #187: two tests fail, and pass again
when it is reverted. Re-record with DOCKSEC_UPDATE_GOLDEN=1 and read the
diff.

Ten examples with documented expected findings, every count produced by
running the scan. Eight Dockerfiles across Node, Python, Java, Go and
BuildKit secret mounts, plus the existing two compose stacks, each
insecure file paired with a hardened one. Two carry a lesson beyond the
delta: the distroless Go image reports a HEALTHCHECK finding that is
correct to keep, and the BuildKit example passes a build secret without
tripping the secret rule that the ENV-based examples do.

Three case studies against official images - node:18 (2,200 findings, 9
worth acting on today, one at the 100th EPSS percentile), python:3.12-slim
(44 findings, none fixable) and nginx:1.31.6-alpine (clean, and what that
does not prove). Official images keep the numbers reproducible and name
no third party unfavourably.

PR comment mode ships as two workflows. Stage one runs in the untrusted
pull-request context with contents: read and no secrets, and emits an
artifact. Stage two runs on workflow_run in the base repository, reads
only that artifact, never checks out the PR's code, and posts the
comment. Everything the renderer prints is escaped, because a fork
controls the text of its own findings; the PR number is validated as an
integer before it reaches an API path. Injection cases are covered by
tests: pipes, HTML, Markdown links, newline-injected rows and backticks.
A pull request touching many compose services rendered a 156KB comment.
GitHub rejects an issue comment over 65536 characters, so the API call
would fail and the job would end with nothing posted - the per-file row
cap bounds each table but not the total.

Whole per-file blocks are now kept while they fit within a 60000
character budget, then the count of dropped files is stated. Truncating
mid-table would leave unbalanced <details> tags, so blocks are never
split. A realistic result is unaffected: the 8-file comment from this
branch renders byte-identical.
@github-actions github-actions Bot added documentation Improvements or additions to documentation ci Changes to CI/CD workflows tests Changes to the test suite labels Sep 20, 2026
@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails
actions/actions/checkout df4cb1c069e1874edd31b4311f1884172cec0e10 🟢 7
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Binary-Artifacts🟢 10no binaries found in the repo
Maintained🟢 1017 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Pinned-Dependencies🟢 3dependency not pinned by hash detected -- score normalized to 3
License🟢 10license file detected
Fuzzing⚠️ 0project is not fuzzed
Packaging⚠️ -1packaging workflow not detected
Signed-Releases⚠️ -1no releases found
Security-Policy🟢 9security policy file detected
Branch-Protection🟢 6branch protection is not maximal on development and all release branches
SAST🟢 10SAST tool is run on all commits
actions/actions/setup-python e9d6f990972a57673cdb72ec29e19d42ba28880f 🟢 6.6
Details
CheckScoreReason
Maintained🟢 1023 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
Binary-Artifacts🟢 10no binaries found in the repo
Code-Review🟢 10all changesets reviewed
Packaging⚠️ -1packaging workflow not detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Pinned-Dependencies🟢 7dependency not pinned by hash detected -- score normalized to 7
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing⚠️ 0project is not fuzzed
License🟢 10license file detected
Signed-Releases⚠️ -1no releases found
Security-Policy🟢 9security policy file detected
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
SAST🟢 9SAST tool is not run on all commits -- score normalized to 9
actions/actions/upload-artifact ea165f8d65b6e75b540449e92b4886f43607fa02 🟢 5.2
Details
CheckScoreReason
Binary-Artifacts🟢 10no binaries found in the repo
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Maintained⚠️ 00 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 0
Code-Review🟢 10all changesets reviewed
Packaging⚠️ -1packaging workflow not detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
Pinned-Dependencies⚠️ 1dependency not pinned by hash detected -- score normalized to 1
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing⚠️ 0project is not fuzzed
License🟢 10license file detected
Signed-Releases⚠️ -1no releases found
Security-Policy🟢 9security policy file detected
SAST🟢 10SAST tool is run on all commits
Branch-Protection⚠️ 0branch protection not enabled on development/release branches

Scanned Files

  • .github/workflows/pr-scan.yml

Comment thread tests/test_pr_comment.py Fixed
Comment thread tests/test_pr_comment.py Fixed
Advait Patel added 2 commits September 20, 2026 14:05
CodeQL flagged py/unused-global-variable: the test module imported _md
but only exercised it through rendered output. It is the single choke
point every untrusted value passes through, so test it directly rather
than drop the import.
The README told new users to pin 2026.8.19 - two releases behind, and
before the fixes that make the container usable from the command line.

The tag does not exist until the release is cut, which is the same
ordering the previous bumps used.
@advaitpatel
advaitpatel merged commit 7c2e8d4 into main Sep 20, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Changes to CI/CD workflows documentation Improvements or additions to documentation tests Changes to the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants