Reject symlinks during skill scans

[cbullinger]

Summary

• Reject symlink bundle roots, recursive entries, directory symlinks, and explicitly supplied symlink files.
• Require scan inputs to be regular files.
• Use no-follow file opens on macOS and Linux, with regular-file validation after opening.
• Document that symlinks are unsupported and add regression coverage.
• Windows reads use  FILE_FLAG_OPEN_REPARSE_POINT  with regular-file validation.
• Markdown extension filtering happens before metadata inspection.
• golang.org/x/sys  was added for Windows filesystem protection.

Security impact

This prevents skill-gate from following PR-controlled symlinks to runner-readable files and sending their contents to the LLM judge or reports.

Validation

• go test ./...
• go test -race ./scanner
• Windows scanner test compilation

The agent-skills workflow pin remains at v0.1.0 until this fix is merged and a patched version is released, then it can be bumped immutably.

[Copilot]

🟡 Changes recommended

The non-(darwin|linux) reader still follows symlinks at open time (leaving a TOCTOU gap on Windows in CI), and the directory walk now stats/validates non-markdown files before filtering, potentially causing unnecessary failures/perf regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the scanner package against symlink-based path escapes during bundle scans by rejecting symlink inputs/entries and tightening file-read behavior to only allow regular files, with documentation and regression tests to match.

Changes:

• Reject symlink bundle roots, symlink entries during recursive walks, and non-regular scan inputs/entries.
• Replace direct os.ReadFile usage with a readRegularFile helper that enforces regular-file validation (and uses O_NOFOLLOW where available).
• Add README documentation and new tests covering symlink rejection cases.

File summaries

File	Description
scanner/scanner.go	Switches file reads to readRegularFile and rejects symlink/non-regular paths during scan enumeration.
scanner/scanner_test.go	Adds regression tests ensuring symlink roots/entries are rejected.
scanner/read_unix.go	Implements no-follow opens + post-open regular-file validation for darwin/linux.
scanner/read_other.go	Fallback reader for other platforms (currently plain open + post-open regular-file validation).
README.md	Documents that symlinks are unsupported and that scanned inputs must be regular files.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cbullinger]

addresses HELP-99354

[Copilot]

🟡 Changes recommended

The new symlink regression tests may be unreliable on Windows CI where symlink creation can be permission-restricted, and should skip gracefully on permission errors to avoid flaky failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

scanner/scanner.go:235

• The error wrapper still says "stat" even though this code now uses os.Lstat; that can confuse callers/debugging (stat vs lstat behave differently with symlinks).

scanner/scanner_test.go:257

• On platforms where creating symlinks is not permitted, this test will fail early during setup. Skipping on permission errors helps keep the suite reliable (especially on Windows CI).

	if err := os.Symlink(filepath.Dir(target), rootLink); err != nil {
		t.Fatal(err)
	}

scanner/scanner_test.go:261

• Same as above: consider skipping if symlink creation is blocked by permissions so the test suite doesn't fail for environmental reasons.

	if err := os.Symlink(target, fileLink); err != nil {
		t.Fatal(err)
	}

• Files reviewed: 7/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces security-sensitive, platform-specific filesystem behavior that should be validated by a human reviewer across target OSes.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scanner/read_unix.go:27

• Using the deprecated syscall package here makes the implementation less maintainable; since this PR already adds golang.org/x/sys, prefer golang.org/x/sys/unix for Open/Close and flag constants.

• Files reviewed: 7/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The current Lstat-based root/file checks can be bypassed when an explicit scan path contains a symlink in a non-final path component (e.g. symlinkDir/real.md), which can still allow scanning outside the intended bundle.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scanner/scanner_test.go:196

• In TestScanRejectsSymlinks, the table-driven make functions close over the outer test's t and are invoked from subtests. This can misattribute failures/skips (and t.TempDir() usage) to the parent test rather than the specific subtest. Prefer passing the subtest t into make (e.g., make func(*testing.T, string) error) and using that within each case.

• Files reviewed: 7/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The current symlink-component validation is applied to the raw input string and can miss symlink components introduced via relative-path resolution, weakening the intended security guarantee for relative bundle paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

scanner/scanner.go:232

• rejectSymlinkComponents is called on the user-supplied path string, which means a relative bundle path does not validate symlink components in the current working directory prefix (since those components are not part of path). This weakens the “reject symlink bundle roots/components” guarantee for callers that pass relative paths (e.g. the CLI). Consider validating symlinks against an absolute version of the path before proceeding.

func markdownFiles(path string) ([]scanFile, error) {
	if err := rejectSymlinkComponents(path); err != nil {
		return nil, err
	}

• Files reviewed: 7/8 changed files
• Comments generated: 1
• Review effort level: Lite

[krollins-mdb]

Thanks for the quick turnaround, @cbullinger! LGTM.

[Copilot]

🔵 Needs a closer look

The directory-walk metadata path uses d.Info() (stat-following) which can still dereference symlinks under TOCTOU, weakening the stated “never follow symlinks” guarantee.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scanner/scanner.go:266

• d.Info() follows symlinks (it uses os.Stat), so metadata inspection can still dereference a symlink if the entry is swapped between the d.Type() check and the Info() call. Use os.Lstat(p) here to avoid ever following symlinks during the walk.

• Files reviewed: 7/8 changed files
• Comments generated: 0 new
• Review effort level: Lite