fix(server): prevent review policy verdict downgrade bypass

Authorize verdicts and policy changes against the stored restrictive review policy, remove downgrade guidance, and cover both restrictive policies with route regressions.

Co-Authored-By: Paperclip <noreply@paperclip.ing>

doc/SPEC-implementation.md

@@ -258,6 +258,8 @@ Invariants:
 - single assignee only
 - task must trace to company goal chain via `goal_id`, `parent_id`, or project-goal linkage
 - `in_progress` requires assignee
+- an `in_review -> done | cancelled` verdict is authorized against the review policy stored before the update; a policy change in the same request cannot relax that verdict gate
+- while a restrictive review policy is stored, changing it requires an actor who is allowed by that current policy
 - terminal states: `done | cancelled`
 
 ## 7.7 `issue_comments`

server/src/__tests__/issue-stalled-review-decision-routes.test.ts

@@ -355,7 +355,7 @@ describeEmbeddedPostgres("stalled review decision routes", () => {
         code: "review_policy_denied",
         policy: "human_only",
         allowedActor: "authenticated_user_with_issue_write_access",
-        remediation: expect.stringContaining("authenticated user"),
+        remediation: "Have an authenticated user with issue write access submit the verdict.",
       },
     });
 
@@ -366,7 +366,7 @@ describeEmbeddedPostgres("stalled review decision routes", () => {
     expect(userVerdict.body).toMatchObject({ id: issueId, status: "cancelled" });
   });
 
