Skip to content

Commit 521630b

Browse files
amirejazclaude
andauthored
Validate CIMD scope, grant_types and response_types against AS policy (#5385)
* Validate CIMD scope, grant_types and response_types against AS policy C3 - Thread ScopesSupported into NewCIMDStorageDecorator so CIMD scope handling is consistent with DCR. Uses registration.ValidateScopes (same function as the DCR handler) to validate declared scopes against the AS allowlist and compute the effective scope list. When ScopesSupported is unset, the document's declared scopes are used directly; omitted scopes default to DefaultScopes. C4 - Reject CIMD documents that declare grant_types or response_types the embedded AS does not support for public clients (authorization_code + refresh_token; code). Consistent with DCR which returns invalid_client_metadata for the same cases. buildFositeClient now receives pre-computed scopes from fetch() rather than re-parsing doc.Scope, matching the DCR handler pattern where scope computation and validation happen before client construction. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 974aab5 commit 521630b

9 files changed

Lines changed: 398 additions & 204 deletions

File tree

pkg/authserver/server/handlers/authorize.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ func (h *Handler) AuthorizeHandler(w http.ResponseWriter, req *http.Request) {
6666
return
6767
}
6868

69-
slog.Debug("parsed client-requested scopes", //nolint:gosec // G706: scope count is an integer
69+
slog.Debug("authorize request received",
70+
"client_id", clientID,
71+
"redirect_uri", redirectURI,
7072
"scope_count", len(scopes),
7173
)
7274

pkg/authserver/server/handlers/dcr.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func (h *Handler) RegisterClientHandler(w http.ResponseWriter, req *http.Request
7777
// offline_access) — every DCR-registered client gains the ability to request
7878
// these scopes at /oauth/authorize regardless of what they registered with.
7979
if len(h.config.BaselineClientScopes) > 0 {
80-
effective := unionScopes(scopes, h.config.BaselineClientScopes)
80+
effective := registration.UnionScopes(scopes, h.config.BaselineClientScopes)
8181
if !slices.Equal(effective, scopes) {
8282
// Baseline-driven expansion is the intended behavior whenever
8383
// baseline_client_scopes is configured, so per-registration

pkg/authserver/server/handlers/scopes.go

Lines changed: 0 additions & 38 deletions
This file was deleted.

pkg/authserver/server/handlers/scopes_test.go

Lines changed: 0 additions & 115 deletions
This file was deleted.

pkg/authserver/server/registration/dcr.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,3 +327,47 @@ func ValidateScopes(requestedScopes, allowedScopes []string) ([]string, *DCRErro
327327

328328
return scopes, nil
329329
}
330+
331+
// UnionScopes returns the union of requested and baseline scopes, preserving
332+
// the order of requested first, then appending any baseline scopes not already
333+
// present. Duplicates are removed. Returns nil when the result is empty.
334+
//
335+
// Both inputs must already be validated by the caller. UnionScopes does not
336+
// filter empty strings or validate scope syntax — it only deduplicates and
337+
// merges in stable order.
338+
func UnionScopes(requested, baseline []string) []string {
339+
seen := make(map[string]bool, len(requested)+len(baseline))
340+
out := make([]string, 0, len(requested)+len(baseline))
341+
for _, s := range requested {
342+
if !seen[s] {
343+
seen[s] = true
344+
out = append(out, s)
345+
}
346+
}
347+
for _, s := range baseline {
348+
if !seen[s] {
349+
seen[s] = true
350+
out = append(out, s)
351+
}
352+
}
353+
if len(out) == 0 {
354+
return nil
355+
}
356+
return out
357+
}
358+
359+
// ValidatePublicGrantTypes validates the grant_types for a public OAuth client,
360+
// applying the same rules as DCR: authorization_code must be present, and all
361+
// declared values must be in the allowed set. Returns the validated slice (with
362+
// defaults applied when nil/empty) or a *DCRError on violation.
363+
func ValidatePublicGrantTypes(grantTypes []string) ([]string, *DCRError) {
364+
return validateGrantTypes(grantTypes)
365+
}
366+
367+
// ValidatePublicResponseTypes validates the response_types for a public OAuth
368+
// client, applying the same rules as DCR: code must be present and all declared
369+
// values must be in the allowed set. Returns the validated slice (with defaults
370+
// applied when nil/empty) or a *DCRError on violation.
371+
func ValidatePublicResponseTypes(responseTypes []string) ([]string, *DCRError) {
372+
return validateResponseTypes(responseTypes)
373+
}

pkg/authserver/server/registration/dcr_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,3 +694,31 @@ func TestValidateScopeSubset(t *testing.T) {
694694
})
695695
}
696696
}
697+
698+
func TestUnionScopes(t *testing.T) {
699+
t.Parallel()
700+
701+
tests := []struct {
702+
name string
703+
req []string
704+
baseline []string
705+
want []string
706+
}{
707+
{name: "both nil returns nil", req: nil, baseline: nil, want: nil},
708+
{name: "both empty returns nil", req: []string{}, baseline: []string{}, want: nil},
709+
{name: "requested only preserved unchanged", req: []string{"openid", "profile"}, baseline: nil, want: []string{"openid", "profile"}},
710+
{name: "baseline only returned when no requested", req: nil, baseline: []string{"openid", "email"}, want: []string{"openid", "email"}},
711+
{name: "requested subset of baseline expands correctly", req: []string{"openid"}, baseline: []string{"openid", "profile", "email"}, want: []string{"openid", "profile", "email"}},
712+
{name: "disjoint sets: requested first then baseline", req: []string{"openid", "profile"}, baseline: []string{"email", "offline_access"}, want: []string{"openid", "profile", "email", "offline_access"}},
713+
{name: "exact match produces no duplicates", req: []string{"openid", "profile"}, baseline: []string{"openid", "profile"}, want: []string{"openid", "profile"}},
714+
{name: "duplicates in requested are deduplicated", req: []string{"openid", "openid", "profile"}, baseline: nil, want: []string{"openid", "profile"}},
715+
}
716+
717+
for _, tt := range tests {
718+
t.Run(tt.name, func(t *testing.T) {
719+
t.Parallel()
720+
got := UnionScopes(tt.req, tt.baseline)
721+
assert.Equal(t, tt.want, got)
722+
})
723+
}
724+
}

pkg/authserver/server_impl.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,19 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage, opts ...se
177177
// so that GetClient calls for HTTPS client_id values are intercepted at the
178178
// fosite level (not just the handler level).
179179
if cfg.CIMDEnabled {
180-
stor, err = storage.NewCIMDStorageDecorator(stor, true, cfg.CIMDCacheMaxSize, cfg.CIMDCacheFallbackTTL)
180+
if len(cfg.BaselineClientScopes) > 0 {
181+
slog.Warn("CIMD is enabled with baseline_client_scopes configured; "+
182+
"any third-party client resolved via CIMD will also receive these scopes — "+
183+
"ensure they are scopes you would grant by default to any unknown client",
184+
"baseline_client_scopes", cfg.BaselineClientScopes)
185+
}
186+
stor, err = storage.NewCIMDStorageDecorator(stor, storage.CIMDDecoratorConfig{
187+
Enabled: true,
188+
CacheMaxSize: cfg.CIMDCacheMaxSize,
189+
FallbackTTL: cfg.CIMDCacheFallbackTTL,
190+
ScopesSupported: cfg.ScopesSupported,
191+
BaselineClientScopes: cfg.BaselineClientScopes,
192+
})
181193
if err != nil {
182194
return nil, fmt.Errorf("failed to initialize CIMD storage decorator: %w", err)
183195
}

0 commit comments

Comments
 (0)