Repository navigation
sec(api/plans): stop leaking raw account UUID + DB error in plan-account validation log - #969
Merged
cristim merged 4 commits intoJun 8, 2026
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reached
More reviews will be available in 24 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR systematically prevents sensitive information (plan/account UUIDs and raw database error strings) from leaking through API error logs. It adds logger output redirection to the logging package, introduces a test helper for capturing logs, and applies ChangesPII/DB Error Leakage Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
sec/plan-account-provider-log-leak
branch
from
June 5, 2026 12:13
6529b70 to
09d4658
Compare
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
…anch (refs #965) getPlanForAccountProviderValidation returned fmt.Errorf("accounts: failed to get plan: %w", err) on a non-ErrNotFound storage error. Because this is not a ClientError, the router logged it via logging.Errorf("API error: %v"), emitting the raw DB error string on every POST /plans and PUT /plans/:id/accounts call that hits a DB failure (same endpoints as the sibling GetCloudAccount leak closed by PR #969). Fix: delegate to the existing mapCreatePlanStorageError helper (introduced by PR #969 for the GetCloudAccount branch) so the response is a generic 500 ClientError and the log line contains only a static format string -- no raw DB error, no plan UUID. Adds two regression tests: - TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak: asserts the handler returns a 500 ClientError and that neither the client response nor the captured log buffer contain the raw DB error string; FAILS pre-fix. - TestGetPlanForAccountProviderValidation_PlanNotFound_Still404: guards that ErrNotFound still maps to 404 after the refactor.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 6, 2026
…wup #965) PR #969 closed the raw-UUID + raw-DB-error leak in validatePlanAccountProviders and getPlanForAccountProviderValidation. Adversarial review surfaced five sibling leak sites of the exact same shape -- a non-ClientError wrapped with a raw account/plan UUID and the underlying DB error -- that all propagate through the router's `logging.Errorf("API error: %v")` path and leak both the UUID and the raw DB error string. Fixed sites (all use `mapCreatePlanStorageError` so a DB error becomes a generic 500 ClientError, and a structured log line is emitted without PII): - handler_plans.go:209 updatePlan -- existingPlan fetch - handler_plans.go:333 getPlanForPurchaseCreation -- plan fetch for createPlannedPurchases - handler_plans.go:447 patchPlan -- plan fetch for PATCH - handler_purchases.go:323/328 disablePlan -- GetPurchasePlan and UpdatePurchasePlan failure paths - handler_purchases.go:2006 lookupContactEmail -- the Warnf used to log the raw account UUID and the raw err; now logs only the failure point - handler_purchases.go:1955 gatherAccountContactEmails -- the wrap site used to re-leak the same account UUID through `%s: %w`; now returns a generic 500 ClientError (lookupContactEmail has already logged without PII) Test changes: - New: TestHandler_patchPlan_GetPlanDBError_NoPIILeak, TestHandler_updatePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_UpdatePlanDBError_NoPIILeak, TestLookupContactEmail_GetAccountDBError_NoPIILeak, TestGatherAccountContactEmails_DBError_NoPIILeak -- each asserts the ClientError is 500 (server-side fault), the returned message contains neither the raw UUID nor the raw DB error string, and (where the path logs via the default logger) the captured log buffer contains neither either. Pattern mirrors the existing TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak. - Updated: TestHandler_patchPlan_NotFound, TestHandler_createPlannedPurchases_PlanNotFound -- assertions moved off the legacy "failed to get plan" wrapper string to the new contract: 500 ClientError without the raw plan UUID. Both tests return a plain `errors.New("not found")` from the mock (not config.ErrNotFound) so they exercise the 500 branch, not the 404 branch. - Updated: TestHandler_resolveApprovalRecipients_LookupErrorPropagates -- the test's stated intent (no silent globalNotify fallback) is retained via the require.Error guard; the ErrorIs(transient) assertion was guarding the implementation detail that broke the no-PII-leak contract. Replaced with 500-ClientError + no-UUID + no-raw-DB-error assertions consistent with the rest of the issue #965 family. Full internal/api suite: 1474 passed; full short suite: only two pre-existing internal/auth failures unrelated to this PR. refs #965
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…anch (refs #965) getPlanForAccountProviderValidation returned fmt.Errorf("accounts: failed to get plan: %w", err) on a non-ErrNotFound storage error. Because this is not a ClientError, the router logged it via logging.Errorf("API error: %v"), emitting the raw DB error string on every POST /plans and PUT /plans/:id/accounts call that hits a DB failure (same endpoints as the sibling GetCloudAccount leak closed by PR #969). Fix: delegate to the existing mapCreatePlanStorageError helper (introduced by PR #969 for the GetCloudAccount branch) so the response is a generic 500 ClientError and the log line contains only a static format string -- no raw DB error, no plan UUID. Adds two regression tests: - TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak: asserts the handler returns a 500 ClientError and that neither the client response nor the captured log buffer contain the raw DB error string; FAILS pre-fix. - TestGetPlanForAccountProviderValidation_PlanNotFound_Still404: guards that ErrNotFound still maps to 404 after the refactor.
cristim
force-pushed
the
sec/plan-account-provider-log-leak
branch
from
June 7, 2026 08:00
9b1a285 to
b783c27
Compare
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…wup #965) PR #969 closed the raw-UUID + raw-DB-error leak in validatePlanAccountProviders and getPlanForAccountProviderValidation. Adversarial review surfaced five sibling leak sites of the exact same shape -- a non-ClientError wrapped with a raw account/plan UUID and the underlying DB error -- that all propagate through the router's `logging.Errorf("API error: %v")` path and leak both the UUID and the raw DB error string. Fixed sites (all use `mapCreatePlanStorageError` so a DB error becomes a generic 500 ClientError, and a structured log line is emitted without PII): - handler_plans.go:209 updatePlan -- existingPlan fetch - handler_plans.go:333 getPlanForPurchaseCreation -- plan fetch for createPlannedPurchases - handler_plans.go:447 patchPlan -- plan fetch for PATCH - handler_purchases.go:323/328 disablePlan -- GetPurchasePlan and UpdatePurchasePlan failure paths - handler_purchases.go:2006 lookupContactEmail -- the Warnf used to log the raw account UUID and the raw err; now logs only the failure point - handler_purchases.go:1955 gatherAccountContactEmails -- the wrap site used to re-leak the same account UUID through `%s: %w`; now returns a generic 500 ClientError (lookupContactEmail has already logged without PII) Test changes: - New: TestHandler_patchPlan_GetPlanDBError_NoPIILeak, TestHandler_updatePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_UpdatePlanDBError_NoPIILeak, TestLookupContactEmail_GetAccountDBError_NoPIILeak, TestGatherAccountContactEmails_DBError_NoPIILeak -- each asserts the ClientError is 500 (server-side fault), the returned message contains neither the raw UUID nor the raw DB error string, and (where the path logs via the default logger) the captured log buffer contains neither either. Pattern mirrors the existing TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak. - Updated: TestHandler_patchPlan_NotFound, TestHandler_createPlannedPurchases_PlanNotFound -- assertions moved off the legacy "failed to get plan" wrapper string to the new contract: 500 ClientError without the raw plan UUID. Both tests return a plain `errors.New("not found")` from the mock (not config.ErrNotFound) so they exercise the 500 branch, not the 404 branch. - Updated: TestHandler_resolveApprovalRecipients_LookupErrorPropagates -- the test's stated intent (no silent globalNotify fallback) is retained via the require.Error guard; the ErrorIs(transient) assertion was guarding the implementation detail that broke the no-PII-leak contract. Replaced with 500-ClientError + no-UUID + no-raw-DB-error assertions consistent with the rest of the issue #965 family. Full internal/api suite: 1474 passed; full short suite: only two pre-existing internal/auth failures unrelated to this PR. refs #965
Member
Author
|
Rebased onto @coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/handler_plans.go (1)
210-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
updatePlanagainst(nil, nil)plan lookups to prevent a panic.Line 222 dereferences
existingPlanwithout a nil check. If the store returns(nil, nil)(already handled in sibling paths), this panics and turns not-found into a 500 crash path.Suggested fix
existingPlan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { return nil, mapCreatePlanStorageError(err, "plan not found", "failed to load plan", "updatePlan: GetPurchasePlan failed") } +if existingPlan == nil { + return nil, NewClientError(404, fmt.Sprintf("plan not found: %s", planID)) +} // Create new plan from request plan := req.toPurchasePlan() plan.ID = planID🤖 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 `@internal/api/handler_plans.go` around lines 210 - 223, The updatePlan flow calls h.config.GetPurchasePlan and immediately dereferences existingPlan; add an explicit nil check after the GetPurchasePlan call to handle the (nil, nil) case: if existingPlan == nil return nil with mapCreatePlanStorageError (using the same "plan not found"/"failed to load plan"/"updatePlan: GetPurchasePlan failed" semantics) so we don't panic when taking existingPlan.CreatedAt; keep the remaining logic (creating plan via req.toPurchasePlan, setting plan.ID, copying CreatedAt and setting UpdatedAt) unchanged.internal/api/handler_purchases.go (1)
382-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize the conflict-recovery fetch too.
deletePlannedPurchaseno longer leaks ondisablePlan, but this fallback still returns a rawfmt.ErrorfwithexecutionIDandgetErr. If the conflict-recoveryGetExecutionByIDhits a DB error,handleRequestErrorwill log that verbatim, so this endpoint still leaks an identifier and backend error text on a realistic retry/conflict path.Suggested fix
existing, getErr := h.config.GetExecutionByID(ctx, executionID) if getErr != nil { - return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr) + logging.Errorf("cancelOrRecoverExecution: GetExecutionByID failed after conflict") + return nil, NewClientError(500, "failed to cancel execution") }🤖 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 `@internal/api/handler_purchases.go` around lines 382 - 385, The fallback error returned after calling h.config.GetExecutionByID leaks sensitive details (executionID and DB error) to handleRequestError; change the return to a sanitized error message that does not include executionID or the raw getErr (e.g., return fmt.Errorf("disable plan: failed to retrieve execution after conflict")), leaving h.config.GetExecutionByID and executionID unchanged; ensure this same sanitized message is used in the disablePlan/conflict-recovery path (where deletePlannedPurchase is called) so handleRequestError no longer logs backend error text or identifiers.
🧹 Nitpick comments (1)
internal/api/handler_plans_test.go (1)
1050-1095: ⚡ Quick winAdd a regression for
updatePlanwhenGetPurchasePlanreturns(nil, nil).These new tests cover DB-error leakage well, but a nil-plan branch test is still missing for
updatePlan. Adding it will lock in 404 behavior and prevent nil-deref regressions.🤖 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 `@internal/api/handler_plans_test.go` around lines 1050 - 1095, Add a test that covers the case where mockStore.GetPurchasePlan returns (nil, nil) so updatePlan handles a missing plan without panicking and returns a 404 ClientError; specifically, in the new test arrange mockStore.On("GetPurchasePlan", ctx, planUUID).Return(nil, nil), call handler.updatePlan(ctx, req, planUUID) and assert it returns a ClientError with code 404, nil result, and that neither the planUUID nor any raw DB string appear in the error message or captured logs; this keeps behavior consistent with the 404 branch and prevents nil-deref regressions in updatePlan.
🤖 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 `@internal/api/handler_purchases.go`:
- Around line 2107-2112: In lookupContactEmail, change the log call that
currently uses logging.Warnf("lookupContactEmail: GetCloudAccount failed") to an
error-level log (logging.Errorf) so the server-side signal remains at error
severity; keep the message generic (do NOT include the raw id or err) exactly as
in the existing string to avoid leaking sensitive DB/account data, e.g., replace
the Warnf invocation with an Errorf invocation in the
lookupContactEmail/GetCloudAccount failure handling block.
---
Outside diff comments:
In `@internal/api/handler_plans.go`:
- Around line 210-223: The updatePlan flow calls h.config.GetPurchasePlan and
immediately dereferences existingPlan; add an explicit nil check after the
GetPurchasePlan call to handle the (nil, nil) case: if existingPlan == nil
return nil with mapCreatePlanStorageError (using the same "plan not
found"/"failed to load plan"/"updatePlan: GetPurchasePlan failed" semantics) so
we don't panic when taking existingPlan.CreatedAt; keep the remaining logic
(creating plan via req.toPurchasePlan, setting plan.ID, copying CreatedAt and
setting UpdatedAt) unchanged.
In `@internal/api/handler_purchases.go`:
- Around line 382-385: The fallback error returned after calling
h.config.GetExecutionByID leaks sensitive details (executionID and DB error) to
handleRequestError; change the return to a sanitized error message that does not
include executionID or the raw getErr (e.g., return fmt.Errorf("disable plan:
failed to retrieve execution after conflict")), leaving
h.config.GetExecutionByID and executionID unchanged; ensure this same sanitized
message is used in the disablePlan/conflict-recovery path (where
deletePlannedPurchase is called) so handleRequestError no longer logs backend
error text or identifiers.
---
Nitpick comments:
In `@internal/api/handler_plans_test.go`:
- Around line 1050-1095: Add a test that covers the case where
mockStore.GetPurchasePlan returns (nil, nil) so updatePlan handles a missing
plan without panicking and returns a 404 ClientError; specifically, in the new
test arrange mockStore.On("GetPurchasePlan", ctx, planUUID).Return(nil, nil),
call handler.updatePlan(ctx, req, planUUID) and assert it returns a ClientError
with code 404, nil result, and that neither the planUUID nor any raw DB string
appear in the error message or captured logs; this keeps behavior consistent
with the 404 branch and prevents nil-deref regressions in updatePlan.
🪄 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: 77770b34-eeba-43bd-8659-f20ca5b75cfb
📒 Files selected for processing (7)
internal/api/handler_accounts.gointernal/api/handler_accounts_test.gointernal/api/handler_plans.gointernal/api/handler_plans_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gopkg/logging/logger.go
…unt validation log
The GetCloudAccount DB-error path inside validatePlanAccountProviders
returned fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr),
a non-ClientError that propagated to the router's
logging.Errorf("API error: %v"), leaking the raw account UUID and the
raw DB error string into logs (gap left by #946 which closed #944).
Wrap the error with the existing mapCreatePlanStorageError helper so
that: (a) a DB error emits a structured logging.Errorf carrying only
the derived provider list and account count (never the UUID), and
(b) returns a generic 500 ClientError ("failed to validate plan
accounts") with no raw UUID or DB string. The ErrNotFound sentinel
still maps to 404; the explicit (nil, nil) not-found path is unchanged.
Add logging.SetOutput as a test seam on the default logger and two
regression tests:
- TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: asserts
that a DB error yields a 500 ClientError whose message and emitted
log contain neither the account UUID nor the raw DB error string.
- TestValidatePlanAccountProviders_AccountNotFound_Still404: guards that
the not-found path still returns a 404 referencing the missing ID.
Also fixes the pre-existing build failure on the base branch
(Session.Role field removed in #940 but the test was not updated) and
gofmt drift in handler_purchases_guards_test.go.
closes #965
…anch (refs #965) getPlanForAccountProviderValidation returned fmt.Errorf("accounts: failed to get plan: %w", err) on a non-ErrNotFound storage error. Because this is not a ClientError, the router logged it via logging.Errorf("API error: %v"), emitting the raw DB error string on every POST /plans and PUT /plans/:id/accounts call that hits a DB failure (same endpoints as the sibling GetCloudAccount leak closed by PR #969). Fix: delegate to the existing mapCreatePlanStorageError helper (introduced by PR #969 for the GetCloudAccount branch) so the response is a generic 500 ClientError and the log line contains only a static format string -- no raw DB error, no plan UUID. Adds two regression tests: - TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak: asserts the handler returns a 500 ClientError and that neither the client response nor the captured log buffer contain the raw DB error string; FAILS pre-fix. - TestGetPlanForAccountProviderValidation_PlanNotFound_Still404: guards that ErrNotFound still maps to 404 after the refactor.
…wup #965) PR #969 closed the raw-UUID + raw-DB-error leak in validatePlanAccountProviders and getPlanForAccountProviderValidation. Adversarial review surfaced five sibling leak sites of the exact same shape -- a non-ClientError wrapped with a raw account/plan UUID and the underlying DB error -- that all propagate through the router's `logging.Errorf("API error: %v")` path and leak both the UUID and the raw DB error string. Fixed sites (all use `mapCreatePlanStorageError` so a DB error becomes a generic 500 ClientError, and a structured log line is emitted without PII): - handler_plans.go:209 updatePlan -- existingPlan fetch - handler_plans.go:333 getPlanForPurchaseCreation -- plan fetch for createPlannedPurchases - handler_plans.go:447 patchPlan -- plan fetch for PATCH - handler_purchases.go:323/328 disablePlan -- GetPurchasePlan and UpdatePurchasePlan failure paths - handler_purchases.go:2006 lookupContactEmail -- the Warnf used to log the raw account UUID and the raw err; now logs only the failure point - handler_purchases.go:1955 gatherAccountContactEmails -- the wrap site used to re-leak the same account UUID through `%s: %w`; now returns a generic 500 ClientError (lookupContactEmail has already logged without PII) Test changes: - New: TestHandler_patchPlan_GetPlanDBError_NoPIILeak, TestHandler_updatePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_GetPlanDBError_NoPIILeak, TestDisablePlan_UpdatePlanDBError_NoPIILeak, TestLookupContactEmail_GetAccountDBError_NoPIILeak, TestGatherAccountContactEmails_DBError_NoPIILeak -- each asserts the ClientError is 500 (server-side fault), the returned message contains neither the raw UUID nor the raw DB error string, and (where the path logs via the default logger) the captured log buffer contains neither either. Pattern mirrors the existing TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak. - Updated: TestHandler_patchPlan_NotFound, TestHandler_createPlannedPurchases_PlanNotFound -- assertions moved off the legacy "failed to get plan" wrapper string to the new contract: 500 ClientError without the raw plan UUID. Both tests return a plain `errors.New("not found")` from the mock (not config.ErrNotFound) so they exercise the 500 branch, not the 404 branch. - Updated: TestHandler_resolveApprovalRecipients_LookupErrorPropagates -- the test's stated intent (no silent globalNotify fallback) is retained via the require.Error guard; the ErrorIs(transient) assertion was guarding the implementation detail that broke the no-PII-leak contract. Replaced with 500-ClientError + no-UUID + no-raw-DB-error assertions consistent with the rest of the issue #965 family. Full internal/api suite: 1474 passed; full short suite: only two pre-existing internal/auth failures unrelated to this PR. refs #965
cristim
force-pushed
the
sec/plan-account-provider-log-leak
branch
from
June 8, 2026 04:17
b783c27 to
3c6e0b6
Compare
The GetCloudAccount failure in lookupContactEmail was logged at Warnf,
but its caller gatherAccountContactEmails maps the error into a generic
500 ClientError. The router returns ClientErrors via the branch that
skips logging.Errorf("API error: %v"), so this log line is the only
error-level breadcrumb for the resulting user-visible 500. Downgrading
it to Warnf weakened error-level alerting for exactly the outage this
change set is preserving visibility for.
Restore Errorf while keeping the message generic (no raw account UUID,
no raw DB error string) so the PII-leak fix is unaffected. Extend the
regression test to assert the [ERROR] severity and the generic message;
it fails pre-fix (emits [WARN]) and passes after.
Addresses CodeRabbit review on PR #969.
Member
Author
|
Addressed the unresolved CodeRabbit finding. Finding ( Fix ( Verification: |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #965. Fixes the PII/info-leak gap left by #946 (which closed #944).
validatePlanAccountProviderscalledh.config.GetCloudAccountand on a DB error returned:This non-
ClientErrorpropagated to the router'slogging.Errorf("API error: %v"), logging the raw account UUID and raw DB error string. Issue #944(b) required structured logs with account count (never raw IDs).Changes
internal/api/handler_accounts.go: wrap theGetCloudAccountDB-error path withmapCreatePlanStorageError, emitting a structured log with provider list + account count; returns a generic 500ClientError. TheErrNotFoundsentinel still maps to 404; the explicit(nil, nil)not-found path is unchanged.pkg/logging/logger.go: addSetOutputas a test seam for capturing default-logger output in tests.internal/api/handler_accounts_test.go: two regression tests -TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeakandTestValidatePlanAccountProviders_AccountNotFound_Still404.internal/api/handler_purchases_test.go+handler_purchases_guards_test.go: fix pre-existing build failure (Session.Rolefield removed in fix(api/purchases): base build broken - session.Role removed by #907 but still used in authorizeSessionExecuteDirect (#803) #940 but test not updated) and gofmt drift, so the test suite compiles.Test plan
go test ./internal/api/... -count=1passes (1460 tests)TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak: DB error yields 500 ClientError with no UUID/DB-error in message or logTestValidatePlanAccountProviders_AccountNotFound_Still404: not-found still returns 404 referencing the missing IDgofmt -lclean on all changed filesgo vet ./...cleango build ./...succeedsSummary by CodeRabbit
Bug Fixes
Tests