feat(threatcrush-scan): omit write scopes and their steps rather than disabling them - #970
Merged
Merged
Conversation
…st disable them
mac-developer-bridge declined the offered workflow on two points that were
the pack's rather than the repository's.
The two write scopes were requested unconditionally even though the workflow
is described as report-only, and `uploadSarif: false` / `commentOnPr: false`
still rendered the steps that use them, guarded by a condition. That makes
"least privilege" a claim about a condition rather than a description of the
file. A shipped-but-disabled Security-tab upload still asks a maintainer to
read an upload and take on trust that the guard is right.
So the template gained a whole-line `{{#if var}} … {{/if}}` block, and the two
optional steps are removed from the render rather than switched off. Each
scope is now emitted by the output that needs it — `pull-requests: write` from
commentOnPr, `security-events: write` from uploadSarif — so the permissions
block cannot drift out of step with what the workflow does. That retires the
hand-assembled `extraPermissions` input, which made least privilege something
a caller had to remember.
`{{#if}}` is the only control construct and it is not nestable. It still takes
a bare variable name, so the renderer's "only {{varName}}, no expressions"
guarantee is unchanged: there is nothing to evaluate, only a value compared
against the literal 'true'. Anything else drops the block, because pack inputs
are strings and treating a stray value as truthy would turn a typo into a
granted write scope. Blocks resolve before substitution, so a variable that
only makes sense inside a kept block need not be supplied when it is dropped.
The second point was size. With both outputs off the render is 291 lines and
one file, against the 382-line workflow plus 235-line converter that was
offered. The report builder is the same behaviour in ~half the lines; the
install step's commentary is trimmed. Nothing was dropped from it: fail-closed
NOT RUN, severity-first ordering, the scoped/backlog fold and the truncation
notice all still render, verified against a real 6-finding SARIF across five
states (scoped, scoped-with-no-touched-findings, unscoped, scan-skipped,
SARIF-missing).
One fix found while testing: the report's "could not read SARIF" warning went
to stdout, which is now the report file itself. It goes to stderr.
Also read-only outputs are the ones that keep working on fork pull requests,
where GitHub downgrades GITHUB_TOKEN — so the richest reporting was least
reliable exactly where an external scan is most useful. That is now in the
README rather than discovered by a maintainer.
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
ThreatCrush Security Scan84 finding(s) HIGH/CRITICAL: 24 | MEDIUM: 51 | LOW: 9
…and 34 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses a maintainer review on mac-developer-bridge#7. Both findings were the pack's, not theirs.
Least privilege was a claim about a condition, not a description of the file.
uploadSarif: false/commentOnPr: falsestill rendered the Security-tab upload and the PR-comment step, guarded by anif:. A disabled upload still asks a maintainer to read an upload and trust the guard. The template gained a whole-line{{#if var}} … {{/if}}block and those steps are now removed from the render. Each scope is emitted by the output that needs it, sopermissions:cannot drift from what the workflow does — retiring the hand-assembledextraPermissionsinput.Size. Read-only render is 291 lines, one file, against the 382-line workflow + 235-line converter that was offered. The report builder is the same behaviour in about half the lines.
{{#if}}is the only control construct, not nestable, and still takes a bare variable name — the renderer's "only{{varName}}, no expressions" guarantee is unchanged. Anything but the literal'true'drops the block, because pack inputs are strings and a stray value read as truthy would be a granted write scope. Blocks resolve before substitution, so variables inside a dropped block need not be supplied.Nothing was dropped from the report: fail-closed NOT RUN, severity-first ordering, the scoped/backlog fold and the truncation notice all still render. Verified against a real 6-finding SARIF across five states (scoped, scoped-with-nothing-touched, unscoped, scan-skipped, SARIF-missing).
One bug found while testing: the "could not read SARIF" warning went to stdout, which is now the report file itself. Moved to stderr.
94 tests pass across
packages/actionsandpackages/actions-fleet-core;actions-fleet-coretypechecks clean.