diff --git a/apps/desktop/BUILDING.md b/apps/desktop/BUILDING.md index 85aa2c8777..7e4563ce06 100644 --- a/apps/desktop/BUILDING.md +++ b/apps/desktop/BUILDING.md @@ -184,8 +184,11 @@ runtime overrides; `--bundle-env HERMES_HOME=` is a default, not a forced clear. For local commit builds, `HERMES_BUNDLE_ENV_JSON` accepts a JSON object whose string values are defaults and whose `null` values are explicit clears. For example, -`{"HERMES_HOME":null,"HERMES_DATA_DIR_SUFFIX":"magic-test"}`. These settings are -not applied to the build runner itself. +`{"HERMES_HOME":null,"HERMES_DATA_DIR_SUFFIX":"magic-test"}`. Only +`HERMES_HOME`, `HERMES_DATA_DIR_SUFFIX`, `HERMES_DESKTOP_USER_DATA_DIR`, +`HERMES_SHARED_AUTH_DIR`, `HERMES_GUEST_ONBOARDING`, and `HERMES_SKIP_INTRO` +are accepted. Process-control variables such as `NODE_OPTIONS` and `PATH` +are rejected. These settings are not applied to the build runner itself. Commit archive keys still use the SHA, so use a fresh commit for different defaults: an existing artifact is never overwritten with different bytes. diff --git a/apps/desktop/electron/updater/app-installer-strategy.test.ts b/apps/desktop/electron/updater/app-installer-strategy.test.ts index 1aad9ecfdc..080ec81b18 100644 --- a/apps/desktop/electron/updater/app-installer-strategy.test.ts +++ b/apps/desktop/electron/updater/app-installer-strategy.test.ts @@ -97,6 +97,19 @@ describe('AppInstallerStrategy.apply', () => { expect(calls).toEqual(['relaunch-marker', 'teardown', 'quit']) }) + it.each(['http://external.example/update.appinstaller', 'https://user:pass@registered.example/update.appinstaller', 'https://registered.example/../update.appinstaller'])( + 'rejects an unsafe registered source URI before staging: %s', + async (sourceUri: string): Promise => { + const { deps, calls } = makeDeps({ + feedBaseUrl: '', + run: async () => ({ code: 2, stdout: JSON.stringify({ available: true, source_uri: sourceUri }) }) + }) + + await expect(new AppInstallerStrategy(deps).apply()).rejects.toThrow() + expect(calls).toEqual([]) + } + ) + it('uses an exact descriptor without a Python check, and rejects unverified prepared packages before teardown', async (): Promise => { const { deps, calls } = makeDeps({ run: async (): Promise => { diff --git a/apps/desktop/electron/updater/app-installer.ts b/apps/desktop/electron/updater/app-installer.ts index fb5aefcf46..c95911f705 100644 --- a/apps/desktop/electron/updater/app-installer.ts +++ b/apps/desktop/electron/updater/app-installer.ts @@ -138,6 +138,10 @@ export class AppInstallerStrategy { if (!feedBaseUrl && !sourceUri) { const { code, stdout } = await this.deps.run(this.deps.python, this.deps.script) sourceUri = parseCheckOutput(code, stdout).sourceUri + + if (sourceUri) { + channelPublicBase(sourceUri) + } } if (!feedBaseUrl && !sourceUri) { diff --git a/apps/desktop/electron/updater/channel-protocol.ts b/apps/desktop/electron/updater/channel-protocol.ts index 4fa892b284..4e67c1bb62 100644 --- a/apps/desktop/electron/updater/channel-protocol.ts +++ b/apps/desktop/electron/updater/channel-protocol.ts @@ -361,8 +361,14 @@ function request(fields: Fields): ChannelRequest { const environment = fields.object('bundleEnv') const bundleEnv: ChannelBuild['bundleEnv'] = {} + // Match the build-time allowlist: a channel request cannot inject process flags. + const allowed = new Set([ + 'HERMES_HOME', 'HERMES_DATA_DIR_SUFFIX', 'HERMES_DESKTOP_USER_DATA_DIR', + 'HERMES_SHARED_AUTH_DIR', 'HERMES_GUEST_ONBOARDING', 'HERMES_SKIP_INTRO' + ]) + for (const key of environment.keys()) { - if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key)) { + if (!allowed.has(key)) { throw new Error('Invalid bundle environment name') } diff --git a/apps/desktop/electron/updater/channel.test.ts b/apps/desktop/electron/updater/channel.test.ts index 98b6dc11e8..4ddc56046e 100644 --- a/apps/desktop/electron/updater/channel.test.ts +++ b/apps/desktop/electron/updater/channel.test.ts @@ -175,6 +175,15 @@ async function retiredFixture( return { ...f, retired } } +test.each(['NODE_OPTIONS', 'PATH', 'HERMES_PYTHON'])( + 'channel manifest rejects process-control bundle key %s', + async (key: string): Promise => { + const f = await fixture() + f.manifest.request.bundleEnv[key] = null + expect((): void => { decodeChannelManifest(JSON.stringify(f.manifest)) }).toThrow('Invalid bundle environment name') + } +) + test('receiver kind mirrors the identity comparison between retired channel and destination', async (): Promise => { const f = await retiredFixture() diff --git a/apps/desktop/scripts/bundle-env.mjs b/apps/desktop/scripts/bundle-env.mjs index ca73915087..8f617eecbb 100644 --- a/apps/desktop/scripts/bundle-env.mjs +++ b/apps/desktop/scripts/bundle-env.mjs @@ -1,5 +1,10 @@ // Defaults must run before bundled modules resolve paths or onboarding flags. // An esbuild define would replace reads, not populate the child process env. +// Match scripts/releases/bundle_env.py and the channel request decoder. +const ALLOWED_KEYS = new Set([ + 'HERMES_HOME', 'HERMES_DATA_DIR_SUFFIX', 'HERMES_DESKTOP_USER_DATA_DIR', + 'HERMES_SHARED_AUTH_DIR', 'HERMES_GUEST_ONBOARDING', 'HERMES_SKIP_INTRO' +]) /** Validate a bundle environment object: plain object of identifiers to * strings (defaults) or null (clears). Shared by the banner writer and the @@ -10,8 +15,8 @@ export function validateBundleEnvironment(values) { throw new Error('Bundle environment must be a JSON object') } for (const [key, value] of Object.entries(values)) { - if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key) || (value !== null && (typeof value !== 'string' || value.includes('\0')))) { - throw new Error('Bundle environment requires valid names and string values without NUL, or null to clear') + if (!ALLOWED_KEYS.has(key) || (value !== null && (typeof value !== 'string' || value.includes('\0')))) { + throw new Error('Bundle environment requires permitted desktop keys and string values without NUL, or null to clear') } } return /** @type {Record} */ (values) diff --git a/apps/desktop/scripts/bundle-env.test.mjs b/apps/desktop/scripts/bundle-env.test.mjs index 79f37d7569..d39869f496 100644 --- a/apps/desktop/scripts/bundle-env.test.mjs +++ b/apps/desktop/scripts/bundle-env.test.mjs @@ -43,9 +43,9 @@ test('explicit clears beat inherited homes and prevent Windows registry fallback test('applyBundleEnvironment replays the banner semantics for defaults, runtime overrides and clears', async () => { const root = mkdtempSync(join(tmpdir(), 'hermes-bundle-pure-')) - const defaults = { KEEP: 'default', MISSING: 'default', CLEARED: null, OVERRIDDEN: 'default', SUFFIX: 'baked' } + const defaults = { HERMES_HOME: null, HERMES_DATA_DIR_SUFFIX: 'baked', HERMES_GUEST_ONBOARDING: '1' } const keys = Object.keys(defaults) - const base = { KEEP: 'x', CLEARED: 'runtime', OVERRIDDEN: 'runtime', SUFFIX: 'explicit' } + const base = { HERMES_HOME: 'runtime', HERMES_DATA_DIR_SUFFIX: 'explicit' } try { // The bundled banner must produce the same effective environment the pure // function computes, so the smoke driver can predict the app's home from @@ -55,11 +55,13 @@ test('applyBundleEnvironment replays the banner semantics for defaults, runtime writeFileSync(entry, `import {values} from './reader.mjs'; console.log(JSON.stringify(values));`, 'utf8') const outfile = join(root, 'bundle.mjs') await build({ entryPoints: [entry], bundle: true, platform: 'node', format: 'esm', outfile, banner: { js: environmentDefaultsBanner(JSON.stringify(defaults)) } }) - const viaBanner = JSON.parse(execFileSync(process.execPath, [outfile], { env: { ...process.env, ...base }, encoding: 'utf8' })) + const env = { ...process.env, ...base } + delete env.HERMES_GUEST_ONBOARDING + const viaBanner = JSON.parse(execFileSync(process.execPath, [outfile], { env, encoding: 'utf8' })) const viaFunction = Object.fromEntries(keys.map(key => [key, applyBundleEnvironment(base, defaults)[key]])) expect(viaFunction).toEqual(viaBanner) expect(viaFunction).toEqual({ - KEEP: 'x', MISSING: 'default', CLEARED: '', OVERRIDDEN: 'runtime', SUFFIX: 'explicit', + HERMES_HOME: '', HERMES_DATA_DIR_SUFFIX: 'explicit', HERMES_GUEST_ONBOARDING: '1', }) } finally { rmSync(root, { recursive: true, force: true }) @@ -68,7 +70,7 @@ test('applyBundleEnvironment replays the banner semantics for defaults, runtime test('baked defaults precede imported module initialization and reach children without overriding explicit env', async () => { const root = mkdtempSync(join(tmpdir(), 'hermes-bundle-env-')) - const defaults = { HERMES_GUEST_ONBOARDING: '1', HERMES_DATA_DIR_SUFFIX: 'magic-test', LITERAL: 'a=b "q"\n$(no)', EMPTY: '' } + const defaults = { HERMES_GUEST_ONBOARDING: '1', HERMES_DATA_DIR_SUFFIX: 'magic-test', HERMES_SHARED_AUTH_DIR: 'a=b "q"\n$(no)', HERMES_SKIP_INTRO: '' } const env = { ...process.env } for (const key of Object.keys(defaults)) { delete env[key] @@ -82,7 +84,8 @@ test('baked defaults precede imported module initialization and reach children w const run = extra => JSON.parse(execFileSync(process.execPath, [outfile], { env: { ...env, ...extra }, encoding: 'utf8' })) expect(run({})).toEqual({ values: defaults, child: defaults.HERMES_DATA_DIR_SUFFIX }) expect(run({ HERMES_DATA_DIR_SUFFIX: '-explicit', HERMES_GUEST_ONBOARDING: '' })).toEqual({ values: { ...defaults, HERMES_DATA_DIR_SUFFIX: '-explicit', HERMES_GUEST_ONBOARDING: '' }, child: '-explicit' }) - for (const bad of ['[]', 'null', '{"BAD-NAME":"x"}', '{"NAME":1}', '{"NAME":"\\u0000"}']) { + for (const bad of ['[]', 'null', '{"BAD-NAME":"x"}', '{"HERMES_HOME":1}', '{"HERMES_HOME":"\\u0000"}', + '{"NODE_OPTIONS":"--require=evil"}', '{"PATH":null}', '{"HERMES_PYTHON":"/untrusted/python"}']) { expect(() => environmentDefaultsBanner(bad)).toThrow() } } finally { diff --git a/scripts/releases/bundle_env.py b/scripts/releases/bundle_env.py index 61cfe95c43..e4805afeb3 100644 --- a/scripts/releases/bundle_env.py +++ b/scripts/releases/bundle_env.py @@ -3,15 +3,20 @@ from __future__ import annotations from collections.abc import Sequence import json -import re + +# Keep this list aligned with the Desktop bundle banner and channel decoder. +_ALLOWED = frozenset({ + "HERMES_HOME", "HERMES_DATA_DIR_SUFFIX", "HERMES_DESKTOP_USER_DATA_DIR", + "HERMES_SHARED_AUTH_DIR", "HERMES_GUEST_ONBOARDING", "HERMES_SKIP_INTRO", +}) def validate(values: object) -> dict[str, str | None]: if not isinstance(values, dict): raise ValueError("Bundle environment must be a JSON object") for key, value in values.items(): - if not isinstance(key, str) or not re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", key): - raise ValueError("Bundle environment names must be valid environment identifiers") + if not isinstance(key, str) or key not in _ALLOWED: + raise ValueError(f"Bundle environment name is not permitted: {key}") if value is not None and (not isinstance(value, str) or "\0" in value): raise ValueError(f"Bundle environment value for {key} must be a string without NUL or null") return values diff --git a/tests/ci/test_channel_build_publication.py b/tests/ci/test_channel_build_publication.py index e10d76664f..a5c5289c5e 100644 --- a/tests/ci/test_channel_build_publication.py +++ b/tests/ci/test_channel_build_publication.py @@ -28,7 +28,7 @@ def request_for(base): return {"schema": 1, "buildId": "a" * 32, "channel": "unknown-at-build-time", "sequence": 7, "repository": "fixture/repo", "commit": "b" * 40, "sourceVersion": "1.2.3", "version": "0.0.7", "windowsVersion": "0.0.7.0", "identity": preview_identity("unknown-at-build-time", "c" * 16), - "bundleEnv": {"FEATURE": "different from the prior request"}, "publicBase": base} + "bundleEnv": {"HERMES_GUEST_ONBOARDING": "different from the prior request"}, "publicBase": base} @pytest.fixture @@ -110,7 +110,7 @@ def test_channel_handoff_binds_full_request_and_feed_bytes(tmp_path, r2_server, filename = file["url"].rsplit("/", 1)[-1] assert file["size"] == (build / filename).stat().st_size changed = copy.deepcopy(request) - changed["bundleEnv"]["FEATURE"] = "other packaging input" + changed["bundleEnv"]["HERMES_GUEST_ONBOARDING"] = "other packaging input" with pytest.raises(ValueError, match="identity"): handoff.fetch_channel_build(changed, ["win32-x64"], tmp_path / "wrong", public_base=request["publicBase"]) assert not (tmp_path / "wrong").exists() diff --git a/tests/ci/test_commit_build_staging.py b/tests/ci/test_commit_build_staging.py index c7ffe125a4..2862a86c71 100644 --- a/tests/ci/test_commit_build_staging.py +++ b/tests/ci/test_commit_build_staging.py @@ -66,7 +66,8 @@ def test_failed_commit_summary_publishes_downloads_or_run_links(tmp_path, r2_ser base = f'http://127.0.0.1:{r2_server.server_port}/hermes-releases' summary = tmp_path / 'summary.md' jobs = _workflow()['jobs'] - bundle_env = {'HERMES_HOME': None, 'EMPTY': '', 'LABEL': ''} + bundle_env = {'HERMES_HOME': None, 'HERMES_SKIP_INTRO': '', + 'HERMES_SHARED_AUTH_DIR': ''} env = dict(HERMES_BUILD_COMMIT=sha, HERMES_PAYLOAD_TAG='', RELEASE_COMMIT=sha, GITHUB_REPOSITORY='fixture-owner/fixture-repo', HERMES_BUNDLE_ENV_JSON=json.dumps(bundle_env), CI_SECRET='must-not-appear', @@ -91,8 +92,8 @@ def test_failed_commit_summary_publishes_downloads_or_run_links(tmp_path, r2_ser page = response.read().decode() assert f'href="https://github.com/fixture-owner/fixture-repo/commit/{sha}"' in page assert 'Bundle environment' in page and 'HERMES_HOME' in page and 'Unset' in page - assert 'EMPTY""' in page - assert html.escape(json.dumps(bundle_env['LABEL'], ensure_ascii=False)) in page + assert 'HERMES_SKIP_INTRO""' in page + assert html.escape(json.dumps(bundle_env['HERMES_SHARED_AUTH_DIR'], ensure_ascii=False)) in page assert '