Skip to content

Commit be8944a

Browse files
BobbyHoclaude
BobbyHo
and
claude
committed
fix: address panel review findings on public OAuth2 clients
Addresses the first full panel review on #27873. Twenty findings; the substantive ones: The transaction test could not detect the regression it was written for. Stubbing InTx to call the closure with the same mock made a call on tx and a call on the outer store indistinguishable, so moving the secret insert back outside the transaction kept it green. The closure now receives a second mock, and an insert issued on the outer handle fails as an unexpected call. The server reported `token_endpoint_auth_method` from the stored column while enforcing on `client_type`. Clients registered with "none" before it was honored are stored confidential and still need their secret, so reporting "none" told them to drop it. Report the method implied by the enforced type instead, which also lets the row repair itself on the client's next update. An admin could mint a client secret for a public app. The token endpoint never validates it, and deleting it revokes nothing, because a public client's tokens carry a NULL `app_secret_id` rather than cascading from the secret. The confidential kill switch silently did not exist for public apps, so secret creation is now rejected for them. The client type is a defined type with a single owner for the mapping from auth method, and `client_type` is constrained at the schema level. It decides whether client authentication runs at all, and the column accepted any text. Redirect URI validation now derives publicness from the same place registration does rather than re-deriving it. PKCE verifiers are checked against RFC 7636 §4.1's 43 to 128 character bound. For a public client the verifier is the only client authentication, and a one-character verifier hashes to a well-formed challenge, so the comparison alone could not tell a secret from a guess. The rest: RFC 7592 rejections name the values compared and log, a public client's exchange is tested with missing, wrong, and too-short verifiers, `client_secret` absence is asserted against the raw body rather than a decoded struct, `registration_client_uri` uses JoinPath so a trailing slash in the access URL cannot double, and the public-client fixture is shared instead of copied. Docs and the token endpoint's swagger annotations described a confidential-only world: both now cover public clients, including the redirect URI schemes they cannot use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2b89218 commit be8944a

28 files changed

Lines changed: 492 additions & 182 deletions

‎coderd/apidoc/docs.go‎

Lines changed: 7 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json‎