-  it("allows an agent writer to relax reviewPolicy in the verdict patch", async () => {
+  it("does not let an agent bypass human_only by relaxing reviewPolicy in the verdict patch", async () => {
     const seeded = await seedCompany("RLP");
     const issueId = await seedReview({
       companyId: seeded.companyId,
@@ -380,8 +380,87 @@ describeEmbeddedPostgres("stalled review decision routes", () => {
       .patch(`/api/issues/${issueId}`)
       .send({ status: "done", reviewPolicy: "anyone" });
 
-    expect(verdict.status, JSON.stringify(verdict.body)).toBe(200);
-    expect(verdict.body).toMatchObject({ id: issueId, status: "done", reviewPolicy: "anyone" });
+    expect(verdict.status).toBe(403);
+    expect(verdict.body).toMatchObject({
+      details: {
+        code: "review_policy_denied",
+        policy: "human_only",
+        allowedActor: "authenticated_user_with_issue_write_access",
+        remediation: "Have an authenticated user with issue write access submit the verdict.",
+      },
+    });
+    const [persisted] = await db.select({
+      status: issues.status,
+      reviewPolicy: issues.reviewPolicy,
+    }).from(issues).where(eq(issues.id, issueId));
+    expect(persisted).toEqual({ status: "in_review", reviewPolicy: "human_only" });
+  });
+
+  it("does not let the review requester bypass not_creator by relaxing reviewPolicy in the verdict patch", async () => {
+    const seeded = await seedCompany("RNC");
+    const issueId = await seedReview({
+      companyId: seeded.companyId,
+      assigneeAgentId: seeded.assigneeAgentId,
+      identifier: "RNC-1",
+      reviewPolicy: "not_creator",
+    });
+    await db.insert(activityLog).values({
+      companyId: seeded.companyId,
+      actorType: "agent",
+      actorId: seeded.assigneeAgentId,
+      agentId: seeded.assigneeAgentId,
+      action: "issue.updated",
+      entityType: "issue",
+      entityId: issueId,
+      details: { status: "in_review", _previous: { status: "in_progress" } },
+    });
+    const runId = await seedRun(seeded.companyId, seeded.assigneeAgentId, issueId);
+
+    const verdict = await request(app(agentActor(seeded.companyId, seeded.assigneeAgentId, runId)))
+      .patch(`/api/issues/${issueId}`)
+      .send({ status: "done", reviewPolicy: "anyone" });
+
+    expect(verdict.status).toBe(403);
+    expect(verdict.body).toMatchObject({
+      details: {
+        code: "review_policy_denied",
+        policy: "not_creator",
+        allowedActor: "writer_other_than_review_requester",
+        remediation: "Have another writer with issue write access submit the verdict.",
+      },
+    });
+    const [persisted] = await db.select({
+      status: issues.status,
+      reviewPolicy: issues.reviewPolicy,
+    }).from(issues).where(eq(issues.id, issueId));
+    expect(persisted).toEqual({ status: "in_review", reviewPolicy: "not_creator" });
+  });
+
+  it("does not let an excluded actor relax an existing review policy in a separate patch", async () => {
+    const seeded = await seedCompany("RSP");
+    const issueId = await seedReview({
+      companyId: seeded.companyId,
+      assigneeAgentId: seeded.assigneeAgentId,
+      identifier: "RSP-1",
+      reviewPolicy: "human_only",
+    });
+    const runId = await seedRun(seeded.companyId, seeded.assigneeAgentId, issueId);
+
+    const relaxation = await request(app(agentActor(seeded.companyId, seeded.assigneeAgentId, runId)))
+      .patch(`/api/issues/${issueId}`)
+      .send({ reviewPolicy: "anyone" });
+
+    expect(relaxation.status).toBe(403);
+    expect(relaxation.body).toMatchObject({
+      details: {
+        code: "review_policy_denied",
+        policy: "human_only",
+      },
+    });
+    const [persisted] = await db.select({ reviewPolicy: issues.reviewPolicy })
+      .from(issues)
+      .where(eq(issues.id, issueId));
+    expect(persisted).toEqual({ reviewPolicy: "human_only" });
   });
 
   it("enforces not_creator when accepting or rejecting pending review interactions", async () => {

server/src/routes/issues.ts

@@ -8780,19 +8780,21 @@ export function issueRoutes(
       onBehalfOfUserId: _requestedOnBehalfOfUserId,
       ...updateFields
     } = req.body;
-    const effectiveReviewPolicy = req.body.reviewPolicy === undefined
-      ? existing.reviewPolicy
-      : req.body.reviewPolicy;
-    if (
+    const reviewPolicyChangeRequested =
+      req.body.reviewPolicy !== undefined
+      && req.body.reviewPolicy !== existing.reviewPolicy;
+    const reviewVerdictRequested =
       existing.status === "in_review"
-      && (updateFields.status === "done" || updateFields.status === "cancelled")
-      && effectiveReviewPolicy != null
-      && effectiveReviewPolicy !== "anyone"
+      && (updateFields.status === "done" || updateFields.status === "cancelled");
+    if (
+      (reviewVerdictRequested || reviewPolicyChangeRequested)
+      && existing.reviewPolicy != null
+      && existing.reviewPolicy !== "anyone"
     ) {
       await assertIssueReviewVerdictActorAllowed(db, {
         issue: existing,
         actor: { type: actor.actorType, id: actor.actorId },
-        reviewPolicy: effectiveReviewPolicy,
+        reviewPolicy: existing.reviewPolicy,
       });
     }
     const shouldCancelActiveRunForCancelledStatus =

server/src/services/issue-review-policy.ts

@@ -106,7 +106,7 @@ export async function assertIssueReviewVerdictActorAllowed(
         code: "review_policy_denied",
         policy,
         allowedActor: "authenticated_user_with_issue_write_access",
-        remediation: "Have an authenticated user with issue write access submit the verdict, or change reviewPolicy to `anyone`.",
+        remediation: "Have an authenticated user with issue write access submit the verdict.",
       },
     );
   }
@@ -119,7 +119,7 @@ export async function assertIssueReviewVerdictActorAllowed(
         code: "review_policy_denied",
         policy,
         allowedActor: "writer_other_than_review_requester",
-        remediation: "Change reviewPolicy to `anyone`, or move the issue out of and back into `in_review` to record a requester before another writer submits the verdict.",
+        remediation: "Move the issue out of and back into `in_review` to record a requester before another writer submits the verdict.",
       },
     );
   }
@@ -131,7 +131,7 @@ export async function assertIssueReviewVerdictActorAllowed(
       code: "review_policy_denied",
       policy,
       allowedActor: "writer_other_than_review_requester",
-      remediation: "Have another writer with issue write access submit the verdict, or change reviewPolicy to `anyone`.",
+      remediation: "Have another writer with issue write access submit the verdict.",
     },
   );
 }