Validate CIMD scope, grant_types and response_types against AS policy

[amirejaz]

Summary

• Stacked on Wire CIMD config through embedded AS and enable storage decorator #5348 (wire CIMD config) — merge that first.
• Addresses two deferred review comments from tgrunnagle on PR Wire CIMD config through embedded AS and enable storage decorator #5348.

C3 — CIMD scope handling now consistent with DCR:
ScopesSupported is threaded into NewCIMDStorageDecorator. Scope validation in fetch() now uses registration.ValidateScopes — the same function as the DCR handler — so both paths enforce the same scope policy. A CIMD document declaring scopes outside ScopesSupported is rejected at GetClient time. The fallback when scope is omitted also respects ScopesSupported.

C4 — grant_types / response_types validated at fetch time:
fetch() now rejects documents declaring grant_types outside [authorization_code, refresh_token] or response_types outside [code]. Consistent with DCR which returns invalid_client_metadata for the same cases, and matches the existing token_endpoint_auth_method validation pattern already in the decorator.

Type of change

• Bug fix / security improvement

Test plan

• go test ./pkg/authserver/... passes
• task lint-fix clean
• New tests: scope validation with/without ScopesSupported, grant_type rejection for unsupported values, response_type rejection for token

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.77%. Comparing base (16ce897) to head (969544a).

Files with missing lines	Patch %	Lines
pkg/authserver/server_impl.go	58.33%	4 Missing and 1 partial ⚠️
pkg/authserver/storage/cimd_decorator.go	95.34%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5385      +/-   ##
==========================================
+ Coverage   68.76%   68.77%   +0.01%     
==========================================
  Files         634      633       -1     
  Lines       64183    64229      +46     
==========================================
+ Hits        44136    44175      +39     
- Misses      16778    16784       +6     
- Partials     3269     3270       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-agent review

Four specialist agents (oauth/security, Go correctness/style, test quality, general/API) reviewed this PR. Codex cross-review was skipped (codex CLI not installed locally). 0 HIGH, 5 MEDIUM, 7 LOW findings survived consensus scoring. None are blocking; this is posted as COMMENT.

Executive summary

A focused follow-up to stacked PR #5348 closing deferred review comments C3/C4. It threads ScopesSupported and BaselineClientScopes into the CIMD storage decorator so CIMD documents are validated against the same policy as DCR (via registration.ValidateScopes), and rejects unsupported grant_types / response_types at fetch time. As supporting refactor, unionScopes is promoted to registration.UnionScopes. Approach is correct in shape: reuse a single validator, reject earlier in the pipeline, surface a startup warning for the CIMD+baseline interaction. Most surviving findings are correctness-leaning details and test-quality nits.

Pros

• Reuses registration.ValidateScopes instead of duplicating policy.
• Rejects invalid CIMD documents at fetch time (not at /authorize), giving operators a clear, early error.
• Error class change ErrNotFound → ErrInvalidClient for "fetched but policy-violating" is semantically correct per RFC 6749 §5.2.
• Public/PKCE invariant preserved (Public: true unconditional; allowlists still exclude client_credentials).
• slog.Warn at startup surfaces the non-obvious CIMD+baseline interaction.
• Doc comments explain the design intent and the test-only nil escape hatch.
• PR title, body, and size comply with .claude/rules/pr-creation.md.

Cons

• The "same scope policy as DCR" claim has one silent divergence: when doc.Scope == "" and ScopesSupported ⊃ DefaultScopes, the CIMD client receives the full ScopesSupported while a DCR client in the same position receives only DefaultScopes. See F5.
• NewCIMDStorageDecorator now takes six positional args with two adjacent same-typed []string slices — easy to swap silently.
• TestUnionScopes was not relocated alongside the moved function and still lives in pkg/authserver/server/handlers/, where scopes.go is now empty.
• New rejection tests assert require.Error(...) only — they would still pass if the code regressed to fosite.ErrNotFound, defeating the PR's main intent.

Consensus findings table

