Add converter round-trip coverage and sanitize cleanup log

Addresses #5196 review comments:
- MEDIUM dcr_store.go (3203565436) F7: added
  TestResolutionCredentialsRoundTrip pinning the field-by-field contract
  between resolutionToCredentials and credentialsToResolution
  (preserved, dropped, key-recovered, nil-shortcircuit). Added
  MUST-update-both-converters comments on the DCRResolution struct in
  dcr.go and the DCRCredentials struct in storage/types.go so a future
  contributor adding a field to either type sees the converter
  obligation at the struct definition rather than only at the
  converters. Documented the ProviderName asymmetry: the field is
  storage-only ("debug/audit only" per its own docstring) and is
  intentionally left unpopulated by the runner; the test asserts that
  invariant so any future threading is paired with the assertion
  update.

- MEDIUM embeddedauthserver.go (3203565446) F8: route both closeErr and
  retErr in the deferred-cleanup slog.Warn through sanitizeErrorForLog
  so a wrapped DCR failure whose error chain inlines an upstream
  /register response body cannot leak userinfo, query, or fragment
  components into operator logs. Renamed the "original_error" key to
  "cause" to match the package-wide vocabulary. Added
  TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog which captures
  the Warn record by swapping slog.Default() and asserts both that
  literal secret markers are scrubbed and that host components survive
  for operator correlation.

Author: tgrunnagle

pkg/authserver/runner/dcr.go

