From 0ce37004afc60fb9fe58d929bb2e827a1e36b55a Mon Sep 17 00:00:00 2001 From: mikiher Date: Thu, 16 Jul 2026 17:11:11 +0300 Subject: [PATCH] fix(auth): harden OIDC web callback URL validation --- server/auth/OidcAuthStrategy.js | 65 +++++++++----------- test/server/auth/OidcAuthStrategy.test.js | 75 +++++++++++++++++++++++ 2 files changed, 103 insertions(+), 37 deletions(-) create mode 100644 test/server/auth/OidcAuthStrategy.test.js diff --git a/server/auth/OidcAuthStrategy.js b/server/auth/OidcAuthStrategy.js index 64ab82448..0997b7cf3 100644 --- a/server/auth/OidcAuthStrategy.js +++ b/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 diff --git a/test/server/auth/OidcAuthStrategy.test.js b/test/server/auth/OidcAuthStrategy.test.js new file mode 100644 index 000000000..00f950853 --- /dev/null +++ b/test/server/auth/OidcAuthStrategy.test.js @@ -0,0 +1,75 @@ +const { expect } = require('chai') +const sinon = require('sinon') + +const OidcAuthStrategy = require('../../../server/auth/OidcAuthStrategy') +const Logger = require('../../../server/Logger') + +describe('OidcAuthStrategy - isValidWebCallbackUrl', () => { + /** @type {OidcAuthStrategy} */ + let strategy + + beforeEach(() => { + global.RouterBasePath = '' + strategy = new OidcAuthStrategy() + sinon.stub(Logger, 'warn') + sinon.stub(Logger, 'error') + }) + + afterEach(() => { + sinon.restore() + }) + + function mockReq({ secure = false, host = 'books.example.com', xForwardedProto = null } = {}) { + return { + secure, + get(header) { + if (header === 'host') return host + if (header === 'x-forwarded-proto') return xForwardedProto + return null + } + } + } + + it('accepts a same-origin relative path when router base path is empty', () => { + expect(strategy.isValidWebCallbackUrl('/library', mockReq())).to.equal(true) + }) + + it('accepts a same-origin absolute https URL', () => { + const req = mockReq({ secure: true }) + expect(strategy.isValidWebCallbackUrl('https://books.example.com/library', req)).to.equal(true) + }) + + it('rejects protocol-relative URLs', () => { + expect(strategy.isValidWebCallbackUrl('//evil.example/capture', mockReq())).to.equal(false) + }) + + it('rejects backslash-prefixed URLs', () => { + expect(strategy.isValidWebCallbackUrl('/\\evil.example/capture', mockReq())).to.equal(false) + }) + + it('rejects absolute external URLs', () => { + expect(strategy.isValidWebCallbackUrl('http://evil.example/capture', mockReq())).to.equal(false) + }) + + it('rejects encoded protocol-relative path segments', () => { + expect(strategy.isValidWebCallbackUrl('/%2F%2Fevil.example/capture', mockReq())).to.equal(false) + }) + + it('rejects same-origin URLs outside router base path', () => { + global.RouterBasePath = '/audiobookshelf' + expect(strategy.isValidWebCallbackUrl('/login', mockReq())).to.equal(false) + expect(strategy.isValidWebCallbackUrl('/audiobookshelf/login', mockReq())).to.equal(true) + }) + + it('rejects empty and malformed callback URLs', () => { + expect(strategy.isValidWebCallbackUrl('', mockReq())).to.equal(false) + expect(strategy.isValidWebCallbackUrl(null, mockReq())).to.equal(false) + expect(strategy.isValidWebCallbackUrl('not a url', mockReq())).to.equal(false) + }) + + it('uses x-forwarded-proto when determining same-origin https URLs', () => { + const req = mockReq({ xForwardedProto: 'https' }) + expect(strategy.isValidWebCallbackUrl('https://books.example.com/login', req)).to.equal(true) + expect(strategy.isValidWebCallbackUrl('http://books.example.com/login', req)).to.equal(false) + }) +})