Address tgrunnagle review feedback on CIMD validation

F1  Move TestUnionScopes to registration package where UnionScopes lives;
    delete now-empty handlers/scopes.go and handlers/scopes_test.go
F2  Add assert.ErrorIs(ErrInvalidClient)/NotErrorIs(ErrNotFound) to
    all CIMD policy rejection tests to pin the error type change
F4  Replace 6 positional NewCIMDStorageDecorator args with
    CIMDDecoratorConfig struct — prevents silent swap of adjacent []string
F5  Omitted-scope now calls ValidateScopes(nil, scopesSupported) matching
    DCR: returns DefaultScopes when DefaultScopes ⊆ ScopesSupported,
    error otherwise (document must declare scope explicitly)
F6  Fix dcrErr.Error → dcrErr.ErrorDescription in scope validation hint
    so the human-readable description reaches the fosite hint field
F7  slices.Clone scope slices in CIMDDecoratorConfig constructor
F8  Fix buildFositeClient doc: "when empty" not "when nil"
F9  Export ValidatePublicGrantTypes/ValidatePublicResponseTypes from
    registration package; replace hand-rolled loops in cimd_decorator.go
    with calls to these shared validators — grant_type/response_type
    validation is now identical on both DCR and CIMD paths
F10 Rename AcceptsSupportedGrantTypes→AcceptsOmittedGrantTypes and
    RejectsRefreshTokenOnly→RejectsGrantTypesMissingAuthorizationCode
F11 Remove redundant TestBuildFositeClient_ScopeDefaultsToScopesSupported
    (real decision lives in fetch(), not buildFositeClient)
F12 Update slog.Warn message to name the security consequence when
    CIMD+BaselineClientScopes are both configured

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Author: amirejaz

pkg/authserver/server/handlers/authorize.go

@@ -66,7 +66,9 @@ func (h *Handler) AuthorizeHandler(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	slog.Debug("parsed client-requested scopes", //nolint:gosec // G706: scope count is an integer
+	slog.Debug("authorize request received",
+		"client_id", clientID,
+		"redirect_uri", redirectURI,
 		"scope_count", len(scopes),
 	)
 

pkg/authserver/server/handlers/scopes.go

@@ -355,3 +355,19 @@ func UnionScopes(requested, baseline []string) []string {
 	}
 	return out
 }
+
+// ValidatePublicGrantTypes validates the grant_types for a public OAuth client,
+// applying the same rules as DCR: authorization_code must be present, and all
+// declared values must be in the allowed set. Returns the validated slice (with
+// defaults applied when nil/empty) or a *DCRError on violation.
+func ValidatePublicGrantTypes(grantTypes []string) ([]string, *DCRError) {
+	return validateGrantTypes(grantTypes)
+}
+
+// ValidatePublicResponseTypes validates the response_types for a public OAuth
+// client, applying the same rules as DCR: code must be present and all declared
+// values must be in the allowed set. Returns the validated slice (with defaults
+// applied when nil/empty) or a *DCRError on violation.
+func ValidatePublicResponseTypes(responseTypes []string) ([]string, *DCRError) {
+	return validateResponseTypes(responseTypes)
+}

pkg/authserver/server/handlers/scopes_test.go

@@ -694,3 +694,31 @@ func TestValidateScopeSubset(t *testing.T) {
 		})
 	}
 }
