fix(frontend): use shared quote-safe escapeHtml in apikeys and users

[cristim]

Problem

Closes #1195 (review finding ARCH-07, P2).

escapeHtml was implemented three times in the frontend with diverging security semantics:

• frontend/src/utils.ts escapes &, <, >, " and ' (quote-safe).
• frontend/src/apikeys.ts and frontend/src/users/utils.ts each re-implemented it via a DOM round-trip (div.textContent = text; return div.innerHTML;), which escapes only &, <, > and leaves both quote characters intact.

The weak copies were interpolated inside double-quoted attributes: data-key-id="${escapeHtml(key.id)}" in apikeys.ts, and aria-label / option value positions across users/userList.ts, users/filters.ts and users/permissionMatrix.ts. A value containing a double quote could break out of the attribute and inject markup (latent attribute-injection trap; exploitability today is low since the values are mostly server-generated IDs).

Fix

Per the report recommendation, delete both local copies and route everything through the shared quote-safe implementation:

• apikeys.ts: local escapeHtml removed; imports escapeHtml from ./utils.
• users/utils.ts: local escapeHtml replaced with a re-export from ../utils (same pattern already used there for formatRelativeTime/formatDate), so all users/ modules pick up the safe implementation with no import churn.

Tests that previously asserted the unescaped-quote behavior were updated to assert the quote-safe output, and the 11-M2 filters test now asserts the real invariant (no injected element; option value round-trips the raw id) instead of a side effect of the old truncation.

Regression tests

Replicating the real failing scenarios:

• apikeys.test.ts: a key id of key-1" onmouseover="alert(1) rendered through renderApiKeysList must not inject an onmouseover attribute, and data-key-id must round-trip the raw id.
• users.test.ts: a quoted value interpolated in attribute position through the users escapeHtml cannot break out of the attribute; plus direct quote-escaping assertions.

Verified fail-before / pass-after: with the source fix stashed, 7 tests fail (the new regression tests plus the updated quote assertions); with the fix applied, all pass.

Test evidence

• npx tsc --noEmit: no errors
• Full frontend suite npx jest: 2555 passed, 0 failed, 1 skipped
• npm run build: succeeds
• Note: npm run lint is broken on main as well (no ESLint config file in the repo), unrelated to this change

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved HTML escaping to safely handle quotes and attribute contexts, preventing injection via API key identifiers and group identifiers.

• Tests

  • Added security regression coverage to ensure hostile values containing quotes and attribute-like payloads do not create injected attributes or elements in rendered API key actions and group filter dropdown options.
  • Expanded escaping tests to explicitly verify correct quote handling (e.g., double quotes).

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 26 minutes and 32 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31280ebf-a651-4cec-a5e3-eb216c5ed1a8

📥 Commits

Reviewing files that changed from the base of the PR and between 862e3b2 and 53d335b.

📒 Files selected for processing (4)

• frontend/src/__tests__/apikeys.test.ts
• frontend/src/__tests__/users.test.ts
• frontend/src/apikeys.ts
• frontend/src/users/utils.ts

📝 Walkthrough

Walkthrough

Consolidates divergent escapeHtml implementations from two modules into a single shared, quote-safe utility, and adds regression tests verifying the shared version prevents attribute-injection attacks when rendering user-controlled IDs in double-quoted attributes.

Changes

HTML escaping consolidation and attribute-injection protection

Layer / File(s)	Summary
Consolidate HTML escaping implementations frontend/src/apikeys.ts, frontend/src/users/utils.ts	Removes duplicate local escapeHtml implementations and replaces them with imports or re-exports of the shared escapeHtml from ../utils, eliminating the quote-unsafe DOM round-trip variant.
Regression tests for quote-safe escaping and attribute injection frontend/src/__tests__/users.test.ts, frontend/src/__tests__/apikeys.test.ts	Adds comprehensive regression tests verifying the shared escapeHtml escapes both single and double quotes, preventing attribute breakout when interpolated into double-quoted attributes; covers group IDs in <option value="..."> contexts and API key IDs in data-key-id attributes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

urgency/this-sprint, impact/many, type/bug

Poem

🐰 Three versions of the same escape dance,
Now unified in safer stance!
Quotes and brackets, tamed at last,
Injection dreams have come to pass.
Security blooms where one is true.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and concisely summarizes the main change: consolidating three divergent escapeHtml implementations into a shared quote-safe version across apikeys and users modules.
Linked Issues check	✅ Passed	All coding objectives from issue #1195 are met: duplicate escapeHtml implementations removed from apikeys.ts and users/utils.ts, shared quote-safe implementation now used, attribute interpolations protected, and regression tests validating quote-safe escaping added.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #1195: consolidating escapeHtml implementations and updating tests. No unrelated modifications to unrelated functionality detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/arch-07-shared-escapehtml

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial-review pass

Ran the playbook across the OWASP-5 escape contract, the XSS sinks the PR touches, the other frontend modules that interpolate API data into innerHTML (i.e. the regression surface the PR could have missed), and the UNSTABLE CI checks.

Confirmed clean