@@ -73,6 +73,16 @@ var authMethodPreference = []string{
 //
 // The struct is the unit of storage in dcrResolutionCache and the unit of
 // application via consumeResolution.
+//
+// MUST update both converters (resolutionToCredentials and
+// credentialsToResolution in dcr_store.go) when adding, renaming, or
+// removing a field here. The two converters are the seam between this
+// runner-side type and the persisted *storage.DCRCredentials shape; a
+// field added here without a paired converter update will silently fail
+// to round-trip across an authserver restart, the exact "parallel types
+// drift" failure mode .claude/rules/go-style.md warns about. The
+// round-trip behaviour is pinned by TestResolutionCredentialsRoundTrip
+// in dcr_store_test.go.
 type DCRResolution struct {
 	// ClientID is the RFC 7591 "client_id" returned by the authorization
 	// server.

pkg/authserver/runner/dcr_store_test.go

@@ -298,3 +298,104 @@ func TestStorageBackedStore_ConcurrentAccess(t *testing.T) {
 	assert.Zero(t, atomic.LoadInt32(&errCount),
 		"no Get/Put should have errored under concurrent access")
 }
+
+// TestResolutionCredentialsRoundTrip pins the field-by-field contract
+// between resolutionToCredentials and credentialsToResolution: which
+// fields survive a round-trip, which are intentionally dropped, and
+// which are recovered from the persisted DCRKey. The test exists
+// because the two converters are the seam where a field added to either
+// DCRResolution or storage.DCRCredentials must be paired with an update
+// here; without coverage, a future field addition would silently fail
+// to persist across an authserver restart.
+//
+// The "preserved" group asserts equality on round-tripped values. The
+// "dropped" group asserts that the post-round-trip value is the type's
+// zero value (ClientIDIssuedAt is informational per RFC 7591 §3.2.1 and
+// is not persisted). RedirectURI is in its own group because it is
+// dropped from DCRCredentials and recovered via Key.RedirectURI on
+// read.
+//
+// ProviderName is the one DCRCredentials field with no DCRResolution
+// counterpart. It is documented in storage.DCRCredentials as "debug /
+// audit only — never used as a primary key" and no current consumer
+// reads it. The decision to leave it unpopulated by the runner is
+// recorded here so a future contributor adding ProviderName threading
+// has a single grep target.
+func TestResolutionCredentialsRoundTrip(t *testing.T) {
+	t.Parallel()
+
+	now := time.Now().UTC().Round(time.Second)
+	expiry := now.Add(30 * 24 * time.Hour)
+
+	key := DCRKey{
+		Issuer:      "https://idp.example.com",
+		RedirectURI: "https://thv.example.com/oauth/callback",
+		ScopesHash:  storage.ScopesHash([]string{"openid", "profile"}),
+	}
+
+	original := &DCRResolution{
+		ClientID:                "round-trip-client-id",
+		ClientSecret:            "round-trip-secret",
+		AuthorizationEndpoint:   "https://idp.example.com/authorize",
+		TokenEndpoint:           "https://idp.example.com/token",
+		RegistrationAccessToken: "rfc7592-token",
+		RegistrationClientURI:   "https://idp.example.com/register/round-trip-client-id",
+		TokenEndpointAuthMethod: "client_secret_basic",
+		// RedirectURI is recovered from Key on read; pre-populate it on
+		// the input to confirm it survives via the key, not via a
+		// dedicated field on DCRCredentials.
+		RedirectURI:           key.RedirectURI,
+		ClientIDIssuedAt:      now, // intentionally dropped on persist
+		ClientSecretExpiresAt: expiry,
+		CreatedAt:             now,
+	}
+
+	creds := resolutionToCredentials(key, original)
+	require.NotNil(t, creds)
+
+	// Persisted-side assertions: which fields the converter writes to
+	// DCRCredentials, which it leaves zero (the asymmetry F7 documents).
+	assert.Equal(t, key, creds.Key,
+		"Key must round-trip via the explicit parameter")
+	assert.Empty(t, creds.ProviderName,
+		"ProviderName has no DCRResolution counterpart and is intentionally not populated; "+
+			"a future contributor threading it through must update this assertion and the converters together")
+
+	roundTripped := credentialsToResolution(creds)
+	require.NotNil(t, roundTripped)
+
+	t.Run("preserved fields survive round-trip", func(t *testing.T) {
+		t.Parallel()
+		assert.Equal(t, original.ClientID, roundTripped.ClientID)
+		assert.Equal(t, original.ClientSecret, roundTripped.ClientSecret)
+		assert.Equal(t, original.AuthorizationEndpoint, roundTripped.AuthorizationEndpoint)
+		assert.Equal(t, original.TokenEndpoint, roundTripped.TokenEndpoint)
+		assert.Equal(t, original.RegistrationAccessToken, roundTripped.RegistrationAccessToken)
+		assert.Equal(t, original.RegistrationClientURI, roundTripped.RegistrationClientURI)
+		assert.Equal(t, original.TokenEndpointAuthMethod, roundTripped.TokenEndpointAuthMethod)
+		assert.Equal(t, original.ClientSecretExpiresAt, roundTripped.ClientSecretExpiresAt)
+		assert.Equal(t, original.CreatedAt, roundTripped.CreatedAt)
+	})
+
+	t.Run("RedirectURI is recovered from Key on read", func(t *testing.T) {
+		t.Parallel()
+		assert.Equal(t, key.RedirectURI, roundTripped.RedirectURI,
+			"RedirectURI on the round-tripped resolution must match Key.RedirectURI; "+
+				"the converter does not store it twice")
+	})
+
+	t.Run("dropped fields zero on read", func(t *testing.T) {
+		t.Parallel()
+		// ClientIDIssuedAt is informational (RFC 7591 §3.2.1) and not
+		// persisted; round-tripping must reset it to the zero value
+		// rather than silently re-deriving it from CreatedAt.
+		assert.True(t, roundTripped.ClientIDIssuedAt.IsZero(),
+			"ClientIDIssuedAt must be zero after round-trip; the field is intentionally dropped from DCRCredentials")
+	})
+
+	t.Run("nil inputs short-circuit", func(t *testing.T) {
+		t.Parallel()
+		assert.Nil(t, resolutionToCredentials(key, nil))
+		assert.Nil(t, credentialsToResolution(nil))
+	})
+}

pkg/authserver/runner/embeddedauthserver.go

@@ -93,12 +93,24 @@ func newEmbeddedAuthServerWithStorage(
 	stor storage.Storage,
 ) (retEAS *EmbeddedAuthServer, retErr error) {
 	// From here on, any error must close stor before returning.
+	//
+	// Both errors are passed through sanitizeErrorForLog before being
+	// recorded: closeErr for symmetry with retErr, retErr because the
+	// most common cause of reaching this gate is a wrapped DCR failure
+	// whose error chain may inline several KiB of the upstream's raw
+	// /register response body — that body is attacker-influenced and may
+	// contain URL components that carry credentials (userinfo, query,
+	// fragment). The existing logDCRStepError boundary log routes
+	// through the same sanitiser; keep the two log paths consistent so
+	// the cleanup log cannot regress to a less-defended state. The
+	// "cause" key matches the package-wide vocabulary for the
+	// triggering error.
 	defer func() {
 		if retErr != nil {
 			if closeErr := stor.Close(); closeErr != nil {
 				slog.Warn("failed to close storage on NewEmbeddedAuthServer error path",
-					"error", closeErr,
-					"original_error", retErr,
+					"error", sanitizeErrorForLog(closeErr),
+					"cause", sanitizeErrorForLog(retErr),
 				)
 			}
 		}

pkg/authserver/runner/embeddedauthserver_test.go

@@ -4,6 +4,7 @@
 package runner
 
 import (
+	"bytes"
 	"context"
 	"crypto/ecdsa"
 	"crypto/elliptic"
@@ -13,6 +14,7 @@ import (
 	"encoding/pem"
 	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -1827,3 +1829,120 @@ func TestNewEmbeddedAuthServer_ClosesStorageOnError(t *testing.T) {
 		"failed NewEmbeddedAuthServer must Close the storage exactly once via the deferred-cleanup gate; "+
 			"a count of 0 indicates the deferred Close did not run, leaking the backend on the error path")
 }
+
+// urlErrorOnCloseStorage wraps an authserver storage and returns a fixed
+// URL-bearing error from Close. It exists so
+// TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog can verify that the
+// deferred-cleanup gate routes both closeErr and retErr through
+// sanitizeErrorForLog, scrubbing any query / userinfo / fragment that might
+// carry credentials in a future regression.
+type urlErrorOnCloseStorage struct {
+	storage.Storage
+	closeErr error
+}
+
+func (s *urlErrorOnCloseStorage) Close() error {
+	// Intentionally drop the inner Close result: this test is about the log
+	// path, not about double-closing the inner storage. Returning the
+	// fixed error makes the slog capture deterministic.
+	_ = s.Storage.Close()
+	return s.closeErr
+}
+
+// TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog pins the post-#5196
+// invariant that the deferred-cleanup slog.Warn at the top of
+// newEmbeddedAuthServerWithStorage routes both closeErr and retErr through
+// sanitizeErrorForLog, so a future regression that drops the call (or that
+// changes the error chain to inline an upstream response body containing a
+// userinfo/query/fragment) cannot silently leak secrets to operator logs.
+//
+// The test injects a closeErr containing a URL with a secret-bearing query
+// (?token=leak-marker) and a discovery URL whose host appears verbatim in
+// the wrapped DCR error chain (so the captured slog record's `cause` field
+// also exercises the sanitiser). It then asserts:
+//   - the captured log record does NOT contain the literal secret marker;
+//   - the captured log record DOES contain the host components, so
+//     operators retain enough context to correlate the failure.
+//
+// NOT t.Parallel(): the test swaps slog.Default() to capture output and
+// restores it via t.Cleanup. Running in parallel would race with any other
+// test in this package that emits a log record. Confirmed against the
+// paralleltest rule on a sample run — every other test failed with a
+// data-race report on slog's internal default-logger handle.
+//
+//nolint:paralleltest // see comment above; mutates the package-global slog.Default()
+func TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog(t *testing.T) {
+	const (
+		closeErrSecretMarker = "close-leak-marker-7f9c"
+		retErrSecretMarker   = "ret-leak-marker-3b2a"
+	)
+
+	closeErr := fmt.Errorf(
+		"redis: connection broken: https://primary:hidden@redis.example.com/0?token=%s",
+		closeErrSecretMarker,
+	)
+	tracker := &urlErrorOnCloseStorage{
+		Storage:  storage.NewMemoryStorage(),
+		closeErr: closeErr,
+	}
+
+	// Discovery endpoint returns 500 so the DCR resolver fails and the
+	// constructor reaches the deferred-cleanup gate. The query string
+	// embedded in the discovery URL is what the resolver wraps into its
+	// error chain via fmt.Errorf("...%w", err) — so retErr's text is the
+	// vehicle for the second secret marker.
+	httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+	t.Cleanup(httpServer.Close)
+
+	discoveryURL := httpServer.URL + "/.well-known/oauth-authorization-server?token=" + retErrSecretMarker
+
+	cfg := &authserver.RunConfig{
+		SchemaVersion: authserver.CurrentSchemaVersion,
+		Issuer:        httpServer.URL,
+		Upstreams: []authserver.UpstreamRunConfig{
+			{
+				Name: "dcr-upstream",
+				Type: authserver.UpstreamProviderTypeOAuth2,
+				OAuth2Config: &authserver.OAuth2UpstreamRunConfig{
+					ClientID:              "",
+					AuthorizationEndpoint: httpServer.URL + "/authorize",
+					TokenEndpoint:         httpServer.URL + "/token",
+					Scopes:                []string{"openid", "profile"},
+					DCRConfig: &authserver.DCRUpstreamConfig{
+						DiscoveryURL: discoveryURL,
+					},
+				},
+			},
+		},
+		AllowedAudiences: []string{"https://mcp.example.com"},
+	}
+
+	// Capture slog output by swapping the default logger for the duration
+	// of this test. Restore on cleanup so parallel tests are unaffected.
+	var buf bytes.Buffer
+	prev := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))
+	t.Cleanup(func() { slog.SetDefault(prev) })
+
+	embed, err := newEmbeddedAuthServerWithStorage(context.Background(), cfg, tracker)
+	require.Error(t, err)
+	assert.Nil(t, embed)
+
+	logged := buf.String()
+
+	// Defense in depth: both secret markers must be stripped before the
+	// Warn record reaches operator logs.
+	assert.NotContains(t, logged, closeErrSecretMarker,
+		"closeErr secret marker must be sanitised before reaching the Warn record")
+	assert.NotContains(t, logged, retErrSecretMarker,
+		"retErr secret marker must be sanitised before reaching the Warn record")
+	assert.NotContains(t, logged, "primary:hidden",
+		"closeErr userinfo must be sanitised before reaching the Warn record")
+
+	// The host components survive sanitisation so operators retain enough
+	// context to correlate the failure with upstream logs.
+	assert.Contains(t, logged, "redis.example.com",
+		"closeErr host must remain in the Warn record after sanitisation")
+}

pkg/authserver/storage/types.go

@@ -229,6 +229,18 @@ func validateDCRCredentialsForStore(creds *DCRCredentials) error {
 // retains entries for the process lifetime and is intentionally excluded from
 // the periodic cleanup loop. The Redis backend (future sub-issue) applies
 // TTL via SetEX when ClientSecretExpiresAt is non-zero.
+//
+// # Converter contract
+//
+// MUST update both converters in
+// pkg/authserver/runner/dcr_store.go (resolutionToCredentials and
+// credentialsToResolution) when adding, renaming, or removing a field
+// here. The two converters are the only translation seam between this
+// persisted type and the runner-side *DCRResolution; a field added here
+// without a paired converter update will silently fail to round-trip
+// across an authserver restart. The round-trip behaviour is pinned by
+// TestResolutionCredentialsRoundTrip in
+// pkg/authserver/runner/dcr_store_test.go.
 type DCRCredentials struct {
 	// Key is the canonical cache key: (Issuer, RedirectURI, ScopesHash).
 	Key DCRKey