Repository navigation
fix(frontend): neutralise provider XSS and dedup modal/global listeners - #1046
Conversation
…rs (#1034) H1 - Stored XSS in plans.ts: both planned-purchase row (line 180) and plan card detail (line 412) injected the API provider string into innerHTML class attributes and text without whitelist/escaping. Extract shared providerBadgeClass() and providerBadgeHtml() helpers to utils.ts (matching the existing whitelist in history.ts:341 and recommendations.ts:2417) and replace both raw interpolations. H3 - Event-listener stacking in plans.ts: setupRampScheduleHandlers added listeners to static modal elements on every modal open. Add rampHandlersInstalled install-once guard; export _resetRampHandlersForTest() for test isolation. D1/L4 - Shared provider badge helper: dashboard.ts was using a loose alphanumeric regex instead of the strict aws|azure|gcp whitelist. Align on providerBadgeClass() from utils.ts. Tests: add regression tests to utils.test.ts (providerBadgeClass/ providerBadgeHtml XSS neutralisation), plans.test.ts (H1 call-through and H3 install-once dedup), and update utils mocks in dashboard.test.ts and dashboard-ownership-950.test.ts for the new import. Note: H2 (users/userList.ts duplicate document change listener) was already fixed via AbortController in a prior PR merged to feat/multicloud-web-frontend.
cristim
added
bug
|
@coderabbitai review |
✅ Action performedReview finished.
|
…-M2, 11-L1) Wrap group.id in escapeHtml() in updateGroupFilterDropdown (finding 11-M2): the option value attribute was injecting the raw API value without escaping, violating the feedback_innerhtml_xss policy. Escape plan.id and purchase.id in all data-id attributes inside the plan-card and planned-purchase-row innerHTML templates (finding 11-L1). UUIDs are safe in practice but must be escaped for consistency and defence-in-depth. Adds regression test in users.test.ts asserting the group.id XSS payload is not emitted verbatim into the rendered option element. Closes part of #1065
…11-M1, 11-M3) savePlan now validates term, target_coverage, notification_days_before, and custom ramp fields with Number.isInteger(Number(raw)) before building the request object (finding 11-M1). Fractional or NaN inputs show a toast error and abort without calling the API, matching the pattern in settings.ts validatePurchasingSettings and feedback_strict_int_parse. collectPermissions in groupModals.ts guards max_amount with Number.isFinite(parsed) && parsed >= 0 before assigning to the constraint object (finding 11-M3). Infinity, NaN, and negative values are silently skipped so the rest of the permission constraints still reach the API. Adds regression tests in plans.test.ts (11-M1) and groups.test.ts (11-M3) that verify the guards reject bad inputs and accept valid values. Closes part of #1065
…(11-L2, 11-L3) Replace blocking window.confirm() calls in handlePlannedPurchaseAction (run and disable actions) with the async confirmDialog helper (finding 11-L2). The dialog uses the same destructive-button styling already applied to the delete-plan and purchase-cancel flows. This removes the last confirm() calls from plans.ts. Replace window.alert() calls with showToast across four files (finding 11-L3): - recommendations.ts: refreshRecommendations success and failure feedback - auth.ts: updateProfile validation error, success, and failure feedback - federation.ts: IaC download failure feedback - modules/registrations.ts: reject-registration failure feedback auth.ts adds a showToast import; federation.ts adds a showToast import. Updates regression tests in plans.test.ts (confirmDialog assertions), auth.test.ts (adds toast mock, updates profile alert assertions), and recommendations.test.ts (updates refreshRecommendations alert assertions) to match the new behaviour. Adds structuredClone polyfill for jsdom in setup.ts. Closes part of #1065
… zero (11-N1, 11-N2) deepClone in utils.ts now delegates to structuredClone instead of a JSON round-trip (finding 11-N1). structuredClone preserves undefined values, Date objects, Set and Map instances, and handles circular-safe trees. The old JSON.parse/stringify variant is retained as an explicit jsonClone() export for callers that need JSON-serialisation semantics. formatCurrency returns '--' for null, undefined, and non-finite (NaN, Infinity) inputs instead of '$0' (finding 11-N2). This makes missing data visually distinguishable from a real zero balance, closing the silent missing-data bug described in feedback_nullable_not_zero. Adds regression tests in utils.test.ts for both changes. Closes #1065
FOLD-1046: additional findings added (4 commits)All findings from the FOLD-1046 bucket (code review report 11) have been implemented and pushed to 11-M2 (Medium) -
|
|
Kicking off a fresh incremental review of the 4 new commits now. 🧠 Learnings used✅ Action performedReview finished.
|
…fabricating defaults
H-2: renderHistorySummary now accepts null summary and renders '--' on all
KPI cards rather than passing {} and coercing every money field to $0.
H-3: dashboard KPI tiles no longer fabricate values for absent fields:
target_coverage removes the hardcoded 80% default (shows '--'); absent
current_coverage and active_commitments show '--' not '0%'/'0'; the
savings range catch block returns '--' not $0.
H-4: extractPlanInfo returns null for absent provider/term/coverage instead
of defaulting to 'aws'/3yr/80%. The plan card renders '--' for missing fields;
the edit modal leaves selects unset so the user must explicitly choose.
H-5: edit modal no longer pre-selects 'no-upfront' when the payment field is
absent from the plan -- the payment select is left blank, requiring an
explicit choice before re-saving.
M-1: savings summary card uses ?? null (not ?? 0) so absent savings propagate
to '--' via formatCostForPeriod/formatScaledRange.
M-2: numericCellValue returns NaN for absent savings/upfront_cost (consistent
with monthly_cost/on_demand_monthly) so filter predicates don't match absent
rows as if they were zero.
M-3: approval modal header skips null savings in the aggregate (not += 0);
total_upfront_cost propagates null to formatCurrency so absent shows '--'.
M-5: lastRecommendationsSummary typed as RecommendationsSummary|null; absent
API summary is no longer coerced to {}.
M-6/M-7: savings-history private formatCurrency gains a null/non-finite guard
returning '--'; chart data mapping uses ?? 0 (not || 0) preserving genuine 0.
L-2: payback fallback changed from '0 months' to '—' to distinguish
inapplicable from instantaneous payback.
Regression tests added for H-2, H-3, H-4, H-5 that fail pre-fix.
Scope expanded: silent-fallback money-display fixes (H-2..H-5, M-1..M-7, L-2)Commit aa8f16e adds fixes for the findings in `docs/code-review/19-hardcoded-fallbacks-frontend.md`. All findings close issue #1086. Per-finding summaryH-2 `history.ts` (lines 134, 213, 265-295): `data.summary || {}` passed an empty `HistorySummary` when the API field was absent; `?? 0` coercions then rendered $0 on Total Upfront / Monthly Savings / Annual Savings. Fixed: `renderHistorySummary` now accepts `HistorySummary | null`; a null argument renders an explicit unknown-state banner with `'--'` on every card rather than fabricating all-zero values. H-3 `dashboard.ts` (lines 163-164, 180, 254-268): Three fabricated defaults removed: (a) `target_coverage || 80` -- the hardcoded 80% business default is gone; (b) `current_coverage || 0` and `active_commitments || 0` -- absent fields now render `'--'`; (c) the savings range catch block now returns `'--'` instead of `formatCurrency(0)`. H-4 `plans.ts` `extractPlanInfo` (lines 878-896): Return type changed to `{ provider: string | null; term: number | null; coverage: number | null; ... }`. Absent fields return `null` instead of defaulting to `'aws'`/`3`/`80`. Plan card renders `'--'`; edit modal leaves selects unset so the user must explicitly choose (no silent 3yr-term or 80%-coverage fabrication). H-5 `plans.ts` (line 1119): `firstService?.payment || 'no-upfront'` replaced with `firstService?.payment || null`; null maps to empty string on the payment select, leaving it unset so a re-save cannot silently change the payment type. M-1 `recommendations.ts` (lines 729-730): `scaleCost(...) ?? 0` on the savings summary cards replaced with `?? null` so absent savings flow to `'--'` via `formatScaledRange`/`formatCostForPeriod`. M-2 `recommendations.ts` `numericCellValue` (lines 1680-1681): `savings ?? 0` and `upfront_cost ?? 0` now return `Number.NaN` (consistent with `monthly_cost`/`on_demand_monthly`) so absent rows do not incorrectly match numeric filter predicates. M-3 `approval-details.ts` (lines 99-109): null savings are skipped in the `totalMonthly` aggregate (not `+= 0`); `total_upfront_cost ?? null` propagates to `formatCurrency` which renders `'--'` for absent upfront. M-5 `recommendations.ts` (line 516): `data.summary || {}` replaced with `data.summary ?? null`; `lastRecommendationsSummary` typed as `RecommendationsSummary | null`. M-6/M-7 `savings-history.ts`: Private `formatCurrency` extended with a `null | undefined | non-finite` guard returning `'--'`; `|| 0` in aggregation and chart data mapping replaced with `?? 0` so a genuine $0 data point is not confused with a missing field. L-2 `recommendations.ts` (line 742): Payback fallback changed from `'0 months'` (implies instantaneous payback) to `'—'`. Regression tests added
All 2478 previously-passing tests still pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/dashboard.ts (1)
252-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty recommendation ranges still render a fabricated
$0.On Line 264,
range.cellCount === 0still maps toformatCurrency(0), which contradicts the new absent-data contract in this PR and surfaces misleading savings.💡 Suggested fix
- savingsDisplay = range.cellCount > 0 - ? formatSavingsRange(range.savingsMin, range.savingsMax) - : formatCurrency(0); + savingsDisplay = range.cellCount > 0 + ? formatSavingsRange(range.savingsMin, range.savingsMax) + : '--';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/dashboard.ts` around lines 252 - 267, The savings display logic incorrectly fabricates "$0" when pageLevelRange returns an empty range; change the branch in the try block that sets savingsDisplay (currently using pageLevelRange and the ternary that calls formatCurrency(0)) so that when range.cellCount === 0 it assigns the absent-data sentinel '--' instead of formatCurrency(0). Update the code that uses groupRecsByCell, pageLevelRange, formatSavingsRange, and formatCurrency to reflect: if range.cellCount > 0 use formatSavingsRange(...), otherwise set savingsDisplay = '--' (preserving the surrounding try/catch and the console.warn on recErr).frontend/src/__tests__/dashboard.test.ts (1)
104-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the
formatCurrencymock with the new sentinel contract.This mock still collapses absent/non-finite values to
$0, which can hide regressions now that dashboard behavior distinguishes missing values with--.Suggested adjustment
jest.mock('../utils', () => ({ - formatCurrency: jest.fn((val) => `$${val || 0}`), + formatCurrency: jest.fn((val) => { + const n = typeof val === 'number' ? val : Number(val); + if (val == null || !Number.isFinite(n)) return '--'; + return `$${n}`; + }), getDateParts: jest.fn(() => ({ day: 15, month: 'Jan' })), escapeHtml: jest.fn((str) => str || ''), populateAccountFilter: jest.fn(() => Promise.resolve()), // providerBadgeClass added for L4/D1 fix: whitelist-based CSS class helper. providerBadgeClass: jest.fn((p) => ['aws', 'azure', 'gcp'].includes((p || '').toLowerCase()) ? (p || '').toLowerCase() : ''), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/dashboard.test.ts` around lines 104 - 111, The test mock for formatCurrency currently returns `$0` for absent/non-finite inputs which breaks the new sentinel contract that represents missing values as `--`; update the jest.mock for formatCurrency in dashboard.test.ts so it checks Number.isFinite(val) (or equivalent) and returns `--` for null/undefined/NaN/Infinity, otherwise returns the formatted string (e.g., `$${val}`); locate the mocked symbol formatCurrency in the jest.mock block and change only its implementation to follow this sentinel behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/history.test.ts`:
- Around line 468-491: The test renderHistorySummary: absent total_upfront
renders -- not $0 is wrong because it only checks that summary markup exists;
update the test for viewPlanHistory to assert the actual rendered value for
total_upfront is the placeholder "--" (not "$0") by locating the Total Upfront
Spent cell in the DOM (e.g., within `#history-summary` or by its label) and
asserting its textContent or innerHTML equals or contains "--"; ensure the mock
path that sends undefined for summary.total_upfront still leads formatCurrency
(or the UI code that post-fixes null/undefined) to produce the "--" output and
assert that exact string rather than just the presence of the summary.
In `@frontend/src/__tests__/plans.test.ts`:
- Around line 112-115: The test mock providerBadgeHtml currently injects raw
provider strings into HTML, allowing malicious fixtures to create real tags;
update the providerBadgeHtml jest.fn mock to HTML-escape the provider text
before concatenation (escape &, <, >, ", and ') and use the escaped value for
both the class and inner text outputs (i.e., transform (p||'') into an
escapedProvider variable inside the mock and insert that), so tests assert text
neutralisation rather than permitting injected markup.
In `@frontend/src/plans.ts`:
- Around line 731-735: The template injects purchase.id and purchase.plan_id
into quoted data-* attributes using escapeHtml, which doesn't escape quotes and
allows attribute-breakout XSS; replace usages of escapeHtml for attribute values
with an attribute-safe encoder (e.g., escapeHtmlAttr that calls escapeHtml then
replaces " and ' with " and &`#39`;) or switch to setting dataset properties
via DOM APIs; update all occurrences where data-id/data-plan-id are built (the
template lines that produce buttons with
data-action="run|pause|resume|edit|disable" and the places also noted for
purchase ids) to use the new escapeHtmlAttr (or dataset assignment) instead of
escapeHtml.
---
Outside diff comments:
In `@frontend/src/__tests__/dashboard.test.ts`:
- Around line 104-111: The test mock for formatCurrency currently returns `$0`
for absent/non-finite inputs which breaks the new sentinel contract that
represents missing values as `--`; update the jest.mock for formatCurrency in
dashboard.test.ts so it checks Number.isFinite(val) (or equivalent) and returns
`--` for null/undefined/NaN/Infinity, otherwise returns the formatted string
(e.g., `$${val}`); locate the mocked symbol formatCurrency in the jest.mock
block and change only its implementation to follow this sentinel behavior.
In `@frontend/src/dashboard.ts`:
- Around line 252-267: The savings display logic incorrectly fabricates "$0"
when pageLevelRange returns an empty range; change the branch in the try block
that sets savingsDisplay (currently using pageLevelRange and the ternary that
calls formatCurrency(0)) so that when range.cellCount === 0 it assigns the
absent-data sentinel '--' instead of formatCurrency(0). Update the code that
uses groupRecsByCell, pageLevelRange, formatSavingsRange, and formatCurrency to
reflect: if range.cellCount > 0 use formatSavingsRange(...), otherwise set
savingsDisplay = '--' (preserving the surrounding try/catch and the console.warn
on recErr).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c28d0881-b707-4984-a398-d21471987939
📒 Files selected for processing (22)
frontend/src/__tests__/auth.test.tsfrontend/src/__tests__/dashboard-ownership-950.test.tsfrontend/src/__tests__/dashboard.test.tsfrontend/src/__tests__/groups.test.tsfrontend/src/__tests__/history.test.tsfrontend/src/__tests__/plans.test.tsfrontend/src/__tests__/recommendations.test.tsfrontend/src/__tests__/setup.tsfrontend/src/__tests__/users.test.tsfrontend/src/__tests__/utils.test.tsfrontend/src/approval-details.tsfrontend/src/auth.tsfrontend/src/dashboard.tsfrontend/src/federation.tsfrontend/src/groups/groupModals.tsfrontend/src/history.tsfrontend/src/modules/registrations.tsfrontend/src/modules/savings-history.tsfrontend/src/plans.tsfrontend/src/recommendations.tsfrontend/src/users/filters.tsfrontend/src/utils.ts
…S (CR #1046) Replace the div.textContent/innerHTML impl with explicit character replacement so double and single quotes are escaped to " and '. Values interpolated into data-* attributes via innerHTML could break out of the attribute value when the string contained a raw quote, enabling XSS (plans.ts:731+, 969-970, 1009-1013). Fix also updates the two utils.test.ts cases that pinned the buggy unescaped-quote output, hardens the providerBadgeHtml mock in plans.test.ts to escape the label before injecting it into the test DOM, and strengthens the history.test.ts absent-total_upfront assertion to verify formatCurrency is called with null and the rendered card shows '--' instead of '$0'.
|
Addressed all three findings in 9df488b: escapeHtml now escapes quotes (closes the data-attribute XSS), the providerBadgeHtml test mock escapes its text, and the history regression test now asserts the @coderabbitai full review |
|
(ᵔ◡ᵔ)∫ 🧠 Learnings used✅ Action performedFull review finished. |
|
@coderabbitai full review Generated by Claude Code |
|
🧠 Learnings used✅ Action performedFull review finished. |
- dashboard.ts: replace formatCurrency(0) with literal '--' when range.cellCount === 0 (CR finding: empty recs fabricated a real $0 instead of the absent-data sentinel) - dashboard.test.ts: align formatCurrency mock with null/non-finite sentinel contract (val==null or !isFinite returns '--'); update three stale assertions that expected '$0' when recs are absent or failed Addresses CR review actionable findings on PR #1046 (outside-diff comments dashboard.ts:252-267 and dashboard.test.ts:104-111). Finding plans.ts:731-735 skipped as CR misread: escapeHtml in utils.ts already escapes both '"' and "'" (lines 162-163).
|
CR finding triage (commit f9abe35): Outside-diff: Outside-diff: Inline: Outside-diff: Inline: @coderabbitai review Generated by Claude Code |
|
Kicking off an incremental review of the new commits now. 🧠 Learnings used✅ Action performedReview finished.
|
Introduces escapeHtmlAttr as a semantic alias for escapeHtml that makes clear the caller is escaping an attribute value. The escapeHtml implementation was already updated in the prior commit to encode " and ' (closing the root vulnerability); escapeHtmlAttr calls through to it so all attribute-context injections are covered. Replaces the remaining escapeHtml calls in data-attribute context: - history.ts: data-retry-id and both data-execution-id sites - plans.ts: data-id and data-name on all plan/purchase action buttons Adds escapeHtmlAttr unit tests to utils.test.ts (quote, ampersand, and bracket encoding; null/undefined; safe strings) and a regression test in history.test.ts that fails against the pre-fix quote-transparent path.
|
Attribute-boundary XSS fix (commit fe49c47) The CR finding was that `escapeHtml` (backed by `div.textContent/innerHTML`) does not encode double-quotes, so values injected into `data-*="..."` attributes could break out of the attribute delimiter and allow markup injection. Fix:
Changed lines in `history.ts`:
jest: 201 pass, 2 fail (pre-existing UTC timezone flake in formatDate/getDateParts, unrelated). tsc: no errors. |
|
@coderabbitai review |
2 participants
Summary
plans.tsinjected the APIproviderstring raw into innerHTML class attributes and text nodes at two sites (~180, ~412). Extract sharedproviderBadgeClass()/providerBadgeHtml()helpers inutils.ts(matching the existing whitelist inhistory.ts:341andrecommendations.ts:2417) and replace both raw interpolations.setupRampScheduleHandlersinplans.tsre-attached listeners to static modal elements on every modal open. AddrampHandlersInstalledinstall-once guard; export_resetRampHandlersForTest()for test isolation.dashboard.tswas using a loose alphanumeric regex instead of the strictaws|azure|gcpwhitelist. Align onproviderBadgeClass()fromutils.ts.dashboard.test.tsanddashboard-ownership-950.test.tsutils mocks extended withproviderBadgeClass.users/userList.tsduplicate document change listener via AbortController) was already fixed in a prior PR merged intofeat/multicloud-web-frontend.Closes #1034
Test plan
utils.test.tsverifyproviderBadgeClassreturns empty string for malicious payloads andproviderBadgeHtmldoes not emit raw<img>/onerrorin outputplans.test.tsH1 regression:providerBadgeHtmlis called (not raw interpolation) when provider contains a malicious stringplans.test.tsH3 regression: dispatching a provider-change event after 3 modal opens callspopulateTermSelectexactly once (not 3x)users.test.tsH2 regression (pre-existing at line 2297): single checkbox toggle after 3renderUserscalls emits exactly one toast--noEmitcleanwebpack --mode production)Scope expanded
Additional findings from FOLD-1046 (code review report 11) added in 4 commits:
users/filters.ts--group.idwas injected raw intovalue=""in an innerHTML template; wrapped inescapeHtml().plans.ts--plan.idandpurchase.idraw indata-idattributes inside innerHTML plan-card and purchase-row templates; all escaped.plans.ts--savePlanused bareparseIntforterm,target_coverage,notification_days_before, and custom ramp fields; replaced withNumber.isInteger(Number(raw))validation (toast on rejection), mirroringsettings.tsvalidatePurchasingSettings.groups/groupModals.ts--collectPermissionscalledparseFloat(maxAmount)without aNumber.isFinite && >= 0guard; non-finite and negative values are now silently skipped.plans.ts--confirm()calls in the Run and Disable planned-purchase actions replaced withconfirmDialog({destructive:true}).alert()replaced withshowToastinrecommendations.ts(refresh success/failure),auth.ts(profile update validation, success, failure),federation.ts(IaC download error),modules/registrations.ts(reject-registration error).utils.ts--deepClonenow delegates tostructuredCloneinstead of a JSON round-trip; old JSON path retained asjsonClone()for callers that need JSON semantics.utils.ts--formatCurrencyreturns'--'for null/undefined/NaN/Infinity to distinguish absent data from a real $0 balance.Also closes #1065
Scope expanded
This PR now also fixes the findings from
docs/code-review/19-hardcoded-fallbacks-frontend.md(H-2..H-5 and associated Mediums). All findings are folded into a single additional commit (aa8f16e1f).Also closes #1086
Added fixes
history.ts):data.summary || {}+?? 0rendered fabricated $0 on all History KPI cards when the API field was absent. Now renders'--'sentinel on each card; null summary shows an explicit unknown-state banner.dashboard.ts):target_coverage || 80removed (hardcoded 80% business default); absentcurrent_coverage/active_commitmentsnow show'--'; savings range catch block shows'--'not$0.plans.tsextractPlanInfo):provider || 'aws'/term || 3/coverage || 80replaced withnull; plan card shows'--'; edit modal leaves selects unset so the user must explicitly choose (no fabricated 3yr term or 80% coverage).plans.ts):payment || 'no-upfront'removed; absent payment leaves the payment select blank so re-saving does not silently change the payment type.recommendations.ts):scaleCost(...) ?? 0on summary savings cards replaced with?? null.recommendations.ts):savings ?? 0/upfront_cost ?? 0innumericCellValuenow returnNumber.NaN(consistent withmonthly_cost/on_demand_monthly) so absent rows don't match filter predicates.approval-details.ts): null savings skipped in aggregate (not += 0);total_upfront_costpropagates null toformatCurrencyshowing'--'.recommendations.ts):lastRecommendationsSummarytyped asRecommendationsSummary | null; absent summary no longer coerced to{}.savings-history.ts): privateformatCurrencygains null/non-finite guard;|| 0replaced with?? 0in chart data mapping.recommendations.ts): payback fallback changed from'0 months'(misleadingly implies instantaneous) to'—'.Added regression tests
Regression tests added to
history.test.ts,dashboard.test.ts, andplans.test.tsthat fail on the pre-fix code for H-2, H-3, H-4, and H-5.Summary by CodeRabbit
Release Notes
Bug Fixes
New Features