Nate Swenson

A branch name that turned auto-merge on for every pull request

Shipflow · No. 099 · 12 min read ·v0.4.0

Shipped

This release closes out an arc that started as a branching-workflow scaffolder and turned into a hardening exercise. Along the way it picked up three selectable workflow patterns, per-job least-privilege permissions in all six templates, a component-release flow that reads a tag back from origin before calling anything done, and a string of security fixes, one of them Critical.

The Critical one is the useful part for anyone else. A tool that renders CI config from a settings file is a code generator, and its output executes with a credential the person editing that settings file is not supposed to hold. That is a trust boundary, and it does not look like one.

The boundary hiding inside your config file

Start with who can write what.

Branch protection, required checks, and secrets are admin-scoped. Adding a file to .github/ is not: anyone with write access can open a PR that edits it, and on many repos, merge it. GitHub’s own security hardening guide is blunt about the surrounding risk, noting that “any user with write access to your repository have read access to all secrets configured in your repository,” and telling you to keep workflow credentials at least privilege.

So when a generator reads .github/yourtool.json and renders a workflow that runs gh with a promotion token, the config is lower-trust input than the thing it controls. A value in that file reaches a privileged credential without ever passing through an admin.

That is CWE-1336, template engine injection: “the product fails to neutralize or incorrectly handles special syntax that template engines can interpret as code.” The classic version is a web template rendering {{7*7}}. This version is quieter, because the injected value never has to look like code. It only has to end a quote.

Build the renderer everyone builds first

Here is a template close to a real promotion workflow. Save it as promote.yml.tmpl:

name: auto-merge {{DEV_BRANCH}} into {{MAIN_BRANCH}}

on:
  pull_request:
    types: [opened, synchronize, reopened]
    branches: ['{{MAIN_BRANCH}}']

permissions: {}

jobs:
  automerge:
    if: github.event.pull_request.head.ref == '{{DEV_BRANCH}}'
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write
    steps:
      - name: Enable auto-merge
        env:
          GH_TOKEN: ${{ secrets.{{MERGE_CREDENTIAL}} }}
          PR: ${{ github.event.pull_request.html_url }}
        run: gh pr merge --auto {{MERGE_FLAG}} "$PR"

The if: line is the security control. It is what stops this workflow from auto-merging pull requests that are not genuine promotions.

Now the renderer. This is the version almost everyone writes first, in render-naive.mjs:

const TOKEN_RE = /\{\{(\w+)\}\}/g;

export function renderNaive(template, params) {
  return template.replace(TOKEN_RE, (_, name) => String(params[name]));
}

Six lines, no dependencies, works. Feed it a config where DEV_BRANCH is dev' || 'x'=='x and watch the control evaporate. Save this as exploit.mjs:

import { readFileSync } from 'node:fs';
import { renderNaive } from './render-naive.mjs';

const template = readFileSync('promote.yml.tmpl', 'utf8');

const rendered = renderNaive(template, {
  DEV_BRANCH: "dev' || 'x'=='x",
  MAIN_BRANCH: 'main',
  MERGE_CREDENTIAL: 'PROMOTE_PAT',
  MERGE_FLAG: '--merge',
});

for (const line of rendered.split('\n')) {
  if (line.includes('if:')) console.log(line.trim());
}

Run it with node exploit.mjs:

if: github.event.pull_request.head.ref == 'dev' || 'x'=='x'

The quote closed the comparison, and || 'x'=='x' made the whole expression unconditionally true. That workflow now enables auto-merge on any pull request into main. A newline in the same value is worse: it starts a new YAML key, which means new steps in a file that is committed and then executed.

Notice what did not happen. Nothing crashed, and nothing looked malformed.

Valid YAML was never the question

The instinct at this point is to add a YAML parse check. It does not help, and it is worth proving to yourself before you rely on it. Save this as check.mjs:

import { readFileSync } from 'node:fs';
import { parse } from 'yaml';
import { renderNaive } from './render-naive.mjs';

const template = readFileSync('promote.yml.tmpl', 'utf8');
const rendered = renderNaive(template, {
  DEV_BRANCH: "dev' || 'x'=='x",
  MAIN_BRANCH: 'main',
  MERGE_CREDENTIAL: 'PROMOTE_PAT',
  MERGE_FLAG: '--merge',
});

console.log('parses as YAML:', !!parse(rendered));
console.log('if condition   :', parse(rendered).jobs.automerge.if);

With npm install yaml done first, node check.mjs prints:

parses as YAML: true
if condition   : github.event.pull_request.head.ref == 'dev' || 'x'=='x'

Perfectly valid YAML. Syntactic validity tells you the file will load; it says nothing about whether the value stayed inside the slot you put it in. Keep the parse check, because it catches a different bug class, but do not let it stand in for validation.

Give every token its own validator

The fix is not one escape function. The same renderer drops values into different grammars, and each grammar has a different set of characters that mean something:

  • DEV_BRANCH and MAIN_BRANCH land inside single-quoted YAML scalars. Quotes and newlines are the escape characters.
  • MERGE_CREDENTIAL lands inside a ${{ secrets.X }} expression, so the only correct rule is GitHub’s own: secret names “can only contain alphanumeric characters ([a-z], [A-Z], [0-9]) or underscores (_)” and must not start with a number.
  • MERGE_FLAG is derived by your code from a config enum, never copied from config text, so it should be checked against a closed set.

One global sanitizer would be wrong for at least three of those. Replace the renderer with render.mjs:

const TOKEN_RE = /\{\{(\w+)\}\}/g;

// A branch name lands inside a single-quoted YAML scalar, so a quote closes
// the string early and a newline starts a new YAML key.
const UNSAFE_IN_QUOTED_SCALAR = /['"\r\n]/;

// A credential name lands inside a ${{ secrets.X }} expression, so it has to
// satisfy GitHub's own secret-naming rule and nothing looser.
const SAFE_SECRET_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/;

// Closed enum: the caller derives it, a config value never reaches it.
const MERGE_FLAGS = new Set(['--merge', '--squash', '--rebase']);

const VALIDATORS = Object.freeze({
  DEV_BRANCH: (v) => !UNSAFE_IN_QUOTED_SCALAR.test(v),
  MAIN_BRANCH: (v) => !UNSAFE_IN_QUOTED_SCALAR.test(v),
  MERGE_CREDENTIAL: (v) => SAFE_SECRET_NAME.test(v),
  MERGE_FLAG: (v) => MERGE_FLAGS.has(v),
});

export function render(template, params) {
  const missing = [];
  const unsafe = [];

  const out = template.replace(TOKEN_RE, (_, name) => {
    // `name in params` is true for a key whose value is undefined, which is
    // how the string "undefined" ends up in a workflow that installs fine
    // and never fires.
    const value = params[name];
    if (value === undefined || value === null) {
      missing.push(name);
      return `{{${name}}}`;
    }
    const str = String(value);
    const validate = VALIDATORS[name];
    if (!validate) {
      unsafe.push(`${name} (no validator declared)`);
      return `{{${name}}}`;
    }
    if (!validate(str)) unsafe.push(name);
    return str;
  });

  if (missing.length) {
    throw new Error(`render: missing value for ${missing.join(', ')}`);
  }
  if (unsafe.length) {
    throw new Error(`render: unsafe value for ${unsafe.join(', ')}`);
  }
  return out;
}

Three properties matter more than the regexes.

It throws instead of sanitizing. A rejected branch name is a config error the maintainer should see and fix, and silently rewriting someone’s branch name into something that renders safely produces a workflow that guards the wrong branch.

A token with no declared validator is treated as unsafe. Add a token to the template, forget the validator, and the render fails rather than passing the value through unchecked. Fail-closed on your own future edits is the part that survives contact with a growing template set.

It collects all failures before throwing. Reporting one bad value per run turns a four-field config into four round trips.

Prove it from both sides

A validation test that only checks good input passes will keep passing after someone weakens the checker. Every rejection needs its own case, using the payload that actually worked against the naive version:

import { test } from 'node:test';
import assert from 'node:assert/strict';
import { readFileSync } from 'node:fs';
import { parse } from 'yaml';
import { render } from './render.mjs';

const template = readFileSync(new URL('promote.yml.tmpl', import.meta.url), 'utf8');

const GOOD = {
  DEV_BRANCH: 'dev',
  MAIN_BRANCH: 'main',
  MERGE_CREDENTIAL: 'PROMOTE_PAT',
  MERGE_FLAG: '--merge',
};

test('a legal config renders parseable YAML with the guard intact', () => {
  const doc = parse(render(template, GOOD));
  assert.equal(
    doc.jobs.automerge.if,
    "github.event.pull_request.head.ref == 'dev'"
  );
  assert.deepEqual(doc.permissions, {});
});

// Two-sided: the same inputs that broke the naive renderer must now throw.
for (const [label, bad] of [
  ['quote breaks out of the if: condition', { DEV_BRANCH: "dev' || 'x'=='x" }],
  ['newline injects a new YAML key', { DEV_BRANCH: 'dev\non: push' }],
  ['secret name with an expression in it', { MERGE_CREDENTIAL: 'A }} ${{ secrets.B' }],
  ['merge flag outside the enum', { MERGE_FLAG: '--merge; curl evil.sh' }],
]) {
  test(`rejects: ${label}`, () => {
    assert.throws(
      () => render(template, { ...GOOD, ...bad }),
      /unsafe value/
    );
  });
}

test('a present-but-undefined value is missing, not the string "undefined"', () => {
  assert.throws(
    () => render(template, { ...GOOD, MAIN_BRANCH: undefined }),
    /missing value for MAIN_BRANCH/
  );
});

node --test gives you this, with the trailing counter lines trimmed and your durations differing:

✔ a legal config renders parseable YAML with the guard intact (3.94075ms)
✔ rejects: quote breaks out of the if: condition (0.166417ms)
✔ rejects: newline injects a new YAML key (0.039583ms)
✔ rejects: secret name with an expression in it (0.034416ms)
✔ rejects: merge flag outside the enum (0.034459ms)
✔ a present-but-undefined value is missing, not the string "undefined" (0.044625ms)
ℹ tests 6
ℹ pass 6
ℹ fail 0

The first test asserts on the parsed if: string, not on a substring of the rendered file. A .includes('dev') assertion passes against the injected version too.

While you are in the templates, set permissions: {} at the workflow level and grant per job, which is what the first test checks. zizmor flags the workflow-level grant as excessive-permissions, and its reasoning is the part worth internalizing: permissions “should be declared as minimally as possible, and as close to their usage site as possible,” because a workflow-level grant silently extends to jobs added later that nobody reviewed for it.

Gotchas

A present-but-undefined param renders the literal string undefined. Testing presence with key in params returns true for a key explicitly set to undefined, so String(undefined) flows through as a real value and passes every safety regex, since "undefined" contains no quotes or newlines. The symptom is the worst kind: a workflow that is valid YAML, installs cleanly, and can never fire. In this arc it rendered name: auto-merge dev into undefined and branches: [undefined], with no error at apply time. Escape: treat undefined and null as missing, not as values, and assert on a parsed field rather than on the file’s text. This one was caught by a baseline eval on its first run, not by review.

Your fix only ships if the fix is what runs. A bare npx <your-tool> <command> can resolve a stale global install already on PATH instead of fetching the current version from the registry, with no warning that it happened. During the dogfood run right after the Critical fix released, a bare invocation silently ran a months-old global copy that predated every security fix in the series. The symptom is a tool that reports success while running pre-patch behavior. Escape: pin @latest on every invocation in your docs and your skill instructions, and add a test that greps your own documentation for unpinned invocations.

A guard that lives in the caller is not a guard. Later in this arc, CodeQL flagged a changelog helper that built a RegExp from a version string and escaped only the dots, leaving \, *, +, ( and [ live. It was not reachable through the CLI, because the calling command rejects a non-semver version first. It was still wrong, because the function is exported and independently callable. The CodeQL guidance for js/incomplete-sanitization is to prefer a well-tested library, or better, “design the application so that sanitization is not needed.” Here that meant dropping the regex entirely for plain string operations, which also made the check identical to the awk that reads the same file at release time. Escape: when a validated value crosses a module boundary, revalidate at the boundary or remove the need to.

Least privilege can break the error path you never exercise. Scoping permissions per job surfaced two merge-back workflows granted only contents: write, whose failure handler calls gh pr create and needs pull-requests: write. That step runs only after a merge conflict, so under a defaulted token it would have failed exactly when it was the last thing left to surface the problem. Escape: when you tighten permissions, enumerate the steps in failure branches, not just the happy path.

Sources

Changelog

  • feat(release): “release devlog” means a tag, and it has to prove it (#158) (3cbe622)
  • fix(shipflow): least-privilege permissions in every rendered workflow (0.3.3) (#150) (6d635eb)
  • chore(shipflow): release 0.3.0 (#99) (aa6049e)
  • shipflow: multi-pattern workflow templates (dev-main-promotion, github-flow, gitflow) (#74) (1e1c848)
  • docs: professional README pass — dynamic version badges, clean repo layout (#69) (e102ac5)
  • fix(shipflow): pin @latest on every npx invocation; docs pass (0.2.6) (#67) (91ce790)
  • fix(shipflow): mandatory TOCTOU guard, forced-override auditability, subprocess timeouts, YAML-validity CI check (0.2.5) (#63) (b696088)
  • fix(shipflow): REST-path encoding, resolveOwnerRepo hardening, file-size cap (0.2.4) (#61) (e517c81)
  • fix(shipflow): Critical — unescaped template substitution allowed workflow injection (0.2.3) (#59) (e9da5e0)
  • fix(shipflow): label-release-pending never fires under GITHUB_TOKEN-attributed auto-merge (0.2.2) (#57) (7eb902f)
  • fix(shipflow): rendered auto-merge workflow was missing --repo on both gh calls (0.2.1) (#54) (494e11f)
  • fix(shipflow): filter requiredChecks candidates to PR-triggered jobs, add agent-driven CI scaffolding (c80c48e)
  • fix(shipflow): first-run interview UX, default-branch mismatch handling, honest ruleset-error classification (0.2.0) (#50) (d348afb)
  • feat(shipflow): Phase A — manual-gate branch/release workflow skill (#49) (eea8f31)