Merge pull request #5386 from mikiher/fix/oidc-url-validation

Fix oidc url validation

server/Server.js

@@ -14,6 +14,7 @@ const { version } = require('../package.json')
 const is = require('./libs/requestIp/isJs')
 const fileUtils = require('./utils/fileUtils')
 const { toNumber } = require('./utils/index')
+const { getRequestOrigin } = require('./utils/requestUtils')
 const Logger = require('./Logger')
 
 const Auth = require('./Auth')
@@ -288,10 +289,9 @@ class Server {
     // if RouterBasePath is set, modify all requests to include the base path
     app.use((req, res, next) => {
       const urlStartsWithRouterBasePath = req.url.startsWith(global.RouterBasePath)
-      const host = req.get('host')
-      const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http'
+      const { origin } = getRequestOrigin(req)
       const prefix = urlStartsWithRouterBasePath ? global.RouterBasePath : ''
-      req.originalHostPrefix = `${protocol}://${host}${prefix}`
+      req.originalHostPrefix = `${origin}${prefix}`
       if (!urlStartsWithRouterBasePath) {
         req.url = `${global.RouterBasePath}${req.url}`
       }

server/auth/OidcAuthStrategy.js

@@ -4,6 +4,7 @@ const OpenIDClient = require('openid-client')
 const axios = require('axios')
 const Database = require('../Database')
 const Logger = require('../Logger')
+const { getRequestOrigin } = require('../utils/requestUtils')
 
 /**
  * OpenID Connect authentication strategy
@@ -289,8 +290,8 @@ class OidcAuthStrategy {
     const sessionKey = strategy._key
 
     try {
-      const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http'
-      const hostUrl = new URL(`${protocol}://${req.get('host')}`)
+      const { origin } = getRequestOrigin(req)
+      const hostUrl = new URL(origin)
       const isMobileFlow = req.query.response_type === 'code' || req.query.redirect_uri || req.query.code_challenge
 
       // Only allow code flow (for mobile clients)
@@ -394,11 +395,10 @@ class OidcAuthStrategy {
       let postLogoutRedirectUri = null
 
       if (authMethod === 'openid') {
-        const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http'
-        const host = req.get('host')
+        const { origin } = getRequestOrigin(req)
         // TODO: ABS does currently not support subfolders for installation
         // If we want to support it we need to include a config for the serverurl
-        postLogoutRedirectUri = `${protocol}://${host}${global.RouterBasePath}/login`
+        postLogoutRedirectUri = `${origin}${global.RouterBasePath}/login`
       }
       // else for openid-mobile we keep postLogoutRedirectUri on null
       //  nice would be to redirect to the app here, but for example Authentik does not implement
@@ -515,42 +515,33 @@ class OidcAuthStrategy {
     if (!callbackUrl) return false
 
     try {
-      // Handle relative URLs - these are always safe if they start with router base path
-      if (callbackUrl.startsWith('/')) {
-        // Only allow relative paths that start with the router base path
-        if (callbackUrl.startsWith(global.RouterBasePath + '/')) {
-          return true
-        }
+      // Reject protocol-relative (//host) and backslash-prefixed (/\host) values,
+      // which browsers resolve to a cross-origin absolute URL.
+      if (callbackUrl.startsWith('//') || callbackUrl.startsWith('/\\')) {
+        Logger.warn(`[OidcAuth] Rejected protocol-relative callback URL: ${callbackUrl}`)
+        return false
+      }
+
+      const { origin: serverOrigin } = getRequestOrigin(req)
+      const resolvedUrl = callbackUrl.startsWith('/') ? new URL(callbackUrl, serverOrigin) : new URL(callbackUrl)
+
+      if (resolvedUrl.origin !== serverOrigin) {
+        Logger.warn(`[OidcAuth] Rejected callback URL to different origin: ${callbackUrl} (expected ${serverOrigin})`)
+        return false
+      }
+
+      const pathname = decodeURIComponent(resolvedUrl.pathname)
+      if (pathname.startsWith('//') || pathname.startsWith('/\\')) {
+        Logger.warn(`[OidcAuth] Rejected protocol-relative callback URL path: ${callbackUrl}`)
+        return false
+      }
+
+      if (!resolvedUrl.pathname.startsWith(global.RouterBasePath + '/')) {
         Logger.warn(`[OidcAuth] Rejected callback URL outside router base path: ${callbackUrl}`)
         return false
       }
 
-      // For absolute URLs, ensure they point to the same origin
-      const callbackUrlObj = new URL(callbackUrl)
-      // NPM appends both http and https in x-forwarded-proto sometimes, so we need to check for both
-      const xfp = (req.get('x-forwarded-proto') || '').toLowerCase()
-      const currentProtocol =
-        req.secure ||
-        xfp
-          .split(',')
-          .map((s) => s.trim())
-          .includes('https')
-          ? 'https'
-          : 'http'
-      const currentHost = req.get('host')
-
-      // Check if protocol and host match exactly
-      if (callbackUrlObj.protocol === currentProtocol + ':' && callbackUrlObj.host === currentHost) {
-        // Additional check: ensure path starts with router base path
-        if (callbackUrlObj.pathname.startsWith(global.RouterBasePath + '/')) {
-          return true
-        }
-        Logger.warn(`[OidcAuth] Rejected same-origin callback URL outside router base path: ${callbackUrl}`)
-        return false
-      }
-
-      Logger.warn(`[OidcAuth] Rejected callback URL to different origin: ${callbackUrl} (expected ${currentProtocol}://${currentHost})`)
-      return false
+      return true
     } catch (error) {
       Logger.error(`[OidcAuth] Invalid callback URL format: ${callbackUrl}`, error)
       return false

server/auth/TokenManager.js

@@ -6,6 +6,7 @@ const Logger = require('../Logger')
 
 const requestIp = require('../libs/requestIp')
 const jwt = require('../libs/jsonwebtoken')
+const { isRequestSecure } = require('../utils/requestUtils')
 
 class TokenManager {
   /** @type {string} JWT secret key */
@@ -59,7 +60,7 @@ class TokenManager {
   setRefreshTokenCookie(req, res, refreshToken) {
     res.cookie('refresh_token', refreshToken, {
       httpOnly: true,
-      secure: req.secure || req.get('x-forwarded-proto') === 'https',
+      secure: isRequestSecure(req),
       sameSite: 'lax',
       maxAge: this.RefreshTokenExpiry * 1000,
       path: '/'

server/utils/requestUtils.js

@@ -0,0 +1,43 @@
+/**
+ * Whether the request was made over HTTPS.
+ * Uses Express `req.secure` and `x-forwarded-proto`
+ *
+ * @param {import('express').Request} req
+ * @returns {boolean}
+ */
+function isRequestSecure(req) {
+  if (req.secure) return true
+  const xfp = (req.get('x-forwarded-proto') || '').toLowerCase()
+  // Nginx Proxy Manager sends "http, https"; see https://github.com/advplyr/audiobookshelf/pull/4635
+  return (
+    xfp === 'https' ||
+    xfp
+      .split(',')
+      .map((s) => s.trim())
+      .includes('https')
+  )
+}
+
+/**
+ * @param {import('express').Request} req
+ * @returns {'https' | 'http'}
+ */
+function getRequestProtocol(req) {
+  return isRequestSecure(req) ? 'https' : 'http'
+}
+
+/**
+ * @param {import('express').Request} req
+ * @returns {{ protocol: 'https' | 'http', host: string, origin: string }}
+ */
+function getRequestOrigin(req) {
+  const protocol = getRequestProtocol(req)
+  const host = req.get('host')
+  return { protocol, host, origin: `${protocol}://${host}` }
+}
+
+module.exports = {
+  isRequestSecure,
+  getRequestProtocol,
+  getRequestOrigin
+}