From f502da306fdc815e7d9f3996d794f5dcfbdbbc26 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 11 May 2026 10:57:07 +0000 Subject: [PATCH] feat(gui/approvals): payload preview with profile-edit diff + cert-issuance preview (A-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MED-10 closure claim in `cowork/auth-bundles-audit-2026-05-10.md` said "PARTIAL: raw JSON preview; diff library deferred", but the 2026-05-11 verifier hit `web/src/pages/auth/ApprovalsPage.tsx` and found ZERO payload rendering — only a doc-comment mention. Approvers in the GUI were clicking Approve / Reject without seeing the change they were authorizing. That defeats the entire two-person-approval primitive. An approver who can't see what they're approving is rubber-stamping, and a rubber-stamp workflow is operationally indistinguishable from auto-approve except for one false promise of integrity. For `kind=cert_issuance` the payload carries CN / SANs / profile / key algorithm — the catch-the-wildcard-against-corp-internal-profile data. For `kind=profile_edit` the payload carries a `{ before, after }` envelope — the catch-the-must-staple-false-flip data. Without the preview, both attacks land at the approval boundary unchallenged. Closure: each row in the approvals table now carries a `Preview` toggle that expands an inline panel. Dispatch by `kind`: - profile_edit → ProfileEditDiff. Field-level before/after table with red/green cell shading; ONLY changed fields render rows (unchanged fields collapse to keep the diff focused on what needs review); `(unset)` sentinel rendered for added or removed fields so the approver can distinguish "this field was added" from "this field flipped value." For the flat-object profile shape Bundle 1 Phase 9 ships, a field diff carries more signal than a unified line diff would and avoids the external-dep cost. - cert_issuance → IssuanceRequestPreview. Definition list of CN / SANs / profile / key algorithm / must-staple / validity (the load-bearing fields an approver needs to gate the issuance decision). Accepts both `subject_common_name` and `common_name` keys because the certificate-service issuance request uses either on different paths. - any other kind → generic
 JSON dump. Forward-compat for
    future enum additions to migration 000033's CHECK constraint —
    a new approval kind ships rendering through this fallback until
    a kind-specific preview component is written.

The payload arrives over the wire as a base64-encoded JSON string
(Go's json.Marshal renders `[]byte` as base64 by default; see
internal/domain/approval.go:41 where `Payload []byte`). The new
exported `decodePayload(payload)` helper atob()s + JSON.parse()s,
returning null on any failure. Malformed base64 or malformed JSON
renders an explicit "Unable to decode payload" fallback with the
raw value visible to the approver — silent failure on the payload
preview is what produced the original bug in the first place, so
the fix can't have a silent-failure mode.

Component dispatch and base64 decode are also exposed for testing:

  decodePayload(undefined) → null
  decodePayload('') → null
  decodePayload(btoa(JSON.stringify(x))) → x
  decodePayload('!!!not-base64!!!') → null (atob throws)
  decodePayload(btoa('not a json document')) → null (JSON.parse throws)

Each interactive element carries a data-testid so future E2E
coverage can exercise the contract without brittle CSS selectors —
same pattern as Bundle 1's RolesPage.

Tests (13 total, all passing under vitest):

Page-level (8):
  A-5 Preview button toggles the payload panel
  A-5 ProfileEdit kind renders field diff with changed-only rows
  A-5 ProfileEdit before/after values are visible in the diff cells
  A-5 ProfileEdit with no changes renders empty-state
  A-5 CertIssuance renders definition list with SANs + profile + key algo
  A-5 Unknown kind falls back to generic JSON pre block
  A-5 Empty payload renders the "No payload attached" sentinel
  A-5 Malformed base64 payload renders the decode-error fallback

decodePayload pure-function suite (5):
  returns null for undefined input
  returns null for empty string
  round-trips base64-encoded JSON
  returns null on malformed base64
  returns null on valid base64 of non-JSON content

Verify gate green: tsc --noEmit clean; vitest passes all 17 tests
in ApprovalsPage.test.tsx (the 4 pre-existing tests still green —
the new preview row doesn't break the existing same-actor self-lock
+ approve-POST tests; new column header increments the colSpan but
the existing rows render unchanged).

Spec at cowork/auth-bundles-fixes-2026-05-11/05-high-approvals-payload-preview.md.
Audit doc: MED-10 row in `cowork/auth-bundles-audit-2026-05-10.md`
status table flipped from `PARTIAL (raw JSON preview; diff library
deferred)` to `CLOSED 2026-05-11 (A-5)`; the MED-10 section body
gains the A-5 follow-on closure annotation with the false-claim
verification and the three-mode rendering breakdown.
Operator-visible CHANGELOG.md entry under Security explains what
changed and why it matters — approvers can now see what they're
approving.
---
 CHANGELOG.md                              |  18 ++
 web/src/pages/auth/ApprovalsPage.test.tsx | 242 +++++++++++++++
 web/src/pages/auth/ApprovalsPage.tsx      | 348 ++++++++++++++++++----
 3 files changed, 549 insertions(+), 59 deletions(-)

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 && ( + + + + + + )} +
); })}