Skip to content

Code review: Critical security and validation gaps in OAuth implementation#592

Closed
Copilot wants to merge 1 commit intofeat/atmn-v2-oauthfrom
copilot/sub-pr-570
Closed

Code review: Critical security and validation gaps in OAuth implementation#592
Copilot wants to merge 1 commit intofeat/atmn-v2-oauthfrom
copilot/sub-pr-570

Conversation

Copy link

Copilot AI commented Jan 19, 2026

Comprehensive security review of PR #570's OAuth 2.1/OpenID implementation and admin role-based auth migration identified critical authorization and input validation gaps.

Critical Security Issues

Missing scope validation in CLI endpoint

/cli/api-keys issues API keys without verifying OAuth token scopes. Any valid token can create keys regardless of granted permissions.

Location: server/src/internal/dev/cli/handlers/handleCreateOAuthApiKeys.ts

Required fix:

const REQUIRED_SCOPE = "apiKeys:create";
if (!tokenRecord.scopes?.includes(REQUIRED_SCOPE)) {
    throw new RecaseError({
        message: "Token lacks required scope",
        code: ErrCode.Forbidden,
        statusCode: 403,
    });
}

Open redirect vulnerability

OAuth callback accepts user-provided redirect_uri from Redis state without whitelist validation.

Location: server/src/internal/auth/handlers/handleOAuthCallback.ts:59

Required fix: Validate against client's registered URIs before redirecting.

Incomplete admin auth migration

Hardcoded ADMIN_USER_IDs still present in server/src/utils/constants.ts and vite/src/views/admin/hooks/useAdmin.tsx despite role-based auth migration. Creates privilege escalation path if database role field is compromised.

Required action: Remove all hardcoded user ID references; rely solely on user.role field.

High Priority Issues

  • Input validation gaps: Redirect URIs, consent IDs, and client names lack format validation
  • XSS risk: Organization logos (vite/src/views/auth/Consent.tsx:95) rendered without URL scheme validation—could accept javascript: URIs
  • Component inconsistency: EditOAuthClientDialog.tsx uses deprecated @/components/ui/textarea instead of v2 components

Medium Priority

  • Missing rate limiting on OAuth token exchange and consent endpoints
  • No audit logging for security-sensitive operations (consent revocation, OAuth key creation)
  • Accessibility gaps: missing aria-label and aria-describedby associations

Implementation Strengths

  • ✅ Proper SHA-256 token hashing with base64url encoding
  • ✅ Token expiration validation throughout
  • ✅ Database RLS enabled on all OAuth tables
  • ✅ Well-structured scope definition system with resource:action format
  • ✅ Atomic consent revocation with cascade deletion

Recommendation

Block merge until scope validation, redirect URI whitelist validation, and hardcoded admin ID removal are complete. Other issues can be addressed in follow-up PRs.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.


Summary by cubic

Introduces OAuth 2.1 sign‑in and a CLI flow to create and manage API keys. Improves security and simplifies auth while keeping existing keys working for now.

  • New Features
    • OAuth 2.1 with PKCE and scopes; supports token refresh and revocation.
    • Server routes for authorize, callback, token exchange, and API key create/list/revoke.
    • CLI commands to log in and manage API keys; opens browser and stores credentials securely.
    • Backwards compatible with current API keys during rollout.

Written for commit bd29694. Summary will update on new commits.

@vercel
Copy link

vercel bot commented Jan 19, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
autumn-vite Ready Ready Preview, Comment Jan 19, 2026 1:14pm

Request Review

Copilot AI changed the title [WIP] Add OAuth 2.1 and CLI flow for API key management Code review: Critical security and validation gaps in OAuth implementation Jan 19, 2026
Copilot AI requested a review from SirTenzin January 19, 2026 13:23
@johnyeocx johnyeocx closed this Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants