From a0403960fafcf6f22a6ace913eb983024a53c9e1 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 17 Aug 2026 05:12:32 +0000 Subject: [PATCH] fix(tcfeed): stop new requests re-adding the converter, and describe what is actually offered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- bin/tcfeed.ts | 101 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 78 insertions(+), 23 deletions(-) diff --git a/bin/tcfeed.ts b/bin/tcfeed.ts index 88c66de..f96eb19 100755 --- a/bin/tcfeed.ts +++ b/bin/tcfeed.ts @@ -760,26 +760,54 @@ const packInputs = (spec: string, integrity: string): Record => * not have to ask. But it is now one variable away for the repositories that * will not grant a third party write access on a first install, and that is a * real answer to a real objection rather than a paragraph about trust. + * + * Two maintainers have now declined on this specific point — SAG, and + * mac-developer-bridge, who also observed that `pull_request` gives a fork's + * GITHUB_TOKEN read-only access, so the comment and the Security-tab upload + * are *least* reliable on exactly the contributor pull requests an external + * scan is most useful for. If a third declines the same way, the default is + * wrong and should be flipped rather than defended again. */ function outputs(): Record { if (process.env.TCFEED_LEAST_PRIVILEGE === '1') { - return { uploadSarif: 'false', commentOnPr: 'false', extraPermissions: '' }; + return { uploadSarif: 'false', commentOnPr: 'false' }; } - return { - uploadSarif: 'true', - commentOnPr: 'true', - extraPermissions: ' pull-requests: write\n security-events: write', - }; + return { uploadSarif: 'true', commentOnPr: 'true' }; } /** - * Substitution, narrow on purpose. `{{name}}` and nothing else: GitHub's own - * `${{ steps.iface.outputs.native }}` contains braces too, and a looser - * expression rewrites the workflow's expressions into empty strings. That - * damage is invisible here and shows up as a broken job in a stranger's repo. + * Whole-line `{{#if name}} … {{/if}}`, resolved before substitution. + * + * The pack's two optional steps are *removed* from the render rather than left + * in it behind a false condition, and the permission each one needs is emitted + * with it. That retires `extraPermissions`, which was a block of literal YAML + * this function pasted in — so least privilege depended on tcfeed remembering + * to pass the empty string, rather than on the template. + * + * mac-developer-bridge is the reason it is worth the code: shipping a disabled + * Security-tab upload still asks a maintainer to read an upload and trust the + * guard around it, and they counted it as surface either way. + * + * Not nestable, and still a bare variable name inside the marker — there is no + * expression to evaluate, only a value compared against the literal 'true'. */ function render(template: string, values: Record): string { - return template.replace(/\{\{([A-Za-z][A-Za-z0-9]*)\}\}/g, (whole, key: string) => { + const blocks = template.replace( + /^[ \t]*\{\{#if ([A-Za-z][A-Za-z0-9]*)\}\}[ \t]*\n([\s\S]*?)^[ \t]*\{\{\/if\}\}[ \t]*\n/gm, + (_whole, key: string, body: string) => { + const value = values[key]; + if (value === undefined) throw new Error(`the pack has an input tcfeed cannot fill: {{#if ${key}}}`); + // Anything but the literal 'true' drops it. Treating a stray value as + // truthy would turn a typo into a granted write scope. + return value === 'true' ? body : ''; + } + ); + + // Substitution, narrow on purpose. `{{name}}` and nothing else: GitHub's own + // `${{ steps.changed.outputs.scoped }}` contains braces too, and a looser + // expression rewrites the workflow's expressions into empty strings. That + // damage is invisible here and shows up as a broken job in a stranger's repo. + return blocks.replace(/\{\{([A-Za-z][A-Za-z0-9]*)\}\}/g, (whole, key: string) => { const value = values[key]; // A pack that gained an input this does not know about is a pack this // cannot install correctly. Stop; do not ship `{{newInput}}` as a literal. @@ -1023,19 +1051,44 @@ const PR_TITLE = 'ci: scan pull requests for credentials and injection with Thre * workflow, discloses who wrote the workflow, and says it will not be sent * twice. Everything past that is the maintainer's call. */ -const prBody = (spec: string, issue: string): string => - [ +const prBody = (spec: string, issue: string): string => { + // Described from the same switch that renders the workflow, not from a + // sentence written once and left behind. The body used to promise the + // Security tab and a comment unconditionally, which was wrong in exactly the + // configuration a cautious maintainer is most likely to be offered. + const leastPrivilege = outputs().uploadSarif !== 'true'; + + return [ 'Adds one workflow. On each pull request it scans the checked-out repository for', - 'hardcoded credentials, injection, SSRF and unsafe deserialisation, and writes', - 'findings to the Security tab and a comment.', + 'hardcoded credentials, injection, SSRF and unsafe deserialisation.', '', '- `.github/workflows/threatcrush-scan.yml`', - '- `.github/scripts/threatcrush-to-sarif.py` — SARIF shim for older CLI versions', + '', + ...(leastPrivilege + ? [ + '**Read-only.** `permissions:` is `contents: read` and nothing else. Findings go', + 'to the job summary and a SARIF artifact — no pull request comment, no Security', + 'tab upload, no write scope requested. Those two steps are not disabled in the', + 'file, they are absent from it. This is also the shape that keeps working on fork', + 'pull requests, where GitHub downgrades `GITHUB_TOKEN` to read-only.', + ] + : [ + 'Findings go to the Security tab and a pull request comment. If you would rather', + 'not grant those write scopes, say so and I will send the `contents: read` build:', + 'same scan, findings in the job summary and a SARIF artifact, and the two steps', + 'that need a write scope removed from the file rather than switched off.', + ]), '', '**Report-only.** `failOn` is empty, so findings never fail the build. An install', 'or scan failure does fail the job: a scanner that reports clean when it did not', 'run is worse than no scanner.', '', + '**Pre-existing findings.** The report leads with findings in the files the pull', + 'request changes and folds the rest of the repository behind a `
` summary,', + 'so an existing backlog is visible without being posted at the author of an', + 'unrelated change. Anything intentional can be excluded with a `.threatcrushignore`', + 'or a `// threatcrush-disable-next-line ` comment.', + '', // Said plainly because it was said wrongly for sixty-nine requests. scanPath // is ".", and "scans the diff" was in every one of them. '**Scope:** it scans the whole checked-out repository, not only the diff.', @@ -1050,6 +1103,7 @@ const prBody = (spec: string, issue: string): string => 'MIT and free. Written with AI assistance. Closing this is a fine answer and I', 'will not send another.', ].join('\n'); +}; /** * How many requests may be opened right now, and why not more. @@ -1207,15 +1261,16 @@ async function openPr( const pack = packDir(); const spec = await resolveSpec(); const inputs = packInputs(spec, await resolveIntegrity(spec)); + // One file. `refresh` already installed only the workflow and actively + // removed the converter from branches that still carried it, but this path — + // the one that opens a *new* request — went on writing both, so every fresh + // offer re-added the file refresh existed to take away. A repository was + // told the diff adds one file and then shown two. const files = [ { destination: '.github/workflows/threatcrush-scan.yml', content: render(fs.readFileSync(path.join(pack, 'workflow.yml'), 'utf8'), inputs), }, - { - destination: '.github/scripts/threatcrush-to-sarif.py', - content: fs.readFileSync(path.join(pack, 'threatcrush-to-sarif.py'), 'utf8'), - }, ]; const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'tcfeed-pr-')); @@ -1703,9 +1758,9 @@ async function followUp(repo: string, me: string, pr: string): Promise { console.log(`· ${repo} — ${row.name} failed`); if (row.link) console.log(` ${row.link}`); } - console.log(' the branch adds only .github/workflows/threatcrush-scan.yml and'); - console.log(' .github/scripts/threatcrush-to-sarif.py, so a scanner failing here may'); - console.log(' well be judging those — read it before assuming it is theirs.'); + console.log(' the branch adds only .github/workflows/threatcrush-scan.yml, so a'); + console.log(' scanner failing here may well be judging that file — read it before'); + console.log(' assuming the failure is theirs.'); // The remedy table, unchanged and still deliberately short. It patches only // what it recognises and prints the rest for a person, which is the line