Просмотр исходного кода

fix(core): skip shell parsing when permissions allow all (#42203)

Kit Langton 3 дней назад
Родитель
Сommit
a31058d76c

+ 24 - 1
packages/core/src/permission.ts

@@ -99,6 +99,11 @@ export function merge(...rulesets: Permission.Ruleset[]): Permission.Ruleset {
 }
 
 export interface Interface {
+  readonly allowsAll: (input: {
+    readonly sessionID: SessionSchema.ID
+    readonly action: string
+    readonly agent?: Agent.ID
+  }) => Effect.Effect<boolean, SessionErrors.NotFoundError>
   readonly ask: (input: AssertInput) => Effect.Effect<AskResult, SessionErrors.NotFoundError>
   readonly assert: (input: AssertInput) => Effect.Effect<void, Error | SessionErrors.NotFoundError>
   readonly reply: (input: ReplyInput) => Effect.Effect<void, NotFoundError>
@@ -154,6 +159,24 @@ const layer = Layer.effect(
       return agent?.permissions ?? missingAgentPermissions
     })
 
+    const allowsAll = Effect.fn("Permission.allowsAll")(function* (input: {
+      readonly sessionID: SessionSchema.ID
+      readonly action: string
+      readonly agent?: Agent.ID
+    }) {
+      const rules = yield* configured(input.sessionID, input.agent)
+      const relevant = rules.filter((rule) => Wildcard.match(input.action, rule.action))
+      for (let index = relevant.length - 1; index >= 0; index--) {
+        const rule = relevant[index]
+        if (rule.resource !== "*") {
+          if (rule.effect !== "allow") return false
+          continue
+        }
+        return rule.effect === "allow"
+      }
+      return false
+    })
+
     function denied(input: AssertInput, rules: Permission.Ruleset) {
       return input.resources.some((resource) => evaluate(input.action, resource, rules).effect === "deny")
     }
@@ -315,7 +338,7 @@ const layer = Layer.effect(
       return Array.from(pending.values(), (item) => item.request).filter((request) => request.sessionID === sessionID)
     })
 
-    return Service.of({ ask, assert, reply, get, forSession, list })
+    return Service.of({ allowsAll, ask, assert, reply, get, forSession, list })
   }),
 )
 

+ 39 - 26
packages/core/src/tool/plugin/shell.ts

