chore(frontend/core): medium/low findings from the 2026-07-28 full review #134

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Tracking issue for the MEDIUM and LOW findings from the 2026-07-28 full-repo review of the frontend core: frontend/src/api/**, frontend/src/lib/**, frontend/src/modules/**, frontend/src/groups/**, and the top-level cross-cutting modules (app.ts, auth.ts, state.ts, inventory.ts, ladder.ts, permissions.ts, navigation.ts, serviceFilters.ts, topbar-filters.ts, federation.ts, archera.ts, modal.ts, toast.ts, confirmDialog.ts, approval-details.ts, commitmentOptions.ts, purchases-deeplink.ts). The two HIGH findings from the same pass are filed as individual issues.

Each checklist item is independently actionable. Please keep the file:line, failure scenario and fix direction with the item when splitting any of these out.

Three recurring shapes account for most of the list:

1. || used where ?? is meant, so a meaningful 0 is read as "unset". Hits a spend cap (the HIGH), the bulk-purchase capacity percent, and the ladder buffer fraction.
2. An awaited loader renders unconditionally, with no request-generation guard, even though frontend/src/topbar-filters.ts:83,92,103,113 already implements exactly that pattern in the same codebase.
3. A listener is registered inside a function reachable more than once, with the dedupe applied to one registration and not its siblings.

Findings from this pass that are NOT in this list

Recorded so nothing looks dropped:

• Group edit modal dropping and widening permissions it cannot render: filed as its own HIGH issue, and closed by the same backend change as sec(auth): the #923 money separation-of-duties carve-out is voidable by any admin in one request cloud-commitments-cli#1550.
• max_amount: 0 erased on group edit: filed as its own HIGH issue.

Explicitly checked and clean

Recorded so the next review pass does not re-derive them: every innerHTML write reachable from this subsystem escapes its interpolations (groups/groupList.ts:37-80, groups/groupModals.ts:161-172, approval-details.ts:247-261, ladder.ts:254-269, auth.ts:401/1225/1310, commitmentOptions.ts:190-216), with lib/skeleton.ts, lib/chip-select.ts, lib/history-filter-popover.ts, modules/registrations.ts, inventory.ts, archera.ts, federation.ts, toast.ts, modal.ts, confirmDialog.ts and serviceFilters.ts createElement/textContent throughout; Permission.constraints matches the API DTO with no wire mismatch; permissions.ts canAccess/isPurchaser correctly mirror adminCarvedOuts; getAuthHeaders/clearAuth/the cross-tab storage listener are correct and no error path renders a token; getUserPermissions, getResetTokenStatus, setupMFA, enableMFA and regenerateMFARecoveryCodes all throw on shape mismatch; utils.ts money/date helpers return '--' rather than $0; lib/capacity.ts treats 0/null/NaN as unknown; lib/column-filters.ts returns an explicit error rather than a silent match-all.

• A failed /api/info fetch renders the first-run admin-setup screen instead of login (MEDIUM)

  • Where: frontend/src/api/auth.ts:412-419 (getPublicInfo), consumed at frontend/src/app.ts:46-60
  • What: getPublicInfo() returns a fabricated { version: '', admin_exists: false } on any non-OK response instead of throwing. init() wraps the call in a try/catch whose comment says "If public info fails, fall through to login", but the catch never fires because the function resolves successfully with the fabricated value. !publicInfo.admin_exists is therefore true, showAdminSetupModal(undefined) runs, and the return means the login modal is never shown. getDeploymentInfo() at frontend/src/api/auth.ts:427-435 has the same shape (returns {} on failure).
  • Failure scenario: the API returns 502 or 503 (cold-start failure, deploy in progress, DB unreachable). Every unauthenticated visitor, including legitimate users who just want to sign in, is shown "Welcome to CUDly / No admin account exists yet. Set up the first admin to get started." with no route to the login form except the small "Already have an account? Log in" link. The screen asserts a false fact about the deployment state and hides the real error.
  • Fix: throw on !response.ok in getPublicInfo so the existing catch in init() reaches showLoginModal(), and surface the transport failure to the user. Same silent-fallback class as fix(frontend): silent fallbacks fabricate money/coverage values on Dashboard, History, Plans, Opportunities cloud-commitments-cli#1086, different file and no money involved.

• Inventory loaders have no generation guard, so a slow response overwrites a newer one (MEDIUM)

  • Where: frontend/src/inventory.ts:102-131 (loadActiveCommitments) and frontend/src/inventory.ts:361-382 (loadCoverageBreakdown)
  • What: both read state.getCurrentProvider() / state.getCurrentAccountIDs(), await a fetch, then render unconditionally. Both are wired to the topbar chip subscriptions (frontend/src/inventory.ts:544-545, :558-560), so a chip change re-invokes them. There is no request-generation counter, unlike frontend/src/topbar-filters.ts:83,92,103,113 which implements exactly that pattern for the account chip.
  • Failure scenario: the user is on Inventory & Coverage -> Active commitments with the Provider chip on AWS (large result set, slow query) and switches the chip to GCP. The GCP request returns in 200 ms and renders 3 rows; the AWS request lands 4 s later and overwrites the table with hundreds of AWS commitments while the chip still reads "GCP". The user reads AWS commitment counts, expiry dates and dollar amounts as GCP's. The same race applies to the coverage percentages on the Coverage sub-tab.
  • Fix: capture a monotonic _gen counter before each await and discard the render when gen !== _gen, mirroring populateAccountOptions in topbar-filters.ts.

• Registrations status-filter listener stacks on every Accounts sub-tab visit (MEDIUM)

  • Where: frontend/src/modules/registrations.ts:254-258 (initRegistrations), invoked from frontend/src/settings.ts:3346 inside loadAccountsTab()
  • What: a bare filterEl?.addEventListener('change', () => void loadRegistrations()) with no dedupe marker and no prior removeEventListener. loadAccountsTab() runs from switchSettingsSubTab's case 'accounts' (frontend/src/navigation.ts:274-276), and that switch executes before the if (isSelfSwitch) return; guard at frontend/src/navigation.ts:286, so even re-clicking the already-active Accounts sub-tab re-runs it. Every visit adds one more closure.
  • Failure scenario: an admin clicks Admin -> Accounts & onboarding five times while triaging registrations, then changes the status dropdown from "Pending" to "All". Five concurrent GET /api/registrations requests fire and five renderRegistrationsTable calls race into #registrations-list; the last response to land wins regardless of which filter value it was issued for, so the table can show "pending" rows while the dropdown reads "All".
  • Fix: use the btn.dataset['wired'] === '1' marker already used at frontend/src/inventory.ts:133-146 and :384-392, or store and remove the previous handler as initSavingsHistory does for the unit select (frontend/src/modules/savings-history.ts:576-584). See feedback_event_listener_dedup; same class as fix(frontend): plans.ts provider XSS (missing whitelist) + duplicate document/modal listeners (users, plans) cloud-commitments-cli#1034, different files.

• Savings-history unit toggle scales only one of two series on a dual-axis chart (MEDIUM)

  • Where: frontend/src/modules/savings-history.ts:382-389, chart config at :404-428, tooltip at :509-522
  • What: periodSavingsData maps through convertFromMonthly(dp.total_savings ?? 0, unit) (line 384) while cumulativeSavingsData uses dp.cumulative_savings ?? 0 raw (line 388) with no conversion. The two series are plotted on y and y1 of the same chart and rendered together under one mode: 'index' tooltip, where only the Period Savings line carries the suffix.
  • Failure scenario: on the Purchases tab a user switches the Savings unit dropdown from "Monthly" to "Yearly". The Period Savings line jumps 12x; the Cumulative Savings line does not move. Switching to "Hourly" divides one line by 730 and leaves the other unchanged. A user hovering a point reads both numbers side by side in the same tooltip and compares them as if they shared a unit; they are off by a factor of 12 or 730 depending on the selection.
  • Fix: either convert both series through convertFromMonthly, or hard-pin the cumulative axis to a single unit and label it explicitly (its axis title at line 497 is a bare "Cumulative Savings" with no unit). Same chart as LeanerCloud/cloud-commitments-cli#963, different defect.

• "Period Savings" KPI is a monthly rate rendered as a bare dollar total (MEDIUM)

  • Where: frontend/src/modules/savings-history.ts:293-321
  • What: monthlyTotal is summary.total_period_savings ?? totalSavings, where the fallback totalSavings (:273-287) is a flat sum of per-bucket total_savings, each of which sums estimated_savings, a monthly figure per this file's own API_UNIT = 'monthly' at line 20. The value is then run through convertFromMonthly(monthlyTotal, unit) at line 298, which is only correct for a rate. It is rendered at line 305 as formatCurrency(displayTotal) with no unit suffix, while the two neighbouring tiles (lines 317, 320) both append suffix (/mo, /hr, /yr). The comment at lines 303-304 asserts the opposite of what line 298 does ("it is already a dollar total ... no per-unit rate suffix").
  • Failure scenario: a user with commitments saving $1,200/month selects a 30-day period and the "Monthly" unit. The Period Savings tile reads "$1,200.00" with no suffix, next to "Avg Monthly Savings $1,200.00/mo", and is read as "$1,200 saved during this period". Switching the unit to "Yearly" makes the same tile read "$14,400.00", still unsuffixed and still labelled "Period Savings", which reads as $14,400 saved in a 30-day window. The sub-line "shown in yearly equivalents" at line 311 is the only disambiguation and sits below the number.
  • Fix: decide whether the tile is a rate or a total and make code, label and suffix agree. If it is a rate, append suffix like its neighbours; if it is a total, drop the convertFromMonthly call at line 298. Also correct the comment, which currently documents the opposite of the code (see feedback_snapshot_runrate_avg for the run-rate vs total distinction).

• Clearing the ladder "Buffer Fraction" field silently removes the over-commitment safety buffer (MEDIUM)

  • Where: frontend/src/ladder.ts:437-443 (saveLadderConfig), inputs at frontend/src/ladder.ts:200-213
  • What: each numeric field is read as Number((el as HTMLInputElement | null)?.value ?? '<default>'). The ?? fires only when the element is missing; when the element exists and the user cleared it, .value is '', which is not nullish, so the expression becomes Number('') = 0. The inputs carry min/max but no required, and min is not enforced on an empty value, so the browser does not block submit. For five of the six fields the backend rejects an explicit 0 with a 400 (internal/api/handler_ladder.go:143-153, applyLadderConfigNumericDefaults), so the user gets an error. buffer_fraction is the exception: its documented valid range is [0, 1) and an explicit 0 is a legitimate "no buffer" choice, passed through untouched.
  • Failure scenario: an operator opens Admin -> Purchasing -> a per-account laddering config, selects the Buffer Fraction field (pre-filled 0.10), deletes the contents intending to retype, tabs away to change the cadence instead, and hits Save Config. The request carries buffer_fraction: 0, the backend accepts it as an intentional "no buffer", and the automated laddering engine for that cloud account now commits to its full computed target with zero headroom, raising the chance of an unused non-refundable commitment. Nothing in the UI signals the change.
  • Fix: read the raw string first and treat '' explicitly, either blocking submit with a targeted message or omitting the key so applyLadderConfigNumericDefaults restores the documented default, instead of letting Number('') produce a meaningful 0.

• base64Encode uses btoa, so any non-Latin-1 password hard-fails login with a raw DOMException (MEDIUM)

  • Where: frontend/src/api/client.ts:191-193, used by frontend/src/api/auth.ts:55 (login), :186 (resetPassword), :286 (setupAdmin), :317-318 (changePassword), :343 / :386 (MFA setup/disable), and frontend/src/api/users.ts:37 (createUser)
  • What: btoa throws InvalidCharacterError for any code point above U+00FF. There is no encoding step (for example TextEncoder to a binary string) before the call, and no guard in describePasswordValidationError (frontend/src/auth.ts:300-313) that would stop a user choosing such a password in the first place.
  • Failure scenario: a password contains a curly apostrophe, a dash outside Latin-1, a CJK character or an emoji, all common in passphrases pasted from a password manager. Clicking Login throws inside api.login before any network call. handleLogin (frontend/src/auth.ts:715-731) catches it, mapServerLoginError does not match, and the form displays a raw browser message such as "Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range." The user cannot sign in, the message gives no actionable guidance, and the same throw blocks the reset-password and admin-setup paths, so there is no self-service recovery.
  • Fix: encode to UTF-8 bytes before base64 (btoa(String.fromCharCode(...new TextEncoder().encode(str))), or an explicit helper), and confirm the Go side's base64.StdEncoding.DecodeString consumer treats the result as UTF-8. Add a regression test with a non-Latin-1 password on the login, reset and setup paths.

• Capacity % input coerces 0 and non-numeric text to 100 and truncates fractions (LOW)

  • Where: frontend/src/app.ts:398-401
  • What: Math.max(1, Math.min(100, parseInt(capacityInput.value, 10) || 100)). parseInt('0') is 0, which is falsy, so || 100 fires before the Math.max(1, ...) clamp ever sees it: 0 becomes 100, not 1. parseInt('abc') is NaN, also 100. parseInt('50.7') silently truncates to 50. capacity_percent is not audit-only: it is folded into the server-side idempotency key (internal/api/handler_purchases.go:2514, purchaseIdempotencyKey(creatorID, execReq.Recommendations, execReq.CapacityPercent)) and cross-checked against the scaled rec counts by validateCapacityConsistency (internal/api/validation.go:661-679).
  • Failure scenario: a user types 0 into the bulk-purchase Capacity field intending to buy nothing, then clicks Execute. The submitted capacity_percent is 100, the exact opposite of their intent, and that is what is recorded on the execution row and used as the idempotency key. Separately a fractional entry like 50.5 submits 50 while the recs may have been scaled at 50.5, tripping validateCapacityConsistency into an opaque 400.
  • Fix: parse strictly and reject empty / non-integer / out-of-range with a targeted message rather than coercing, matching readServiceFilterControls in frontend/src/serviceFilters.ts:201-213, which already does this correctly. See feedback_strict_int_parse.

• Fan-out execute leaves the Execute button permanently disabled if any post-submit step throws (LOW)

  • Where: frontend/src/app.ts:495-656, contrast frontend/src/app.ts:464-469
  • What: the single-purchase path (handleExecutePurchase) re-enables the button in a finally block. The fan-out path re-enables it only at the tail of the function (:652-655), with no finally, after formatPaymentAdjustmentNotice, showToast, openArcheraOfferModal and await loadDashboard().
  • Failure scenario: loadDashboard() rejects (API 500 after the purchases were already submitted). The rejection propagates out of handleFanOutExecute, lines 652-655 never run, and the Execute button stays disabled reading "Sending 0/3..." until a page reload, while the purchases actually did submit. The user has no signal that they succeeded and may reasonably assume they need to retry.
  • Fix: move the re-enable into a finally, as the sibling path already does.

• getVisibleRecommendations / getRecommendations return shallow copies despite claiming otherwise (LOW)

  • Where: frontend/src/state.ts:190-201 and frontend/src/state.ts:106-108
  • What: the comment at lines 192-193 says "Defensive clone on read so callers can't mutate module state", but return [...visibleRecommendations] copies only the array; every element is the same Recommendation object held in module state. Consumers cast the result to a local type and pass it around (frontend/src/recommendations.ts:3753, :3784, :2405).
  • Failure scenario: any consumer that assigns to a field on a returned element (an in-place payment or count edit) mutates the module's canonical list, so a later render, or a purchase built from state, picks up the edit even after the user cancels. No live exploit was confirmed in scope, because the current purchase path spreads each rec (frontend/src/app.ts:387-393), but the comment actively invites the unsafe pattern.
  • Fix: either deep-clone (the codebase already has deepClone in frontend/src/utils.ts:252-256, or use structuredClone) or correct the comment to state that the elements are shared. See feedback_deep_copy_state.

• API path parameters are interpolated without encoding, inconsistently with sibling calls (LOW)

  • Where: frontend/src/api/purchases.ts:49,56,69,88,122,138,145,152,159,166,201,212; frontend/src/api/plans.ts:29,46,56,66; frontend/src/api/groups.ts:19,36,46; frontend/src/api/users.ts:24,49,59; frontend/src/api/apikeys.ts:29,36; frontend/src/api/accounts.ts:159,163,170,174,181,185,195,206
  • What: IDs are dropped straight into the template literal. The same file family gets this right elsewhere: frontend/src/api/recommendations.ts:138 (/recommendations/${encodeURIComponent(id)}/detail), frontend/src/api/riexchange.ts:150, frontend/src/purchases-deeplink.ts:105. saveAccountServiceOverride also interpolates the free-form service slug at frontend/src/api/accounts.ts:195.
  • Failure scenario: any id containing /, ?, # or .. re-routes the request to a different endpoint. createMarketplaceListing(purchaseId) takes an AWS ReservedInstancesId (frontend/src/api/purchases.ts:191) rather than an internal UUID, so the value is provider-shaped rather than format-guaranteed by CUDly. The realistic outcome today is a 404 rather than a security break, but the inconsistency means the safe pattern is currently applied by accident rather than by rule.
  • Fix: encodeURIComponent every path segment across frontend/src/api/*, and add a lint rule or a review check so the next endpoint added inherits it.

• fetchOverridesForAccounts swallows every error, so a broken API renders as "no overrides" (LOW)

  • Where: frontend/src/lib/overrides.ts:14-20
  • What: each per-account listAccountServiceOverrides call is wrapped in a bare catch {} with the comment "Silent fallback -- callers handle missing entries gracefully". The returned Map simply omits the failed account, which is indistinguishable from "this account has no overrides configured".
  • Failure scenario: the overrides endpoint 500s, or 403s for a non-admin. The Settings service-defaults UI seeds every affected account from the global default and shows no error. An operator who had previously scoped an account to include_regions: [us-east-1] sees the account presented as unrestricted, and saving from that screen can persist the wider default over their narrower override.
  • Fix: return per-account outcomes (value or error) rather than a bare Map, and let the caller render a partial-failure state. See feedback_no_silent_fallbacks.

• getCostPeriod returns the static default on corrupt storage without repairing memory state (LOW)

  • Where: frontend/src/state.ts:217-236
  • What: when localStorage holds a value outside VALID_PERIODS, the function returns the literal 'monthly' (line 231) but leaves both costPeriodMemory and the corrupt localStorage entry untouched, and notifies nothing. Every subsequent call re-reads and re-discards the same bad value.
  • Failure scenario: a stale or hand-edited cudly.recs.costPeriod entry, for example left over from a renamed period value, permanently pins the cost-period selector to Monthly. The user picks "Yearly", setCostPeriod writes it and the selector works, but any code path that reads before that write still gets Monthly, and the mismatch never surfaces.
  • Fix: overwrite the corrupt key with the default on read so the state self-heals and subsequent reads are consistent.

• MFA step retains the plaintext password in module scope after an abandoned login (LOW)

  • Where: frontend/src/auth.ts:675-676, cleared at :678-681, :712, :765, :795
  • What: pendingMFAPassword holds the plaintext password between the first login POST and the MFA code submit. It is cleared on success and on the explicit "Back to login" link, but not on a failed MFA submit that is neither invalid_mfa_code nor mfa_required (frontend/src/auth.ts:810-811 falls through without clearing), and not if the user simply walks away from the MFA prompt.
  • Failure scenario: the password is correct, the server prompts for the TOTP code, and the user leaves the tab open without finishing. The plaintext password stays reachable in the module closure for the lifetime of the page, recoverable from a heap snapshot or by any script that later achieves execution on the origin, long after the login attempt is over. Amplified by LeanerCloud/cloud-commitments-cli#1150 (auth token and API key in localStorage).
  • Fix: clear on any terminal error and on a timeout, and prefer holding a server-issued short-lived MFA continuation token over the raw password.

• initSavingsHistory de-dupes only one of its four listener registrations (LOW)

  • Where: frontend/src/modules/savings-history.ts:560-597
  • What: the unit <select> handler is explicitly de-duped via a stored _unitChangeHandler property (lines 576-584, citing feedback_event_listener_dedup), but the period <select> (line 566), the refresh button (line 570) and the two state.subscribe* registrations (lines 595-596, whose unsubscribe handles are discarded) have no such protection.
  • Failure scenario: initSavingsHistory() is currently reached once per page load from frontend/src/app.ts:187, so no live duplication was confirmed. If it is ever wired into a per-tab-entry path, exactly as initRegistrations already is (first MEDIUM in this list), a single period change would fire N parallel loadSavingsHistory() calls with no generation guard between them.
  • Fix: apply the same stored-reference dedupe, or the dataset['wired'] marker, to all four, and keep the unsubscribe handles so the subscriptions can be torn down.

• renderConfigTable crashes the whole ladder table if target_coverage is absent (LOW)

  • Where: frontend/src/ladder.ts:261
  • What: ${cfg.target_coverage.toFixed(1)}% has no null guard, unlike every other cell in the same row (escapeHtml(...), cfg.updated_at ? ... : 'N/A'). LadderConfig.target_coverage is typed non-nullable in frontend/src/api/ladder.ts:42, but the field is not runtime-validated anywhere on the response path (getLadderConfigs at frontend/src/api/ladder.ts:59-62 just unwraps the envelope), in contrast to the explicit shape validation in frontend/src/api/auth.ts:133-158.
  • Failure scenario: a ladder row written before the column existed, or by a partially-applied migration, comes back with target_coverage: null. .toFixed throws a TypeError inside configs.map, the whole renderConfigTable call aborts, and the per-account configuration table renders as nothing at all, with no error message unless the caller happens to have a try/catch.
  • Fix: guard with the same != null pattern used for updated_at on the adjacent line, rendering 'N/A' for an absent value. Consider runtime-validating the getLadderConfigs envelope the way api/auth.ts validates its responses.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A11-016 (medium)

The same swallow-renders-as-empty shape as the fetchOverridesForAccounts bullet, on an authorization-scoping path. openDuplicateGroupModal catches every listAccounts failure into accounts = [] (frontend/src/groups/groupModals.ts:620-626), and the empty branch of renderDuplicateAccountsList then prints 'No cloud accounts configured yet. Duplicating without scope clones the full source group - add accounts first if you want to restrict.' unconditionally (:562-568). On a 403 or a transient 5xx the operator is told a factual untruth about their deployment and is nudged to create the duplicate with no account scoping, which inherits the source group's allowed_accounts unchanged (:646-650) - the widest available outcome. The modal still opens with the submit path enabled (:630). Rendering an error line and disabling submit on a fetch failure, keeping the 'none configured' copy for a genuinely empty list, is the fix. Audit finding A11-016.

A11-024 (low)

A second defect on the setupAdmin path this issue already names at auth.ts:286. Its non-OK branch awaits a bare response.json() (frontend/src/api/auth.ts:295-298), so a 502 from a proxy or any non-JSON error body throws a SyntaxError that propagates to handleAdminSetupSubmit and renders something like "Unexpected token < in JSON at position 0" in the setup form instead of a usable message. The other three handlers in the same file all guard it: login at :72, requestPasswordReset at :175 and resetPassword at :202 each use .catch(() => ({})). One-line fix, and it lands on the same admin-setup path with no self-service recovery that the base64Encode item calls out. Finding A11-024.

A12-064 (low)

The same coerce-rather-than-reject shape as the Capacity % bullet, on the RI-exchange quote path. collectTargets silently turns an empty or sub-1 Count into 1 (frontend/src/riexchange.ts:1756-1758), four lines after the offering-id branch returns an explicit error for the analogous empty case (:1747-1749). updateRunningTotal repeats the coercion (:1679-1681), so the displayed total agrees with the substitution and nothing signals that the typed value was discarded. A user who clears the Count field intending to retype it, or types 0, gets a quote for a single unit instead of an inline validation error. Returning the same style of error the offering-id check returns is the fix. Audit finding A12-064.

A12-072 (low)

One dead branch in archera.ts, which is in this pass's scope. ArcheraContext declares 'purchase' | 'plan' (archera.ts:47) and openArcheraOfferModal branches on it for the modal headline (:114), but both production call sites pass 'purchase' (app.ts:456, :645), so the "Insure this plan with Archera?" headline is unreachable and frontend/src/__tests__/archera.test.ts:228 asserts a string no user can see. Either wire the plan-creation surface to call openArcheraOfferModal('plan') or drop the parameter and the branch; as it stands the test reads as coverage of a live path. (audit finding A12-072)