fix(server): require confidential client auth in authorization_code grant (#22548)
## Summary
Closes a **confidential-client authentication bypass** in the OAuth
`authorization_code` grant.
`OAuthService.exchangeAuthorizationCode` only validated `client_secret`
**when one was supplied** (`if (clientSecret)`), and the fallback check
at the end (`if (!clientSecret && !storedCodeChallenge)`) treats a valid
PKCE `code_verifier` as sufficient to complete the exchange. As a
result, a **confidential client** — one registered with a
`client_secret` (`oAuthClientSecretHash` set) — could have its
authorization codes redeemed using PKCE alone, with **no client
authentication**.
PKCE is defense-in-depth for public clients; it is not a substitute for
authenticating a confidential client (RFC 6749 §4.1.3, OAuth 2.1
§4.1.3). The `refresh_token` grant already enforces this exact rule —
this PR mirrors that gate in the `authorization_code` grant so any
client issued a secret must always present it.
## The fix
```ts
// Confidential clients (those issued a secret) must always authenticate,
// even when PKCE is used.
if (applicationRegistration.oAuthClientSecretHash && !clientSecret) {
return this.errorResponse(
'invalid_client',
'Client authentication required for confidential clients',
);
}
```
The check runs immediately after client resolution and before the
authorization code is even looked up. Public (PKCE-only) clients — those
without a stored secret hash — are unaffected.
## Testing
Added `oauth.service.spec.ts` covering:
- **Regression:** a confidential client presenting only PKCE and no
`client_secret` is rejected with `invalid_client` before any code
lookup.
- A wrong `client_secret` for a confidential client is still rejected.
- A public (PKCE) client is **not** blocked by the new gate and proceeds
to the code lookup.
Verified the regression test fails without the fix and passes with it.
Existing `application-oauth` suites remain green (8/8). Lint (`oxlint
--type-aware`, `oxfmt`) clean; the touched files typecheck.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/twentyhq/twenty/pull/22548?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
This commit is contained in:
+7
@@ -70,6 +70,13 @@ export class OAuthService {
|
||||
|
||||
const applicationRegistration = clientValidation;
|
||||
|
||||
if (applicationRegistration.oAuthClientSecretHash && !clientSecret) {
|
||||
return this.errorResponse(
|
||||
'invalid_client',
|
||||
'Client authentication required for confidential clients',
|
||||
);
|
||||
}
|
||||
|
||||
if (clientSecret) {
|
||||
const secretError = await this.validateClientSecret(
|
||||
applicationRegistration,
|
||||
|
||||
@@ -29,7 +29,7 @@ const insertRegistration = async (
|
||||
ds: DataSource,
|
||||
params: {
|
||||
name: string;
|
||||
clientSecretHash: string;
|
||||
clientSecretHash: string | null;
|
||||
redirectUris: string[];
|
||||
scopes: string[];
|
||||
},
|
||||
@@ -129,6 +129,8 @@ describe('OAuth (integration)', () => {
|
||||
let testClientSecret: string;
|
||||
let testApplication: TestApplication;
|
||||
|
||||
let publicRegistration: TestRegistration;
|
||||
|
||||
let autoInstallRegistration: TestRegistration;
|
||||
let autoInstallClientSecret: string;
|
||||
|
||||
@@ -159,6 +161,22 @@ describe('OAuth (integration)', () => {
|
||||
applicationRegistrationId: testRegistration.id,
|
||||
});
|
||||
|
||||
publicRegistration = await insertRegistration(ds, {
|
||||
name: 'OAuth Public PKCE Test App',
|
||||
clientSecretHash: null,
|
||||
redirectUris: ['https://example.com/callback'],
|
||||
scopes: ['read', 'write'],
|
||||
});
|
||||
createdEntityIds.registrations.push(publicRegistration.id);
|
||||
|
||||
const publicApplication = await insertApplication(ds, {
|
||||
universalIdentifier: publicRegistration.universalIdentifier,
|
||||
name: publicRegistration.name,
|
||||
workspaceId: TEST_WORKSPACE_ID,
|
||||
applicationRegistrationId: publicRegistration.id,
|
||||
});
|
||||
createdEntityIds.applications.push(publicApplication.id);
|
||||
|
||||
autoInstallClientSecret = crypto.randomBytes(32).toString('hex');
|
||||
const autoInstallSecretHash = await bcrypt.hash(
|
||||
autoInstallClientSecret,
|
||||
@@ -430,13 +448,13 @@ describe('OAuth (integration)', () => {
|
||||
|
||||
it('should require either client_secret or code_verifier', async () => {
|
||||
const code = await createAuthorizationCode(
|
||||
testRegistration.oAuthClientId,
|
||||
publicRegistration.oAuthClientId,
|
||||
);
|
||||
|
||||
const res = await postToken({
|
||||
grant_type: 'authorization_code',
|
||||
code,
|
||||
client_id: testRegistration.oAuthClientId,
|
||||
client_id: publicRegistration.oAuthClientId,
|
||||
redirect_uri: 'https://example.com/callback',
|
||||
}).expect(400);
|
||||
|
||||
@@ -479,13 +497,13 @@ describe('OAuth (integration)', () => {
|
||||
|
||||
it('should exchange code with valid PKCE verifier', async () => {
|
||||
const { code, codeVerifier } = await createAuthCodeWithPkce(
|
||||
testRegistration.oAuthClientId,
|
||||
publicRegistration.oAuthClientId,
|
||||
);
|
||||
|
||||
const res = await postToken({
|
||||
grant_type: 'authorization_code',
|
||||
code,
|
||||
client_id: testRegistration.oAuthClientId,
|
||||
client_id: publicRegistration.oAuthClientId,
|
||||
code_verifier: codeVerifier,
|
||||
redirect_uri: 'https://example.com/callback',
|
||||
}).expect(200);
|
||||
@@ -497,13 +515,13 @@ describe('OAuth (integration)', () => {
|
||||
|
||||
it('should reject code with wrong PKCE verifier', async () => {
|
||||
const { code } = await createAuthCodeWithPkce(
|
||||
testRegistration.oAuthClientId,
|
||||
publicRegistration.oAuthClientId,
|
||||
);
|
||||
|
||||
const res = await postToken({
|
||||
grant_type: 'authorization_code',
|
||||
code,
|
||||
client_id: testRegistration.oAuthClientId,
|
||||
client_id: publicRegistration.oAuthClientId,
|
||||
code_verifier: 'wrong-verifier',
|
||||
redirect_uri: 'https://example.com/callback',
|
||||
}).expect(400);
|
||||
@@ -513,6 +531,22 @@ describe('OAuth (integration)', () => {
|
||||
|
||||
it('should require code_verifier when PKCE was used in authorization', async () => {
|
||||
const { code } = await createAuthCodeWithPkce(
|
||||
publicRegistration.oAuthClientId,
|
||||
);
|
||||
|
||||
const res = await postToken({
|
||||
grant_type: 'authorization_code',
|
||||
code,
|
||||
client_id: publicRegistration.oAuthClientId,
|
||||
redirect_uri: 'https://example.com/callback',
|
||||
}).expect(400);
|
||||
|
||||
expect(res.body.error).toBe('invalid_request');
|
||||
expect(res.body.error_description).toContain('code_verifier is required');
|
||||
});
|
||||
|
||||
it('should reject a confidential client that presents only PKCE and no client_secret', async () => {
|
||||
const { code, codeVerifier } = await createAuthCodeWithPkce(
|
||||
testRegistration.oAuthClientId,
|
||||
);
|
||||
|
||||
@@ -520,12 +554,11 @@ describe('OAuth (integration)', () => {
|
||||
grant_type: 'authorization_code',
|
||||
code,
|
||||
client_id: testRegistration.oAuthClientId,
|
||||
client_secret: testClientSecret,
|
||||
code_verifier: codeVerifier,
|
||||
redirect_uri: 'https://example.com/callback',
|
||||
}).expect(400);
|
||||
}).expect(401);
|
||||
|
||||
expect(res.body.error).toBe('invalid_request');
|
||||
expect(res.body.error_description).toContain('code_verifier is required');
|
||||
expect(res.body.error).toBe('invalid_client');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user