Skip to content

chore(hooks): give nobody test runs the .nvmrc node and drop an unreadable CA bundle - #669

Closed
aliasunder wants to merge 19 commits into
mainfrom
fix/nobody-runs-match-ci
Closed

aliasunder wants to merge 19 commits into
mainfrom
fix/nobody-runs-match-ci

Conversation

@aliasunder

@aliasunder aliasunder commented Oct 11, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

The root-test hook stops a test run in a root session and replies with a setpriv prefix that reruns it as nobody. In Claude Code's cloud containers, that prefix gave a run CI does not:

Problem Effect
nobody's node is the container's own (v22), not the major version .nvmrc names and CI runs (24). nvm installs the right one under root's home, which nobody cannot enter. server-error-contracts.test.ts run alone on main: 26 failed with RegExp.escape is not a function.
The container sets NODE_EXTRA_CA_CERTS to a file under root's home. Every node that nobody starts prints "Ignoring extra certs … Permission denied" on stderr, so the two exact-stderr tests in render-social-preview.test.ts fail.

The prefix now matches CI in both respects:

  • Node. When nobody's node has another major version than .nvmrc names, and root has a node of that major, the hook copies it to node_modules/.nobody-node/node-<version>/node and puts that folder first on PATH in the prefix.
    • Where the node comes from: the hook's own node (the binary it runs from, process.execPath, so a version-manager shim is never copied). Otherwise it takes the newest node of that major under $NVM_DIR, then ~/.nvm, matching the version only in nvm's own version folders. Claude Code starts hooks with its own PATH, not the session's, so in a cloud container the hook's node is usually the container's.
    • The copy: the folder and binary belong to root with mode 755, so nobody can run the copy but not replace it. The hook writes to a temp name and moves it into place, so a later run never reuses a partial copy, and it reuses a whole one.
    • What runs it: npx, npm exec and tsx look node up on PATH, and the integration harness starts servers with process.execPath, so all of them run the copy.
    • How the hook checks nobody's node: it runs /usr/bin/env node. setpriv keeps root's capabilities until it starts its command, so setpriv … node would find root's nvm node instead of the one nobody's run uses.
    • When no node of that major exists: the hook gives the old prefix and the old version warning.
  • CA bundle. When NODE_EXTRA_CA_CERTS is set and nobody cannot read the file, the prefix adds -u NODE_EXTRA_CA_CERTS. The suites this prefix runs make no network calls. A file nobody can read stays set.
  • Prefix form. The prefix names /usr/bin/env in full for the same reason the check does. It also quotes "$PATH", so a session PATH with a space in a folder name stays one argument.
  • Runs the hook now stops. Main let each of these through as root. The first three matter because a run that keeps the reply's prefix but drops setpriv would otherwise go through:
    • a run through env, time, timeout, bash, sh or zsh named by path, such as /usr/bin/env HOME=… npm test;
    • env's -u NAME, also at the end of a group of flags (env -iu NAME npm test), and a setting that mixes quoted and unquoted text (PATH=/copy:"$PATH");
    • a setting with a backslash-escaped space, which is how the prefix writes a checkout path that holds a space (HOME=/my\ repo/…);
    • env's -C/--chdir and the folder it takes.

A reply in a cloud container now reads:

  setpriv --reuid=65534 --regid=65534 --clear-groups /usr/bin/env -u NODE_EXTRA_CA_CERTS HOME=/home/user/vault-cortex/node_modules/.nobody-home PATH=/home/user/vault-cortex/node_modules/.nobody-node/node-v24.21.0:"$PATH"
The folders nobody needs to write in /home/user/vault-cortex are ready.
nobody's own node is v22.22.0, so the prefix runs a copy of node v24.21.0, whose major version .nvmrc names and CI runs.

A session that is not root still exits at the hook's first check, so nothing changes on a developer machine.

