fix: prevent report output symlink races

[yashrajp22]

A report destination can change after an extension or CLI preflight check, allowing a later path-based write to overwrite an unrelated file. CLI reports, baselines, and batch reports now use descriptor-relative atomic replacement. Each parent is opened without following symlinks, a private temporary file is created, and the destination is replaced through the opened parent descriptor. Symlinks and special files are rejected; hard links are replaced without modifying the linked file.

Unsupported file output fails before scan work begins. Reports and baselines support stdout, including pure baseline JSON with -o -. Errors identify the requested path and invalid parent. Report files, including replacements, and new baselines use private 0600 permissions. Existing baselines keep their ownership, permissions, and access ACLs when safely replaced. Updates that would require writing into an existing shared file are refused without changing it; use a new file or -o - instead. Baseline JSON retains Unicode and loader limits on stdout too. The Docker example documents UID/GID ownership.

Validation on the updated branch: 529 tests passed against source and a freshly installed wheel; five macOS-specific ACL tests were skipped in the Linux container. Cases cover leaf/parent races, hard links, baseline metadata and Unicode, bounded concurrent writers, cleanup, missing parents, unsupported APIs, narrow terminals, batch output, and single/recursive/registry/baseline commands. Verification was offline.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for routing every CLI report and baseline write through a single descriptor-relative writer! The no-follow parent walk, the O_EXCL temp file and the dir_fd replace close the leaf-swap, parent-swap and hard-link cases together, and the parametrised race test exercises each of them deterministically.

Value and readiness: The problem is real on main. With main's own code, _write_result and dump_baseline writing through a leaf symlink overwrote an external sentinel, a hard-linked destination was modified in place, and scan --mcp-registry and scan --recursive --format json with a symlinked --output exited 0 and overwrote the sentinel. The most realistic case also reproduces: skillspector baseline . --no-llm inside a checkout that ships .skillspector-baseline.yaml -> ../outside/victim_rc (the default -o path) exited 0 and overwrote victim_rc. I transcribed file_output.py:27-85 and the changed call sites and ran them with main's CLI on macOS. Every one of these cases is now refused or contained, report bytes are unchanged, and the planted-baseline checkout exits 2 with victim_rc intact. All seven write sites are covered and the exit-2 contract on write errors is kept. One blocker remains: on Windows, which the project supports, skillspector baseline stops working entirely, its error points to stdout redirection that baseline does not offer, and both baseline and scan --output fail only after the full scan has run (finding 1). Findings 2-8 are non-blocking; 2-4 (error messages, file mode and newly refused targets) and 5 (tests for the baseline, recursive and registry writers) are the most useful to address here. Once finding 1 is resolved, the PR should be ready for final maintainer review.

Material findings

