History

fix(mcp): harden project creation boundaries

Julius Marminge committed 2 months ago

Commit 7e7069a1 parent e395ca911 files · +176 −61Browse files

Unified · Split

11 files
  1. apps/server/src/mcp/ProjectMcpService.test.ts +58 −0
  2. apps/server/src/mcp/ProjectMcpService.ts +34 −45
  3. apps/server/src/mcp/toolkits/project/tools.ts +1 −1
  4. apps/server/src/mcp/toolkits/worktree/registration.test.ts +6 −0
  5. apps/server/src/sourceControl/SourceControlRepositoryService.test.ts +1 −1
  6. apps/server/src/sourceControl/SourceControlRepositoryService.ts +1 −1
  7. packages/contracts/src/orchestration.ts +8 −6
  8. packages/contracts/src/project.test.ts +27 −0
  9. packages/contracts/src/project.ts +9 −3
  10. packages/contracts/src/projectMcp.test.ts +28 −1
  11. packages/contracts/src/projectMcp.ts +3 −3
apps/server/src/mcp/ProjectMcpService.test.ts+58−0
@@ -11,6 +11,7 @@ import { · 10 unmodified lines
1111 ProviderInstanceId,
1212 type ServerSettings,
1313 ServerSettingsError,
14+ SourceControlRepositoryError,
1415 ThreadId,
1516 } from "@t3tools/contracts";
1617 import * as Deferred from "effect/Deferred";
@@ -222,11 +223,68 @@ it.effect("loads response settings before committing project mutations", () => · 205 unmodified lines
222223 })
223224 .pipe(Effect.flip);
224225 expect(error.code).toBe("operation_failed");
226+ expect(error.message).toBe("Unable to read project defaults.");
227+ expect(error.message).not.toMatch(/settings unavailable|settings\.json/);
225228 expect(yield* Ref.get(createCalls)).toBe(0);
226229 }).pipe(Effect.provide(testLayer));
227230 }),
228231 );
229232
233+it.effect("redacts repository failures from public project MCP results", () =>
234+ Effect.gen(function* () {
235+ const secretRemote = "https://oauth-token@example.com/private/repo.git";
236+ const testLayer = ProjectMcp.layer.pipe(
237+ Layer.provide(
238+ Layer.mock(ProjectService.ProjectService)({
239+ getById: () => Effect.succeed(Option.none()),
240+ }),
241+ ),
242+ Layer.provide(Layer.mock(T3ProjectFileLoader.T3ProjectFileLoader)({})),
243+ Layer.provide(
244+ Layer.mock(ServerSettingsService.ServerSettingsService)({
245+ getSettings: Effect.succeed({ defaultThreadEnvMode: "local" } as ServerSettings),
246+ }),
247+ ),
248+ Layer.provide(
249+ Layer.mock(SourceControlRepositoryService.SourceControlRepositoryService)({
250+ cloneRepository: () =>
251+ Effect.fail(
252+ new SourceControlRepositoryError({
253+ provider: "unknown",
254+ operation: "cloneRepository",
255+ detail: `fatal: authentication failed for ${secretRemote}`,
256+ }),
257+ ),
258+ }),
259+ ),
260+ Layer.provide(Layer.mock(ThreadManagement.ThreadManagementService)({})),
261+ Layer.provide(NodeServices.layer),
262+ );
263+
264+ const error = yield* Effect.gen(function* () {
265+ const service = yield* ProjectMcp.ProjectMcpService;
266+ return yield* service
267+ .create(scope, {
268+ title: "Private clone",
269+ source: {
270+ type: "clone",
271+ destinationPath: "/work/private-clone",
272+ remoteUrl: secretRemote,
273+ },
274+ clientRequestId: "private-clone",
275+ })
276+ .pipe(Effect.flip);
277+ }).pipe(Effect.provide(testLayer));
278+
279+ expect(error).toMatchObject({
280+ code: "operation_failed",
281+ message: "Unable to clone the requested repository.",
282+ });
283+ expect(error.message).not.toMatch(/oauth-token|example\.com|fatal|authentication/i);
284+ expect(error).not.toHaveProperty("cause");
285+ }),
286+);
287+
230288 it.effect("clones before registration and requires explicit cascading for nonempty projects", () =>
231289 Effect.gen(function* () {
232290 const projectId = ProjectId.make("project:mcp:provider-session-project-mcp:clone-and-delete");
apps/server/src/mcp/ProjectMcpService.ts+34−45
@@ -58,8 +58,14 @@ function failure(code: ProjectMcpFailure["code"], message: string): ProjectMcpFa · 57 unmodified lines
5858 return new ProjectMcpFailure({ code, message });
5959 }
6060
61−function errorMessage(error: unknown): string {
62− return error instanceof Error ? error.message : String(error);
61+function redactOperationFailure(operation: string, publicMessage: string) {
62+ return <A, E, R>(effect: Effect.Effect<A, E, R>): Effect.Effect<A, ProjectMcpFailure, R> =>
63+ effect.pipe(
64+ Effect.tapError((error) =>
65+ Effect.logWarning("Project MCP operation failed.", { operation, error }),
66+ ),
67+ Effect.mapError(() => failure("operation_failed", publicMessage)),
68+ );
6369 }
6470
6571 function stablePart(value: string): string {
@@ -113,9 +119,7 @@ const make = Effect.gen(function* () { · 47 unmodified lines
113119 );
114120
115121 const loadSettings = serverSettings.getSettings.pipe(
116− Effect.mapError((error) =>
117− failure("operation_failed", `Unable to read server settings: ${errorMessage(error)}`),
118− ),
122+ redactOperationFailure("read-server-settings", "Unable to read project defaults."),
119123 );
120124
121125 const projectWorkspaceDefaults = Effect.fn("ProjectMcpService.projectWorkspaceDefaults")(
@@ -171,14 +175,7 @@ const make = Effect.gen(function* () { · 49 unmodified lines
171175 const loadProject = Effect.fn("ProjectMcpService.loadProject")(function* (projectId: ProjectId) {
172176 const project = yield* projects
173177 .getById(projectId)
174− .pipe(
175− Effect.mapError((error) =>
176− failure(
177− "operation_failed",
178− `Unable to read project ${projectId}: ${errorMessage(error)}`,
179− ),
180− ),
181− );
178+ .pipe(redactOperationFailure("read-project", "Unable to read the requested project."));
182179 if (Option.isNone(project)) {
183180 return yield* failure("project_not_found", `Project '${projectId}' was not found.`);
184181 }
@@ -189,9 +186,7 @@ const make = Effect.gen(function* () { · 4 unmodified lines
189186 Effect.gen(function* () {
190187 yield* requireCapability(scope);
191188 const snapshot = yield* projects.snapshot.pipe(
192− Effect.mapError((error) =>
193− failure("operation_failed", `Unable to list projects: ${errorMessage(error)}`),
194− ),
189+ redactOperationFailure("list-projects", "Unable to list projects."),
195190 );
196191 const settings = yield* loadSettings;
197192 const cursor = input.cursor ?? 0;
@@ -230,11 +225,9 @@ const make = Effect.gen(function* () { · 32 unmodified lines
230225 const existing = yield* projects
231226 .getById(projectId, { includeDeleted: true })
232227 .pipe(
233− Effect.mapError((error) =>
234− failure(
235− "operation_failed",
236− `Unable to retry project creation: ${errorMessage(error)}`,
237− ),
228+ redactOperationFailure(
229+ "read-project-create-receipt",
230+ "Unable to check the project creation result.",
238231 ),
239232 );
240233 if (Option.isSome(existing)) {
@@ -276,8 +269,9 @@ const make = Effect.gen(function* () { · 35 unmodified lines
276269 ...(input.source.protocol === undefined ? {} : { protocol: input.source.protocol }),
277270 })
278271 .pipe(
279− Effect.mapError((error) =>
280− failure("operation_failed", `Unable to clone project: ${errorMessage(error)}`),
272+ redactOperationFailure(
273+ "clone-project-repository",
274+ "Unable to clone the requested repository.",
281275 ),
282276 );
283277 workspaceRoot = cloned.cwd;
@@ -300,9 +294,7 @@ const make = Effect.gen(function* () { · 16 unmodified lines
300294 ...(input.scripts === undefined ? {} : { scripts: input.scripts }),
301295 })
302296 .pipe(
303− Effect.mapError((error) =>
304− failure("operation_failed", `Unable to create project: ${errorMessage(error)}`),
305− ),
297+ redactOperationFailure("create-project", "Unable to create the requested project."),
306298 );
307299 return yield* projectView(project, settings);
308300 }),
@@ -345,10 +337,16 @@ const make = Effect.gen(function* () { · 36 unmodified lines
345337 ...(input.scripts === undefined ? {} : { scripts: input.scripts }),
346338 })
347339 .pipe(
340+ Effect.tapError((error) =>
341+ Effect.logWarning("Project MCP operation failed.", {
342+ operation: "update-project",
343+ error,
344+ }),
345+ ),
348346 Effect.mapError((error) =>
349347 error._tag === "ProjectNotFoundError"
350− ? failure("project_not_found", error.message)
351− : failure("operation_failed", `Unable to update project: ${errorMessage(error)}`),
348+ ? failure("project_not_found", `Project '${input.projectId}' was not found.`)
349+ : failure("operation_failed", "Unable to update the requested project."),
352350 ),
353351 );
354352 return yield* projectView(project, settings);
@@ -359,14 +357,7 @@ const make = Effect.gen(function* () { · 4 unmodified lines
359357 yield* requireCapability(scope);
360358 const projectOption = yield* projects
361359 .getById(input.projectId, { includeDeleted: true })
362− .pipe(
363− Effect.mapError((error) =>
364− failure(
365− "operation_failed",
366− `Unable to read project ${input.projectId}: ${errorMessage(error)}`,
367− ),
368− ),
369− );
360+ .pipe(redactOperationFailure("read-project-for-delete", "Unable to read the project."));
370361 if (Option.isNone(projectOption)) {
371362 return yield* failure("project_not_found", `Project '${input.projectId}' was not found.`);
372363 }
@@ -385,8 +376,9 @@ const make = Effect.gen(function* () { · 12 unmodified lines
385376 threads.getShellSnapshot({ location: "active" }),
386377 threads.getShellSnapshot({ location: "archive" }),
387378 ]).pipe(
388− Effect.mapError((error) =>
389− failure("operation_failed", `Unable to inspect project threads: ${errorMessage(error)}`),
379+ redactOperationFailure(
380+ "inspect-project-threads",
381+ "Unable to inspect the project's threads.",
390382 ),
391383 );
392384 const projectThreads = [
@@ -423,8 +415,9 @@ const make = Effect.gen(function* () { · 30 unmodified lines
423415 }),
424416 { concurrency: 1, discard: true },
425417 ).pipe(
426− Effect.mapError((error) =>
427− failure("operation_failed", `Unable to delete project threads: ${errorMessage(error)}`),
418+ redactOperationFailure(
419+ "delete-project-threads",
420+ "Unable to delete the project's thread records.",
428421 ),
429422 );
430423 yield* projects
@@ -437,11 +430,7 @@ const make = Effect.gen(function* () { · 6 unmodified lines
437430 }),
438431 projectId: input.projectId,
439432 })
440− .pipe(
441− Effect.mapError((error) =>
442− failure("operation_failed", `Unable to delete project: ${errorMessage(error)}`),
443− ),
444− );
433+ .pipe(redactOperationFailure("delete-project", "Unable to delete the requested project."));
445434 return {
446435 projectId: input.projectId,
447436 deleted: true,
apps/server/src/mcp/toolkits/project/tools.ts+1−1
@@ -58,7 +58,7 @@ export const ProjectCreateTool = Tool.make("t3_project_create", { · 57 unmodified lines
5858 .annotate(Tool.Title, "Create a T3 project")
5959 .annotate(Tool.Readonly, false)
6060 .annotate(Tool.Destructive, false)
61− .annotate(Tool.Idempotent, true)
61+ .annotate(Tool.Idempotent, false)
6262 .annotate(Tool.OpenWorld, true);
6363
6464 export const ProjectUpdateTool = Tool.make("t3_project_update", {
apps/server/src/mcp/toolkits/worktree/registration.test.ts+6−0
@@ -118,6 +118,12 @@ it.effect("production mcp layer lists worktree tools over http", () => · 117 unmodified lines
118118 expect(toolNames).toContain("delegate_task");
119119 expect(toolNames).toContain("t3_project_list");
120120 expect(toolNames).toContain("t3_project_delete");
121+ const projectCreate = tools.find((tool) => tool.name === "t3_project_create");
122+ expect(
123+ projectCreate?.annotations && "idempotentHint" in projectCreate.annotations
124+ ? projectCreate.annotations.idempotentHint
125+ : undefined,
126+ ).not.toBe(true);
121127
122128 // The handoff tool mutates thread state, reaches the network (origin
123129 // fetch), and runs project setup scripts, so its MCP hints must not
apps/server/src/sourceControl/SourceControlRepositoryService.test.ts+1−1
@@ -209,7 +209,7 @@ it.effect("clones a looked-up repository into the requested destination", () => · 208 unmodified lines
209209 assert.deepStrictEqual(cloneCalls, [
210210 {
211211 cwd: `${parent}/missing/nested`,
212− args: ["clone", CLONE_URLS.url, "t3code"],
212+ args: ["clone", "--origin", "origin", CLONE_URLS.url, "t3code"],
213213 },
214214 ]);
215215 }).pipe(
apps/server/src/sourceControl/SourceControlRepositoryService.ts+1−1
@@ -310,7 +310,7 @@ export const make = Effect.gen(function* () { · 309 unmodified lines
310310 yield* git.execute({
311311 operation: "SourceControlRepositoryService.cloneRepository",
312312 cwd: preparedDestination.parentPath,
313− args: ["clone", remoteUrl, preparedDestination.directoryName],
313+ args: ["clone", "--origin", "origin", remoteUrl, preparedDestination.directoryName],
314314 timeoutMs: 120_000,
315315 maxOutputBytes: 256 * 1024,
316316 });
packages/contracts/src/orchestration.ts+8−6
@@ -26,7 +26,13 @@ import { · 25 unmodified lines
2626 RuntimeMode,
2727 } from "./providerPolicy.ts";
2828 import { ProviderInstanceId } from "./providerInstance.ts";
29−import { ProjectScript, ProjectIconOverride } from "./project.ts";
29+import {
30+ Project,
31+ ProjectFaviconPath,
32+ ProjectIconOverride,
33+ ProjectScript,
34+ ProjectScriptIcon,
35+} from "./project.ts";
3036 export { ProjectIconColor, ProjectIconOverride } from "./project.ts";
3137 import { RepositoryIdentity, ThreadEnvMode } from "./environment.ts";
3238 import { OrchestrationProjectShell } from "./orchestrationProject.ts";
@@ -61,11 +67,7 @@ export { · 28 unmodified lines
6167 export const CorrelationId = CommandId;
6268 export type CorrelationId = typeof CorrelationId.Type;
6369
64−export const ProjectFaviconPath = TrimmedNonEmptyString.check(
65− Schema.isMaxLength(1024),
66− Schema.isPattern(/\.(?:avif|gif|ico|jpe?g|png|svg|webp)$/i),
67−);
68−export type ProjectFaviconPath = typeof ProjectFaviconPath.Type;
70+export { ProjectFaviconPath } from "./project.ts";
6971
7072 export const OrchestrationProject = Schema.Struct({
7173 id: ProjectId,
packages/contracts/src/project.test.ts+27−0
@@ -3,6 +3,7 @@ import { describe, expect, it } from "vite-plus/test"; · 2 unmodified lines
33
44 import {
55 ProjectReadFileError,
6+ ProjectMutation,
67 ProjectSearchContentsError,
78 ProjectSearchContentsInput,
89 ProjectSearchEntriesError,
@@ -12,6 +13,7 @@ import { · 3 unmodified lines
1213
1314 const decodeSearchEntriesInput = Schema.decodeUnknownSync(ProjectSearchEntriesInput);
1415 const decodeSearchContentsInput = Schema.decodeUnknownSync(ProjectSearchContentsInput);
16+const decodeProjectMutation = Schema.decodeUnknownSync(ProjectMutation);
1517
1618 describe("project search inputs", () => {
1719 it("allows an empty entries query for bounded frecency browsing", () => {
@@ -37,6 +39,31 @@ describe("project search inputs", () => { · 19 unmodified lines
3739 });
3840 });
3941
42+describe("project mutations", () => {
43+ it("rejects favicon paths the asset endpoint cannot serve", () => {
44+ const created = decodeProjectMutation({
45+ type: "project.create",
46+ commandId: "command-project-create",
47+ projectId: "project-1",
48+ title: "Project",
49+ workspaceRoot: "/work/project",
50+ faviconPath: "/work/project/favicon.png",
51+ });
52+ expect(created).toMatchObject({
53+ type: "project.create",
54+ faviconPath: "/work/project/favicon.png",
55+ });
56+ expect(() =>
57+ decodeProjectMutation({
58+ type: "project.update",
59+ commandId: "command-project-update",
60+ projectId: "project-1",
61+ faviconPath: "/work/project/.env",
62+ }),
63+ ).toThrow();
64+ });
65+});
66+
4067 describe("project RPC errors", () => {
4168 it("derives stable messages from structured request context while retaining causes", () => {
4269 const cause = new Error("sensitive platform detail");
packages/contracts/src/project.ts+9−3
@@ -16,6 +16,12 @@ const PROJECT_SEARCH_CONTENTS_MAX_LIMIT = 500; · 15 unmodified lines
1616 const PROJECT_WRITE_FILE_PATH_MAX_LENGTH = 512;
1717 const PROJECT_READ_FILE_PATH_MAX_LENGTH = 512;
1818
19+export const ProjectFaviconPath = TrimmedNonEmptyString.check(
20+ Schema.isMaxLength(1024),
21+ Schema.isPattern(/\.(?:avif|gif|ico|jpe?g|png|svg|webp)$/i),
22+);
23+export type ProjectFaviconPath = typeof ProjectFaviconPath.Type;
24+
1925 export const ProjectScriptIcon = Schema.Literals([
2026 "play",
2127 "test",
@@ -84,7 +90,7 @@ export const Project = Schema.Struct({ · 62 unmodified lines
8490 title: TrimmedNonEmptyString,
8591 workspaceRoot: TrimmedNonEmptyString,
8692 repositoryIdentity: Schema.optional(Schema.NullOr(RepositoryIdentity)),
87− faviconPath: Schema.optional(Schema.NullOr(TrimmedNonEmptyString)),
93+ faviconPath: Schema.optional(Schema.NullOr(ProjectFaviconPath)),
8894 projectIcon: Schema.optional(Schema.NullOr(ProjectIconOverride)),
8995 defaultModelSelection: Schema.NullOr(ModelSelection),
9096 defaultThreadEnvMode: Schema.optional(Schema.NullOr(ThreadEnvMode)),
@@ -123,7 +129,7 @@ export const ProjectMutation = Schema.Union([ · 32 unmodified lines
123129 createWorkspaceRootIfMissing: Schema.optional(Schema.Boolean),
124130 defaultModelSelection: Schema.optional(Schema.NullOr(ModelSelection)),
125131 defaultThreadEnvMode: Schema.optional(Schema.NullOr(ThreadEnvMode)),
126− faviconPath: Schema.optional(Schema.NullOr(TrimmedNonEmptyString)),
132+ faviconPath: Schema.optional(Schema.NullOr(ProjectFaviconPath)),
127133 scripts: Schema.optional(Schema.Array(ProjectScript)),
128134 }),
129135 Schema.Struct({
@@ -135,7 +141,7 @@ export const ProjectMutation = Schema.Union([ · 5 unmodified lines
135141 defaultModelSelection: Schema.optional(Schema.NullOr(ModelSelection)),
136142 autoPull: Schema.optional(Schema.Boolean),
137143 projectIcon: Schema.optional(Schema.NullOr(ProjectIconOverride)),
138− faviconPath: Schema.optional(Schema.NullOr(TrimmedNonEmptyString)),
144+ faviconPath: Schema.optional(Schema.NullOr(ProjectFaviconPath)),
139145 defaultThreadEnvMode: Schema.optional(Schema.NullOr(ThreadEnvMode)),
140146 scripts: Schema.optional(Schema.Array(ProjectScript)),
141147 }),
packages/contracts/src/projectMcp.test.ts+28−1
@@ -1,10 +1,16 @@
11 import { describe, expect, it } from "@effect/vitest";
22 import * as Schema from "effect/Schema";
33
4−import { ProjectMcpCreateInput, ProjectMcpFailure, ProjectMcpListInput } from "./projectMcp.ts";
4+import {
5+ ProjectMcpCreateInput,
6+ ProjectMcpFailure,
7+ ProjectMcpListInput,
8+ ProjectMcpUpdateInput,
9+} from "./projectMcp.ts";
510
611 const decodeCreateInput = Schema.decodeUnknownSync(ProjectMcpCreateInput);
712 const decodeListInput = Schema.decodeUnknownSync(ProjectMcpListInput);
13+const decodeUpdateInput = Schema.decodeUnknownSync(ProjectMcpUpdateInput);
814 const decodeFailure = Schema.decodeUnknownSync(ProjectMcpFailure);
915
1016 describe("project MCP contracts", () => {
@@ -71,4 +77,25 @@ describe("project MCP contracts", () => { · 60 unmodified lines
7177 }),
7278 ).toMatchObject({ code: "project_deleted" });
7379 });
80+
81+ it("accepts only favicon paths served by the project asset route", () => {
82+ expect(
83+ decodeCreateInput({
84+ title: "Icon project",
85+ source: { type: "existing_directory", workspaceRoot: "/work/icon" },
86+ faviconPath: "/work/icon/favicon.svg",
87+ }).faviconPath,
88+ ).toBe("/work/icon/favicon.svg");
89+ expect(() =>
90+ decodeCreateInput({
91+ title: "Secret project",
92+ source: { type: "existing_directory", workspaceRoot: "/work/secret" },
93+ faviconPath: "/work/secret/.env",
94+ }),
95+ ).toThrow();
96+ expect(decodeUpdateInput({ projectId: "project-1", faviconPath: null }).faviconPath).toBeNull();
97+ expect(() =>
98+ decodeUpdateInput({ projectId: "project-1", faviconPath: "/work/secret/.env" }),
99+ ).toThrow();
100+ });
74101 });
packages/contracts/src/projectMcp.ts+3−3
@@ -3,7 +3,7 @@ import * as Schema from "effect/Schema"; · 2 unmodified lines
33 import { NonNegativeInt, PositiveInt, ProjectId, TrimmedNonEmptyString } from "./baseSchemas.ts";
44 import { ThreadEnvMode } from "./environment.ts";
55 import { ModelSelection } from "./modelSelection.ts";
6−import { Project, ProjectScript } from "./project.ts";
6+import { Project, ProjectFaviconPath, ProjectScript } from "./project.ts";
77 import { SourceControlCloneProtocol, SourceControlProviderKind } from "./sourceControl.ts";
88
99 const ProjectMcpClientRequestId = TrimmedNonEmptyString.check(Schema.isMaxLength(256)).annotate({
@@ -91,7 +91,7 @@ export const ProjectMcpCreateInput = Schema.Struct({ · 81 unmodified lines
9191 source: ProjectMcpCreateSource,
9292 defaultModelSelection: Schema.optional(Schema.NullOr(ModelSelection)),
9393 defaultThreadEnvMode: Schema.optional(Schema.NullOr(ThreadEnvMode)),
94− faviconPath: Schema.optional(Schema.NullOr(TrimmedNonEmptyString)),
94+ faviconPath: Schema.optional(Schema.NullOr(ProjectFaviconPath)),
9595 scripts: Schema.optional(Schema.Array(ProjectScript)),
9696 clientRequestId: Schema.optional(ProjectMcpClientRequestId),
9797 });
@@ -103,7 +103,7 @@ export const ProjectMcpUpdateInput = Schema.Struct({ · 5 unmodified lines
103103 workspaceRoot: Schema.optional(TrimmedNonEmptyString),
104104 defaultModelSelection: Schema.optional(Schema.NullOr(ModelSelection)),
105105 defaultThreadEnvMode: Schema.optional(Schema.NullOr(ThreadEnvMode)),
106− faviconPath: Schema.optional(Schema.NullOr(TrimmedNonEmptyString)),
106+ faviconPath: Schema.optional(Schema.NullOr(ProjectFaviconPath)),
107107 scripts: Schema.optional(Schema.Array(ProjectScript)),
108108 clientRequestId: Schema.optional(ProjectMcpClientRequestId),
109109 });

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t