OAuth security hardening: RFC compliance, PKCE binding, rate limiting (#18305)

## Summary Follow-up to #18267. Hardens the OAuth implementation with security fixes identified during audit: **P0 — Critical:** - Bind authorization codes to `client_id` in context to prevent auth code injection (RFC 6749 §4.1.3) - Store PKCE `code_challenge` directly in auth code context instead of a separate `CodeChallenge` token — cryptographically binds the challenge to its code - Enforce `code_verifier` when `code_challenge` was used during authorization - Hash authorization codes (SHA-256) before storage to prevent exposure if DB is compromised - Add `Cache-Control: no-store` + `Pragma: no-cache` headers on token responses (RFC 6749 §5.1) - Add rate limiting on `/oauth/token` endpoint (20 req/min per client via existing `ThrottlerService`) **P1 — High:** - Return HTTP 401 for `invalid_client` errors instead of 400 (RFC 6749 §5.2) - Verify refresh tokens belong to the presenting client (cross-client token theft prevention) - Limit fields exposed by public `findApplicationRegistrationByClientId` query to only what the frontend needs (`id`, `name`, `logoUrl`, `websiteUrl`, `oAuthScopes`) - Require `API_KEYS_AND_WEBHOOKS` permission for `createApplicationRegistration` mutation **P2/P3 — Medium/Low:** - Add error handling and loading states to frontend Authorize page - Rename redirect URL param from `authorizationCode` to `code` (RFC standard) - Add unit tests for `validateRedirectUri` utility (8 test cases) ## Test plan - [ ] Existing OAuth integration tests updated for all changes (hashed codes, context-based PKCE, client binding, 401 status codes, cache headers) - [ ] New test: auth code rejected when presented by a different client - [ ] New test: refresh token rejected when presented by a different client - [ ] New test: `code_verifier` required when PKCE was used in authorization - [ ] New test: `Cache-Control: no-store` header present on responses - [ ] New unit tests for `validateRedirectUri` (HTTPS, localhost, fragments, invalid URIs) - [ ] Verify frontend authorize page shows errors gracefully Made with [Cursor](https://cursor.com)
2026-03-02 12:21:26 +01:00
parent d021f7e369
commit 1a8be234de
24 changed files with 1697 additions and 168 deletions
@@ -1,6 +1,6 @@
 import { Field, ArgsType } from '@nestjs/graphql';

-import { IsNotEmpty, IsOptional, IsString } from 'class-validator';
+import { IsNotEmpty, IsOptional, IsString, MaxLength } from 'class-validator';

@ArgsType()
 export class AuthorizeAppInput {
@@ -11,10 +11,24 @@ export class AuthorizeAppInput {

  @Field(() => String, { nullable: true })
  @IsString()
+  @MaxLength(256)
  @IsOptional()
  codeChallenge?: string;

  @Field(() => String)
  @IsString()
+  @MaxLength(2048)
  redirectUrl: string;
+
+  @Field(() => String, { nullable: true })
+  @IsString()
+  @MaxLength(1024)
+  @IsOptional()
+  state?: string;
+
+  @Field(() => String, { nullable: true })
+  @IsString()
+  @MaxLength(1024)
+  @IsOptional()
+  scope?: string;
 }
@@ -526,6 +526,27 @@ export class AuthService {
      );
    }

+    // Validate requested scopes are a subset of the registration's allowed scopes
+    const parsedScopes = authorizeAppInput.scope
+      ? authorizeAppInput.scope.split(' ').filter(Boolean)
+      : [];
+
+    const requestedScopes =
+      parsedScopes.length > 0
+        ? parsedScopes
+        : applicationRegistration.oAuthScopes;
+
+    const invalidScopes = requestedScopes.filter(
+      (scope) => !applicationRegistration.oAuthScopes.includes(scope),
+    );
+
+    if (invalidScopes.length > 0) {
+      throw new AuthException(
+        `Invalid scopes: ${invalidScopes.join(', ')}`,
+        AuthExceptionCode.FORBIDDEN_EXCEPTION,
+      );
+    }
+
    const redirectUriValidation = validateRedirectUri(
      authorizeAppInput.redirectUrl,
    );
@@ -538,49 +559,40 @@ export class AuthService {
    }

    const authorizationCode = crypto.randomBytes(42).toString('hex');
+    const hashedAuthorizationCode = crypto
+      .createHash('sha256')
+      .update(authorizationCode)
+      .digest('hex');

    const expiresAt = addMilliseconds(new Date().getTime(), ms('5m'));

-    const authCodeContext = { redirectUri: authorizeAppInput.redirectUrl };
+    const authCodeContext = {
+      redirectUri: authorizeAppInput.redirectUrl,
+      clientId: applicationRegistration.oAuthClientId,
+      scope: requestedScopes.join(' '),
+      ...(codeChallenge ? { codeChallenge } : {}),
+    };

-    if (codeChallenge) {
-      const tokens = this.appTokenRepository.create([
-        {
-          value: codeChallenge,
-          type: AppTokenType.CodeChallenge,
-          userId: user.id,
-          workspaceId: workspace.id,
-          expiresAt,
-        },
-        {
-          value: authorizationCode,
-          type: AppTokenType.AuthorizationCode,
-          userId: user.id,
-          workspaceId: workspace.id,
-          expiresAt,
-          context: authCodeContext,
-        },
-      ]);
+    const token = this.appTokenRepository.create({
+      value: hashedAuthorizationCode,
+      type: AppTokenType.AuthorizationCode,
+      userId: user.id,
+      workspaceId: workspace.id,
+      expiresAt,
+      context: authCodeContext,
+    });

-      await this.appTokenRepository.save(tokens);
-    } else {
-      const token = this.appTokenRepository.create({
-        value: authorizationCode,
-        type: AppTokenType.AuthorizationCode,
-        userId: user.id,
-        workspaceId: workspace.id,
-        expiresAt,
-        context: authCodeContext,
-      });
+    await this.appTokenRepository.save(token);

-      await this.appTokenRepository.save(token);
+    redirectUriValidation.parsed.searchParams.set('code', authorizationCode);
+
+    if (authorizeAppInput.state) {
+      redirectUriValidation.parsed.searchParams.set(
+        'state',
+        authorizeAppInput.state,
+      );
    }

-    redirectUriValidation.parsed.searchParams.set(
-      'authorizationCode',
-      authorizationCode,
-    );
-
    return { redirectUrl: redirectUriValidation.parsed.toString() };
  }

@@ -148,6 +148,47 @@ export class ApplicationTokenService {
    }
  }

