chore(api): medium/low findings from the 2026-07-28 full review #100

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 internal/api (all 40 non-test Go files; every route in router.go:99-371 enumerated and traced to its handler for allowed-accounts scoping, requirePermissionConstraints, and money-path / silent-failure behaviour). The CRITICAL and 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.

Two structural root causes account for most of the internal/api findings, including several below:

1. The read side of a feature is scoped by allowed_accounts; the write side of the same feature is not. getAllowedAccounts (session/group scope) and requirePermissionConstraints (per-permission Constraints) are independent gates, and several mutating handlers satisfy only the second, or neither.
2. Where a scope check was retrofitted, it was applied to the no-explicit-filter branch only. A client that supplies an account filter bypasses the retrofit. This shape recurs in three places; two are saved by a downstream post-filter, one is not (the dashboard, filed separately as a HIGH).

Findings from this pass that are NOT in this list

Recorded so nothing looks dropped:

• Dashboard cross-tenant KPI leak on the explicit-filter path: filed as its own HIGH issue (related to, but not covered by, bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959).
• listPlans unscoped: evidence added to LeanerCloud/cloud-commitments-cli#996.
• listExchangeableAzureRIs tenant-wide: evidence added to sec(api): listExchangeableAzureRIs returns tenant-wide reservations without allowed-accounts scoping cloud-commitments-cli#1532.
• requirePermissionConstraints present on only 2 of ~12 mutating endpoints: evidence and full census added to LeanerCloud/cloud-commitments-cli#1300.
• /api/auth/me/permissions returning the owner's permissions for API-key callers: evidence added to LeanerCloud/cloud-commitments-cli#1302.
• finalizePurchaseStatus reporting "pending" after a failed failed-state write: evidence added to LeanerCloud/cloud-commitments-cli#1290.
• Unauthenticated /health disclosing dev_key_in_use: evidence added to LeanerCloud/cloud-commitments-cli#450.
• fetchCommitmentPurchases zeroing dashboard KPIs on a store error: evidence added to LeanerCloud/cloud-commitments-cli#1377.
• Marketplace zero-timestamp residual pricing: promoted to its own HIGH issue by this review and not repeated here.

