sec(api/plans): stop leaking raw account UUID + DB error in plan-account validation log

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))

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")
+}

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))

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) {