Browse Source

Fix patch (#277)

* fix(hooks): normalize safe apply_patch absolute paths

* Update checma
Alvin 2 days ago
parent
commit
47a8e147e3

+ 14 - 4
oh-my-opencode-slim.schema.json

@@ -261,8 +261,10 @@
             },
             "options": {
               "type": "object",
-              "additionalProperties": true,
-              "description": "Provider-specific model options (e.g., textVerbosity, thinking budget)"
+              "propertyNames": {
+                "type": "string"
+              },
+              "additionalProperties": {}
             }
           }
         }
@@ -329,8 +331,10 @@
           },
           "options": {
             "type": "object",
-            "additionalProperties": true,
-            "description": "Provider-specific model options (e.g., textVerbosity, thinking budget)"
+            "propertyNames": {
+              "type": "string"
+            },
+            "additionalProperties": {}
           }
         }
       }
@@ -440,6 +444,12 @@
         "autoOpenBrowser": {
           "default": true,
           "type": "boolean"
+        },
+        "port": {
+          "default": 0,
+          "type": "integer",
+          "minimum": 0,
+          "maximum": 65535
         }
       }
     },

+ 117 - 4
src/hooks/apply-patch/execution-context.ts

@@ -41,6 +41,7 @@ export type PreparedFileState =
 
 export type PatchExecutionContext = {
   hunks: PatchHunk[];
+  pathsNormalized: boolean;
   staged: Map<string, PreparedFileState>;
   getPreparedFileState: (
     filePath: string,
@@ -226,6 +227,97 @@ function validatePatchPaths(hunks: PatchHunk[]): void {
   }
 }
 
+function toPortablePatchPath(filePath: string): string {
+  return filePath.split(path.sep).join('/');
+}
+
+function toRelativePatchPath(root: string, target: string): string {
+  const relative = path.relative(root, target);
+
+  return toPortablePatchPath(
+    relative.length === 0 ? path.basename(target) : relative,
+  );
+}
+
+async function normalizeAbsolutePatchPath(
+  root: string,
+  worktree: string | undefined,
+  value: string,
+): Promise<string> {
+  if (!path.isAbsolute(value)) {
+    return value;
+  }
+
+  const guardContext = createPathGuardContext(root, worktree);
+  const target = path.resolve(value);
+
+  await guard(guardContext, target);
+
+  const [rootReal, targetReal] = await Promise.all([
+    guardContext.rootReal,
+    realCached(guardContext, target),
+  ]);
+
+  if (!inside(rootReal, targetReal)) {
+    throw createApplyPatchBlockedError(
+      `patch contains path outside workspace root: ${target}`,
+    );
+  }
+
+  return toRelativePatchPath(root, target);
+}
+
+async function normalizeAbsolutePatchPaths(
+  root: string,
+  worktree: string | undefined,
+  hunks: PatchHunk[],
+): Promise<{
+  hunks: PatchHunk[];
+  changed: boolean;
+}> {
+  const normalized: PatchHunk[] = [];
+  let changed = false;
+
+  for (const hunk of hunks) {
+    const normalizedPath = await normalizeAbsolutePatchPath(
+      root,
+      worktree,
+      hunk.path,
+    );
+
+    if (hunk.type !== 'update') {
+      changed ||= normalizedPath !== hunk.path;
+      normalized.push(
+        normalizedPath === hunk.path
+          ? hunk
+          : {
+              ...hunk,
+              path: normalizedPath,
+            },
+      );
+      continue;
+    }
+
+    const normalizedMovePath = hunk.move_path
+      ? await normalizeAbsolutePatchPath(root, worktree, hunk.move_path)
+      : undefined;
+    changed ||=
+      normalizedPath !== hunk.path || normalizedMovePath !== hunk.move_path;
+
+    normalized.push(
+      normalizedPath === hunk.path && normalizedMovePath === hunk.move_path
+        ? hunk
+        : {
+            ...hunk,
+            path: normalizedPath,
+            move_path: normalizedMovePath,
+          },
+    );
+  }
+
+  return { hunks: normalized, changed };
+}
+
 async function guardPatchTargets(
   root: string,
   worktree: string | undefined,
@@ -240,7 +332,14 @@ async function guardPatchTargets(
   return targets.length;
 }
 
-export function parseValidatedPatch(patchText: string): PatchHunk[] {
+export async function parseValidatedPatch(
+  root: string,
+  patchText: string,
+  worktree?: string,
+): Promise<{
+  hunks: PatchHunk[];
+  pathsNormalized: boolean;
+}> {
   let hunks: PatchHunk[];
 
   try {
@@ -258,9 +357,18 @@ export function parseValidatedPatch(patchText: string): PatchHunk[] {
     throw createApplyPatchValidationError('no hunks found');
   }
 
-  validatePatchPaths(hunks);
+  const normalizedPatch = await normalizeAbsolutePatchPaths(
+    root,
+    worktree,
+    hunks,
+  );
+
+  validatePatchPaths(normalizedPatch.hunks);
 
-  return hunks;
+  return {
+    hunks: normalizedPatch.hunks,
+    pathsNormalized: normalizedPatch.changed,
+  };
 }
 
 async function readPreparedFileText(
@@ -288,7 +396,11 @@ export async function createPatchExecutionContext(
   patchText: string,
   worktree?: string,
 ): Promise<PatchExecutionContext> {
-  const hunks = parseValidatedPatch(patchText);
+  const { hunks, pathsNormalized } = await parseValidatedPatch(
+    root,
+    patchText,
+    worktree,
+  );
   await guardPatchTargets(root, worktree, collectPatchTargets(root, hunks));
   const files = createFileCacheContext();
   const staged = new Map<string, PreparedFileState>();
@@ -352,6 +464,7 @@ export async function createPatchExecutionContext(
 
   return {
     hunks,
+    pathsNormalized,
     staged,
     getPreparedFileState,
     assertPreparedPathMissing,

+ 52 - 0
src/hooks/apply-patch/hook.test.ts

@@ -91,6 +91,30 @@ PATCH`,
     );
   });
 
+  test('normaliza paths absolutos dentro del root antes del nativo', async () => {
+    const root = await createTempDir('apply-patch-hook-');
+    const absolutePath = path.join(root, 'sample.txt');
+    await writeFixture(root, 'sample.txt', 'alpha\nbeta\n');
+    const hook = createHook();
+    const patchText = `*** Begin Patch
+*** Update File: ${absolutePath}
+@@
+-alpha
++omega
+*** End Patch`;
+    const output = { args: { patchText } };
+
+    await hook['tool.execute.before'](
+      { tool: 'apply_patch', directory: root },
+      output,
+    );
+
+    expect(parsePatch(output.args.patchText as string).hunks[0]).toMatchObject({
+      type: 'update',
+      path: 'sample.txt',
+    });
+  });
+
   test('reescribe stale patch de prefijo y sigue siendo aplicable', async () => {
     const root = await createTempDir('apply-patch-hook-');
     await writeFixture(
@@ -557,6 +581,34 @@ garbage
     expect(await readFile(outside, 'utf-8')).toBe('outside\n');
   });
 
+  test('bloquea un path absoluto dentro del worktree pero fuera del root', async () => {
+    const worktree = await createTempDir('apply-patch-worktree-');
+    const root = path.join(worktree, 'subdir');
+    await mkdir(root, { recursive: true });
+    const siblingPath = path.join(worktree, 'shared.txt');
+    const hook = createApplyPatchHook({
+      client: {} as never,
+      directory: root,
+      worktree,
+    } as never);
+    const patchText = `*** Begin Patch
+*** Add File: ${siblingPath}
++fresh
+*** End Patch`;
+    const output = { args: { patchText } };
+
+    await expect(
+      hook['tool.execute.before'](
+        { tool: 'apply_patch', directory: root },
+        output,
+      ),
+    ).rejects.toThrow(
+      `apply_patch blocked: patch contains path outside workspace root: ${siblingPath}`,
+    );
+
+    expect(output.args.patchText).toBe(patchText);
+  });
+
   test('aborta temprano y no aplica nada si un patch mixto tiene rutas fuera', async () => {
     const root = await createTempDir('apply-patch-hook-');
     const outsideDir = await createTempDir('apply-patch-hook-outside-');

+ 124 - 20
src/hooks/apply-patch/operations.test.ts

@@ -353,11 +353,12 @@ garbage
     );
   });
 
-  test('preparePatchChanges rechaza un Update File con path absoluto como validation', async () => {
+  test('preparePatchChanges normaliza un Update File con path absoluto dentro del root', async () => {
     const root = await createTempDir();
     const absolutePath = path.join(root, 'sample.txt');
+    await writeFixture(root, 'sample.txt', 'alpha\nbeta\n');
 
-    const error = await preparePatchChanges(
+    const rewritten = await rewritePatch(
       root,
       `*** Begin Patch
 *** Update File: ${absolutePath}
@@ -366,42 +367,80 @@ garbage
 +omega
 *** End Patch`,
       DEFAULT_OPTIONS,
-    ).catch((caughtError) => caughtError);
-
-    expect(isApplyPatchValidationError(error)).toBeTrue();
-    expect(error).toBeInstanceOf(Error);
-    expect((error as Error).message).toBe(
-      `apply_patch validation failed: absolute patch paths are not allowed: ${absolutePath}`,
     );
+
+    expect(rewritten.changed).toBeTrue();
+    expect(rewritten.rewriteModes).toContain('normalize:patch-paths');
+    const [rewrittenHunk] = parsePatch(rewritten.patchText).hunks;
+    expect(rewrittenHunk.type).toBe('update');
+    expect(rewrittenHunk.path).toBe('sample.txt');
+
+    await expect(
+      preparePatchChanges(
+        root,
+        `*** Begin Patch
+*** Update File: ${absolutePath}
+@@
+-alpha
++omega
+*** End Patch`,
+        DEFAULT_OPTIONS,
+      ),
+    ).resolves.toEqual([
+      {
+        type: 'update',
+        file: absolutePath,
+        move: undefined,
+        text: 'omega\nbeta\n',
+      },
+    ]);
   });
 
-  test('preparePatchChanges rechaza un Add File con path absoluto como validation', async () => {
+  test('preparePatchChanges normaliza un Add File con path absoluto dentro del root', async () => {
     const root = await createTempDir();
     const absolutePath = path.join(root, 'added.txt');
 
-    const error = await preparePatchChanges(
+    const rewritten = await rewritePatch(
       root,
       `*** Begin Patch
 *** Add File: ${absolutePath}
 +fresh
 *** End Patch`,
       DEFAULT_OPTIONS,
-    ).catch((caughtError) => caughtError);
-
-    expect(isApplyPatchValidationError(error)).toBeTrue();
-    expect(error).toBeInstanceOf(Error);
-    expect((error as Error).message).toBe(
-      `apply_patch validation failed: absolute patch paths are not allowed: ${absolutePath}`,
     );
+
+    expect(rewritten.changed).toBeTrue();
+    expect(parsePatch(rewritten.patchText).hunks[0]).toMatchObject({
+      type: 'add',
+      path: 'added.txt',
+      contents: 'fresh',
+    });
+
+    await expect(
+      preparePatchChanges(
+        root,
+        `*** Begin Patch
+*** Add File: ${absolutePath}
++fresh
+*** End Patch`,
+        DEFAULT_OPTIONS,
+      ),
+    ).resolves.toEqual([
+      {
+        type: 'add',
+        file: absolutePath,
+        text: 'fresh\n',
+      },
+    ]);
   });
 
-  test('preparePatchChanges rechaza un Move to con path absoluto como validation', async () => {
+  test('preparePatchChanges normaliza un Move to con path absoluto dentro del root', async () => {
     const root = await createTempDir();
     const absoluteMovePath = path.join(root, 'nested/after.txt');
 
     await writeFixture(root, 'before.txt', 'alpha\nbeta\n');
 
-    const error = await preparePatchChanges(
+    const rewritten = await rewritePatch(
       root,
       `*** Begin Patch
 *** Update File: before.txt
@@ -412,12 +451,77 @@ garbage
 +BETA
 *** End Patch`,
       DEFAULT_OPTIONS,
+    );
+
+    expect(rewritten.changed).toBeTrue();
+    expect(parsePatch(rewritten.patchText).hunks[0]).toMatchObject({
+      type: 'update',
+      path: 'before.txt',
+      move_path: 'nested/after.txt',
+    });
+
+    await expect(
+      preparePatchChanges(
+        root,
+        `*** Begin Patch
+*** Update File: before.txt
+*** Move to: ${absoluteMovePath}
+@@
+ alpha
+-beta
++BETA
+*** End Patch`,
+        DEFAULT_OPTIONS,
+      ),
+    ).resolves.toEqual([
+      {
+        type: 'update',
+        file: path.join(root, 'before.txt'),
+        move: absoluteMovePath,
+        text: 'alpha\nBETA\n',
+      },
+    ]);
+  });
+
+  test('preparePatchChanges bloquea un path absoluto fuera del root/worktree', async () => {
+    const root = await createTempDir();
+    const outsidePath = path.join(path.dirname(root), 'outside.txt');
+
+    const error = await preparePatchChanges(
+      root,
+      `*** Begin Patch
+*** Add File: ${outsidePath}
++fresh
+*** End Patch`,
+      DEFAULT_OPTIONS,
     ).catch((caughtError) => caughtError);
 
-    expect(isApplyPatchValidationError(error)).toBeTrue();
+    expect(isApplyPatchBlockedError(error)).toBeTrue();
+    expect(error).toBeInstanceOf(Error);
+    expect((error as Error).message).toBe(
+      `apply_patch blocked: patch contains path outside workspace root: ${outsidePath}`,
+    );
+  });
+
+  test('preparePatchChanges bloquea un path absoluto dentro del worktree pero fuera del root', async () => {
+    const worktree = await createTempDir();
+    const root = path.join(worktree, 'subdir');
+    const siblingPath = path.join(worktree, 'shared.txt');
+
+    const error = await preparePatchChanges(
+      root,
+      `*** Begin Patch
+*** Add File: ${siblingPath}
++fresh
+*** End Patch`,
+      DEFAULT_OPTIONS,
+      worktree,
+    ).catch((caughtError) => caughtError);
+
+    expect(isApplyPatchBlockedError(error)).toBeTrue();
     expect(error).toBeInstanceOf(Error);
     expect((error as Error).message).toBe(
-      `apply_patch validation failed: absolute patch paths are not allowed: ${absoluteMovePath}`,
+      `apply_patch blocked: patch contains path outside workspace root: ${siblingPath}`,
     );
   });
 

+ 17 - 2
src/hooks/apply-patch/rewrite.ts

@@ -318,8 +318,13 @@ export async function rewritePatch(
   worktree?: string,
 ): Promise<RewritePatchResult> {
   try {
-    const { hunks, staged, getPreparedFileState, assertPreparedPathMissing } =
-      await createPatchExecutionContext(root, patchText, worktree);
+    const {
+      hunks,
+      pathsNormalized,
+      staged,
+      getPreparedFileState,
+      assertPreparedPathMissing,
+    } = await createPatchExecutionContext(root, patchText, worktree);
     const normalizedPatchText = normalizePatchText(patchText);
     const rewritten: PatchHunk[] = [];
     let changed = false;
@@ -485,6 +490,16 @@ export async function rewritePatch(
     }
 
     if (!changed) {
+      if (pathsNormalized) {
+        return {
+          patchText: formatPatch({ hunks }),
+          changed: true,
+          rewrittenChunks: 0,
+          totalChunks,
+          rewriteModes: ['normalize:patch-paths'],
+        };
+      }
+
       if (normalizedPatchText !== patchText) {
         return {
           patchText: normalizedPatchText,