#	Severity	File	Title
F1	MEDIUM	handlers/scopes_test.go	TestUnionScopes lives in wrong package after move; pair with deleting now-empty scopes.go
F2	MEDIUM	storage/cimd_decorator_test.go	Rejection tests don't assert errors.Is(err, fosite.ErrInvalidClient)
F3	MEDIUM	storage/cimd_decorator_test.go	Consolidate 11 new top-level tests into ~3 table-driven tests
F4	MEDIUM	storage/cimd_decorator.go	NewCIMDStorageDecorator signature: 6 positional args, two adjacent same-typed slices
F5	MEDIUM	storage/cimd_decorator.go	Omitted-scope branch on CIMD diverges from DCR when ScopesSupported ⊃ DefaultScopes
F6	LOW	storage/cimd_decorator.go	WithHint(dcrErr.Error) passes OAuth error code where descriptive hint belongs
F7	LOW	storage/cimd_decorator.go	Constructor stores caller-supplied scope slices without defensive slices.Clone
F8	LOW	storage/cimd_decorator.go	Doc comments document cross-file invariants not enforced in code
F9	LOW	storage/cimd_decorator.go	Missing DCR-parity defense-in-depth: response_types must include 'code'; grant-type validation order reversed vs DCR
F10	LOW	storage/cimd_decorator_test.go	Test names don't match assertions
F11	LOW	storage/cimd_decorator_test.go	Redundant unit-level test; missing baseline-exceeds-supported coverage
F12	LOW	authserver/server_impl.go	slog.Warn level/clarity for CIMD+baseline interaction

Detailed comments are inline on the relevant changed lines.

How to test

git fetch origin pull/5385/head:pr-5385 && git checkout pr-5385
task lint-fix
task test

Plus targeted checks against an embedded AS:

1. C3 — declared scope outside ScopesSupported: serve a CIMD doc declaring scope: "openid profile" with scopes_supported: ["openid"]. Expect fosite.ErrInvalidClient.
2. C3 — omitted scope: same setup but doc omits scope. Confirm the client carries exactly ["openid"], not DefaultScopes (and consider what should happen when ScopesSupported ⊃ DefaultScopes — see F5).
3. C4 — unsupported grant_type: serve a doc with grant_types: ["client_credentials"]. Expect rejection.
4. C4 — missing authorization_code: serve a doc with grant_types: ["refresh_token"] only. Expect rejection.
5. C4 — unsupported response_type: serve a doc with response_types: ["token"]. Expect rejection.
6. slog.Warn: start the AS with cimd.enabled: true and baseline_client_scopes: ["offline_access"]. Expect one WARN line at startup.
7. Load-bearing-test check: temporarily revert if !allowedCIMDGrantTypes[gt] to always-false, re-run go test ./pkg/authserver/storage/.... TestFetch_RejectsUnsupportedGrantType should fail; revert when done.

Generated with Claude Code (multi-agent /dev:pr-review).

[tgrunnagle]

Re-review LGTM — all 12 findings from the prior multi-agent review are resolved. F3 went from 11 separate tests to 3 table-driven tests (plus a serveCIMDDocWithFields helper), F5 chose the fail-closed option (require explicit scope when DefaultScopes ⊄ ScopesSupported), and F9 extracted ValidatePublicGrantTypes / ValidatePublicResponseTypes so DCR and CIMD now share a single validation source. The errors.Is(err, fosite.ErrInvalidClient) assertions pin the error-type change so a regression to ErrNotFound would fail tests.

One nice-to-have follow-up (not blocking)

defaultCIMDGrantTypes and defaultCIMDResponseTypes in cimd_decorator.go are still parallel to defaultGrantTypes / defaultResponseTypes in the registration package. After F9 the validation is shared, but the default application is still duplicated — if someone later widens registration.defaultGrantTypes for DCR (e.g., adds a new public-client grant), they'd need to remember to update the CIMD copy too, and task lint won't catch the mismatch.

Since ValidatePublicGrantTypes / ValidatePublicResponseTypes already return the defaulted slice, buildFositeClient could use those return values and the CIMD-side constants could be deleted. Closes the last DCR/CIMD parity gap. Not blocking for this PR — could be a small follow-up.

Approving.