Failure paths.

  • If the nvm search cannot read a folder or meets a symlink loop, the hook keeps the nodes it could reach, and it still stops the run (exit 2).
  • If the copy itself fails, for example on a full disk, the hook removes its temp file and exits 1, as any failing step in this hook does. Claude Code treats exit 1 as a non-blocking error, so that run goes through as root.

Not covered. Runs through sudo, nice, nohup or env -S '…' still go through as root, as on main.

Type of change

  • New MCP tool
  • Bug fix
  • Refactoring
  • Documentation
  • CI / workflow change
  • Infrastructure (SST, Docker)
  • Other: Claude Code session hook (.claude/hooks/require-nonroot-tests.sh)

Checklist

  • The pre-commit hook passed: typecheck, knip, and ESLint, Prettier, markdownlint and ShellCheck on the staged files
  • npm test passes
  • npm run build succeeds (no source the build compiles changed)
  • New MCP tools follow the naming and description conventions in AGENTS.md (no tool changes)
  • README or ARCHITECTURE.md updated (if user-facing behavior changed): no user-facing change. AGENTS.md's line for the hook now says the prefix runs a copy of root's Node of the .nvmrc major, if root has one

Tests

require-nonroot-tests.test.ts has 159 tests, up from 130 on main.

  • Stubs.
    • A node stub answers the version and binary-path questions the hook asks of its own node, and hands every other call (the payload parse) to the real node.
    • The setpriv stub now models setpriv's own PATH search. A bare command name resolves on the hook's PATH; a command given by path searches nobody's PATH.
    • A cp stub cuts a copy short.
  • New cases.
    • The copy is made, reused, and replaced after a cut-short copy.
    • The newest node is chosen, $NVM_DIR is searched before ~/.nvm, and ~/.nvm is used when $NVM_DIR has none.
    • A symlinked versions/node is followed, and a folder find cannot read is survived.
    • A /v24.x/ in the nvm folder's own path does not count as a match.
    • A full version in .nvmrc (v24.1.0) is honoured.
    • The hook copies the node binary rather than a shim.
    • The copy and its folders get mode 755 under umask 077.
    • The copy's folder is quoted when the checkout path holds a space, and the copy folder is never handed to nobody.
    • NODE_EXTRA_CA_CERTS is dropped or kept as described above.
    • Each run listed under "Runs the hook now stops" is stopped. A setting whose value ends in an escaped space (A=a\ npm test, which runs test) goes through.
  • Mutation checks. 17 mutations from the test audit, plus 9 from later fixes. Each failed its intended test. They include reverting the /usr/bin/env check, which fails 9 tests.

Review

