fix(runtime): stop the scan output ancestry walk at root-owned parents

[rohanpoudel2]

Fixes #212

Problem

Two checks walk every parent up to the filesystem root and reject any that is group- or world-writable without the sticky bit.

requireSecureOutputAncestry in src/runtime.ts:

requireTrustedOutputAncestor(metadata, current, effectiveUid);
const parent = dirname(current);
if (parent === current) return;
current = parent;

and require_canonical_scan_directory in _bundled_plugin/scripts/workbench_db.py:

for parent in scan_dir.parents:

Path.parents ends at /, and the Node walk terminates on dirname(current) === current, which is also /. Every absolute path contains /, so on a host where / is mode 0777 without the sticky bit there is no output directory that can pass. The reporter's host is a shared Linux box where / is drwxrwxrwx root root by machine policy, with no root access to change it. 0.1.4 worked; 0.1.5 fails at the Node prepare step and, if that is bypassed, again at the Python save step. There is no opt-out.

Change

Stop the walk once it reaches a root-owned parent, after checking that parent. Keep the full walk when the process itself is root.

The threat model is another unprivileged user replacing scan output through a shared parent, and that requires a parent the attacker can write to. A root-owned directory is not one an unprivileged user can create entries in, and it is not one they can repair either — so refusing to run leaves the host exactly as exposed while making the tool unusable. When the process is root the strict walk is kept, because root can repair the mode.

This is the second option the issue proposes. I did not take the first (skip root-owned ancestors) or the third (an env-var opt-out) — see below.

What is still rejected

The change deliberately checks the root-owned parent before stopping, so the guarantee that matters is intact:

• A world-writable root is still rejected when output is placed directly in it (/results), because that parent is where an attacker can actually rename the entry.
• A world-writable parent the user owns is still rejected wherever it appears in the chain.
• A sticky world-writable parent is still accepted, unchanged — that is the /tmp case the sticky rule exists for.
• requireTrustedOutputAncestor is untouched, so a parent owned by a third user is still rejected.

An attacker who does exploit a world-writable / to interpose a tree — rename /home, recreate it, rebuild the path — still fails the existing trusted-owner check, because every directory they create is owned by them and only root can chown to root. The residual exposure is the time-of-check/time-of-use window, which is identical to the one already accepted for sticky shared parents and is not something walking to / closes.

Why not skip root-owned ancestors entirely

The issue's first option is to skip ancestors owned by uid 0. That would accept /results on the reporter's host, where any user can rename /results outright. Root ownership does not help when the mode grants world write, so the offending directory has to stay checked when it is the one holding the output. Checking it and then stopping keeps that case rejected.

Why not an env-var opt-out

The issue's third option is an opt-out with a documented security note. I looked for precedent and there is none: every CODEX_SECURITY_* variable in src/ and _bundled_plugin/scripts/ configures paths, registries or notices, and not one disables a security check. Adding the first such switch is a product decision about a footgun, and it also does not remove the need for this fix — a user on the reporter's host would have to disable the entire ancestry check, including the parts that were correctly protecting them, to get any scan to run. Bounding the walk keeps the protection and needs no new surface.

Verification

Both checks are covered, and the reported host cannot be reproduced without root, so the ancestry is simulated in each case rather than depending on the mode CI gives /.

runtime.test.ts — a new test mocks node:fs/promises (the pattern the file already uses for link) with a synthetic chain: / root-owned 0777 non-sticky, /tmp root-owned sticky, and private user-owned directories below. It asserts four things: the walk resolves for output under /tmp, still rejects /results, still rejects a user-owned world-writable parent, and still rejects when run as root.

workbench-canonical-paths.test.ts — a new test drives the real Python function through the existing runPythonProbe harness, patching Path.lstat to simulate the same chain, and asserts it is accepted when the stopping parent is root-owned and sticky but still rejected when that parent is user-owned and world-writable.

With the fixes reverted, each new test fails with the exact message from the issue:

runtime.test.ts:          Expected promise that resolves / Received promise that rejected
workbench-canonical-paths.test.ts:
  + "error": "Scan output parent must not be group- or world-writable without the sticky bit."

With the fixes: runtime.test.ts 80 pass / 1 skip / 0 fail, workbench-canonical-paths.test.ts 4 pass / 2 skip / 0 fail.

Full suite on this base: 728 pass / 5 skip / 0 fail. pnpm run types and pnpm run format are clean, and workbench_db.py compiles under python3 -m py_compile.

[mldangelo-oai]

@codex review exact head 448769f

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 448769fa8f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep checking parents above root-owned directories

For a non-root process whose output is below a root-owned directory that itself sits in a non-sticky writable parent (including the motivating case of / being 0777), this return skips that writable parent. Directory ownership does not prevent another user with write permission on its parent from renaming or replacing it, so an attacker can race the subsequent path-based output operations after this check. The matching break in _bundled_plugin/scripts/workbench_db.py:3531 creates the same gap in the repeated artifact checks; both walks must inspect the containing parent rather than treating root ownership alone as a safe boundary.

Useful? React with 👍 / 👎.