From 536205c95d5e68ba7c4c421d391820978ee77787 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sat, 8 Aug 2026 00:28:46 +0200
Subject: [PATCH] sec(frontend): escape payment field in plans and recs tables

purchase.payment (plans.ts) and rec.payment (recommendations.ts) were
interpolated raw into innerHTML while every sibling field in the same
row was escaped with the existing escapeHtml() helper. payment is a
bare string in both PlannedPurchase and LocalRecommendation, so an
attacker-controlled or provider-echoed value with markup would be
parsed and executed in the viewer's session.

Wraps both interpolations with escapeHtml() and defaults the
plans.ts term cell's payment to an empty string so the unguarded
.replace() call no longer throws on a missing value.

Closes #1633
Closes #1632
---
 .../__tests__/xss-purchase-payment.test.ts    | 199 ++++++++++++++++++
 .../xss-recommendations-payment.test.ts       | 194 +++++++++++++++++
 frontend/src/plans.ts                         |   2 +-
 frontend/src/recommendations.ts               |   2 +-
 4 files changed, 395 insertions(+), 2 deletions(-)
 create mode 100644 frontend/src/__tests__/xss-purchase-payment.test.ts
 create mode 100644 frontend/src/__tests__/xss-recommendations-payment.test.ts

diff --git a/frontend/src/__tests__/xss-purchase-payment.test.ts b/frontend/src/__tests__/xss-purchase-payment.test.ts
new file mode 100644
index 000000000..ba5cba738
--- /dev/null
+++ b/frontend/src/__tests__/xss-purchase-payment.test.ts
@@ -0,0 +1,199 @@
+/**
+ * Regression tests for stored XSS via purchase.payment rendered unescaped
+ * inside the planned-purchases term cell (issue #1633).
+ *
+ * plans.ts:renderPlannedPurchaseRow used to emit:
+ *   <td>${purchase.term}yr ${purchase.payment.replace('-', ' ')}</td>
+ *
+ * Every sibling string field in the same row (plan_name, service,
+ * resource_type, region, status) is escaped with the existing
+ * escapeHtml() from utils.ts -- see xss-purchase-status.test.ts (#445) --
+ * but `payment` was the one field left raw. `payment` is typed as a bare
+ * `string` in api/types.ts (not a closed union like `status`), so nothing
+ * on the frontend constrains its content. The fix wraps the interpolation
+ * with escapeHtml() and defaults a missing value to '' so the unguarded
+ * .replace() call on a null/undefined payment no longer throws.
+ */
+
+import { loadPlans } from '../plans';
+
+// Use the real escapeHtml so the DOM-based escaping is exercised, not a stub.
+jest.mock('../utils', () => {
+  const actual = jest.requireActual<typeof import('../utils')>('../utils');
+  return {
+    ...actual,
+    formatDate: jest.fn((val: string) => val ? new Date(val).toLocaleDateString() : ''),
+    formatTerm: jest.fn((years: number | null | undefined) =>
+      years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`),
+    formatRampSchedule: jest.fn((val: string) => val || 'Unknown'),
+    formatCurrency: jest.fn((val: number) => `$${val || 0}`),
+    populateAccountFilter: jest.fn(() => Promise.resolve()),
+  };
+});
+
+jest.mock('../api', () => ({
+  getPlans: jest.fn(),
+  getPlannedPurchases: jest.fn(),
+  getPlan: jest.fn(),
+  createPlan: jest.fn(),
+  updatePlan: jest.fn(),
+  patchPlan: jest.fn(),
+  deletePlan: jest.fn(),
+  runPlannedPurchase: jest.fn(),
+  pausePlannedPurchase: jest.fn(),
+  resumePlannedPurchase: jest.fn(),
+  deletePlannedPurchase: jest.fn(),
+  createPlannedPurchases: jest.fn(),
+  listPlanAccounts: jest.fn().mockResolvedValue([]),
+  setPlanAccounts: jest.fn().mockResolvedValue(undefined),
+  listAccounts: jest.fn().mockResolvedValue([]),
+}));
+
+jest.mock('../state', () => ({
+  getRecommendations: jest.fn().mockReturnValue([]),
+  getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()),
+  getVisibleRecommendations: jest.fn().mockReturnValue([]),
+  setVisibleRecommendations: jest.fn(),
+  getCurrentProvider: jest.fn().mockReturnValue(''),
+  setCurrentProvider: jest.fn(),
+  getCurrentAccountIDs: jest.fn().mockReturnValue([]),
+  setCurrentAccountIDs: jest.fn(),
+  subscribeProvider: jest.fn().mockReturnValue(() => {}),
+  subscribeAccount: jest.fn().mockReturnValue(() => {}),
+  getCurrentUser: jest.fn().mockReturnValue({ id: 'u-admin', email: 'admin@example.com', groups: ['00000000-0000-5000-8000-000000000001'] }),
+  getPlansColumnFilters: jest.fn().mockReturnValue({}),
+  setPlansColumnFilter: jest.fn(),
+  clearAllPlansColumnFilters: jest.fn(),
+}));
+
+jest.mock('../history', () => ({
+  viewPlanHistory: jest.fn(),
+}));
+
+jest.mock('../commitmentOptions', () => ({
+  populateTermSelect: jest.fn(),
+  populatePaymentSelect: jest.fn(),
+  isValidCombination: jest.fn().mockReturnValue(true),
+  normalizePaymentValue: jest.fn((value: unknown) => value),
+}));
+
+jest.mock('../toast', () => ({
+  showToast: jest.fn(() => ({ dismiss: jest.fn() })),
+}));
+
+jest.mock('../confirmDialog', () => ({
+  confirmDialog: jest.fn(() => Promise.resolve(true)),
+}));
+
+jest.mock('../modal', () => ({
+  openModal: jest.fn(),
+  closeModal: jest.fn(),
+}));
+
+jest.mock('../archera', () => ({
+  openArcheraOfferModal: jest.fn(),
+}));
+
+jest.mock('../lib/skeleton', () => ({
+  showSkeletonTiles: jest.fn(),
+  showSkeletonRows: jest.fn(),
+  teardownSkeleton: jest.fn(),
+}));
+
+jest.mock('../permissions', () => ({
+  canAccess: jest.fn().mockReturnValue(true),
+}));
+
+import * as api from '../api';
+
+const SCRIPT_PAYLOAD = '<script>alert(1)</script>';
+const IMG_PAYLOAD = '"><img src=x onerror="alert(1)">';
+
+function makePurchase(payment: string | undefined) {
+  const purchase: Record<string, unknown> = {
+    id: 'pp-1',
+    plan_id: 'plan-1',
+    plan_name: 'Test Plan',
+    scheduled_date: '2024-06-01',
+    provider: 'aws',
+    service: 'ec2',
+    resource_type: 't3.medium',
+    region: 'us-east-1',
+    count: 1,
+    term: 1,
+    payment,
+    estimated_savings: 50,
+    upfront_cost: 0,
+    status: 'pending',
+    step_number: 1,
+    total_steps: 1,
+  };
+  if (payment === undefined) delete purchase['payment'];
+  return purchase;
+}
+
+describe('XSS regression: purchase.payment rendered as text, not markup (#1633)', () => {
+  beforeEach(() => {
+    document.body.innerHTML = `
+      <div id="plans-list"></div>
+      <div id="planned-purchases-list"></div>
+      <button id="new-plan-btn"></button>
+    `;
+    jest.clearAllMocks();
+    (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] });
+  });
+
+  test('script-tag payload in purchase.payment does not create a <script> element', async () => {
+    (api.getPlannedPurchases as jest.Mock).mockResolvedValue({
+      purchases: [makePurchase(SCRIPT_PAYLOAD)],
+    });
+
+    await loadPlans();
+
+    // No <script> element should have been injected.
+    expect(document.querySelectorAll('script').length).toBe(0);
+
+    const list = document.getElementById('planned-purchases-list');
+    expect(list).not.toBeNull();
+    // Raw tags must not appear in the rendered markup.
+    expect(list!.innerHTML).not.toContain('<script>');
+    // The payload must appear only as escaped text content (the term cell
+    // also runs payment through .replace('-', ' '), which no-ops here since
+    // the payload has no hyphen).
+    expect(list!.textContent).toContain(SCRIPT_PAYLOAD);
+  });
+
+  test('img-onerror payload in purchase.payment does not inject a live <img> element', async () => {
+    (api.getPlannedPurchases as jest.Mock).mockResolvedValue({
+      purchases: [makePurchase(IMG_PAYLOAD)],
+    });
+
+    await loadPlans();
+
+    const list = document.getElementById('planned-purchases-list');
+    expect(list).not.toBeNull();
+    // Strongest assertion: no <img> DOM node was ever parsed out of the cell.
+    expect(list!.querySelectorAll('img').length).toBe(0);
+    expect(list!.textContent).toContain('onerror');
+  });
+
+  test('valid payment "no-upfront" still renders as readable text with hyphen replaced', async () => {
+    (api.getPlannedPurchases as jest.Mock).mockResolvedValue({
+      purchases: [makePurchase('no-upfront')],
+    });
+
+    await loadPlans();
+
+    const list = document.getElementById('planned-purchases-list');
+    expect(list).not.toBeNull();
+    expect(list!.textContent).toContain('no upfront');
+  });
+
+  test('missing purchase.payment does not throw (guarded, not just escaped)', async () => {
+    (api.getPlannedPurchases as jest.Mock).mockResolvedValue({
+      purchases: [makePurchase(undefined)],
+    });
+
+    await expect(loadPlans()).resolves.not.toThrow();
+  });
+});
diff --git a/frontend/src/__tests__/xss-recommendations-payment.test.ts b/frontend/src/__tests__/xss-recommendations-payment.test.ts
new file mode 100644
index 000000000..b2b76dc73
--- /dev/null
+++ b/frontend/src/__tests__/xss-recommendations-payment.test.ts
@@ -0,0 +1,194 @@
+/**
+ * Regression tests for stored XSS via rec.payment rendered unescaped in the
+ * Opportunities table's payment column (issue #1632).
+ *
+ * recommendations.ts:renderColumnCell used to emit:
+ *   case 'payment': return `<td>${formatPayment(rec.payment)}</td>`;
+ *
+ * while every other string-bearing arm of the same switch (provider,
+ * service, resource_type, capacity, region) is escaped with escapeHtml().
+ * formatPayment() passes any value that is not a key of
+ * PAYMENT_DISPLAY_LABELS straight through, so an unrecognised `payment`
+ * string reached innerHTML verbatim. `payment` is a bare `string` on
+ * LocalRecommendation (types.ts), so nothing on the frontend constrains
+ * its content. The fix wraps the emission site with escapeHtml().
+ */
+
+// Mock the api module
+jest.mock('../api', () => ({
+  getRecommendations: jest.fn(),
+  refreshRecommendations: jest.fn(),
+  listAccounts: jest.fn().mockResolvedValue([]),
+  listAccountsMinimal: jest.fn().mockResolvedValue([]),
+  getConfig: jest.fn().mockResolvedValue({ global: {} }),
+  listAccountServiceOverrides: jest.fn().mockResolvedValue([]),
+}));
+
+jest.mock('../api/recommendations', () => ({
+  getRecommendationDetail: jest.fn().mockResolvedValue({
+    id: 'rec-default',
+    usage_history: [],
+    confidence_bucket: 'low',
+    provenance_note: '',
+  }),
+  getRecommendationsFreshness: jest.fn().mockResolvedValue({
+    last_collected_at: new Date(Date.now() - 60 * 60 * 1000).toISOString(),
+    last_collection_error: null,
+  }),
+  refreshRecommendations: jest.fn().mockResolvedValue({}),
+}));
+
+const mockShowToast = jest.fn<{ dismiss: () => void }, [unknown]>(() => ({ dismiss: jest.fn() }));
+jest.mock('../toast', () => ({
+  showToast: (opts: unknown) => mockShowToast(opts),
+}));
+
+jest.mock('../state', () => ({
+  getCurrentProvider: jest.fn().mockReturnValue('all'),
+  setCurrentProvider: jest.fn(),
+  getCurrentAccountIDs: jest.fn().mockReturnValue([]),
+  setCurrentAccountIDs: jest.fn(),
+  getRecommendations: jest.fn().mockReturnValue([]),
+  getRecommendationByID: jest.fn().mockReturnValue(undefined),
+  setRecommendations: jest.fn(),
+  getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()),
+  clearSelectedRecommendations: jest.fn(),
+  addSelectedRecommendation: jest.fn(),
+  removeSelectedRecommendation: jest.fn(),
+  getRecommendationsSort: jest.fn().mockReturnValue({ column: 'savings', direction: 'desc' }),
+  setRecommendationsSort: jest.fn(),
+  getRecommendationsColumnFilters: jest.fn().mockReturnValue({}),
+  setRecommendationsColumnFilter: jest.fn(),
+  clearAllRecommendationsColumnFilters: jest.fn(),
+  getVisibleRecommendations: jest.fn().mockReturnValue([]),
+  setVisibleRecommendations: jest.fn(),
+  getCostPeriod: jest.fn().mockReturnValue('monthly'),
+  setCostPeriod: jest.fn(),
+  getHiddenColumns: jest.fn().mockReturnValue(new Set()),
+  setHiddenColumns: jest.fn(),
+  getCurrentUser: jest.fn().mockReturnValue({ id: 'u-admin', email: 'admin@example.com', groups: ['00000000-0000-5000-8000-000000000001'] }),
+  subscribeProvider: jest.fn(),
+  subscribeAccount: jest.fn(),
+}));
+
+// Use the real escapeHtml so the DOM-based escaping is exercised, not a
+// stub. formatCurrency/formatTerm are stubbed for readable assertions,
+// matching recommendations.test.ts's utils mock shape.
+jest.mock('../utils', () => {
+  const actual = jest.requireActual<typeof import('../utils')>('../utils');
+  return {
+    ...actual,
+    formatCurrency: jest.fn((val: number) => `$${val || 0}`),
+    formatTerm: jest.fn((years: number | null | undefined) =>
+      years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`),
+    populateAccountFilter: jest.fn(() => Promise.resolve()),
+    CURRENCY_DEFAULT_DIGITS: 0,
+  };
+});
+
+import { loadRecommendations } from '../recommendations';
+import * as api from '../api';
+
+const SCRIPT_PAYLOAD = '<script>alert(1)</script>';
+const IMG_PAYLOAD = '"><img src=x onerror="alert(1)">';
+
+function makeRec(payment: string) {
+  return {
+    id: 'rec-1',
+    provider: 'aws',
+    service: 'ec2',
+    resource_type: 't3.medium',
+    region: 'us-east-1',
+    count: 1,
+    term: 1,
+    payment,
+    savings: 100,
+    upfront_cost: 500,
+  };
+}
+
+describe('XSS regression: rec.payment rendered as text, not markup (#1632)', () => {
+  beforeEach(() => {
+    document.body.innerHTML = `
+      <div id="opportunities-tab" class="tab-content active">
+        <div id="recommendations-summary"></div>
+        <div id="recommendations-list"></div>
+      </div>
+      <div id="purchase-modal" class="hidden">
+        <div id="purchase-details"></div>
+        <div class="modal-buttons">
+          <button type="button" id="close-purchase-modal-btn">Cancel</button>
+          <button type="button" id="execute-purchase-btn" class="primary">Send for Approval</button>
+        </div>
+      </div>
+    `;
+    jest.clearAllMocks();
+    jest.useFakeTimers();
+    (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [], regions: [] });
+  });
+
+  afterEach(() => {
+    jest.useRealTimers();
+  });
+
+  test('script-tag payload in rec.payment does not create a <script> element', async () => {
+    (api.getRecommendations as jest.Mock).mockResolvedValue({
+      summary: {},
+      recommendations: [makeRec(SCRIPT_PAYLOAD)],
+      regions: ['us-east-1'],
+    });
+
+    await loadRecommendations();
+
+    expect(document.querySelectorAll('script').length).toBe(0);
+
+    const list = document.getElementById('recommendations-list');
+    expect(list).not.toBeNull();
+    expect(list!.innerHTML).not.toContain('<script>');
+    expect(list!.textContent).toContain(SCRIPT_PAYLOAD);
+  });
+
+  test('img-onerror payload in rec.payment does not inject a live <img> element', async () => {
+    (api.getRecommendations as jest.Mock).mockResolvedValue({
+      summary: {},
+      recommendations: [makeRec(IMG_PAYLOAD)],
+      regions: ['us-east-1'],
+    });
+
+    await loadRecommendations();
+
+    const list = document.getElementById('recommendations-list');
+    expect(list).not.toBeNull();
+    // Strongest assertion: no <img> DOM node was ever parsed out of the cell.
+    expect(list!.querySelectorAll('img').length).toBe(0);
+    expect(list!.textContent).toContain('onerror');
+  });
+
+  test('known payment option "all-upfront" still renders its display label', async () => {
+    (api.getRecommendations as jest.Mock).mockResolvedValue({
+      summary: {},
+      recommendations: [makeRec('all-upfront')],
+      regions: ['us-east-1'],
+    });
+
+    await loadRecommendations();
+
+    const list = document.getElementById('recommendations-list');
+    expect(list).not.toBeNull();
+    expect(list!.textContent).toContain('All Upfront');
+  });
+
+  test('unrecognised payment value falls through formatPayment as escaped text', async () => {
+    (api.getRecommendations as jest.Mock).mockResolvedValue({
+      summary: {},
+      recommendations: [makeRec('some-future-option')],
+      regions: ['us-east-1'],
+    });
+
+    await loadRecommendations();
+
+    const list = document.getElementById('recommendations-list');
+    expect(list).not.toBeNull();
+    expect(list!.textContent).toContain('some-future-option');
+  });
+});
diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts
index ae02d4d3a..4a9e677cd 100644
--- a/frontend/src/plans.ts
+++ b/frontend/src/plans.ts
@@ -687,7 +687,7 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string {
   const isPending = purchase.status === 'pending';
   const canRun = isPending || isPaused;
   const termCell = purchase.term > 0
-    ? `${purchase.term}yr ${purchase.payment.replace('-', ' ')}`
+    ? `${purchase.term}yr ${escapeHtml((purchase.payment ?? '').replace('-', ' '))}`
     : '—';
   // Show an em-dash for upfront=0 unless the plan truly is all-upfront;
   // $0 upfront on "partial" or "no-upfront" is informative ($0 is real).
diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts
index da2afce6b..d0d7d3322 100644
--- a/frontend/src/recommendations.ts
+++ b/frontend/src/recommendations.ts
@@ -2951,7 +2951,7 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen
     case 'term':
       return `<td>${formatTerm(rec.term)}</td>`;
     case 'payment':
-      return `<td>${formatPayment(rec.payment)}</td>`;
+      return `<td>${escapeHtml(formatPayment(rec.payment))}</td>`;
     case 'savings':
       // issue #319: savings display value scales with the active cost period.
       return `<td class="savings">${formatCostForPeriod(rec.savings, ctx.period)}</td>`;