+  validateApplicationAccessToken(
+    token: string,
+  ): ApplicationAccessTokenJwtPayload {
+    try {
+      this.jwtWrapperService.verifyJwtToken(token);
+
+      const payload =
+        this.jwtWrapperService.decode<ApplicationAccessTokenJwtPayload>(token, {
+          json: true,
+        });
+
+      if (payload.type !== JwtTokenTypeEnum.APPLICATION_ACCESS) {
+        throw new AuthException(
+          'Expected an application access token',
+          AuthExceptionCode.INVALID_JWT_TOKEN_TYPE,
+        );
+      }
+
+      return payload;
+    } catch (error) {
+      if (error instanceof AuthException) {
+        throw error;
+      }
+
+      throw new AuthException(
+        'Invalid application access token',
+        AuthExceptionCode.UNAUTHENTICATED,
+      );
+    }
+  }
+
+  decodeToken(token: string): (
+    | ApplicationAccessTokenJwtPayload
+    | ApplicationRefreshTokenJwtPayload
+  ) & {
+    exp?: number;
+    iat?: number;
+  } {
+    return this.jwtWrapperService.decode(token, { json: true });
+  }
+
  async renewApplicationTokens(payload: {
    workspaceId: string;
    applicationId: string;
@@ -0,0 +1,69 @@
+import { validateRedirectUri } from 'src/engine/core-modules/auth/utils/validate-redirect-uri.util';
+
+describe('validateRedirectUri', () => {
+  it('should accept a valid HTTPS URI', () => {
+    const result = validateRedirectUri('https://example.com/callback');
+
+    expect(result.valid).toBe(true);
+
+    if (result.valid) {
+      expect(result.parsed.href).toBe('https://example.com/callback');
+    }
+  });
+
+  it('should accept localhost HTTP', () => {
+    const result = validateRedirectUri('http://localhost:3000/callback');
+
+    expect(result.valid).toBe(true);
+  });
+
+  it('should accept 127.0.0.1 HTTP', () => {
+    const result = validateRedirectUri('http://127.0.0.1:8080/callback');
+
+    expect(result.valid).toBe(true);
+  });
+
+  it('should reject non-HTTPS non-localhost URIs', () => {
+    const result = validateRedirectUri('http://example.com/callback');
+
+    expect(result.valid).toBe(false);
+
+    if (!result.valid) {
+      expect(result.reason).toContain('HTTPS');
+    }
+  });
+
+  it('should reject URIs with fragments', () => {
+    const result = validateRedirectUri('https://example.com/callback#section');
+
+    expect(result.valid).toBe(false);
+
+    if (!result.valid) {
+      expect(result.reason).toContain('fragments');
+    }
+  });
+
+  it('should reject invalid URIs', () => {
+    const result = validateRedirectUri('not-a-url');
+
+    expect(result.valid).toBe(false);
+
+    if (!result.valid) {
+      expect(result.reason).toContain('Invalid redirect URI');
+    }
+  });
+
+  it('should accept HTTPS with query parameters', () => {
+    const result = validateRedirectUri(
+      'https://example.com/callback?state=abc',
+    );
+
+    expect(result.valid).toBe(true);
+  });
+
+  it('should accept HTTPS with port', () => {
+    const result = validateRedirectUri('https://example.com:8443/callback');
+
+    expect(result.valid).toBe(true);
+  });
+});