opencode icon indicating copy to clipboard operation
opencode copied to clipboard

fix(security): add path traversal protection to File.read and File.list

Open edlsh opened this issue 2 months ago • 8 comments

Summary

Adds path containment checks to File.read() and File.list() to prevent directory traversal attacks (e.g., ../../../etc/passwd).

Problem

The File module constructs paths via path.join(Instance.directory, file) without validating containment. An attacker-controlled path like ../../../etc/passwd resolves to a valid path outside the project directory.

Solution

Uses the existing Filesystem.contains() utility (already used in tool/read.ts, tool/write.ts, etc.) to validate that resolved paths remain within Instance.directory. Throws on violation.

Changes

  • packages/opencode/src/file/index.ts: Added containment checks to File.read() and File.list()
  • packages/opencode/test/file/path-traversal.test.ts: Added tests for traversal prevention

Known Limitations (documented via TODO)

  • Symlinks inside the project can still escape (lexical check only)
  • Windows cross-drive paths may bypass the check

These are pre-existing limitations in Filesystem.contains() affecting all current callers and warrant a separate PR.

Testing

bun test test/file/path-traversal.test.ts
# 4 pass, 0 fail

edlsh avatar Dec 22 '25 20:12 edlsh

/review

rekram1-node avatar Dec 22 '25 20:12 rekram1-node

lgtm

github-actions[bot] avatar Dec 22 '25 20:12 github-actions[bot]

I'd argue it would be good to use a whitelist / blacklist / have some sort of config setting configure this.

Preventing malicious path traversals is important, but I find myself having genuine use cases for path traversal. E.g. cloning repos to study in /tmp or having opencode read / edit some config files (outside of current project)

We could:

  1. Block all path traversal by default, have config option with whitelist
  2. same as 1 but with built in white list for common paths like /tmp
  3. Only block predefined blacklist traversal for sensitive paths

Davincible avatar Dec 25 '25 14:12 Davincible

The agent tools already have a permission system for external paths and aren't affected here. This PR only hardens the UI file browser API, which should be scoped to the project directory. For external repos, you'd run opencode from that directory.

edlsh avatar Dec 26 '25 04:12 edlsh

Hm im pretty sure the code works as is actually, try this:

import { Agent } from "./agent/agent"
import { bootstrap } from "./cli/bootstrap"
import { SystemPrompt } from "./session/system"
import { mergeDeep } from "remeda"
import { ReadTool } from "./tool/read"

async function main() {
  await bootstrap(process.cwd(), async () => {
    const read = await ReadTool.init()
    const result = await read.execute(
      {
        filePath: "../../../../etc/passwd",
      },
      {
        agent: "build",
      },
    )
    console.log(result)
  })
}

await main()

It will correctly prevent it from being read (i set permission to deny instead of read here) Screenshot 2025-12-26 at 2 13 13 PM

rekram1-node avatar Dec 26 '25 20:12 rekram1-node

The ReadTool you tested has its own containment check in tool/read.ts:L51. My PR addresses a different code path: the File.read()/File.list() functions used by the HTTP API (/file/list, /file/read endpoints in server.ts:L1840-1889) that power the TUI file browser. These endpoints don't go through the agent tool layer and previously had no path validation.

edlsh avatar Dec 26 '25 21:12 edlsh

oh right i forgot

rekram1-node avatar Dec 26 '25 21:12 rekram1-node

i misread rhe description my b

rekram1-node avatar Dec 26 '25 21:12 rekram1-node