fix(ci): run the root-directory guard on stock macOS bash 3.2 (#12879)

* fix(ci): run the root-directory guard on stock macOS bash 3.2

The guard script builds its base-tree lookup with `declare -A`, which
needs bash 4+. Its test spawns plain `bash` from PATH, and stock macOS
has shipped /bin/bash 3.2 since 2007, so on any Mac without a Homebrew
bash the script exits 2 before asserting anything and the default
`pnpm test` suite fails 3 of the guard's 4 cases. Machines with a
Homebrew bash on PATH never see it, which is why it went unnoticed.

Replace the associative array with a plain-array linear scan. Root
directories number in the dozens, so the O(n^2) membership check is
negligible, and the NUL-delimited reads that protect unusual filenames
stay as they were. The empty-array expansion is guarded for
`set -u` under bash 3.2.

All four guard tests now pass with /bin/bash 3.2; behavior under CI's
bash 5 is unchanged.

* fix(ci): run the root-directory guard under node instead of bash

The guard is the only check in the repo written in shell, and it used
`declare -A`, which stock macOS `/bin/bash` 3.2 does not have — so the
guard's own test suite failed 3 of 4 cases on any Mac without a Homebrew
bash. CI never noticed because runners ship bash 5.

Porting it to node removes the interpreter-version variable instead of
working around one construct: node is what the sibling script in this
directory already uses, it is the runtime that runs the test, and the
NUL-delimited read is the same shape as check-changed-code-quality.mjs.
It also drops a latent false pass — a failing `git ls-tree` inside the
shell's `< <(...)` was not caught by `pipefail`, so the read loop saw
nothing and the guard reported success. `execFileSync` throws instead,
which is why the two `git rev-parse --verify` probes are no longer
needed.

Output and exit codes are otherwise unchanged; the usage line now prints
node's script path where the shell printed `$0`.

Tests pin each guarantee and fail when it is reverted: NUL-delimited
reads so odd paths are reported unmangled, exit 2 on bad usage, and
git's own 128 with no node stack trace when a sha does not resolve.

* fix(ci): keep root entry bytes intact and fence guard output

git pathnames are arbitrary bytes, but the guard read ls-tree with
encoding 'utf8', so every invalid sequence collapsed to U+FFFD. That
mangled the reported name and, because the replacement is not
injective, let two different entries compare equal — a genuinely new
root entry could be waved through as pre-existing. Read the bytes as
latin1 and write them back unchanged.

The blocked-entry list is also attacker-controlled and went straight to
stdout. The runner trims leading whitespace before matching '::', so an
indented entry name still parses as a workflow command, and a pathname
may embed a newline. Wrap the list in ::stop-commands:: with a random
resume token so only the guard's own annotation is acted on.

---------

Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
This commit is contained in:
Hyeonho Lee (Ethan) 2026-08-10 06:06:25 +09:00 committed by GitHub
parent 9a6c5cddc9
commit 850342a3e0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 194 additions and 41 deletions

View File

@ -0,0 +1,64 @@
import { execFileSync } from 'node:child_process'
import { randomUUID } from 'node:crypto'
function readRootEntries(sha) {
// Why: a git pathname is arbitrary bytes, and 'utf8' folds every invalid
// sequence to U+FFFD — that mangles the reported name and makes two different
// entries compare equal, so a genuinely new one can slip past the Set below.
// latin1 maps each byte to one code unit, so the bytes survive the round trip.
const stdout = execFileSync('git', ['ls-tree', '-z', '--name-only', sha], {
encoding: 'latin1',
stdio: ['ignore', 'pipe', 'inherit']
})
return stdout.split('\0').filter(Boolean)
}
function checkRootDirectoryEntries(argv) {
if (argv.length !== 2) {
console.error(`Usage: ${process.argv[1]} <base-sha> <head-sha>`)
return 2
}
const [baseSha, headSha] = argv
const baseEntries = new Set(readRootEntries(baseSha))
const blockedEntries = readRootEntries(headSha).filter((entry) => !baseEntries.has(entry))
if (blockedEntries.length === 0) {
console.log('Root directory guard passed: no new root-level files or folders.')
return 0
}
console.log(
'::error title=Root-level additions blocked::New root-level files or folders bloat the GitHub landing page.'
)
console.log('Root directory guard failed.')
console.log(
'New root-level files or folders are not allowed because they bloat the GitHub landing page.'
)
console.log('Move each new entry under an existing top-level directory.')
console.log('Blocked entries:')
// Why: an entry name is attacker-controlled and may start with '::' (the runner
// trims leading spaces before matching) or embed a newline, so printing it bare
// lets a PR forge annotations. Fence the untrusted list with an unguessable
// stop-commands token, and write the raw bytes rather than a re-encoded string.
const resumeToken = randomUUID()
console.log(`::stop-commands::${resumeToken}`)
for (const entry of blockedEntries) {
process.stdout.write(Buffer.from(` ${entry}\n`, 'latin1'))
}
console.log(`::${resumeToken}::`)
return 1
}
try {
// Why: process.exit truncates a piped write part-way through on macOS, so set
// exitCode and let node flush the blocked-entry list before it exits.
process.exitCode = checkRootDirectoryEntries(process.argv.slice(2))
} catch (error) {
// Why: git already reported the failure on the inherited stderr, so surface its
// status rather than a node stack trace. Anything else is a real bug — rethrow.
if (typeof error.status !== 'number') {
throw error
}
process.exitCode = error.status
}

View File

@ -1,37 +0,0 @@
#!/usr/bin/env bash
set -euo pipefail
if [[ $# -ne 2 ]]; then
echo "Usage: $0 <base-sha> <head-sha>" >&2
exit 2
fi
base_sha=$1
head_sha=$2
git rev-parse --verify "${base_sha}^{tree}" >/dev/null
git rev-parse --verify "${head_sha}^{tree}" >/dev/null
declare -A base_entries=()
while IFS= read -r -d '' entry; do
base_entries["$entry"]=1
done < <(git ls-tree -z --name-only "$base_sha")
blocked_entries=()
while IFS= read -r -d '' entry; do
if [[ -z "${base_entries[$entry]+present}" ]]; then
blocked_entries+=("$entry")
fi
done < <(git ls-tree -z --name-only "$head_sha")
if (( ${#blocked_entries[@]} == 0 )); then
echo "Root directory guard passed: no new root-level files or folders."
exit 0
fi
echo "::error title=Root-level additions blocked::New root-level files or folders bloat the GitHub landing page."
echo "Root directory guard failed."
echo "New root-level files or folders are not allowed because they bloat the GitHub landing page."
echo "Move each new entry under an existing top-level directory."
printf 'Blocked entries:\n'
printf ' %s\n' "${blocked_entries[@]}"
exit 1

View File

@ -108,7 +108,7 @@ jobs:
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: bash .github/scripts/check-root-directory-entries.sh "$BASE_SHA" "$HEAD_SHA"
run: node .github/scripts/check-root-directory-entries.mjs "$BASE_SHA" "$HEAD_SHA"
typecheck:
runs-on: ubuntu-latest

View File

@ -6,7 +6,7 @@ import { afterEach, describe, expect, it } from 'vitest'
import { parse } from 'yaml'
const projectDir = resolve(import.meta.dirname, '../..')
const guardScript = join(projectDir, '.github/scripts/check-root-directory-entries.sh')
const guardScript = join(projectDir, '.github/scripts/check-root-directory-entries.mjs')
const tempDirs = []
function git(cwd, args) {
@ -37,13 +37,45 @@ function commitFiles(root, files) {
return git(root, ['rev-parse', 'HEAD'])
}
// Why: a root entry name can be bytes no filesystem here accepts (APFS rejects
// invalid UTF-8), so build the tree in the object database instead of on disk.
// git ls-tree -z emits exactly the record format git mktree -z reads back.
function commitRawEntries(root, parent, entries) {
const parentTree = execFileSync('git', ['ls-tree', '-z', parent], { cwd: root })
const records = entries.map((name) => {
const blob = execFileSync('git', ['hash-object', '-w', '--stdin'], {
cwd: root,
encoding: 'utf8',
input: 'too prominent\n'
}).trim()
return Buffer.concat([Buffer.from(`100644 blob ${blob}\t`), name, Buffer.from([0])])
})
const tree = execFileSync('git', ['mktree', '-z'], {
cwd: root,
encoding: 'utf8',
input: Buffer.concat([parentTree, ...records])
}).trim()
return execFileSync('git', ['commit-tree', tree, '-p', parent, '-m', 'head'], {
cwd: root,
encoding: 'utf8'
}).trim()
}
function runGuard({ root, base, head }) {
return spawnSync('bash', [guardScript, base, head], {
return runGuardArgs(root, [base, head])
}
function runGuardArgs(root, args) {
return spawnSync(process.execPath, [guardScript, ...args], {
cwd: root,
encoding: 'utf8'
})
}
function runGuardBytes({ root, base, head }) {
return spawnSync(process.execPath, [guardScript, base, head], { cwd: root })
}
afterEach(() => {
while (tempDirs.length > 0) {
rmSync(tempDirs.pop(), { force: true, recursive: true })
@ -84,6 +116,100 @@ describe('root directory guard', () => {
expect(output).toContain('new-folder')
})
// Why: git escapes odd paths unless it is read NUL-delimited, so dropping -z
// (or decoding the bytes wrong) reports a mangled name nobody can act on.
it.skipIf(process.platform === 'win32')('reports a blocked entry byte-for-byte', () => {
const awkwardName = '日本 root file\nwith newline.txt'
const fixture = makeFixture()
const head = commitFiles(fixture.root, [[awkwardName, 'too prominent\n']])
const result = runGuard({ ...fixture, head })
expect(result.status).toBe(1)
expect(result.stdout).toContain(awkwardName)
})
// Why: decoding git's output as UTF-8 rewrites every invalid byte to U+FFFD, so
// the name the guard prints is not the name anyone has to rename.
it('reports an entry whose name is not valid UTF-8 byte-for-byte', () => {
const rawName = Buffer.concat([Buffer.from([0xff, 0xfe]), Buffer.from('-raw.txt')])
const fixture = makeFixture()
const head = commitRawEntries(fixture.root, fixture.base, [rawName])
const result = runGuardBytes({ ...fixture, head })
expect(result.status).toBe(1)
expect(result.stdout.includes(rawName)).toBe(true)
})
// Why: U+FFFD is not injective, so two different invalid names decode to the
// same string and a new root entry gets waved through as pre-existing.
it('does not confuse two different invalid UTF-8 names for the same entry', () => {
const fixture = makeFixture()
const base = commitRawEntries(fixture.root, fixture.base, [
Buffer.concat([Buffer.from([0xc0, 0x80]), Buffer.from('.txt')])
])
const head = commitRawEntries(fixture.root, fixture.base, [
Buffer.concat([Buffer.from([0xc0, 0x81]), Buffer.from('.txt')])
])
const result = runGuardBytes({ root: fixture.root, base, head })
expect(result.status).toBe(1)
expect(result.stdout.toString('latin1')).not.toContain('guard passed')
})
// Why: the runner trims leading spaces before matching '::', so an indented
// entry name still reaches the workflow-command parser and can forge output.
it('prints blocked entries with workflow-command parsing disabled', () => {
const fixture = makeFixture()
const forgedName = '::error title=forged::injected\n::warning::second line.txt'
const head = commitRawEntries(fixture.root, fixture.base, [Buffer.from(forgedName)])
const result = runGuard({ ...fixture, head })
const lines = result.stdout.split('\n')
const stopIndex = lines.findIndex((line) => line.startsWith('::stop-commands::'))
const resumeToken = lines[stopIndex]?.slice('::stop-commands::'.length)
const resumeIndex = lines.indexOf(`::${resumeToken}::`)
const escaped = lines.filter(
(line, index) =>
(index < stopIndex || index > resumeIndex) && line.trimStart().startsWith('::')
)
expect(result.status).toBe(1)
expect(resumeToken).toMatch(/^[\da-f-]{36}$/)
expect(stopIndex).toBeLessThan(resumeIndex)
// Why: the guard's own annotation is the only line the runner may act on.
expect(escaped).toHaveLength(1)
expect(escaped[0]).toContain('Root-level additions blocked')
expect(lines.slice(stopIndex, resumeIndex).join('\n')).toContain(forgedName)
})
it('exits 2 with usage when the two shas are not both supplied', () => {
const fixture = makeFixture()
const result = runGuardArgs(fixture.root, [fixture.base])
expect(result.status).toBe(2)
expect(result.stderr).toContain('<base-sha> <head-sha>')
})
it('fails loudly instead of passing when a sha does not resolve', () => {
const fixture = makeFixture()
const result = runGuardArgs(fixture.root, [
fixture.base,
'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
])
// Why: git's own exit status, not node's. An unhandled throw is also non-zero,
// so assert the status and the absent stack trace or the guard's error
// handling can be deleted without a test noticing.
expect(result.status).toBe(128)
expect(result.stderr).not.toContain('node:internal')
expect(result.stdout).not.toContain('guard passed')
})
it('is wired into the PR verify gate', () => {
const workflow = parse(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8'))
const guardJob = workflow.jobs.root_directory_guard
@ -93,7 +219,7 @@ describe('root directory guard', () => {
expect(guardJob.name).toBe('root directory guard')
expect(guardJob.steps[0].with['fetch-depth']).toBe(0)
expect(guardStep.run).toContain('.github/scripts/check-root-directory-entries.sh')
expect(guardStep.run).toContain('node .github/scripts/check-root-directory-entries.mjs')
expect(workflow.jobs.verify.needs).toContain('root_directory_guard')
})
})