Feat/OIDC login v3#9
Conversation
|
The i18n check failed because translation messages are out of sync. This usually happens when you've added or modified translation strings in your code but haven't updated the translation file. Please run |
| authRoutes.post('/oidc/callback/:slug', async (req, res, next) => { | ||
| const settings = getSettings(); | ||
| const provider = settings.oidc.providers.find( | ||
| (p) => p.slug === req.params.slug | ||
| ); | ||
|
|
||
| if (!settings.main.oidcLogin || !provider) { | ||
| return next({ | ||
| status: 403, | ||
| error: ApiErrorCode.Unauthorized, | ||
| }); | ||
| } | ||
|
|
||
| let config: openIdClient.Configuration; | ||
| try { | ||
| config = await openIdClient.discovery( | ||
| new URL(provider.issuerUrl), | ||
| provider.clientId, | ||
| provider.clientSecret, | ||
| undefined, | ||
| { | ||
| execute: | ||
| process.env.OIDC_ALLOW_INSECURE === 'true' | ||
| ? [openIdClient.allowInsecureRequests] | ||
| : [], | ||
| } | ||
| ); | ||
| } catch (error) { | ||
| logger.error('Failed OIDC provider discovery', { | ||
| label: 'Auth', | ||
| provider: provider.name, | ||
| ip: req.ip, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcProviderDiscoveryFailed, | ||
| }); | ||
| } | ||
|
|
||
| const pkceCodeVerifier = req.cookies['oidc-code-verifier']; | ||
| const expectedState = req.cookies['oidc-state']; | ||
|
|
||
| const redirectUrl = new URL(req.body.callbackUrl); | ||
|
|
||
| let tokens: openIdClient.TokenEndpointResponse & | ||
| openIdClient.TokenEndpointResponseHelpers; | ||
| try { | ||
| tokens = await openIdClient.authorizationCodeGrant(config, redirectUrl, { | ||
| pkceCodeVerifier, | ||
| expectedState, | ||
| }); | ||
| } catch (error) { | ||
| logger.error('Failed OIDC authorization code grant', { | ||
| label: 'Auth', | ||
| provider: provider.name, | ||
| ip: req.ip, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcAuthorizationFailed, | ||
| }); | ||
| } | ||
|
|
||
| const claims = tokens.claims(); | ||
| if (claims == null) { | ||
| logger.info('Failed OIDC login attempt', { | ||
| cause: | ||
| 'Missing ID token in response. Provider does not support OpenID Connect.', | ||
| ip: req.ip, | ||
| provider: provider.name, | ||
| }); | ||
|
|
||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcAuthorizationFailed, | ||
| }); | ||
| } | ||
|
|
||
| const requiredClaims = (provider.requiredClaims ?? '') | ||
| .split(' ') | ||
| .filter((s) => !!s); | ||
|
|
||
| let fullUserInfo: openIdClient.IDToken & openIdClient.UserInfoResponse = | ||
| claims; | ||
|
|
||
| if (config.serverMetadata().userinfo_endpoint) { | ||
| try { | ||
| const userInfo = await openIdClient.fetchUserInfo( | ||
| config, | ||
| tokens.access_token, | ||
| claims.sub | ||
| ); | ||
| fullUserInfo = { ...claims, ...userInfo }; | ||
| } catch (error) { | ||
| logger.error('Failed to fetch OIDC user info', { | ||
| label: 'Auth', | ||
| provider: provider.name, | ||
| ip: req.ip, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcAuthorizationFailed, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Validate that user meets required claims | ||
| const hasRequiredClaims = requiredClaims.every((claim) => { | ||
| const value = fullUserInfo[claim]; | ||
| return value === true; | ||
| }); | ||
|
|
||
| if (!hasRequiredClaims) { | ||
| logger.info('Failed OIDC login attempt', { | ||
| cause: 'Failed to validate required claims', | ||
| ip: req.ip, | ||
| requiredClaims: provider.requiredClaims, | ||
| }); | ||
| return next({ | ||
| status: 403, | ||
| error: ApiErrorCode.Unauthorized, | ||
| }); | ||
| } | ||
|
|
||
| // Map identifier to linked account | ||
| const userRepository = getRepository(User); | ||
| const linkedAccountsRepository = getRepository(LinkedAccount); | ||
|
|
||
| const linkedAccount = await linkedAccountsRepository.findOne({ | ||
| relations: { | ||
| user: true, | ||
| }, | ||
| where: { | ||
| provider: provider.slug, | ||
| sub: fullUserInfo.sub, | ||
| }, | ||
| }); | ||
| let user = linkedAccount?.user; | ||
|
|
||
| // If there is already a user logged in, handle account linking | ||
| if (req.user != null) { | ||
| // Check if this OIDC account is already linked to a different user | ||
| if (linkedAccount != null && linkedAccount.user.id !== req.user.id) { | ||
| logger.warn('Failed OIDC account linking attempt', { | ||
| cause: 'Account is already linked to a different user', | ||
| ip: req.ip, | ||
| provider: provider.slug, | ||
| currentUserId: req.user.id, | ||
| linkedUserId: linkedAccount.user.id, | ||
| }); | ||
| return next({ | ||
| status: 409, | ||
| error: ApiErrorCode.OidcAccountAlreadyLinked, | ||
| }); | ||
| } | ||
|
|
||
| // If no linked account exists, link the account | ||
| if (linkedAccount == null) { | ||
| const newLinkedAccount = new LinkedAccount({ | ||
| user: req.user, | ||
| provider: provider.slug, | ||
| sub: fullUserInfo.sub, | ||
| username: fullUserInfo.preferred_username ?? req.user.displayName, | ||
| }); | ||
|
|
||
| await linkedAccountsRepository.save(newLinkedAccount); | ||
| } | ||
|
|
||
| return res.sendStatus(204); | ||
| } | ||
|
|
||
| // Create user if one doesn't already exist | ||
| if (!user && fullUserInfo.email != null && provider.newUserLogin) { | ||
| // Check if a user with this email already exists | ||
| const existingUser = await userRepository.findOne({ | ||
| where: { email: fullUserInfo.email }, | ||
| }); | ||
|
|
||
| if (existingUser) { | ||
| return next({ | ||
| status: 403, | ||
| error: ApiErrorCode.Unauthorized, | ||
| }); | ||
| } | ||
|
|
||
| logger.info(`Creating user for ${fullUserInfo.email}`, { | ||
| ip: req.ip, | ||
| email: fullUserInfo.email, | ||
| }); | ||
|
|
||
| const avatar = | ||
| fullUserInfo.picture ?? | ||
| gravatarUrl(fullUserInfo.email, { default: 'mm', size: 200 }); | ||
| user = new User({ | ||
| avatar: avatar, | ||
| username: fullUserInfo.preferred_username, | ||
| email: fullUserInfo.email, | ||
| permissions: settings.main.defaultPermissions, | ||
| plexToken: '', | ||
| userType: UserType.LOCAL, | ||
| }); | ||
| await userRepository.save(user); | ||
|
|
||
| const linkedAccount = new LinkedAccount({ | ||
| user, | ||
| provider: provider.slug, | ||
| sub: fullUserInfo.sub, | ||
| username: fullUserInfo.preferred_username ?? fullUserInfo.email, | ||
| }); | ||
| await linkedAccountsRepository.save(linkedAccount); | ||
|
|
||
| user.linkedAccounts = [linkedAccount]; | ||
| await userRepository.save(user); | ||
| } | ||
|
|
||
| if (!user) { | ||
| logger.debug('Failed OIDC sign-up attempt', { | ||
| cause: provider.newUserLogin | ||
| ? 'User did not have an account, and was missing an associated email address.' | ||
| : 'User did not have an account, and new user login was disabled.', | ||
| }); | ||
| return next({ | ||
| status: provider.newUserLogin ? 400 : 403, | ||
| error: provider.newUserLogin | ||
| ? ApiErrorCode.OidcMissingEmail | ||
| : ApiErrorCode.Unauthorized, | ||
| }); | ||
| } | ||
|
|
||
| // Set logged in session and return | ||
| if (req.session) { | ||
| req.session.userId = user.id; | ||
| } | ||
|
|
||
| // Success! | ||
| return res.sendStatus(204); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, to fix missing rate limiting on an authorization endpoint in an Express app, you add a rate-limiting middleware (e.g., from express-rate-limit) and apply it specifically to that route (or to the whole router) so that each client can only invoke the endpoint a bounded number of times per time window. This reduces the risk of denial-of-service attacks and brute-force attempts without changing the core business logic of the handler.
For this codebase, the minimal, targeted fix is to: (1) import express-rate-limit at the top of server/routes/auth.ts; (2) define a rate limiter configuration dedicated to OIDC callback requests (e.g., limiting per IP to a modest number of requests per 15 minutes); and (3) apply this limiter as middleware on the /oidc/callback/:slug route while leaving all existing handler logic untouched. We will place the limiter definition near the router instantiation so it can be reused if needed and chain it into the existing authRoutes.post('/oidc/callback/:slug', ...) line as an additional middleware argument.
Concretely:
- Add
import rateLimit from 'express-rate-limit';alongside existing imports. - After
const authRoutes = Router();, defineconst oidcCallbackLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100, standardHeaders: true, legacyHeaders: false });(values chosen as a reasonable default, preserving functionality while limiting abuse). - Modify the route definition on line 755 from
authRoutes.post('/oidc/callback/:slug', async (req, res, next) => {toauthRoutes.post('/oidc/callback/:slug', oidcCallbackLimiter, async (req, res, next) => {.
This keeps all existing behavior intact for normal users while adding protection against excessive invocation of the OIDC callback.
| @@ -21,9 +21,17 @@ | ||
| import net from 'net'; | ||
| import * as openIdClient from 'openid-client'; | ||
| import validator from 'validator'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| const authRoutes = Router(); | ||
|
|
||
| const oidcCallbackLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 OIDC callback requests per window | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| authRoutes.get('/me', isAuthenticated(), async (req, res) => { | ||
| const userRepository = getRepository(User); | ||
| if (!req.user) { | ||
| @@ -752,7 +757,7 @@ | ||
| }); | ||
| }); | ||
|
|
||
| authRoutes.post('/oidc/callback/:slug', async (req, res, next) => { | ||
| authRoutes.post('/oidc/callback/:slug', oidcCallbackLimiter, async (req, res, next) => { | ||
| const settings = getSettings(); | ||
| const provider = settings.oidc.providers.find( | ||
| (p) => p.slug === req.params.slug |
| authRoutes.get('/oidc/login/:slug', async (req, res, next) => { | ||
| const settings = getSettings(); | ||
| const provider = settings.oidc.providers.find( | ||
| (p) => p.slug === req.params.slug | ||
| ); | ||
|
|
||
| if (!settings.main.oidcLogin || !provider) { | ||
| return next({ | ||
| status: 403, | ||
| error: ApiErrorCode.Unauthorized, | ||
| }); | ||
| } | ||
|
|
||
| let config: openIdClient.Configuration; | ||
| try { | ||
| config = await openIdClient.discovery( | ||
| new URL(provider.issuerUrl), | ||
| provider.clientId, | ||
| provider.clientSecret, | ||
| undefined, | ||
| { | ||
| execute: | ||
| process.env.OIDC_ALLOW_INSECURE === 'true' | ||
| ? [openIdClient.allowInsecureRequests] | ||
| : [], | ||
| } | ||
| ); | ||
| } catch (error) { | ||
| logger.error('Failed OIDC provider discovery', { | ||
| label: 'Auth', | ||
| provider: provider.name, | ||
| ip: req.ip, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcProviderDiscoveryFailed, | ||
| }); | ||
| } | ||
|
|
||
| const code_verifier = openIdClient.randomPKCECodeVerifier(); | ||
| const code_challenge = | ||
| await openIdClient.calculatePKCECodeChallenge(code_verifier); | ||
| res.cookie('oidc-code-verifier', code_verifier, { | ||
| maxAge: 60000, | ||
| httpOnly: true, | ||
| secure: req.protocol === 'https', | ||
| }); | ||
|
|
||
| const callbackUrl = getOidcRedirectUrl(req); | ||
|
|
||
| const parameters: Record<string, string> = { | ||
| redirect_uri: callbackUrl.toString(), | ||
| scope: provider.scopes ?? 'openid profile email', | ||
| code_challenge, | ||
| code_challenge_method: 'S256', | ||
| }; | ||
|
|
||
| /** | ||
| * We cannot be sure the server supports PKCE so we're going to use state too. | ||
| * Use of PKCE is backwards compatible even if the AS doesn't support it which | ||
| * is why we're using it regardless. Like PKCE, random state must be generated | ||
| * for every redirect to the authorization_endpoint. | ||
| */ | ||
| if (!config.serverMetadata().supportsPKCE()) { | ||
| const state = openIdClient.randomState(); | ||
| parameters.state = state; | ||
| res.cookie('oidc-state', state, { | ||
| maxAge: 60000, | ||
| httpOnly: true, | ||
| secure: req.protocol === 'https', | ||
| }); | ||
| } | ||
|
|
||
| let redirectUrl: URL; | ||
| try { | ||
| redirectUrl = openIdClient.buildAuthorizationUrl(config, parameters); | ||
| } catch (error) { | ||
| logger.error('Failed to build OIDC authorization URL', { | ||
| label: 'Auth', | ||
| provider: provider.name, | ||
| ip: req.ip, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| return next({ | ||
| status: 500, | ||
| error: ApiErrorCode.OidcAuthorizationFailed, | ||
| }); | ||
| } | ||
|
|
||
| return res.status(200).json({ | ||
| redirectUrl, | ||
| }); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, the fix is to introduce rate limiting middleware for routes that perform authentication/authorization, especially those that may trigger remote calls or other expensive operations. In an Express app, the common approach is to use express-rate-limit to cap the number of requests per IP over a time window.
For this specific code, the best low-impact fix is to:
- Import
express-rate-limitinserver/routes/auth.ts. - Define a limiter specifically for OIDC login, with reasonable defaults (e.g., 10 requests per 15 minutes per IP).
- Apply that limiter to the
/oidc/login/:slugroute by passing it as middleware before the async handler.
This avoids changing the existing logic of the route while protecting it from brute-force or DoS-style abuse. All changes are confined to server/routes/auth.ts: add one import, add a const oidcLoginLimiter = rateLimit({ ... }) near the top (after router creation is a natural spot), and change the route definition from authRoutes.get('/oidc/login/:slug', async (req, res, next) => { ... }) to authRoutes.get('/oidc/login/:slug', oidcLoginLimiter, async (req, res, next) => { ... }).
| @@ -21,9 +21,17 @@ | ||
| import net from 'net'; | ||
| import * as openIdClient from 'openid-client'; | ||
| import validator from 'validator'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| const authRoutes = Router(); | ||
|
|
||
| const oidcLoginLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, | ||
| max: 10, | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| authRoutes.get('/me', isAuthenticated(), async (req, res) => { | ||
| const userRepository = getRepository(User); | ||
| if (!req.user) { | ||
| @@ -657,7 +662,7 @@ | ||
| ); | ||
| }; | ||
|
|
||
| authRoutes.get('/oidc/login/:slug', async (req, res, next) => { | ||
| authRoutes.get('/oidc/login/:slug', oidcLoginLimiter, async (req, res, next) => { | ||
| const settings = getSettings(); | ||
| const provider = settings.oidc.providers.find( | ||
| (p) => p.slug === req.params.slug |
d71389d to
153b30b
Compare
153b30b to
e6a9c86
Compare
Description
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extract