From 3111644c8c0ba18512109e3bcb26abfb3f5ace68 Mon Sep 17 00:00:00 2001 From: Leandro Menezes Date: Mon, 11 Nov 2024 11:59:35 -0300 Subject: [PATCH] fixes, ai code review, linter --- jest.global-setup.js | 25 ++++++++++++--- src/account-db.js | 12 ++++++++ src/accounts/openid.js | 44 +++++++++++++++++++++++++-- src/accounts/password.js | 4 +++ src/app-account.js | 15 +++++++-- src/app-admin.js | 18 +++++------ src/app-openid.js | 18 ++++++++--- src/app-secrets.js | 2 +- src/app-sync.js | 4 +-- src/load-config.js | 64 +++++++++++++++++---------------------- src/util/middlewares.js | 5 ++- src/util/validate-user.js | 6 ++-- 12 files changed, 150 insertions(+), 67 deletions(-) diff --git a/jest.global-setup.js b/jest.global-setup.js index 072dfc68d..075fc38bc 100644 --- a/jest.global-setup.js +++ b/jest.global-setup.js @@ -35,10 +35,27 @@ const createUser = (userId, userName, role, owner = 0, enabled = 1) => { }; const setSessionUser = (userId, token = 'valid-token') => { - getAccountDb().mutate('UPDATE sessions SET user_id = ? WHERE token = ?', [ - userId, - token, - ]); + if (!userId) { + throw new Error('userId is required'); + } + + try { + const db = getAccountDb(); + const session = db.get('SELECT token FROM sessions WHERE token = ?', [ + token, + ]); + if (!session) { + throw new Error(`Session not found for token: ${token}`); + } + + getAccountDb().mutate('UPDATE sessions SET user_id = ? WHERE token = ?', [ + userId, + token, + ]); + } catch (error) { + console.error(`Error updating session for user ${userId}:`, error); + throw error; + } }; export default async function setup() { diff --git a/src/account-db.js b/src/account-db.js index adf245fe3..90e68c7ff 100644 --- a/src/account-db.js +++ b/src/account-db.js @@ -107,6 +107,10 @@ export function hasPermission(userId, permission) { } export async function enableOpenID(loginSettings) { + if (!loginSettings || !loginSettings.openId) { + return { error: 'invalid-login-settings' }; + } + let { error } = (await bootstrapOpenId(loginSettings.openId)) || {}; if (error) { return { error }; @@ -119,6 +123,10 @@ export async function disableOpenID( loginSettings, checkForOldPassword = false, ) { + if (!loginSettings || !loginSettings.password) { + return { error: 'invalid-login-settings' }; + } + if (checkForOldPassword) { let accountDb = getAccountDb(); const { extra_data: passwordHash } = @@ -126,6 +134,10 @@ export async function disableOpenID( 'password', ]) || {}; + if (!passwordHash) { + return { error: 'invalid-password' }; + } + if (!loginSettings?.password) { return { error: 'invalid-password' }; } diff --git a/src/accounts/openid.js b/src/accounts/openid.js index c0d353f46..a635594da 100644 --- a/src/accounts/openid.js +++ b/src/accounts/openid.js @@ -67,8 +67,8 @@ async function setupOpenIdClient(config) { return client; } -export async function loginWithOpenIdSetup(body) { - if (!body.return_url) { +export async function loginWithOpenIdSetup(returnUrl) { + if (!returnUrl) { return { error: 'return-url-missing' }; } @@ -107,7 +107,7 @@ export async function loginWithOpenIdSetup(body) { ); accountDb.mutate( 'INSERT INTO pending_openid_requests (state, code_verifier, return_url, expiry_time) VALUES (?, ?, ?, ?)', - [state, code_verifier, body.return_url, expiry_time], + [state, code_verifier, returnUrl, expiry_time], ); const url = client.authorizationUrl({ @@ -165,6 +165,7 @@ export async function loginWithOpenIdFinalize(body) { const params = { code: body.code, state: body.state }; let tokenSet = await client.callback(client.redirect_uris[0], params, { code_verifier, + state: body.state, }); const userInfo = await client.userinfo(tokenSet.access_token); const identity = @@ -248,3 +249,40 @@ export async function loginWithOpenIdFinalize(body) { return { error: 'openid-grant-failed' }; } } + +export function getServerHostname() { + const auth = getAccountDb().first( + 'select * from auth WHERE method = ? and active = 1', + ['openid'], + ); + if (auth && auth.extra_data) { + try { + const openIdConfig = JSON.parse(auth.extra_data); + return openIdConfig.server_hostname; + } catch (error) { + console.error('Error parsing OpenID configuration:', error); + } + } + return null; +} + +export function isValidRedirectUrl(url) { + const serverHostname = getServerHostname(); + + if (!serverHostname) { + return false; + } + + try { + const redirectUrl = new URL(url); + const serverUrl = new URL(serverHostname); + + if (redirectUrl.hostname === serverUrl.hostname) { + return true; + } else { + return false; + } + } catch (err) { + return false; + } +} diff --git a/src/accounts/password.js b/src/accounts/password.js index 9109e2afa..e841697a7 100644 --- a/src/accounts/password.js +++ b/src/accounts/password.js @@ -42,6 +42,10 @@ export function loginWithPassword(password) { 'password', ]) || {}; + if (!passwordHash) { + return { error: 'invalid-password' }; + } + let confirmed = bcrypt.compareSync(password, passwordHash); if (!confirmed) { diff --git a/src/app-account.js b/src/app-account.js index 5c7388eb0..abf577407 100644 --- a/src/app-account.js +++ b/src/app-account.js @@ -12,7 +12,7 @@ import { getUserInfo, } from './account-db.js'; import { changePassword, loginWithPassword } from './accounts/password.js'; -import { loginWithOpenIdSetup } from './accounts/openid.js'; +import { isValidRedirectUrl, loginWithOpenIdSetup } from './accounts/openid.js'; import config from './load-config.js'; let app = express(); @@ -78,7 +78,14 @@ app.post('/login', async (req, res) => { break; } case 'openid': { - let { error, url } = await loginWithOpenIdSetup(req.body); + if (!isValidRedirectUrl(req.body.return_url)) { + res + .status(400) + .send({ status: 'error', reason: 'Invalid redirect URL' }); + return; + } + + let { error, url } = await loginWithOpenIdSetup(req.body.return_url); if (error) { res.status(400).send({ status: 'error', reason: error }); return; @@ -119,6 +126,10 @@ app.get('/validate', (req, res) => { let session = validateSession(req, res); if (session) { const user = getUserInfo(session.user_id); + if (!user) { + res.status(400).send({ status: 'error', reason: 'User not found' }); + return; + } res.send({ status: 'ok', diff --git a/src/app-admin.js b/src/app-admin.js index 34891200a..27589d096 100644 --- a/src/app-admin.js +++ b/src/app-admin.js @@ -37,7 +37,7 @@ app.get('/users/', validateSessionMiddleware, (req, res) => { }); app.post('/users', validateSessionMiddleware, async (req, res) => { - if (!isAdmin(res.locals.user_id)) { + if (!isAdmin(res.locals.session.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', @@ -89,7 +89,7 @@ app.post('/users', validateSessionMiddleware, async (req, res) => { }); app.patch('/users', validateSessionMiddleware, async (req, res) => { - if (!isAdmin(res.locals.user_id)) { + if (!isAdmin(res.locals.session.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', @@ -141,7 +141,7 @@ app.patch('/users', validateSessionMiddleware, async (req, res) => { }); app.delete('/users', validateSessionMiddleware, async (req, res) => { - if (!isAdmin(res.locals.user_id)) { + if (!isAdmin(res.locals.session.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', @@ -191,8 +191,8 @@ app.get('/access', validateSessionMiddleware, (req, res) => { const accesses = UserService.getUserAccess( fileId, - res.locals.user_id, - isAdmin(res.locals.user_id), + res.locals.session.user_id, + isAdmin(res.locals.session.user_id), ); res.json(accesses); @@ -305,12 +305,12 @@ app.get('/access/users', validateSessionMiddleware, async (req, res) => { const { granted } = UserService.checkFilePermission( fileId, - res.locals.user_id, + res.locals.session.user_id, ) || { granted: 0, }; - if (granted === 0 && !isAdmin(res.locals.user_id)) { + if (granted === 0 && !isAdmin(res.locals.session.user_id)) { res.status(400).send({ status: 'error', reason: 'file-denied', @@ -341,12 +341,12 @@ app.post( const { granted } = UserService.checkFilePermission( newUserOwner.fileId, - res.locals.user_id, + res.locals.session.user_id, ) || { granted: 0, }; - if (granted === 0 && !isAdmin(res.locals.user_id)) { + if (granted === 0 && !isAdmin(res.locals.session.user_id)) { res.status(400).send({ status: 'error', reason: 'file-denied', diff --git a/src/app-openid.js b/src/app-openid.js index 3a38358af..a4c9ce78c 100644 --- a/src/app-openid.js +++ b/src/app-openid.js @@ -5,7 +5,10 @@ import { validateSessionMiddleware, } from './util/middlewares.js'; import { disableOpenID, enableOpenID, isAdmin } from './account-db.js'; -import { loginWithOpenIdFinalize } from './accounts/openid.js'; +import { + isValidRedirectUrl, + loginWithOpenIdFinalize, +} from './accounts/openid.js'; import * as UserService from './services/user-service.js'; let app = express(); @@ -15,7 +18,7 @@ app.use(requestLoggerMiddleware); export { app as handlers }; app.post('/enable', validateSessionMiddleware, async (req, res) => { - if (!isAdmin(res.locals.user_id)) { + if (!isAdmin(res.locals.session.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', @@ -34,7 +37,7 @@ app.post('/enable', validateSessionMiddleware, async (req, res) => { }); app.post('/disable', validateSessionMiddleware, async (req, res) => { - if (!isAdmin(res.locals.user_id)) { + if (!isAdmin(res.locals.session.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', @@ -81,9 +84,14 @@ app.get('/config', async (req, res) => { app.get('/callback', async (req, res) => { let { error, url } = await loginWithOpenIdFinalize(req.query); + if (error) { - console.error('OpenID Callback Error:', error); - res.status(400).send({ status: 'error', reason: 'Invalid request' }); + res.status(400).send({ status: 'error', reason: error }); + return; + } + + if (!isValidRedirectUrl(url)) { + res.status(400).send({ status: 'error', reason: 'Invalid redirect URL' }); return; } diff --git a/src/app-secrets.js b/src/app-secrets.js index d95e94ea8..9cc608460 100644 --- a/src/app-secrets.js +++ b/src/app-secrets.js @@ -20,7 +20,7 @@ app.post('/', async (req, res) => { const { name, value } = req.body; if (method === 'openid') { - let canSaveSecrets = isAdmin(res.locals.user_id); + let canSaveSecrets = isAdmin(res.locals.session.user_id); if (!canSaveSecrets) { res.status(403).send({ diff --git a/src/app-sync.js b/src/app-sync.js index f97f859a5..5c7fa3d44 100644 --- a/src/app-sync.js +++ b/src/app-sync.js @@ -247,7 +247,7 @@ app.post('/upload-user-file', async (req, res) => { name: name, encryptMeta: encryptMeta, owner: - res.locals.user_id || + res.locals.session.user_id || (() => { throw new Error('User ID is required for file creation'); })(), @@ -310,7 +310,7 @@ app.post('/update-user-filename', (req, res) => { app.get('/list-user-files', (req, res) => { const fileService = new FilesService(getAccountDb()); - const rows = fileService.find({ userId: res.locals.user_id }); + const rows = fileService.find({ userId: res.locals.session.user_id }); res.send({ status: 'ok', data: rows.map((row) => ({ diff --git a/src/load-config.js b/src/load-config.js index 3327b7ded..abbde5a10 100644 --- a/src/load-config.js +++ b/src/load-config.js @@ -144,43 +144,35 @@ const finalConfig = { config.upload.fileSizeLimitMB, } : config.upload, - openId: - process.env.ACTUAL_OPENID_DISCOVERY_URL || - process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT - ? { - ...(process.env.ACTUAL_OPENID_DISCOVERY_URL - ? { - issuer: process.env.ACTUAL_OPENID_DISCOVERY_URL, - } - : process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT - ? { - issuer: { - name: process.env.ACTUAL_OPENID_PROVIDER_NAME, - authorization_endpoint: - process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT, - token_endpoint: process.env.ACTUAL_OPENID_TOKEN_ENDPOINT, - userinfo_endpoint: - process.env.ACTUAL_OPENID_USERINFO_ENDPOINT, - }, - } - : config.openId), - ...{ - client_id: process.env.ACTUAL_OPENID_CLIENT_ID - ? process.env.ACTUAL_OPENID_CLIENT_ID - : config.openId?.client_id, - }, - ...{ - client_secret: process.env.ACTUAL_OPENID_CLIENT_SECRET - ? process.env.ACTUAL_OPENID_CLIENT_SECRET - : config.openId?.client_secret, - }, - ...{ - server_hostname: process.env.ACTUAL_OPENID_SERVER_HOSTNAME - ? process.env.ACTUAL_OPENID_SERVER_HOSTNAME - : config.openId?.server_hostname, + openId: (() => { + if ( + !process.env.ACTUAL_OPENID_DISCOVERY_URL && + !process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT + ) { + return config.openId; + } + const baseConfig = process.env.ACTUAL_OPENID_DISCOVERY_URL + ? { issuer: process.env.ACTUAL_OPENID_DISCOVERY_URL } + : { + issuer: { + name: process.env.ACTUAL_OPENID_PROVIDER_NAME, + authorization_endpoint: + process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT, + token_endpoint: process.env.ACTUAL_OPENID_TOKEN_ENDPOINT, + userinfo_endpoint: process.env.ACTUAL_OPENID_USERINFO_ENDPOINT, }, - } - : config.openId, + }; + return { + ...baseConfig, + client_id: + process.env.ACTUAL_OPENID_CLIENT_ID ?? config.openId?.client_id, + client_secret: + process.env.ACTUAL_OPENID_CLIENT_SECRET ?? config.openId?.client_secret, + server_hostname: + process.env.ACTUAL_OPENID_SERVER_HOSTNAME ?? + config.openId?.server_hostname, + }; + })(), token_expiration: process.env.ACTUAL_TOKEN_EXPIRATION ? process.env.ACTUAL_TOKEN_EXPIRATION : config.token_expiration, diff --git a/src/util/middlewares.js b/src/util/middlewares.js index e606e9688..bf02e247d 100644 --- a/src/util/middlewares.js +++ b/src/util/middlewares.js @@ -40,9 +40,9 @@ async function errorMiddleware(err, req, res, next) { const validateSessionMiddleware = async (req, res, next) => { let session = await validateSession(req, res); if (!session) { - res.status(401).json({ + res.status(401).json({ status: 'error', - reason: 'invalid-session' + reason: 'invalid-session', }); return; } @@ -50,7 +50,6 @@ const validateSessionMiddleware = async (req, res, next) => { res.locals.session = session; next(); }; -}; const requestLoggerMiddleware = expressWinston.logger({ transports: [new winston.transports.Console()], diff --git a/src/util/validate-user.js b/src/util/validate-user.js index 3481ad294..5d16273a2 100644 --- a/src/util/validate-user.js +++ b/src/util/validate-user.js @@ -4,6 +4,7 @@ import ipaddr from 'ipaddr.js'; import { getSession } from '../account-db.js'; export const TOKEN_EXPIRATION_NEVER = -1; +const MS_PER_SECOND = 1000; /** * @param {import('express').Request} req @@ -30,13 +31,14 @@ export default function validateSession(req, res) { if ( session.expires_at !== TOKEN_EXPIRATION_NEVER && - session.expires_at * 1000 <= Date.now() + session.expires_at * MS_PER_SECOND <= Date.now() ) { res.status(401); res.send({ status: 'error', reason: 'token-expired', - details: 'Token Expired. Login again', + details: 'token-expired', + message: 'Token expired. Please login again.', }); return null; }