sec(frontend): escape payment field in plans and recs tables

[cristim]

Summary

Both issues are the same defect: payment interpolated raw into an innerHTML template while every sibling field in the same table row was already escaped with escapeHtml(). Fixed together.

• frontend/src/plans.ts:690 (renderPlannedPurchaseRow, term cell) - purchase.payment.replace('-', ' ') was unescaped. The very next cell, escapeHtml(purchase.status), is the sec(frontend): purchase.status rendered unescaped in plans.ts:173 — text-node XSS #445 fix; payment is the field that was left raw.
• frontend/src/recommendations.ts:2954 (renderColumnCell, case 'payment') - formatPayment(rec.payment) was unescaped, the one exception in a switch where every other string arm (provider, service, resource_type, capacity, region) calls escapeHtml().

What payment can contain

payment is typed as a bare string on both PlannedPurchase and LocalRecommendation (frontend/src/api/types.ts), not a closed union like status. Nothing on the frontend constrains its content before render. formatPayment() passes any value that isn't a key of PAYMENT_DISPLAY_LABELS straight through unchanged, so an unrecognised value reaches innerHTML verbatim. The backend and any provider-echoed or partially-validated write path are the realistic sources; this is a stored-XSS path, not merely theoretical, since the type system provides zero protection.

Fix

// plans.ts
const termCell = purchase.term > 0
  ? `${purchase.term}yr ${escapeHtml((purchase.payment ?? '').replace('-', ' '))}`
  : '—';

// recommendations.ts
case 'payment':
  return `<td>${escapeHtml(formatPayment(rec.payment))}</td>`;

Escaping only, using the existing escapeHtml() helper - no new escaper, no rendering-layer refactor. The plans.ts line also defaults a missing payment to '' so the previously-unguarded .replace() call no longer throws on null/undefined.

Sibling sweep

Searched every file with an innerHTML sink in frontend/src for other unescaped API-sourced fields in the same row-builder pattern, including every other payment occurrence in the codebase (history.ts:1760, approval-details.ts:256 both already escape it correctly). No other unescaped field was found; the two fixed sites were the only gaps.

Regression tests

New files, following the existing xss-purchase-status.test.ts pattern (real escapeHtml, not a stub):