Ship-check phases 1–5 ran on this branch, plus delta reviews of every commit after them. Every finding was fixed on the branch:

  • PR review:
    • checking nobody's node through /usr/bin/env;
    • making the copy atomic;
    • moving the copy out of nobody's HOME into a root-owned folder.
  • Code quality: 30 readability fixes from a fresh-eyes read, with names, comments and a named condition. No behaviour change apart from one reply wording.
  • Test audit: the stubs and cases above.
  • Bug check:
    • quoting "$PATH";
    • copying process.execPath instead of a possible shim;
    • removing the temp file when the copy fails;
    • following symlinked nvm folders;
    • qualifying the AGENTS.md line.
  • After the review bot:
    • keeping a failing find from ending the hook;
    • naming /usr/bin/env in the prefix;
    • listing the node stub in the test file's header.
  • Delta reviews:
    • stopping runs through a command named by path;
    • anchoring the nvm search to nvm's version folders;
    • a comment naming the symlink loop;
    • stopping the full prefix without setpriv (env's -u, mixed quoting), and quieting the nvm search's cd;
    • stopping the prefix with an escaped space, and env's -C and grouped -u.

Live Test Results

Run in a Claude Code cloud container: a root session, nobody on node v22.22.0, nvm node v24.21.0 under /root/.nvm, and NODE_EXTRA_CA_CERTS=/root/.ccr/ca-bundle.crt. Both full-suite rows ran in the same checkout at e8ca7dd; only the prefix differs.

Probe Before (main) This branch
Hook reply to npx vitest run src/utils as root prefix env HOME=…/.nobody-home, plus "nobody's node is v22.22.0, not the 24 that .nvmrc names" the prefix and lines shown above
server-error-contracts.test.ts with the hook's prefix 26 failed (RegExp.escape is not a function) passes
render-social-preview.test.ts with a Node 24 prefix that keeps NODE_EXTRA_CA_CERTS 2 failed (CA warning on stderr) passes
Full suite with the hook's exact prefix 7 files failed: 39 failed, 5,404 passed, 1 skipped 117 files passed: 5,443 passed, 1 skipped, 0 failed
Leaked integration servers after that run (control) 0 0
The real hook on /usr/bin/env HOME=… npm test as root (at 231b599) exit 0, the run goes through as root exit 2, stopped with the reply

The bug check also exercised the hook live:

  • Six concurrent runs left one whole copy.
  • The first copy took about 0.15 s, well inside the hook's 10 s limit.
  • The .nvmrc forms v24, 24.1.0 and CRLF 24 get a copy. lts/* and an empty file get none.
  • A PATH holding a folder with a space works.

No test deploy was run: .dockerignore excludes .claude and **/__tests__/, and the Dockerfile does not copy AGENTS.md, so the image built from this branch has the same contents as main's.

🤖 Generated with Claude Code

…ble CA bundle

In Claude Code's cloud containers the setpriv prefix ran the container's own
node, another major version than .nvmrc, and inherited a NODE_EXTRA_CA_CERTS
file nobody cannot read. The first fails every test that needs a newer node
API; the second adds a warning to every child process's stderr.

- When nobody's node is another major version but the session's matches
  .nvmrc, the hook copies the session's node into nobody's HOME and puts it
  first on PATH in the prefix.
- When nobody cannot read the NODE_EXTRA_CA_CERTS file, the prefix unsets it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkA1FHrosoEim4HpZyQGyQ
… node is another version

Claude Code starts hooks with its own PATH, so in a cloud container the hook's
node is the container's, not the session's nvm node. The hook now takes the
newest node of the .nvmrc major version from $NVM_DIR or ~/.nvm instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkA1FHrosoEim4HpZyQGyQ
…hole and root-owned

- setpriv keeps root's capabilities until it starts its command, so
  `setpriv … node --version` reached nvm's node under /root and reported the
  .nvmrc version even when nobody's runs (env, then npx's env shebang) get
  another. The probe now goes through /usr/bin/env, as the prefix does.
- cp makes its file executable before writing it, so a copy cut short by the
  hook's timeout, or one a concurrent hook was still writing, passed the -x
  reuse check for good. The copy now lands under a temporary name and mv
  renames it into place.
- The copy moves from nobody's HOME to node_modules/.nobody-node, a root-owned
  folder (755), so root no longer writes where nobody could plant a symlink,
  and nobody can run the copy but not replace it.

Ship-Check: pr-review · claude-opus-5-5
…which PATH each lookup uses

- Rename ci_node_* to nvmrc_node_* and hook_node_*, set the copied
  version where its path is set, and name the copies' parent folder.
- Compute node_major_differs once and use it for both the copy and the
  version warning.
- Build env's arguments as an array, so the reply's spacing needs no
  per-setting convention.
- Keep the writable-folder setup in one block before the version check.
- Reword the comments a stranger stopped on: the major-version match,
  the unset NODE_EXTRA_CA_CERTS, which PATH the probe and the prefix
  search, why a missing node lets the run through, the shebang lookup
  that makes npx and tsx use the copy, and sh's $0.

Ship-Check: code-quality · claude-opus-5-5
…s edges