+
+func TestUnionScopes(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		req      []string
+		baseline []string
+		want     []string
+	}{
+		{name: "both nil returns nil", req: nil, baseline: nil, want: nil},
+		{name: "both empty returns nil", req: []string{}, baseline: []string{}, want: nil},
+		{name: "requested only preserved unchanged", req: []string{"openid", "profile"}, baseline: nil, want: []string{"openid", "profile"}},
+		{name: "baseline only returned when no requested", req: nil, baseline: []string{"openid", "email"}, want: []string{"openid", "email"}},
+		{name: "requested subset of baseline expands correctly", req: []string{"openid"}, baseline: []string{"openid", "profile", "email"}, want: []string{"openid", "profile", "email"}},
+		{name: "disjoint sets: requested first then baseline", req: []string{"openid", "profile"}, baseline: []string{"email", "offline_access"}, want: []string{"openid", "profile", "email", "offline_access"}},
+		{name: "exact match produces no duplicates", req: []string{"openid", "profile"}, baseline: []string{"openid", "profile"}, want: []string{"openid", "profile"}},
+		{name: "duplicates in requested are deduplicated", req: []string{"openid", "openid", "profile"}, baseline: nil, want: []string{"openid", "profile"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := UnionScopes(tt.req, tt.baseline)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

pkg/authserver/server/registration/dcr.go

@@ -179,12 +179,17 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage, opts ...se
 	if cfg.CIMDEnabled {
 		if len(cfg.BaselineClientScopes) > 0 {
 			slog.Warn("CIMD is enabled with baseline_client_scopes configured; "+
-				"all dynamically resolved CIMD clients will receive the baseline scopes",
+				"any third-party client resolved via CIMD will also receive these scopes — "+
+				"ensure they are scopes you would grant by default to any unknown client",
 				"baseline_client_scopes", cfg.BaselineClientScopes)
 		}
-		stor, err = storage.NewCIMDStorageDecorator(
-			stor, true, cfg.CIMDCacheMaxSize, cfg.CIMDCacheFallbackTTL,
-			cfg.ScopesSupported, cfg.BaselineClientScopes)
+		stor, err = storage.NewCIMDStorageDecorator(stor, storage.CIMDDecoratorConfig{
+			Enabled:              true,
+			CacheMaxSize:         cfg.CIMDCacheMaxSize,
+			FallbackTTL:          cfg.CIMDCacheFallbackTTL,
+			ScopesSupported:      cfg.ScopesSupported,
+			BaselineClientScopes: cfg.BaselineClientScopes,
+		})
 		if err != nil {
 			return nil, fmt.Errorf("failed to initialize CIMD storage decorator: %w", err)
 		}

pkg/authserver/server/registration/dcr_test.go

@@ -41,44 +41,45 @@ type cimdCacheEntry struct {
 	expires time.Time
 }
 
-// NewCIMDStorageDecorator wraps base with CIMD client lookup. When enabled=false
-// it returns base unchanged (no allocation). cacheMaxSize must be >= 1;
-// fallbackTTL is the fixed TTL applied to every cache entry (Cache-Control
-// header parsing is not yet implemented; all entries use this value).
-// scopesSupported is the AS-configured scope allowlist; documents that declare
-// scopes outside this set are rejected at fetch time. In production this is
-// always non-nil because applyDefaults populates ScopesSupported before the
-// decorator is constructed. Pass nil only in tests that need unconstrained scope
-// passthrough.
-// baselineClientScopes mirrors the AS-level baseline: it is unioned into every
-// CIMD client's scope set after validation, matching DCR handler behaviour.
-func NewCIMDStorageDecorator(
-	base Storage,
-	enabled bool,
-	cacheMaxSize int,
-	fallbackTTL time.Duration,
-	scopesSupported []string,
-	baselineClientScopes []string,
-) (Storage, error) {
-	if !enabled {
+// CIMDDecoratorConfig holds the configuration for NewCIMDStorageDecorator.
+// Using a struct prevents silent swaps of the two adjacent []string fields.
+type CIMDDecoratorConfig struct {
+	// Enabled returns base unchanged when false, avoiding an allocation.
+	Enabled bool
+	// CacheMaxSize is the maximum number of documents in the LRU cache (must be >= 1).
+	CacheMaxSize int
+	// FallbackTTL is the fixed TTL applied to every cache entry.
+	FallbackTTL time.Duration
+	// ScopesSupported is the AS scope allowlist; see pkg/authserver/config.go
+	// applyDefaults for production guarantees. Pass nil in tests only.
+	ScopesSupported []string
+	// BaselineClientScopes is unioned into every CIMD client's scope set,
+	// matching DCR handler behaviour.
+	BaselineClientScopes []string
+}
+
+// NewCIMDStorageDecorator wraps base with CIMD client lookup.
+// When cfg.Enabled is false it returns base unchanged (no allocation).
+func NewCIMDStorageDecorator(base Storage, cfg CIMDDecoratorConfig) (Storage, error) {
+	if !cfg.Enabled {
 		return base, nil
 	}
 
-	if cacheMaxSize < 1 {
-		return nil, fmt.Errorf("CIMD storage decorator cacheMaxSize must be >= 1, got %d", cacheMaxSize)
+	if cfg.CacheMaxSize < 1 {
+		return nil, fmt.Errorf("CIMD storage decorator cacheMaxSize must be >= 1, got %d", cfg.CacheMaxSize)
 	}
 
-	c, err := lru.New[string, *cimdCacheEntry](cacheMaxSize)
+	c, err := lru.New[string, *cimdCacheEntry](cfg.CacheMaxSize)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create CIMD LRU cache: %w", err)
 	}
 
 	return &CIMDStorageDecorator{
 		Storage:              base,
 		cache:                c,
-		ttl:                  fallbackTTL,
-		scopesSupported:      scopesSupported,
-		baselineClientScopes: baselineClientScopes,
+		ttl:                  cfg.FallbackTTL,
+		scopesSupported:      slices.Clone(cfg.ScopesSupported),
+		baselineClientScopes: slices.Clone(cfg.BaselineClientScopes),
 	}, nil
 }
 
@@ -142,41 +143,26 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli
 			id, m, defaultCIMDTokenEndpointAuthMethod)
 	}
 
-	// Reject documents that declare grant_types the embedded AS does not support.
-	// Mirrors DCR's validateGrantTypes which restricts public clients to
-	// authorization_code + refresh_token and requires authorization_code to be present.
-	for _, gt := range doc.GrantTypes {
-		if !allowedCIMDGrantTypes[gt] {
-			return nil, fmt.Errorf("%w: CIMD document at %s claims grant_type %q "+
-				"but this server only supports %v for public clients",
-				fosite.ErrInvalidClient.WithHint("unsupported grant_type"),
-				id, gt, defaultCIMDGrantTypes)
-		}
+	// Reject documents that declare grant_types or response_types the embedded AS
+	// does not support for public clients. Uses the same validators as DCR so the
+	// error messages and allowed sets are identical on both registration paths.
+	if _, dcrErr := registration.ValidatePublicGrantTypes(doc.GrantTypes); dcrErr != nil {
+		return nil, fmt.Errorf("%w: CIMD document at %s: %s",
+			fosite.ErrInvalidClient.WithHint(dcrErr.ErrorDescription), id, dcrErr.ErrorDescription)
 	}
-	if len(doc.GrantTypes) > 0 && !slices.Contains(doc.GrantTypes, "authorization_code") {
-		return nil, fmt.Errorf("%w: CIMD document at %s grant_types must include %q",
-			fosite.ErrInvalidClient.WithHint("grant_types must include authorization_code"),
-			id, "authorization_code")
-	}
-
-	// Reject documents that declare response_types the embedded AS does not support.
-	for _, rt := range doc.ResponseTypes {
-		if !allowedCIMDResponseTypes[rt] {
-			return nil, fmt.Errorf("%w: CIMD document at %s claims response_type %q "+
-				"but this server only supports %v",
-				fosite.ErrInvalidClient.WithHint("unsupported response_type"),
-				id, rt, defaultCIMDResponseTypes)
-		}
+	if _, dcrErr := registration.ValidatePublicResponseTypes(doc.ResponseTypes); dcrErr != nil {
+		return nil, fmt.Errorf("%w: CIMD document at %s: %s",
+			fosite.ErrInvalidClient.WithHint(dcrErr.ErrorDescription), id, dcrErr.ErrorDescription)
 	}
 
 	// Compute and validate the client scope list consistent with DCR.
 	// When ScopesSupported is configured:
 	//   - Declared scopes are validated via registration.ValidateScopes (same
 	//     function as the DCR handler).
-	//   - When the document omits scope, the client receives ScopesSupported
-	//     rather than DefaultScopes — a CIMD document that doesn't declare scope
-	//     means "whatever the AS supports", not "give me the full default set"
-	//     (which may exceed ScopesSupported).
+	//   - Omitted scope uses ValidateScopes(nil, scopesSupported) which returns
+	//     DefaultScopes when DefaultScopes ⊆ ScopesSupported, matching DCR.
+	//     If DefaultScopes ⊄ ScopesSupported the document must declare scope
+	//     explicitly to avoid ambiguous privilege grant.
 	// When ScopesSupported is not configured: no AS-level validation; declared
 	// scopes are used directly, or nil to let buildFositeClient apply DefaultScopes.
 	// In both cases BaselineClientScopes is unioned in after validation,
@@ -187,11 +173,20 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli
 			computed, dcrErr := registration.ValidateScopes(strings.Fields(doc.Scope), d.scopesSupported)
 			if dcrErr != nil {
 				return nil, fmt.Errorf("%w: CIMD document at %s: %s",
-					fosite.ErrInvalidClient.WithHint(dcrErr.Error), id, dcrErr.ErrorDescription)
+					fosite.ErrInvalidClient.WithHint(dcrErr.ErrorDescription), id, dcrErr.ErrorDescription)
 			}
 			resolvedScopes = computed
 		} else {
-			resolvedScopes = slices.Clone(d.scopesSupported)
+			// Omitted scope: match DCR — give DefaultScopes when they fit, else require explicit scope.
+			computed, dcrErr := registration.ValidateScopes(nil, d.scopesSupported)
+			if dcrErr != nil {
+				return nil, fmt.Errorf("%w: CIMD document at %s omits scope but "+
+					"DefaultScopes are not a subset of this server's scopes_supported — "+
+					"the document must explicitly declare its required scopes",
+					fosite.ErrInvalidClient.WithHint("scope field required"),
+					id)
+			}
+			resolvedScopes = computed
 		}
 	} else if doc.Scope != "" {
 		resolvedScopes = strings.Fields(doc.Scope)
@@ -215,19 +210,10 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli
 // that use the authorization code flow with refresh token rotation.
 var defaultCIMDGrantTypes = []string{"authorization_code", "refresh_token"}
 
-// allowedCIMDGrantTypes is the set of grant_type values a CIMD document may
-// declare. Values outside this set are rejected at fetch time, consistent with
-// DCR which restricts public clients to authorization_code + refresh_token.
-var allowedCIMDGrantTypes = map[string]bool{"authorization_code": true, "refresh_token": true}
-
 // defaultCIMDResponseTypes are the OAuth 2.0 response types applied when the
 // CIMD document omits response_types.
 var defaultCIMDResponseTypes = []string{"code"}
 
-// allowedCIMDResponseTypes is the set of response_type values a CIMD document
-// may declare. Values outside this set are rejected at fetch time.
-var allowedCIMDResponseTypes = map[string]bool{"code": true}
-
 // defaultCIMDTokenEndpointAuthMethod is the token endpoint authentication
 // method applied when the CIMD document omits token_endpoint_auth_method.
 // Documents that declare any other value are rejected by fetch() before
@@ -238,7 +224,8 @@ const defaultCIMDTokenEndpointAuthMethod = "none"
 // Redirect URIs containing http://localhost are wrapped in a LoopbackClient
 // so that RFC 8252 §7.3 dynamic port matching applies.
 // resolvedScopes is the already-validated scope list computed by fetch() via
-// registration.ValidateScopes; when nil, DefaultScopes is used (unconstrained AS).
+// registration.ValidateScopes; when empty, DefaultScopes is used — this occurs when
+// the decorator has no ScopesSupported restriction (unconstrained AS).
 func buildFositeClient(doc *cimd.ClientMetadataDocument, resolvedScopes []string) fosite.Client {
 	grantTypes := doc.GrantTypes
 	if len(grantTypes) == 0 {