fix(server): enforce read auth for single issue comments (#8346)
## 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 <noreply@paperclip.ing>
This commit is contained in:
@@ -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");
|
||||
|
||||
@@ -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<string, unknown>) => ({
|
||||
@@ -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 }) => ({
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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" });
|
||||
|
||||
Reference in New Issue
Block a user