- The setpriv stub now looks a bare command up on the hook's PATH, as real
  setpriv does with root's capabilities, and leaves a command named by path
  to search nobody's PATH. A probe that drops /usr/bin/env now fails the
  tests.
- New cases: a copy cut short is copied again, $NVM_DIR wins over ~/.nvm and
  falls through to it, a full .nvmrc version compares by major, the copy and
  its folders stay 0755 under umask 077, the copy's folder is quoted in the
  prefix, and the copy is never handed to nobody.
- The reuse test asserts the whole run.

Ship-Check: test-audit · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
Comment thread .claude/hooks/require-nonroot-tests.sh
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
@umm-actually

umm-actually Bot commented Oct 11, 2026

Copy link
Copy Markdown

Update the test header's stub list for the new node stub
Low severity · conventions · medium confidence

src/__tests__/require-nonroot-tests.test.ts:22 — beyond the diff's line ranges, in code the changes touch or depend on.

The header still says the hook runs 'with real node' and lists only id, setpriv and chown as stubs, but this PR adds a node stub first on PATH that intercepts the hook's node --version (delegating other calls to the real node). A reader trusting the header would assume every node call reaches the real binary.

Failure scenario: A contributor adds a hook-behavior test assuming the hook's node --version hits the real node; it silently gets the stubbed version instead, so the test passes or fails for the wrong reason.

Suggested fix
Reword the header to list the node stub and state that only `--version` is stubbed, with other calls handed to the real node.

umm-actually · z-ai/glm-5.3-flash

@umm-actually

umm-actually Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

umm-actually re-reviewed at 5707820

3 new finding(s) posted (16 tracked finding(s) across all runs).


umm-actually · deepseek/deepseek-v4.1-flash, z-ai/glm-5.3-flash

…de runs from

- The prefix printed `PATH=<copy>:$PATH` with $PATH unquoted. bash splits
  it when a folder on the session's PATH holds a space (WSL appends
  Windows folders such as "Program Files"), and env then takes the text
  after the space as its command. The prefix now prints `:"$PATH"`.
- When the hook's own node matched .nvmrc, the hook copied
  `command -v node`, which can be a version manager's shim (asdf, mise,
  volta). The copy then fails as nobody, and the -x reuse check keeps the
  broken copy. The hook now copies `node -p process.execPath`, the binary
  node runs from.
- A failed cp (a full disk) left its partial temporary file behind,
  holding the space it took. The hook now removes it before ending as
  set -e would.
- find -L enters a symlinked versions/node or version folder under nvm's
  folders.
- AGENTS.md: the prefix runs a copy of root's Node of the .nvmrc major
  only if root has one.

Ship-Check: bug-check · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/usr/bin/env in the prefix

- find exits non-zero on a folder it cannot read, and under set -e and
  pipefail that ended the hook with status 1, which lets the run through as
  root. The search now keeps the nodes it could list.
- The printed prefix names /usr/bin/env in full, for the same reason the
  probe does: setpriv searches PATH with root's capabilities.
- The test file's header lists the node stub.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

Replying to umm-actually's PR-level finding "Update the test header's stub list for the new node stub" (comment 6113553059): fixed in 203b1df. The header of require-nonroot-tests.test.ts now lists the node stub alongside id, setpriv and chown. It also says the stub answers only the version and binary-path questions the hook asks of its own node, and hands every other call, such as the payload parse, to the real node.


🔍 ship-check · pr-monitor · claude-opus-5-5


Generated by Claude Code

…etpriv cases through /usr/bin/env

- Root reads a folder whatever its mode, so the hook, which runs only as
  root, rarely meets one find cannot read. A symlink loop that -L walks into
  also makes find exit non-zero while it still lists the other nodes, so the
  comment on || true names both.
- The two test commands that stand for a run through the reply's prefix now
  use /usr/bin/env, as the reply does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ship-Check: bug-check · claude-opus-5-5
…ath, and anchor the nvm search

