From cb8c992af334bcee07efe855467f945048457297 Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 2 Nov 2026 14:44:30 -0600 Subject: [PATCH] Fix expired live MCP publication repair links Renew only the authorized live attachment repair expiry, preserving immutable identity and verified cleanup. Cover actual repair PUT, replay, mismatched ownership and overlapping deletion. Carry the exact controller identity accessor and regression from upstream 026fd9e34520a29cf8df1e224afc050e19009914 (#18649); original CI two-second deadline cause remains undetermined. Co-Authored-By: Paperclip --- .../__tests__/idle-completed-history.test.ts | 116 +++++++++++++++++- server/src/server-info.ts | 5 + .../native-controller-identity.test.ts | 37 ++++++ .../native-runtime/native-restart-recovery.ts | 4 +- .../src/services/public-mcp/file-transfers.ts | 20 +++ 5 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 server/src/services/native-runtime/native-controller-identity.test.ts diff --git a/server/src/__tests__/idle-completed-history.test.ts b/server/src/__tests__/idle-completed-history.test.ts index 4e86383c51b..d79057fa43e 100644 --- a/server/src/__tests__/idle-completed-history.test.ts +++ b/server/src/__tests__/idle-completed-history.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import net from "node:net"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { eq, sql } from "drizzle-orm"; import * as tables from "@paperclipai/db"; import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; @@ -235,6 +235,120 @@ const support = externalDatabaseUrl ? { supported: true } : await getEmbeddedPos } finally { await p.cleanup(); } }, 60_000); + it("renews only an expired live agent attachment's publication repair window", async () => { + const { createPublicMcpTransfers } = await import("../services/public-mcp/file-transfers.js"); + const { createPublicMcpOAuth } = await import("../services/public-mcp/oauth.js"); + const { instanceSettingsService } = await import("../services/instance-settings.js"); + const { default: express } = await import("express"); + const { default: request } = await import("supertest"); + const f = await seed(), p = await connectedMcp(f); + try { + const board = await p.oauth.authenticate(p.token), sessionId = randomUUID(); + const resource = `${p.oauth.config.origin}/mcp/runner`; + await instanceSettingsService(db).updateExperimental({ enableOpenAiDot: true }); + await db.update(tables.mcpOauthGrants).set({ purpose: "agent", agentId: f.agentId, + resource, scopes: ["paperclip:agent", "offline_access"] }).where(eq(tables.mcpOauthGrants.id, p.grantId)); + await db.update(tables.issues).set({ status: "in_progress" }).where(eq(tables.issues.id, f.issueId)); + await db.update(tables.heartbeatRuns).set({ status: "running", finishedAt: null, runtimeMode: "native", + driverKind: "grok_bot_mcp", nativeIssueId: f.issueId }).where(eq(tables.heartbeatRuns.id, f.runId)); + const oauth = createPublicMcpOAuth(db, { ...p.oauth.config, resource }); + const principal = await oauth.authenticate(p.token); + let ownerAvailable = true; + let ownerRunId = f.runId; + let pauseRepair = false, repairLocked!: () => void, releaseRepair!: () => void; + const locked = new Promise(resolve => { repairLocked = resolve; }); + const released = new Promise(resolve => { releaseRepair = resolve; }); + // Named work-session boundary; OAuth, transfer HTTP, storage, attachment + // registration and native publication receipt all use their real owners. + const transfers = createPublicMcpTransfers(db, oauth, p.api, p.storage, { agentSession: { + authorize: async (actor, session, taskId, _write, queryDb) => { + if (!ownerAvailable || actor.grant.id !== p.grantId || actor.grant.agentId !== f.agentId + || session !== sessionId || taskId !== f.issueId) throw new Error("work session unavailable"); + if (pauseRepair && queryDb) { pauseRepair = false; repairLocked(); await released; } + return { agentId: f.agentId, runId: ownerRunId }; + }, + } }); + const app = express(); app.use(transfers.router); + const bytes = Buffer.from("expired live publication repair"); + const args = { companyId: f.companyId, taskId: f.issueId, requestId: randomUUID(), filename: "repair.txt", + contentType: "text/plain", byteSize: bytes.length, sha256: createHash("sha256").update(bytes).digest("hex") }; + const upload = await transfers.getUploadUrl(principal, args, sessionId); + const put = async (value: unknown) => { + const url = new URL(String(value)); + return request(app).put(url.pathname + url.search).set("Content-Type", "text/plain").send(bytes) + .timeout({ response: 5000, deadline: 10_000 }); + }; + const initial = await put(upload.url); + expect(initial.status).toBe(200); + const uploadId = String(upload.uploadId), attachmentId = initial.body.attachment.id; + const current = async () => (await db.select().from(tables.mcpAttachmentUploads) + .where(eq(tables.mcpAttachmentUploads.id, uploadId)))[0]; + const loseReceipt = async () => { + const [run] = await db.select().from(tables.heartbeatRuns).where(eq(tables.heartbeatRuns.id, f.runId)); + const receipts = { ...run.resultJson?.semanticToolReceipts as Record }; + delete receipts[`external-upload:${uploadId}`]; + await db.update(tables.heartbeatRuns).set({ resultJson: { ...run.resultJson, semanticToolReceipts: receipts } }) + .where(eq(tables.heartbeatRuns.id, f.runId)); + await db.update(tables.mcpAttachmentUploads).set({ expiresAt: new Date(0) }) + .where(eq(tables.mcpAttachmentUploads.id, uploadId)); + }; + await loseReceipt(); + const expired = await current(); + expect((await put(upload.url)).status).toBe(403); + const repair = await transfers.getUploadUrl(principal, args, sessionId); + const renewed = await current(); + expect(renewed).toEqual({ ...expired, expiresAt: expect.any(Date) }); + expect(renewed.expiresAt.getTime()).toBeGreaterThan(Date.now()); + const repaired = await put(repair.url); + expect(repaired.status).toBe(200); + expect(repaired.body).toMatchObject({ attachment: { id: attachmentId }, publication: { + source: "authenticated_upload", evidenceRef: `deliverable:${attachmentId}` } }); + expect(await transfers.getUploadUrl(principal, args, sessionId)).toMatchObject({ status: "completed", + attachment: { id: attachmentId }, publication: repaired.body.publication }); + expect(await current()).toEqual(renewed); + expect(await db.select().from(tables.issueAttachments).where(eq(tables.issueAttachments.issueId, f.issueId))).toHaveLength(1); + await loseReceipt(); + const beforeDenied = await current(); + ownerAvailable = false; + await expect(transfers.getUploadUrl(principal, args, sessionId)).rejects.toThrow("work session unavailable"); + expect(await current()).toEqual(beforeDenied); + ownerAvailable = true; + ownerRunId = randomUUID(); + await expect(transfers.getUploadUrl(principal, args, sessionId)).rejects.toThrow(); + expect(await current()).toEqual(beforeDenied); + ownerRunId = f.runId; + await db.update(tables.mcpAttachmentUploads).set({ cleanedAt: new Date(), cleanupVerifiedAt: new Date() }) + .where(eq(tables.mcpAttachmentUploads.id, uploadId)); + const contradictory = await current(); + await expect(transfers.getUploadUrl(principal, args, sessionId)).rejects.toThrow(); + expect(await current()).toEqual(contradictory); + expect((await put(repair.url)).status).toBe(403); + await db.update(tables.mcpAttachmentUploads).set({ cleanedAt: null, cleanupVerifiedAt: null }) + .where(eq(tables.mcpAttachmentUploads.id, uploadId)); + // Deletion owns metadata/cleanup settlement. Overlap it with the locked + // repair to prove renewal cannot revive a pointer or recreate its bytes. + pauseRepair = true; + const repairing = transfers.getUploadUrl(principal, args, sessionId); + await locked; + const deleting = p.api(board, "DELETE", `/attachments/${attachmentId}`); + let overlappingRepair: Awaited; + try { + await vi.waitFor(async () => expect(await p.storage.headObject(f.companyId, renewed.objectKey)).toEqual({ exists: false }), + { timeout: 5000, interval: 20 }); + } finally { + releaseRepair(); + [overlappingRepair] = await Promise.all([repairing, deleting]); + } + const deleted = await current(); + expect(deleted).toMatchObject({ attachmentId, cleanedAt: expect.any(Date), cleanupVerifiedAt: expect.any(Date) }); + await expect(transfers.getUploadUrl(principal, args, sessionId)).rejects.toThrow(); + expect(await current()).toEqual(deleted); + expect((await put(repair.url)).status).toBe(403); + expect((await put(overlappingRepair.url)).status).toBe(403); + expect(await p.storage.headObject(f.companyId, deleted.objectKey)).toEqual({ exists: false }); + } finally { await p.cleanup(); } + }, 60_000); + it.each(["delete_failed", "object_survived", "head_failed"] as const)("retains MCP cleanup uncertainty after actual attachment removal: %s", async failure => { const { createPublicMcpTransfers } = await import("../services/public-mcp/file-transfers.js"); const { default: express } = await import("express"); diff --git a/server/src/server-info.ts b/server/src/server-info.ts index 455e622c870..6d3e861e6bb 100644 --- a/server/src/server-info.ts +++ b/server/src/server-info.ts @@ -172,6 +172,11 @@ const GIT_INFO_CACHE_TTL_MS = 3000; const processStartedAt = new Date().toISOString(); let gitInfoCache: { value: ServerGitInfo; expiresAt: number } | null = null; +// Lifecycle identity must not run the synchronous Git probes used by diagnostics. +export function getServerProcessStartedAt(): string { + return processStartedAt; +} + export function getServerInfoSnapshot( opts: { now?: number; diff --git a/server/src/services/native-runtime/native-controller-identity.test.ts b/server/src/services/native-runtime/native-controller-identity.test.ts new file mode 100644 index 00000000000..fd11591da9a --- /dev/null +++ b/server/src/services/native-runtime/native-controller-identity.test.ts @@ -0,0 +1,37 @@ +import { afterEach, expect, it, vi } from "vitest"; +import * as childProcess from "node:child_process"; +import * as hotRestart from "../hot-restart.js"; +import { getServerInfoSnapshot, resetServerInfoCacheForTests } from "../../server-info.js"; +import { currentNativeControllerIdentity } from "./native-restart-recovery.js"; + +vi.mock("node:child_process", async importOriginal => ({ + ...await importOriginal(), + execFileSync: vi.fn(() => { + throw new Error("Git metadata is unavailable; identity must still work"); + }), +})); + +afterEach(() => { + vi.restoreAllMocks(); + resetServerInfoCacheForTests(); +}); + +it.each([true, false])("keeps controller identity independent of Git commands (OS start available=%s)", async available => { + const observed = "2026-11-02T12:00:00.000Z"; + const fallback = getServerInfoSnapshot({ gitCommand: () => { throw new Error("no checkout"); } }).processStartedAt; + resetServerInfoCacheForTests(); + vi.spyOn(hotRestart, "readProcessStartedAt").mockImplementation(async () => { + if (!available) throw new Error("process start unavailable"); + return observed; + }); + const subprocess = vi.mocked(childProcess.execFileSync); + subprocess.mockClear(); + + const first = await currentNativeControllerIdentity(); + const second = await currentNativeControllerIdentity(); + + expect(first).toEqual(second); + expect(first.pid).toBe(process.pid); + expect(first.processStartedAt.toISOString()).toBe(available ? observed : fallback); + expect(subprocess).not.toHaveBeenCalled(); +}); diff --git a/server/src/services/native-runtime/native-restart-recovery.ts b/server/src/services/native-runtime/native-restart-recovery.ts index 96a4112e5a6..a27af96ac3b 100644 --- a/server/src/services/native-runtime/native-restart-recovery.ts +++ b/server/src/services/native-runtime/native-restart-recovery.ts @@ -31,7 +31,7 @@ import { } from "@paperclipai/db"; import { REMOTE_CONTROLLER_LEASE_MS } from "./native-runner-recovery-contract.js"; import { readProcessStartedAt } from "../hot-restart.js"; -import { getServerInfoSnapshot } from "../../server-info.js"; +import { getServerProcessStartedAt } from "../../server-info.js"; import { redactSensitiveText } from "../../redaction.js"; import { issueRecoveryActionService } from "../issue-recovery-actions.js"; import { isNewTerminalRunFailure, reportRunFailure } from "../run-failure-report.js"; @@ -157,7 +157,7 @@ async function observedProcessStart(pid: number): Promise { export async function currentNativeControllerIdentity(): Promise { const observed = await observedProcessStart(process.pid); - const fallback = new Date(getServerInfoSnapshot().processStartedAt); + const fallback = new Date(getServerProcessStartedAt()); return { bootId: controllerBootId, pid: process.pid, diff --git a/server/src/services/public-mcp/file-transfers.ts b/server/src/services/public-mcp/file-transfers.ts index 83837600769..d9e49eeaacb 100644 --- a/server/src/services/public-mcp/file-transfers.ts +++ b/server/src/services/public-mcp/file-transfers.ts @@ -228,6 +228,26 @@ export function createPublicMcpTransfers(db: Db, oauth: PublicMcpOAuth, api: Api ...(publication ? { publication } : {}) }; // An older upload can lack publication proof. Verify the same bytes // through another bounded PUT; never manufacture a receipt from metadata. + await db.transaction(async tx => { + const [current] = await tx.select().from(mcpAttachmentUploads) + .where(eq(mcpAttachmentUploads.id, upload.id)).for("update"); + if (!current || current.companyId !== p.grant.companyId || current.userId !== p.grant.userId + || current.argumentsHash !== hash || current.workSessionId !== sessionId + || current.grantId && current.grantId !== p.grant.id + || current.attachmentId !== attachment.id || current.storageProvider !== storage.provider + || current.cleanedAt || current.cleanupVerifiedAt) throw unavailable(); + const owner = await workSession(p, sessionId, current.taskId, true, tx as unknown as Db); + const live = await attachmentAccess(p, attachment.id, tx as unknown as Db, sessionId); + if (!owner || live.issueId !== current.taskId || live.provider !== current.storageProvider + || live.objectKey !== current.objectKey || live.originalFilename !== current.originalFilename + || live.byteSize !== current.byteSize || live.sha256 !== current.sha256 + || live.originatingRunId !== owner.runId || live.createdByAgentId !== owner.agentId) throw unavailable(); + // Keep the committed attachment and cleanup identity immutable. A + // concurrent deletion still removes that attachment/stamps this row; + // PUT rechecks both and never stores bytes for a committed upload. + await tx.update(mcpAttachmentUploads).set({ expiresAt: new Date(Date.now() + lifetime) }) + .where(eq(mcpAttachmentUploads.id, current.id)); + }); } return { ...await ticket(p, { uploadId: upload.id }, sessionId), method: "PUT", headers: { "Content-Type": a.contentType }, uploadId: upload.id, maxBytes: MAX_ATTACHMENT_BYTES, instructions: "Send the exact file bytes with the given Content-Type. Success saves the attachment and returns its ID. An agent upload also returns a verified publication receipt; cite its evidenceRef in your completion report. There is no separate upload completion tool. Do not share this temporary file credential. Reuse requestId after a lost response." };