From 99f99adf8fa7b64ea6e31f52399374abfa72a432 Mon Sep 17 00:00:00 2001 From: Charles Bochet Date: Sat, 4 Jul 2026 20:36:03 +0200 Subject: [PATCH] fix(server): require confidential client auth in authorization_code grant (#22548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. Review in cubic --- .../application-oauth/oauth.service.ts | 7 +++ .../oauth/suites/oauth.integration-spec.ts | 55 +++++++++++++++---- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/application/application-oauth/oauth.service.ts b/packages/twenty-server/src/engine/core-modules/application/application-oauth/oauth.service.ts index df4a3bb4d0..148d483591 100644 --- a/packages/twenty-server/src/engine/core-modules/application/application-oauth/oauth.service.ts +++ b/packages/twenty-server/src/engine/core-modules/application/application-oauth/oauth.service.ts @@ -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, diff --git a/packages/twenty-server/test/integration/oauth/suites/oauth.integration-spec.ts b/packages/twenty-server/test/integration/oauth/suites/oauth.integration-spec.ts index 2953f05cb6..f3aa4adb19 100644 --- a/packages/twenty-server/test/integration/oauth/suites/oauth.integration-spec.ts +++ b/packages/twenty-server/test/integration/oauth/suites/oauth.integration-spec.ts @@ -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'); }); });