From 67f97e8fb08b64799d314fb4ecf7499b90b09acd Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 19 Jun 2026 15:57:36 -0700 Subject: [PATCH] fix(server): enforce read auth for single issue comments (#8346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Issue comments are part of the control-plane audit trail and must respect the same company and issue authorization boundaries as issue reads. > - Mention-scoped commenting gives low-trust agents a narrow way to reply when an authorized assignee mentions them. > - The comment list route already enforces issue-read authorization, but the single-comment read route only checked same-company access before returning a known comment id. > - That created a broken object-level authorization gap for same-company low-trust agents outside the issue boundary. > - This pull request applies the existing issue-read guard to the single-comment route before loading the comment. > - The benefit is consistent comment-read authorization across list and single-comment endpoints, with regression coverage for the low-trust boundary. ## Linked Issues or Issue Description Refs #7389 Bug fix context: - What happened: `GET /api/issues/:id/comments/:commentId` checked company access but did not enforce the issue-read authorization boundary before returning a single comment by known id. - Expected behavior: single-comment reads should use the same issue-read boundary as issue thread list reads. - Steps to reproduce: authenticate as a same-company low-trust agent outside an issue's readable boundary, then request a known comment id via the single-comment endpoint. - Deployment mode: applies to server authorization behavior in authenticated agent API usage. ## What Changed - Added `assertIssueReadAllowed` to the single issue-comment read route before `getComment` is called. - Added mocked route coverage proving peer agents outside the issue-read boundary get `403` and the comment is not loaded. - Added authorization and embedded route coverage for mention-scoped low-trust comment grants so the intended narrow reply path remains allowed. ## Verification - `NODE_ENV=test pnpm exec vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts` — passed, 60 tests. - `NODE_ENV=test pnpm exec vitest run server/src/__tests__/authorization-service.test.ts` — passed, 26 tests. - `NODE_ENV=test pnpm exec vitest run server/src/__tests__/low-trust-red-team-routes.test.ts` — passed, 8 tests. - `git diff --check` — passed. ## Risks Low risk. This reuses the existing issue-read guard for a read endpoint. The main behavioral shift is that same-company actors who cannot read an issue can no longer fetch a known comment id from that issue, which is the intended authorization boundary. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI GPT-5 via Codex, with repository tool use and command execution. Runtime context-window details were not exposed by the environment. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .../__tests__/authorization-service.test.ts | 48 +++++++++++++++++++ ...ue-agent-mutation-ownership-routes.test.ts | 25 ++++++++++ .../low-trust-red-team-routes.test.ts | 44 +++++++++++++++++ server/src/routes/issues.ts | 1 + 4 files changed, 118 insertions(+) diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index 12292707..09b98af2 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -706,6 +706,54 @@ describeEmbeddedPostgres("authorization service", () => { })).resolves.toMatchObject({ allowed: false, reason: "deny_low_trust_boundary" }); }); + it("allows a mentioned non-assignee to comment when the mention author is the issue assignee", async () => { + const company = await createCompany(db, "MentionCommentAssigneeGrant"); + const allowedProject = await createProject(db, company.id, "MentionAssigneeAllowed"); + const targetProject = await createProject(db, company.id, "MentionAssigneeTarget"); + const assigneeAgent = await createAgent(db, company.id, { role: "coach" }); + const mentionedAgent = await createAgent(db, company.id, { + role: "qa", + permissions: { + trustPreset: LOW_TRUST_REVIEW_PRESET, + authorizationPolicy: { + trustBoundary: { + mode: LOW_TRUST_REVIEW_PRESET, + companyId: company.id, + projectIds: [allowedProject.id], + }, + }, + }, + }); + const issue = await createIssue(db, company.id, { + title: "Assignee-authored mention reply target", + projectId: targetProject.id, + assigneeAgentId: assigneeAgent.id, + }); + await db.insert(issueComments).values({ + companyId: company.id, + issueId: issue.id, + authorAgentId: assigneeAgent.id, + authorType: "agent", + body: `[@QA](agent://${mentionedAgent.id}) please reply on this issue.`, + }); + + await expect(authorizationService(db).decide({ + actor: { type: "agent", agentId: mentionedAgent.id, companyId: company.id, source: "agent_key" }, + action: "issue:comment", + resource: { + type: "issue", + companyId: company.id, + issueId: issue.id, + projectId: issue.projectId, + assigneeAgentId: assigneeAgent.id, + status: issue.status, + }, + })).resolves.toMatchObject({ + allowed: true, + reason: "allow_issue_mention_grant", + }); + }); + it("does not grant mention-scoped issue access from self-authored or unauthorized-author comments", async () => { const company = await createCompany(db, "MentionCommentDenied"); const allowedProject = await createProject(db, company.id, "MentionDeniedAllowed"); diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index 701f3d50..436915de 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -19,6 +19,7 @@ const mockIssueService = vi.hoisted(() => ({ getAttachmentById: vi.fn(), getByIdentifier: vi.fn(), getById: vi.fn(), + getComment: vi.fn(), getRelationSummaries: vi.fn(), getWakeableParentAfterChildCompletion: vi.fn(), list: vi.fn(), @@ -376,6 +377,7 @@ describe("agent issue mutation checkout ownership", () => { mockIssueService.getAttachmentById.mockReset(); mockIssueService.getByIdentifier.mockReset(); mockIssueService.getById.mockReset(); + mockIssueService.getComment.mockReset(); mockIssueService.getRelationSummaries.mockReset(); mockIssueService.getWakeableParentAfterChildCompletion.mockReset(); mockIssueService.list.mockReset(); @@ -484,6 +486,12 @@ describe("agent issue mutation checkout ownership", () => { mockCompanyService.getById.mockResolvedValue({ id: companyId, issuePrefix: "PAP" }); mockIssueService.getById.mockResolvedValue(makeIssue()); mockIssueService.getByIdentifier.mockResolvedValue(null); + mockIssueService.getComment.mockResolvedValue({ + id: "comment-1", + issueId, + companyId, + body: "Mentioned reply context.", + }); mockIssueService.list.mockResolvedValue([makeIssue()]); mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); mockIssueService.create.mockImplementation(async (_companyId: string, input: Record) => ({ @@ -801,6 +809,23 @@ describe("agent issue mutation checkout ownership", () => { }); }); + it("rejects peer agents from reading a specific comment when issue read is outside their boundary", async () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: false, + action: input.action, + reason: "deny_low_trust_boundary", + explanation: "Issue is outside this low-trust boundary.", + })); + + const res = await request(await createApp(peerActor())) + .get(`/api/issues/${issueId}/comments/comment-1`); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toBe("Issue is outside this actor's authorization boundary"); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" })); + expect(mockIssueService.getComment).not.toHaveBeenCalled(); + }); + it("keeps true issue mutations denied for mentioned peer agents", async () => { mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", assigneeAgentId: ownerAgentId })); mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ diff --git a/server/src/__tests__/low-trust-red-team-routes.test.ts b/server/src/__tests__/low-trust-red-team-routes.test.ts index d97a8400..e34a42b0 100644 --- a/server/src/__tests__/low-trust-red-team-routes.test.ts +++ b/server/src/__tests__/low-trust-red-team-routes.test.ts @@ -586,6 +586,50 @@ describeEmbeddedPostgres("low-trust red-team HTTP route regression suite", () => }); }); + it("allows mentioned low-trust agents to comment on out-of-bound assigned issues", async () => { + const fixture = await seedLowTrustFixture(db); + const [targetIssue] = await db.insert(issues).values({ + companyId: fixture.company.id, + projectId: fixture.projects.outOfScope.id, + title: "Coach-owned mention target", + status: "in_progress", + priority: "medium", + assigneeAgentId: fixture.agents.standard.id, + }).returning(); + await db.insert(issueComments).values({ + companyId: fixture.company.id, + issueId: targetIssue!.id, + authorAgentId: fixture.agents.standard.id, + authorType: "agent", + body: `[@Low Trust Reviewer](agent://${fixture.agents.lowTrust.id}) please verify this issue.`, + }); + + const unmentioned = await db.insert(agents).values({ + companyId: fixture.company.id, + name: "Unmentioned Low Trust Reviewer", + role: "engineer", + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + permissions: fixture.agents.lowTrust.permissions, + }).returning().then((rows) => rows[0]!); + + const comment = await request(createApp(db, agentActor(fixture))) + .post(`/api/issues/${targetIssue!.id}/comments`) + .send({ body: "Mention-scoped verification complete." }); + expect(comment.status, JSON.stringify(comment.body)).toBe(201); + expect(comment.body).toMatchObject({ + issueId: targetIssue!.id, + authorAgentId: fixture.agents.lowTrust.id, + }); + + const unmentionedComment = await request(createApp(db, agentActor(fixture, unmentioned.id))) + .post(`/api/issues/${targetIssue!.id}/comments`) + .send({ body: "I was not mentioned." }); + expect(unmentionedComment.status, JSON.stringify(unmentionedComment.body)).toBe(403); + expect(unmentionedComment.body.error).toBe("Issue is outside this actor's authorization boundary"); + }); + it("propagates denied low-trust policy conflicts on control-plane guards", async () => { const fixture = await seedLowTrustFixture(db); const conflictingExecutionPolicy = { diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 2565eb3f..1a4897b0 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -7122,6 +7122,7 @@ export function issueRoutes( return; } assertCompanyAccess(req, issue.companyId); + if (!(await assertIssueReadAllowed(req, res, issue))) return; const comment = await svc.getComment(commentId); if (!comment || comment.issueId !== id) { res.status(404).json({ error: "Comment not found" });