fix(tcfeed): stop new requests re-adding the converter, and describe what is actually offered - #155
Merged
Merged
Conversation
…what is actually offered
Three things a maintainer review on mac-developer-bridge found, all of them
tcfeed's rather than the repository's.
`openPr` still wrote `.github/scripts/threatcrush-to-sarif.py`. `refresh`
stopped installing it in pack 1.7.0 and actively deletes it from branches that
still carry it, but the path that opens a *new* request never got the change —
so every fresh offer re-added the file refresh existed to take away, and the
body said "adds one workflow" above a diff that added two.
The body also promised the Security tab and a pull request comment
unconditionally, including under TCFEED_LEAST_PRIVILEGE=1, where neither is
rendered. It now reads the same switch the workflow does, so the read-only
offer describes itself as read-only and says the two write-scoped steps are
absent from the file rather than disabled in it. The default offer now names
the read-only build as something a maintainer can just ask for.
`render` grew whole-line `{{#if name}} … {{/if}}`, matching the pack renderer
in sh1pt, so those steps are omitted rather than shipped behind a false
condition. That retires `extraPermissions` — a block of literal YAML pasted in
from here, which made least privilege depend on tcfeed remembering to pass an
empty string rather than on the template. Each scope is now emitted by the
output that needs it. The marker still takes a bare variable name and anything
but the literal 'true' drops the block, because a stray value read as truthy
would be a granted write scope.
Verified against the updated pack: both renderers produce byte-identical
output, no marker survives either render, and the read-only render is valid
YAML whose permissions are exactly `contents: read` with the Security-tab and
comment steps gone from the step list.
Also adds the reason the default is still write-enabled, and what would change
it: two maintainers have now declined on this point, and the second observed
that `pull_request` hands a fork's GITHUB_TOKEN read-only access — so the
comment and the upload are least reliable on exactly the contributor pull
requests the scan is most useful for.
Pre-commit skipped: it builds the CLI and the web app, and this worktree has no
node_modules. bin/tcfeed.ts is a standalone script and was verified directly —
it parses, `--help` runs, and its render was executed against the updated pack.
ThreatCrush Security Scan63 finding(s) HIGH/CRITICAL: 4 | MEDIUM: 52 | LOW: 7
…and 13 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.
Three things a maintainer review on mac-developer-bridge#7 found, all of them tcfeed's rather than the repository's.
openPrstill shipped the SARIF converter.refreshstopped installing it in pack 1.7.0 and actively deletes it from branches that carry it, but the path that opens a new request never got the change. Every fresh offer re-added the file refresh existed to take away, and the body said "adds one workflow" above a diff that added two.The body promised outputs that were not rendered. It named the Security tab and a PR comment unconditionally, including under
TCFEED_LEAST_PRIVILEGE=1where neither exists. It now reads the same switch the workflow does, and the default offer names the read-only build as something a maintainer can just ask for.rendergrew{{#if name}} … {{/if}}, matching the pack renderer in sh1pt#970, so the optional steps are omitted rather than shipped behind a false condition. RetiresextraPermissions, which made least privilege depend on tcfeed remembering to pass an empty string rather than on the template.Verified against the updated pack: both renderers produce byte-identical output, no marker survives either render, and the read-only render is valid YAML whose permissions are exactly
contents: readwith the two write-scoped steps gone from the step list. Applied live to mac-developer-bridge#7 (one file, 286 lines,contents: read) and to a fresh offer on mulgadc/spinifex#809.Also records why the default is still write-enabled and what would change it: two maintainers have now declined on this point, and the second observed that
pull_requesthands a fork'sGITHUB_TOKENread-only access — so the comment and the upload are least reliable on exactly the contributor pull requests the scan is most useful for.Pre-commit was skipped: it builds the CLI and the web app and this worktree has no
node_modules.bin/tcfeed.tsis a standalone script, verified directly — it parses,--helpruns, and itsrenderwas executed against the updated pack.