diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 4f151dfb3..095e81cdb 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)) @@ -1254,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 e3fdab2ed..7c2fe5d88 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,146 @@ 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") +} + +// 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") +} 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..4c2e37e56 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,17 @@ 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). 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 b68634c37..aed754d00 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,153 @@ 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)") + // 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 +// 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)") +} 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 +}