Add the protect plugin, refusing writes to tool-owned files
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
co-authored by
Sisyphus
parent
551e518711
commit
26d62bc926
+66
-8
@@ -1,5 +1,6 @@
|
|||||||
import { tool } from 'ai';
|
import { tool } from 'ai';
|
||||||
import { z } from 'zod';
|
import { z } from 'zod';
|
||||||
|
import { posix } from './ignore';
|
||||||
import type { Plugin } from './plugins';
|
import type { Plugin } from './plugins';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -87,7 +88,7 @@ const SECRET_PATHS: { re: RegExp; why: string }[] = [
|
|||||||
{ re: /(^|[\\/])\.gnupg[\\/]/i, why: 'a GPG directory' },
|
{ re: /(^|[\\/])\.gnupg[\\/]/i, why: 'a GPG directory' },
|
||||||
];
|
];
|
||||||
|
|
||||||
/** Every path a write tool might carry, including a patch's markers. */
|
/** Every path a write tool might carry, including a patch's markers and a move's ends. */
|
||||||
function writtenPaths(toolName: string, input: unknown): string[] {
|
function writtenPaths(toolName: string, input: unknown): string[] {
|
||||||
const o = (input ?? {}) as Record<string, unknown>;
|
const o = (input ?? {}) as Record<string, unknown>;
|
||||||
|
|
||||||
@@ -99,9 +100,16 @@ function writtenPaths(toolName: string, input: unknown): string[] {
|
|||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (toolName === 'move_file') {
|
||||||
|
return [o['from'], o['to']].filter((p): p is string => typeof p === 'string');
|
||||||
|
}
|
||||||
|
|
||||||
return typeof o['path'] === 'string' ? [o['path']] : [];
|
return typeof o['path'] === 'string' ? [o['path']] : [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Write tools a path-based guard has to cover. Missing one is a silent bypass. */
|
||||||
|
const WRITE_TOOLS = ['write_file', 'edit_file', 'multi_edit', 'apply_patch', 'move_file', 'delete_file'];
|
||||||
|
|
||||||
export const secretsPlugin: Plugin = {
|
export const secretsPlugin: Plugin = {
|
||||||
name: 'secrets',
|
name: 'secrets',
|
||||||
description: 'refuses to write credential files',
|
description: 'refuses to write credential files',
|
||||||
@@ -110,7 +118,7 @@ export const secretsPlugin: Plugin = {
|
|||||||
'user which file and which key, and let them write it themselves. Do not work around the refusal by writing ' +
|
'user which file and which key, and let them write it themselves. Do not work around the refusal by writing ' +
|
||||||
'the same content somewhere else.',
|
'the same content somewhere else.',
|
||||||
beforeToolCall: ({ toolName, input }) => {
|
beforeToolCall: ({ toolName, input }) => {
|
||||||
if (!['write_file', 'edit_file', 'multi_edit', 'apply_patch'].includes(toolName)) return undefined;
|
if (!WRITE_TOOLS.includes(toolName)) return undefined;
|
||||||
|
|
||||||
for (const path of writtenPaths(toolName, input)) {
|
for (const path of writtenPaths(toolName, input)) {
|
||||||
for (const { re, why } of SECRET_PATHS) {
|
for (const { re, why } of SECRET_PATHS) {
|
||||||
@@ -123,6 +131,49 @@ export const secretsPlugin: Plugin = {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Paths a write must not touch, for reasons other than secrecy.
|
||||||
|
*
|
||||||
|
* These are not credentials, so the secrets plugin has nothing to say about them.
|
||||||
|
* They are files whose contents are owned by a tool rather than by anyone editing
|
||||||
|
* them by hand: git's own object store, a resolver's lockfile, an installed
|
||||||
|
* dependency tree, a build directory. A model editing one of these produces a
|
||||||
|
* repository that looks fine and behaves wrongly, and the failure surfaces
|
||||||
|
* somewhere else entirely.
|
||||||
|
*/
|
||||||
|
const PROTECTED_PATHS: { re: RegExp; why: string }[] = [
|
||||||
|
{ re: /(^|[\\/])\.git[\\/]/i, why: "git's own object store" },
|
||||||
|
{
|
||||||
|
re: /(^|[\\/])(bun\.lock|bun\.lockb|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|Cargo\.lock|poetry\.lock|uv\.lock|composer\.lock|go\.sum|Gemfile\.lock)$/i,
|
||||||
|
why: 'a lockfile the package manager owns',
|
||||||
|
},
|
||||||
|
{ re: /(^|[\\/])node_modules[\\/]/i, why: 'an installed dependency' },
|
||||||
|
{ re: /(^|[\\/])(vendor|target[\\/]debug|target[\\/]release)[\\/]/i, why: 'a vendored or build directory' },
|
||||||
|
{ re: /(^|[\\/])(dist|build|out|\.next|\.nuxt|\.svelte-kit|coverage)[\\/]/i, why: 'generated build output' },
|
||||||
|
{ re: /(^|[\\/])\.(venv|tox|mypy_cache|pytest_cache|ruff_cache|turbo|parcel-cache)[\\/]/i, why: 'a tool cache' },
|
||||||
|
];
|
||||||
|
|
||||||
|
export const protectPlugin: Plugin = {
|
||||||
|
name: 'protect',
|
||||||
|
description: 'refuses writes to lockfiles, .git, dependencies, and build output',
|
||||||
|
appendix:
|
||||||
|
'The protect plugin refuses writes to .git, lockfiles, node_modules, vendored code, and build output. A ' +
|
||||||
|
'lockfile is regenerated by its package manager: run the install or update command through bash instead of ' +
|
||||||
|
'editing the file. Generated output is regenerated by its build. Do not route around the refusal.',
|
||||||
|
beforeToolCall: ({ toolName, input }) => {
|
||||||
|
if (!WRITE_TOOLS.includes(toolName)) return undefined;
|
||||||
|
|
||||||
|
for (const path of writtenPaths(toolName, input)) {
|
||||||
|
for (const { re, why } of PROTECTED_PATHS) {
|
||||||
|
if (re.test(posix(path))) {
|
||||||
|
return `refusing to write ${path} (${why}). Regenerate it with the tool that owns it rather than editing it.`;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return undefined;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
const FORMATTERS: { file: string; script: string; command: string[] }[] = [
|
const FORMATTERS: { file: string; script: string; command: string[] }[] = [
|
||||||
{ file: 'package.json', script: 'format', command: ['bun', 'run', 'format'] },
|
{ file: 'package.json', script: 'format', command: ['bun', 'run', 'format'] },
|
||||||
{ file: 'Cargo.toml', script: '', command: ['cargo', 'fmt'] },
|
{ file: 'Cargo.toml', script: '', command: ['cargo', 'fmt'] },
|
||||||
@@ -167,15 +218,22 @@ export const formatPlugin: Plugin = {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
export const BUILTIN_PLUGINS: Plugin[] = [guardPlugin, secretsPlugin, bellPlugin, timePlugin, formatPlugin];
|
export const BUILTIN_PLUGINS: Plugin[] = [
|
||||||
|
guardPlugin,
|
||||||
|
secretsPlugin,
|
||||||
|
protectPlugin,
|
||||||
|
bellPlugin,
|
||||||
|
timePlugin,
|
||||||
|
formatPlugin,
|
||||||
|
];
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Enabled unless the config turns them off.
|
* Enabled unless the config turns them off.
|
||||||
*
|
*
|
||||||
* `guard` and `secrets` are refusals, so they are on: a user who has to opt into a
|
* `guard`, `secrets`, and `protect` are refusals, so they are on: a user who has to
|
||||||
* safety check does not have it. `bell` and `format` both act on their own — one
|
* opt into a safety check does not have it. `bell` and `format` both act on their
|
||||||
* makes noise, the other writes files — so they are opt-in.
|
* own — one makes noise, the other writes files — so they are opt-in.
|
||||||
*/
|
*/
|
||||||
export const DEFAULT_ENABLED = ['guard', 'secrets', 'time'];
|
export const DEFAULT_ENABLED = ['guard', 'secrets', 'protect', 'time'];
|
||||||
|
|
||||||
export { DESTRUCTIVE, SECRET_PATHS };
|
export { DESTRUCTIVE, SECRET_PATHS, PROTECTED_PATHS };
|
||||||
|
|||||||
@@ -3,14 +3,16 @@ import { mkdtempSync, rmSync } from 'node:fs';
|
|||||||
import { tmpdir } from 'node:os';
|
import { tmpdir } from 'node:os';
|
||||||
import { join } from 'node:path';
|
import { join } from 'node:path';
|
||||||
import { createHost } from '../src/plugins';
|
import { createHost } from '../src/plugins';
|
||||||
import { BUILTIN_PLUGINS, DEFAULT_ENABLED, formatPlugin, secretsPlugin } from '../src/plugins-builtin';
|
import { BUILTIN_PLUGINS, DEFAULT_ENABLED, formatPlugin, protectPlugin, secretsPlugin } from '../src/plugins-builtin';
|
||||||
|
|
||||||
const cwd = process.cwd();
|
const cwd = process.cwd();
|
||||||
const check = (toolName: string, input: unknown) => secretsPlugin.beforeToolCall!({ toolName, input, cwd });
|
const check = (toolName: string, input: unknown) => secretsPlugin.beforeToolCall!({ toolName, input, cwd });
|
||||||
|
const guarded = (toolName: string, input: unknown) => protectPlugin.beforeToolCall!({ toolName, input, cwd });
|
||||||
|
|
||||||
test('secrets is on by default, because an opt-in safety check is not one', () => {
|
test('secrets is on by default, because an opt-in safety check is not one', () => {
|
||||||
expect(DEFAULT_ENABLED).toContain('secrets');
|
expect(DEFAULT_ENABLED).toContain('secrets');
|
||||||
expect(DEFAULT_ENABLED).toContain('guard');
|
expect(DEFAULT_ENABLED).toContain('guard');
|
||||||
|
expect(DEFAULT_ENABLED).toContain('protect');
|
||||||
// Both of these act on their own rather than refusing, so they are opt-in.
|
// Both of these act on their own rather than refusing, so they are opt-in.
|
||||||
expect(DEFAULT_ENABLED).not.toContain('bell');
|
expect(DEFAULT_ENABLED).not.toContain('bell');
|
||||||
expect(DEFAULT_ENABLED).not.toContain('format');
|
expect(DEFAULT_ENABLED).not.toContain('format');
|
||||||
@@ -41,7 +43,7 @@ test('writing a registry or credential file is refused', async () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test('every write tool is covered, including apply_patch', async () => {
|
test('every write tool is covered, including apply_patch', async () => {
|
||||||
for (const tool of ['write_file', 'edit_file', 'multi_edit']) {
|
for (const tool of ['write_file', 'edit_file', 'multi_edit', 'delete_file']) {
|
||||||
expect(await check(tool, { path: '.env' }), tool).toContain('refusing to write');
|
expect(await check(tool, { path: '.env' }), tool).toContain('refusing to write');
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -50,6 +52,59 @@ test('every write tool is covered, including apply_patch', async () => {
|
|||||||
const patch = '*** Update File: src/app.ts\n-a\n+b\n*** Add File: .env\n+SECRET=x';
|
const patch = '*** Update File: src/app.ts\n-a\n+b\n*** Add File: .env\n+SECRET=x';
|
||||||
expect(await check('apply_patch', { patch })).toContain('refusing to write');
|
expect(await check('apply_patch', { patch })).toContain('refusing to write');
|
||||||
expect(await check('apply_patch', { patch: '*** Move to: .env.production\n' })).toContain('refusing to write');
|
expect(await check('apply_patch', { patch: '*** Move to: .env.production\n' })).toContain('refusing to write');
|
||||||
|
|
||||||
|
// A move carries two paths and neither is called `path`.
|
||||||
|
expect(await check('move_file', { from: 'src/app.ts', to: '.env' })).toContain('refusing to write');
|
||||||
|
expect(await check('move_file', { from: '.env', to: 'src/app.ts' })).toContain('refusing to write');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('protect refuses git internals, lockfiles, dependencies, and build output', async () => {
|
||||||
|
for (const path of [
|
||||||
|
'.git/config',
|
||||||
|
'.git/hooks/pre-commit',
|
||||||
|
'bun.lock',
|
||||||
|
'package-lock.json',
|
||||||
|
'Cargo.lock',
|
||||||
|
'go.sum',
|
||||||
|
'node_modules/react/index.js',
|
||||||
|
'vendor/github.com/pkg/errors.go',
|
||||||
|
'dist/bundle.js',
|
||||||
|
'coverage/lcov.info',
|
||||||
|
'.next/build-manifest.json',
|
||||||
|
]) {
|
||||||
|
expect(await guarded('write_file', { path }), path).toContain('refusing to write');
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('protect covers every write tool and both ends of a move', async () => {
|
||||||
|
for (const tool of ['write_file', 'edit_file', 'multi_edit', 'delete_file']) {
|
||||||
|
expect(await guarded(tool, { path: 'bun.lock' }), tool).toContain('refusing to write');
|
||||||
|
}
|
||||||
|
expect(await guarded('apply_patch', { patch: '*** Update File: bun.lock\n-a\n+b' })).toContain('refusing to write');
|
||||||
|
expect(await guarded('move_file', { from: 'src/a.ts', to: 'node_modules/a.ts' })).toContain('refusing to write');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('protect matches a Windows path separator as well', async () => {
|
||||||
|
expect(await guarded('write_file', { path: 'node_modules\\react\\index.js' })).toContain('refusing to write');
|
||||||
|
expect(await guarded('write_file', { path: '.git\\config' })).toContain('refusing to write');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('protect leaves ordinary files and lookalike names alone', async () => {
|
||||||
|
for (const path of [
|
||||||
|
'src/app.ts',
|
||||||
|
'docs/dist-layout.md',
|
||||||
|
'src/gitignore-parser.ts',
|
||||||
|
'test/node_modules-resolution.test.ts',
|
||||||
|
'package.json',
|
||||||
|
'distributed/queue.ts',
|
||||||
|
]) {
|
||||||
|
expect(await guarded('write_file', { path }), path).toBeUndefined();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('protect says what to do instead of editing the file', async () => {
|
||||||
|
expect(protectPlugin.appendix).toContain('package manager');
|
||||||
|
expect(String(await guarded('write_file', { path: 'bun.lock' }))).toContain('Regenerate it');
|
||||||
});
|
});
|
||||||
|
|
||||||
test('ordinary source files are untouched', async () => {
|
test('ordinary source files are untouched', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user