From ec0ed9b2919b07e5fda58bf17799527fa6dd3f8d Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Mon, 15 Jun 2026 17:00:42 -0400 Subject: [PATCH 1/6] refactor: apply default env vars at configure-time, not at save Admin-defined default environment variables were merged into a container's DB record when the container was created. This froze the defaults to their creation-time values and exposed them in the edit UI. Now the container record stores only user-defined env vars. System defaults (from Settings) and NVIDIA GPU defaults are merged in at configure-time, just before the Proxmox API call, with user-defined values taking precedence. - models/container.js: buildLxcEnvConfig() now merges defaults < NVIDIA defaults < user vars; add getSystemDefaultEnvVars(), parseEnvironmentVars(), and nvidiaDefaultEnvVars() helpers. - bin/create-container.js: merge system defaults at configure-time and stop writing merged env/entrypoint back into the DB record. - bin/reconfigure-container.js: merge system defaults on edit so newly added defaults also reach existing containers. - routers/api/v1/containers.js: stop baking NVIDIA defaults into the saved record on create. Closes #349 --- create-a-container/bin/create-container.js | 100 +++------------- .../bin/reconfigure-container.js | 12 +- create-a-container/models/container.js | 109 ++++++++++++++---- .../routers/api/v1/containers.js | 10 +- 4 files changed, 119 insertions(+), 112 deletions(-) diff --git a/create-a-container/bin/create-container.js b/create-a-container/bin/create-container.js index 766ed815..7cf76351 100755 --- a/create-a-container/bin/create-container.js +++ b/create-a-container/bin/create-container.js @@ -340,62 +340,21 @@ async function main() { console.log('Container configured'); } - // Apply environment variables and entrypoint - // First read defaults from the image, then merge with user-specified values - const defaultConfig = await client.lxcConfig(node.name, vmid); - const defaultEntrypoint = defaultConfig['entrypoint'] || null; - const defaultEnvStr = defaultConfig['env'] || null; - - // Parse default env vars - let mergedEnvVars = {}; - if (defaultEnvStr) { - const pairs = defaultEnvStr.split('\0'); - for (const pair of pairs) { - const eqIndex = pair.indexOf('='); - if (eqIndex > 0) { - mergedEnvVars[pair.substring(0, eqIndex)] = pair.substring(eqIndex + 1); - } - } - } - - // Merge user-specified env vars (user values override defaults) - const userEnvVars = container.environmentVars ? JSON.parse(container.environmentVars) : {}; - - // Load system-wide default env vars from Settings. - // Descriptions are metadata only and are not passed into the container. - let systemDefaultEnvVars = {}; - try { - const entries = await Setting.getDefaultContainerEnvVars(); - for (const entry of entries) { - if (entry.key && entry.key.trim()) { - systemDefaultEnvVars[entry.key.trim()] = entry.value || ''; - } - } - } catch (_) { - console.warn('Could not load default_container_env_vars from settings, skipping'); - } + // Apply environment variables and entrypoint. + // Admin-defined system defaults are merged in here, at configure-time, and are + // intentionally NOT written back into the container's DB record (see below). + // Precedence (lowest to highest): system defaults < NVIDIA defaults < user values. + // Image-provided env/entrypoint defaults are submitted by the UI as user values, + // so they already live in container.environmentVars / container.entrypoint. + const systemDefaultEnvVars = await Container.getSystemDefaultEnvVars(); + const userEnvVars = container.parseEnvironmentVars(); + const envConfig = container.buildLxcEnvConfig(systemDefaultEnvVars); + // buildLxcEnvConfig may add a 'delete' key to unset env/entrypoint; on a fresh + // container there is nothing to delete, so drop it to avoid a no-op API param. + delete envConfig.delete; - // Merge priority: image defaults < system defaults < per-container user values - mergedEnvVars = { ...mergedEnvVars, ...systemDefaultEnvVars, ...userEnvVars }; - - // Use user entrypoint if specified, otherwise keep default - const finalEntrypoint = container.entrypoint || defaultEntrypoint; - - // Build config to apply - const envConfig = {}; - if (finalEntrypoint) { - envConfig.entrypoint = finalEntrypoint; - } - if (Object.keys(mergedEnvVars).length > 0) { - envConfig.env = Object.entries(mergedEnvVars) - .map(([key, value]) => `${key}=${value}`) - .join('\0'); - } - if (Object.keys(envConfig).length > 0) { console.log('Applying environment variables and entrypoint...'); - if (defaultEntrypoint) console.log(`Default entrypoint: ${defaultEntrypoint}`); - if (defaultEnvStr) console.log(`Image default env vars: ${Object.keys(mergedEnvVars).length - Object.keys(userEnvVars).length - Object.keys(systemDefaultEnvVars).length}`); if (Object.keys(systemDefaultEnvVars).length > 0) console.log(`System default env vars: ${Object.keys(systemDefaultEnvVars).length} from settings`); if (Object.keys(userEnvVars).length > 0) console.log(`Per-container env vars: ${Object.keys(userEnvVars).length}`); await client.updateLxcConfig(node.name, vmid, envConfig); @@ -447,11 +406,9 @@ async function main() { throw new Error('Could not extract MAC address from container configuration'); } - // Read back entrypoint and environment variables from config + // Read back configuration from Proxmox. console.log('Querying container configuration...'); const config = await client.lxcConfig(node.name, vmid); - const actualEntrypoint = config['entrypoint'] || null; - const actualEnv = config['env'] || null; // Read back the actual provisioned resources so downstream systems // (e.g. NetBox) mirror what the container really has rather than assuming @@ -459,28 +416,7 @@ async function main() { const actualCores = config['cores'] != null ? parseInt(config['cores'], 10) : null; const actualMemoryMb = config['memory'] != null ? parseInt(config['memory'], 10) : null; const actualDiskGb = parseRootfsSizeGb(config['rootfs']); - - // Parse NUL-separated env string back to JSON object - let environmentVars = {}; - if (actualEnv) { - const pairs = actualEnv.split('\0'); - for (const pair of pairs) { - const eqIndex = pair.indexOf('='); - if (eqIndex > 0) { - const key = pair.substring(0, eqIndex); - const value = pair.substring(eqIndex + 1); - environmentVars[key] = value; - } - } - } - - if (actualEntrypoint) { - console.log(`Entrypoint: ${actualEntrypoint}`); - } - if (Object.keys(environmentVars).length > 0) { - console.log(`Environment variables: ${Object.keys(environmentVars).length} vars`); - } - + // Get IP address from Proxmox interfaces API const ipv4Address = await client.getLxcIpAddress(node.name, vmid); @@ -488,13 +424,15 @@ async function main() { throw new Error('Could not get IP address from Proxmox interfaces API'); } - // Update the container record + // Update the container record. Note: environmentVars and entrypoint are NOT + // written back from the live config — the DB record stores only the values + // the user supplied. Admin-defined system defaults (and NVIDIA defaults) are + // merged in at configure-time and must not be persisted here, otherwise they + // would be frozen into the record and exposed in the edit UI. console.log('Updating container record...'); await container.update({ macAddress, ipv4Address, - entrypoint: actualEntrypoint, - environmentVars: JSON.stringify(environmentVars), status: 'running' }); diff --git a/create-a-container/bin/reconfigure-container.js b/create-a-container/bin/reconfigure-container.js index 5bcebf3a..753ccc94 100644 --- a/create-a-container/bin/reconfigure-container.js +++ b/create-a-container/bin/reconfigure-container.js @@ -80,8 +80,16 @@ async function main() { const client = await node.api(); console.log('Proxmox API client initialized'); - // Build config from environment variables and entrypoint - const lxcConfig = container.buildLxcEnvConfig(); + // Merge admin-defined default environment variables at configure-time rather + // than storing them in the container's DB record. Precedence (lowest to + // highest): system defaults < NVIDIA defaults < user-defined values. + // (NVIDIA and user-defined are applied inside buildLxcEnvConfig.) + // Image-provided defaults are submitted by the UI as user-defined values, so + // they are already part of container.environmentVars and not handled here. + const systemDefaultEnvVars = await Container.getSystemDefaultEnvVars(); + + // Build config from environment variables and entrypoint, merging defaults + const lxcConfig = container.buildLxcEnvConfig(systemDefaultEnvVars); if (Object.keys(lxcConfig).length > 0) { console.log('Applying LXC configuration...'); diff --git a/create-a-container/models/container.js b/create-a-container/models/container.js index e0ae3f18..f9a0b102 100644 --- a/create-a-container/models/container.js +++ b/create-a-container/models/container.js @@ -21,37 +21,100 @@ module.exports = (sequelize, DataTypes) => { } /** - * Build LXC config object for environment variables and entrypoint - * Returns config suitable for Proxmox API updateLxcConfig + * Load the admin-defined system default environment variables from the + * Settings table, flattened to a { KEY: value } object. Descriptions are + * metadata only and are not included. Returns an empty object if the + * setting is missing or malformed. + * @returns {Promise} Flat object of { KEY: value } system defaults + */ + static async getSystemDefaultEnvVars() { + const Setting = this.sequelize.models.Setting; + const defaults = {}; + try { + const entries = await Setting.getDefaultContainerEnvVars(); + for (const entry of entries) { + if (entry.key && entry.key.trim()) { + defaults[entry.key.trim()] = entry.value || ''; + } + } + } catch (_) { + console.warn('Could not load default_container_env_vars from settings, skipping'); + } + return defaults; + } + + /** + * Parse the container's user-defined environment variables. + * The database record only ever stores the variables the user explicitly + * provided — admin/system, image, and NVIDIA defaults are merged in at + * configure-time (see buildLxcEnvConfig) and are intentionally NOT persisted. + * @returns {object} Flat object of { KEY: value } user-defined env vars + */ + parseEnvironmentVars() { + if (!this.environmentVars) return {}; + try { + const parsed = JSON.parse(this.environmentVars); + return parsed && typeof parsed === 'object' ? parsed : {}; + } catch (err) { + console.error('Failed to parse environment variables JSON:', err.message); + return {}; + } + } + + /** + * Environment variables implied by this container's configuration that are + * applied as defaults but never stored in the DB record. Currently this is + * the NVIDIA GPU passthrough defaults, applied when nvidiaRequested is set. + * @returns {object} Flat object of { KEY: value } defaults + */ + nvidiaDefaultEnvVars() { + if (!this.nvidiaRequested) return {}; + return { + NVIDIA_VISIBLE_DEVICES: 'all', + NVIDIA_DRIVER_CAPABILITIES: 'utility compute' + }; + } + + /** + * Build LXC config object for environment variables and entrypoint. + * Returns config suitable for Proxmox API updateLxcConfig. + * + * Default environment variables are merged in here, at configure-time, + * rather than being baked into the container's DB record. User-defined + * variables take precedence over any provided defaults. + * + * @param {object} [defaults={}] - Flat object of default env vars, e.g. the + * admin-defined system defaults. User-defined values and this container's + * own NVIDIA defaults override these. * @returns {object} Config object with 'env' and 'entrypoint' properties */ - buildLxcEnvConfig() { + buildLxcEnvConfig(defaults = {}) { const config = {}; const deleteList = []; - - // Parse environment variables from JSON and format as NUL-separated list - // Format: KEY1=value1\0KEY2=value2\0KEY3=value3 - if (this.environmentVars) { - try { - const envObj = JSON.parse(this.environmentVars); - const envPairs = []; - for (const [key, value] of Object.entries(envObj)) { - if (key && value !== undefined) { - envPairs.push(`${key}=${value}`); - } - } - if (envPairs.length > 0) { - config['env'] = envPairs.join('\0'); - } else { - deleteList.push('env'); - } - } catch (err) { - console.error('Failed to parse environment variables JSON:', err.message); + + // Merge precedence (lowest to highest): + // provided defaults (admin-defined system defaults) < NVIDIA defaults + // < user-defined values + const userEnvVars = this.parseEnvironmentVars(); + const mergedEnvVars = { + ...(defaults && typeof defaults === 'object' ? defaults : {}), + ...this.nvidiaDefaultEnvVars(), + ...userEnvVars + }; + + // Format as NUL-separated list: KEY1=value1\0KEY2=value2\0KEY3=value3 + const envPairs = []; + for (const [key, value] of Object.entries(mergedEnvVars)) { + if (key && value !== undefined) { + envPairs.push(`${key}=${value}`); } + } + if (envPairs.length > 0) { + config['env'] = envPairs.join('\0'); } else { deleteList.push('env'); } - + // Set entrypoint command if (this.entrypoint && this.entrypoint.trim()) { config['entrypoint'] = this.entrypoint.trim(); diff --git a/create-a-container/routers/api/v1/containers.js b/create-a-container/routers/api/v1/containers.js index 2ca64da8..ba62d7a1 100644 --- a/create-a-container/routers/api/v1/containers.js +++ b/create-a-container/routers/api/v1/containers.js @@ -243,6 +243,10 @@ router.post( if (!hostname || !hostname.trim()) throw new ApiError(400, 'invalid_request', 'hostname is required'); const wantsNvidia = !!nvidiaRequested; + // Only the user-defined env vars are persisted on the container record. + // NVIDIA defaults (for GPU passthrough) and admin-defined system defaults + // are merged in at configure-time by the create/reconfigure jobs, so they + // are intentionally not stored here. let envVarsJson = null; if (Array.isArray(environmentVars) && environmentVars.length > 0) { const envObj = {}; @@ -251,12 +255,6 @@ router.post( } if (Object.keys(envObj).length > 0) envVarsJson = JSON.stringify(envObj); } - if (wantsNvidia) { - const obj = envVarsJson ? JSON.parse(envVarsJson) : {}; - if (!obj.NVIDIA_VISIBLE_DEVICES) obj.NVIDIA_VISIBLE_DEVICES = 'all'; - if (!obj.NVIDIA_DRIVER_CAPABILITIES) obj.NVIDIA_DRIVER_CAPABILITIES = 'utility compute'; - envVarsJson = JSON.stringify(obj); - } const imageRef = template === 'custom' ? customTemplate?.trim() : template; if (!imageRef) throw new ApiError(400, 'invalid_request', 'template is required'); From b6537115c29eed198a5cd2487dc477edacd66398 Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Tue, 16 Jun 2026 08:19:34 -0400 Subject: [PATCH 2/6] refactor: make buildLxcEnvConfig the single env-merge entrypoint Move all environment-variable merge logic into Container#buildLxcEnvConfig so there is exactly one place that determines the env to deploy. The method now loads the admin-defined system defaults itself (it became async) and composes them with the NVIDIA and user-defined values: system defaults < NVIDIA defaults < user-defined values Callers in create-container.js and reconfigure-container.js are reduced to a single `await container.buildLxcEnvConfig()` call and no longer load or pass defaults themselves. getSystemDefaultEnvVars, parseEnvironmentVars and nvidiaDefaultEnvVars are now internal helpers of that method. --- create-a-container/bin/create-container.js | 20 ++++------ .../bin/reconfigure-container.js | 15 +++----- create-a-container/models/container.js | 37 +++++++++++-------- 3 files changed, 34 insertions(+), 38 deletions(-) diff --git a/create-a-container/bin/create-container.js b/create-a-container/bin/create-container.js index 7cf76351..57fd84ba 100755 --- a/create-a-container/bin/create-container.js +++ b/create-a-container/bin/create-container.js @@ -340,23 +340,17 @@ async function main() { console.log('Container configured'); } - // Apply environment variables and entrypoint. - // Admin-defined system defaults are merged in here, at configure-time, and are - // intentionally NOT written back into the container's DB record (see below). - // Precedence (lowest to highest): system defaults < NVIDIA defaults < user values. - // Image-provided env/entrypoint defaults are submitted by the UI as user values, - // so they already live in container.environmentVars / container.entrypoint. - const systemDefaultEnvVars = await Container.getSystemDefaultEnvVars(); - const userEnvVars = container.parseEnvironmentVars(); - const envConfig = container.buildLxcEnvConfig(systemDefaultEnvVars); - // buildLxcEnvConfig may add a 'delete' key to unset env/entrypoint; on a fresh - // container there is nothing to delete, so drop it to avoid a no-op API param. + // Apply environment variables and entrypoint. buildLxcEnvConfig is the single + // source of truth for the effective env: it merges admin-defined system + // defaults and NVIDIA defaults under the user-defined values. None of this is + // written back into the DB record (see below). + const envConfig = await container.buildLxcEnvConfig(); + // On a fresh container there is nothing to unset, so drop any 'delete' key to + // avoid a no-op API param. delete envConfig.delete; if (Object.keys(envConfig).length > 0) { console.log('Applying environment variables and entrypoint...'); - if (Object.keys(systemDefaultEnvVars).length > 0) console.log(`System default env vars: ${Object.keys(systemDefaultEnvVars).length} from settings`); - if (Object.keys(userEnvVars).length > 0) console.log(`Per-container env vars: ${Object.keys(userEnvVars).length}`); await client.updateLxcConfig(node.name, vmid, envConfig); console.log('Environment/entrypoint configuration applied'); } diff --git a/create-a-container/bin/reconfigure-container.js b/create-a-container/bin/reconfigure-container.js index 753ccc94..1a345c8e 100644 --- a/create-a-container/bin/reconfigure-container.js +++ b/create-a-container/bin/reconfigure-container.js @@ -80,16 +80,11 @@ async function main() { const client = await node.api(); console.log('Proxmox API client initialized'); - // Merge admin-defined default environment variables at configure-time rather - // than storing them in the container's DB record. Precedence (lowest to - // highest): system defaults < NVIDIA defaults < user-defined values. - // (NVIDIA and user-defined are applied inside buildLxcEnvConfig.) - // Image-provided defaults are submitted by the UI as user-defined values, so - // they are already part of container.environmentVars and not handled here. - const systemDefaultEnvVars = await Container.getSystemDefaultEnvVars(); - - // Build config from environment variables and entrypoint, merging defaults - const lxcConfig = container.buildLxcEnvConfig(systemDefaultEnvVars); + // Build config from environment variables and entrypoint. buildLxcEnvConfig + // is the single source of truth for the effective env: it merges admin-defined + // system defaults and NVIDIA defaults under the user-defined values, applied + // here at configure-time rather than stored in the DB record. + const lxcConfig = await container.buildLxcEnvConfig(); if (Object.keys(lxcConfig).length > 0) { console.log('Applying LXC configuration...'); diff --git a/create-a-container/models/container.js b/create-a-container/models/container.js index f9a0b102..7bf58264 100644 --- a/create-a-container/models/container.js +++ b/create-a-container/models/container.js @@ -21,6 +21,7 @@ module.exports = (sequelize, DataTypes) => { } /** + * Internal helper for buildLxcEnvConfig. * Load the admin-defined system default environment variables from the * Settings table, flattened to a { KEY: value } object. Descriptions are * metadata only and are not included. Returns an empty object if the @@ -44,6 +45,7 @@ module.exports = (sequelize, DataTypes) => { } /** + * Internal helper for buildLxcEnvConfig. * Parse the container's user-defined environment variables. * The database record only ever stores the variables the user explicitly * provided — admin/system, image, and NVIDIA defaults are merged in at @@ -62,6 +64,7 @@ module.exports = (sequelize, DataTypes) => { } /** + * Internal helper for buildLxcEnvConfig. * Environment variables implied by this container's configuration that are * applied as defaults but never stored in the DB record. Currently this is * the NVIDIA GPU passthrough defaults, applied when nvidiaRequested is set. @@ -76,30 +79,34 @@ module.exports = (sequelize, DataTypes) => { } /** - * Build LXC config object for environment variables and entrypoint. - * Returns config suitable for Proxmox API updateLxcConfig. + * Build the LXC config object for environment variables and entrypoint to + * deploy to Proxmox via updateLxcConfig. * - * Default environment variables are merged in here, at configure-time, - * rather than being baked into the container's DB record. User-defined - * variables take precedence over any provided defaults. + * This is the single entrypoint for determining a container's effective + * environment. It owns the full merge of every env-var source, applied here + * at configure-time rather than being baked into the container's DB record: * - * @param {object} [defaults={}] - Flat object of default env vars, e.g. the - * admin-defined system defaults. User-defined values and this container's - * own NVIDIA defaults override these. - * @returns {object} Config object with 'env' and 'entrypoint' properties + * admin-defined system defaults < NVIDIA defaults < user-defined values + * + * (Image-provided defaults are submitted by the UI as user-defined values, + * so they arrive via parseEnvironmentVars and need no special handling.) + * + * Callers should simply `await container.buildLxcEnvConfig()` — no env-var + * merging belongs anywhere else. + * + * @returns {Promise} Config object with 'env' and 'entrypoint' + * properties (and a 'delete' list for any that should be unset) */ - buildLxcEnvConfig(defaults = {}) { + async buildLxcEnvConfig() { const config = {}; const deleteList = []; // Merge precedence (lowest to highest): - // provided defaults (admin-defined system defaults) < NVIDIA defaults - // < user-defined values - const userEnvVars = this.parseEnvironmentVars(); + // system defaults < NVIDIA defaults < user-defined values const mergedEnvVars = { - ...(defaults && typeof defaults === 'object' ? defaults : {}), + ...(await this.constructor.getSystemDefaultEnvVars()), ...this.nvidiaDefaultEnvVars(), - ...userEnvVars + ...this.parseEnvironmentVars() }; // Format as NUL-separated list: KEY1=value1\0KEY2=value2\0KEY3=value3 From b8230ed36e99cd5066a23833d80e90ca30868820 Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Tue, 16 Jun 2026 08:53:25 -0400 Subject: [PATCH 3/6] cleanup --- create-a-container/bin/create-container.js | 15 ++------------- create-a-container/bin/reconfigure-container.js | 5 +---- create-a-container/routers/api/v1/containers.js | 4 ---- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/create-a-container/bin/create-container.js b/create-a-container/bin/create-container.js index 57fd84ba..b1b81f2b 100755 --- a/create-a-container/bin/create-container.js +++ b/create-a-container/bin/create-container.js @@ -340,15 +340,8 @@ async function main() { console.log('Container configured'); } - // Apply environment variables and entrypoint. buildLxcEnvConfig is the single - // source of truth for the effective env: it merges admin-defined system - // defaults and NVIDIA defaults under the user-defined values. None of this is - // written back into the DB record (see below). + // Apply environment variables and entrypoint const envConfig = await container.buildLxcEnvConfig(); - // On a fresh container there is nothing to unset, so drop any 'delete' key to - // avoid a no-op API param. - delete envConfig.delete; - if (Object.keys(envConfig).length > 0) { console.log('Applying environment variables and entrypoint...'); await client.updateLxcConfig(node.name, vmid, envConfig); @@ -418,11 +411,7 @@ async function main() { throw new Error('Could not get IP address from Proxmox interfaces API'); } - // Update the container record. Note: environmentVars and entrypoint are NOT - // written back from the live config — the DB record stores only the values - // the user supplied. Admin-defined system defaults (and NVIDIA defaults) are - // merged in at configure-time and must not be persisted here, otherwise they - // would be frozen into the record and exposed in the edit UI. + // Update the container record console.log('Updating container record...'); await container.update({ macAddress, diff --git a/create-a-container/bin/reconfigure-container.js b/create-a-container/bin/reconfigure-container.js index 1a345c8e..11d8e031 100644 --- a/create-a-container/bin/reconfigure-container.js +++ b/create-a-container/bin/reconfigure-container.js @@ -80,10 +80,7 @@ async function main() { const client = await node.api(); console.log('Proxmox API client initialized'); - // Build config from environment variables and entrypoint. buildLxcEnvConfig - // is the single source of truth for the effective env: it merges admin-defined - // system defaults and NVIDIA defaults under the user-defined values, applied - // here at configure-time rather than stored in the DB record. + // Build config from environment variables and entrypoint const lxcConfig = await container.buildLxcEnvConfig(); if (Object.keys(lxcConfig).length > 0) { diff --git a/create-a-container/routers/api/v1/containers.js b/create-a-container/routers/api/v1/containers.js index ba62d7a1..a4556ad8 100644 --- a/create-a-container/routers/api/v1/containers.js +++ b/create-a-container/routers/api/v1/containers.js @@ -243,10 +243,6 @@ router.post( if (!hostname || !hostname.trim()) throw new ApiError(400, 'invalid_request', 'hostname is required'); const wantsNvidia = !!nvidiaRequested; - // Only the user-defined env vars are persisted on the container record. - // NVIDIA defaults (for GPU passthrough) and admin-defined system defaults - // are merged in at configure-time by the create/reconfigure jobs, so they - // are intentionally not stored here. let envVarsJson = null; if (Array.isArray(environmentVars) && environmentVars.length > 0) { const envObj = {}; From 8dd152e531f053b5c8c663fce48bec6b126bf8b3 Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Tue, 16 Jun 2026 09:10:33 -0400 Subject: [PATCH 4/6] fix: validate and normalize env var keys/values to prevent corrupting the Proxmox env string Addresses PR review: env var keys were only trimmed, so a key containing '=' or NUL (or a non-string/array value) could corrupt the NUL-separated KEY=value blob sent to Proxmox (splitting one var into many, breaking parsing, or stringifying objects to "[object Object]"). Add Container.normalizeEnvVars() as the single definition of a valid env var: input must be a plain object; keys are trimmed and must match a conventional env-var name (no '=', NUL, or whitespace); values are coerced to strings, with null/undefined, objects/arrays, and NUL-containing values dropped. Apply it at every ingest/load point: - getSystemDefaultEnvVars() and parseEnvironmentVars() normalize their output. - API create/update routes serialize user env vars via a shared serializeUserEnvVars() helper that normalizes before persisting. buildLxcEnvConfig can now trust its inputs, so the per-pair guards are dropped. --- create-a-container/models/container.js | 78 +++++++++++++++---- .../routers/api/v1/containers.js | 36 +++++---- 2 files changed, 84 insertions(+), 30 deletions(-) diff --git a/create-a-container/models/container.js b/create-a-container/models/container.js index 7bf58264..a715f1b8 100644 --- a/create-a-container/models/container.js +++ b/create-a-container/models/container.js @@ -20,43 +20,87 @@ module.exports = (sequelize, DataTypes) => { Container.belongsTo(models.Job, { foreignKey: 'creationJobId', as: 'creationJob' }); } + /** + * Normalize a set of environment variables into a safe, flat + * { KEY: stringValue } object suitable for building the Proxmox `env` + * string. This is the single place that decides what a valid env var is. + * + * Rules (applied to every source — user input, settings, image defaults): + * - Input that is not a plain object (e.g. an array or null) yields {}. + * - Keys are trimmed. A key is dropped unless it is a non-empty string with + * no whitespace and no `=` or NUL — i.e. it matches a conventional env + * var name. This prevents a key from corrupting the NUL-separated + * `KEY=value` encoding. + * - Values are coerced to strings. Entries whose value is null/undefined or + * a non-primitive (object/array) are dropped rather than stringified to + * something like "[object Object]". Values containing NUL are also + * dropped, since NUL is the pair separator. + * + * @param {*} input - Candidate env vars, ideally a { key: value } object + * @returns {object} Flat object of validated { KEY: stringValue } + */ + static normalizeEnvVars(input) { + const out = {}; + if (!input || typeof input !== 'object' || Array.isArray(input)) return out; + + // Conventional env var name: starts with a letter or underscore, followed + // by letters, digits, or underscores. Excludes `=`, NUL, whitespace, etc. + const validKey = /^[A-Za-z_][A-Za-z0-9_]*$/; + + for (const [rawKey, rawValue] of Object.entries(input)) { + const key = typeof rawKey === 'string' ? rawKey.trim() : ''; + if (!validKey.test(key)) continue; + + // Only primitives (string/number/boolean) become values; skip + // null/undefined and objects/arrays. + if (rawValue === null || rawValue === undefined) continue; + if (typeof rawValue === 'object') continue; + const value = String(rawValue); + if (value.includes('\0')) continue; + + out[key] = value; + } + return out; + } + /** * Internal helper for buildLxcEnvConfig. * Load the admin-defined system default environment variables from the - * Settings table, flattened to a { KEY: value } object. Descriptions are - * metadata only and are not included. Returns an empty object if the - * setting is missing or malformed. + * Settings table, flattened to a validated { KEY: value } object. + * Descriptions are metadata only and are not included. Returns an empty + * object if the setting is missing or malformed. * @returns {Promise} Flat object of { KEY: value } system defaults */ static async getSystemDefaultEnvVars() { const Setting = this.sequelize.models.Setting; - const defaults = {}; + const raw = {}; try { const entries = await Setting.getDefaultContainerEnvVars(); for (const entry of entries) { - if (entry.key && entry.key.trim()) { - defaults[entry.key.trim()] = entry.value || ''; + // getDefaultContainerEnvVars yields { key, value, description }. + if (entry && typeof entry.key === 'string') { + raw[entry.key] = entry.value; } } } catch (_) { console.warn('Could not load default_container_env_vars from settings, skipping'); } - return defaults; + return this.normalizeEnvVars(raw); } /** * Internal helper for buildLxcEnvConfig. - * Parse the container's user-defined environment variables. - * The database record only ever stores the variables the user explicitly - * provided — admin/system, image, and NVIDIA defaults are merged in at - * configure-time (see buildLxcEnvConfig) and are intentionally NOT persisted. - * @returns {object} Flat object of { KEY: value } user-defined env vars + * Parse the container's user-defined environment variables into a validated, + * flat { KEY: value } object. The database record only ever stores the + * variables the user explicitly provided — admin/system, image, and NVIDIA + * defaults are merged in at configure-time (see buildLxcEnvConfig) and are + * intentionally NOT persisted. + * @returns {object} Flat object of validated { KEY: value } user env vars */ parseEnvironmentVars() { if (!this.environmentVars) return {}; try { - const parsed = JSON.parse(this.environmentVars); - return parsed && typeof parsed === 'object' ? parsed : {}; + return this.constructor.normalizeEnvVars(JSON.parse(this.environmentVars)); } catch (err) { console.error('Failed to parse environment variables JSON:', err.message); return {}; @@ -103,6 +147,8 @@ module.exports = (sequelize, DataTypes) => { // Merge precedence (lowest to highest): // system defaults < NVIDIA defaults < user-defined values + // Every source is already normalized to a safe { KEY: stringValue } map + // (see normalizeEnvVars), so the encoding below cannot be corrupted. const mergedEnvVars = { ...(await this.constructor.getSystemDefaultEnvVars()), ...this.nvidiaDefaultEnvVars(), @@ -112,9 +158,7 @@ module.exports = (sequelize, DataTypes) => { // Format as NUL-separated list: KEY1=value1\0KEY2=value2\0KEY3=value3 const envPairs = []; for (const [key, value] of Object.entries(mergedEnvVars)) { - if (key && value !== undefined) { - envPairs.push(`${key}=${value}`); - } + envPairs.push(`${key}=${value}`); } if (envPairs.length > 0) { config['env'] = envPairs.join('\0'); diff --git a/create-a-container/routers/api/v1/containers.js b/create-a-container/routers/api/v1/containers.js index a4556ad8..70c2947d 100644 --- a/create-a-container/routers/api/v1/containers.js +++ b/create-a-container/routers/api/v1/containers.js @@ -27,6 +27,25 @@ const router = express.Router({ mergeParams: true }); router.use(apiAuth); +/** + * Convert the `environmentVars` request payload (an array of { key, value } + * objects) into the JSON string stored on the container record, or null when + * there are no valid vars. Keys/values are validated and normalized via + * Container.normalizeEnvVars so only safe env var names are persisted (keys + * containing `=`/NUL or non-primitive values are dropped at ingest time). + * @param {Array<{key: string, value: *}>} environmentVars + * @returns {string|null} + */ +function serializeUserEnvVars(environmentVars) { + if (!Array.isArray(environmentVars)) return null; + const flat = {}; + for (const e of environmentVars) { + if (e && typeof e.key === 'string') flat[e.key] = e.value; + } + const normalized = Container.normalizeEnvVars(flat); + return Object.keys(normalized).length > 0 ? JSON.stringify(normalized) : null; +} + function normalizeDockerRef(ref) { if (ref.startsWith('http://') || ref.startsWith('https://') || ref.startsWith('git@')) return ref; let tag = 'latest'; @@ -243,14 +262,9 @@ router.post( if (!hostname || !hostname.trim()) throw new ApiError(400, 'invalid_request', 'hostname is required'); const wantsNvidia = !!nvidiaRequested; - let envVarsJson = null; - if (Array.isArray(environmentVars) && environmentVars.length > 0) { - const envObj = {}; - for (const e of environmentVars) { - if (e?.key && e.key.trim()) envObj[e.key.trim()] = e.value || ''; - } - if (Object.keys(envObj).length > 0) envVarsJson = JSON.stringify(envObj); - } + // Only the user-defined env vars are persisted on the container record; + // NVIDIA and admin-defined system defaults are merged in at configure-time. + const envVarsJson = serializeUserEnvVars(environmentVars); const imageRef = template === 'custom' ? customTemplate?.trim() : template; if (!imageRef) throw new ApiError(400, 'invalid_request', 'template is required'); @@ -383,11 +397,7 @@ router.put( let envVarsJson = container.environmentVars; if (!isRestartOnly && Array.isArray(environmentVars)) { - const obj = {}; - for (const e of environmentVars) { - if (e?.key) obj[e.key.trim()] = e.value || ''; - } - envVarsJson = Object.keys(obj).length > 0 ? JSON.stringify(obj) : null; + envVarsJson = serializeUserEnvVars(environmentVars); } else if (!isRestartOnly && !environmentVars) { envVarsJson = null; } From 2f968d0dcb919364b3690c78b175f5438e190f1f Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Tue, 16 Jun 2026 09:30:06 -0400 Subject: [PATCH 5/6] fix: don't unset template-provided env/entrypoint on container create buildLxcEnvConfig always added env/entrypoint to Proxmox's `delete` list when the container record had no value. On create the container is freshly cloned from a template that may carry its own env/entrypoint, so emitting `delete` actively wiped those template-provided defaults. Add a deleteMissing option (default false). Proxmox's config PUT is a partial update, so omitting a key leaves it untouched: - create-container.js uses the default (false): empty env/entrypoint are omitted, preserving whatever the cloned template provides. - reconfigure-container.js passes deleteMissing:true so clearing the last env var or removing a custom entrypoint still unsets it on the existing container. The reconfigure restart-detection already ignores the `delete` key, so a pure-delete config does not, by itself, trigger a restart. --- create-a-container/bin/create-container.js | 5 +++- .../bin/reconfigure-container.js | 7 +++-- create-a-container/models/container.js | 26 ++++++++++++++----- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/create-a-container/bin/create-container.js b/create-a-container/bin/create-container.js index b1b81f2b..81ae0399 100755 --- a/create-a-container/bin/create-container.js +++ b/create-a-container/bin/create-container.js @@ -340,7 +340,10 @@ async function main() { console.log('Container configured'); } - // Apply environment variables and entrypoint + // Apply environment variables and entrypoint. Use the default + // (deleteMissing=false) so that any env/entrypoint provided by the template + // we just cloned is preserved when the user didn't supply their own — only + // explicit values are pushed, nothing is unset. const envConfig = await container.buildLxcEnvConfig(); if (Object.keys(envConfig).length > 0) { console.log('Applying environment variables and entrypoint...'); diff --git a/create-a-container/bin/reconfigure-container.js b/create-a-container/bin/reconfigure-container.js index 11d8e031..ee5ba86e 100644 --- a/create-a-container/bin/reconfigure-container.js +++ b/create-a-container/bin/reconfigure-container.js @@ -80,8 +80,11 @@ async function main() { const client = await node.api(); console.log('Proxmox API client initialized'); - // Build config from environment variables and entrypoint - const lxcConfig = await container.buildLxcEnvConfig(); + // Build config from environment variables and entrypoint. Pass + // deleteMissing so that clearing env vars or removing a custom entrypoint + // actually unsets them on the existing container (vs. create, which must + // preserve template-provided values). + const lxcConfig = await container.buildLxcEnvConfig({ deleteMissing: true }); if (Object.keys(lxcConfig).length > 0) { console.log('Applying LXC configuration...'); diff --git a/create-a-container/models/container.js b/create-a-container/models/container.js index a715f1b8..8f28ae57 100644 --- a/create-a-container/models/container.js +++ b/create-a-container/models/container.js @@ -135,13 +135,27 @@ module.exports = (sequelize, DataTypes) => { * (Image-provided defaults are submitted by the UI as user-defined values, * so they arrive via parseEnvironmentVars and need no special handling.) * - * Callers should simply `await container.buildLxcEnvConfig()` — no env-var - * merging belongs anywhere else. + * Proxmox's config endpoint is a partial update: keys present in the body are + * set, omitted keys are left untouched, and a key is only removed if named in + * the special `delete` parameter. That distinction matters here: * + * - On create, the container has just been cloned from a template that may + * carry its own `env`/`entrypoint`. We must NOT delete those when the user + * didn't provide a value, or we'd wipe the template's defaults. So the + * default (deleteMissing=false) simply omits anything with no value. + * - On reconfigure, the user may have cleared their last env var or removed a + * custom entrypoint, and that change must take effect — so the caller + * passes deleteMissing=true to emit `delete` for the now-empty fields. + * + * @param {object} [options] + * @param {boolean} [options.deleteMissing=false] - When true, env/entrypoint + * that resolve to empty are added to Proxmox's `delete` list (removing any + * existing value). When false, they are simply omitted, preserving whatever + * the container/template already has. * @returns {Promise} Config object with 'env' and 'entrypoint' - * properties (and a 'delete' list for any that should be unset) + * properties (and, when deleteMissing is set, a 'delete' list) */ - async buildLxcEnvConfig() { + async buildLxcEnvConfig({ deleteMissing = false } = {}) { const config = {}; const deleteList = []; @@ -162,14 +176,14 @@ module.exports = (sequelize, DataTypes) => { } if (envPairs.length > 0) { config['env'] = envPairs.join('\0'); - } else { + } else if (deleteMissing) { deleteList.push('env'); } // Set entrypoint command if (this.entrypoint && this.entrypoint.trim()) { config['entrypoint'] = this.entrypoint.trim(); - } else { + } else if (deleteMissing) { deleteList.push('entrypoint'); } From 87875376c003a9b4ebc18828df76344d41453142 Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Tue, 16 Jun 2026 09:56:05 -0400 Subject: [PATCH 6/6] feat: snapshot template env/entrypoint onto the record at create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconfigure uses deleteMissing, so any env/entrypoint that exists only on the cloned template (and was never stored on the container) would be unset on the first reconfigure — surprising the user. We can't re-query the template later: templates are mutable Docker refs that may change or disappear, and we don't cache them. Compromise: at creation, after the template is cloned, read its LXC config and fold its env/entrypoint into the container record as if the user had supplied them (precedence: template < user). This guarantees those values persist across future reconfigures without changing API client expectations. System and NVIDIA defaults are deliberately NOT persisted — they remain configure-time-only (merged by buildLxcEnvConfig) so they aren't frozen into the record or exposed as user values in the edit UI. - models/container.js: add Container.parseLxcEnvString() and Container#persistTemplateDefaults(). - bin/create-container.js: call persistTemplateDefaults() with the cloned template's config before building/applying the LXC env config. --- create-a-container/bin/create-container.js | 16 ++++-- create-a-container/models/container.js | 58 ++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/create-a-container/bin/create-container.js b/create-a-container/bin/create-container.js index 81ae0399..2ea89ea3 100755 --- a/create-a-container/bin/create-container.js +++ b/create-a-container/bin/create-container.js @@ -340,10 +340,20 @@ async function main() { console.log('Container configured'); } + // Snapshot the template's env/entrypoint onto the container record now, as + // if the user had supplied them (user-supplied values still win). Templates + // are mutable Docker refs we can't re-query on a later reconfigure, so we + // persist them here; otherwise a future reconfigure (which uses + // deleteMissing) would unset template-provided values that were never + // stored. System/NVIDIA defaults are intentionally left out — they stay + // configure-time-only. + const templateConfig = await client.lxcConfig(node.name, vmid); + await container.persistTemplateDefaults(templateConfig); + // Apply environment variables and entrypoint. Use the default - // (deleteMissing=false) so that any env/entrypoint provided by the template - // we just cloned is preserved when the user didn't supply their own — only - // explicit values are pushed, nothing is unset. + // (deleteMissing=false): only explicit values are pushed, nothing is unset. + // The record now already includes the template's values, and system/NVIDIA + // defaults are merged in by buildLxcEnvConfig. const envConfig = await container.buildLxcEnvConfig(); if (Object.keys(envConfig).length > 0) { console.log('Applying environment variables and entrypoint...'); diff --git a/create-a-container/models/container.js b/create-a-container/models/container.js index 8f28ae57..721cebfe 100644 --- a/create-a-container/models/container.js +++ b/create-a-container/models/container.js @@ -63,6 +63,24 @@ module.exports = (sequelize, DataTypes) => { return out; } + /** + * Parse a Proxmox LXC `env` string (NUL-separated `KEY=value` pairs) into a + * normalized, flat { KEY: stringValue } object. Anything malformed is + * dropped via normalizeEnvVars. The inverse of the encoding done in + * buildLxcEnvConfig. + * @param {string|null|undefined} envStr - Raw Proxmox `env` value + * @returns {object} Flat object of validated { KEY: value } + */ + static parseLxcEnvString(envStr) { + if (!envStr || typeof envStr !== 'string') return {}; + const raw = {}; + for (const pair of envStr.split('\0')) { + const eq = pair.indexOf('='); + if (eq > 0) raw[pair.substring(0, eq)] = pair.substring(eq + 1); + } + return this.normalizeEnvVars(raw); + } + /** * Internal helper for buildLxcEnvConfig. * Load the admin-defined system default environment variables from the @@ -122,6 +140,46 @@ module.exports = (sequelize, DataTypes) => { }; } + /** + * Fold a template's environment variables and entrypoint into this + * container's persisted record, as if the user had supplied them. + * + * Called once at creation, after the template has been cloned, with the + * template's own LXC config. We can't recover these values later (templates + * are mutable Docker refs that may change or disappear before a reconfigure, + * and we don't cache them), so we snapshot them onto the record now. This + * keeps env/entrypoint stable across future reconfigures — which use + * deleteMissing and would otherwise unset template-provided values that were + * never stored. + * + * Precedence is template < user (user-supplied values win). System and + * NVIDIA defaults are intentionally NOT folded in here; they remain + * configure-time-only so they are not frozen into the record. + * + * Persists and returns nothing; mutates this instance. + * + * @param {object} templateConfig - The cloned template's LXC config + * @param {string} [templateConfig.env] - Proxmox NUL-separated `env` string + * @param {string} [templateConfig.entrypoint] - Template entrypoint + */ + async persistTemplateDefaults(templateConfig = {}) { + const templateEnv = this.constructor.parseLxcEnvString(templateConfig.env); + // template < user + const mergedEnv = { ...templateEnv, ...this.parseEnvironmentVars() }; + + const templateEntrypoint = + typeof templateConfig.entrypoint === 'string' && templateConfig.entrypoint.trim() + ? templateConfig.entrypoint.trim() + : null; + const userEntrypoint = this.entrypoint && this.entrypoint.trim() ? this.entrypoint.trim() : null; + const mergedEntrypoint = userEntrypoint || templateEntrypoint; + + await this.update({ + environmentVars: Object.keys(mergedEnv).length > 0 ? JSON.stringify(mergedEnv) : null, + entrypoint: mergedEntrypoint + }); + } + /** * Build the LXC config object for environment variables and entrypoint to * deploy to Proxmox via updateLxcConfig.