- The reply's prefix now names /usr/bin/env, so a run that kept only the
  env part of that prefix went through as root: the wrapper patterns
  matched env, time, timeout and the shells only by their bare names. Each
  may now be named by path.
- The nvm search matched a node of another major version when the nvm
  folder's own path held a folder such as v24.x. find now runs from inside
  versions/node, so -path sees only nvm's own folders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…et the nvm search's cd

- In a cloud container the reply's prefix carries env's -u
  NODE_EXTRA_CA_CERTS and a PATH setting of the form /copy:"$PATH". The
  wrapper read -u's name as the command env runs, and a variable setting
  took only one quoting form for its whole value, so that prefix used
  without setpriv still went through as root. env's -u and --unset now take
  their name, and a value may mix unquoted and quoted parts.
- The nvm search's cd printed its error into the reply when $NVM_DIR's
  versions/node passed the -d test but could not be entered, where find had
  dropped it. cd's error now goes to /dev/null, and CDPATH is cleared so a
  relative $NVM_DIR enters the folder the -d test checked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ship-Check: bug-check · claude-opus-5-5
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
Comment thread src/__tests__/require-nonroot-tests.test.ts
…-C and grouped -u

- In a checkout whose path holds a space, the reply's prefix writes the
  space escaped (`HOME=/my\ repo/...`). The unquoted part of a setting
  stopped at that space, so the prefix without setpriv ran as root. A
  backslash now escapes the next character.
- env's -C (--chdir) takes a folder, and -u also takes a name at the end of
  a group of flags (`env -iu NAME`). Both read their argument as the
  command, so the run after them went through.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkA1FHrosoEim4HpZyQGyQ
@aliasunder aliasunder changed the title fix(hooks): give nobody test runs the .nvmrc node and drop an unreadable CA bundle chore(hooks): give nobody test runs the .nvmrc node and drop an unreadable CA bundle Oct 11, 2026
…copy

No test set both parts of the prefix at once, though a cloud container's
reply has both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkA1FHrosoEim4HpZyQGyQ
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
@umm-actually

umm-actually Bot commented Oct 11, 2026

Copy link
Copy Markdown

Resolve cd targets with escaped spaces to the real directory
Low severity · subtle bugs · high confidence

.claude/hooks/require-nonroot-tests.sh:184 — beyond the diff's line ranges, in code the changes touch or depend on.

Pre-existing: an unquoted cd target is read only up to the first space, so the backslash-escaped spelling (cd /my; repo && npm test) resolves to the truncated folder /my; instead of the real one — the same spelling the reply's own prefix emits for a spaced checkout and that the variable-setting pattern was just extended to match. The run is still stopped, but the prepared folders and node copy land in the wrong checkout, and when the session's cwd is in no checkout both git lookups fail and the hook exits 0, letting the run through as root. The quoted form (cd ;/my repo;) is handled correctly, so only the escaped spelling is the gap.

Failure scenario: Root session with cwd outside any checkout (e.g. /root) and the command cd /spaced; repo && npm test, where /spaced repo is a checkout with node_modules: the walk resolves the cd target to /spaced;, both git rev-parse lookups fail, and the hook exits 0 — the suite runs as root. With the session's cwd inside a checkout, the same command stops the run but prepares that checkout's folders and offers a prefix whose HOME and PATH point there, so the rerun as nobody fails en masse with EACCES creating /spaced repo/node_modules/.vite-temp.

