fix: TOCTOU race in resolveWorkspacePath allows symlink-based path traversal

[Sunil56224972]

Summary

Security Fix: TOCTOU (Time-of-Check-to-Time-of-Use) race condition in resolveWorkspacePath() (internal/tool/filereader.go) allows symlink-based path traversal to read arbitrary files outside the repository.

Root Cause

When filepath.EvalSymlinks(fullPath) returns os.IsNotExist, the function returns the unresolved fullPath directly:

resolvedPath, err := filepath.EvalSymlinks(fullPath)
if err != nil {
    if os.IsNotExist(err) {
        return fullPath, nil  // ← returned without symlink validation
    }
}

The caller (readFromDisk / readLinesFromDisk) then calls os.ReadFile(fullPath) or os.Open(fullPath). Between the EvalSymlinks check and the actual file read, an attacker can:

1. Create a symlink at fullPath pointing to a sensitive file outside the repo (e.g., /etc/shadow, ~/.ssh/id_rsa)
2. The symlink itself lives inside the repo (passes WithinBase), but its target is outside
3. os.ReadFile follows the symlink and reads the target — no WithinBase re-check happens

Impact

An attacker with the ability to create symlinks within the repository (e.g., via a malicious commit in a PR being reviewed) could cause OCR to read and leak arbitrary file contents from the host system into the LLM conversation. This is particularly dangerous in CI/CD environments where the runner may have access to secrets, SSH keys, or cloud credentials.

Fix

Remove the os.IsNotExist special-case that returns the unvalidated path. Instead, propagate the error to the caller. All callers (readFromDisk, readLinesFromDisk) already handle file-not-found errors gracefully by returning an error message to the LLM, so no behavioral change for legitimate use cases.

Attack Scenario

1. Attacker creates PR with file: malicious/exploit → (normal file)
2. OCR starts reviewing, LLM requests file_read("malicious/exploit")
3. resolveWorkspacePath → EvalSymlinks finds the file, all checks pass ✓
4. Between reviews, attacker replaces malicious/exploit with symlink → /etc/shadow
5. LLM requests file_read("malicious/exploit") again
6. resolveWorkspacePath → EvalSymlinks fails (IsNotExist for intermediate dir)
7. Old code: returns fullPath → os.ReadFile follows symlink → LEAK
8. New code: returns error → safe

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[github-actions]

This is a security-critical change to resolveWorkspacePath, but the test file (filereader_test.go) has no tests covering this function's behavior. Consider adding tests that verify:

1. Non-existent files now return an error (the changed behavior)
2. Symlinks pointing outside the repo are rejected
3. Valid files within the repo still resolve correctly
4. Path traversal attempts (e.g., ../../etc/passwd) are rejected

Without tests, future refactors could accidentally reintroduce the TOCTOU vulnerability.

[lizhengfeng101]

Thanks for the contribution! The security concern is thoughtful, but I have a few reservations about this change:

Behavior regression for the common case

Before this patch, when a user (or the LLM) requests a file that simply does not exist, the error path is:

1. resolveWorkspacePath returns the path (no error)
2. os.ReadFile fails → caller returns "read file \"foo\": no such file or directory"

After this patch, the error becomes:

"resolve file \"foo\": lstat ...: no such file or directory"

The original message is semantically clear ("read failed because the file does not exist"), while the new one leaks an implementation detail about path resolution. For the LLM consumer of these errors, the former is more understandable.

On the TOCTOU concern

If the file truly does not exist (not even as a symlink), os.ReadFile will also fail — there is no information leak. The theoretical TOCTOU window requires an attacker to create a symlink between EvalSymlinks and ReadFile on the same machine in real time, which is not a realistic threat in a code review workflow (files come from a git checkout, not a live attacker-controlled filesystem).

Suggestion

If you would still like to harden this path, a better approach might be using os.Lstat (which does not follow symlinks) to distinguish between "nothing exists here" (safe — let the caller report a clean file-not-found) vs. "a broken symlink exists here" (reject). But honestly, the original behavior is fine for the actual threat model of this tool.

I would lean toward not merging this as-is due to the UX regression for a theoretical concern. Happy to discuss further!

[Sunil56224972]

Thanks for the thoughtful review! You raise valid points:

On the UX regression — You're right. The original error message (read file 'foo': no such file or directory) is cleaner and more meaningful for the LLM consumer than leaking lstat internals. That's a real concern.

On the threat model — Fair point that in a typical code review workflow, files come from a git checkout and the TOCTOU window is extremely narrow. The scenario requires an attacker with real-time filesystem access during the review, which is not a realistic attack surface for most deployments.

Your Lstat suggestion is better — Distinguishing 'nothing exists' (safe, let caller report clean file-not-found) from 'broken symlink exists' (reject) preserves the original UX while still hardening the edge case. I agree this is the right approach if the team wants to harden it later.

Given your analysis, I'm okay with this PR being closed. The original behavior is indeed fine for the actual threat model. Thanks for explaining the reasoning — learned something about balancing security hardening vs UX in LLM-facing tooling! 🙏