• frontend/src/__tests__/xss-purchase-payment.test.ts (sec(frontend/plans): escape purchase.payment in the planned-purchases term cell (stored XSS) #1633)
• frontend/src/__tests__/xss-recommendations-payment.test.ts (sec(frontend/recs): escape payment in the Opportunities table cell (stored XSS) #1632)

Each asserts the strongest form: after rendering a payment value containing "><img src=x onerror="alert(1)">, list.querySelectorAll('img').length === 0 (no live element parsed out of the cell), plus a <script> payload produces zero <script> elements. Valid/unrecognised payment values are also covered so the fix doesn't regress display.

Verified against the pre-fix source (stashed the two source edits, kept the new tests) - all 4 payload tests failed as expected:

1. script-tag payload in purchase.payment does not create a <script> element
   Expected: 0 / Received: 1
2. img-onerror payload in purchase.payment does not inject a live <img> element
   Expected: 0 / Received: 1
3. script-tag payload in rec.payment does not create a <script> element
   Expected: 0 / Received: 1
4. img-onerror payload in rec.payment does not inject a live <img> element
   Expected: 0 / Received: 1

Restored the fix - all 8 tests (4 payload + 4 valid-value) pass:

PASS (8) FAIL (0)

Test plan

• npx tsc --noEmit - clean
• npx eslint on changed files - clean
• New regression tests fail pre-fix, pass post-fix (pasted above)
• npx jest full suite - no new failures (8 pre-existing failures in riexchange.test.ts/utils.test.ts are a toLocaleString() thousands-separator locale mismatch on this machine, reproduced identically on a clean origin/main checkout before any of this PR's changes; unrelated to this diff)
• npm run build - clean production build

Closes #1633
Closes #1632

Summary by CodeRabbit

• Bug Fixes

  • Improved security when displaying payment information in planned purchases and recommendations.
  • Prevented potentially harmful HTML or script content from executing in payment fields.
  • Ensured missing payment values display safely without causing errors.
  • Preserved readable formatting for valid and unrecognized payment labels.

• Tests

  • Added regression coverage for unsafe, missing, and standard payment values.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ec6c5244-0fc7-495d-8fe1-8279a90f8678

📥 Commits

Reviewing files that changed from the base of the PR and between ff808b2 and 4647153.

📒 Files selected for processing (4)

• frontend/src/__tests__/xss-purchase-payment.test.ts
• frontend/src/__tests__/xss-recommendations-payment.test.ts
• frontend/src/plans.ts
• frontend/src/recommendations.ts

---

📝 Walkthrough

Walkthrough

The change escapes payment values in planned-purchase and recommendation tables. It also handles missing planned-purchase payments and adds regression tests for malicious, recognized, unrecognized, and absent payment values.

Changes

Payment rendering security

Layer / File(s)	Summary
Payment rendering and regression tests frontend/src/plans.ts, frontend/src/recommendations.ts, frontend/src/__tests__/xss-purchase-payment.test.ts, frontend/src/__tests__/xss-recommendations-payment.test.ts	Planned-purchase payment formatting now handles missing values. Recommendation payment labels are HTML-escaped before insertion. Regression tests verify that malicious values remain text and that valid payment labels render correctly.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• LeanerCloud/CUDly#283: Modifies recommendation payment rendering and tests.
• LeanerCloud/CUDly#375: Adds related XSS-safe frontend rendering and regression tests.
• LeanerCloud/CUDly#1046: Extends XSS fixes in the same planned-purchase and recommendation rendering paths.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the security fix for payment fields in the plans and recommendations tables.
Linked Issues check	✅ Passed	The changes address both linked XSS issues, including escaping payment values, handling missing planned-purchase payments, and adding regression tests.
Out of Scope Changes check	✅ Passed	All code and test changes directly support the linked XSS fixes and their required regression coverage.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/escape-payment-fields

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Rebased onto current origin/main (post-#1729 npm advisory fix) to clear the merge-blocked state. Mechanical rebase, no conflicts. Diff vs merge-base is byte-identical pre/post rebase (git diff <old-base> <old-head> == git diff <new-base> <new-head>). Re-verified post-rebase: the 8 XSS regression tests pass, including the querySelectorAll('img').length === 0 assertions; tsc --noEmit, eslint, and npm run build clean. Full suite shows the same known 8 pre-existing #1728 locale failures (unrelated, unfixed on main until #1732 merges), no new failures. New head: 536205c.

[cristim]

Merging on green CI with zero unresolved threads. CodeRabbit has posted no verdict; its quota is shared across the open PRs and throttled.

Proportionate judgement rather than a waiver: this is a two-line change using the existing escapeHtml helper, and what stands in place of a bot verdict is stronger than a bot verdict would be:

• Fail-then-pass verified by execution — 4 payload tests fail against pre-fix code with the exact predicted symptoms (a live <script> element, a live <img>), pass after.
• The assertion form proves the browser did not parse the payload (querySelectorAll('img').length === 0), rather than merely checking an escaped string appears somewhere — which would have passed against a partially-broken fix.
• The sibling sweep produced a negative result with evidence: all 21 files with innerHTML sinks were checked, and every other payment occurrence (history.ts:1760, approval-details.ts:256) already escapes correctly. These two were the only gap. That is the first single-location finding this session that genuinely was single-location.
• It also fixed an unguarded dereference neither issue mentioned: purchase.payment would throw on .replace for a null value.

The same author's sibling PR #1730 received a full independent adversarial review which specifically confirmed the escaping pattern holds under five breakout payloads, including verifying that the ../users/utils import is a bare re-export of ../utils rather than a weaker local variant.