Skip to content

Commit fe60dd5

Browse files
committed
refactor: validate Windows override values before shell injection
The wrapper injects org identifiers and Azure signing config into the electron-builder command as double-quoted CLI overrides, then runs it through spawnSync with shell:true (needed to split the space-separated multi-target string into argv). A value containing a double quote, newline, or shell metacharacter could shift argument boundaries or be expanded by the shell. These values come from repo maintainers, not untrusted input, so this isn't a security boundary — it's a fail-fast guard that surfaces a misconfigured repo Variable here, where it's fixable, instead of as a malformed electron-builder command line. Route both override sites through one helper that rejects the quote-breaking characters. The shell:true call itself is slated to go away in a future electron-builder upgrade, passing argv as an array so no quoting is needed at all.
1 parent f5c4b08 commit fe60dd5

1 file changed

Lines changed: 35 additions & 11 deletions

File tree

scripts/electron-builder-wrapper.js

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,30 @@ const getPlatformFlag = function () {
4747
throw new Error(`Could not determine platform flag for platform: ${process.platform}`);
4848
};
4949

50+
/**
51+
* Append a `--c.<key>="<value>"` electron-builder override after checking the value can't break out of
52+
* its double quotes. electron-builder runs through spawnSync with shell:true (needed to split the
53+
* space-separated multi-target string into argv), so a value containing a double quote, newline, or a
54+
* shell metacharacter could shift argument boundaries or be expanded by the shell. These values come
55+
* from repo maintainers (org identifiers, Azure signing config), not untrusted input, so this is a
56+
* fail-fast guard against a misconfigured repo Variable — it surfaces the problem here, where it's
57+
* fixable, rather than as a malformed electron-builder command line. (The shell:true call itself is
58+
* slated to go away in a future electron-builder upgrade, passing argv as an array so no quoting is
59+
* needed at all.)
60+
* @param {Array<string>} args - the electron-builder argument array to append to.
61+
* @param {string} key - the config key, e.g. 'appx.publisher'.
62+
* @param {string} value - the value to inject.
63+
*/
64+
const pushQuotedOverride = function (args, key, value) {
65+
if (/["\r\n`%]|\$\(/.test(value)) {
66+
throw new Error(
67+
`Refusing to inject --c.${key}: value contains a character that could break shell quoting ` +
68+
'(one of " \\r \\n ` % or $(). Check the repo Variable or signing config that supplies it.'
69+
);
70+
}
71+
args.push(`--c.${key}="${value}"`);
72+
};
73+
5074
/**
5175
* Run `electron-builder` once to build one or more target(s).
5276
* @param {object} wrapperConfig - overall configuration object for the wrapper script.
@@ -78,8 +102,7 @@ const runBuilder = function (wrapperConfig, target) {
78102
// Organization-specific identifiers. electron-builder only expands ${env.X} macros in
79103
// filename fields like artifactName, not in config fields like these, so we inject them as
80104
// CLI overrides from the environment. Each is applied whenever its variable is set (a fork
81-
// that doesn't set them builds with electron-builder's defaults). Values are double-quoted
82-
// so spaces (e.g. a CN= publisher string) survive spawnSync(shell: true) re-tokenization.
105+
// that doesn't set them builds with electron-builder's defaults).
83106
const idOverrides = {
84107
'appx.identityName': childEnvironment.APPX_IDENTITY_NAME,
85108
'appx.publisher': childEnvironment.APPX_PUBLISHER,
@@ -88,7 +111,7 @@ const runBuilder = function (wrapperConfig, target) {
88111
};
89112
for (const [key, value] of Object.entries(idOverrides)) {
90113
if (value) {
91-
allArgs.push(`--c.${key}="${value}"`);
114+
pushQuotedOverride(allArgs, key, value);
92115
}
93116
}
94117
}
@@ -114,14 +137,15 @@ const runBuilder = function (wrapperConfig, target) {
114137
if (missing.length > 0) {
115138
throw new Error(`Azure signing requires env vars: ${missing.join(', ')}`);
116139
}
117-
// Wrap values in double quotes so spaces (e.g. a multi-word publisherName) survive shell
118-
// re-tokenization. spawnSync(shell: true) joins argv with spaces and runs through cmd.exe /
119-
// /bin/sh, so any internal whitespace would otherwise become an arg separator.
120-
allArgs.push(
121-
`--c.win.azureSignOptions.endpoint="${childEnvironment.AZURE_SIGNING_ENDPOINT}"`,
122-
`--c.win.azureSignOptions.codeSigningAccountName="${childEnvironment.AZURE_SIGNING_ACCOUNT_NAME}"`,
123-
`--c.win.azureSignOptions.certificateProfileName="${childEnvironment.AZURE_SIGNING_PROFILE_NAME}"`,
124-
`--c.win.azureSignOptions.publisherName="${childEnvironment.AZURE_SIGNING_PUBLISHER_NAME}"`
140+
pushQuotedOverride(allArgs, 'win.azureSignOptions.endpoint', childEnvironment.AZURE_SIGNING_ENDPOINT);
141+
pushQuotedOverride(
142+
allArgs, 'win.azureSignOptions.codeSigningAccountName', childEnvironment.AZURE_SIGNING_ACCOUNT_NAME
143+
);
144+
pushQuotedOverride(
145+
allArgs, 'win.azureSignOptions.certificateProfileName', childEnvironment.AZURE_SIGNING_PROFILE_NAME
146+
);
147+
pushQuotedOverride(
148+
allArgs, 'win.azureSignOptions.publisherName', childEnvironment.AZURE_SIGNING_PUBLISHER_NAME
125149
);
126150
}
127151
if (!wrapperConfig.doPackage) {

0 commit comments

Comments
 (0)