Lines changed: 7 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/check_constraint.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/constants.go‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,22 @@ import (
1111
// codersdk constant so the two cannot drift.
1212
var PrebuildsSystemUserID = uuid.MustParse(codersdk.PrebuildsSystemUserID)
1313

14-
// Values stored in oauth2_provider_apps.client_type.
14+
// Values stored in oauth2_provider_apps.client_type, as plain strings for
15+
// comparison against the nullable column.
1516
//
16-
// Aliased from codersdk rather than redeclared. Registration derives the value
17-
// there (OAuth2ClientRegistrationRequest.DetermineClientType) while
18-
// OAuth2ProviderApp.IsPublic reads it back here to decide whether the token
19-
// endpoint validates a client secret at all. A literal that drifted toward
20-
// "public" between the two would silently disable client authentication, and
21-
// nothing would catch it, so the compiler holds the two spellings equal
22-
// instead of a test asserting it after the fact.
17+
// Converted from the codersdk constants rather than redeclared, so the value
18+
// registration writes and the value OAuth2ProviderApp.IsPublic reads back
19+
// cannot disagree. That divergence would fail closed anyway (the app would read
20+
// as confidential and demand a secret it was never issued), but it would fail
21+
// visibly to a client rather than here.
22+
//
23+
// What this does not protect against is the two constants colliding on the same
24+
// value, which would make IsPublic true for confidential apps. Nothing in the
25+
// type system can catch that; the tests that pin these spellings to the wire
26+
// values do, so do not delete them as redundant:
27+
// TestOAuth2ClientRegistrationRequest_DetermineClientType and
28+
// TestCreateDynamicClientRegistration_ClientType.
2329
const (
24-
OAuth2ProviderAppClientTypeConfidential = codersdk.OAuth2ClientTypeConfidential
25-
OAuth2ProviderAppClientTypePublic = codersdk.OAuth2ClientTypePublic
30+
OAuth2ProviderAppClientTypeConfidential = string(codersdk.OAuth2ClientTypeConfidential)
31+
OAuth2ProviderAppClientTypePublic = string(codersdk.OAuth2ClientTypePublic)
2632
)

‎coderd/database/dbgen/dbgen.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1732,7 +1732,7 @@ func OAuth2ProviderApp(t testing.TB, db database.Store, seed database.OAuth2Prov
17321732
Icon: takeFirst(seed.Icon, ""),
17331733
CallbackURL: takeFirst(seed.CallbackURL, "http://localhost"),
17341734
RedirectUris: takeFirstSlice(seed.RedirectUris, []string{}),
1735-
ClientType: takeFirst(seed.ClientType, sql.NullString{String: database.OAuth2ProviderAppClientTypeConfidential, Valid: true}),
1735+
ClientType: takeFirst(seed.ClientType, database.OAuth2ProviderAppClientTypeConfidential),
17361736
DynamicallyRegistered: takeFirst(seed.DynamicallyRegistered, sql.NullBool{Bool: false, Valid: true}),
17371737
ClientIDIssuedAt: takeFirst(seed.ClientIDIssuedAt, sql.NullTime{}),
17381738
ClientSecretExpiresAt: takeFirst(seed.ClientSecretExpiresAt, sql.NullTime{}),

‎coderd/database/dump.sql‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000563_oauth2_client_type_constraint.down.sql‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
ALTER TABLE oauth2_provider_apps
2+
ALTER COLUMN client_type DROP NOT NULL;
3+
4+
ALTER TABLE oauth2_provider_apps
5+
DROP CONSTRAINT oauth2_provider_apps_client_type_check;

‎coderd/database/migrations/000563_oauth2_client_type_constraint.up.sql‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
-- client_type decides whether the token endpoint validates a client secret at
2+
-- all, so a row holding an unrecognized value is a client whose authentication
3+
-- rules are undefined. Constrain it at the schema level: no Go path can write a
4+
-- bad value today, but a future migration writing 'public' onto a row that
5+
-- holds a secret would turn off client authentication for that app with nothing
6+
-- to catch it.
7+
--
8+
-- This should touch zero rows: migration 000344 added the column with a default
9+
-- of 'confidential' and backfilled existing rows with COALESCE.
10+
UPDATE oauth2_provider_apps SET client_type = 'confidential' WHERE client_type IS NULL;
11+
12+
ALTER TABLE oauth2_provider_apps
13+
ADD CONSTRAINT oauth2_provider_apps_client_type_check
14+
CHECK (client_type IN ('confidential', 'public'));
15+
16+
ALTER TABLE oauth2_provider_apps
17+
ALTER COLUMN client_type SET NOT NULL;

‎coderd/database/modelmethods.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -690,7 +690,7 @@ func (OAuth2ProviderApp) RBACObject() rbac.Object {
690690
// An unset or unrecognized client type reads as confidential, so an app can
691691
// never skip client authentication by accident.
692692
func (a OAuth2ProviderApp) IsPublic() bool {
693-
return a.ClientType.String == OAuth2ProviderAppClientTypePublic
693+
return a.ClientType == OAuth2ProviderAppClientTypePublic
694694
}
695695

696696
func (a GetOAuth2ProviderAppsByUserIDRow) RBACObject() rbac.Object {

‎coderd/database/modelmethods_internal_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,37 @@ func TestWorkspaceACLDisabled(t *testing.T) {
221221
})
222222
}
223223

224+
// TestOAuth2ProviderAppIsPublic pins IsPublic's contract directly, since it is
225+
// what decides whether the token endpoint validates a client secret at all.
226+
// Only the exact string "public" may read as public: anything else, including
227+
// an unset column or a differently-cased value, must read as confidential so
228+
// that a garbled value cannot silently skip client authentication.
229+
func TestOAuth2ProviderAppIsPublic(t *testing.T) {
230+
t.Parallel()
231+
232+
tests := []struct {
233+
clientType string
234+
want bool
235+
}{
236+
{clientType: "public", want: true},
237+
{clientType: "confidential", want: false},
238+
{clientType: "", want: false},
239+
{clientType: "Public", want: false},
240+
{clientType: "PUBLIC", want: false},
241+
{clientType: " public", want: false},
242+
{clientType: "public ", want: false},
243+
{clientType: "bogus", want: false},
244+
}
245+
246+
for _, tt := range tests {
247+
t.Run(tt.clientType, func(t *testing.T) {
248+
t.Parallel()
249+
app := OAuth2ProviderApp{ClientType: tt.clientType}
250+
require.Equal(t, tt.want, app.IsPublic())
251+
})
252+
}
253+
}
254+
224255
// Helpers
225256
func requirePermission(t *testing.T, s rbac.Scope, resource string, action policy.Action) {
226257
t.Helper()

0 commit comments

Comments
 (0)