fixes, ai code review, linter

actualbudget · Nov 11, 2024 · 3111644 · 3111644
1 parent f6273a6
commit 3111644
Show file tree

Hide file tree

Showing 12 changed files with 150 additions and 67 deletions.
diff --git 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
@@ -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,13 +123,21 @@ 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 } =
       accountDb.first('SELECT extra_data FROM auth WHERE method = ?', [
         '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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,