1. [Blocker] src/skillspector/file_output.py:42: on Windows, baseline becomes unusable and every --output write fails only after the scan, with a hint that baseline cannot follow.
   • Windows CPython has no os.O_NOFOLLOW or os.O_DIRECTORY and an empty os.supports_dir_fd (per the CPython docs; not run on Windows). So _SECURE_OUTPUT_SUPPORTED (file_output.py:27-32) is False, and lines 42-45 raise "Safe file output is unsupported on this platform; use stdout redirection." Every writer reaches it: cli.py:363, 697, 3236, 3264, 3297 and suppression.py:650/656.
   • baseline --output is a plain Path defaulting to .skillspector-baseline.yaml (cli.py:3373-3380), with no stdout mode. graph.invoke (cli.py:3420, LLM analysis on by default) runs before dump_baseline (cli.py:3433), and the error becomes exit 2 at cli.py:3439-3441. scan calls _write_result at cli.py:857, after scan_skill. Nothing checks platform support before the scan; the only pre-scan output check is the samefile guard at cli.py:717-727.
   • Measured with main's CLI in-process, the writers replaced by a transcription of the PR code and the gate forced False: baseline <skill> --no-llm -o b.yaml, baseline with the default path, and scan --no-llm -f json --output r.json each ran the graph once, then exited 2 with that message and wrote nothing. main exits 0 and writes the file in all three cases. With the gate True, the transcription succeeds.
   • Windows is a supported target: pyproject.toml:24 declares "OS Independent", input_handler.py:314-315 routes Windows reads to a reparse-point-safe _open_regular_file_from_windows_handle instead of refusing, the 2.12.0 notes list Windows fixes (#484, #503, #518), and both extensions have win32 branches and pass --output (extensions/skillspector.ts:112, .opencode/tools/skillspector_scan_lib.ts:216). CI is ubuntu-only, so nothing catches this. tests/unit/test_suppression.py:713/736 and about 33 pre-existing --output/-o invocations in test_cli.py would fail on Windows.
   • Consequence: the PR body discloses the fail-closed scan --output behaviour, and stdout is a workable fallback there (an agent can re-run the extension tool without output). baseline has no workaround: a documented command (README.md:239, docs/SUPPRESSION.md:37) disappears on Windows, the message suggests something impossible, the scan and any LLM cost are spent before the failure, and nothing in the user docs says so.
   • Expected fix (minimal, keeps failing closed):
     • Expose a support check from file_output and call it before any analysis when an output file is requested: in scan next to the pre-scan check at cli.py:717-727 (this covers single-skill, --recursive and --mcp-registry) and in baseline before graph.invoke. Exit 2 with a command-specific message.
     • Give baseline a stdout mode (for example -o -), or at least stop suggesting stdout redirection for it.
     • Add baseline and recursive unsupported-platform tests that assert no scan ran and no file was written, and skip or monkeypatch the existing file-output tests on unsupported platforms.
     • Document the limitation in the README --output section and docs/SUPPRESSION.md.
     • Alternatives for the PIC: keep main's plain write with a warning on unsupported platforms (no worse than main), or add a native Windows writer as a follow-up. A Windows writer that checks the parent by handle and then calls os.replace by path is not race-free, because the rename resolves the path again. It would need to hold each ancestor handle open without FILE_SHARE_DELETE, or rename relative to the parent handle (SetFileInformationByHandle with FileRenameInfo).
2. [Non-blocking] src/skillspector/file_output.py:52: an output path through a symlinked directory now fails after the scan with "[Errno 20] Not a directory: ''".
   • Only root-owned aliases directly under / are normalised (file_output.py:46, via input_handler.py:251-262). Every other ancestor is opened with O_DIRECTORY|O_NOFOLLOW at lines 51-54 with no error mapping, whereas input_handler.py:340-344 maps the same errors to messages that carry the full path.
   • Measured (transcription in main's CLI, macOS): scan <skill> --no-llm -f json -o <abs>/plink/r.json and baseline -o <abs>/plink/b.yaml, with plink a user-owned symlink to a directory, both exit 2 with "Error: [Errno 20] Not a directory: 'plink'" and write nothing; main exits 0. A relative plink/r2.json behaves the same. A missing parent already failed on main (exit 2, naming missingdir/r.json); the PR names only missingdir.
   • Scope: only paths that spell out a symlinked component are affected, such as "$PWD/..." or ~/link/... when those cross a symlink. A relative path from a cwd reached through a symlink still works, because os.path.abspath uses the physical cwd. The Pi and OpenCode extensions are not affected. Linux was not run; the same walk on the input side suggests ENOTDIR or ELOOP there.
   • Consequence: refusing symlinked parents is intended (the parent-before-open test expects it), but the error calls a directory "not a directory", names a single component, and the finished report is lost. The PR body's "Explicit regular output paths outside the workspace remain supported" does not hold for these paths.
   • Expected fix: catch OSError around the walk. Map ELOOP/ENOTDIR to a message naming the full output path (for example "Refusing to write output through a symlinked directory: ") and FileNotFoundError to "Output directory does not exist: ". Optionally run a cheap no-follow pre-check before scanning, keeping the write-time walk as the real guard. Document the refusal and correct the PR body.
3. [Non-blocking] src/skillspector/file_output.py:66: reports and baselines are always written 0600, including rewrites of existing 0644 files.
   • The temp file is created with mode 0o600 (lines 63-68) and os.replace installs that inode; nothing restores the mode.
   • Measured (transcription vs main's dump_baseline): under umask 022, a new file is 0644 on main and 0600 with the PR. An existing 0664 file stays 0664 on main (inode kept) and becomes 0600 with the PR; because it is a new inode, owner, group, ACLs and xattrs also reset. Under umask 002 the PR still gives 0600.
   • The README Docker example (README.md:116-121) runs as root, since the Dockerfile has no USER. On rootful Linux Docker the bind-mounted report.json therefore becomes root:root 0600 and the host user cannot read it; on main it is 0644. This is inferred from Docker semantics and was not run; Docker Desktop and rootless setups are unaffected. Git is unaffected (a 0600 file is still stored as 100644), so the shared-baseline impact is limited to other uids reading the same filesystem copy, such as multi-user hosts or mixed-uid CI.
   • Expected fix, either:
     • Create the temp file with 0o666 and let the kernel apply the umask (no os.umask call needed), and call os.fchmod(fd, stat.S_IMODE(existing.st_mode)) when replacing an existing regular file. Update the 0600 assertion at tests/unit/test_cli.py:7109. The O_EXCL and no-follow protections are unchanged.
     • Or keep 0600 as deliberate policy, document it, and add --user "$(id -u):$(id -g)" to the Docker --output example.
4. [Non-blocking] src/skillspector/file_output.py:60: some previously working targets are now refused, and the refusal messages do not name the output path.
   • Lines 60-61 reject any non-regular leaf with "Refusing to overwrite a non-regular output file." (no path, no file type). Lines 62-77 always create a temp file in the parent, so the parent must be writable. macOS has no O_PATH, so line 47 opens every ancestor, the parent included, with O_RDONLY, which needs read permission.
   • Measured (main's real CLI vs main's CLI with a transcribed _write_result, macOS):
     • -o /dev/null: exit 0 on main; exit 2 with the message above on the PR.
     • An existing writable file in a 0555 directory: exit 0 on main; exit 2 with "Permission denied: '.skillspector-output-'" on the PR.
     • A new file under an ancestor at mode 0311: exit 0 on main; exit 2 with "Permission denied: 'outer'" on the PR. This case is macOS/BSD only; Linux uses O_PATH, which needs search permission only.
     • -o /dev/stdout is negligible: main already rejects it with exit 2 when stdout is a file or pipe, and accepts it only on a tty.
   • Consequence: -o /dev/null as an exit-code-only idiom, pre-provisioned output files in read-only directories and restrictive ancestors on macOS now fail, with messages naming a temp file or a single path component. No in-repo caller uses these targets, and the PR body mentions the special-file refusal.
   • Expected fix: include the requested path and file type in the non-regular message, and re-raise write OSErrors with the user's output path. Decide whether to allow character devices (open the existing leaf with O_WRONLY|O_NOFOLLOW when it is S_ISCHR) or document the refusal. Document that the parent must be writable. On macOS, os.O_SEARCH may remove the read requirement on ancestors (it exists on this Python 3.13; not tested as a dirfd here).
5. [Non-blocking] tests/unit/test_cli.py:7138: no regression test covers the baseline, --recursive or --mcp-registry writers.

   • The tests at test_cli.py:7067 and 7115 call write_text_no_follow directly, and both would catch a naive Path.write_text helper. The test at 7138 drives single-skill scan on the unsupported branch, which does protect cli.py:363. Nothing exercises cli.py:697, 3236, 3264, 3297 or suppression.py:650/656.

   • main's tests/unit/test_cli.py, tests/unit/test_suppression.py and tests/nodes/analyzers/test_sc2_command_boundaries.py pass on main (582 passed), where all of these writers are still Path.write_text. Reverting any of them would therefore keep CI green, although each is exploitable: with main's CLI, baseline . in a checkout with a planted .skillspector-baseline.yaml symlink, --mcp-registry --output <symlink> and --recursive --format json --output <symlink> each exit 0 and overwrite the sentinel. The PR body says the baseline CLI race was checked, but no test in the diff exercises a baseline race.

   • Expected fix: one POSIX-only CLI test parametrised over:

     • baseline <skill> --no-llm after chdir into a directory whose .skillspector-baseline.yaml is a relative symlink to a sentinel, with graph.invoke stubbed as at test_cli.py:6988;
     • baseline -o out.json where out.json is a symlink;
     • scan <root> --recursive --format json|sarif|markdown --output <symlink>, with detect_skills and _scan_skill stubbed as at test_cli.py:3672;
     • scan registry.json --mcp-registry --format json --output <symlink>.

     Each case should assert exit 2, a "non-regular" message and an unchanged sentinel. The supported success path is already covered by existing tests (test_cli.py:443, 6988, test_cli_mcp_registry_routes_and_writes_json).

6. [Non-blocking] README.md:760: the user-visible changes are not documented.
   • The PR changes no docs. README.md:760 ("-o, --output PATH Output file path"), the --output help at cli.py:472 and cli.py:3378, the Docker example at README.md:116-121, README.md:239/769 and docs/SUPPRESSION.md:24/37 do not mention the refused targets (Windows, symlinked parents, special files, non-writable parents) or the 0600 mode.
   • Your earlier merged security PRs (#736, #737, #744, #747) updated README or docs for their user-visible changes. A CHANGELOG line is optional; only 2 of 86 first-parent commits since 2.12.0 touched it.
   • Expected fix: short notes in the README CLI-options and Docker sections and in docs/SUPPRESSION.md, reflecting whatever is decided for findings 1-4.
7. [Non-blocking] contrib/batch_scan/batch_scan.py:525: the batch scanner's documented -o still writes with Path.write_text.
   • batch_scan.py:524-525 is unchanged, and the usage is documented at README.md:178. With main's module (identical on the head), -o report.json run inside a directory where report.json is a symlink overwrote the symlink's target with the batch report, and -o hard.json wrote through a hard link.
   • This is not a regression: the PR body scopes the change to CLI writers, contrib ships outside the wheel, and the attacker must control the output directory. It is the same bug class, though, and the PR's one-line summary is unqualified.
   • Expected fix: route that write through write_text_no_follow, catching ValueError/OSError and exiting non-zero as the CLI does, or state in the PR description that contrib writers are out of scope. A follow-up PR is fine.
8. [Non-blocking] tests/unit/test_cli.py:7151: the assertion can fail when Rich wraps the error line.
   • The printed line "Error: Safe file output is unsupported on this platform; use stdout redirection." is exactly 80 characters and goes through err_console = Console(stderr=True) (cli.py:118). With COLUMNS=79, a transcription of the print and the assertion wraps to "use stdout \nredirection." and fails, while "unsupported on this platform" still matches.
   • main's test_cli.py passes at COLUMNS=78 and 79 and already fails at 76 and below, so this only adds failures at 77-79. Default CI (no COLUMNS, no tty) uses width 80.
   • Expected fix: assert on "unsupported on this platform", or normalise whitespace before matching.

PIC tradeoffs:

• Windows output (finding 1). The options are: fail closed before the scan and give baseline a stdout mode; keep main's plain write with a warning on Windows (usable, but the race stays open there); or build a handle-pinned Windows writer (parity, but non-trivial and not exercised by the Linux-only CI). input_handler already has a Windows read path that a writer could mirror.
• Rename-based replace. It protects hard-linked targets and makes output atomic, but it does not preserve mode, owner, ACLs or xattrs, it needs a writable parent, and it presumably cannot replace a single-file bind mount (EBUSY on Linux; known kernel behaviour, not measured).
• Symlinked parent directories. Refusing them matches the input-path policy (validate_local_input_path) and keeps the parent-swap guarantee simple, at the cost of users whose output paths cross symlinked directories. That is acceptable with a clear error and documentation (finding 2).
• 0600 vs umask. Private output matches the Pi extension's existing report policy, but it breaks readability in the documented rootful-Docker flow and for other uids on the same host (finding 3).
• Special files. Refusing /dev/null is simplest; allowing character devices keeps an existing idiom (finding 4).
• When to check. Write-time checks keep the race window minimal. A cheap pre-scan check (platform support, parent walk, leaf type) would avoid discarding a long or LLM-backed scan, at the cost of one extra directory walk; the write-time walk must remain the real guard.

Verification and gaps:

• Baseline on main (main's own code): leaf symlinks through _write_result and dump_baseline overwrote the sentinel; a hard-linked destination was modified in place; --mcp-registry and --recursive --format json through a symlinked --output exited 0 and overwrote the sentinel; the planted .skillspector-baseline.yaml checkout exited 0 and overwrote victim_rc. The batch scanner's -o overwrote both a symlink target and a hard-linked file.
• PR behaviour (my own transcriptions of file_output.py:27-85 and the changed call sites, run with main's venv on macOS / Python 3.13; nothing was imported from the PR):
  • A leaf symlink, a FIFO and a directory at the destination are rejected.
  • A leaf swapped to a symlink just before the replace has only the link entry replaced, and the sentinel is untouched.
  • A parent swapped after open sends the write into the originally opened directory.
  • A hard-linked destination is replaced without modifying the linked file.
  • A failed replace leaves no .skillspector-output-* file.
  • Output bytes match Path.write_text for \n, \r\n and non-ASCII content. Relative paths and the /tmp//var root aliases still work.
  • The planted-baseline checkout now exits 2 with victim_rc intact.
• Gate and errors: the gate is True on darwin / Python 3.13.14 (O_PATH absent, so O_RDONLY is used). Checking os.rename in os.supports_dir_fd is the right proxy for os.replace with dir_fd. The new ValueError/OSError cases reach the existing handlers and exit 2 without a traceback (cli.py:879-888, 707-709, 783-791, 3439-3447). There is no import cycle.
• Tests: the three new tests fail on main, where skillspector.file_output does not exist, and the two helper tests would also fail against a naive Path.write_text helper. The pre-existing tests pass with either implementation of the registry, recursive and baseline writers (finding 5).
• CI: no checks have run. The workflow run on a8c13b6 is action_required (waiting for a maintainer), so the PR description's test counts are unverified, and CI must pass before merge.
• Conflicts: mergeable with main. The PR shares cli.py, suppression.py and tests/unit/test_cli.py with open PR #792 and merges cleanly with it. #792 changes cache-key derivation, _bounded_cache_update and suppression.py 172-206, while this PR touches only the write sites and suppression.py 646-656, so I found no semantic interplay. No trial merge was run.
• Head update: I reviewed a8c13b62eff70de8bd40b180ceba72cf53a790c9. The current head 8b500a1e99738b6bbc539f5c7c024ef5b95ee73b only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Gaps: Linux was not run locally (the O_PATH walk is inferred from the identical, CI-tested walk in input_handler). Windows was not run (the gate value comes from the CPython docs, and the failure was simulated by forcing the gate False). Docker was not built or run, so finding 3's root-owned file is inferred. Local Python is 3.13; CI uses 3.12.

---

Decision: Changes Requested (reviewed head a8c13b62eff70de8bd40b180ceba72cf53a790c9; current head 8b500a1e99738b6bbc539f5c7c024ef5b95ee73b only adds merges of main)