From 5052db576b7df978e9dd3a140848caed9120fa9c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:21:28 +0200 Subject: [PATCH 1/4] sec(api/plans): stop leaking raw account UUID + DB error in plan-account 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 --- internal/api/handler_accounts.go | 11 +++- internal/api/handler_accounts_test.go | 85 +++++++++++++++++++++++++++ pkg/logging/logger.go | 15 +++++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 4f151dfb3..72c34ef4c 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1230,7 +1230,16 @@ func (h *Handler) validatePlanAccountProviders(ctx context.Context, planID strin for _, aid := range accountIDs { acct, getErr := h.config.GetCloudAccount(ctx, aid) if getErr != nil { - return fmt.Errorf("accounts: failed to get account %s: %w", aid, getErr) + // Do NOT wrap getErr with the raw account UUID: a non-ClientError + // here propagates to the router's `logging.Errorf("API error: %v")`, + // which would leak the account UUID and the raw DB error string + // into logs (issue #944(b): log account counts, never raw IDs). + // Map to a 404 if the store reports not-found, else a structured + // 500 that logs only the derived providers + account count. + return mapCreatePlanStorageError(getErr, + "account not found", "failed to validate plan accounts", + "validatePlanAccountProviders: GetCloudAccount failed (providers=%v accounts=%d)", + expected, len(accountIDs)) } if acct == nil { return NewClientError(404, fmt.Sprintf("account not found: %s", aid)) diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index e3fdab2ed..4c63417a1 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1,6 +1,7 @@ package api import ( + "bytes" "context" "encoding/json" "errors" @@ -10,6 +11,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/accounts" "github.com/LeanerCloud/CUDly/internal/commitmentopts" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" "github.com/jackc/pgx/v5" @@ -1944,3 +1946,86 @@ func TestTestAccountCredentials_RequiresUpdateAccounts(t *testing.T) { require.True(t, ok, "expected ClientError, got %T: %v", err, err) assert.Equal(t, 403, ce.code, "expected 403, got %d", ce.code) } + +// captureDefaultLog redirects the default logger's output to a buffer for the +// duration of the test and restores it on cleanup. Used to assert that the +// log line emitted on the GetCloudAccount DB-error path leaks no PII. +func captureDefaultLog(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + prev := logging.SetOutput(&buf) + t.Cleanup(func() { logging.SetOutput(prev) }) + return &buf +} + +// TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak is a regression +// test for the PII-leak gap left by PR #946 (which closed #944): the +// GetCloudAccount DB-error path inside validatePlanAccountProviders wrapped the +// raw error with the account UUID and returned a non-ClientError, so the raw +// UUID + raw DB error string leaked into the router's +// `logging.Errorf("API error: %v")`. The fix returns a generic 500 ClientError +// and logs only the derived providers + account count. +// +// Asserts that on a DB error: (1) a 500 ClientError is returned, (2) neither +// the returned error message nor the emitted log contains the account UUID or +// the raw DB error string; and that account-not-found still maps to 404 with +// the offending ID (intentional, since the ID came from the caller's request). +func TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const rawDBErr = "pq: connection refused to host db-internal-99.example.com" + + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return awsPlan209(), nil + } + store.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return nil, errors.New(rawDBErr) + } + handler := &Handler{config: store} + + logBuf := captureDefaultLog(t) + + err := handler.validatePlanAccountProviders(ctx, planID209, []string{awsAcct209}) + require.Error(t, err) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must be mapped to a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code, "a DB error during account validation is a server-side fault") + + // The user-facing error message must carry no raw UUID or DB string. + assert.NotContains(t, ce.Error(), awsAcct209, "500 message must not leak the account UUID") + assert.NotContains(t, ce.Error(), rawDBErr, "500 message must not leak the raw DB error") + + // Simulate the router logging a leaked non-ClientError; with the fix in + // place no such log is emitted (the error is already a ClientError), so the + // buffer must contain neither the UUID nor the raw DB string regardless of + // whether validate logged its own structured line. + logged := logBuf.String() + assert.NotContains(t, logged, awsAcct209, "log must not contain the raw account UUID (issue #944(b))") + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string") +} + +// TestValidatePlanAccountProviders_AccountNotFound_Still404 guards that the +// no-PII-leak fix did not regress the account-not-found path: a store-style +// (nil, nil) result must still produce a 404 referencing the missing ID (which +// is safe to echo, since the caller supplied it). +func TestValidatePlanAccountProviders_AccountNotFound_Still404(t *testing.T) { + ctx := context.Background() + + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return awsPlan209(), nil + } + store.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return nil, nil // store-style not-found + } + handler := &Handler{config: store} + + err := handler.validatePlanAccountProviders(ctx, planID209, []string{missingAcct}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 404, ce.code) + assert.Contains(t, ce.Error(), missingAcct, "404 must still reference the missing account ID") +} diff --git a/pkg/logging/logger.go b/pkg/logging/logger.go index f4ca31d7e..e33a1693c 100644 --- a/pkg/logging/logger.go +++ b/pkg/logging/logger.go @@ -36,6 +36,7 @@ const ( type Logger struct { level atomic.Int32 logger *log.Logger + output io.Writer prefix string metadata map[string]interface{} } @@ -97,6 +98,7 @@ func New(cfg Config) *Logger { l := &Logger{ logger: log.New(output, cfg.Prefix, flags), + output: output, prefix: cfg.Prefix, metadata: make(map[string]interface{}), } @@ -123,6 +125,7 @@ func GetLevel() Level { func (l *Logger) With(key string, value interface{}) *Logger { newLogger := &Logger{ logger: l.logger, + output: l.output, prefix: l.prefix, metadata: make(map[string]interface{}), } @@ -260,3 +263,15 @@ func With(key string, value interface{}) *Logger { func GetDefaultLogger() *Logger { return defaultLogger } + +// SetOutput redirects the default logger's output to w and returns the +// previous io.Writer so callers can restore it (e.g. in a test t.Cleanup). +// The default logger binds os.Stderr at init() time, so reassigning the +// os.Stderr variable does not redirect it; this is the supported seam for +// capturing default-logger output in tests. +func SetOutput(w io.Writer) io.Writer { + prev := defaultLogger.output + defaultLogger.output = w + defaultLogger.logger.SetOutput(w) + return prev +} From 7265ab29376e44be46a74d657e8c09fa42e00b72 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 16:07:34 +0200 Subject: [PATCH 2/4] fix(api/plans): stop leaking raw DB error in plan-fetch validation branch (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. --- internal/api/handler_accounts.go | 11 +++-- internal/api/handler_accounts_test.go | 60 +++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 72c34ef4c..095e81cdb 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1263,10 +1263,13 @@ func (h *Handler) validatePlanAccountProviders(ctx context.Context, planID strin func (h *Handler) getPlanForAccountProviderValidation(ctx context.Context, planID string) (*config.PurchasePlan, error) { plan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { - if errors.Is(err, config.ErrNotFound) { - return nil, NewClientError(404, fmt.Sprintf("plan not found: %s", planID)) - } - return nil, fmt.Errorf("accounts: failed to get plan: %w", err) + // Do NOT wrap err with the raw plan UUID: a non-ClientError propagates + // to the router's logging.Errorf("API error: %v"), leaking the raw DB + // error string (issue #965). Map to 404 on ErrNotFound; else log a + // structured 500 without the DB error or the plan UUID. + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to validate plan", + "getPlanForAccountProviderValidation: GetPurchasePlan failed") } if plan == nil { return nil, NewClientError(404, fmt.Sprintf("plan not found: %s", planID)) diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index 4c63417a1..7c2fe5d88 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -2029,3 +2029,63 @@ func TestValidatePlanAccountProviders_AccountNotFound_Still404(t *testing.T) { assert.Equal(t, 404, ce.code) assert.Contains(t, ce.Error(), missingAcct, "404 must still reference the missing account ID") } + +// TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak is a +// regression test for the sibling leak left unfixed by PR #969: the +// GetPurchasePlan DB-error branch inside getPlanForAccountProviderValidation +// wrapped the raw DB error via fmt.Errorf (a non-ClientError), so the router's +// logging.Errorf("API error: %v") emitted the raw DB error string (issue #965). +// +// Asserts: (1) a 500 ClientError is returned, (2) neither the returned error +// message nor the emitted log contains the raw DB error string, and (3) a +// store-level ErrNotFound still maps to 404 (ErrNotFound is not a DB leak risk +// but the mapping must survive the refactor). +func TestGetPlanForAccountProviderValidation_GetPlanDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const rawDBErr = "pq: SSL connection has been closed unexpectedly by host db-plans-99.example.com" + + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return nil, errors.New(rawDBErr) + } + handler := &Handler{config: store} + + logBuf := captureDefaultLog(t) + + err := handler.validatePlanAccountProviders(ctx, planID209, []string{awsAcct209}) + require.Error(t, err) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error from GetPurchasePlan must be mapped to a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code, "a DB error fetching the plan is a server-side fault") + + // The user-facing message must not contain the raw DB error string. + assert.NotContains(t, ce.Error(), rawDBErr, "500 message must not leak the raw DB error string") + // planID209 is the caller-supplied plan UUID; we do not echo it back in the 500 generic message. + assert.NotContains(t, ce.Error(), planID209, "500 message must not echo the plan UUID") + + // The structured log line emitted by mapCreatePlanStorageError must not + // contain the raw DB error string (it logs only a static format string). + logged := logBuf.String() + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965)") +} + +// TestGetPlanForAccountProviderValidation_PlanNotFound_Still404 guards that the +// no-PII-leak fix for GetPurchasePlan did not regress the ErrNotFound mapping: +// a store-level ErrNotFound must still return a 404 ClientError. +func TestGetPlanForAccountProviderValidation_PlanNotFound_Still404(t *testing.T) { + ctx := context.Background() + + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return nil, config.ErrNotFound + } + handler := &Handler{config: store} + + err := handler.validatePlanAccountProviders(ctx, planID209, []string{awsAcct209}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "ErrNotFound from GetPurchasePlan must map to a ClientError") + assert.Equal(t, 404, ce.code, "ErrNotFound must map to 404") +} From 3c6e0b6c9b8617363060e3806a156efc4ab49bfb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 09:07:32 +0200 Subject: [PATCH 3/4] sec(api): fix sibling PII leaks in plan/purchase handler paths (followup #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 --- internal/api/handler_plans.go | 32 ++++- internal/api/handler_plans_test.go | 116 +++++++++++++++++- internal/api/handler_purchases.go | 30 ++++- internal/api/handler_purchases_test.go | 161 ++++++++++++++++++++++++- 4 files changed, 325 insertions(+), 14 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index eed686d06..43a659303 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -203,10 +203,15 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction return nil, NewClientError(400, "invalid request body") } - // Fetch existing plan to preserve data not sent in request + // Fetch existing plan to preserve data not sent in request. Do NOT wrap + // err with the raw plan UUID: a non-ClientError propagates to the router's + // logging.Errorf("API error: %v"), leaking the plan UUID and the raw DB + // error string (issue #965, same shape as the createPlan-side fix). existingPlan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { - return nil, fmt.Errorf("failed to get plan %s: %w", planID, err) + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to load plan", + "updatePlan: GetPurchasePlan failed") } // Create new plan from request @@ -337,14 +342,23 @@ func (h *Handler) parseCreatePurchasesRequest(body string) (*CreatePlannedPurcha return &req, startDate, nil } -// getPlanForPurchaseCreation retrieves and validates the purchase plan +// getPlanForPurchaseCreation retrieves and validates the purchase plan. +// +// Errors are routed through mapCreatePlanStorageError so a DB failure never +// leaks the raw error string through the router's +// logging.Errorf("API error: %v") path (issue #965, same shape as the +// validatePlanAccountProviders fix). The "plan not found" case (nil, nil) is +// returned as a 404 ClientError; planID is the caller-supplied value, so +// echoing it back in the 404 body is acceptable. func (h *Handler) getPlanForPurchaseCreation(ctx context.Context, planID string) (*config.PurchasePlan, error) { plan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { - return nil, fmt.Errorf("failed to get plan: %w", err) + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to load plan", + "getPlanForPurchaseCreation: GetPurchasePlan failed") } if plan == nil { - return nil, fmt.Errorf("plan not found: %s", planID) + return nil, NewClientError(404, fmt.Sprintf("plan not found: %s", planID)) } return plan, nil } @@ -463,9 +477,15 @@ func (h *Handler) patchPlan(ctx context.Context, httpReq *events.LambdaFunctionU return nil, NewClientError(400, "invalid request body") } + // Do NOT wrap err with the raw plan UUID: a non-ClientError propagates to + // the router's logging.Errorf("API error: %v"), leaking the plan UUID and + // the raw DB error string (issue #965, same shape as the createPlan-side + // fix). plan, err := h.config.GetPurchasePlan(ctx, planID) if err != nil { - return nil, fmt.Errorf("failed to get plan %s: %w", planID, err) + return nil, mapCreatePlanStorageError(err, + "plan not found", "failed to load plan", + "patchPlan: GetPurchasePlan failed") } if plan == nil { return nil, NewClientError(404, fmt.Sprintf("plan not found: %s", planID)) diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 3f7e3e29b..8cf2366f8 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -766,7 +766,14 @@ func TestHandler_createPlannedPurchases_PlanNotFound(t *testing.T) { result, err := handler.createPlannedPurchases(ctx, req, "99999999-9999-9999-9999-999999999999") assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "failed to get plan") + // Post-#965-followup: getPlanForPurchaseCreation routes DB errors through + // mapCreatePlanStorageError, returning a generic 500 ClientError without + // the raw plan UUID or the raw DB error string in the user-facing message. + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), "99999999-9999-9999-9999-999999999999", "500 message must not echo the plan UUID") + assert.NotContains(t, ce.Error(), "plan not found", "500 message must not be the 404 branch (it would mask a real DB outage)") } func TestCalculateNextExecutionDate(t *testing.T) { @@ -979,7 +986,112 @@ func TestHandler_patchPlan_NotFound(t *testing.T) { result, err := handler.patchPlan(ctx, req, "99999999-9999-9999-9999-999999999999") assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "failed to get plan") + // Post-#965-followup: patchPlan routes generic DB errors through + // mapCreatePlanStorageError, returning a generic 500 ClientError without + // the raw plan UUID or the raw DB error string in the user-facing message. + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), "99999999-9999-9999-9999-999999999999", "500 message must not echo the plan UUID") +} + +// TestHandler_patchPlan_GetPlanDBError_NoPIILeak is a regression test for the +// PII-leak sibling of issue #965: the GetPurchasePlan DB-error path in patchPlan +// previously wrapped the raw error with the raw plan UUID +// (`fmt.Errorf("failed to get plan %s: %w", planID, err)`), and that non- +// ClientError propagated to the router's logging.Errorf("API error: %v"), +// leaking both the UUID and the raw DB error string. Asserts that on a DB +// error: (1) a 500 ClientError is returned, (2) neither the returned message +// nor the captured default-log line contains the plan UUID or the raw DB +// error string. +func TestHandler_patchPlan_GetPlanDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + planUUID = "99999999-9999-9999-9999-999999999999" + rawDBErr = "pq: SSL connection has been closed unexpectedly by host db-plans-99.example.com" + ) + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("GetPurchasePlan", ctx, planUUID).Return(nil, errors.New(rawDBErr)) + + handler := &Handler{config: mockStore, auth: mockAuth} + + logBuf := captureDefaultLog(t) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"enabled": true}`, + } + result, err := handler.patchPlan(ctx, req, planUUID) + require.Error(t, err) + assert.Nil(t, result) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code, "a DB error fetching the plan is a server-side fault") + assert.NotContains(t, ce.Error(), planUUID, "500 message must not echo the plan UUID") + assert.NotContains(t, ce.Error(), rawDBErr, "500 message must not leak the raw DB error string") + + logged := logBuf.String() + assert.NotContains(t, logged, planUUID, "log must not contain the raw plan UUID (issue #965 sibling)") + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965 sibling)") +} + +// TestHandler_updatePlan_GetPlanDBError_NoPIILeak guards the same shape on the +// PUT updatePlan path (handler_plans.go:209): the existingPlan-fetch DB error +// path used to wrap with the raw plan UUID. +func TestHandler_updatePlan_GetPlanDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + planUUID = "99999999-9999-9999-9999-999999999999" + rawDBErr = "pq: connection refused to host db-internal-99.example.com" + ) + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("GetPurchasePlan", ctx, planUUID).Return(nil, errors.New(rawDBErr)) + + handler := &Handler{config: mockStore, auth: mockAuth} + + logBuf := captureDefaultLog(t) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + // Minimal valid body; updatePlan validates fields after the fetch. + Body: `{"name":"x","provider":"aws","service":"ec2"}`, + } + result, err := handler.updatePlan(ctx, req, planUUID) + require.Error(t, err) + assert.Nil(t, result) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), planUUID, "500 message must not echo the plan UUID") + assert.NotContains(t, ce.Error(), rawDBErr, "500 message must not leak the raw DB error string") + + logged := logBuf.String() + assert.NotContains(t, logged, planUUID) + assert.NotContains(t, logged, rawDBErr) } func TestHandler_patchPlan_NilPlan(t *testing.T) { diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 26d789d65..b6d130759 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -404,12 +404,22 @@ func (h *Handler) disablePlan(ctx context.Context, planID string) error { if errors.Is(err, config.ErrNotFound) { return NewClientError(404, fmt.Sprintf("disable plan: plan %s not found", planID)) } - return fmt.Errorf("disable plan: failed to fetch plan %s: %w", planID, err) + // Do NOT wrap err with the raw plan UUID: a non-ClientError propagates + // to the router's logging.Errorf("API error: %v"), leaking the plan + // UUID and the raw DB error string (issue #965, same shape as the + // validatePlanAccountProviders fix). + return mapCreatePlanStorageError(err, + "plan not found", "failed to disable plan", + "disablePlan: GetPurchasePlan failed") } if plan.Enabled { plan.Enabled = false if err := h.config.UpdatePurchasePlan(ctx, plan); err != nil { - return fmt.Errorf("disable plan: failed to update plan %s: %w", planID, err) + // Same rationale: avoid leaking planID + raw DB error through the + // router's API error log. + return mapCreatePlanStorageError(err, + "plan not found", "failed to disable plan", + "disablePlan: UpdatePurchasePlan failed") } } return nil @@ -2050,7 +2060,14 @@ func (h *Handler) gatherAccountContactEmails(ctx context.Context, recs []config. for _, id := range orderedIDs { addr, err := h.lookupContactEmail(ctx, id) if err != nil { - return nil, fmt.Errorf("lookup contact email for account %s: %w", id, err) + // Do NOT wrap err with the raw account UUID: this error is + // returned up to the caller which surfaces it through the + // router's logging.Errorf("API error: %v") path, leaking the + // account UUID and the raw DB error string (issue #965, same + // shape as the validatePlanAccountProviders fix). Return a + // generic 500 ClientError; lookupContactEmail has already logged + // the failure point without PII. + return nil, NewClientError(500, "failed to resolve contact emails") } if addr == "" { continue @@ -2101,7 +2118,12 @@ func uniqueAccountIDsFromRecs(recs []config.RecommendationRecord) []string { func (h *Handler) lookupContactEmail(ctx context.Context, id string) (string, error) { acct, err := h.config.GetCloudAccount(ctx, id) if err != nil { - logging.Warnf("lookupContactEmail: GetCloudAccount(%s) failed: %v", id, err) + // Do NOT log id or err verbatim: id is the raw account UUID and err + // can be a raw DB error string (issue #965, same shape as the + // validatePlanAccountProviders fix). The caller wraps this into a + // ClientError that reaches the router's logging.Errorf("API error: + // %v"); leaking either field here would propagate through that path. + logging.Warnf("lookupContactEmail: GetCloudAccount failed") return "", err } if acct == nil { diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index b68634c37..a5b32fd93 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -699,6 +699,13 @@ func TestHandler_resolveApprovalRecipients_NoContactEmail(t *testing.T) { // Instead, the lookup error propagates to the caller, which surfaces // it as a retriable failure so the operator's next attempt sees the // real approver list. +// +// Post-#965-followup: the underlying transient is intentionally NOT exposed +// in the returned error chain — gatherAccountContactEmails maps DB failures +// to a generic 500 ClientError so the router's `API error: %v` log cannot +// leak the raw account UUID or DB error string. Equivalence of "propagate as +// a server-side failure" is asserted via the 500 ClientError code, not via +// errors.Is on the underlying transient. func TestHandler_resolveApprovalRecipients_LookupErrorPropagates(t *testing.T) { ctx := context.Background() globalNotify := "global@cudly.example" @@ -715,8 +722,14 @@ func TestHandler_resolveApprovalRecipients_LookupErrorPropagates(t *testing.T) { {ID: "r1", CloudAccountID: &accountID}, } to, cc, approvers, err := h.resolveApprovalRecipients(ctx, recs, globalNotify) - require.Error(t, err, "transient lookup error must propagate") - assert.ErrorIs(t, err, transient, "wrapped error chain must preserve the underlying cause") + require.Error(t, err, "transient lookup error must propagate (no silent globalNotify fallback)") + ce, ok := IsClientError(err) + require.True(t, ok, "lookup error must surface as a ClientError so the router does not log the raw DB cause") + assert.Equal(t, 500, ce.code, "transient DB failure is a server-side fault, not a client fault") + // PII-leak guards: neither the raw account ID nor the underlying transient + // error message must appear in the surfaced error. + assert.NotContains(t, err.Error(), accountID) + assert.NotContains(t, err.Error(), transient.Error()) assert.Empty(t, to) assert.Nil(t, cc) assert.Nil(t, approvers) @@ -3600,3 +3613,147 @@ func TestHandler_authorizeExecutionManagement_LegacyNullCreator(t *testing.T) { require.True(t, ok) assert.Equal(t, 403, ce.code) } + +// TestDisablePlan_GetPlanDBError_NoPIILeak is a regression test for the +// PII-leak sibling of issue #965: disablePlan previously wrapped its +// GetPurchasePlan and UpdatePurchasePlan failures with the raw plan UUID +// (`fmt.Errorf("disable plan: failed to ... %s: %w", planID, err)`), and that +// non-ClientError propagated to the router's logging.Errorf("API error: %v"), +// leaking both the UUID and the raw DB error string. Asserts: 500 ClientError; +// neither the returned message nor the captured default-log line contains the +// plan UUID or the raw DB error string. +func TestDisablePlan_GetPlanDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + planUUID = "99999999-9999-9999-9999-999999999999" + rawDBErr = "pq: SSL connection has been closed unexpectedly by host db-plans-99.example.com" + ) + + mockStore := new(MockConfigStore) + mockStore.On("GetPurchasePlan", ctx, planUUID).Return(nil, errors.New(rawDBErr)) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + + logBuf := captureDefaultLog(t) + + err := handler.disablePlan(ctx, planUUID) + require.Error(t, err) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), planUUID, "500 message must not echo the plan UUID") + assert.NotContains(t, ce.Error(), rawDBErr, "500 message must not leak the raw DB error string") + + logged := logBuf.String() + assert.NotContains(t, logged, planUUID, "log must not contain the raw plan UUID (issue #965 sibling)") + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965 sibling)") +} + +// TestDisablePlan_UpdatePlanDBError_NoPIILeak guards the same shape on the +// UpdatePurchasePlan failure path (the second leak site). +func TestDisablePlan_UpdatePlanDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + planUUID = "99999999-9999-9999-9999-999999999999" + rawDBErr = "pq: deadlock detected on relation plans" + ) + + enabledPlan := &config.PurchasePlan{ID: planUUID, Enabled: true} + + mockStore := new(MockConfigStore) + mockStore.On("GetPurchasePlan", ctx, planUUID).Return(enabledPlan, nil) + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(errors.New(rawDBErr)) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + + logBuf := captureDefaultLog(t) + + err := handler.disablePlan(ctx, planUUID) + require.Error(t, err) + + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError, not propagate raw to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), planUUID) + assert.NotContains(t, ce.Error(), rawDBErr) + + logged := logBuf.String() + assert.NotContains(t, logged, planUUID) + assert.NotContains(t, logged, rawDBErr) +} + +// TestLookupContactEmail_GetAccountDBError_NoPIILeak guards that the +// GetCloudAccount failure path in lookupContactEmail no longer logs the raw +// account UUID or the raw DB error string (issue #965 sibling at +// handler_purchases.go:2006). +func TestLookupContactEmail_GetAccountDBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + acctUUID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + rawDBErr = "pq: connection refused to host db-internal-99.example.com" + ) + + mockStore := new(MockConfigStore) + mockStore.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return nil, errors.New(rawDBErr) + } + + handler := &Handler{config: mockStore} + + logBuf := captureDefaultLog(t) + + addr, err := handler.lookupContactEmail(ctx, acctUUID) + require.Error(t, err) + assert.Empty(t, addr) + + logged := logBuf.String() + assert.NotContains(t, logged, acctUUID, "log must not contain the raw account UUID (issue #965 sibling)") + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965 sibling)") +} + +// TestGatherAccountContactEmails_DBError_NoPIILeak guards the wrap site at +// gatherAccountContactEmails (handler_purchases.go:1955) which previously +// re-wrapped lookupContactEmail's error with the raw account UUID +// (`fmt.Errorf("lookup contact email for account %s: %w", id, err)`) and +// propagated it up to the router's `API error: %v` log. +func TestGatherAccountContactEmails_DBError_NoPIILeak(t *testing.T) { + ctx := context.Background() + + const ( + acctUUID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + rawDBErr = "pq: connection refused to host db-internal-99.example.com" + ) + + mockStore := new(MockConfigStore) + mockStore.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return nil, errors.New(rawDBErr) + } + + handler := &Handler{config: mockStore} + + logBuf := captureDefaultLog(t) + + id := acctUUID + recs := []config.RecommendationRecord{{CloudAccountID: &id}} + emails, err := handler.gatherAccountContactEmails(ctx, recs) + require.Error(t, err) + assert.Nil(t, emails) + + // The returned error must be a ClientError so it does not propagate the + // raw account UUID or the raw DB error through the router's API-error log. + ce, ok := IsClientError(err) + require.True(t, ok, "DB error must surface as a ClientError; raw error must not propagate to the router log") + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), acctUUID, "ClientError message must not echo the account UUID") + assert.NotContains(t, ce.Error(), rawDBErr, "ClientError message must not leak the raw DB error string") + + logged := logBuf.String() + assert.NotContains(t, logged, acctUUID, "log must not contain the raw account UUID (issue #965 sibling)") + assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965 sibling)") +} From b409abfd28faef4a1faa89dfc4ed33f9abf95eb8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 21:49:08 -0700 Subject: [PATCH 4/4] sec(api/purchases): keep lookupContactEmail failure at error level 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. --- internal/api/handler_purchases.go | 13 +++++++++---- internal/api/handler_purchases_test.go | 6 ++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index b6d130759..4c2e37e56 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2120,10 +2120,15 @@ func (h *Handler) lookupContactEmail(ctx context.Context, id string) (string, er if err != nil { // Do NOT log id or err verbatim: id is the raw account UUID and err // can be a raw DB error string (issue #965, same shape as the - // validatePlanAccountProviders fix). The caller wraps this into a - // ClientError that reaches the router's logging.Errorf("API error: - // %v"); leaking either field here would propagate through that path. - logging.Warnf("lookupContactEmail: GetCloudAccount failed") + // validatePlanAccountProviders fix). Keep the message generic. + // + // Log at error level: the caller (gatherAccountContactEmails) maps + // this into a generic 500 ClientError, which the router emits via the + // non-error-logging ClientError branch (handler.go: it returns the + // ce.code/body before reaching logging.Errorf("API error: %v")). This + // line is therefore the only error-level breadcrumb for the resulting + // user-visible 500, so it must stay at Errorf to preserve alerting. + logging.Errorf("lookupContactEmail: GetCloudAccount failed") return "", err } if acct == nil { diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index a5b32fd93..aed754d00 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3715,6 +3715,12 @@ func TestLookupContactEmail_GetAccountDBError_NoPIILeak(t *testing.T) { logged := logBuf.String() assert.NotContains(t, logged, acctUUID, "log must not contain the raw account UUID (issue #965 sibling)") assert.NotContains(t, logged, rawDBErr, "log must not contain the raw DB error string (issue #965 sibling)") + // The caller maps this failure into a generic 500 ClientError, which the + // router returns before reaching logging.Errorf("API error: %v"). This is + // therefore the only error-level breadcrumb for the user-visible 500, so it + // must be emitted at ERROR severity to preserve alerting (CR PR #969). + assert.Contains(t, logged, "[ERROR]", "GetCloudAccount failure must log at error level so the user-visible 500 stays alertable") + assert.Contains(t, logged, "lookupContactEmail: GetCloudAccount failed", "the generic breadcrumb message must be present") } // TestGatherAccountContactEmails_DBError_NoPIILeak guards the wrap site at