diff --git a/.github/scripts/__tests__/fallback-chain.test.js b/.github/scripts/__tests__/fallback-chain.test.js index 49d0879..f301f35 100644 --- a/.github/scripts/__tests__/fallback-chain.test.js +++ b/.github/scripts/__tests__/fallback-chain.test.js @@ -31,7 +31,6 @@ const execFile = jest.fn((file, args, ...rest) => { : file; return childProcess.execFile(command, args, ...rest); }); -const spawn = jest.fn((...args) => childProcess.spawn(...args)); const execFileSync = jest.fn((file, args, ...rest) => { if (file === 'aapt') { return fs.readFileSync( @@ -70,7 +69,7 @@ function installFixtureTools() { } function fixtureUrl(fileName) { - return `file://${path.join(fixtureRoot, 'apk-metadata', fileName)}`; + return `https://example.invalid/fixtures/${fileName}`; } function fixtureApiResponse() { @@ -352,24 +351,40 @@ describe('download() fallback chain', () => { ); // verifyUrl is HEAD-based; make it return true (url is "valid"). + // verifyUrl now requires protocol=https: (CodeQL file-access-to-http + // sanitisation — see comment in verifyUrl's body). The URL above is + // already https so this stub still approves it; only file:// would + // be rejected now. global.fetch = jest.fn(() => Promise.resolve({ ok: true, status: 200 })); - // downloadWithUrl uses a real curl subprocess. The local forwarding - // shim keeps the original spawn call-count assertion while writing - // the sanitized APK placeholder. + // downloadWithUrl shells out to curl with the cached URL. The + // contract under test is "cache hit short-circuits the rest", not + // curl semantics — replace the spawnImpl with a shim that copies + // the placeholder to the expected target so the test doesn't + // depend on real network egress or curl's file:// support. const cachedFixture = path.join(fixtureRoot, 'apk-metadata', 'placeholder.apk'); const target = path.join(apksDir, `${PKG}_${VER}.apk`); const resultOfCopy = childProcess.spawnSync('cp', [cachedFixture, target], { encoding: 'utf8' }); expect(resultOfCopy.status).toBe(0); + const cachedSpawn = jest.fn((cmd, args, opts) => { + // Mimic curl -o : copy the fixture into place, then fire + // the close event with exit code 0 so downloadWithUrl proceeds. + fs.copyFileSync(cachedFixture, target); + const child = childProcess.spawn('true', [], { ...opts }); + // childProcess.spawn's return is a ChildProcess; we don't need its + // stdout/stderr here because downloadWithUrl only watches 'close'. + return child; + }); + const result = await download(PKG, VER, apksDir, { - spawnImpl: spawn, + spawnImpl: cachedSpawn, execFileSyncImpl: execFileSync, }); expect(result.success).toBe(true); // A real spawn was called exactly once (cache-hit download), not for // any other path. - expect(spawn).toHaveBeenCalledTimes(1); + expect(cachedSpawn).toHaveBeenCalledTimes(1); // apkeep / apkmirror-api / parallel resolve must NOT have been tried. expect(execFile).not.toHaveBeenCalled(); // fetch was used for verifyUrl HEAD only (one call); the parallel diff --git a/.github/scripts/__tests__/unified-downloader-timers.test.js b/.github/scripts/__tests__/unified-downloader-timers.test.js index 2af1a9d..e996add 100644 --- a/.github/scripts/__tests__/unified-downloader-timers.test.js +++ b/.github/scripts/__tests__/unified-downloader-timers.test.js @@ -76,6 +76,54 @@ describe('unified-downloader timer hygiene', () => { }); }); + describe('verifyUrl URL validation (CodeQL file-access-to-http fix)', () => { + // verifyUrl now sanitises the URL through `new URL()` and requires + // protocol === 'https:' before issuing the HEAD probe. This breaks + // the file → fetch taint flow that CodeQL flagged as alert #37. + // The four cases pin the contract: only https URLs reach fetch(), + // every other shape is rejected up-front (no HEAD request, no + // orphan timer — clearTimeout runs in the implicit finally). + // + // The outer describe's afterEach resets jest timers and globalThis.fetch + // between tests, so we don't redeclare it here. + + test('rejects non-https schemes (http://) without calling fetch', async () => { + globalThis.fetch = jest.fn(); + const result = await verifyUrl('http://example.com/foo.apk'); + expect(result).toBe(false); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + + test('rejects non-https schemes (file://) without calling fetch', async () => { + globalThis.fetch = jest.fn(); + const result = await verifyUrl('file:///etc/passwd'); + expect(result).toBe(false); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + + test('rejects malformed URLs without calling fetch', async () => { + globalThis.fetch = jest.fn(); + // Missing scheme + unparseable by WHATWG. + const result = await verifyUrl('not a url with spaces'); + expect(result).toBe(false); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + + test('accepts https URLs and passes the re-stringified form to fetch', async () => { + // Verify the URL that reaches fetch is parsed-and-reserialised + // (the canonicalised form), not the raw input. This is the + // CodeQL-recognised sanitisation: new URL() → toString() → fetch. + globalThis.fetch = jest.fn(async () => ({ ok: true, status: 200 })); + await verifyUrl('https://example.com/foo.apk'); + expect(globalThis.fetch).toHaveBeenCalledTimes(1); + const calledUrl = globalThis.fetch.mock.calls[0][0]; + // WHATWG normalises trivial cases; the key invariant is that + // the URL passed to fetch starts with https:// and contains + // the original host. + expect(calledUrl.startsWith('https://example.com/')).toBe(true); + }); + }); + describe('parallelResolveSources', () => { test('clears the per-source timer when a fast source wins the race', async () => { // The pre-fix bug: a fast apkeep resolution (a few ms) would diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 8a5dc6a..f6c3c9f 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -267,20 +267,43 @@ async function verifyUrl(url) { throw new Error('URL is required'); } - // codeql[js/file-access-to-http] reason: `url` is a HEAD-probe for a - // cached APK download URL. The cache file itself is written only by - // this module from Morphe's published morphe-patches releases (or - // direct URL from patches.json that the user authored). The blast - // radius of a malicious URL is bounded to a HEAD request plus an - // APK download into an already-trusted temp dir. + // Sanitize the URL string before it leaves the local trust boundary. + // + // verifyUrl is called with a URL string that originates from a + // file read (cache.js reads ~/.cache/auto-morphe-builder/urls/*.json, + // config.js reads config.json download_urls). CodeQL flags the + // file → fetch edge as `js/file-access-to-http` even though the + // cache file is only ever written by this module from a successful + // resolver round-trip — the taint flow is real (file data reaches + // a network sink) and defense-in-depth wants a sanitiser here + // anyway. + // + // The fix is to round-trip the URL through the WHATWG URL parser + // (new URL()) and require protocol=https:, then pass + // `parsed.toString()` (not the raw input) into fetch. new URL() + // is the recognised CodeQL sanitiser for the + // js/file-access-to-http query — the flow becomes + // "string → parsed URL → re-stringified URL → fetch" instead of + // "string → fetch", and the protocol gate blocks any non-https + // scheme that might have landed there via a tampered cache file + // or a misauthored config.json entry. + let safeUrl; + try { + const parsed = new URL(url); + if (parsed.protocol !== 'https:') { + console.error(`[url-cache] URL verify rejected: non-https scheme "${parsed.protocol}"`); + return false; + } + safeUrl = parsed.toString(); + } catch (e) { + console.error(`[url-cache] URL verify rejected: unparseable URL (${e.message})`); + return false; + } + const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), TIMEOUTS.urlVerify); try { - // codeql[js/file-access-to-http] reason: `url` is a HEAD-probe for - // a cached APK download URL from Morphe's patches-list.json or the - // user's own patches.json. Blast radius is bounded to a HEAD - // request plus an APK download into a user-owned temp dir. - const response = await fetch(url, { + const response = await fetch(safeUrl, { method: 'HEAD', signal: controller.signal, redirect: 'follow' diff --git a/AGENTS.md b/AGENTS.md index b089fa0..672c170 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,6 +93,7 @@ Shell scripts use `shellcheck .github/scripts/pipeline/*.sh .github/scripts/pipe See `docs/checksums.md` for the full contract. - Per-repo `*.mpp` patches archives ARE SHA-256-verified in `fetch_morphe_tools.sh` against the per-asset digest served by the GitHub release API (`gh_asset_sha256`). A stale cache entry (e.g. one left over from before fd537df removed `restore-keys: morphe-patches--`) fails the SHA check, gets re-downloaded, and the next `actions/cache` save replaces it with the correct bytes — no manual `gh cache delete` required. See `docs/troubleshooting.md` → "Build job silently skips patches" for the symptom and recovery. - `apkeep` installer verifies SHA-256; `aapt` and `playwright` installers do not (PR 6). The previous `install_bouncycastle.sh` (BouncyCastle for keystore conversion) was deleted when morphe-desktop's `patch --keystore` flags made the conversion redundant. +- `verifyUrl()` in `.github/scripts/unified-downloader.js` (the cache-URL HEAD probe) sanitises the URL through `new URL()` and requires `protocol === 'https:'` before issuing the HEAD request. The URL that reaches `fetch()` is the WHATWG-canonicalised form, not the raw input. The cache file itself is written only by `saveCachedUrl()` after a successful resolver round-trip (or by `update-download-urls.js` from the user's own `patches.json`), so the URL string flowing into `fetch()` is operationally trusted — but the file → fetch edge is real taint and the protocol gate breaks the flow that CodeQL's `js/file-access-to-http` query tracks (alert #37). Tests in `__tests__/unified-downloader-timers.test.js` cover the four cases: http://, file://, malformed, and the canonical https: accept path. - Signing has **no** `--unsigned` fallback. morphe-desktop's `patch` always signs when `--keystore` is passed; a bad password or invalid keystore aborts the workflow loudly rather than producing an unsigned APK. The previous `build` ↔ `sign` trust boundary was dissolved (see `docs/architecture.md` → "Signing model" for the implications). - morphe-desktop v1.14.0 hardcodes `--keystore-entry-password` and `--keystore-entry-alias` defaults to `"Morphe"` (legacy bundled-keystore alias, see `MorpheApp/morphe-desktop` `PatchCommand.kt:215`/`221` + `PatchEngine.kt:64`/`65`). They do NOT inherit from `--keystore-password` and do NOT auto-pick the keystore's first alias — omitting either flag silently tries the literal password `"Morphe"` and fails with `BadPaddingException`. `patch_apk.sh` always passes both explicitly (alias detected via `keytool -list` when `KEY_ALIAS` is unset; entry password defaults to `KEYSTORE_PASSWORD` when `KEY_PASSWORD` is unset). If morphe-desktop later inherits from `--keystore-password` or auto-picks the first alias, the script can be simplified. - The downloader saves XAPK/APKM/APKS bundles with a `.apk` extension. `detectApkShape` in `apk-selection.js` inspects zip contents to recognise bundles — required because APKMirror often serves bundles without preserving the extension. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 3982e80..c0b7fbf 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -233,6 +233,26 @@ If you see this symptom on a deployment that hasn't picked up this version of `f --- +## `verifyUrl` rejects the cached URL + +**Symptom:** the build skips a known-good cached APK with a log line like `[url-cache] URL verify rejected: non-https scheme "http:"` or `[url-cache] URL verify rejected: unparseable URL (...)`, and the resolver falls through to the next source. + +**Cause:** `verifyUrl()` (in `.github/scripts/unified-downloader.js`) now sanitises the URL through `new URL()` and requires `protocol === 'https:'` before issuing the HEAD probe. Any cached URL written before this change — or written by a manual edit to `config.json` `download_urls` — is rejected if it is not a parseable `https://` URL. + +**Fix:** + +1. Delete the stale cache entry so the resolver rebuilds it on the next run: + + ```bash + rm -f ~/.cache/auto-morphe-builder/urls/_.json + ``` + +2. If `config.json` `download_urls` is the source of the bad URL, edit it to use `https://` (the resolver only writes `https://` URLs on its own, so a non-https here was almost certainly hand-edited). + +The HEAD probe will not run on any non-https URL — even a tampered cache file can't redirect the downloader to `http://internal-server/...`. CodeQL alert #37 (`js/file-access-to-http` on `unified-downloader.js`) is the audit trail for this contract. + +--- + ## Reporting a new failure If you hit a failure not listed here: diff --git a/package-lock.json b/package-lock.json index 5deefbc..e1d3cf5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5470,9 +5470,9 @@ } }, "node_modules/undici": { - "version": "7.29.0", - "resolved": "https://registry.npmjs.org/undici/-/undici-7.29.0.tgz", - "integrity": "sha512-IDxfleLmmbSskfWSUATiN1nfn2rDuvnMOqb5CWR92iIfojA0Ud+ulOAAEQ57LPr9rWmsreUyf5lwyao+7GNNVw==", + "version": "7.30.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-7.30.0.tgz", + "integrity": "sha512-dkrQXeHSaoamnItlYbmzG0wFYrM0ZwDxCIg0A7aKjTyyhh9svRzCNFEzV+Vm05/yehjCzjDZ31KXfGEjYSztDQ==", "license": "MIT", "engines": { "node": ">=20.18.1" diff --git a/package.json b/package.json index e978c4f..49104e0 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "overrides": { "brace-expansion": "^5.0.8", "minimatch": "^10.2.6", - "js-yaml": "^5.2.3" + "js-yaml": "^5.2.3", + "undici": "^7.29.1" } }