Address code review feedback

Fixed issues from review of df84256 (Extract DCR resolver into
pkg/auth/dcr) for issue #5145 sub-issue 4a:

- HIGH: Stale identifier references in buildUpstreamConfigs doc comment
  in pkg/authserver/runner/embeddedauthserver.go named the pre-rename
  symbols (consumeResolution, applyResolutionToOAuth2Config,
  resolveDCRCredentials, logDCRStepError). Updated the comment to use
  the new public names (dcr.ConsumeResolution,
  dcr.ApplyResolutionToOAuth2Config, dcr.ResolveCredentials,
  dcr.LogStepError). Re-grepped the runner package; no other survivors.

- HIGH: pkg/auth/dcr's package doc claimed "profile-agnostic" while the
  public API takes embedded-authserver types directly. Revised the
  package doc comment in pkg/auth/dcr/resolver.go to call out the
  current coupling explicitly and labelled the profile-agnostic framing
  as a target state for sub-issue 4b rather than the current API shape.
  Designing a profile-neutral input type with only one consumer in hand
  would be speculative; the right design moment is when 4b lands the
  CLI flow as the second consumer.

- MEDIUM: DCRStore() accessor doc comment in
  pkg/authserver/runner/embeddedauthserver.go referenced the dropped
  "runner-DCRCredentialStore adapter" name. Replaced with
  "dcr.CredentialStore adapter".

- MEDIUM: resolveSecret is duplicated across pkg/authserver/runner
  and pkg/auth/dcr. Added a parallel TestResolveSecret /
  TestResolveSecretWithEnvVar suite in pkg/auth/dcr/secret_test.go
  that mirrors the runner-package twin's observable contract, so any
  future drift between the two copies will fail one of the two tests
  at CI time. Lifting into a shared helper is deferred until 4b lands
  a third call site.

- MEDIUM: Process-global singleflight was undocumented at the package
  surface. Added a "Concurrency" section to the package doc comment
  in pkg/auth/dcr/resolver.go. Extended the dcrFlight var doc with a
  cross-consumer caveat that names the "third consumer with colliding
  redirect URI" failure mode. Extracted the flight-key construction
  into a new flightKeyOf(Key) string helper so the canonical key shape
  is inspectable rather than buried in a string-concatenation literal.

- MEDIUM: endpointsFromMetadata dereferenced metadata without a
  defensive nil check. Added the guard immediately after the fetchErr
  check in pkg/auth/dcr/resolver.go::endpointsFromMetadata with an
  error message that names the cross-package contract being defended.
  Documented the oauthproto contract on the function's doc comment so
  the assumption is visible at the call site.

Verified via task lint-fix (0 issues), task license-check (clean),
and go test -count=1 -race ./pkg/auth/dcr/... ./pkg/authserver/...
(all pass).

Author: tgrunnagle

pkg/auth/dcr/resolver.go

@@ -11,9 +11,33 @@
 // the registration body. Stateless RFC 7591 wire-shape primitives live in
 // pkg/oauthproto.
 //
-// Profile differences (public PKCE vs confidential client, redirect URI
-// policy, persistence backend) stay at the call site — the package is
-// profile-agnostic and lets each consumer plug a CredentialStore in.
+// # Concurrency
+//
+// The package maintains a process-global singleflight keyed on the tuple
+// (issuer, redirectURI, scopesHash) so concurrent ResolveCredentials calls
+// across all consumers in a single process coalesce when their cache keys
+// match. Consumers that share any of those three values will share a flight
+// — the deduplication is a feature for the embedded authserver but means
+// callers cannot assume per-call-site flight isolation. See the dcrFlight
+// doc comment in resolver.go for the rationale.
+//
+// # Current API coupling — sub-issue 4a only
+//
+// As of issue #5145 sub-issue 4a (the slice that lifted this code out of
+// pkg/authserver/runner), the public functions on this package take
+// embedded-authserver types — *authserver.OAuth2UpstreamRunConfig,
+// *authserver.DCRUpstreamConfig, *upstream.OAuth2Config — directly on
+// their signatures. This matches the embedded authserver's existing
+// internal shapes verbatim and was the cheapest move-only change.
+//
+// The CLI flow migration in sub-issue 4b will introduce the second
+// consumer (pkg/auth/discovery::PerformOAuthFlow) and is the right
+// trigger for replacing those parameters with a profile-neutral input
+// type — designing the neutral shape now, with only one consumer in
+// hand, would be speculative. Until 4b lands, callers outside the
+// embedded authserver MUST adapt their inputs to the authserver types
+// at the call site, and the "profile-agnostic" framing in this package's
+// charter is a target state, not the current state of the API.
 //
 // See issue #5145 for the design discussion that motivated lifting this out
 // of pkg/authserver/runner.