• OWASP-5 contract is met. The shared escapeHtml in frontend/src/utils.ts:194 escapes &, <, >, ", and ' — both quote characters, not just <>. escapeHtmlAttr is a documented alias so attribute-context sites can self-document without diverging.
• All three duplicate copies are gone. grep -rn 'function escapeHtml' frontend/src returns only the canonical definition in utils.ts; users/utils.ts is a pure re-export (export { escapeHtml } from '../utils'), preserving the existing import surface for users/, groups/ (which imports via ../users/utils), and apikeys.ts.
• No remaining DOM-round-trip escapers. No div.textContent = ... ; return div.innerHTML pattern survives anywhere under frontend/src. No outerHTML, document.write, insertAdjacentHTML, or DOMParser.parseFromString source sinks at all in non-test code.
• Stored-XSS surfaces against user-controlled fields render safely:
  • API key NAME — <strong>${escapeHtml(key.name)}</strong> (apikeys.ts:84).
  • API key ID in data-key-id="..." attributes on the revoke/delete buttons — escapeHtml(key.id) in attribute position (apikeys.ts:91-92); regression test added in this PR ('issue #1195: double quote in key id cannot inject attributes').
  • One-time API key display — <code>${escapeHtml(apiKey)}</code> and the curl -H "X-API-Key: ${escapeHtml(apiKey)}" example (apikeys.ts:229,235).
  • User email in userList.ts — all 4 attribute-position uses and the visible <strong> use go through escapeHtml(user.email).
  • Group id/name through users/filters.ts:128, users/userModals.ts:187-188, users/userList.ts:110,193,253, groups/groupList.ts, groups/groupModals.ts — all routed through the shared escaper now that users/utils.ts re-exports it.
  • permissionMatrix.ts — every dynamic value (group name, action label, resource-list title attr) is escaped.
• No new pages left vulnerable. The other frontend modules that build innerHTML from API data — history.ts, plans.ts, dashboard.ts, recommendations.ts, riexchange.ts, approval-details.ts, auth.ts, groups/groupList.ts, groups/groupModals.ts — all already imported escapeHtml from ./utils (or transitively via ../users/utils), i.e. the OWASP-5 version. They were never affected by the apikeys/users gap. commitmentOptions.ts is the only top-level innerHTML writer that doesn't import escapeHtml, and that's intentional: it interpolates only the hardcoded static AWS_PAYMENTS / AZURE_PAYMENTS / GCP_PAYMENTS arrays, no API data.
• No double-escape regressions. grep -rn 'escapeHtml(escapeHtml' frontend/src returns nothing. Confirmed empirically that the shared escaper is NOT idempotent (escapeHtml("<") == "&lt;", escapeHtml(escapeHtml("<")) == "&amp;lt;"), so a future nested-call regression would corrupt rendered output — flagging for any future refactor, but no current site triggers it.
• feedback_innerhtml_xss.md rule (every API field in an innerHTML template must go through escapeHtml() or a whitelist) is now uniformly enforced across all four target files; the in-PR test additions positively assert the invariant rather than the prior side-effect (e.g. the 11-M2 filters test now checks "no <img> injected and the option value round-trips the raw id" rather than "value truncated at the first \"").
• Regression tests replicate the real failing scenario (feedback_no_silent_fallbacks.md §verification): the apikeys test pins the onmouseover attribute null + data-key-id round-trip on a hostile key-1" onmouseover="alert(1), and the users escapeHtml test does the same against an attribute interpolation. Verified fail-without-fix / pass-with-fix by the PR author.

In-scope fix pushed (53d335b33, rebased on current main)

• Migration-number collision resolved (feedback_migration_number_collisions.md). PR was UNSTABLE on the Run pre-commit hooks job because the branch was behind main after fix(db): renumber audit_actor_stamps migration to clear 000074 collision #1261 renumbered audit_actor_stamps from 000074 -> 000077. The PR branch still carried the old 000074_audit_actor_stamps files, so the merge ref had a duplicate 000074 (audit_actor_stamps + repair_partial_migration_058_067). Rebased the single PR commit onto current main; post-rebase, internal/database/postgres/migrations/ aligns with main (000074 = repair, 000077 = audit_actor_stamps), no duplicate. The PR commit itself only touches the four frontend files it claims to touch; rebase diffstat unchanged (4 files changed, 59 insertions(+), 22 deletions(-)).

UNSTABLE checks NOT caused by this PR

Flagging for visibility so #1214 itself isn't blamed for the residual red after the rebase clears the pre-commit hook:

• Lint Code: pre-existing errcheck / staticcheck / revive / misspell etc. violations across cmd/, internal/api, internal/auth, etc. — none of the flagged files are touched by this PR (frontend-only diff). Same backlog blocking several other open PRs.
• Integration Tests / E2E Tests / Security Scanning: pre-existing failures previously rooted in the migration collision; should clear with the rebase. Confirming on the next CI run.

Out-of-scope follow-up

None worth filing — the codebase-wide scan turned up no additional source files that interpolate API data into innerHTML without escapeHtml. The "quote-safe escapeHtml is not idempotent" property is a latent footgun if a future refactor ever wraps an already-escaped value, but no current site triggers it and the inverse (skipping escapeHtml somewhere) is the bigger risk — a CI grep for \.innerHTML\s*=\s*\[^\`]*${[^}]+.(?!|escapeHtml)[a-z]+}` would be the structural guard, but that's a separate hardening initiative, not a follow-up to this PR.

Re-pinging CR.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.