Skip to content

Conversation

@elithrar
Copy link
Contributor

@elithrar elithrar commented Jan 9, 2026

Addresses gaps in the external_directory permission checks where symlinks inside a project could escape to read/write files outside the project boundary.

  • Add Filesystem.containsResolved() that resolves symlinks before checking path containment, preventing symlink escape attacks where a link like project/escape -> /etc/passwd would bypass lexical checks
  • Add missing external_directory permission check to WriteTool (was a TODO) and ReadTool
  • Update File.read() and File.list() to use dual-layer protection: fast lexical check first, then resolved check for existing files
  • Document TOCTOU limitation in containsResolved() - acceptable for the threat model of protecting against malicious symlinks in user-controlled directories

Test coverage:

  • Add 17 new tests for containsResolved() covering symlink chains, broken symlinks, relative symlink escapes, and positive cases for internal symlinks
  • Add integration tests validating File.read() blocks symlink escapes while allowing valid internal symlinks

Validated in practice — I had OpenCode build itself and then test via the actual filesystem as well:

Walk me through the tests, and consider:
- How do you KNOW they are checking symlink traversal correctly?
- Do we have tests that are trivial and not useful for validating symlink traversal and/or external_directory checks?
- Can you build opencode, pass a generate OPENCODE_CONFIG='<config>' with external_directory on/off with `opencode run`, and validate that the implementation solves the issues?

resulted in:

# Set up test scenario with symlink escaping project boundary
mkdir -p /tmp/test/project && cd /tmp/test
echo "SECRET" > secret.txt
ln -s /tmp/test/secret.txt project/escape-link.txt  # symlink points outside
echo "ALLOWED" > project/allowed.txt
cd project && git init && git add . && git commit -m "init"

# Build and run integration test against File.read()
cd packages/opencode && bun run build
bun run -e '
import { File } from "./src/file"
import { Instance } from "./src/project/instance"

await Instance.provide({
  directory: "/tmp/test/project",
  fn: async () => {
    // Should succeed - regular file
    console.log(await File.read("allowed.txt"))  // { content: "ALLOWED" }
    
    // Should throw - symlink escapes project
    await File.read("escape-link.txt")  // "Access denied: path escapes project directory"
  }
})
'

- Add Filesystem.containsResolved() that resolves symlinks before checking
  path containment, preventing symlink escape attacks
- Add external_directory permission check to write.ts (was a TODO)
- Update File.read/File.list to use containsResolved for existing files
- Add dual-layer protection: lexical check first, then resolved check for
  existing paths
- Document TOCTOU limitation in function docstring
- Improve test robustness: explicit skip for unsupported symlink operations
- Add comprehensive tests for symlink traversal scenarios
Unit tests for Filesystem.contains() and containsResolved() are in
test/util/filesystem.test.ts - no need to duplicate in path-traversal.test.ts
ReadTool was missing the containsResolved() check that WriteTool and
File.read() have. This meant a symlink inside the project pointing
outside could bypass external_directory permission when using ReadTool.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

The following comment was made by an LLM, it may be inaccurate:

Based on the search results, I found one potentially related PR:

Related PR:

This PR appears to address a similar security concern around symlink escapes in path containment checks, which is directly relevant to the current PR's focus on preventing symlink escape attacks.

The current PR (7515) builds upon or may supersede this work by introducing the Filesystem.containsResolved() method and adding missing permission checks to WriteTool and ReadTool.

…ypass

- Add containsResolved() check to EditTool and PatchTool to prevent
  symlink escape attacks
- Add bypassCwdCheck flag to write.ts for consistency with read.ts
- Fix broken symlink bypass: check symlinks BEFORE exists() check
  so broken symlinks pointing outside are caught
- Reorder read.ts to check symlinks before exists() for consistency
- Add 4 symlink tests to ReadTool: escape, broken escape, internal symlink
- Create write.test.ts with 7 tests including 4 symlink tests
- Tests verify external_directory permission is requested for symlink escapes
- Tests verify broken symlinks pointing outside are caught
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant