From 6e8c78b8d22fb0f8ab76cb3d25b72976b3b12d50 Mon Sep 17 00:00:00 2001 From: lorentz Date: Thu, 16 Jul 2026 23:29:11 -0400 Subject: [PATCH] fix(23-06): capture actionId in auto-post audit payload, guard manual re-approval of acknowledge_user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 23-06-REVIEW.md found two real defects in the just-merged idempotency fix: - CR-01: autoPostAcknowledgment's audit payload omitted actionId, which the campaign-detail API requires to derive completedAt — every auto-posted acknowledge_user row rendered a null completion date in the Action Area UI. - CR-02: the manual approve/remediate path had no server-side guard against re-approving acknowledge_user for a campaign that already got auto-posted — only a client-side UI check prevented the exact duplicate-note bug 23-06 was chartered to close, reachable via a direct API call. Fixes both: capture RETURNING id from the insert and include it in the audit payload; add an existence check in approveRemediationActions that rejects acknowledge_user when already posted for the campaign. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01W6RuWdiUiXrPK6FLBHjtpY --- lib/services/remediation-service.test.ts | 35 ++++++++++++++++++++++-- lib/services/remediation-service.ts | 22 +++++++++++++-- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/lib/services/remediation-service.test.ts b/lib/services/remediation-service.test.ts index 0293b4f..ca81a56 100644 --- a/lib/services/remediation-service.test.ts +++ b/lib/services/remediation-service.test.ts @@ -161,6 +161,35 @@ describe('approveRemediationActions', () => { expect(result).toHaveLength(2); expect(result[0]).toMatchObject({ id: 'action-1', actionType: 'block_sender', status: 'approved' }); }); + + it('rejects re-approving acknowledge_user when it has already been posted for the campaign (CR-02 fix)', async () => { + stage({ + classification: [{ recommended_actions: ['acknowledge_user'] }], + existingAckRows: [{ id: 'auto-posted-action' }], + }); + + await expect( + approveRemediationActions('campaign-1', [{ actionType: 'acknowledge_user' }], 'operator@example.com') + ).rejects.toThrow(RemediationValidationError); + expect(callsContaining('INSERT INTO remediation_actions')).toHaveLength(0); + }); + + it('still allows approving acknowledge_user when no prior post exists for the campaign', async () => { + stage({ + classification: [{ recommended_actions: ['acknowledge_user'] }], + existingAckRows: [], + insertRemediation: [{ id: 'action-1' }], + }); + + const result = await approveRemediationActions( + 'campaign-1', + [{ actionType: 'acknowledge_user' }], + 'operator@example.com' + ); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ id: 'action-1', actionType: 'acknowledge_user', status: 'approved' }); + }); }); // ============================================================================= @@ -322,8 +351,8 @@ describe('markCampaignFalsePositive', () => { // ============================================================================= describe('autoPostAcknowledgment', () => { - it('first pass: inserts one acknowledge_user row, writes one audit row, posts the note once, returns posted:true', async () => { - stage({ existingAckRows: [] }); + it('first pass: inserts one acknowledge_user row, writes one audit row (with actionId), posts the note once, returns posted:true', async () => { + stage({ existingAckRows: [], insertRemediation: [{ id: 'auto-action-1' }] }); const result = await autoPostAcknowledgment('campaign-1', 'system:auto_report'); @@ -331,6 +360,7 @@ describe('autoPostAcknowledgment', () => { expect(insertCalls).toHaveLength(1); expect(insertCalls[0].sql).toContain("'acknowledge_user'"); expect(insertCalls[0].sql).toContain("'completed'"); + expect(insertCalls[0].sql).toContain('RETURNING id'); expect(insertCalls[0].params).toEqual(['campaign-1', 'system:auto_report']); expect(writeAuditEventMock).toHaveBeenCalledTimes(1); @@ -338,6 +368,7 @@ describe('autoPostAcknowledgment', () => { campaignId: 'campaign-1', actor: 'system:auto_report', eventType: 'remediation_completed', + payload: { actionId: 'auto-action-1', actionType: 'acknowledge_user', auto: true }, }); expect(generateAndPostAcknowledgmentMock).toHaveBeenCalledTimes(1); diff --git a/lib/services/remediation-service.ts b/lib/services/remediation-service.ts index 1d5f8a0..bc44a2d 100644 --- a/lib/services/remediation-service.ts +++ b/lib/services/remediation-service.ts @@ -109,12 +109,27 @@ export async function approveRemediationActions( } const recommendedActions = new Set(parseRecommendedActions(classification.recommended_actions)); + const requestsAcknowledgeUser = actions.some((action) => action.actionType === 'acknowledge_user'); + let acknowledgeUserAlreadyPosted = false; + if (requestsAcknowledgeUser) { + const existingAckRes = await client.query<{ id: string }>( + `SELECT id FROM remediation_actions WHERE campaign_id = $1 AND action_type = 'acknowledge_user' LIMIT 1`, + [campaignId] + ); + acknowledgeUserAlreadyPosted = existingAckRes.rows.length > 0; + } + for (const action of actions) { if (!recommendedActions.has(action.actionType)) { throw new RemediationValidationError( `Action ${action.actionType} is not a recommended action for this campaign` ); } + if (action.actionType === 'acknowledge_user' && acknowledgeUserAlreadyPosted) { + throw new RemediationValidationError( + 'acknowledge_user has already been posted for this campaign' + ); + } } const approved: ApprovedRemediationAction[] = []; @@ -353,9 +368,10 @@ export async function autoPostAcknowledgment( return { inserted: false }; } - await client.query( + const insertRes = await client.query<{ id: string }>( `INSERT INTO remediation_actions (campaign_id, action_type, status, approved_by, approved_at) - VALUES ($1, 'acknowledge_user', 'completed', $2, NOW())`, + VALUES ($1, 'acknowledge_user', 'completed', $2, NOW()) + RETURNING id::text AS id`, [campaignId, actor] ); await writeAuditEvent( @@ -363,7 +379,7 @@ export async function autoPostAcknowledgment( campaignId, actor, eventType: 'remediation_completed', - payload: { actionType: 'acknowledge_user', auto: true }, + payload: { actionId: insertRes.rows[0].id, actionType: 'acknowledge_user', auto: true }, }, client );