• listPlanAccounts returns full CloudAccount rows for plans outside the caller's scope (MEDIUM)

  • Where: internal/api/handler_accounts.go:1351-1370
  • What: GET /api/plans/{id}/accounts gates on view:plans only. No requirePlanAccess, and the returned []config.CloudAccount is not filtered by allowed_accounts. Every other plan read (getPlan, handler_plans.go:158) does scope.
  • Failure scenario: a Read-Only user with allowed_accounts = [acct-A] iterates plan UUIDs (obtainable from GET /api/plans, which per LeanerCloud/cloud-commitments-cli#996 is itself unscoped) and calls /api/plans/{id}/accounts to enumerate the account UUIDs, display names, provider and external IDs (AWS account numbers / Azure subscription IDs) of every other tenant's accounts. Those account UUIDs are exactly the input the dashboard explicit-filter leak needs.
  • Fix: add requirePlanAccess and filter the returned slice with auth.MatchesAccount. Closely related to LeanerCloud/cloud-commitments-cli#996; worth folding into that issue if it is being reworked.

• listTargetOfferings and getExchangeQuote skip the account scope their siblings enforce (MEDIUM)

  • Where: internal/api/handler_ri_exchange.go:115-164 (listTargetOfferings), :659-700 (getExchangeQuote)
  • What: both gate on view:purchases and then reach for the deployment's ambient AWS credentials. Neither calls reshapeCloudAccountInScope, which listConvertibleRIs / getRIUtilization / getReshapeRecommendations all do.
  • Failure scenario: user U (allowed_accounts = [acct-A], deployment account is acct-B) gets {"instances":[]} from /api/ri-exchange/instances. U then calls /api/ri-exchange/target-offerings?source_ri_id=<guess>: a 404 (:149) vs a 200 with offerings distinguishes "this RI exists in acct-B" from "it does not", turning the endpoint into an existence oracle for out-of-scope RIs. POST /api/ri-exchange/quote with acct-B's ri_ids similarly returns acct-B pricing and exchange economics.
  • Fix: add the same reshapeCloudAccountInScope short-circuit (empty offerings / 404) to both. listTargetOfferings is already flagged as out-of-scope-but-not-lost in sec(api): listExchangeableAzureRIs returns tenant-wide reservations without allowed-accounts scoping cloud-commitments-cli#1532's triage note; this is its home.

• Coverage tab renders 100% coverage when the recommendations read fails (MEDIUM)

  • Where: internal/api/handler_inventory.go:224-230, coveragePct at :390
  • What: recs, err := h.scheduler.ListRecommendations(...); on error the handler sets recs = nil and continues ("Non-fatal: recommendations are best-effort"). onDemandByKey is then empty, and coveragePct(covered, 0) computes covered/(covered+0)*100 = 100.
  • Failure scenario: a transient Postgres error (or the cold-start CollectRecommendations failure path inside ListRecommendations, internal/scheduler/scheduler.go:947-953) fires while an operator is on Inventory -> Coverage. Every provider/service row renders "100%" with a non-nil overall_coverage_pct. The operator concludes there is no uncovered on-demand spend and skips a commitment purchase. Note coveragePct was deliberately built to return nil for "no signal"; the swallowed error produces the one input shape that defeats that design.
  • Fix: propagate the error (the commitments half already does), or distinguish "no recs" from "recs unavailable" and return nil coverage plus an explicit recs_unavailable flag. Sibling of LeanerCloud/cloud-commitments-cli#1377 (fetchCommitmentPurchases zeroing on the dashboard), but a different function and a different wrong value: 100% rather than 0.

• Reshape staleness banner is suppressed on a freshness read error, so stale recs render as fresh (MEDIUM)

  • Where: internal/api/handler_ri_exchange.go:607-621 (attachReshapeStaleness), response-type contract at :849-853
  • What: the function logs and returns on a GetRecommendationsFreshness error, leaving RecsStaleness == "". Per the contract, empty means fresh; the cold-start branch three lines below deliberately maps "unknown" to "hard", so the error branch contradicts the design decision made immediately next to it.
  • Failure scenario: a DB read blip on recommendations_state while the Cost Explorer cache is 3 days stale. GET /api/ri-exchange/reshape-recommendations returns recs_staleness: "" and no recs_collected_at, the frontend hides the stale-data banner, and the user executes a reshape against 3-day-old utilization data.
  • Fix: set RecsStaleness = "hard" on the error path, same as the nil-LastCollectedAt path.

• A transient DB error permanently fails an approved RI exchange and returns HTTP 200 (MEDIUM)

  • Where: internal/api/handler_ri_exchange.go:1356-1378 (executeApprovedExchange), failExchange at :1276-1281, CAS at :1094/:1127, executeRequest at handler.go:652-660
  • What: executeApprovedExchange runs after the row has already been CAS'd pending -> processing. Its first two statements are GetRIExchangeDailySpend and GetGlobalConfig; either error routes to h.failExchange(...), which writes status=failed and returns (map{"status":"failed"}, nil), a nil error, so executeRequest emits HTTP 200.
  • Failure scenario: an approver clicks the email link. A 2-second Postgres connection blip makes GetRIExchangeDailySpend return an error. The exchange is permanently marked failed (there is no failed -> pending path), the approval token is consumed, and the caller's browser shows a 200 with {"status":"failed","reason":"daily spending cap check failed"}. The exchange must be re-created by a whole new analysis run. No 5xx is emitted, so no alert fires and no client retry is attempted.
  • Fix: distinguish policy rejections (genuinely failed, 200/409) from infrastructure errors. On a store error, return a wrapped error (500) and leave the row in processing for the reconciler, or CAS it back to pending.

• Marketplace maps AWS AuthFailure / UnauthorizedOperation to HTTP 400 (MEDIUM)

  • Where: internal/api/handler_marketplace.go:493-518 (awsMarketplaceClientFaultCodes)
  • What: the client-fault list includes "AuthFailure" and "UnauthorizedOperation", and the fallback apiErr.ErrorFault() == smithy.FaultClient catches the rest. Both codes mean CUDly's own IAM role lacks the permission: a deployment defect, not caller input. Contrast mapAWSExchangeError (handler_ri_exchange.go:809-828), whose fault list deliberately excludes them.
  • Failure scenario: the Lambda role is missing ec2:CreateReservedInstancesListing. Every POST /api/purchases/{id}/marketplace-list returns 400 with AWS's "You are not authorized to perform this operation". The frontend treats it as bad user input, no 5xx alarm fires, and the operator chases a phantom client bug instead of the IAM gap.
  • Fix: drop the two auth codes from the client-fault map, stop trusting ErrorFault() alone, and map them to 502 like other server-side AWS failures. See feedback_http_status_classification.

• Three divergent implementations of the allowed-accounts check; the revoke one ignores the * wildcard and account names (MEDIUM)

  • Where: internal/api/handler_purchases_revoke.go:384-402 (checkRevokeOwnAccountAccess), internal/api/handler_marketplace.go:418-444 (authorizeAllowedAccount), vs the canonical auth.IsUnrestrictedAccess / auth.MatchesAccount (internal/auth/types.go:150-176) used by all 10+ other scoping sites
  • What: the canonical matcher treats a "*" entry as unrestricted and matches an allowed entry against the account's display name as well as its UUID. checkRevokeOwnAccountAccess does neither; it is a plain len(allowed) > 0 && !stringInSlice(uuid, allowed). authorizeAllowedAccount handles "*" but still not the name.
  • Failure scenario: a group's allowed_accounts is configured with display names (a shape MatchesAccount explicitly supports and which filterPurchaseHistoryByAllowedAccounts at handler_history.go:992-1021 relies on), e.g. ["prod-payments"]. User U with revoke-own:purchases sees the account row in History, clicks Revoke inside the 7-day Azure window, and gets 403 permission denied: purchase is in an account you do not have access to. The free-cancel window expires while the user is told they lack access to an account they demonstrably can read. The "*" wildcard case fails the same way for any group carrying the explicit wildcard rather than an empty list.
  • Fix: replace both bespoke matchers with auth.MatchesAccount(allowed, id, nameByID[id]) over h.resolveAccountNamesByID(ctx).

• GET /api/notifications/unsubscribe mutates state, so email prefetchers silently mute approval emails (MEDIUM)

  • Where: internal/api/router.go:325-326 (GET and POST both routed to unsubscribeHandler), internal/api/handler_notifications.go:86-127, validation short-circuit at :56-58
  • What: validateOneClickUnsubscribeBody returns nil immediately for non-POST, and the handler then calls h.config.UpsertNotificationMute on the GET path with no confirmation step. The repo already knows this pattern is wrong: revokeViaEmailToken renders an HTML confirmation form on GET specifically so "email prefetchers cannot auto-trigger the revocation" (handler_purchases.go:1241-1246).
  • Failure scenario: an approval email carries the signed unsubscribe URL. Gmail's image/link proxy, an Outlook Safe-Links scanner, or a corporate mail-security crawler issues a GET against it. The HMAC is valid (it is the real URL), so the approver is muted from purchase_approvals. Every subsequent purchase approval request silently fails to reach them, and the mute is invisible unless someone inspects notification_mutes.
  • Fix: make GET render a confirm form that POSTs, matching renderRevokeConfirmPage; keep the RFC 8058 List-Unsubscribe=One-Click POST as the only mutating path.

• Direct-execute audit stamp failure is logged, not surfaced; the purchase still commits (MEDIUM)

  • Where: internal/api/handler_purchases.go:2629-2643 (directExecutePurchase), invariant documented at :2617-2623
  • What: the function stamps ExecutedByUserID, ExecutedAt and PreApprovalSkipReason onto the in-memory execution and persists them via SavePurchaseExecution. On failure it logs AUDIT GAP: ... and proceeds to ApproveAndExecute. The doc asserts the invariant "a non-nil executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, and both are set atomically in the same SavePurchaseExecution call here"; the error branch is exactly the case that breaks it, and ApproveAndExecute reloads the row from the DB, so the in-memory stamp is not recovered.
  • Failure scenario: a DB write blip during an execute_mode:"direct" request. The RI/SP purchase completes against the cloud provider, and the resulting purchase_executions row shows status=completed with executed_by_user_id = NULL and pre_approval_skip_reason = NULL, indistinguishable from a normally-approved purchase. There is no record that the approval workflow was bypassed or by whom.
  • Fix: return a 500 before calling ApproveAndExecute when the audit stamp cannot be persisted; nothing irreversible has happened yet at that point. LeanerCloud/cloud-commitments-cli#1290 / Silent save-failure after irreversible Execute can bypass MaxPaymentDailyUSD cloud-commitments-cli#1310 cover post-execute save failures, and LeanerCloud/cloud-commitments-cli#1290's "Sweep target" section names this exact site; this one is pre-execute and therefore cleanly recoverable.

• Non-cancellable sleeps up to 13 s inside the Azure revoke request path (LOW)

  • Where: internal/api/handler_purchases_revoke.go:763-787 (persistAzureRevocation), backoffs at :122-126
  • What: retries MarkPurchaseRevoked with time.Sleep(backoff) (1 s + 3 s + 9 s) and never checks ctx.Done().
  • Failure scenario: Azure has already refunded and the DB is down. The Lambda burns 13 s of wall clock past client / API Gateway disconnect before returning the 207; if the invocation deadline lands inside a sleep the handler is killed mid-retry and the 207 RECONCILE_PENDING body never reaches the client, which then retries the whole revoke and hits Azure's "already returned" error.
  • Fix: select { case <-ctx.Done(): return ...; case <-time.After(backoff): }, per feedback_go_ctx_aware_patterns.

• mapAWSMarketplaceError echoes the raw AWS error string to the client on the 502 path (LOW)

  • Where: internal/api/handler_marketplace.go:517 (NewClientError(502, opMsg+": "+err.Error()))
  • What: the server-side branch concatenates the full SDK error. Elsewhere in this package raw store/SDK error text is deliberately withheld from ClientError bodies (see mapCreatePlanStorageError and the issue sec(api/plans): validatePlanAccountProviders leaks raw account UUID + DB error into logs (gap left by #946) cloud-commitments-cli#965 comments at handler_plans.go:220-229, handler_purchases.go:3076-3083).
  • Failure scenario: an SDK failure surfaces the request ID, the account-scoped ARN in the message, and the assumed role name in a body the frontend renders verbatim in a toast.
  • Fix: log err and return a generic 502 message, matching mapAWSExchangeError.

• Token-guarded approve/cancel/revoke endpoints rate-limit fail-open (LOW)

  • Where: internal/api/middleware.go:500-517 (checkRateLimit), used by router.go:584/592/607/858/865 for the approve_cancel_public bucket; fail-closed variant at :519-546
  • What: these five AuthPublic routes are guarded solely by a token, and they use the fail-open checkRateLimit (which returns nil on limiter error) rather than the fail-closed checkRateLimitStrict that the credential endpoints use for exactly this reason.
  • Failure scenario: an attacker induces DB pressure (or the limiter's pool is exhausted under normal load); AllowWithIP errors, and token guessing against /api/purchases/approve/{uuid}?token= becomes unthrottled. Impact is bounded by the 256-bit token (common.GenerateApprovalToken), so this is defence-in-depth rather than an exploitable path today, but it is the same reasoning that produced checkRateLimitStrict, applied inconsistently.
  • Fix: route the approve_cancel_public bucket through checkRateLimitStrict.

• executeExchange discards the exactness flag from maxRat.Float64() and never checks currency (LOW)

  • Where: internal/api/handler_ri_exchange.go:766-780
  • What: maxPayment, _ := maxRat.Float64() drops the exactness bool, and maxPayment is then passed as MaxPurchaseAmount in the constraint set. MaxPurchaseAmount carries no currency field (see feedback_money_cap_usd_denominated / PR feat(azure/ri-exchange): find compatible offerings and execute exchange (closes #596) cloud-commitments-cli#1515), and the body field is named max_payment_due_usd but nothing verifies the exchange is USD-denominated.
  • Failure scenario: the float conversion is inexact only at absurd magnitudes, so the practical exposure is the currency one: an aws-cn or other non-USD deployment's exchange is checked against a USD-denominated cap.
  • Fix: keep the cap comparison in *big.Rat, and fail closed when the quoted currency is not USD. Note: a separate issue from this same review covers the RI-exchange spend cap being compared against a quote whose CurrencyCode is never checked, from the exchange side. If that lands first, this bullet reduces to the maxRat.Float64() exactness half; do not fix the currency check twice.

• firstNonEmptyCurrency defaults reshape output to "USD" (LOW)

  • Where: internal/api/handler_ri_exchange.go:629-637, consumed at :594-596
  • What: when no ConvertibleRI carries a CurrencyCode, the reshape recommendations are labelled "USD" by fiat rather than treated as unknown.
  • Failure scenario: an aws-cn deployment whose RIs come back without CurrencyCode sees CNY-denominated savings figures rendered and compared as USD on the Reshape page. Bounded, display-side, and the field is populated in practice; flagged as the money-path magic-value pattern rather than a demonstrated production defect.
  • Fix: return ("", false) and omit the currency (or 500) rather than defaulting. Same class as the fix(providers/aws): getDurationString/getDurationValue silently fallback to 1yr on unrecognized term cloud-commitments-cli#1266 / LeanerCloud/cloud-commitments-cli#1320 silent-default family.

Checked and found clean (recorded so the next review pass does not re-derive them)

• Router.Route (router.go:384-424): defence-in-depth auth per route; validateRoutes panics on authUnset; every one of the 90 registered routes carries an explicit level, and each AuthPublic route is also present in isPublicEndpoint (middleware.go:19-51), which uses exact-match for /version and /api/register to block prefix-overlap bypass.
• validation.go: UUID / provider / region / service-name / IAM-ARN / GCP-email / token-file-path validators are all allowlist-based; parseAccountIDs caps at 200 and UUID-validates; parseMinSavingsParam rejects NaN/Inf; parseDateRange caps at 366 days.
• purchaseConstraintSets / recTotalCommitment / requireNonZeroCommitment (handler_purchases.go:2122-2183): the MaxPurchaseAmount evasion paths (split batches, no-upfront recs, negative MonthlyCost, zero total) are all closed.
• validateAnalyticsAccountScope (handler_analytics.go:234-250): correctly requires an in-scope account_id for restricted sessions on all three analytics endpoints. This is the pattern the dashboard is missing.
• filterPurchaseHistoryByAllowedAccounts (handler_history.go:992-1021): the empty-AccountID exemption is correctly gated on creator ownership.
• filterRecommendationsByAllowedAccounts in-place recs[:0] filter: verified safe, because Scheduler.ListRecommendations (internal/scheduler/scheduler.go:939) returns a fresh ListStoredRecommendations slice per call, so there is no shared cache array to corrupt.
• updateConfig (handler_config.go:59-120): serialized read-modify-write under advisory lock; the partial-PUT / propagate-defaults gating is correct and does not clobber per-service money fields.

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.

A04-009 (medium)

The store writers behind this have no status guard either, which is what turns the mis-routing into a money-accounting loss. FailRIExchange is an unconditional UPDATE ri_exchange_history SET status='failed' WHERE id=$1 (internal/config/store_postgres.go:2863-2882), so when execErr is a client timeout arriving after AWS accepted the quote, the row leaves the ledger: GetRIExchangeDailySpend sums only completed and processing (:2893-2900), and the next approval's cap check under-counts by that payment_due. Nothing in the store distinguishes 'never submitted' from 'outcome unknown after submission'. CompleteRIExchange (:2798-2816) and CompleteRIExchangeWithPayment (:2822-2840) are equally unguarded, so a duplicate completion callback can overwrite a failed audit row. Fix direction: CAS the terminal writers on the expected source status, and add a distinct ambiguous terminal status the ledger keeps counting until reconciled. Finding A04-009.

A02-023 (low)

A second instance of the divergent-scope-check shape in this list, with a different failure mode. filterDashboardRecommendations (handler_dashboard.go:168-189) and filterRecommendationsByAllowedAccounts (handler_recommendations.go:123-157) implement the same scope/name-map/loop, but diverge on a DB error: the first goes through resolveAccountNamesByID, which returns an empty map when ListCloudAccounts fails (scoping.go:293-296), while the second returns that failure as an error. So the same user hits the same DB error and sees a silently narrowed list on the dashboard and a 500 on the recommendations page. It also means any future scope fix has to land in three places (both filters plus the name-map builder). Deleting filterDashboardRecommendations and calling filterRecommendationsByAllowedAccounts from the dashboard collapses the pair. (audit finding A02-023)