@@ -149,36 +149,49 @@ export const Plugin = {
                 (invocation) =>
                   Effect.gen(function* () {
                     const target = yield* mutation.resolve({ path: invocation.cwd, kind: "directory" })
-                    const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute)
-                    const directories = yield* Effect.forEach(parsed.directories, (directory) =>
-                      mutation.resolve({ path: path.resolve(target.absolute, directory), kind: "directory" }),
-                    )
-                    invocation.cwd = target.absolute
-                    finalTimeout = invocation.timeout
-                    const external = [target, ...directories]
-                      .map((item) => item.externalDirectory)
-                      .filter((item) => item !== undefined)
-                      .filter(
-                        (item, index, items) => items.findIndex((other) => other.resource === item.resource) === index,
-                      )
-                    if (external.length > 0)
-                      yield* permission.assert({
-                        action: "external_directory",
-                        resources: external.map((item) => item.resource),
-                        save: external.map((item) => item.save),
+                    const unrestricted =
+                      (yield* permission.allowsAll({
                         sessionID: context.sessionID,
-                        agent: context.agent,
-                        source,
-                      })
-                    if (parsed.commands.length > 0)
-                      yield* permission.assert({
                         action: name,
-                        resources: parsed.commands.map((command) => command.resource),
-                        save: parsed.commands.map((command) => command.save),
+                        agent: context.agent,
+                      })) &&
+                      (yield* permission.allowsAll({
                         sessionID: context.sessionID,
+                        action: "external_directory",
                         agent: context.agent,
-                        source,
-                      })
+                      }))
+                    invocation.cwd = target.absolute
+                    finalTimeout = invocation.timeout
+                    if (!unrestricted) {
+                      const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute)
+                      const directories = yield* Effect.forEach(parsed.directories, (directory) =>
+                        mutation.resolve({ path: path.resolve(target.absolute, directory), kind: "directory" }),
+                      )
+                      const external = [target, ...directories]
+                        .map((item) => item.externalDirectory)
+                        .filter((item) => item !== undefined)
+                        .filter(
+                          (item, index, items) => items.findIndex((other) => other.resource === item.resource) === index,
+                        )
+                      if (external.length > 0)
+                        yield* permission.assert({
+                          action: "external_directory",
+                          resources: external.map((item) => item.resource),
+                          save: external.map((item) => item.save),
+                          sessionID: context.sessionID,
+                          agent: context.agent,
+                          source,
+                        })
+                      if (parsed.commands.length > 0)
+                        yield* permission.assert({
+                          action: name,
+                          resources: parsed.commands.map((command) => command.resource),
+                          save: parsed.commands.map((command) => command.save),
+                          sessionID: context.sessionID,
+                          agent: context.agent,
+                          source,
+                        })
+                    }
                     const workdir = yield* Environment.typeFollowing(environment.files, target.absolute).pipe(
                       Effect.catchTag("Environment.NotFound", () =>
                         Effect.fail(new Error(`Working directory does not exist: ${target.absolute}`)),

+ 25 - 0
packages/core/test/permission.test.ts

@@ -112,6 +112,31 @@ describe("Permission", () => {
     }),
   )
 
+  it.effect("proves only unconditional configured allows", () =>
+    Effect.gen(function* () {
+      const service = yield* Permission.Service
+      const input = { sessionID: Session.ID.make("ses_test"), action: "shell" }
+
+      yield* setup([{ action: "shell", resource: "*", effect: "allow" }])
+      expect(yield* service.allowsAll(input)).toBe(true)
+
+      yield* setRules([
+        { action: "shell", resource: "*", effect: "allow" },
+        { action: "shell", resource: "rm *", effect: "deny" },
+      ])
+      expect(yield* service.allowsAll(input)).toBe(false)
+
+      yield* setRules([{ action: "shell", resource: "git *", effect: "allow" }])
+      expect(yield* service.allowsAll(input)).toBe(false)
+
+      yield* setRules([
+        { action: "shell", resource: "rm *", effect: "deny" },
+        { action: "shell", resource: "*", effect: "allow" },
+      ])
+      expect(yield* service.allowsAll(input)).toBe(true)
+    }),
+  )
+
   it.effect("evaluates against an explicit provider-turn agent", () =>
     Effect.gen(function* () {
       yield* setup([{ action: "read", resource: "*", effect: "allow" }])

+ 1 - 0
packages/core/test/session-instructions.test.ts

@@ -61,6 +61,7 @@ const projects = Layer.succeed(
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: () => Effect.void,
     ask: () => Effect.die("unused"),
     reply: () => Effect.die("unused"),

+ 1 - 0
packages/core/test/session-runner-recorded.test.ts

@@ -58,6 +58,7 @@ const client = LLMClient.layer.pipe(Layer.provide(executor))
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: () => Effect.die("unused"),
     ask: () => Effect.die("unused"),
     reply: () => Effect.die("unused"),

+ 1 - 0
packages/core/test/session-runner.test.ts

@@ -221,6 +221,7 @@ const permissionFail = {
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: () => Effect.die("unused"),
     ask: () => Effect.die("unused"),
     reply: () => Effect.die("unused"),

+ 1 - 0
packages/core/test/tool-edit.test.ts

@@ -46,6 +46,7 @@ let formatFile = (_target: string): Effect.Effect<boolean> => Effect.succeed(fal
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) =>
       Effect.sync(() => assertions.push(input)).pipe(
         Effect.andThen(

+ 1 - 0
packages/core/test/tool-patch.test.ts

@@ -41,6 +41,7 @@ let formatFile = (_target: string): Effect.Effect<boolean> => Effect.succeed(fal
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) =>
       Effect.sync(() => {
         assertions.push(input)

+ 1 - 0
packages/core/test/tool-question.test.ts

@@ -31,6 +31,7 @@ const questionInput = {
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) =>
       Effect.sync(() => assertions.push(input)).pipe(
         Effect.andThen(

+ 1 - 0
packages/core/test/tool-read.test.ts

@@ -73,6 +73,7 @@ let allow = true
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) =>
       Effect.sync(() => {
         assertions.push(input)

+ 1 - 0
packages/core/test/tool-search.test.ts

@@ -52,6 +52,7 @@ const withTools = <A, E, R>(
           Layer.succeed(
             Permission.Service,
             Permission.Service.of({
+              allowsAll: () => Effect.succeed(false),
               assert: (input) =>
                 Effect.sync(() => {
                   assertions?.push(input)

+ 27 - 0
packages/core/test/tool-shell.test.ts

@@ -44,12 +44,14 @@ import { toolIdentity, executeTool, registerToolPlugin, toolDefinitions } from "
 const sessionID = Session.ID.make("ses_shell_tool_test")
 const sessionModel = Model.Ref.make({ id: Model.ID.make("test"), providerID: Provider.ID.make("test") })
 const assertions: Permission.AssertInput[] = []
+const allowedActions = new Set<string>()
 let denyAction: string | undefined
 let afterPermission = (_input: Permission.AssertInput): Effect.Effect<void> => Effect.void
 
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: (input) => Effect.succeed(allowedActions.has(input.action)),
     assert: (input) =>
       Effect.sync(() => assertions.push(input)).pipe(
         Effect.andThen(Effect.suspend(() => afterPermission(input))),
@@ -75,6 +77,7 @@ const permission = Layer.succeed(
 
 const reset = () => {
   assertions.length = 0
+  allowedActions.clear()
   denyAction = undefined
   afterPermission = () => Effect.void
 }
@@ -337,6 +340,30 @@ describe("ShellTool", () => {
     { timeout: 15_000 },
   )
 
+  it.live(
+    "skips command decomposition when shell and external directories are unrestricted",
+    () =>
+      Effect.acquireUseRelease(
+        Effect.promise(() => tmpdir()),
+        (tmp) => {
+          reset()
+          allowedActions.add("shell")
+          allowedActions.add("external_directory")
+          return withSession(tmp.path, (registry) =>
+            executeTool(registry, call({ command: "printf one && printf two" }, "call-unrestricted")),
+          ).pipe(
+            Effect.andThen(
+              Effect.sync(() => {
+                expect(assertions).toEqual([])
+              }),
+            ),
+          )
+        },
+        (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]().then(() => undefined)),
+      ),
+    { timeout: 15_000 },
+  )
+
   it.live(
     "captures stderr-only and mixed stdout/stderr output",
     () =>

+ 1 - 0
packages/core/test/tool-skill.test.ts

@@ -55,6 +55,7 @@ describe("SkillTool", () => {
           const permission = Layer.succeed(
             Permission.Service,
             Permission.Service.of({
+              allowsAll: () => Effect.succeed(false),
               assert: (input) =>
                 Effect.sync(() => assertions.push(input)).pipe(
                   Effect.andThen(

+ 1 - 0
packages/core/test/tool-webfetch.test.ts

@@ -39,6 +39,7 @@ const http = Layer.succeed(
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) => Effect.sync(() => assertions.push(input)),
     ask: () => Effect.die("unused"),
     reply: () => Effect.die("unused"),

+ 1 - 0
packages/core/test/tool-websearch.test.ts

@@ -69,6 +69,7 @@ beforeEach(() => {
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) => Effect.sync(() => assertions.push(input)),
     ask: () => Effect.die("unused"),
     reply: () => Effect.die("unused"),

+ 1 - 0
packages/core/test/tool-write.test.ts

@@ -36,6 +36,7 @@ let denyAction: string | undefined
 const permission = Layer.succeed(
   Permission.Service,
   Permission.Service.of({
+    allowsAll: () => Effect.succeed(false),
     assert: (input) =>
       Effect.sync(() => assertions.push(input)).pipe(
         Effect.andThen(