fix(core): authorize external read paths

This commit is contained in:
Dax Raad
2026-06-26 13:55:04 -04:00
parent 0befd9b049
commit 1ac6b4bec4
2 changed files with 80 additions and 22 deletions
+23 -20
View File
@@ -1,12 +1,10 @@
export * as ReadTool from "./read" export * as ReadTool from "./read"
import { ToolFailure } from "@opencode-ai/llm" import { ToolFailure } from "@opencode-ai/llm"
import path from "path"
import { Effect, Layer, Schema } from "effect" import { Effect, Layer, Schema } from "effect"
import { FileSystem } from "../filesystem" import { FileSystem } from "../filesystem"
import { FSUtil } from "../fs-util"
import { Image } from "../image" import { Image } from "../image"
import { Location } from "../location" import { LocationMutation } from "../location-mutation"
import { PermissionV2 } from "../permission" import { PermissionV2 } from "../permission"
import { AbsolutePath } from "../schema" import { AbsolutePath } from "../schema"
import { ReadToolFileSystem } from "./read-filesystem" import { ReadToolFileSystem } from "./read-filesystem"
@@ -30,9 +28,8 @@ const Output = Schema.Union([FileSystem.Content, ReadToolFileSystem.TextPage, Re
export const layer = Layer.effectDiscard( export const layer = Layer.effectDiscard(
Effect.gen(function* () { Effect.gen(function* () {
const tools = yield* Tools.Service const tools = yield* Tools.Service
const fs = yield* FSUtil.Service
const reader = yield* ReadToolFileSystem.Service const reader = yield* ReadToolFileSystem.Service
const location = yield* Location.Service const mutation = yield* LocationMutation.Service
const image = yield* Image.Service const image = yield* Image.Service
const permission = yield* PermissionV2.Service const permission = yield* PermissionV2.Service
@@ -40,7 +37,7 @@ export const layer = Layer.effectDiscard(
.register({ .register({
[name]: Tool.make({ [name]: Tool.make({
description: description:
"Read a text file or supported image, page through a large UTF-8 text file by line offset, or list a directory page. Relative paths resolve from the current location; absolute paths are read directly.", "Read a text file or supported image, page through a large UTF-8 text file by line offset, or list a directory page. Relative paths resolve from the current location; absolute paths inside it are accepted, while external absolute paths require external_directory approval.",
input: Input, input: Input,
output: Output, output: Output,
toModelOutput: ({ input, output }) => { toModelOutput: ({ input, output }) => {
@@ -53,27 +50,33 @@ export const layer = Layer.effectDiscard(
}, },
execute: (input, context) => { execute: (input, context) => {
return Effect.gen(function* () { return Effect.gen(function* () {
const absolute = path.resolve(location.directory, input.path) const source = {
const selected = path.isAbsolute(input.path) ? path.dirname(absolute) : location.directory type: "tool" as const,
if (!path.isAbsolute(input.path) && !FSUtil.contains(location.directory, absolute)) messageID: context.assistantMessageID,
return yield* Effect.die(new Error("Path escapes the allowed read root")) callID: context.toolCallID,
const real = yield* fs.realPath(absolute) }
const root = yield* fs.realPath(selected) const target = yield* mutation.resolve({ path: input.path, kind: "directory" })
if (!FSUtil.contains(root, real)) const external = target.externalDirectory
return yield* Effect.die(new Error("Path escapes the allowed read root")) if (external)
const resource = path.relative(root, real).replaceAll("\\", "/") || "." yield* permission.assert({
const target = AbsolutePath.make(real) ...LocationMutation.externalDirectoryPermission(external),
const type = yield* reader.inspect(target) sessionID: context.sessionID,
agent: context.agent,
source,
})
const resource = target.resource
const absolute = AbsolutePath.make(target.canonical)
const type = yield* reader.inspect(absolute)
yield* permission.assert({ yield* permission.assert({
action: name, action: name,
resources: [resource], resources: [resource],
save: ["*"], save: ["*"],
sessionID: context.sessionID, sessionID: context.sessionID,
agent: context.agent, agent: context.agent,
source: { type: "tool", messageID: context.assistantMessageID, callID: context.toolCallID }, source,
}) })
if (type === "directory") return yield* reader.list(target, { offset: input.offset, limit: input.limit }) if (type === "directory") return yield* reader.list(absolute, { offset: input.offset, limit: input.limit })
const content = yield* reader.read(target, resource, { const content = yield* reader.read(absolute, resource, {
offset: input.offset, offset: input.offset,
limit: input.limit, limit: input.limit,
}) })
+57 -2
View File
@@ -11,6 +11,7 @@ import { PermissionV2 } from "@opencode-ai/core/permission"
import { SessionV2 } from "@opencode-ai/core/session" import { SessionV2 } from "@opencode-ai/core/session"
import { AbsolutePath } from "@opencode-ai/core/schema" import { AbsolutePath } from "@opencode-ai/core/schema"
import { Global } from "@opencode-ai/core/global" import { Global } from "@opencode-ai/core/global"
import { LocationMutation } from "@opencode-ai/core/location-mutation"
import { location } from "./fixture/location" import { location } from "./fixture/location"
import { ToolRegistry } from "@opencode-ai/core/tool/registry" import { ToolRegistry } from "@opencode-ai/core/tool/registry"
import { ReadTool } from "@opencode-ai/core/tool/read" import { ReadTool } from "@opencode-ai/core/tool/read"
@@ -97,6 +98,32 @@ const infrastructure = Layer.mergeAll(
Layer.succeed(Location.Service, Location.Service.of(location({ directory: AbsolutePath.make(process.cwd()) }))), Layer.succeed(Location.Service, Location.Service.of(location({ directory: AbsolutePath.make(process.cwd()) }))),
Global.layerWith({ data: Global.Path.data }), Global.layerWith({ data: Global.Path.data }),
) )
const mutation = Layer.succeed(
LocationMutation.Service,
LocationMutation.Service.of({
resolve: (input) => {
if (input.path === missingPath)
return Effect.fail(new LocationMutation.PathError({ path: input.path, reason: "non_directory_ancestor" }))
const canonical = path.resolve(process.cwd(), input.path)
const external = path.isAbsolute(input.path) && !FSUtil.contains(process.cwd(), canonical)
const resource = external ? canonical.replaceAll("\\", "/") : path.relative(process.cwd(), canonical) || "."
const directory = path.dirname(canonical)
const externalResource = path.join(directory, "*").replaceAll("\\", "/")
return Effect.succeed({
canonical,
resource,
externalDirectory: external
? {
action: "external_directory" as const,
directory,
resource: externalResource,
save: externalResource,
}
: undefined,
})
},
}),
)
const unavailableImage = Layer.succeed( const unavailableImage = Layer.succeed(
Image.Service, Image.Service,
Image.Service.of({ normalize: () => Effect.fail(new Image.ResizerUnavailableError()) }), Image.Service.of({ normalize: () => Effect.fail(new Image.ResizerUnavailableError()) }),
@@ -107,19 +134,21 @@ const read = ReadTool.layer.pipe(
Layer.provide(permission), Layer.provide(permission),
Layer.provide(config), Layer.provide(config),
Layer.provide(image), Layer.provide(image),
Layer.provide(mutation),
Layer.provide(infrastructure), Layer.provide(infrastructure),
) )
const it = testEffect(Layer.mergeAll(registry, reader, permission, config, image, infrastructure, read)) const it = testEffect(Layer.mergeAll(registry, reader, permission, config, image, mutation, infrastructure, read))
const unavailableRead = ReadTool.layer.pipe( const unavailableRead = ReadTool.layer.pipe(
Layer.provide(registry), Layer.provide(registry),
Layer.provide(reader), Layer.provide(reader),
Layer.provide(permission), Layer.provide(permission),
Layer.provide(config), Layer.provide(config),
Layer.provide(unavailableImage), Layer.provide(unavailableImage),
Layer.provide(mutation),
Layer.provide(infrastructure), Layer.provide(infrastructure),
) )
const itWithoutResizer = testEffect( const itWithoutResizer = testEffect(
Layer.mergeAll(registry, reader, permission, config, unavailableImage, infrastructure, unavailableRead), Layer.mergeAll(registry, reader, permission, config, unavailableImage, mutation, infrastructure, unavailableRead),
) )
const sessionID = SessionV2.ID.make("ses_read_tool_test") const sessionID = SessionV2.ID.make("ses_read_tool_test")
@@ -174,6 +203,32 @@ describe("ReadTool", () => {
}), }),
) )
it.effect("asks for external_directory approval before reading an external absolute path", () =>
Effect.gen(function* () {
const registry = yield* ToolRegistry.Service
const external = path.join(path.parse(process.cwd()).root, "external-read", "notes.txt")
expect(
yield* executeTool(registry, {
sessionID,
...toolIdentity,
call: { type: "tool-call", id: "call-external-read", name: "read", input: { path: external } },
}),
).toMatchObject({ type: "json" })
expect(assertions).toMatchObject([
{
sessionID,
action: "external_directory",
resources: [path.join(path.dirname(external), "*").replaceAll("\\", "/")],
},
{ sessionID, action: "read", resources: [external.replaceAll("\\", "/")], save: ["*"] },
])
expect(readCalls).toEqual([
{ input: AbsolutePath.make(external), page: { offset: undefined, limit: undefined } },
])
}),
)
it.effect("returns a small PNG as native media instead of durable base64 text", () => it.effect("returns a small PNG as native media instead of durable base64 text", () =>
Effect.gen(function* () { Effect.gen(function* () {
const png = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=" const png = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII="