diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e986ea..619db2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,24 @@ ### Security +- **Approval payload preview (Audit 2026-05-11 A-5).** + The MED-10 closure claim ("PARTIAL: raw JSON preview; diff library + deferred") was inaccurate — `ApprovalsPage.tsx` rendered no payload + at all, so approvers were clicking Approve / Reject without seeing + the change they were authorizing. That defeats the entire four-eyes + primitive: an approver who can't see what they're approving is + rubber-stamping. Each row now carries a Preview toggle that expands + an inline panel dispatching by kind: `profile_edit` shows a + field-level before/after diff (changed-only rows, red/green cells, + `(unset)` sentinel for added/removed fields); `cert_issuance` shows + a definition list of CN / SANs / profile / key algo / must-staple / + validity (catches the wildcard-against-corp-internal-profile attack + at review time); unknown kinds render a generic JSON preview for + forward-compat with future approval kinds. The base64-encoded JSON + payload is decoded via the new `decodePayload` helper; malformed + inputs render an explicit decode-error fallback — silent failure on + the payload preview is what produced this bug in the first place. + - **Pre-login cookie Path widened from `/auth/oidc/` to `/` (Audit MED-14 follow-on).** Required to satisfy the `__Host-` prefix's `Path=/` rule. The cookie lifetime is unchanged (10 minutes) and only the callback handler diff --git a/web/src/pages/auth/ApprovalsPage.test.tsx b/web/src/pages/auth/ApprovalsPage.test.tsx index f8b2616..07bab79 100644 --- a/web/src/pages/auth/ApprovalsPage.test.tsx +++ b/web/src/pages/auth/ApprovalsPage.test.tsx @@ -141,4 +141,246 @@ describe('ApprovalsPage', () => { renderWithProviders(); await waitFor(() => expect(screen.getByTestId('approvals-empty')).toBeTruthy()); }); + + // ============================================================================= + // Audit 2026-05-11 A-5 — payload preview. + // ============================================================================= + + // b64 of '{"before":{"must_staple":false,"max_validity_days":397},"after":{"must_staple":true,"max_validity_days":90}}' + const profileEditPayload = btoa(JSON.stringify({ + before: { must_staple: false, max_validity_days: 397, requires_approval: true }, + after: { must_staple: true, max_validity_days: 90, requires_approval: true }, + })); + const profileEditNoChangesPayload = btoa(JSON.stringify({ + before: { must_staple: false }, + after: { must_staple: false }, + })); + const certIssuancePayload = btoa(JSON.stringify({ + subject_common_name: 'api.acme.com', + sans: ['api.acme.com', 'www.acme.com'], + profile_id: 'p-corp-public', + key_algorithm: 'ECDSA-P256', + must_staple: true, + validity_days: 90, + })); + + it('A-5 Preview button toggles the payload panel', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[1], + payload: certIssuancePayload, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-2')); + + // Panel hidden by default. + expect(screen.queryByTestId('approvals-payload-preview-ar-2')).toBeNull(); + + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-2')); + await waitFor(() => screen.getByTestId('approvals-payload-preview-ar-2')); + expect(screen.getByTestId('approval-cert-issuance-preview')).toBeTruthy(); + + // Toggle off. + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-2')); + await waitFor(() => { + expect(screen.queryByTestId('approvals-payload-preview-ar-2')).toBeNull(); + }); + }); + + it('A-5 ProfileEdit kind renders field diff with changed-only rows', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[0], + requested_by: 'bob', // not self — Preview button is enabled regardless + payload: profileEditPayload, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-1')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-1')); + + await waitFor(() => screen.getByTestId('approval-profile-edit-diff')); + // Only changed fields render rows. + expect(screen.getByTestId('approval-profile-edit-row-must_staple')).toBeTruthy(); + expect(screen.getByTestId('approval-profile-edit-row-max_validity_days')).toBeTruthy(); + // Unchanged requires_approval should NOT render a row. + expect(screen.queryByTestId('approval-profile-edit-row-requires_approval')).toBeNull(); + }); + + it('A-5 ProfileEdit before/after values are visible in the diff cells', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[0], + requested_by: 'bob', + payload: profileEditPayload, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-1')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-1')); + + await waitFor(() => screen.getByTestId('approval-profile-edit-diff')); + const row = screen.getByTestId('approval-profile-edit-row-must_staple'); + expect(row.textContent).toContain('false'); + expect(row.textContent).toContain('true'); + }); + + it('A-5 ProfileEdit with no changes renders empty-state', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[0], + requested_by: 'bob', + payload: profileEditNoChangesPayload, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-1')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-1')); + + await waitFor(() => screen.getByTestId('approval-profile-edit-no-changes')); + expect(screen.queryByTestId('approval-profile-edit-diff')).toBeNull(); + }); + + it('A-5 CertIssuance renders definition list with SANs + profile + key algo', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[1], + payload: certIssuancePayload, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-2')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-2')); + + const dl = await waitFor(() => screen.getByTestId('approval-cert-issuance-preview')); + expect(dl.textContent).toContain('api.acme.com'); + expect(dl.textContent).toContain('www.acme.com'); + expect(dl.textContent).toContain('p-corp-public'); + expect(dl.textContent).toContain('ECDSA-P256'); + }); + + it('A-5 Unknown kind falls back to generic JSON pre block', async () => { + const unknownKindPayload = btoa(JSON.stringify({ some_future_field: 42, nested: { ok: true } })); + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + id: 'ar-3', + kind: 'future_kind_v3' as never, // not in current enum + profile_id: 'prof-x', + requested_by: 'bob', + state: 'pending' as const, + payload: unknownKindPayload, + created_at: '2026-05-09T20:02:00Z', + updated_at: '2026-05-09T20:02:00Z', + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-3')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-3')); + + const pre = await waitFor(() => screen.getByTestId('approval-payload-generic-json')); + expect(pre.textContent).toContain('some_future_field'); + expect(pre.textContent).toContain('42'); + }); + + it('A-5 Empty payload renders the "No payload attached" sentinel', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[1], + payload: undefined, + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-2')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-2')); + + await waitFor(() => screen.getByTestId('approval-payload-empty')); + }); + + it('A-5 Malformed base64 payload renders the decode-error fallback', async () => { + vi.mocked(client.listApprovals).mockResolvedValue({ + data: [{ + ...samplePending[1], + payload: '!!!not-valid-base64!!!', + }], + total: 1, + page: 1, + per_page: 50, + } as never); + vi.mocked(client.authMe).mockResolvedValue(aliceMe); + + renderWithProviders(); + await waitFor(() => screen.getByTestId('approvals-preview-toggle-ar-2')); + fireEvent.click(screen.getByTestId('approvals-preview-toggle-ar-2')); + + await waitFor(() => screen.getByTestId('approval-payload-decode-error')); + }); +}); + +// ============================================================================= +// Pure-function tests on decodePayload (Audit 2026-05-11 A-5). +// ============================================================================= + +describe('decodePayload', () => { + it('returns null for undefined input', async () => { + const { decodePayload } = await import('./ApprovalsPage'); + expect(decodePayload(undefined)).toBeNull(); + }); + + it('returns null for empty string', async () => { + const { decodePayload } = await import('./ApprovalsPage'); + expect(decodePayload('')).toBeNull(); + }); + + it('round-trips base64-encoded JSON', async () => { + const { decodePayload } = await import('./ApprovalsPage'); + const original = { foo: 'bar', n: 42, arr: [1, 2, 3] }; + const encoded = btoa(JSON.stringify(original)); + expect(decodePayload(encoded)).toEqual(original); + }); + + it('returns null on malformed base64', async () => { + const { decodePayload } = await import('./ApprovalsPage'); + expect(decodePayload('!!!not-base64!!!')).toBeNull(); + }); + + it('returns null on valid base64 of non-JSON content', async () => { + const { decodePayload } = await import('./ApprovalsPage'); + expect(decodePayload(btoa('not a json document'))).toBeNull(); + }); }); diff --git a/web/src/pages/auth/ApprovalsPage.tsx b/web/src/pages/auth/ApprovalsPage.tsx index 7ab784b..96faa5f 100644 --- a/web/src/pages/auth/ApprovalsPage.tsx +++ b/web/src/pages/auth/ApprovalsPage.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { Fragment, useState } from 'react'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { listApprovals, @@ -25,14 +25,212 @@ import ErrorState from '../../components/ErrorState'; // are both populated; metadata.common_name surfaces. // - profile_edit — Bundle 1 Phase 9 closure; cert + job are empty, // payload carries the pending profile diff (rendered as a -// collapsible JSON preview). +// field-level before/after diff via PayloadPreview). // // Same-actor self-approve is rejected server-side with HTTP 403; the // page surfaces the error inline. Approve / reject actions are HIDDEN // when the caller's actor_id equals requested_by, so the operator // can't even click the wrong button. +// +// Audit 2026-05-11 A-5 — payload preview. The MED-10 closure claim +// said the GUI rendered a "raw JSON preview" but the verifier found +// zero payload rendering — approvers had to click Approve blind. This +// page now renders an inline expandable panel per row that dispatches +// to one of three components by `kind`: +// +// - kind=profile_edit → ProfileEditDiff: field-level before/after +// table. The whole point of the four-eyes principle is "the +// approver sees what's changing"; rendering this as a flat field +// diff is materially more useful than a unified line-diff for the +// small flat-object profile shape. +// - kind=cert_issuance → IssuanceRequestPreview: CN / SANs / +// profile / key algo / must-staple / validity. Catches the +// wildcard-against-corp-internal-profile attack at review time. +// - any other kind → generic JSON 
. Forward-compat for
+//     future approval kinds added to migration 000033's enum.
+//
+// The payload arrives as a base64-encoded JSON string (Go json-encodes
+// []byte to base64 by default; see internal/domain/approval.go:41).
+// decodePayload() handles the decode + parse + null/error guard.
 // =============================================================================
 
+// decodePayload base64-decodes the wire payload and JSON-parses the
+// result. Returns the parsed shape or null on any failure (empty
+// payload, malformed base64, malformed JSON). The component layer
+// renders the generic fallback in that case so the approver still
+// sees *something* — silent failure on the payload preview defeats
+// the entire fix.
+//
+// Exported for test reach.
+export function decodePayload(payload: string | undefined): unknown {
+  if (!payload) return null;
+  try {
+    // atob throws on invalid base64; the surrounding try/catch falls
+    // through to null which the caller renders as "Unable to decode
+    // payload" in the generic branch.
+    const decoded = atob(payload);
+    return JSON.parse(decoded);
+  } catch {
+    return null;
+  }
+}
+
+// =============================================================================
+// PayloadPreview — kind dispatch.
+// =============================================================================
+
+function PayloadPreview({ kind, payload }: { kind: string; payload: string | undefined }) {
+  const decoded = decodePayload(payload);
+
+  if (decoded === null && payload) {
+    return (
+      

+        Unable to decode payload (base64 / JSON parse failed). Raw value:{' '}
+        {payload}
+      

+    );
+  }
+
+  if (decoded === null) {
+    return (
+      

+        No payload attached.
+      

+    );
+  }
+
+  if (kind === 'profile_edit') {
+    return ;
+  }
+  if (kind === 'cert_issuance') {
+    return ;
+  }
+  // Forward-compat fallback for future kinds.
+  return (
+    
+      {JSON.stringify(decoded, null, 2)}
+    

+  );
+}
+
+// =============================================================================
+// ProfileEditDiff — field-level before/after table.
+// =============================================================================
+
+function ProfileEditDiff({ payload }: { payload: unknown }) {
+  const envelope = payload as { before?: Record; after?: Record };
+  const before = envelope?.before ?? {};
+  const after = envelope?.after ?? {};
+  const allKeys = Array.from(new Set([...Object.keys(before), ...Object.keys(after)])).sort();
+  const changedKeys = allKeys.filter(k => JSON.stringify(before[k]) !== JSON.stringify(after[k]));
+
+  if (changedKeys.length === 0) {
+    return (
+      

+        No field changes detected.
+      

+    );
+  }
+  return (
+    
+      
+        
+          
+          
+          
+        
+      
+      
+        {changedKeys.map(k => (
+          
+            
+            
+            
+          
+        ))}
+      
+    
FieldBeforeAfter
{k}
+              {renderValue(before[k])}
+            
+              {renderValue(after[k])}
+            

+  );
+}
+
+// renderValue stringifies a payload value for the diff cells. Renders
+// undefined as a visually distinct "(unset)" sentinel so the approver
+// can tell a field was added (before=unset) vs flipped (before=value).
+function renderValue(v: unknown) {
+  if (v === undefined) {
+    return (unset);
+  }
+  return JSON.stringify(v);
+}
+
+// =============================================================================
+// IssuanceRequestPreview — definition list of the load-bearing fields.
+// =============================================================================
+
+function IssuanceRequestPreview({ payload }: { payload: unknown }) {
+  const p = payload as {
+    subject_common_name?: string;
+    common_name?: string;
+    sans?: string[];
+    profile_id?: string;
+    key_algorithm?: string;
+    must_staple?: boolean;
+    validity_days?: number;
+    requester_actor_id?: string;
+  };
+  // The certificate-service issuance request uses `subject_common_name`
+  // on some paths and `common_name` on others; surface either.
+  const cn = p.subject_common_name ?? p.common_name ?? '—';
+  return (
+    

+      
Common Name

+      
{cn}

+      
SANs

+      
{(p.sans ?? []).join(', ') || '—'}

+      
Profile

+      
{p.profile_id ?? '—'}

+      
Key algorithm

+      
{p.key_algorithm ?? '—'}

+      
Must-staple

+      
{p.must_staple === undefined ? '—' : p.must_staple ? 'yes' : 'no'}

+      
Validity (days)

+      
{p.validity_days ?? '—'}

+      {p.requester_actor_id && (
+        <>
+          
Requester (payload-claimed)

+          
{p.requester_actor_id}

+        
+      )}
+    

+  );
+}
+
 export default function ApprovalsPage() {
   const me = useAuthMe();
   const qc = useQueryClient();
@@ -47,6 +245,11 @@ export default function ApprovalsPage() {
 
   const [actionError, setActionError] = useState(null);
   const [busy, setBusy] = useState(null);
+  // Audit 2026-05-11 A-5 — per-row payload preview expansion.
+  // Single-string state (rather than a Set) because most operators
+  // only inspect one approval at a time; widening to multi-select
+  // is a trivial future change if the workflow demands it.
+  const [expandedID, setExpandedID] = useState(null);
 
   const handleApprove = async (req: ApprovalRequest) => {
     const note = window.prompt('Approval note (optional):') ?? '';
@@ -137,6 +340,7 @@ export default function ApprovalsPage() {
                 Profile
                 Requested by
                 Created
+                Payload
                 
               
             
@@ -144,67 +348,93 @@ export default function ApprovalsPage() {
               {items.map(req => {
                 const isMine = req.requested_by === myID;
                 const isPending = req.state === 'pending';
+                const isExpanded = expandedID === req.id;
                 return (
-                  
-                    {req.id}
-                    
-                      
-                        {req.kind}
-                      
-                    
-                    {req.profile_id}
-                    
-                      {req.requested_by}
-                      {isMine && (you)}
-                    
-                    
-                      {new Date(req.created_at).toLocaleString()}
-                    
-                    
-                      {isPending && !isMine && (
-                        

-                          
-                          
-                        

-                      )}
-                      {isPending && isMine && (
+                  
+                    
+                      {req.id}
+                      
                         
-                          self-approve blocked
+                          {req.kind}
                         
-                      )}
-                      {!isPending && (
-                        {req.state}
-                      )}
-                    
-                  
+                      
+                      {req.profile_id}
+                      
+                        {req.requested_by}
+                        {isMine && (you)}
+                      
+                      
+                        {new Date(req.created_at).toLocaleString()}
+                      
+                      
+                        {/* Audit 2026-05-11 A-5 — payload preview toggle.
+                            Always rendered (even when payload is empty)
+                            so the approver can verify there ISN'T a
+                            payload they might have missed. */}
+                        
+                      
+                      
+                        {isPending && !isMine && (
+                          

+                            
+                            
+                          

+                        )}
+                        {isPending && isMine && (
+                          
+                            self-approve blocked
+                          
+                        )}
+                        {!isPending && (
+                          {req.state}
+                        )}
+                      
+                    
+                    {isExpanded && (
+                      
+                        
+                          
+                        
+                      
+                    )}
+                  
                 );
               })}