From 15c7eff282cbcf121d68885d49435e4e38de982f Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 1 Aug 2026 17:15:10 +0000 Subject: [PATCH] fix: close issue privacy revocation gaps Authorize privacy mutations against the subtree root, avoid caching positive grants across revocation, and preserve uniform 404 read denials. Co-Authored-By: Paperclip --- .../__tests__/authorization-service.test.ts | 2 +- .../__tests__/external-object-routes.test.ts | 6 ++-- server/src/routes/issues.ts | 31 ++++++++++++++++--- server/src/services/authorization.ts | 7 ++++- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index 30e09cd7226..9437c8dd986 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -2207,7 +2207,7 @@ describeEmbeddedPostgres("authorization service", () => { const previousMode = process.env.PAPERCLIP_ISSUE_PRIVACY_MODE; const previousTtl = process.env.PAPERCLIP_ISSUE_PRIVACY_CACHE_TTL_MS; process.env.PAPERCLIP_ISSUE_PRIVACY_MODE = "enforce"; - process.env.PAPERCLIP_ISSUE_PRIVACY_CACHE_TTL_MS = "0"; + process.env.PAPERCLIP_ISSUE_PRIVACY_CACHE_TTL_MS = "5000"; try { const company = await createCompany(db, "IssuePrivacy"); const ownerId = await createUser(db); diff --git a/server/src/__tests__/external-object-routes.test.ts b/server/src/__tests__/external-object-routes.test.ts index 14b142b7dfc..2905690c1e6 100644 --- a/server/src/__tests__/external-object-routes.test.ts +++ b/server/src/__tests__/external-object-routes.test.ts @@ -218,11 +218,13 @@ describe("external object routes", () => { const app = await createApp(ownerActor()); const summary = await request(app).get(`/api/issues/${issueId}/external-object-summary`); - expect(summary.status).toBe(403); + expect(summary.status).toBe(404); + expect(summary.body.error).toBe("Issue not found"); expect(mockExternalObjectsService.getIssueSummary).not.toHaveBeenCalled(); const list = await request(app).get(`/api/issues/${issueId}/external-objects`); - expect(list.status).toBe(403); + expect(list.status).toBe(404); + expect(list.body.error).toBe("Issue not found"); expect(mockExternalObjectsService.listForIssue).not.toHaveBeenCalled(); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 16c6aee1971..057d558a53c 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -3522,6 +3522,25 @@ export function issueRoutes( return false; } + async function resolveIssuePrivacyManagementRoot( + req: Request, + res: Response, + issue: Parameters[1] & { + privacyRootIssueId: string | null; + responsibleUserId: string | null; + createdByUserId: string | null; + }, + ) { + const privacyRootId = issue.privacyRootIssueId ?? issue.id; + const privacyRoot = privacyRootId === issue.id + ? issue + : await getAccessibleResource(req, res, svc.getById(privacyRootId), "Issue not found"); + if (!privacyRoot) return null; + if (privacyRoot.id !== issue.id && !(await assertIssueReadAllowed(req, res, privacyRoot))) return null; + if (!(await assertCanManageIssuePrivacy(req, res, privacyRoot))) return null; + return privacyRoot; + } + async function assertAgentIssueCommentAllowed( req: Request, res: Response, @@ -7834,8 +7853,9 @@ export function issueRoutes( router.get("/issues/:id/access-grants", async (req, res) => { const issue = await getAccessibleResource(req, res, svc.getById(req.params.id as string), "Issue not found"); if (!issue || !(await assertIssueReadAllowed(req, res, issue))) return; - if (!(await assertCanManageIssuePrivacy(req, res, issue))) return; - const grantIssueIds = [...new Set([issue.id, issue.privacyRootIssueId ?? issue.id])]; + const privacyRoot = await resolveIssuePrivacyManagementRoot(req, res, issue); + if (!privacyRoot) return; + const grantIssueIds = [...new Set([issue.id, privacyRoot.id])]; const grants = await db .select() .from(issueAccessGrants) @@ -7850,8 +7870,9 @@ export function issueRoutes( async (req, res) => { const issue = await getAccessibleResource(req, res, svc.getById(req.params.id as string), "Issue not found"); if (!issue || !(await assertIssueReadAllowed(req, res, issue))) return; - if (!(await assertCanManageIssuePrivacy(req, res, issue))) return; - const grantIssueIds = [...new Set([issue.id, issue.privacyRootIssueId ?? issue.id])]; + const privacyRoot = await resolveIssuePrivacyManagementRoot(req, res, issue); + if (!privacyRoot) return; + const grantIssueIds = [...new Set([issue.id, privacyRoot.id])]; const [grant] = await db .update(issueAccessGrants) .set({ revokedAt: new Date() }) @@ -7886,7 +7907,7 @@ export function issueRoutes( const id = req.params.id as string; const existing = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); if (!existing) return; - if (req.body.visibility !== undefined && !(await assertCanManageIssuePrivacy(req, res, existing))) return; + if (req.body.visibility !== undefined && !(await resolveIssuePrivacyManagementRoot(req, res, existing))) return; assertNoAgentHostWorkspaceCommandMutation(req, collectIssueWorkspaceCommandPaths(req.body)); if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return; if (!(await assertCheapRecoveryIssueAssigneeProfileAllowed(req, res, existing, req.body))) return; diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index 1eb561a24db..4d5ce20bdcb 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -520,7 +520,12 @@ async function actorHasIssuePrivacyGrant(db: Db, actor: AuthorizationActor, issu .limit(1) .then((rows) => rows.length > 0); issuePrivacyGrantCache.set(cacheKey, { expiresAt: now + issuePrivacyCacheTtlMs(), promise }); - return promise; + const granted = await promise; + // Positive authorization decisions must observe revocation on the next read. + // Keep only negative decisions cached; grant creation paths already tolerate + // their short TTL, while revocation must fail closed immediately. + if (granted) issuePrivacyGrantCache.delete(cacheKey); + return granted; } /** Canonical row-level issue privacy predicate, also consumed by run-derived checks. */