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

Author: cristim

internal/api/handler_accounts.go

@@ -1227,7 +1227,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))

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"
@@ -1915,3 +1917,86 @@ func (m *mockConfigStoreAccounts) SavePurchaseExecutionTx(ctx context.Context, _
 func (m *mockConfigStoreAccounts) WithTx(_ context.Context, fn func(tx pgx.Tx) error) error {
 	return fn(nil)
 }
+
+// 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")
+}

pkg/logging/logger.go

@@ -30,6 +30,7 @@ const (
 type Logger struct {
 	level    Level
 	logger   *log.Logger
+	output   io.Writer
 	prefix   string
 	metadata map[string]interface{}
 }
@@ -82,6 +83,7 @@ func New(cfg Config) *Logger {
 	return &Logger{
 		level:    ParseLevel(cfg.Level),
 		logger:   log.New(output, cfg.Prefix, flags),
+		output:   output,
 		prefix:   cfg.Prefix,
 		metadata: make(map[string]interface{}),
 	}
@@ -107,6 +109,7 @@ func (l *Logger) With(key string, value interface{}) *Logger {
 	newLogger := &Logger{
 		level:    l.level,
 		logger:   l.logger,
+		output:   l.output,
 		prefix:   l.prefix,
 		metadata: make(map[string]interface{}),
 	}
@@ -243,3 +246,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
+}