Suggested fix
Extend the two cd path atoms with the same escaped-character alternative the variable-setting grammar uses (e.g. (/([^[:space:]'\"\\\\]|\\\\.)*) and the relative equivalent) and strip the escaping backslashes before use (current_dir=${BASH_REMATCH[2]//\\/}) so git -C receives the real path; add a stop-case for `cd /spaced\\ repo && npm test` whose cwd is outside any checkout.

umm-actually · z-ai/glm-5.3-flash

An unquoted cd path stopped at its first space, escaped or not, so
`cd /my\ repo && npm test` named /my\ and, from a folder in no checkout,
let the run through as root. The cd patterns now take a backslash-escaped
character, and the walk drops the backslash as the shell does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Only env's flags that take no argument (-0, -i, -v) may come before a
  -u or -C in one group. A u or C earlier in the group takes the rest of
  the group as its argument, so the next word is the command env runs,
  and the hook stopped such runs although they run no tests.
- A -u or -C argument is now a shell word, the pattern a setting's value
  uses, so a quoted or escaped space in a -C folder no longer lets the
  run through.
- The walk does not follow env's -C folder, as it does not follow npm's
  --prefix; the comment now says so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

Re: Resolve cd targets with escaped spaces to the real directory: fixed in 592cca1. I reproduced it: cd /path/sp\ ace && npm test from a folder in no checkout exited 0, while the quoted cd '/path/sp ace' exited 2.

  • Both unquoted cd patterns now take a backslash-escaped character.
  • The walk drops the backslash, as the shell does, before handing the path to git -C.

Two new tests cover an absolute and a relative escaped path, each run from a folder in no checkout. They fail when the pattern is reverted, and also when only the unescape is removed.


Generated by Claude Code

Comment thread .claude/hooks/require-nonroot-tests.sh
Comment thread .claude/hooks/require-nonroot-tests.sh
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
… quotes

- The walk unescaped each cd path in a subshell, with a loop that rescanned
  the path per escape, so a command with thousands of cds could pass the
  hook's 10-second limit and run as root. read without -r drops the
  backslashes as a builtin.
- An escaped leading - in a relative cd path (`cd \-foo`) is a flag to
  cd, as the comment says, so the walk no longer follows it.
- A double-quoted part may hold an escaped character, so an env -u or -C
  argument or a setting's value holding \" is read whole.
- npm and npx named by path (/usr/local/bin/npm test) are runners too.
- The ~/.nvm search reads HOME with a default, so an unset HOME no longer
  ends the hook under set -u.
- A test pins that env's -C folder is not followed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The case created a folder named with the backslash, so a hook that
followed the cd fell back to the session's checkout and passed anyway.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread .claude/hooks/require-nonroot-tests.sh Outdated
Comment thread .claude/hooks/require-nonroot-tests.sh
…\/ and += forms

- A folder holding = is a setting's value, so `FOO=/x/npm test`, which
  runs the test builtin, is no longer stopped as npm named by path.
- node named by path running vitest.mjs is a runner, as npm, npx and
  vitest by path already are.
- A VAR+=value setting before the command is read as one.
- A cd path starting with \/ is absolute, not relative.
- A .nvmrc that starts with a space still names its version.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The split breaks a command at ;, &, |, ( and backticks even inside quotes,
so a commit message, an inline script or a heredoc that only mentioned a
test run produced a line starting with one, and the hook blocked a command
that runs no tests.

The node step that parses the payload now blanks those characters inside
quotes and drops heredoc bodies and comments, unless a word of the command
names bash, sh or zsh, whose quoted text and heredocs run. A $( ) inside
double quotes is still read as commands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

Closing unmerged. The root-test hook decides whether a Bash command runs vitest by reading the command text with regular expressions, and every review round found more shell forms it misreads in both directions. Instead, a follow-up PR replaces the hook with a check inside vitest itself, which runs only when vitest actually starts as root. That PR removes the hook, so this one's fixes to it won't be needed.


Generated by Claude Code

@aliasunder aliasunder closed this Oct 12, 2026
const QUOTE = "\u0027"
const SEPARATORS = ";&|(`\n"
const SHELL_NAME = /(^|\/)(bash|sh|zsh)$/
const HEREDOC_OPERATOR = /^<<(-?)[ \t]*(?:"([^"]*)"|\u0027([^\u0027]*)\u0027|([^\s;&|()<>]+))/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treat <<< as a here-string, not a heredoc, in the command masker
Low severity · subtle bugs · high confidence

The new masker's HEREDOC_OPERATOR matches << without checking for a third <, so a here-string like grep x <<< 'nothing' is parsed as a heredoc whose delimiter is the quoted text; at the next newline the body reader then swallows every following line searching for the never-appearing delimiter. A test run on a later line is thereby dropped from the masker's output, changing the outcome from main's stop to a silent run as root. The same <<< blind spot also leaves a here-string fed to a shell (bash <<< 'npm test') unrecognized by the wrapper patterns.

Failure scenario: In a root cloud session the command grep -q x <<< 'nothing' followed by npm test on the next line: the masker consumes << 'nothing' as a heredoc operator, readHeredocBodies then drops the npm test line while searching for the delimiter 'nothing', the walk sees no run, and the hook exits 0 so vitest runs as root — on main the same command was stopped.

Suggested fix
Make HEREDOC_OPERATOR require that the `<<` is not part of `<<<` (e.g. check command[index+2] !== "<" before accepting the match, or add a negative check on the preceding character), and add a stop-case whose command is a here-string line followed by `npm test` on the next line.

umm-actually · z-ai/glm-5.3-flash

shell_wrapper="(bash|sh|zsh)${shell_flags}[[:space:]]+([\"'][[:space:]]*)?"
wrapper="((env|time)${flags_without_argument}|exec|command|${timeout_wrapper})[[:space:]]+|${shell_wrapper}"
shell_wrapper="${optional_command_folder}(bash|sh|zsh)${shell_flags}[[:space:]]+([\"'][[:space:]]*)?"
wrapper="(${optional_command_folder}env${env_flags}|${optional_command_folder}time${flags_without_argument}|exec|command|${timeout_wrapper})[[:space:]]+|${shell_wrapper}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stop test runs through eval like the other command wrappers
Low severity · correctness · high confidence

Pre-existing: the wrapper list covers env, time, exec, command, timeout and the shells, but not eval, so an eval'd test run is never recognized and the hook exits 0. exec and command are handled as sibling builtins that run a command, so eval is the asymmetry in the same family.

Failure scenario: In a root cloud session the command eval 'npm test' (or eval npm test): eval is not a wrapper, so no command_prefix matches, test_run fails, the hook exits 0 with no reply, and vitest runs as root with the root-only permission-test failures the hook exists to prevent.

Suggested fix
Add `eval` to the wrapper alternation with no flags (eval takes none), i.e. `|eval` beside `exec|command`, and add a stop-case for `eval 'npm test'`.

umm-actually · z-ai/glm-5.3-flash

Comment on lines +509 to +510
{ label: "a bash heredoc", command: "bash <<'EOF'\nnpm test\nEOF" },
{ label: "a line after a cat heredoc", command: "cat <<EOF >notes\nhello\nEOF\nnpm test" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for the heredoc reader's tab-stripping branch
Low severity · tests · high confidence

The new stripsTabs branch of readHeredocBodies (for <<- heredocs) has no test: every heredoc case in the stop and pass tables uses << or <<'EOF'. If the branch regresses, a tab-indented terminator never matches its delimiter and the reader consumes the rest of the command into the blanked body, so any test run after the heredoc is silently let through with no failing test.

Failure scenario: A regression flips stripsTabs (e.g. tabsFlag === false for <<-): for cat <<-EOF newline notes newline «tab»EOF newline npm test, the delimiter line «tab»EOF never equals EOF, readHeredocBodies swallows every remaining line including npm test, the hook exits 0, and the suite runs as root — no existing test fails because none exercises <<-.

Suggested fix
Add a stop case `cat <<-EOF` newline `notes` newline tab-`EOF` newline `npm test` (expected: the exact refusal) and, if the tab-indented body of a bash heredoc matters, a matching pass/stop pair — the delimiter-line handling under <<- is currently unexercised by all 159 tests.

umm-actually · z-ai/glm-5.3-flash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants