fix(security): reserve integration secrets to admins, and remove the shell from the backup chain

front/src/routes/integration/all/telegram/actions.js

@@ -11,10 +11,24 @@ const actions = store => ({
       telegramGetApiKeyStatus: RequestStatus.Getting
     });
     try {
-      const variable = await state.httpClient.get('/api/v1/service/telegram/variable/TELEGRAM_API_KEY');
-      store.setState({
-        telegramApiKey: variable.value
-      });
+      // The bot API key is a service-wide secret: the server answers 403 to a
+      // non-admin, and only an admin is shown the form to change it. Its own
+      // failure must not abort the load: every user comes to this page for
+      // their linking link, which is per-user.
+      // The role is deliberately not read from the store here. On a hard page
+      // load this action runs during the first render, before checkSession()
+      // has filled in the user, so an admin would be treated as a non-admin
+      // and never see the key.
+      try {
+        const variable = await state.httpClient.get('/api/v1/service/telegram/variable/TELEGRAM_API_KEY');
+        store.setState({
+          telegramApiKey: variable.value
+        });
+      } catch (e) {
+        store.setState({
+          telegramApiKey: ''
+        });
+      }
       const { link } = await state.httpClient.get('/api/v1/service/telegram/link');
       store.setState({
         telegramCustomLink: link,

front/src/routes/integration/index.js

@@ -17,7 +17,10 @@ import { RequestStatus } from '../../utils/consts';
 // the role rules stay expressed on the technical `type` (spec §2.2): the
 // browse categories are display metadata and play no part in visibility
 const HIDDEN_TYPES_FOR_NON_ADMIN_USERS = ['device', 'weather'];
-const HIDDEN_INTEGRATIONS_FOR_NON_ADMIN_USERS = ['homekit'];
+// homekit exposes the whole house to a hub; free-mobile is a single global SMS
+// account whose page reads service-wide credentials. Neither has anything
+// per-user, so a non-admin has no business on those pages.
+const HIDDEN_INTEGRATIONS_FOR_NON_ADMIN_USERS = ['homekit', 'free-mobile'];
 // cross-cutting views: they are not browse categories, they filter the whole
 // catalog (a favorite, or an integration with a pending update, can be of any
 // category) — so no category filter must be applied to them

front/src/routes/settings/SettingsLayout.jsx

@@ -1,7 +1,9 @@
 import { Text } from 'preact-i18n';
 import { Link } from 'preact-router/match';
+import { connect } from 'unistore/preact';
 import cx from 'classnames';
 import config from '../../config';
+import { USER_ROLE } from '../../../../server/utils/constants';
 
 import ChipsScroll from '../../components/chips-scroll';
 import style from './style.css';
@@ -12,7 +14,7 @@ const MENU_ITEMS = [
   { href: '/dashboard/settings/user', icon: 'user', textId: 'settings.usersTab', matchPrefix: true },
   { href: '/dashboard/settings/session', icon: 'smartphone', textId: 'settings.sessionsTab' },
   { href: '/dashboard/settings/security', icon: 'shield', textId: 'settings.securityTab', gatewayOnly: true },
-  { href: '/dashboard/settings/gateway', icon: 'globe', textId: 'settings.gatewayTab' },
+  { href: '/dashboard/settings/gateway', icon: 'globe', textId: 'settings.gatewayTab', adminOnly: true },
   { href: '/dashboard/settings/gateway-users', icon: 'user', textId: 'settings.gatewayUsersTab', gatewayOnly: true },
   {
     href: '/dashboard/settings/gateway-open-api',
@@ -21,12 +23,17 @@ const MENU_ITEMS = [
     gatewayOnly: true
   },
   { href: '/dashboard/settings/billing', icon: 'credit-card', textId: 'settings.billingTab', gatewayOnly: true },
-  { href: '/dashboard/settings/backup', icon: 'database', textId: 'settings.backupTab' },
-  { href: '/dashboard/settings/jobs', icon: 'cpu', textId: 'settings.jobsTab' },
+  { href: '/dashboard/settings/backup', icon: 'database', textId: 'settings.backupTab', adminOnly: true },
+  { href: '/dashboard/settings/jobs', icon: 'cpu', textId: 'settings.jobsTab', adminOnly: true },
   { href: '/dashboard/settings/service', icon: 'grid', textId: 'settings.serviceTab' },
-  { href: '/dashboard/settings/system', icon: 'power', textId: 'settings.systemTab' }
+  { href: '/dashboard/settings/system', icon: 'power', textId: 'settings.systemTab', adminOnly: true }
 ];
 
+// `adminOnly` marks the tabs whose API is reserved to admins: the system
+// settings and the backup key are instance-wide, the Gladys Plus status and the
+// background jobs are admin routes of their own. A non-admin used to reach them
+// and land on error states. Hiding the entry mirrors the app nav
+// (components/header), the server stays the authority on a deep link.
 // The settings live on the same Horizon glass scene as the dashboard: the
 // global .glass-theme class gates the shared theme layer (cards, alerts,
 // badges, buttons), .settings-page scopes the settings-only pass (style.css
@@ -68,7 +75,11 @@ const DashboardSettings = ({ children, ...props }) => (
             activeSelector={`.${style.tabLinkActive}`}
             activeKey={props.currentUrl}
           >
-            {MENU_ITEMS.filter(item => !item.gatewayOnly || config.gatewayMode).map(item => (
+            {MENU_ITEMS.filter(
+              item =>
+                (!item.gatewayOnly || config.gatewayMode) &&
+                (!item.adminOnly || (props.user && props.user.role === USER_ROLE.ADMIN))
+            ).map(item => (
               <Link
                 key={item.href}
                 href={item.href}
@@ -92,4 +103,6 @@ const DashboardSettings = ({ children, ...props }) => (
   </div>
 );
 
-export default DashboardSettings;
+// connected rather than fed by its callers: the layout is rendered from sixteen
+// settings pages, none of which pass the user down
+export default connect('user', {})(DashboardSettings);

server/api/controllers/variable.controller.js

@@ -1,5 +1,27 @@
 const asyncMiddleware = require('../middlewares/asyncMiddleware');
-const { NotFoundError } = require('../../utils/coreErrors');
+const { NotFoundError, ForbiddenError } = require('../../utils/coreErrors');
+const { USER_ROLE } = require('../../utils/constants');
+
+/**
+ * @description Ensure only an admin can reach a service-wide variable.
+ * Service variables that are not scoped to a user hold the credentials of the
+ * integrations (API keys, broker passwords, OAuth tokens), so reading or
+ * writing them is an administration gesture. Variables scoped to the calling
+ * user are left alone: a user can only ever touch their own row.
+ * @param {object} req - The Express request.
+ * @param {string} [userId] - The user the variable is scoped to, or null when it is service-wide.
+ * @returns {void}
+ * @example
+ * ensureAdminOnServiceWideVariable(req, null);
+ */
+function ensureAdminOnServiceWideVariable(req, userId) {
+  if (userId) {
+    return;
+  }
+  if (!req.user || req.user.role !== USER_ROLE.ADMIN) {
+    throw new ForbiddenError('This route is only accessible to admin user.');
+  }
+}
 
 module.exports = function VariableController(gladys) {
   /**
@@ -10,6 +32,7 @@ module.exports = function VariableController(gladys) {
    */
   async function setForLocalService(req, res) {
     const userId = req.body.userRelated ? req.user.id : null;
+    ensureAdminOnServiceWideVariable(req, userId);
     const service = await gladys.service.getLocalServiceByName(req.params.service_name);
     const variable = await gladys.variable.setValue(req.params.variable_key, req.body.value, service.id, userId);
     res.json(variable);
@@ -22,6 +45,7 @@ module.exports = function VariableController(gladys) {
    */
   async function getByLocalService(req, res) {
     const userId = req.query.userRelated ? req.user.id : null;
+    ensureAdminOnServiceWideVariable(req, userId);
     const service = await gladys.service.getLocalServiceByName(req.params.service_name);
     const value = await gladys.variable.getValue(req.params.variable_key, service.id, userId);
     if (!value) {

server/api/routes.js

@@ -409,9 +409,13 @@ function getRoutes(gladys) {
       admin: true,
       controller: gatewayController.createBackup,
     },
+    // reachable without authentication while the instance has no user (signup
+    // restore flow), and it makes the server download and unpack a remote file:
+    // rate limited like the other pre-authentication routes
     'post /api/v1/gateway/backup/restore': {
       authenticatedOrNotConfigured: true,
       admin: true,
+      rateLimit: true,
       controller: gatewayController.restoreBackup,
     },
     'get /api/v1/gateway/backup/restore/status': {
@@ -825,12 +829,18 @@ function getRoutes(gladys) {
       authenticated: true,
       controller: variableController.getByLocalService,
     },
+    // global variables hold instance-wide secrets (Gladys Plus keys, backup
+    // keys...): reading and writing them is reserved to admins. Per-user
+    // settings go through /api/v1/user/variable below, which stays open to
+    // every authenticated user.
     'post /api/v1/variable/:variable_key': {
       authenticated: true,
+      admin: true,
       controller: variableController.setValue,
     },
     'get /api/v1/variable/:variable_key': {
       authenticated: true,
+      admin: true,
       controller: variableController.getValue,
     },
     'post /api/v1/user/variable/:variable_key': {

server/lib/gateway/gateway.backup.js

@@ -6,7 +6,8 @@ const fsPromise = require('fs').promises;
 const retry = require('async-retry');
 const db = require('../../models');
 const logger = require('../../utils/logger');
-const { exec } = require('../../utils/childProcess');
+const { execFile } = require('../../utils/childProcess');
+const { escapeSqlStringLiteral } = require('../../utils/backupSafety');
 const { readChunk } = require('../../utils/readChunk');
 const { NotFoundError } = require('../../utils/coreErrors');
 const { USER_ROLE } = require('../../utils/constants');
@@ -86,7 +87,7 @@ async function backup(jobId) {
         await fse.emptyDir(this.config.backupsFolder);
         // We backup database
         logger.info(`Starting Gateway backup in folder ${sqliteBackupFilePath}`);
-        await exec(`sqlite3 ${this.config.storage} ".backup '${sqliteBackupFilePath}'"`);
+        await execFile('sqlite3', [this.config.storage, `.backup '${sqliteBackupFilePath}'`]);
         logger.info(`Gateway backup: Unlocking Database`);
       });
     }, SQLITE_BACKUP_RETRY_OPTIONS);
@@ -101,7 +102,7 @@ async function backup(jobId) {
     try {
       // ZSTD compresses better than GZIP and needs less memory during the export
       await backupInstance.allAsync(
-        ` EXPORT DATABASE '${duckDbBackupFolderPath}' (
+        ` EXPORT DATABASE '${escapeSqlStringLiteral(duckDbBackupFolderPath)}' (
             FORMAT PARQUET,
             COMPRESSION ZSTD
         )`,
@@ -113,15 +114,24 @@ async function backup(jobId) {
     }
     // compress backup
     logger.info(`Gateway backup: Compressing backup`);
-    await exec(
-      `cd ${this.config.backupsFolder} && tar -czvf ${compressedBackupFileName} ${sqliteBackupFileName} ${duckDbBackupFolder}`,
-    );
+    await execFile('tar', ['-czvf', compressedBackupFileName, sqliteBackupFileName, duckDbBackupFolder], {
+      cwd: this.config.backupsFolder,
+    });
     await this.job.updateProgress(jobId, 20);
     // encrypt backup
     logger.info(`Gateway backup: Encrypting backup`);
-    await exec(
-      `openssl enc -aes-256-cbc -pass pass:${encryptKey} -in ${compressedBackupFilePath} -out ${encryptedBackupFilePath}`,
-    );
+    // the encryption key is a passphrase the user chose: passed through a shell it
+    // would be a command injection, so it goes to openssl as a plain argument
+    await execFile('openssl', [
+      'enc',
+      '-aes-256-cbc',
+      '-pass',
+      `pass:${encryptKey}`,
+      '-in',
+      compressedBackupFilePath,
+      '-out',
+      encryptedBackupFilePath,
+    ]);
     await this.job.updateProgress(jobId, 30);
     // Upload file to the Gladys Gateway
     const encryptedFileInfos = await fsPromise.stat(encryptedBackupFilePath);

server/lib/gateway/gateway.downloadBackup.js

@@ -3,10 +3,14 @@ const fse = require('fs-extra');
 const fs = require('fs');
 const logger = require('../../utils/logger');
 const { EVENTS, WEBSOCKET_MESSAGE_TYPES } = require('../../utils/constants');
-const { exec, execFile } = require('../../utils/childProcess');
+const { execFile, spawnToFile } = require('../../utils/childProcess');
 const { NotFoundError } = require('../../utils/coreErrors');
+const { assertSafeBackupName, isSafeArchiveEntry } = require('../../utils/backupSafety');
 
 const RESTORE_FOLDER = 'restore';
+// thrown by the checks that must never fall back to the old restore strategy:
+// a rejected archive is a rejected backup, not a backup to try another way
+const UNSAFE_BACKUP_ERRORS = ['BACKUP_CONTAINS_UNSAFE_PATHS', 'BACKUP_UNSAFE_FILE_NAME'];
 
 /**
  * @description Restore a backup.
@@ -28,7 +32,10 @@ async function downloadBackup(fileUrl) {
   // we empty the restore backup folder
   await fse.emptyDir(restoreFolderPath);
 
-  const encryptedBackupName = path.basename(fileWithoutSignedParams, '.enc');
+  // the name comes from a URL the caller chose: `path.basename` stops a path
+  // traversal but keeps every shell and SQL metacharacter, and this name ends up
+  // building the paths handed to gzip, sqlite3 and DuckDB below
+  const encryptedBackupName = assertSafeBackupName(path.basename(fileWithoutSignedParams, '.enc'));
   const encryptedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.enc`);
   const compressedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.gz`);
 
@@ -58,13 +65,13 @@ async function downloadBackup(fileUrl) {
     logger.info(`Trying to restore the backup new style (DuckDB)`);
     // Check archive for path traversal attempts and symlinks
     const tarEntries = await execFile('tar', ['-tzf', compressedBackupFilePath]);
+    // every entry must be a plain relative name: this rejects path traversal and
+    // absolute paths, and also the quotes, `$`, backticks and semicolons that
+    // would otherwise reach sqlite3 and DuckDB through the extracted file names
     const hasUnsafePath = tarEntries
       .split('\n')
       .filter(Boolean)
-      .some((entry) => {
-        const normalized = path.posix.normalize(entry);
-        return path.posix.isAbsolute(entry) || normalized === '..' || normalized.startsWith('../');
-      });
+      .some((entry) => !isSafeArchiveEntry(entry));
     const tarList = await execFile('tar', ['-tzvf', compressedBackupFilePath]);
     const hasSymlink = tarList.split('\n').some((line) => line.startsWith('l'));
     if (hasUnsafePath || hasSymlink) {
@@ -83,13 +90,15 @@ async function downloadBackup(fileUrl) {
     );
   } catch (e) {
     // Re-throw security errors - don't fall back to old strategy
-    if (e.message === 'BACKUP_CONTAINS_UNSAFE_PATHS') {
+    if (UNSAFE_BACKUP_ERRORS.includes(e.message)) {
       throw e;
     }
     logger.info(`Extracting failed using new strategy (Error: ${e})`);
     logger.info(`Restoring using old backup strategy (SQLite only)`);
     sqliteBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.db`);
-    await exec(`gzip -dc ${compressedBackupFilePath} > ${sqliteBackupFilePath}`);
+    // no shell here: the redirection is a write stream, so the backup name can
+    // never be read as a command
+    await spawnToFile('gzip', ['-dc', compressedBackupFilePath], sqliteBackupFilePath);
   }
   // done!
   logger.info(`Gladys backup downloaded with success.`);

server/lib/gateway/gateway.restoreBackup.js

@@ -7,8 +7,9 @@ const { promisify } = require('util');
 
 const db = require('../../models');
 const logger = require('../../utils/logger');
-const { exec } = require('../../utils/childProcess');
+const { execFile } = require('../../utils/childProcess');
 const { NotFoundError } = require('../../utils/coreErrors');
+const { escapeSqlStringLiteral } = require('../../utils/backupSafety');
 
 /**
  * @description Replace the local sqlite database with a backup.
@@ -39,8 +40,10 @@ async function restoreBackup(sqliteBackupFilePath, duckDbBackupFolderPath) {
   logger.info('Backup seems to be a valid file. Restoring.');
   // shutting down the current DB
   await this.sequelize.close();
-  // copy the backupFile to the new DB
-  await exec(`sqlite3 ${this.config.storage} ".restore '${sqliteBackupFilePath}'"`);
+  // copy the backupFile to the new DB. The dot command is passed as a single
+  // argument to sqlite3, with no shell in between: a backup file name can no
+  // longer be read as a command.
+  await execFile('sqlite3', [this.config.storage, `.restore '${sqliteBackupFilePath}'`]);
   // done!
   logger.info(`SQLite backup restored`);
   if (duckDbBackupFolderPath) {
@@ -62,7 +65,9 @@ async function restoreBackup(sqliteBackupFilePath, duckDbBackupFolderPath) {
       .replace('CREATE SCHEMA information_schema;', '')
       .replace('CREATE SCHEMA pg_catalog;', '');
     await fse.writeFile(schemaFilePath, schemaCleaned);
-    await duckDbWriteConnection.run(`IMPORT DATABASE '${duckDbBackupFolderPath}'`);
+    // the folder name comes from the restored archive: DuckDB can read and write
+    // files from SQL, so the path is escaped before being inlined in the statement
+    await duckDbWriteConnection.run(`IMPORT DATABASE '${escapeSqlStringLiteral(duckDbBackupFolderPath)}'`);
     logger.info(`DuckDB restored with success`);
     duckDbWriteConnection.disconnectSync();
     duckDbInstance.closeSync();

server/services/mqtt/api/mqtt.controller.js

@@ -78,8 +78,10 @@ module.exports = function MqttController(mqttManager) {
       admin: true,
       controller: asyncMiddleware(setDebugMode),
     },
+    // returns the broker URL, username and password in clear
     'get /api/v1/service/mqtt/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/mqtt/config/docker': {

server/services/netatmo/api/netatmo.controller.js

@@ -98,12 +98,15 @@ module.exports = function NetatmoController(netatmoHandler) {
   }
 
   return {
+    // returns the Netatmo client id and client secret in clear
     'get /api/v1/service/netatmo/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/netatmo/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'get /api/v1/service/netatmo/status': {

server/services/nuki/api/nuki.controller.js

@@ -73,12 +73,15 @@ module.exports = function NukiController(nukiHandler) {
       authenticated: true,
       controller: asyncMiddleware(connect),
     },
+    // returns the Nuki API key in clear
     'get /api/v1/service/nuki/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/nuki/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'get /api/v1/service/nuki/discover/:protocol': {

server/services/tuya/api/tuya.controller.js

@@ -101,6 +101,7 @@ module.exports = function TuyaController(tuyaManager) {
     },
     'post /api/v1/service/tuya/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'post /api/v1/service/tuya/disconnect': {