feat(threatcrush-scan): omit write scopes and their steps rather than disabling them

packages/actions-fleet-core/src/action-pack/render.test.ts

@@ -67,6 +67,50 @@ describe('applyTemplate', () => {
   it('rejects unknown variables', () => {
     expect(() => applyTemplate('{{missing}}', {})).toThrow(TemplateRenderError);
   });
+
+  it('keeps a {{#if}} block when the variable is exactly "true"', () => {
+    const out = applyTemplate('a\n{{#if on}}\nb\n{{/if}}\nc\n', { on: 'true' });
+    expect(out).toBe('a\nb\nc\n');
+  });
+
+  it('drops a {{#if}} block, and its markers, otherwise', () => {
+    for (const value of ['false', '', 'TRUE', 'yes', '1']) {
+      const out = applyTemplate('a\n{{#if on}}\nb\n{{/if}}\nc\n', { on: value });
+      // Anything but the literal 'true' drops it: pack inputs are strings with
+      // a 'true'/'false' enum, and treating a stray value as truthy would turn
+      // a typo into a granted write scope.
+      expect(out).toBe('a\nc\n');
+    }
+  });
+
+  it('leaves variables inside a dropped block unresolved rather than erroring', () => {
+    // The block is removed before substitution, so a variable that only makes
+    // sense when the block is on does not have to be supplied when it is off.
+    const out = applyTemplate('a\n{{#if on}}\n{{onlyWhenOn}}\n{{/if}}\nc\n', { on: 'false' });
+    expect(out).toBe('a\nc\n');
+  });
+
+  it('still substitutes variables inside a kept block', () => {
+    const out = applyTemplate('{{#if on}}\nnode: {{nodeVersion}}\n{{/if}}\n', {
+      on: 'true',
+      nodeVersion: '22',
+    });
+    expect(out).toBe('node: 22\n');
+  });
+
+  it('rejects a {{#if}} on an unknown variable', () => {
+    expect(() => applyTemplate('{{#if nope}}\nx\n{{/if}}\n', {})).toThrow(TemplateRenderError);
+  });
+
+  it('rejects an unterminated {{#if}}', () => {
+    // Falls through to the scalar pass, where "#if on" is not a variable name.
+    expect(() => applyTemplate('{{#if on}}\nx\n', { on: 'true' })).toThrow(TemplateRenderError);
+  });
+
+  it('does not treat a GitHub expression inside a block as a template tag', () => {
+    const out = applyTemplate('{{#if on}}\nrun: ${{ github.sha }}\n{{/if}}\n', { on: 'true' });
+    expect(out).toBe('run: ${{ github.sha }}\n');
+  });
 });
 
 describe('resolveInputs', () => {

packages/actions-fleet-core/src/action-pack/render.ts

@@ -42,8 +42,42 @@ export class MissingInputError extends Error {
 const TAG_RE = /(?<!\$)\{\{([^{}]*)\}\}/g;
 const SAFE_VAR_RE = /^[A-Za-z_][A-Za-z0-9_]*$/;
 
+// A whole-line {{#if varName}} … {{/if}} pair, markers included.
+//
+// Deliberately the only control construct, and deliberately not nestable: the
+// point is to *omit* an optional step rather than leave it in the file behind a
+// false `if:` condition. A workflow that ships a disabled Security-tab upload
+// still asks a reviewer to read and trust that upload, which is the objection
+// this exists to answer — dead surface in a security-sensitive file is surface
+// all the same.
+//
+// Still a bare variable name inside the marker, so the "only {{varName}}"
+// guarantee below is unchanged: there is no expression to evaluate, only a
+// value to compare against 'true'.
+const BLOCK_RE = /^[ \t]*\{\{#if ([^{}]*)\}\}[ \t]*\n([\s\S]*?)^[ \t]*\{\{\/if\}\}[ \t]*\n/gm;
+
+function applyBlocks(template: string, values: Record<string, string>): string {
+  return template.replace(BLOCK_RE, (_match, rawExpr: string, body: string) => {
+    const expr = rawExpr.trim();
+    if (!SAFE_VAR_RE.test(expr)) {
+      throw new TemplateRenderError(
+        `unsupported template expression "{{#if ${rawExpr}}}" — only {{#if varName}} is allowed`,
+      );
+    }
+    if (!Object.prototype.hasOwnProperty.call(values, expr)) {
+      throw new TemplateRenderError(`template referenced unknown variable "${expr}"`);
+    }
+    // Anything other than the literal 'true' drops the block. Pack inputs are
+    // strings with a 'true'/'false' enum, and treating a stray value as truthy
+    // would turn a typo into a granted write scope.
+    return values[expr] === 'true' ? body : '';
+  });
+}
+
 export function applyTemplate(template: string, values: Record<string, string>): string {
-  return template.replace(TAG_RE, (_match, rawExpr: string) => {
+  // Blocks first: a dropped block must not have its {{vars}} resolved, and an
+  // unresolvable variable inside a dropped block is not an error.
+  return applyBlocks(template, values).replace(TAG_RE, (_match, rawExpr: string) => {
     const expr = rawExpr.trim();
     if (!SAFE_VAR_RE.test(expr)) {
       throw new TemplateRenderError(

packages/actions/src/index.test.ts

@@ -350,4 +350,70 @@ describe('built-in packs', () => {
     expect(directives).toMatch(/^on:\n\s+pull_request:\s*$/m);
     expect(entry.manifest.security.allowPullRequestTarget).toBe(false);
   });
+
+  it('omits the write scopes and the steps that need them, rather than disabling them', async () => {
+    // The read-only install is the version a first-time reviewer is asked to
+    // trust, so "least privilege" has to be a property of the rendered file
+    // rather than of a condition inside it. A shipped-but-disabled Security
+    // tab upload still asks a maintainer to read and reason about an upload.
+    //
+    // mac-developer-bridge declined the earlier shape on exactly this: the two
+    // write scopes were requested unconditionally even though the workflow was
+    // described as report-only, and GitHub downgrades them on fork pull
+    // requests anyway — so the richest outputs were the least reliable ones
+    // precisely where the scan is most useful.
+    const catalog = await loadBuiltinPacks();
+    const entry = catalog.get('threatcrush-scan');
+    if (!entry) throw new Error('threatcrush-scan not in catalog');
+    const result = await renderPack({
+      packDir: entry.packDir,
+      manifest: entry.manifest,
+      inputs: { commentOnPr: 'false', uploadSarif: 'false' },
+    });
+    const content = result.files[0]?.content ?? '';
+    const workflow = parseYaml(content);
+
+    expect(workflow.permissions).toEqual({ contents: 'read' });
+
+    const names = (workflow?.jobs?.scan?.steps ?? []).map((step: { name?: string }) => step?.name);
+    expect(names).not.toContain('Upload to the Security tab');
+    expect(names).not.toContain('Comment on PR');
+
+    // Gone from the file, not merely unreachable in it. `upload-sarif` and
+    // `github-script` are the two actions that would hold those scopes.
+    expect(content).not.toContain('upload-sarif');
+    expect(content).not.toContain('github-script');
+    expect(content).not.toContain('security-events');
+    expect(content).not.toContain('pull-requests: write');
+
+    // The read-only outputs are the ones that survive, and they are also the
+    // two that work on a fork pull request.
+    expect(names).toContain('Build the report');
+    expect(names).toContain('Upload SARIF artifact');
+    expect(content).toContain('$GITHUB_STEP_SUMMARY');
+  });
+
+  it('still asks for a write scope only when the output that needs it is on', async () => {
+    // Each scope is emitted by its own output, so the block cannot drift out
+    // of step with what the workflow does. It used to be one hand-assembled
+    // `extraPermissions` string, which made least privilege something a caller
+    // had to remember.
+    const catalog = await loadBuiltinPacks();
+    const entry = catalog.get('threatcrush-scan');
+    if (!entry) throw new Error('threatcrush-scan not in catalog');
+
+    const permissionsFor = async (inputs: Record<string, string>) => {
+      const result = await renderPack({ packDir: entry.packDir, manifest: entry.manifest, inputs });
+      return parseYaml(result.files[0]?.content ?? '').permissions;
+    };
+
+    expect(await permissionsFor({ commentOnPr: 'true', uploadSarif: 'false' })).toEqual({
+      contents: 'read',
+      'pull-requests': 'write',
+    });
+    expect(await permissionsFor({ commentOnPr: 'false', uploadSarif: 'true' })).toEqual({
+      contents: 'read',
+      'security-events': 'write',
+    });
+  });
 });

packages/actions/threatcrush-scan/README.md

@@ -17,7 +17,33 @@ sh1pt actions install threatcrush-scan --repo owner/name --pr
 | `threatcrushPackageSpec` | `@profullstack/threatcrush@0.11.2` | npm spec used to install the CLI. Pinned rather than `@latest` so one bad publish cannot break every consumer at once; bump it in a pack release. |
 | `threatcrushIntegrity` | *(sha512 of 0.11.2)* | SRI hash of that tarball. The workflow downloads, hashes and compares before installing, and refuses to install on a mismatch. Bump it with the spec — read it from `npm view <spec> dist.integrity`. Empty skips the check. |
 | `failOn` | *(empty)* | Comma-separated severities that fail the job, e.g. `critical,high`. Empty is report-only. |
-| `uploadSarif` | `true` | Upload to the Security tab. |
+| `uploadSarif` | `true` | Upload to the Security tab. Emits `security-events: write`. |
+| `commentOnPr` | `true` | Post the report as a pull request comment. Emits `pull-requests: write`. |
+
+## Least privilege is a property of the file, not a condition inside it
+
+Set `uploadSarif` and `commentOnPr` both to `false` and the rendered workflow
+asks for `contents: read` and nothing else. The findings go to the job summary
+and the SARIF artifact, neither of which needs a write scope.
+
+The two steps that would use those scopes are **not present** in that render —
+not shipped-and-disabled. This is the difference the pack cares about: a
+disabled Security-tab upload still asks a maintainer to read an upload, reason
+about what it mutates, and take on trust that the condition guarding it is
+correct. Dead surface in a security-sensitive file is surface all the same, and
+a reviewer counting what they are being asked to trust counts it.
+
+Each scope is emitted by the output that needs it, so the `permissions:` block
+cannot drift out of step with what the workflow actually does. It used to be a
+single hand-assembled `extraPermissions` string, which made least privilege
+something a caller had to remember rather than something the template
+guaranteed.
+
+There is a second reason to prefer this shape on a first install, and it is not
+about trust: **fork pull requests get a read-only `GITHUB_TOKEN`**. The comment
+and the Security-tab upload are the outputs GitHub downgrades, so the richest
+reporting is least reliable exactly where an external scan is most useful. The
+job summary and the artifact work the same on every pull request.
 
 ## Pinned means pinned — including for fixes
 

packages/actions/threatcrush-scan/sh1pt.actionpack.yaml

@@ -5,7 +5,7 @@ description: >-
   Scans pull requests for hardcoded credentials, injection, SSRF, unsafe
   deserialisation and dependency tampering, and uploads SARIF to the Security
   tab.
-version: 1.7.0
+version: 2.0.0
 publisher: profullstack
 visibility: public
 license: MIT
@@ -74,17 +74,15 @@ inputs:
       and the SARIF artifact instead. That is the configuration for a
       repository that wants the scan without granting a third-party CLI any
       write scope, which is a substantial part of what reviewers decline on.
-  extraPermissions:
-    type: string
-    default: "  pull-requests: write\n  security-events: write"
-    description: >-
-      The permission lines added beneath `contents: read`, computed from
-      uploadSarif and commentOnPr rather than set by hand. Two spaces of
-      indentation per line; empty when neither output is enabled.
-
-      An input rather than a fixed block because a workflow that asks for a
-      write scope it will not use cannot argue it is least-privilege, and the
-      two scopes here only exist to serve features a consumer can switch off.
+  # No input controls the `permissions:` block. Each write scope is emitted by
+  # the output that needs it — `pull-requests: write` from commentOnPr,
+  # `security-events: write` from uploadSarif — so the block cannot drift out of
+  # step with what the workflow actually does. It used to be an `extraPermissions`
+  # string the caller assembled by hand, which made "least privilege" a thing a
+  # caller had to remember rather than a property of the template.
+  #
+  # Omitted, not disabled: with both outputs off the rendered file has no write
+  # scope and no step that would use one.
   failOn:
     type: string
     default: ''