diff --git a/server/Server.js b/server/Server.js index c1657ff1e..ef0888927 100644 --- a/server/Server.js +++ b/server/Server.js @@ -14,7 +14,6 @@ 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') @@ -289,9 +288,10 @@ 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 { origin } = getRequestOrigin(req) + const host = req.get('host') + const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http' const prefix = urlStartsWithRouterBasePath ? global.RouterBasePath : '' - req.originalHostPrefix = `${origin}${prefix}` + req.originalHostPrefix = `${protocol}://${host}${prefix}` if (!urlStartsWithRouterBasePath) { req.url = `${global.RouterBasePath}${req.url}` } diff --git a/server/auth/OidcAuthStrategy.js b/server/auth/OidcAuthStrategy.js index 0997b7cf3..64ab82448 100644 --- a/server/auth/OidcAuthStrategy.js +++ b/server/auth/OidcAuthStrategy.js @@ -4,7 +4,6 @@ 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 @@ -290,8 +289,8 @@ class OidcAuthStrategy { const sessionKey = strategy._key try { - const { origin } = getRequestOrigin(req) - const hostUrl = new URL(origin) + const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http' + const hostUrl = new URL(`${protocol}://${req.get('host')}`) const isMobileFlow = req.query.response_type === 'code' || req.query.redirect_uri || req.query.code_challenge // Only allow code flow (for mobile clients) @@ -395,10 +394,11 @@ class OidcAuthStrategy { let postLogoutRedirectUri = null if (authMethod === 'openid') { - const { origin } = getRequestOrigin(req) + const protocol = req.secure || req.get('x-forwarded-proto') === 'https' ? 'https' : 'http' + const host = req.get('host') // 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 = `${origin}${global.RouterBasePath}/login` + postLogoutRedirectUri = `${protocol}://${host}${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,33 +515,42 @@ class OidcAuthStrategy { if (!callbackUrl) return false try { - // 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 + '/')) { + // 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 + } Logger.warn(`[OidcAuth] Rejected callback URL outside router base path: ${callbackUrl}`) return false } - return true + // 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 } catch (error) { Logger.error(`[OidcAuth] Invalid callback URL format: ${callbackUrl}`, error) return false diff --git a/server/auth/TokenManager.js b/server/auth/TokenManager.js index 0c59ae7a1..5933209c7 100644 --- a/server/auth/TokenManager.js +++ b/server/auth/TokenManager.js @@ -6,7 +6,6 @@ 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 */ @@ -60,7 +59,7 @@ class TokenManager { setRefreshTokenCookie(req, res, refreshToken) { res.cookie('refresh_token', refreshToken, { httpOnly: true, - secure: isRequestSecure(req), + secure: req.secure || req.get('x-forwarded-proto') === 'https', sameSite: 'lax', maxAge: this.RefreshTokenExpiry * 1000, path: '/' diff --git a/server/controllers/UserController.js b/server/controllers/UserController.js index 4a4da0366..3ec10539e 100644 --- a/server/controllers/UserController.js +++ b/server/controllers/UserController.js @@ -363,16 +363,15 @@ class UserController { * @param {Response} res */ async delete(req, res) { - const user = req.reqUser - + if (req.params.id === 'root') { + Logger.error('[UserController] Attempt to delete root user. Root user cannot be deleted') + return res.sendStatus(400) + } if (req.user.id === req.params.id) { Logger.error(`[UserController] User ${req.user.username} is attempting to delete self`) return res.sendStatus(400) } - if (user.isRoot) { - Logger.error(`[UserController] Admin user "${req.user.username}" attempted to delete root user`) - return res.sendStatus(403) - } + const user = req.reqUser // Todo: check if user is logged in and cancel streams diff --git a/server/models/User.js b/server/models/User.js index 0b9d49438..f380f8e4f 100644 --- a/server/models/User.js +++ b/server/models/User.js @@ -530,14 +530,7 @@ class User extends Model { }, { sequelize, - modelName: 'user', - hooks: { - beforeDestroy(user) { - if (user.type === 'root') { - throw new Error('Root user cannot be deleted') - } - } - } + modelName: 'user' } ) } diff --git a/server/utils/requestUtils.js b/server/utils/requestUtils.js deleted file mode 100644 index afac4fcc0..000000000 --- a/server/utils/requestUtils.js +++ /dev/null @@ -1,43 +0,0 @@ -/** - * 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 -} diff --git a/test/server/auth/OidcAuthStrategy.test.js b/test/server/auth/OidcAuthStrategy.test.js deleted file mode 100644 index 6a84edac0..000000000 --- a/test/server/auth/OidcAuthStrategy.test.js +++ /dev/null @@ -1,77 +0,0 @@ -const { expect } = require('chai') -const sinon = require('sinon') - -// Load Database first so Auth resolves OidcAuthStrategy before the circular require completes. -require('../../../server/Database') -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) - }) -}) diff --git a/test/server/controllers/UserController.test.js b/test/server/controllers/UserController.test.js deleted file mode 100644 index add025d6e..000000000 --- a/test/server/controllers/UserController.test.js +++ /dev/null @@ -1,26 +0,0 @@ -const { expect } = require('chai') -const sinon = require('sinon') - -const UserController = require('../../../server/controllers/UserController') -const Logger = require('../../../server/Logger') - -describe('UserController - delete', () => { - beforeEach(() => { - sinon.stub(Logger, 'error') - }) - - afterEach(() => { - sinon.restore() - }) - - it('rejects deleting the root user by UUID', async () => { - const rootUser = { id: 'root-uuid', isRoot: true, username: 'root' } - const adminUser = { id: 'admin-uuid', isRoot: false, username: 'admin' } - const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - - await UserController.delete({ user: adminUser, reqUser: rootUser, params: { id: rootUser.id } }, fakeRes) - - expect(fakeRes.sendStatus.calledWith(403)).to.be.true - expect(fakeRes.json.called).to.be.false - }) -}) diff --git a/test/server/utils/requestUtils.test.js b/test/server/utils/requestUtils.test.js deleted file mode 100644 index 589c6f1cd..000000000 --- a/test/server/utils/requestUtils.test.js +++ /dev/null @@ -1,40 +0,0 @@ -const { expect } = require('chai') - -const { isRequestSecure, getRequestProtocol, getRequestOrigin } = require('../../../server/utils/requestUtils') - -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 - } - } -} - -describe('requestUtils', () => { - it('isRequestSecure uses req.secure', () => { - expect(isRequestSecure(mockReq({ secure: true }))).to.equal(true) - expect(isRequestSecure(mockReq({ secure: false }))).to.equal(false) - }) - - it('isRequestSecure uses x-forwarded-proto', () => { - expect(isRequestSecure(mockReq({ xForwardedProto: 'https' }))).to.equal(true) - expect(isRequestSecure(mockReq({ xForwardedProto: 'http' }))).to.equal(false) - expect(isRequestSecure(mockReq({ xForwardedProto: 'http, https' }))).to.equal(true) - }) - - it('getRequestProtocol returns https or http', () => { - expect(getRequestProtocol(mockReq({ secure: true }))).to.equal('https') - expect(getRequestProtocol(mockReq())).to.equal('http') - }) - - it('getRequestOrigin builds origin from protocol and host', () => { - expect(getRequestOrigin(mockReq({ secure: true }))).to.deep.equal({ - protocol: 'https', - host: 'books.example.com', - origin: 'https://books.example.com' - }) - }) -})