@@ -62,8 +86,30 @@ import (
 // decides whether the resolution is fresh enough to reuse. A future
 // Redis-backed store would still want this in-process gate so a single
 // replica does not double-register against itself.
+//
+// Cross-consumer caveat (matters once issue #5145 sub-issue 4b lands the
+// CLI flow as the second consumer): because dcrFlight is package-global,
+// two consumers that happen to construct identical Keys (same issuer, same
+// redirect URI, same scopes hash) will share a single in-flight
+// registration even if they semantically want different client profiles.
+// The current call sites do not collide — the embedded authserver's
+// redirect URI lives on the AS origin, the CLI flow's lives on a
+// loopback — but a future consumer that defaults its redirect URI into
+// either of those spaces would silently coalesce. Keep this in mind when
+// adding a third consumer.
 var dcrFlight singleflight.Group
 
+// flightKeyOf canonicalises a Key into the singleflight string used by
+// dcrFlight. The "\n" separator is safe because newline is not a valid byte
+// in any of the three components: OAuth scope tokens (RFC 6749 §3.3), URI
+// reference characters (RFC 3986 §2), or hex digests (the form
+// storage.ScopesHash always emits). Exposed as a function so tests and
+// future inspection helpers can compute the exact key the resolver would
+// route through dcrFlight without re-implementing the concatenation.
+func flightKeyOf(key Key) string {
+	return key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash
+}
+
 // defaultUpstreamRedirectPath is the redirect path derived from the issuer
 // origin when the caller's run-config does not supply an explicit RedirectURI.
 // Matches the authserver's public callback route.
@@ -369,9 +415,9 @@ func ResolveCredentials(
 
 	// Coalesce concurrent registrations for the same Key — see dcrFlight
 	// doc comment. The leader runs the registerOnce closure; followers
-	// receive the leader's *Resolution result. The flight key embeds the
-	// Key fields with a separator that cannot appear in any of them
-	// (newline is not valid in OAuth scope tokens, URLs, or hex digests).
+	// receive the leader's *Resolution result. flightKeyOf canonicalises the
+	// Key into a singleflight string with a separator that cannot appear in
+	// any of the three Key components.
 	//
 	// A defer/recover inside the closure converts a panic in registerAndCache
 	// (or anything it calls) into a normal error. Without this, singleflight
@@ -381,8 +427,7 @@ func ResolveCredentials(
 	// dcrStepError and surfaces in the single boundary log emitted by
 	// LogStepError, so the failure produces exactly one Error record (no
 	// in-defer log here) and callers can react to it as a normal failure.
-	flightKey := key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash
-	resolutionAny, err, _ := dcrFlight.Do(flightKey, func() (res any, err error) {
+	resolutionAny, err, _ := dcrFlight.Do(flightKeyOf(key), func() (res any, err error) {
 		defer func() {
 			if r := recover(); r != nil {
 				stepErr := newDCRStepError(dcrStepRegister, localIssuer, redirectURI,
@@ -948,6 +993,15 @@ func deriveExpectedIssuerFromDiscoveryURL(discoveryURL string) (string, error) {
 // document — possible if TLS to the metadata host is compromised, or if the
 // upstream is misconfigured — could otherwise plant http:// URLs that flow
 // through to the authorization-code and token-exchange call paths.
+//
+// Contract with oauthproto: FetchAuthorizationServerMetadata* guarantees a
+// non-nil *AuthorizationServerMetadata whenever fetchErr is nil OR
+// fetchErr is ErrRegistrationEndpointMissing (in the latter case the
+// metadata is otherwise valid; only registration_endpoint is missing).
+// The defensive nil guard below catches a future cross-package contract
+// regression — e.g., a new oauthproto sentinel that returns nil metadata
+// alongside a non-fatal error — and converts it into a clean validation
+// error rather than a nil-pointer dereference at the field accesses.
 func endpointsFromMetadata(
 	metadata *oauthproto.AuthorizationServerMetadata,
 	fetchErr error,
@@ -956,6 +1010,11 @@ func endpointsFromMetadata(
 	if fetchErr != nil && !errors.Is(fetchErr, oauthproto.ErrRegistrationEndpointMissing) {
 		return nil, fmt.Errorf("discover authorization server metadata: %w", fetchErr)
 	}
+	if metadata == nil {
+		return nil, fmt.Errorf(
+			"dcr: authorization server metadata is nil (oauthproto contract " +
+				"violation: nil metadata returned alongside a non-fatal fetch error)")
+	}
 
 	if err := validateUpstreamEndpointURL(metadata.AuthorizationEndpoint, "authorization_endpoint"); err != nil {
 		return nil, fmt.Errorf("dcr: discovered %w", err)

pkg/auth/dcr/secret_test.go

@@ -0,0 +1,118 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package dcr
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestResolveSecret pins the dcr-package copy of resolveSecret to the same
+// observable contract as the parallel runner-package copy
+// (pkg/authserver/runner/embeddedauthserver_test.go::TestResolveSecret*).
+// Two physically-distinct copies of this helper exist by design (the dcr
+// package must not reach back into pkg/authserver/runner per its
+// profile-agnostic charter); this test guards against silent drift between
+// them. If a future bug fix lands on one copy without being mirrored to the
+// other, this test or its runner-package twin will fail.
+//
+// Cases that take t.Setenv() are kept out of the parallel sub-suite because
+// t.Setenv requires a non-parallel test scope.
+func TestResolveSecret(t *testing.T) {
+	t.Parallel()
+
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "secret")
+	require.NoError(t, os.WriteFile(secretFile, []byte("  secret-value  \n"), 0o600))
+
+	cases := []struct {
+		name        string
+		file        string
+		envVar      string
+		want        string
+		wantErr     bool
+		wantErrSubs []string
+	}{
+		{
+			name: "neither file nor env var set returns empty string and no error",
+			file: "", envVar: "",
+			want: "",
+		},
+		{
+			name: "file content is read and surrounding whitespace trimmed",
+			file: secretFile, envVar: "",
+			want: "secret-value",
+		},
+		{
+			name: "missing file returns wrapped read error",
+			file: "/nonexistent/file", envVar: "",
+			wantErr: true, wantErrSubs: []string{"failed to read secret file"},
+		},
+		{
+			name: "env var name is set but env var is empty returns explanatory error",
+			// Use a unique env var name that won't be set in the environment.
+			file: "", envVar: "TEST_SECRET_NOT_SET_DCR_PKG_12345",
+			wantErr: true, wantErrSubs: []string{"environment variable", "is not set"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := resolveSecret(tc.file, tc.envVar)
+			if tc.wantErr {
+				require.Error(t, err)
+				for _, sub := range tc.wantErrSubs {
+					assert.Contains(t, err.Error(), sub)
+				}
+				assert.Empty(t, got)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// TestResolveSecretWithEnvVar covers the env-var paths separately because
+// t.Setenv requires a non-parallel test scope. Mirrors the runner-package
+// twin (TestResolveSecretWithEnvVar in embeddedauthserver_test.go).
+func TestResolveSecretWithEnvVar(t *testing.T) {
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "secret")
+	require.NoError(t, os.WriteFile(secretFile, []byte("secret-from-file"), 0o600))
+
+	t.Run("file takes precedence over env var when both are set", func(t *testing.T) {
+		envVar := "TEST_SECRET_FILE_PRECEDENCE_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret(secretFile, envVar)
+		require.NoError(t, err)
+		assert.Equal(t, "secret-from-file", got)
+	})
+
+	t.Run("env var is read when file is empty", func(t *testing.T) {
+		envVar := "TEST_SECRET_ENV_ONLY_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret("", envVar)
+		require.NoError(t, err)
+		assert.Equal(t, "secret-from-env", got)
+	})
+
+	t.Run("missing file does not silently fall back to env var", func(t *testing.T) {
+		envVar := "TEST_SECRET_NO_FALLBACK_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret("/nonexistent/file", envVar)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "failed to read secret file")
+		assert.Empty(t, got)
+	})
+}

pkg/authserver/runner/embeddedauthserver.go

@@ -221,8 +221,8 @@ func (e *EmbeddedAuthServer) KeyProvider() keys.KeyProvider {
 // and is intended for admin / diagnostic code paths and integration tests
 // that need to verify that the DCR resolver is wired into the same backend
 // the authserver writes to. It does NOT expose any I/O the
-// runner-DCRCredentialStore adapter does not already provide; the returned
-// value is the same storage.DCRCredentialStore the constructor surfaced.
+// dcr.CredentialStore adapter does not already provide; the returned value
+// is the same storage.DCRCredentialStore the constructor surfaced.
 func (e *EmbeddedAuthServer) DCRStore() storage.DCRCredentialStore {
 	return e.dcrStore
 }
@@ -352,14 +352,14 @@ func parseTokenLifespans(cfg *authserver.TokenLifespanRunConfig) (access, refres
 // RFC 7591 Dynamic Client Registration against the upstream authorization
 // server (hitting the network on first call, using dcrStore on subsequent
 // calls) and overlays the resulting ClientID / ClientSecret onto the output
-// config via consumeResolution + applyResolutionToOAuth2Config. The
+// config via dcr.ConsumeResolution + dcr.ApplyResolutionToOAuth2Config. The
 // caller's runConfigs slice is not mutated: in-place mutation of
 // caller-provided values surprises callers and can cause data races, so
 // each element is cloned before applying DCR resolution.
 //
 // Error logging: this function is the boundary for DCR errors — on any
-// failure from resolveDCRCredentials it emits exactly one structured
-// slog.Error via logDCRStepError and returns the wrapped error to the
+// failure from dcr.ResolveCredentials it emits exactly one structured
+// slog.Error via dcr.LogStepError and returns the wrapped error to the
 // caller without logging further. The resolver itself does not log
 // errors, which avoids the log-and-return double-reporting pattern.
 func buildUpstreamConfigs(