Skip to content

Commit 070de2d

Browse files
amirejazclaude
andcommitted
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>
1 parent 8fbc935 commit 070de2d

7 files changed

Lines changed: 305 additions & 64 deletions

File tree

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 & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,37 +2,3 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
package handlers
5-
6-
// unionScopes returns the union of requested and baseline scopes, preserving
7-
// the order of requested first, then appending any baseline scopes not already
8-
// present. Duplicates are removed. Returns nil when the result is empty.
9-
//
10-
// This is used by the DCR registration handler to inject an
11-
// operator-configured scope baseline into the registered client's scope set:
12-
// if a client narrows the scope field at /oauth/register, the baseline scopes
13-
// are still part of the client's registered set so that the client can
14-
// request them later at /oauth/authorize without invalid_scope rejection.
15-
//
16-
// Both inputs must already be validated by the caller (e.g. via ValidateScopes
17-
// for client-supplied scopes). unionScopes does not filter empty strings or
18-
// validate scope syntax — it only deduplicates and merges in stable order.
19-
func unionScopes(requested, baseline []string) []string {
20-
seen := make(map[string]bool, len(requested)+len(baseline))
21-
out := make([]string, 0, len(requested)+len(baseline))
22-
for _, s := range requested {
23-
if !seen[s] {
24-
seen[s] = true
25-
out = append(out, s)
26-
}
27-
}
28-
for _, s := range baseline {
29-
if !seen[s] {
30-
seen[s] = true
31-
out = append(out, s)
32-
}
33-
}
34-
if len(out) == 0 {
35-
return nil
36-
}
37-
return out
38-
}

pkg/authserver/server/handlers/scopes_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"testing"
88

99
"github.com/stretchr/testify/assert"
10+
11+
"github.com/stacklok/toolhive/pkg/authserver/server/registration"
1012
)
1113

1214
func TestUnionScopes(t *testing.T) {
@@ -108,7 +110,7 @@ func TestUnionScopes(t *testing.T) {
108110
t.Run(tt.name, func(t *testing.T) {
109111
t.Parallel()
110112

111-
got := unionScopes(tt.req, tt.baseline)
113+
got := registration.UnionScopes(tt.req, tt.baseline)
112114
assert.Equal(t, tt.want, got)
113115
})
114116
}

pkg/authserver/server/registration/dcr.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,3 +327,31 @@ 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+
}

pkg/authserver/server_impl.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,9 @@ 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+
stor, err = storage.NewCIMDStorageDecorator(
181+
stor, true, cfg.CIMDCacheMaxSize, cfg.CIMDCacheFallbackTTL,
182+
cfg.ScopesSupported, cfg.BaselineClientScopes)
181183
if err != nil {
182184
return nil, fmt.Errorf("failed to initialize CIMD storage decorator: %w", err)
183185
}

pkg/authserver/storage/cimd_decorator.go

