Skip to content

fix: prevent path traversal in ClaudeAPIClient._read_file()#79

Open
qinlongli2024-ai wants to merge 1 commit intoanthropics:mainfrom
qinlongli2024-ai:fix/path-traversal-in-read-file
Open

fix: prevent path traversal in ClaudeAPIClient._read_file()#79
qinlongli2024-ai wants to merge 1 commit intoanthropics:mainfrom
qinlongli2024-ai:fix/path-traversal-in-read-file

Conversation

@qinlongli2024-ai
Copy link

Summary

_read_file() in ClaudeAPIClient accepts file paths from untrusted security findings without validating that the resolved path stays within the repository root. A malicious PR could craft a finding with "file": "../../../etc/shadow" to exfiltrate arbitrary files from the CI runner — their contents are included verbatim in the prompt sent to the Claude API.

Attack scenario

  1. Attacker opens a PR against a repository that uses claude-code-security-review.
  2. The automated audit produces a finding referencing a crafted path (e.g., ../../../etc/shadow, ../../../home/runner/.ssh/id_rsa).
  3. _read_file() follows the .. segments, reads the sensitive file, and embeds its content in the Claude API prompt.
  4. Depending on the response handling, the file contents may be logged, stored in CI artifacts, or surfaced in PR comments.

Symlink-based escapes are also possible: a repo could include a symlink like data/config -> /etc/passwd, and the old code would follow it without complaint.

Fix

  • Reject absolute paths — findings should never reference absolute paths.
  • Resolve with os.path.realpath() — collapses .. segments and follows symlinks to their true destination.
  • Validate containment — the resolved path must start with repo_root + os.sep to prevent partial-prefix bypasses (e.g., /repo-data matching /repo).
  • Reject empty/whitespace paths early.

This mirrors the validated path-checking pattern already used in the sibling claude-code-action project (src/mcp/path-validation.ts).

Tests

Added 14 test cases covering:

  • ✅ Happy paths (relative, root-level, latin-1 encoding fallback)
  • 🛡️ ../ traversal (direct and prefixed)
  • 🛡️ Absolute path injection (/etc/passwd)
  • 🛡️ Symlink escape (symlink inside repo → file outside repo)
  • 🔧 Edge cases (empty, whitespace, directory, nonexistent, REPO_PATH unset fallback)

All 14 tests pass.

Checklist

  • Security fix with clear attack path
  • Comprehensive test coverage
  • No breaking changes (existing valid paths still work)
  • Consistent with path validation in claude-code-action

The _read_file() method in ClaudeAPIClient accepted file paths from
untrusted security findings without validating that the resolved path
stayed within the repository root.  An attacker could craft a finding
with a file field like '../../../etc/shadow' to read arbitrary files
from the CI runner, with their contents sent to the Claude API prompt.

Changes:
- Reject absolute paths outright (never legitimate from findings)
- Resolve the candidate path with os.path.realpath() to collapse '..'
  segments and follow symlinks to their true destination
- Verify the resolved path starts with the repository root + os.sep
  to prevent partial-prefix false positives
- Reject empty and whitespace-only paths early
- Add comprehensive test suite (14 tests) covering:
  - happy-path reads (relative, root-level, latin-1 fallback)
  - traversal via '..' sequences
  - absolute path injection
  - symlink escape attacks
  - edge cases (empty, whitespace, directory, nonexistent)
  - fallback behavior when REPO_PATH is unset

This approach mirrors the validated path-checking pattern used in the
sibling claude-code-action project (src/mcp/path-validation.ts).
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.

1 participant