From 90cb91489ed180ef2b58855dfffb2ff6064b412a Mon Sep 17 00:00:00 2001 From: Roy Date: Sun, 26 Jul 2026 16:36:47 +0000 Subject: [PATCH 1/3] fix(coding-agent): safely glob memory directories --- packages/coding-agent/CHANGELOG.md | 4 ++ .../src/internal-urls/memory-protocol.ts | 66 +++++++++++++++++-- .../src/internal-urls/skill-protocol.ts | 7 +- packages/coding-agent/src/tools/glob.ts | 19 +++++- .../internal-urls/memory-protocol.test.ts | 56 ++++++++++++++++ 5 files changed, 144 insertions(+), 8 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e4352d056..ee6546e59 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `glob` rejecting safe `memory://root//**` patterns. Memory globs now resolve their directory prefix inside the project memory root while rejecting traversal and percent-encoded path separators across the complete glob path. + ## [17.1.4] - 2026-07-26 ### Added diff --git a/packages/coding-agent/src/internal-urls/memory-protocol.ts b/packages/coding-agent/src/internal-urls/memory-protocol.ts index 789ae82b4..7d5e4317e 100644 --- a/packages/coding-agent/src/internal-urls/memory-protocol.ts +++ b/packages/coding-agent/src/internal-urls/memory-protocol.ts @@ -6,6 +6,7 @@ import { getMnemopiSessionState, type MnemopiScopedMemoryHit, type MnemopiSessio import { AgentRegistry } from "../registry/agent-registry"; import { isMarkdownPath } from "../utils/lang-from-path"; import { buildDirectoryResource } from "./filesystem-resource"; +import { parseInternalUrl } from "./parse"; import { validateRelativePath } from "./skill-protocol"; import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext, UrlCompletion } from "./types"; @@ -45,6 +46,57 @@ function toMemoryValidationError(error: unknown): Error { return new Error(message.replace("skill://", "memory://")); } +export interface MemoryGlobPattern { + baseUrl: string; + globPattern: string; +} + +/** + * Split a memory:// glob at its first wildcard after validating the complete + * decoded path. The suffix is validated before filesystem globbing so `..` + * cannot escape a safely resolved base directory. + */ +export function splitMemoryGlobPattern(input: string): MemoryGlobPattern { + const url = parseInternalUrl(input); + const namespace = url.rawHost || url.hostname; + if (url.protocol !== "memory:" || namespace !== MEMORY_NAMESPACE) { + throw new Error(`Memory glob patterns require the ${MEMORY_NAMESPACE} namespace: ${input}`); + } + + const rawPathname = url.rawPathname ?? url.pathname; + if (/%(?:2f|5c)/i.test(rawPathname)) { + throw new Error(`Encoded path separators are not allowed in memory:// glob patterns: ${input}`); + } + + let relativePath: string; + try { + relativePath = decodeURIComponent(rawPathname.replace(/^\//, "")); + } catch { + throw new Error(`Invalid URL encoding in memory:// path: ${input}`); + } + + try { + validateRelativePath(relativePath); + } catch (error) { + throw toMemoryValidationError(error); + } + + const decodedSegments = relativePath.split("/"); + const firstGlobIndex = decodedSegments.findIndex(segment => + ["*", "?", "[", "{"].some(char => segment.includes(char)), + ); + if (firstGlobIndex === -1) { + throw new Error(`memory:// URL does not contain a glob pattern: ${input}`); + } + + const rawSegments = rawPathname.replace(/^\//, "").split("/"); + const rawBasePath = rawSegments.slice(0, firstGlobIndex).join("/") || "."; + return { + baseUrl: `memory://${namespace}/${rawBasePath}`, + globPattern: decodedSegments.slice(firstGlobIndex).join("/"), + }; +} + /** * Resolve a memory:// URL to an absolute filesystem path under memory root. */ @@ -91,12 +143,14 @@ async function tryResolveInRoot(url: InternalUrl, memoryRoot: string): Promise { ); } if (hasGlobPathChars(rawPattern)) { - throw new ToolError(`Glob patterns are not supported for internal URLs: ${rawPattern}`); + if (!/^memory:\/\//i.test(rawPattern)) { + throw new ToolError(`Glob patterns are not supported for internal URLs: ${rawPattern}`); + } + const memoryGlob = splitMemoryGlobPattern(rawPattern); + const resource = await internalRouter.resolve(memoryGlob.baseUrl, { + cwd: this.session.cwd, + settings: this.session.settings, + signal, + localProtocolOptions: this.session.localProtocolOptions, + skills: this.session.skills, + pathOnly: true, + }); + if (!resource.sourcePath) { + throw new ToolError(`Cannot find internal URL without a backing file: ${memoryGlob.baseUrl}`); + } + normalizedPatterns.push(path.join(resource.sourcePath, memoryGlob.globPattern)); + continue; } const resource = await internalRouter.resolve(rawPattern, { cwd: this.session.cwd, diff --git a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts index b25412340..f367ce8ea 100644 --- a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts @@ -3,6 +3,7 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { getMemoryRoot } from "@oh-my-pi/pi-coding-agent/memories"; import { loadMnemopi, @@ -12,6 +13,8 @@ import { } from "@oh-my-pi/pi-coding-agent/mnemopi/state"; import { AgentRegistry } from "@oh-my-pi/pi-coding-agent/registry/agent-registry"; import type { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { GlobTool } from "@oh-my-pi/pi-coding-agent/tools/glob"; import { getAgentDir, removeWithRetries, setAgentDir, TempDir } from "@oh-my-pi/pi-utils"; // Mnemopi state is loaded lazily; preload so `new MnemopiSessionState(...)` can @@ -55,6 +58,17 @@ async function withMemoryFixture(fn: (fixture: MemoryFixture) => Promise): } } +function createGlobTool(cwd: string): GlobTool { + const session: ToolSession = { + cwd, + hasUI: false, + settings: Settings.isolated({}), + getSessionFile: () => null, + getSessionSpawns: () => null, + }; + return new GlobTool(session); +} + describe("MemoryProtocolHandler", () => { beforeEach(() => { AgentRegistry.resetGlobalForTests(); @@ -234,6 +248,48 @@ describe("MemoryProtocolHandler", () => { }); }); + it("globs nested directories within the memory root", async () => { + await withMemoryFixture(async ({ cwd, memoryRoot }) => { + const nestedSkill = path.join(memoryRoot, "skills", "demo", "nested", "SKILL.md"); + await fs.mkdir(path.dirname(nestedSkill), { recursive: true }); + await Bun.write(nestedSkill, "nested skill"); + await Bun.write(path.join(memoryRoot, "skills", "demo", "notes.txt"), "not markdown"); + + const tool = createGlobTool(cwd); + const result = await tool.execute("memory-glob", { path: "memory://root/skills/**/*.md" }); + + expect(result.details?.files).toHaveLength(1); + expect(result.details?.files?.[0]).toEndWith("/skills/demo/nested/SKILL.md"); + + const rootResult = await tool.execute("memory-root-glob", { path: "memory://root/**/*.md" }); + + expect(rootResult.details?.files).toHaveLength(1); + expect(rootResult.details?.files?.[0]).toEndWith("/skills/demo/nested/SKILL.md"); + }); + }); + + it.each([ + "memory://root/skills/**/../*.md", + "memory://root/skills/**/%2e%2e/*.md", + ])("rejects traversal in a memory glob suffix: %s", async pattern => { + await withMemoryFixture(async ({ cwd }) => { + await expect(createGlobTool(cwd).execute("memory-glob-traversal", { path: pattern })).rejects.toThrow( + /traversal/i, + ); + }); + }); + + it.each([ + "memory://root/skills/**/demo%2fnested/*.md", + "memory://root/skills/**/demo%5cnested/*.md", + ])("rejects encoded separators in a memory glob suffix: %s", async pattern => { + await withMemoryFixture(async ({ cwd }) => { + await expect(createGlobTool(cwd).execute("memory-glob-separator", { path: pattern })).rejects.toThrow( + /encoded path separator/i, + ); + }); + }); + it("throws clear error for missing files", async () => { await withMemoryFixture(async () => { const router = InternalUrlRouter.instance(); From 6b65fd29963e92f6f754a2556e0cb3f0d8e3e23b Mon Sep 17 00:00:00 2001 From: usr-bin-roygbiv Date: Sun, 26 Jul 2026 20:58:33 +0000 Subject: [PATCH 2/3] fix(coding-agent): preserve question-mark memory globs --- .../src/internal-urls/memory-protocol.ts | 11 +++- .../internal-urls/memory-protocol.test.ts | 54 ++++++++++++------- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/packages/coding-agent/src/internal-urls/memory-protocol.ts b/packages/coding-agent/src/internal-urls/memory-protocol.ts index 7d5e4317e..95823fe0f 100644 --- a/packages/coding-agent/src/internal-urls/memory-protocol.ts +++ b/packages/coding-agent/src/internal-urls/memory-protocol.ts @@ -57,13 +57,20 @@ export interface MemoryGlobPattern { * cannot escape a safely resolved base directory. */ export function splitMemoryGlobPattern(input: string): MemoryGlobPattern { - const url = parseInternalUrl(input); + const urlMatch = input.match(/^([a-z][a-z0-9+.-]*:\/\/[^/?#]*)(\/.*)?$/i); + if (!urlMatch) { + throw new Error(`Invalid memory glob URL: ${input}`); + } + + // Parse only the scheme and authority. A literal `?` in the path is glob + // syntax, not a query delimiter, and must survive unchanged. + const url = parseInternalUrl(urlMatch[1]); const namespace = url.rawHost || url.hostname; if (url.protocol !== "memory:" || namespace !== MEMORY_NAMESPACE) { throw new Error(`Memory glob patterns require the ${MEMORY_NAMESPACE} namespace: ${input}`); } - const rawPathname = url.rawPathname ?? url.pathname; + const rawPathname = urlMatch[2] ?? ""; if (/%(?:2f|5c)/i.test(rawPathname)) { throw new Error(`Encoded path separators are not allowed in memory:// glob patterns: ${input}`); } diff --git a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts index f367ce8ea..fddfe96e7 100644 --- a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts @@ -2,8 +2,8 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { getMemoryRoot } from "@oh-my-pi/pi-coding-agent/memories"; import { loadMnemopi, @@ -268,27 +268,43 @@ describe("MemoryProtocolHandler", () => { }); }); - it.each([ - "memory://root/skills/**/../*.md", - "memory://root/skills/**/%2e%2e/*.md", - ])("rejects traversal in a memory glob suffix: %s", async pattern => { - await withMemoryFixture(async ({ cwd }) => { - await expect(createGlobTool(cwd).execute("memory-glob-traversal", { path: pattern })).rejects.toThrow( - /traversal/i, - ); + it("preserves literal question-mark wildcards in memory globs", async () => { + await withMemoryFixture(async ({ cwd, memoryRoot }) => { + const skillsDir = path.join(memoryRoot, "skills"); + await fs.mkdir(skillsDir, { recursive: true }); + await Bun.write(path.join(skillsDir, "a.md"), "single character"); + await Bun.write(path.join(skillsDir, "ab.md"), "two characters"); + + const result = await createGlobTool(cwd).execute("memory-question-glob", { + path: "memory://root/skills/?.md", + }); + + expect(result.details?.files).toHaveLength(1); + expect(result.details?.files?.[0]).toEndWith("/skills/a.md"); }); }); - it.each([ - "memory://root/skills/**/demo%2fnested/*.md", - "memory://root/skills/**/demo%5cnested/*.md", - ])("rejects encoded separators in a memory glob suffix: %s", async pattern => { - await withMemoryFixture(async ({ cwd }) => { - await expect(createGlobTool(cwd).execute("memory-glob-separator", { path: pattern })).rejects.toThrow( - /encoded path separator/i, - ); - }); - }); + it.each(["memory://root/skills/**/../*.md", "memory://root/skills/**/%2e%2e/*.md"])( + "rejects traversal in a memory glob suffix: %s", + async pattern => { + await withMemoryFixture(async ({ cwd }) => { + await expect(createGlobTool(cwd).execute("memory-glob-traversal", { path: pattern })).rejects.toThrow( + /traversal/i, + ); + }); + }, + ); + + it.each(["memory://root/skills/**/demo%2fnested/*.md", "memory://root/skills/**/demo%5cnested/*.md"])( + "rejects encoded separators in a memory glob suffix: %s", + async pattern => { + await withMemoryFixture(async ({ cwd }) => { + await expect(createGlobTool(cwd).execute("memory-glob-separator", { path: pattern })).rejects.toThrow( + /encoded path separator/i, + ); + }); + }, + ); it("throws clear error for missing files", async () => { await withMemoryFixture(async () => { From 49d5145ffff5db60a52c8dc9f6c354b305c2eaeb Mon Sep 17 00:00:00 2001 From: usr-bin-roygbiv Date: Sun, 26 Jul 2026 21:33:13 +0000 Subject: [PATCH 3/3] fix(coding-agent): preserve encoded memory glob bases --- .../src/internal-urls/memory-protocol.ts | 8 +++----- packages/coding-agent/src/tools/glob.ts | 4 +++- .../test/internal-urls/memory-protocol.test.ts | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/src/internal-urls/memory-protocol.ts b/packages/coding-agent/src/internal-urls/memory-protocol.ts index 95823fe0f..4653b7514 100644 --- a/packages/coding-agent/src/internal-urls/memory-protocol.ts +++ b/packages/coding-agent/src/internal-urls/memory-protocol.ts @@ -88,15 +88,13 @@ export function splitMemoryGlobPattern(input: string): MemoryGlobPattern { throw toMemoryValidationError(error); } - const decodedSegments = relativePath.split("/"); - const firstGlobIndex = decodedSegments.findIndex(segment => - ["*", "?", "[", "{"].some(char => segment.includes(char)), - ); + const rawSegments = rawPathname.replace(/^\//, "").split("/"); + const firstGlobIndex = rawSegments.findIndex(segment => ["*", "?", "[", "{"].some(char => segment.includes(char))); if (firstGlobIndex === -1) { throw new Error(`memory:// URL does not contain a glob pattern: ${input}`); } - const rawSegments = rawPathname.replace(/^\//, "").split("/"); + const decodedSegments = relativePath.split("/"); const rawBasePath = rawSegments.slice(0, firstGlobIndex).join("/") || "."; return { baseUrl: `memory://${namespace}/${rawBasePath}`, diff --git a/packages/coding-agent/src/tools/glob.ts b/packages/coding-agent/src/tools/glob.ts index 6a15e538e..494b186d4 100644 --- a/packages/coding-agent/src/tools/glob.ts +++ b/packages/coding-agent/src/tools/glob.ts @@ -192,7 +192,9 @@ export class GlobTool implements AgentTool { if (!resource.sourcePath) { throw new ToolError(`Cannot find internal URL without a backing file: ${memoryGlob.baseUrl}`); } - normalizedPatterns.push(path.join(resource.sourcePath, memoryGlob.globPattern)); + normalizedPatterns.push( + path.join(resource.sourcePath.replace(/[*?[{]/g, "[$&]"), memoryGlob.globPattern), + ); continue; } const resource = await internalRouter.resolve(rawPattern, { diff --git a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts index fddfe96e7..d3a3821fb 100644 --- a/packages/coding-agent/test/internal-urls/memory-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/memory-protocol.test.ts @@ -284,6 +284,21 @@ describe("MemoryProtocolHandler", () => { }); }); + it("resolves encoded literal glob characters before the wildcard boundary", async () => { + await withMemoryFixture(async ({ cwd, memoryRoot }) => { + const encodedLiteralDir = path.join(memoryRoot, "skills", "[demo]"); + await fs.mkdir(encodedLiteralDir, { recursive: true }); + await Bun.write(path.join(encodedLiteralDir, "SKILL.md"), "encoded literal directory"); + + const result = await createGlobTool(cwd).execute("memory-encoded-literal-glob", { + path: "memory://root/skills/%5Bdemo%5D/*.md", + }); + + expect(result.details?.files).toHaveLength(1); + expect(result.details?.files?.[0]).toEndWith("/skills/[demo]/SKILL.md"); + }); + }); + it.each(["memory://root/skills/**/../*.md", "memory://root/skills/**/%2e%2e/*.md"])( "rejects traversal in a memory glob suffix: %s", async pattern => {