Lines changed: 89 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,12 @@ import (
2828
// Only GetClient is overridden. DCR clients (opaque IDs) continue to work
2929
// exactly as before.
3030
type CIMDStorageDecorator struct {
31-
Storage // embed full interface — all methods delegate
32-
sf singleflight.Group // deduplicates concurrent fetches for the same URL
33-
cache *lru.Cache[string, *cimdCacheEntry]
34-
ttl time.Duration
31+
Storage // embed full interface — all methods delegate
32+
sf singleflight.Group // deduplicates concurrent fetches for the same URL
33+
cache *lru.Cache[string, *cimdCacheEntry]
34+
ttl time.Duration
35+
scopesSupported []string // AS-configured scopes; nil means accept any
36+
baselineClientScopes []string // unioned into every client's scope set, same as DCR
3537
}
3638

3739
type cimdCacheEntry struct {
@@ -43,11 +45,18 @@ type cimdCacheEntry struct {
4345
// it returns base unchanged (no allocation). cacheMaxSize must be >= 1;
4446
// fallbackTTL is the fixed TTL applied to every cache entry (Cache-Control
4547
// header parsing is not yet implemented; all entries use this value).
48+
// scopesSupported is the AS-configured scope allowlist; documents that declare
49+
// scopes outside this set are rejected at fetch time. Pass nil to skip scope
50+
// validation (e.g. when ScopesSupported is unset and DefaultScopes applies).
51+
// baselineClientScopes mirrors the AS-level baseline: it is unioned into every
52+
// CIMD client's scope set after validation, matching DCR handler behaviour.
4653
func NewCIMDStorageDecorator(
4754
base Storage,
4855
enabled bool,
4956
cacheMaxSize int,
5057
fallbackTTL time.Duration,
58+
scopesSupported []string,
59+
baselineClientScopes []string,
5160
) (Storage, error) {
5261
if !enabled {
5362
return base, nil
@@ -63,9 +72,11 @@ func NewCIMDStorageDecorator(
6372
}
6473

6574
return &CIMDStorageDecorator{
66-
Storage: base,
67-
cache: c,
68-
ttl: fallbackTTL,
75+
Storage: base,
76+
cache: c,
77+
ttl: fallbackTTL,
78+
scopesSupported: scopesSupported,
79+
baselineClientScopes: baselineClientScopes,
6980
}, nil
7081
}
7182

@@ -129,7 +140,59 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli
129140
id, m, defaultCIMDTokenEndpointAuthMethod)
130141
}
131142

132-
client := buildFositeClient(doc)
143+
// Reject documents that declare grant_types the embedded AS does not support.
144+
// Consistent with DCR which restricts public clients to authorization_code + refresh_token.
145+
for _, gt := range doc.GrantTypes {
146+
if !allowedCIMDGrantTypes[gt] {
147+
return nil, fmt.Errorf("%w: CIMD document at %s claims grant_type %q "+
148+
"but this server only supports %v for public clients",
149+
fosite.ErrNotFound.WithHint("unsupported grant_type"),
150+
id, gt, defaultCIMDGrantTypes)
151+
}
152+
}
153+
154+
// Reject documents that declare response_types the embedded AS does not support.
155+
for _, rt := range doc.ResponseTypes {
156+
if !allowedCIMDResponseTypes[rt] {
157+
return nil, fmt.Errorf("%w: CIMD document at %s claims response_type %q "+
158+
"but this server only supports %v",
159+
fosite.ErrNotFound.WithHint("unsupported response_type"),
160+
id, rt, defaultCIMDResponseTypes)
161+
}
162+
}
163+
164+
// Compute and validate the client scope list consistent with DCR.
165+
// When ScopesSupported is configured:
166+
// - Declared scopes are validated via registration.ValidateScopes (same
167+
// function as the DCR handler).
168+
// - When the document omits scope, the client receives ScopesSupported
169+
// rather than DefaultScopes — a CIMD document that doesn't declare scope
170+
// means "whatever the AS supports", not "give me the full default set"
171+
// (which may exceed ScopesSupported).
172+
// When ScopesSupported is not configured: no AS-level validation; declared
173+
// scopes are used directly, or nil to let buildFositeClient apply DefaultScopes.
174+
// In both cases BaselineClientScopes is unioned in after validation,
175+
// matching the DCR handler's behaviour.
176+
var resolvedScopes []string
177+
if len(d.scopesSupported) > 0 {
178+
if doc.Scope != "" {
179+
computed, dcrErr := registration.ValidateScopes(strings.Fields(doc.Scope), d.scopesSupported)
180+
if dcrErr != nil {
181+
return nil, fmt.Errorf("%w: CIMD document at %s: %s",
182+
fosite.ErrNotFound.WithHint(string(dcrErr.Error)), id, dcrErr.ErrorDescription)
183+
}
184+
resolvedScopes = computed
185+
} else {
186+
resolvedScopes = slices.Clone(d.scopesSupported)
187+
}
188+
} else if doc.Scope != "" {
189+
resolvedScopes = strings.Fields(doc.Scope)
190+
}
191+
if len(d.baselineClientScopes) > 0 {
192+
resolvedScopes = registration.UnionScopes(resolvedScopes, d.baselineClientScopes)
193+
}
194+
195+
client := buildFositeClient(doc, resolvedScopes)
133196

134197
d.cache.Add(id, &cimdCacheEntry{
135198
client: client,
@@ -144,10 +207,19 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli
144207
// that use the authorization code flow with refresh token rotation.
145208
var defaultCIMDGrantTypes = []string{"authorization_code", "refresh_token"}
146209

210+
// allowedCIMDGrantTypes is the set of grant_type values a CIMD document may
211+
// declare. Values outside this set are rejected at fetch time, consistent with
212+
// DCR which restricts public clients to authorization_code + refresh_token.
213+
var allowedCIMDGrantTypes = map[string]bool{"authorization_code": true, "refresh_token": true}
214+
147215
// defaultCIMDResponseTypes are the OAuth 2.0 response types applied when the
148216
// CIMD document omits response_types.
149217
var defaultCIMDResponseTypes = []string{"code"}
150218

219+
// allowedCIMDResponseTypes is the set of response_type values a CIMD document
220+
// may declare. Values outside this set are rejected at fetch time.
221+
var allowedCIMDResponseTypes = map[string]bool{"code": true}
222+
151223
// defaultCIMDTokenEndpointAuthMethod is the token endpoint authentication
152224
// method applied when the CIMD document omits token_endpoint_auth_method.
153225
// Documents that declare any other value are rejected by fetch() before
@@ -157,7 +229,9 @@ const defaultCIMDTokenEndpointAuthMethod = "none"
157229
// buildFositeClient converts a ClientMetadataDocument into a fosite.Client.
158230
// Redirect URIs containing http://localhost are wrapped in a LoopbackClient
159231
// so that RFC 8252 §7.3 dynamic port matching applies.
160-
func buildFositeClient(doc *cimd.ClientMetadataDocument) fosite.Client {
232+
// resolvedScopes is the already-validated scope list computed by fetch() via
233+
// registration.ValidateScopes; when nil, DefaultScopes is used (unconstrained AS).
234+
func buildFositeClient(doc *cimd.ClientMetadataDocument, resolvedScopes []string) fosite.Client {
161235
grantTypes := doc.GrantTypes
162236
if len(grantTypes) == 0 {
163237
grantTypes = defaultCIMDGrantTypes
@@ -173,13 +247,12 @@ func buildFositeClient(doc *cimd.ClientMetadataDocument) fosite.Client {
173247
tokenEndpointAuthMethod = defaultCIMDTokenEndpointAuthMethod
174248
}
175249

176-
// When the document omits the scope field, apply the same defaults as DCR
177-
// registration so CIMD clients can request openid/profile/email/offline_access
178-
// without needing to enumerate them explicitly in the metadata document.
179-
// Clone to avoid aliasing the package-level DefaultScopes slice.
180-
scopes := slices.Clone(registration.DefaultScopes)
181-
if doc.Scope != "" {
182-
scopes = strings.Fields(doc.Scope)
250+
// Scopes were computed and validated by fetch() via registration.ValidateScopes,
251+
// consistent with the DCR handler. Fall back to DefaultScopes only when the
252+
// decorator has no ScopesSupported restriction (unconstrained AS).
253+
scopes := resolvedScopes
254+
if len(scopes) == 0 {
255+
scopes = slices.Clone(registration.DefaultScopes)
183256
}
184257

185258
defaultClient := &fosite.DefaultClient{

0 commit comments

Comments
 (0)