You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
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:
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.
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).
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)
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.
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)
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)
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.
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)
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.
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)
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore 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 inrouter.go:99-371enumerated 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/apifindings, including several below:allowed_accounts; the write side of the same feature is not.getAllowedAccounts(session/group scope) andrequirePermissionConstraints(per-permissionConstraints) are independent gates, and several mutating handlers satisfy only the second, or neither.Findings from this pass that are NOT in this list
Recorded so nothing looks dropped:
listPlansunscoped: evidence added to LeanerCloud/cloud-commitments-cli#996.listExchangeableAzureRIstenant-wide: evidence added to sec(api): listExchangeableAzureRIs returns tenant-wide reservations without allowed-accounts scoping cloud-commitments-cli#1532.requirePermissionConstraintspresent on only 2 of ~12 mutating endpoints: evidence and full census added to LeanerCloud/cloud-commitments-cli#1300./api/auth/me/permissionsreturning the owner's permissions for API-key callers: evidence added to LeanerCloud/cloud-commitments-cli#1302.finalizePurchaseStatusreporting"pending"after a failed failed-state write: evidence added to LeanerCloud/cloud-commitments-cli#1290./healthdisclosingdev_key_in_use: evidence added to LeanerCloud/cloud-commitments-cli#450.fetchCommitmentPurchaseszeroing dashboard KPIs on a store error: evidence added to LeanerCloud/cloud-commitments-cli#1377.listPlanAccounts returns full CloudAccount rows for plans outside the caller's scope (MEDIUM)
internal/api/handler_accounts.go:1351-1370GET /api/plans/{id}/accountsgates onview:plansonly. NorequirePlanAccess, and the returned[]config.CloudAccountis not filtered byallowed_accounts. Every other plan read (getPlan,handler_plans.go:158) does scope.allowed_accounts = [acct-A]iterates plan UUIDs (obtainable fromGET /api/plans, which per LeanerCloud/cloud-commitments-cli#996 is itself unscoped) and calls/api/plans/{id}/accountsto 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.requirePlanAccessand filter the returned slice withauth.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)
internal/api/handler_ri_exchange.go:115-164(listTargetOfferings),:659-700(getExchangeQuote)view:purchasesand then reach for the deployment's ambient AWS credentials. Neither callsreshapeCloudAccountInScope, whichlistConvertibleRIs/getRIUtilization/getReshapeRecommendationsall do.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/quotewith acct-B'sri_idssimilarly returns acct-B pricing and exchange economics.reshapeCloudAccountInScopeshort-circuit (empty offerings / 404) to both.listTargetOfferingsis 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)
internal/api/handler_inventory.go:224-230,coveragePctat:390recs, err := h.scheduler.ListRecommendations(...); on error the handler setsrecs = niland continues ("Non-fatal: recommendations are best-effort").onDemandByKeyis then empty, andcoveragePct(covered, 0)computescovered/(covered+0)*100 = 100.CollectRecommendationsfailure path insideListRecommendations,internal/scheduler/scheduler.go:947-953) fires while an operator is on Inventory -> Coverage. Every provider/service row renders "100%" with a non-niloverall_coverage_pct. The operator concludes there is no uncovered on-demand spend and skips a commitment purchase. NotecoveragePctwas deliberately built to returnnilfor "no signal"; the swallowed error produces the one input shape that defeats that design.nilcoverage plus an explicitrecs_unavailableflag. Sibling of LeanerCloud/cloud-commitments-cli#1377 (fetchCommitmentPurchaseszeroing 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)
internal/api/handler_ri_exchange.go:607-621(attachReshapeStaleness), response-type contract at:849-853GetRecommendationsFreshnesserror, leavingRecsStaleness == "". 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.recommendations_statewhile the Cost Explorer cache is 3 days stale.GET /api/ri-exchange/reshape-recommendationsreturnsrecs_staleness: ""and norecs_collected_at, the frontend hides the stale-data banner, and the user executes a reshape against 3-day-old utilization data.RecsStaleness = "hard"on the error path, same as the nil-LastCollectedAtpath.A transient DB error permanently fails an approved RI exchange and returns HTTP 200 (MEDIUM)
internal/api/handler_ri_exchange.go:1356-1378(executeApprovedExchange),failExchangeat:1276-1281, CAS at:1094/:1127,executeRequestathandler.go:652-660executeApprovedExchangeruns after the row has already been CAS'dpending -> processing. Its first two statements areGetRIExchangeDailySpendandGetGlobalConfig; either error routes toh.failExchange(...), which writesstatus=failedand returns(map{"status":"failed"}, nil), a nil error, soexecuteRequestemits HTTP 200.GetRIExchangeDailySpendreturn an error. The exchange is permanently markedfailed(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.failed, 200/409) from infrastructure errors. On a store error, return a wrapped error (500) and leave the row inprocessingfor the reconciler, or CAS it back topending.Marketplace maps AWS AuthFailure / UnauthorizedOperation to HTTP 400 (MEDIUM)
internal/api/handler_marketplace.go:493-518(awsMarketplaceClientFaultCodes)"AuthFailure"and"UnauthorizedOperation", and the fallbackapiErr.ErrorFault() == smithy.FaultClientcatches the rest. Both codes mean CUDly's own IAM role lacks the permission: a deployment defect, not caller input. ContrastmapAWSExchangeError(handler_ri_exchange.go:809-828), whose fault list deliberately excludes them.ec2:CreateReservedInstancesListing. EveryPOST /api/purchases/{id}/marketplace-listreturns 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.ErrorFault()alone, and map them to 502 like other server-side AWS failures. Seefeedback_http_status_classification.Three divergent implementations of the allowed-accounts check; the revoke one ignores the
*wildcard and account names (MEDIUM)internal/api/handler_purchases_revoke.go:384-402(checkRevokeOwnAccountAccess),internal/api/handler_marketplace.go:418-444(authorizeAllowedAccount), vs the canonicalauth.IsUnrestrictedAccess/auth.MatchesAccount(internal/auth/types.go:150-176) used by all 10+ other scoping sites"*"entry as unrestricted and matches an allowed entry against the account's display name as well as its UUID.checkRevokeOwnAccountAccessdoes neither; it is a plainlen(allowed) > 0 && !stringInSlice(uuid, allowed).authorizeAllowedAccounthandles"*"but still not the name.allowed_accountsis configured with display names (a shapeMatchesAccountexplicitly supports and whichfilterPurchaseHistoryByAllowedAccountsathandler_history.go:992-1021relies on), e.g.["prod-payments"]. User U withrevoke-own:purchasessees the account row in History, clicks Revoke inside the 7-day Azure window, and gets403 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.auth.MatchesAccount(allowed, id, nameByID[id])overh.resolveAccountNamesByID(ctx).GET /api/notifications/unsubscribe mutates state, so email prefetchers silently mute approval emails (MEDIUM)
internal/api/router.go:325-326(GET and POST both routed tounsubscribeHandler),internal/api/handler_notifications.go:86-127, validation short-circuit at:56-58validateOneClickUnsubscribeBodyreturns nil immediately for non-POST, and the handler then callsh.config.UpsertNotificationMuteon the GET path with no confirmation step. The repo already knows this pattern is wrong:revokeViaEmailTokenrenders an HTML confirmation form on GET specifically so "email prefetchers cannot auto-trigger the revocation" (handler_purchases.go:1241-1246).purchase_approvals. Every subsequent purchase approval request silently fails to reach them, and the mute is invisible unless someone inspectsnotification_mutes.renderRevokeConfirmPage; keep the RFC 8058List-Unsubscribe=One-ClickPOST as the only mutating path.Direct-execute audit stamp failure is logged, not surfaced; the purchase still commits (MEDIUM)
internal/api/handler_purchases.go:2629-2643(directExecutePurchase), invariant documented at:2617-2623ExecutedByUserID,ExecutedAtandPreApprovalSkipReasononto the in-memory execution and persists them viaSavePurchaseExecution. On failure it logsAUDIT GAP: ...and proceeds toApproveAndExecute. The doc asserts the invariant "a non-nilexecuted_by_user_idalways co-occurs with a non-nilpre_approval_skip_reason, and both are set atomically in the sameSavePurchaseExecutioncall here"; the error branch is exactly the case that breaks it, andApproveAndExecutereloads the row from the DB, so the in-memory stamp is not recovered.execute_mode:"direct"request. The RI/SP purchase completes against the cloud provider, and the resultingpurchase_executionsrow showsstatus=completedwithexecuted_by_user_id = NULLandpre_approval_skip_reason = NULL, indistinguishable from a normally-approved purchase. There is no record that the approval workflow was bypassed or by whom.ApproveAndExecutewhen 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)
internal/api/handler_purchases_revoke.go:763-787(persistAzureRevocation), backoffs at:122-126MarkPurchaseRevokedwithtime.Sleep(backoff)(1 s + 3 s + 9 s) and never checksctx.Done().RECONCILE_PENDINGbody never reaches the client, which then retries the whole revoke and hits Azure's "already returned" error.select { case <-ctx.Done(): return ...; case <-time.After(backoff): }, perfeedback_go_ctx_aware_patterns.mapAWSMarketplaceError echoes the raw AWS error string to the client on the 502 path (LOW)
internal/api/handler_marketplace.go:517(NewClientError(502, opMsg+": "+err.Error()))mapCreatePlanStorageErrorand the issue sec(api/plans): validatePlanAccountProviders leaks raw account UUID + DB error into logs (gap left by #946) cloud-commitments-cli#965 comments athandler_plans.go:220-229,handler_purchases.go:3076-3083).errand return a generic 502 message, matchingmapAWSExchangeError.Token-guarded approve/cancel/revoke endpoints rate-limit fail-open (LOW)
internal/api/middleware.go:500-517(checkRateLimit), used byrouter.go:584/592/607/858/865for theapprove_cancel_publicbucket; fail-closed variant at:519-546AuthPublicroutes are guarded solely by a token, and they use the fail-opencheckRateLimit(which returns nil on limiter error) rather than the fail-closedcheckRateLimitStrictthat the credential endpoints use for exactly this reason.AllowWithIPerrors, 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 producedcheckRateLimitStrict, applied inconsistently.approve_cancel_publicbucket throughcheckRateLimitStrict.executeExchange discards the exactness flag from maxRat.Float64() and never checks currency (LOW)
internal/api/handler_ri_exchange.go:766-780maxPayment, _ := maxRat.Float64()drops the exactness bool, andmaxPaymentis then passed asMaxPurchaseAmountin the constraint set.MaxPurchaseAmountcarries no currency field (seefeedback_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 namedmax_payment_due_usdbut nothing verifies the exchange is USD-denominated.*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 whoseCurrencyCodeis never checked, from the exchange side. If that lands first, this bullet reduces to themaxRat.Float64()exactness half; do not fix the currency check twice.firstNonEmptyCurrency defaults reshape output to "USD" (LOW)
internal/api/handler_ri_exchange.go:629-637, consumed at:594-596ConvertibleRIcarries aCurrencyCode, the reshape recommendations are labelled "USD" by fiat rather than treated as unknown.CurrencyCodesees 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.("", 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;validateRoutespanics onauthUnset; every one of the 90 registered routes carries an explicit level, and eachAuthPublicroute is also present inisPublicEndpoint(middleware.go:19-51), which uses exact-match for/versionand/api/registerto block prefix-overlap bypass.validation.go: UUID / provider / region / service-name / IAM-ARN / GCP-email / token-file-path validators are all allowlist-based;parseAccountIDscaps at 200 and UUID-validates;parseMinSavingsParamrejects NaN/Inf;parseDateRangecaps at 366 days.purchaseConstraintSets/recTotalCommitment/requireNonZeroCommitment(handler_purchases.go:2122-2183): theMaxPurchaseAmountevasion paths (split batches, no-upfront recs, negativeMonthlyCost, zero total) are all closed.validateAnalyticsAccountScope(handler_analytics.go:234-250): correctly requires an in-scopeaccount_idfor restricted sessions on all three analytics endpoints. This is the pattern the dashboard is missing.filterPurchaseHistoryByAllowedAccounts(handler_history.go:992-1021): the empty-AccountIDexemption is correctly gated on creator ownership.filterRecommendationsByAllowedAccountsin-placerecs[:0]filter: verified safe, becauseScheduler.ListRecommendations(internal/scheduler/scheduler.go:939) returns a freshListStoredRecommendationsslice 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 oforigin/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) andfilterRecommendationsByAllowedAccounts(handler_recommendations.go:123-157) implement the same scope/name-map/loop, but diverge on a DB error: the first goes throughresolveAccountNamesByID, which returns an empty map whenListCloudAccountsfails (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). DeletingfilterDashboardRecommendationsand callingfilterRecommendationsByAllowedAccountsfrom the dashboard collapses the pair. (audit finding A02-023)