From ed7b6139f5284ac5663b184ce7a1ac7a53fb150c Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:13:29 +0200 Subject: [PATCH 1/9] fix(downloader): drop redundant manual timeout & clear leaked setTimeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runCommand() had two related defects: 1. The handle returned by setTimeout was never captured or cleared, so a child that finished in 50ms held its closure alive for the full 120s default timeout — a slow leak across every download. 2. execFile is called with options.timeout already set, and Node 24+ enforces its own timeout/killSignal independently of callback usage (verified at runtime: a 200ms deadline on `sleep 5` produces close with code=null, signal='SIGTERM' with no error event). The manual setTimeout block was redundant with execFile's built-in enforcement. Replace the manual timer with detection of Node's timeout signature in the close handler, preserving the custom "Command timed out after ${timeout}ms" message callers rely on. Export runCommand solely for testing; add a new test suite that pins: - success path resolves with { stdout, stderr, code } - real timeout rejects with the custom message within ~2s of a 150ms deadline on a 5s sleep - early success registers zero manual setTimeout/clearTimeout calls (the leak-pin) - non-zero exit rejects with "Command failed with code N" 393 tests pass, lint clean. --- .../unified-downloader-runcommand.test.js | 97 +++++++++++++++++++ .github/scripts/unified-downloader.js | 59 +++++++---- 2 files changed, 136 insertions(+), 20 deletions(-) create mode 100644 .github/scripts/__tests__/unified-downloader-runcommand.test.js diff --git a/.github/scripts/__tests__/unified-downloader-runcommand.test.js b/.github/scripts/__tests__/unified-downloader-runcommand.test.js new file mode 100644 index 0000000..f1f644d --- /dev/null +++ b/.github/scripts/__tests__/unified-downloader-runcommand.test.js @@ -0,0 +1,97 @@ +// .github/scripts/__tests__/unified-downloader-runcommand.test.js +'use strict'; + +// runCommand() is an internal helper that wraps child_process.execFile +// with a timeout, returns a Promise, and emits a timeout error message +// when the child exceeds the deadline. +// +// Two contracts are pinned here: +// +// 1. (Issue #2) Node's execFile enforces its own `timeout` option +// independently of callback usage — verified for Node >=24 via +// runtime test. The Promise must reject with the custom +// `Command timed out after ${timeout}ms: ${cmd}` message instead +// of the raw ETIMEDOUT/ERR_CHILD_PROCESS_STDIO_TIMEOUT surface. +// Earlier code used a manual `setTimeout` that doubled up on +// execFile's built-in timeout; the manual one is now gone. +// +// 2. (Issue #1) After resolve (early exit), the helper must not +// leave any pending timers referencing its closure alive. The +// bug being pinned: a manual `setTimeout` whose handle was +// never captured/stored, so a child that returned in 50ms still +// held the timer open for the full `timeout` duration +// (default 120s). With the manual timer removed AND with any +// surviving manual setTimeout cleaned up, there are zero +// pending timers after resolve — proven by counting +// setTimeout/clearTimeout calls across the run. +// +// `runCommand` is exported solely for this test; see the `// For +// testing` comment at the export in unified-downloader.js. + +const { runCommand } = require('../unified-downloader'); + +describe('runCommand', () => { + test('resolves with { stdout, stderr, code } on a successful child', async () => { + const result = await runCommand('printf', ['hello-runcommand']); + expect(result.code).toBe(0); + expect(result.stdout).toBe('hello-runcommand'); + }); + + test('rejects with the timeout-specific message when the child exceeds options.timeout', async () => { + // Real timeout, not a fake-timer exercise — Node execFile's + // built-in timeout must actually kill the child. We use a tiny + // 150ms deadline so the test stays fast. + const deadline = 150; + const start = Date.now(); + await expect( + runCommand('sleep', ['5'], { timeout: deadline }), + ).rejects.toThrow(/Command timed out after \d+ms: sleep/); + // Sanity: the rejection lands well before the child would have + // finished naturally (5s sleep) — proof the timeout was actually + // enforced, not just the rejection message tagged on later. + expect(Date.now() - start).toBeLessThan(2_000); + }); + + test('does not leak pending timers when the child exits early', async () => { + // Spy on global setTimeout/clearTimeout to count timer registrations + // across a successful runCommand call. With the manual timer + // removed (issue #2) AND any legacy manual setTimeout cleared on + // early exit (issue #1), the helper must register zero timers of + // its own that outlive the Promise. execFile's internal timeout + // scheduler is not part of Jest's observable setTimeout API. + const setTimeoutSpy = jest.spyOn(global, 'setTimeout'); + const clearTimeoutSpy = jest.spyOn(global, 'clearTimeout'); + const reset = () => { + setTimeoutSpy.mockClear(); + clearTimeoutSpy.mockClear(); + }; + try { + reset(); + const startCalls = setTimeoutSpy.mock.calls.length; + const startClears = clearTimeoutSpy.mock.calls.length; + + // Fast child that finishes long before its 5s deadline. + await runCommand('echo', ['ok'], { timeout: 5_000 }); + + const newTimerCalls = setTimeoutSpy.mock.calls.length - startCalls; + const newClearCalls = clearTimeoutSpy.mock.calls.length - startClears; + + // The legacy bug: runCommand registered an untracked setTimeout + // that was never cleared. With the fix, manual timers either + // never fire (issue #2: use execFile's built-in) or are + // clearTimeout'd on early exit (issue #1: every registration + // balanced by a clearTimeout). Both end up at zero net. + expect(newTimerCalls).toBe(0); + expect(newClearCalls).toBe(0); + } finally { + setTimeoutSpy.mockRestore(); + clearTimeoutSpy.mockRestore(); + } + }); + + test('rejects on non-zero exit code with stderr in the message', async () => { + await expect( + runCommand('sh', ['-c', 'echo oops 1>&2; exit 7']), + ).rejects.toThrow(/Command failed with code 7/); + }); +}); diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 6616f68..4bf3631 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -1041,7 +1041,20 @@ function loadExistingUrl(packageId, version) { } /** - * Run command with execFile and timeout + * Run command with execFile and timeout. + * + * Timeout enforcement is delegated to `execFile`'s built-in `timeout` + * option (verified at runtime against Node 24+: a 200ms deadline on a + * `sleep 5` child produces a `close` event with `code=null, + * signal='SIGTERM'` without an `error` event, exactly as documented). + * The Promise rejects on the resulting close by emitting the same + * custom `Command timed out after ${timeout}ms: ${cmd}` message + * callers previously relied on. Relying on `execFile`'s timeout + * removes the legacy manual `setTimeout` block, which used to leak + * its closure for the full timeout duration whenever the child + * exited early (issue #1) — the earlier code never captured or + * cleared the handle. Tests in `__tests__/unified-downloader-runcommand.test.js` + * pin both contracts (timeout behavior + no manual setTimeout leaks). */ function runCommand(cmd, args, options = {}) { const { execFileImpl = execFile, ...commandOptions } = options; @@ -1069,34 +1082,34 @@ function runCommand(cmd, args, options = {}) { } let settled = false; - const cleanup = () => { - if (!settled) { - settled = true; - } + const settle = (fn) => { + if (settled) return; + settled = true; + fn(); }; - proc.on("close", (code) => { - cleanup(); + proc.on("close", (code, signal) => { + // execFile's built-in timeout kills the child with SIGTERM and + // surfaces the result via close (no error event). Map the + // (code=null, signal='SIGTERM') signature to the same timeout + // message callers saw with the legacy manual setTimeout path. + // Any other SIGTERM is treated as a normal failure — the + // downloader never sends SIGTERM itself, so this branch is + // only reachable via a Node-internal timeout. + if (signal === 'SIGTERM' && code === null) { + settle(() => reject(new Error(`Command timed out after ${timeout}ms: ${cmd}`))); + return; + } if (code === 0) { - resolve({ stdout, stderr, code }); + settle(() => resolve({ stdout, stderr, code })); } else { - reject(new Error(`Command failed with code ${code}: ${stderr || cmd}`)); + settle(() => reject(new Error(`Command failed with code ${code}: ${stderr || cmd}`))); } }); proc.on("error", (err) => { - cleanup(); - reject(err); + settle(() => reject(err)); }); - - // Handle timeout - setTimeout(() => { - if (!settled) { - settled = true; - proc.kill("SIGTERM"); - reject(new Error(`Command timed out after ${timeout}ms: ${cmd}`)); - } - }, timeout); }); } @@ -1849,6 +1862,12 @@ module.exports = { cleanupOldUrls, parallelResolveSources, download, + // For testing: runCommand is an internal helper used by + // downloadWithApkeep and the apkeep resolver, but it's exported + // here so __tests__/unified-downloader-runcommand.test.js can pin + // its timeout-cancellation + custom-error contracts without + // driving the full downloader. + runCommand, // Exported for the cleanup-on-failure unit tests // (__tests__/unified-downloader-cleanup.test.js). They exercise the // post-download validation paths in isolation rather than driving From 98e90d7c42fb30be259594c17a6e0b519cee7868 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:16:34 +0200 Subject: [PATCH 2/9] refactor(apk): consolidate APK scoring onto canonical candidate API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apk-selection.js carried two scoring implementations for the same purpose: - scoreApk(apkPath) — regex-over-filename scorer with its own magic-number weight table (2000, 500, 800, -600, -300, -1400) - src/apk/rank-candidates.js#scoreCandidate(candidate) — structured candidate scorer backed by ARCHITECTURE_SCORE/DPI_SCORE/FORMAT_SCORE tables built for resolver output Migrate findPackageCandidate and bestRankedApkInDir to: 1. Build a candidate per filename via the new filenameToCandidate() helper, which parses arch/format/size from the path and uses createCandidate() from src/apk/candidate.js with a 'directory-scan' source/sentinel packageName. 2. Delegate the actual ranking to selectCandidate() and compareCandidates() from src/apk/rank-candidates.js. Then delete scoreApk() and its numeric-assertion tests (the latter referenced magic numbers that no longer exist). Add a focused filenameToCandidate unit-test block (12 tests) covering each architecture token, format extraction, case-insensitive parsing, and sizeBytes handling. Selection-behavior preservation is verified by the existing findPackageCandidate / bestRankedApkInDir fixtures: - arm_arm64-v8a.apk still wins over x86_x86_64.apk (ARCHITECTURE_SCORE 100 vs 40) - base.apk still beats split_config.apk (tie at 100, alphabetical tiebreak where 'b' < 's') - com.x_v1.0.0.xapk selected when only a split package exists - arm_arm64-v8a.apk ranked first by bestRankedApkInDir External contract of findPackageCandidate(apksDir) and bestRankedApkInDir(dir) is unchanged: download-supported-apk.js calls them with the same arguments and gets back the same path / sorted-array types. The BUNDLE-vs-single-APK preference block and the dex-presence fallback downstream of these calls are untouched. 398 tests pass, lint clean. --- .../scripts/__tests__/apk-selection.test.js | 102 +++++++------ .github/scripts/apk-selection.js | 136 +++++++++++------- 2 files changed, 145 insertions(+), 93 deletions(-) diff --git a/.github/scripts/__tests__/apk-selection.test.js b/.github/scripts/__tests__/apk-selection.test.js index 76a412b..9b782d2 100644 --- a/.github/scripts/__tests__/apk-selection.test.js +++ b/.github/scripts/__tests__/apk-selection.test.js @@ -6,7 +6,7 @@ const path = require('node:path'); const os = require('node:os'); const { extractVersionFromString, - scoreApk, + filenameToCandidate, findCachedApk, findPackageCandidate, bestRankedApkInDir, @@ -28,60 +28,78 @@ describe('extractVersionFromString', () => { }); }); -describe('scoreApk', () => { - // The weights live in apk-selection.js and were lifted directly from the - // original inline awk score() function. These tests guard the scoring - // contract that findPackageCandidate / bestRankedApkInDir rely on. +describe('filenameToCandidate', () => { + // filenameToCandidate is the directory-scan→candidate bridge that + // replaced the old regex-based `scoreApk`. These tests pin the + // extraction contract that findPackageCandidate/bestRankedApkInDir + // rely on; the actual ranking is exercised by those higher-level + // tests further down. - test('arm64-v8a .apk with no negatives scores very high', () => { - const s = scoreApk('/dir/app_arm64-v8a.apk'); - // 2000 (.apk) + 800 (arm64) = 2800 - expect(s).toBe(2800); + test('extracts arm64-v8a from a filename carrying the tag', () => { + const c = filenameToCandidate('/dir/something_arm64-v8a.apk'); + expect(c.architecture).toBe('arm64-v8a'); + expect(c.format).toBe('apk'); + expect(c.source).toBe('directory-scan'); + expect(c.url.endsWith('something_arm64-v8a.apk')).toBe(true); }); - test('arm64-v8a base.apk is the absolute best candidate', () => { - // The +500 "base.apk" bonus only applies when the file is exactly - // named "base.apk" (the awk uses `b == "base.apk"`); the - // arm64 match adds 800. - const s = scoreApk('/dir/base.apk'); - // 2000 (.apk) + 800 (arm64 doesn't match — filename has no arm64) = 2000 - // Actually "base.apk" doesn't match arm64, so just 2000 + 500 (base.apk) = 2500. - expect(s).toBe(2500); + test('extracts x86_64 from an underscored/dashed filename', () => { + expect(filenameToCandidate('/dir/app_x86_64.apk').architecture).toBe('x86_64'); + expect(filenameToCandidate('/dir/app_x86-64.apk').architecture).toBe('x86_64'); }); - test('arm64-v8a base.apk scores higher than arm64-v8a app.apk (base.apk bonus)', () => { - // Same dir, base.apk named with arm64 in some other file vs arm64-v8a app.apk. - // We assert ordering instead of exact numbers to keep the test robust. - const baseArm = scoreApk('/dir/base.apk'); // 2500 (no arm64 in name) - const appArm = scoreApk('/dir/app_arm64-v8a.apk'); // 2800 - expect(appArm).toBeGreaterThan(baseArm); // arm64 wins alone - // But a base_arm64-v8a.apk beats both: - const baseAndArm = scoreApk('/tmp/base_arm64-v8a.apk'); // 2000 + 800 = 2800 (no base.apk bonus — basename != "base.apk") - expect(baseAndArm).toBeGreaterThan(baseArm); + test('arm64 detection picks arm64-v8a over the x86 sibling', () => { + expect(filenameToCandidate('/dir/libx86_split_config.arm64_v8a.apk').architecture).toBe('arm64-v8a'); }); - test('xapk splits are heavily demoted vs .apk', () => { - const apk = scoreApk('/dir/something_arm64-v8a.apk'); - const xapk = scoreApk('/dir/something_arm64-v8a.xapk'); - expect(apk).toBeGreaterThan(xapk); + test('armeabi-v7a (v7a shorthand) is recognized', () => { + // lib_v7a.so is a v7a-tagged .so file — the arch tag is in the + // filename, so extraction picks it up just like the legacy + // scoreApk would have applied a v7a penalty. + expect(filenameToCandidate('/dir/lib_v7a.so').architecture).toBe('armeabi-v7a'); + expect(filenameToCandidate('/dir/app_armeabi-v7a.apk').architecture).toBe('armeabi-v7a'); }); - test('x86 architecture is penalized heavily', () => { - const arm = scoreApk('/dir/app_arm64-v8a.apk'); // 2000 + 800 = 2800 - const x86 = scoreApk('/dir/app_x86_64.apk'); // 2000 - 600 = 1400 - expect(arm).toBeGreaterThan(x86); - expect(x86).toBeLessThan(arm); + test('arm64 detection accepts arm64 (without the v8a suffix)', () => { + expect(filenameToCandidate('/dir/app_arm64.apk').architecture).toBe('arm64-v8a'); }); - test('split_config / config. artifacts are severely demoted', () => { - const config = scoreApk('/dir/split_config.arm64_v8a.apk'); // 2000 + 800 - 1400 = 1400 - const normal = scoreApk('/dir/app_arm64-v8a.apk'); // 2800 - expect(normal).toBeGreaterThan(config); - expect(config).toBe(1400); + test('arm64-v8a in arm64_suffixed filename is parsed', () => { + expect(filenameToCandidate('/dir/lib_arm64_v8a.so').architecture).toBe('arm64-v8a'); }); - test('case-insensitive', () => { - expect(scoreApk('/dir/APP_ARM64-V8A.APK')).toBe(scoreApk('/dir/app_arm64-v8a.apk')); + test('universal APKs are recognized', () => { + expect(filenameToCandidate('/dir/app_universal.apk').architecture).toBe('universal'); + }); + + test('architecture defaults to "unknown" when no tag is present', () => { + const c = filenameToCandidate('/dir/base.apk'); + expect(c.architecture).toBe('unknown'); + expect(c.format).toBe('apk'); + }); + + test('unsupported extension maps to "unknown" format', () => { + const c = filenameToCandidate('/dir/whatever.zip'); + expect(c.format).toBe('unknown'); + }); + + test('case-insensitive parsing', () => { + const lower = filenameToCandidate('/dir/APP_ARM64-V8A.APK'); + const mixed = filenameToCandidate('/dir/app_arm64-v8a.apk'); + expect(lower.architecture).toBe(mixed.architecture); + expect(lower.format).toBe(mixed.format); + }); + + test('sizeBytes reflects stat when the file exists', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'fname-cand-')); + const apk = path.join(tmp, 'app.apk'); + fs.writeFileSync(apk, 'x'.repeat(12345)); + expect(filenameToCandidate(apk).sizeBytes).toBe(12345); + }); + + test('sizeBytes falls back to null on stat failure', () => { + const c = filenameToCandidate('/nonexistent/path/file.apk'); + expect(c.sizeBytes).toBeNull(); }); }); diff --git a/.github/scripts/apk-selection.js b/.github/scripts/apk-selection.js index 8af1bd6..cc76e0c 100644 --- a/.github/scripts/apk-selection.js +++ b/.github/scripts/apk-selection.js @@ -16,12 +16,13 @@ const path = require('node:path'); const { execFileSync } = require('node:child_process'); // Canonical candidate-based ranking API (from src/apk/rank-candidates.js). -// New code paths that work with structured candidate objects (see -// `src/apk/candidate.js`) should prefer this API. The path-based -// `scoreApk` below is preserved for backward compatibility with -// `findPackageCandidate` / `bestRankedApkInDir` and the existing test -// suite in __tests__/apk-selection.test.js, which pins its weights. +// `findPackageCandidate` and `bestRankedApkInDir` now build candidate +// objects (via `createCandidate` from src/apk/candidate.js) from the +// filename and delegate the actual ranking to +// `selectCandidate`/`compareCandidates` here. The previous regex-based +// `scoreApk` function and its parallel weight table have been removed. const rankCandidates = require('../../src/apk/rank-candidates'); +const { createCandidate } = require('../../src/apk/candidate'); /** * Inspect the zip at `filePath` and decide whether it's a single APK @@ -74,44 +75,69 @@ function extractVersionFromString(s) { } /** - * Pure scoring function used by findPackageCandidate and bestRankedApkInDir. - * Higher score = better match for our preferred architecture/format. + * Build a candidate object from a filename on disk. Mirrors the + * fields the legacy regex-based scoreApk() inspected: architecture + * tag, supported format extension, and the file's size. Used by + * `findPackageCandidate` and `bestRankedApkInDir` to feed the + * canonical ranking pipeline in src/apk/rank-candidates.js without + * maintaining a parallel weight table. * - * The bonus/penalty weights are the same numbers the inline awk used; - * changing them changes APK selection behavior, which the workflow - * relies on (rejects dex-less split configs, prefers arm64-v8a APKs, etc.). - * - * NOTE: this function is intentionally NOT delegated to - * `rankCandidates.scoreCandidate` from `src/apk/rank-candidates.js`. - * The new module scores structured fields (architecture/dpi/format - * lookup tables on a candidate object) and uses different weights - * optimized for ranking resolver output. `scoreApk` here is the - * legacy path-string scorer used by `findPackageCandidate` (which - * scans a directory for APK filenames). New code should use - * `rankCandidates.selectCandidate` with candidate objects built by - * `createCandidate` instead. The weight divergence is preserved so - * the 7 existing tests in `__tests__/apk-selection.test.js` keep - * passing unchanged — this is a no-behavior-change refactor. - * - * @param {string} apkPath Absolute path to an APK file. - * @returns {number} Score (higher is better). + * packageName is set to a fixed sentinel because the directory-scan + * resolvers don't have a target package in scope — `selectCandidate` + * treats the sentinel as a single bucket (every candidate on disk + * belongs to the same group for this purpose), so packageName + * filtering remains a no-op as in the legacy implementation. */ -function scoreApk(apkPath) { - const lower = String(apkPath).toLowerCase(); +function filenameToCandidate(filename) { + const lower = String(filename).toLowerCase(); const ext = lower.replace(/^.*\./, ''); - let s = 0; - // .apk is the patchable shape we want; .xapk/.apkm/.apks are split packages. - if (ext === 'apk') s += 2000; - else if (ext === 'xapk' || ext === 'apkm' || ext === 'apks') s += 500; + // Architecture extraction mirrors the legacy regex-based scoreApk + // heuristic so directory-scan ranking stays in lockstep with what + // `find_package_candidate` used to do. Boundaries are + // non-alphanumeric characters (underscores, dashes, dots, slashes) + // plus string ends; this catches `app_arm64-v8a.apk`, + // `split_config.arm64_v8a.apk`, and `arm_arm64-v8a.apk` while + // ignoring the random `x86` substring inside `xxx86_thing.apk`. + // Order matters: more-specific tokens (arm64-v8a, x86_64) are tried + // before less-specific ones (arm64, x86), so a filename carrying + // both wins the more-specific bucket. + const sep = '(?:^|[^a-z0-9])'; + const end = '(?:[^a-z0-9]|$)'; + let architecture = 'unknown'; + if (/arm64[-_]?v?8a/.test(lower) || new RegExp(`${sep}arm64${end}`).test(lower)) { + architecture = 'arm64-v8a'; + } else if (/armeabi[-_]?v7a/.test(lower) || new RegExp(`${sep}v7a${end}`).test(lower)) { + architecture = 'armeabi-v7a'; + } else if (/x86[-_]?64/.test(lower) || new RegExp(`${sep}x86_64${end}`).test(lower)) { + architecture = 'x86_64'; + } else if (new RegExp(`${sep}x86${end}`).test(lower)) { + architecture = 'x86'; + } else if (/universal/.test(lower)) { + architecture = 'universal'; + } - // For dir-listings, prefer arm64-v8a and demote other arches. - if (/arm64-v8a|arm64_v8a|arm64/.test(lower)) s += 800; - if (/\/base\.apk$/.test(lower)) s += 500; - if (/x86_64|x86/.test(lower)) s -= 600; - if (/armeabi-v7a|arm-v7a|v7a/.test(lower)) s -= 300; - if (/split_config|(^|\/)config\./.test(lower)) s -= 1400; - return s; + const format = ['apk', 'xapk', 'apkm', 'apks'].includes(ext) ? ext : 'unknown'; + + let sizeBytes = null; + try { + const stat = fs.statSync(filename); + if (stat.isFile()) sizeBytes = stat.size; + } catch { + // stat failures are non-fatal for selection — leave sizeBytes null + // and let the rank-candidates scorer fall back to its 0-bonus + // default. (Test fixtures sometimes use empty files where statSync + // works fine; the catch covers symlinks-to-nowhere and the like.) + } + + return createCandidate({ + source: 'directory-scan', + url: filename, + packageName: 'directory-scan', + architecture, + format, + sizeBytes, + }); } /** @@ -142,7 +168,9 @@ function findCachedApk(apksDir, targetVersion) { /** * Scan APKS_DIR for all .apk/.xapk/.apkm/.apks files and return the - * highest-scored one. Mirrors the find_package_candidate awk pipeline. + * highest-scored one. Mirrors the find_package_candidate awk pipeline; + * now uses the canonical selectCandidate ranking so legacy regex + * weight tables no longer live here. * * @param {string} apksDir Directory to scan (recursive). * @returns {string|null} Absolute path of best candidate, or null. @@ -163,13 +191,17 @@ function findPackageCandidate(apksDir) { }; walk(apksDir); if (entries.length === 0) return null; - let best = null; - let bestScore = -Infinity; - for (const e of entries) { - const s = scoreApk(e); - if (s > bestScore) { best = e; bestScore = s; } - } - return best; + const candidates = entries.map(filenameToCandidate); + // No preferredArch / versionName at the directory-scan level — the + // packaging-around-arch callers pass those via selectCandidate + // upstream. packageName is the directory-scan sentinel shared by + // every candidate, so isCompatible() treats the whole set as one + // pool and the comparator + tiebreaker pick the same APK that the + // legacy regex weights used to (verified against __tests__). + const chosen = rankCandidates.selectCandidate(candidates, { + packageName: 'directory-scan', + }); + return chosen ? chosen.url : null; } /** @@ -185,10 +217,12 @@ function bestRankedApkInDir(dir) { const entries = fs.readdirSync(dir) .filter(f => f.endsWith('.apk')) .map(f => path.join(dir, f)); - return entries - .map(p => ({ path: p, score: scoreApk(p) })) - .sort((a, b) => b.score - a.score) - .map(o => o.path); + if (entries.length === 0) return []; + const candidates = entries.map(filenameToCandidate); + const comparator = rankCandidates.compareCandidates({ + packageName: 'directory-scan', + }); + return candidates.sort(comparator).map(c => c.url); } /** @@ -294,7 +328,7 @@ function listApkAbis(apk) { module.exports = { extractVersionFromString, - scoreApk, + filenameToCandidate, findCachedApk, findPackageCandidate, bestRankedApkInDir, From 346406be5ffa5eeedbddf358ae6f996f914d8452 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:17:49 +0200 Subject: [PATCH 3/9] refactor(downloader): extract URL builders to src/download/url.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three network-independent helpers moved out of unified-downloader.js into src/download/url.js: - buildReleasePageUrl — pure URL construction - apkmirrorReleaseTailCandidates — pure list of pre-release suffixes (exact → rc0..rc9 → beta → alpha) - apkMirrorAuthHeader — env-var → Basic header; now accepts an optional env object for testing instead of reading process.env directly unified-downloader.js now imports them and orchestrates them around the actual HTTP/Playwright paths. The downloader's APK_MIRROR_API_* module-load env reads move inside apkMirrorAuthHeader's process.env fallback, so the callers don't change. 11 unit tests in src/download/__tests__/url.test.js cover: - buildReleasePageUrl: 3 (basic, double-digit major, slug derivation from deep paths) - apkmirrorReleaseTailCandidates: 3 (canonical SD-Maid fixture, double-digit major, exhaustive rc0..rc9) - apkMirrorAuthHeader: 5 (each missing-env throw case, the canonical Basic-base64 output, and a colon-containing password to pin base64 encoding behavior) apkmirror-scraper.test.js's `buildReleasePageUrl` and `apkmirrorReleaseTailCandidates` tests already exercise the same behaviors; they stay as integration coverage at the unified-downloader layer. 409 tests pass, lint clean. unified-downloader.js shrunk by ~60 lines; it's now 1858 lines (was 1862 before this extraction, with the runCommand/timeout fixes from the previous commit having shifted it to 1899). --- .github/scripts/unified-downloader.js | 74 ++++-------------- src/download/__tests__/url.test.js | 103 ++++++++++++++++++++++++++ src/download/url.js | 97 ++++++++++++++++++++++++ 3 files changed, 215 insertions(+), 59 deletions(-) create mode 100644 src/download/__tests__/url.test.js create mode 100644 src/download/url.js diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 4bf3631..36aa3f9 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -20,10 +20,15 @@ const os = require("node:os"); const cheerio = require('cheerio'); const { validateDownloadedApkAbi } = require('./apk-abi-validator'); const { detectApkShape } = require('./apk-selection'); - -// APKMirror API credentials (from environment; no defaults — see apkMirrorAuthHeader). -const APK_MIRROR_API_USER = process.env.APKMIRROR_API_USER; -const APK_MIRROR_API_PASS = process.env.APKMIRROR_API_PASS; +// Network-independent URL/auth helpers extracted to a pure module so +// they can be unit-tested without standing up the downloader. The +// downloader imports them and orchestrates them around the actual +// HTTP/Playwright fetches below. +const { + buildReleasePageUrl, + apkmirrorReleaseTailCandidates, + apkMirrorAuthHeader, +} = require('../../src/download/url'); // URL cache directory - stores resolved URLs as JSON const URL_CACHE_DIR = path.join(os.homedir(), ".cache", "auto-morphe-builder", "urls"); @@ -66,27 +71,6 @@ const TIMEOUTS = { playwrightDownload:120_000, // page.waitForEvent('download') ceiling. }; -/** - * Build the Authorization header for APKMirror's wp-json API. - * Used by both the URL resolver and the (now-removed) legacy API download path; - * kept centralized so the auth scheme stays in one place. - * - * Throws if either credential is unset. The caller's fallback chain - * (apkeep → apkmirror Playwright) will then be used; the apkmirror-api - * path is just one of several resolution sources. - */ -function apkMirrorAuthHeader() { - if (!APK_MIRROR_API_USER || !APK_MIRROR_API_PASS) { - throw new Error( - 'APKMIRROR_API_USER and/or APKMIRROR_API_PASS are not set. ' + - 'Configure them as repo secrets to enable the APKMirror-API ' + - 'resolution path; the fallback chain (apkeep → apkmirror Playwright) ' + - 'will be used otherwise.' - ); - } - return `Basic ${Buffer.from(`${APK_MIRROR_API_USER}:${APK_MIRROR_API_PASS}`).toString("base64")}`; -} - /** * Get APKMirror path for a package from config.json patch_repos. */ @@ -95,40 +79,12 @@ function getApkmirrorPath(packageId) { return config.patch_repos?.[packageId]?.apkmirror_path || null; } -/** - * Build APKMirror release page URL for a given version. - * Slug is derived from the last path component of apkmirrorPath. - * e.g. "google-inc/youtube" + "20.44.38" → ".../youtube-20-44-38-release/" - */ -function buildReleasePageUrl(apkmirrorPath, version) { - const slug = apkmirrorPath.split('/').pop(); - const versionSlug = version.replace(/\./g, '-'); - return `https://www.apkmirror.com/apk/${apkmirrorPath}/${slug}-${versionSlug}-release/`; -} - -/** - * Pre-release suffixes APKMirror inserts between the version and the - * trailing `-release/` segment when a developer uploads a release - * candidate, beta, or alpha build. The upstream patch repo (and - * `patches-list.json`) usually records the bare version (e.g. `2.0.2`), - * but APKMirror's URL slug uses the pre-release form (`2.0.2-rc0`), - * which our `-release/` selector would miss. We try the - * exact match first, then progressively widen to common suffixes so - * apps like SD Maid (2.0.2 → /sd-maid-2-se-system-cleaner-2-0-2-rc0-release/) - * resolve without per-app configuration. - * - * Ordered by frequency on APKMirror; rc0..rc9 covers the full release - * candidate sequence without skipping numbers (some devs ship rc1 - * straight to rc3, but listing the gaps cheaply). - */ -function apkmirrorReleaseTailCandidates(version) { - const dashed = version.replace(/\./g, '-'); - const tails = [`-${dashed}-release/`]; - for (let i = 0; i < 10; i++) tails.push(`-${dashed}-rc${i}-release/`); - tails.push(`-${dashed}-beta-release/`, `-${dashed}-beta1-release/`); - tails.push(`-${dashed}-alpha-release/`, `-${dashed}-alpha1-release/`); - return tails; -} +// buildReleasePageUrl, apkmirrorReleaseTailCandidates, and +// apkMirrorAuthHeader now live in src/download/url.js (imported at +// the top of this file). They were extracted because the downloader +// grew past 1800 lines and these helpers are pure network- +// independent logic that the apkmirror-scraper test suite already +// exercises; they're now reachable as a focused unit-test target. /** * Resolve APKMirror's actual release-page slug for a given (path, version). diff --git a/src/download/__tests__/url.test.js b/src/download/__tests__/url.test.js new file mode 100644 index 0000000..bd46e90 --- /dev/null +++ b/src/download/__tests__/url.test.js @@ -0,0 +1,103 @@ +'use strict'; + +const { + buildReleasePageUrl, + apkmirrorReleaseTailCandidates, + apkMirrorAuthHeader, +} = require('../url'); + +describe('download/url', () => { + describe('buildReleasePageUrl', () => { + test('constructs correct URL with slug prefix', () => { + expect(buildReleasePageUrl('google-inc/youtube', '20.44.38')).toBe( + 'https://www.apkmirror.com/apk/google-inc/youtube/youtube-20-44-38-release/', + ); + }); + + test('handles double-digit major versions', () => { + expect(buildReleasePageUrl('company/app', '26.07.27')).toBe( + 'https://www.apkmirror.com/apk/company/app/app-26-07-27-release/', + ); + }); + + test('derives slug from the last path component', () => { + // a deeper apkmirrorPath still ends up with the trailing + // component as the slug prefix the route expects. + expect(buildReleasePageUrl('a/b/c/long-name', '1.0.0')).toBe( + 'https://www.apkmirror.com/apk/a/b/c/long-name/long-name-1-0-0-release/', + ); + }); + }); + + describe('apkmirrorReleaseTailCandidates', () => { + test('puts the exact dashed-version tail first, then rc/beta/alpha fallbacks', () => { + expect(apkmirrorReleaseTailCandidates('2.0.2')).toEqual([ + '-2-0-2-release/', + '-2-0-2-rc0-release/', + '-2-0-2-rc1-release/', + '-2-0-2-rc2-release/', + '-2-0-2-rc3-release/', + '-2-0-2-rc4-release/', + '-2-0-2-rc5-release/', + '-2-0-2-rc6-release/', + '-2-0-2-rc7-release/', + '-2-0-2-rc8-release/', + '-2-0-2-rc9-release/', + '-2-0-2-beta-release/', + '-2-0-2-beta1-release/', + '-2-0-2-alpha-release/', + '-2-0-2-alpha1-release/', + ]); + }); + + test('handles a double-digit major version like sofascore 26.07.27', () => { + const tails = apkmirrorReleaseTailCandidates('26.07.27'); + expect(tails[0]).toBe('-26-07-27-release/'); + expect(tails).toContain('-26-07-27-rc0-release/'); + expect(tails).toContain('-26-07-27-alpha1-release/'); + }); + + test('every rc tier from rc0 through rc9 is present', () => { + const tails = apkmirrorReleaseTailCandidates('1.0.0'); + for (let i = 0; i <= 9; i += 1) { + expect(tails).toContain(`-1-0-0-rc${i}-release/`); + } + }); + }); + + describe('apkMirrorAuthHeader', () => { + test('throws when APKMIRROR_API_USER is missing', () => { + expect(() => apkMirrorAuthHeader({ + APKMIRROR_API_PASS: 'pass', + })).toThrow(/APKMIRROR_API_USER and\/or APKMIRROR_API_PASS are not set/); + }); + + test('throws when APKMIRROR_API_PASS is missing', () => { + expect(() => apkMirrorAuthHeader({ + APKMIRROR_API_USER: 'user', + })).toThrow(/APKMIRROR_API_USER and\/or APKMIRROR_API_PASS are not set/); + }); + + test('throws when both are missing', () => { + expect(() => apkMirrorAuthHeader({})).toThrow(/APKMIRROR_API_USER and\/or APKMIRROR_API_PASS are not set/); + }); + + test('returns a Basic auth header with base64-encoded user:pass', () => { + const header = apkMirrorAuthHeader({ + APKMIRROR_API_USER: 'alice', + APKMIRROR_API_PASS: 's3cret', + }); + // 'alice:s3cret' base64 = 'YWxpY2U6czNjcmV0' + expect(header).toBe('Basic YWxpY2U6czNjcmV0'); + }); + + test('accepts colon-containing passwords without mangling', () => { + const header = apkMirrorAuthHeader({ + APKMIRROR_API_USER: 'u', + APKMIRROR_API_PASS: 'p:p', + }); + // 'u:p:p' base64 = 'dTpwOnA=' + expect(header).toBe('Basic dTpwOnA='); + }); + }); +}); diff --git a/src/download/url.js b/src/download/url.js new file mode 100644 index 0000000..ebc23b3 --- /dev/null +++ b/src/download/url.js @@ -0,0 +1,97 @@ +'use strict'; + +/** + * URL building for the downloader. + * + * Pure network-independent helpers extracted from + * `.github/scripts/unified-downloader.js`. Kept side-effect free so + * they can be unit-tested without standing up a downloader — the + * downloader now imports these and orchestrates them around the + * actual HTTP/Playwright fetches. + * + * Functions: + * - buildReleasePageUrl pure URL construction for an APKMirror + * release page + * - apkmirrorReleaseTailCandidates pure list of dashed-version + * suffix tails (exact, rc, beta, + * alpha) tried in order when + * APKMirror's slug includes a + * pre-release marker + * - apkMirrorAuthHeader Basic auth header from configured + * credentials; throws when either + * APKMIRROR_API_USER or APKMIRROR_API_PASS + * is missing + */ + +/** + * Build APKMirror release page URL for a given version. + * Slug is derived from the last path component of apkmirrorPath. + * e.g. "google-inc/youtube" + "20.44.38" → ".../youtube-20-44-38-release/" + */ +function buildReleasePageUrl(apkmirrorPath, version) { + const slug = apkmirrorPath.split('/').pop(); + const versionSlug = version.replace(/\./g, '-'); + return `https://www.apkmirror.com/apk/${apkmirrorPath}/${slug}-${versionSlug}-release/`; +} + +/** + * Pre-release suffixes APKMirror inserts between the version and the + * trailing `-release/` segment when a developer uploads a release + * candidate, beta, or alpha build. The upstream patch repo (and + * `patches-list.json`) usually records the bare version (e.g. `2.0.2`), + * but APKMirror's URL slug uses the pre-release form (`2.0.2-rc0`), + * which our `-release/` selector would miss. We try the + * exact match first, then progressively widen to common suffixes so + * apps like SD Maid (2.0.2 → /sd-maid-2-se-system-cleaner-2-0-2-rc0-release/) + * resolve without per-app configuration. + * + * Ordered by frequency on APKMirror; rc0..rc9 covers the full release + * candidate sequence. + */ +function apkmirrorReleaseTailCandidates(version) { + const tails = [`-${version.replace(/\./g, '-')}-release/`]; + for (let i = 0; i <= 9; i += 1) { + tails.push(`-${version.replace(/\./g, '-')}-rc${i}-release/`); + } + tails.push(`-${version.replace(/\./g, '-')}-beta-release/`); + tails.push(`-${version.replace(/\./g, '-')}-beta1-release/`); + tails.push(`-${version.replace(/\./g, '-')}-alpha-release/`); + tails.push(`-${version.replace(/\./g, '-')}-alpha1-release/`); + return tails; +} + +/** + * Build the Authorization header for APKMirror's wp-json API. + * + * @param {object} [env=process.env] Override for the env object — + * tests pass a stub so they don't + * depend on process.env state. + * The default (process.env) is what + * production callers use. + * @returns {string} `Basic `. + * @throws {Error} When either APKMIRROR_API_USER or APKMIRROR_API_PASS + * is unset. The caller's fallback chain + * (apkeep → apkmirror Playwright) is used in that case; + * the apkmirror-api path is just one of several + * resolution sources. + */ +function apkMirrorAuthHeader(env) { + const source = env || process.env; + const user = source.APKMIRROR_API_USER; + const pass = source.APKMIRROR_API_PASS; + if (!user || !pass) { + throw new Error( + 'APKMIRROR_API_USER and/or APKMIRROR_API_PASS are not set. ' + + 'Configure them as repo secrets to enable the APKMirror-API ' + + 'resolution path; the fallback chain (apkeep → apkmirror Playwright) ' + + 'will be used otherwise.', + ); + } + return `Basic ${Buffer.from(`${user}:${pass}`).toString('base64')}`; +} + +module.exports = { + buildReleasePageUrl, + apkmirrorReleaseTailCandidates, + apkMirrorAuthHeader, +}; From 9fbd3e7fdd2bde4aa287d65294631a52063f8ee8 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:19:32 +0200 Subject: [PATCH 4/9] refactor(downloader): extract APKMirror variant selection to src/download/variant.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move buildVariantPriorities() and selectVariant() from unified-downloader.js into src/download/variant.js. Both are pure operations on a cheerio-loaded DOM and the priority cartesian product — no network — so they're safe to extract even though selectVariant walks a cheerio handle. buildVariantPriorities(preferredArch): Cartesian product of {[preferredArch, universal, noarch]} × {nodpi, 120-640dpi, 480-640dpi, 120-480dpi, 240-480dpi} × {APK, BUNDLE}, with BUNDLE suppressed for the noarch tier. The 25-entry list per preferredArch matches the picker in the Playwright fallback. selectVariant($, priorities): walks the .table-row entries on a cheerio-loaded release page, dedupes by href, returns the first href that matches an entry in priorities (in iteration order), and throws with the available variants if nothing matched. src/download/__tests__/variant.test.js exercises the priority ordering (5 tests, including the doc-pinned "5 arch/type combos × 5 DPIs = 25 priorities" sanity check), and selectVariant under 5 scenarios: basic first-priority match, fall-through to a later priority, the throw-with-available-variants contract, anchor-only link skipping, and BUNDLE detection from the row label even when the link text doesn't include the word. apkmirror-scraper.test.js's existing selectVariant / buildVariantPriorities tests at the unified-downloader layer stay as integration coverage alongside the new unit-test target. 420 tests pass, lint clean. --- .github/scripts/unified-downloader.js | 75 +-------------- src/download/__tests__/variant.test.js | 121 +++++++++++++++++++++++++ src/download/variant.js | 105 +++++++++++++++++++++ 3 files changed, 230 insertions(+), 71 deletions(-) create mode 100644 src/download/__tests__/variant.test.js create mode 100644 src/download/variant.js diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 36aa3f9..8228922 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -29,6 +29,10 @@ const { apkmirrorReleaseTailCandidates, apkMirrorAuthHeader, } = require('../../src/download/url'); +const { + buildVariantPriorities, + selectVariant, +} = require('../../src/download/variant'); // URL cache directory - stores resolved URLs as JSON const URL_CACHE_DIR = path.join(os.homedir(), ".cache", "auto-morphe-builder", "urls"); @@ -200,77 +204,6 @@ async function resolveApkmirrorReleaseSlug(apkmirrorPath, version, opts = {}) { } } -/** - * Build ordered variant priority list from preferred arch. - * Outer loop = DPI tier (outer is more important), inner loop = arch/type. - * Within each DPI tier: preferred APK → preferred BUNDLE → universal APK - * → universal BUNDLE → noarch APK. - * - * DPI preference is APKMirror-only. APKMirror exposes a variant table - * with explicit DPI columns, so we can pick a precise target. APKPure - * (via apkeep) doesn't expose DPI as a selectable axis — the apkeep - * path takes whatever APKPure serves, then validates the resulting - * .apk against `preferred_arch` post-download and falls back to the - * next source if the ABI doesn't match. - * - * Tiers ordered by band tightness: - * nodpi → no DPI-specific resources, runs on any density. - * 120-640dpi → assets-up to 640, asset-densities up to 480 — a - * wide umbrella that covers every shipping device. - * 480-640dpi → upper-density-only, falls back to lower densities - * visually (smaller assets on a phone but fine). - * 120-480dpi → explicit upper bound of 480 (xxxhdpi excluded). - * 240-480dpi → narrower band than 120-480dpi, last resort. - */ -function buildVariantPriorities(preferredArch) { - const archs = [preferredArch, 'universal', 'noarch']; - const dpis = ['nodpi', '120-640dpi', '480-640dpi', '120-480dpi', '240-480dpi']; - const priorities = []; - for (const dpi of dpis) { - for (const arch of archs) { - priorities.push({ arch, dpi, type: 'APK' }); - if (arch !== 'noarch') priorities.push({ arch, dpi, type: 'BUNDLE' }); - } - } - return priorities; -} - -/** - * Parse variant table rows from a cheerio-loaded release page. - * Returns the href of the first row matching the priority list. - * Throws with available variants if nothing matches. - */ -function selectVariant($, priorities) { - const rows = []; - $('.table-row').each((_, row) => { - const cells = $(row).find('.table-cell'); - if (cells.length < 4) return; - // Real APKMirror DOM: cells[0]=variant name+type+link, cells[1]=arch, cells[2]=minver, cells[3]=dpi - const href = $(cells[0]).find('a.accent_color[href], a[href*="/apk/"]').attr('href'); - if (!href || href.includes('#')) return; // Skip anchor-only sidebar links - const variantText = $(cells[0]).text().toUpperCase(); - const type = variantText.includes('BUNDLE') ? 'BUNDLE' : 'APK'; - rows.push({ - dpi: $(cells[3]).text().trim().toLowerCase(), - arch: $(cells[1]).text().trim().toLowerCase(), - type, - href, - }); - }); - - for (const { arch, dpi, type } of priorities) { - const match = rows.find(r => - r.arch.includes(arch.toLowerCase()) && - r.dpi === dpi.toLowerCase() && - r.type === type - ); - if (match) return match.href; - } - - const found = rows.map(r => `${r.arch}/${r.dpi}/${r.type}`).join(', ') || 'none'; - throw new Error(`No matching variant found on APKMirror. Available: ${found}`); -} - /** * Collect cookies from a fetch Response's Set-Cookie headers into a plain object. * Uses getSetCookie() which returns an array — safe for multi-cookie responses. diff --git a/src/download/__tests__/variant.test.js b/src/download/__tests__/variant.test.js new file mode 100644 index 0000000..3bdb2b1 --- /dev/null +++ b/src/download/__tests__/variant.test.js @@ -0,0 +1,121 @@ +'use strict'; + +const cheerio = require('cheerio'); +const { buildVariantPriorities, selectVariant } = require('../variant'); + +function makeRowHtml(row) { + return ` +
+ +
${row.arch}
+
minver
+
${row.dpi}
+
`; +} + +function makePageHtml(rows) { + return `
${rows.map(makeRowHtml).join('')}
`; +} + +describe('download/variant', () => { + describe('buildVariantPriorities', () => { + test('preferred_arch is first priority as APK', () => { + const priorities = buildVariantPriorities('arm64-v8a'); + expect(priorities[0]).toEqual({ arch: 'arm64-v8a', dpi: 'nodpi', type: 'APK' }); + }); + + test('universal APK is third priority', () => { + const priorities = buildVariantPriorities('arm64-v8a'); + expect(priorities[2]).toEqual({ arch: 'universal', dpi: 'nodpi', type: 'APK' }); + }); + + test('noarch APK is fifth priority', () => { + const priorities = buildVariantPriorities('arm64-v8a'); + expect(priorities[4]).toEqual({ arch: 'noarch', dpi: 'nodpi', type: 'APK' }); + }); + + test('returns 25 priorities total (5 arch/type combos × 5 DPIs)', () => { + // 5 DPIs × 5 arch/type combos per DPI (preferredArch APK, + // preferredArch BUNDLE, universal APK, universal BUNDLE, noarch APK) + // = 25. noarch skips the BUNDLE entry, so 4 BUNDLE + 5 APK per tier. + expect(buildVariantPriorities('arm64-v8a')).toHaveLength(25); + }); + + test('order within a DPI tier: preferred APK, preferred BUNDLE, universal APK, universal BUNDLE, noarch APK', () => { + const priorities = buildVariantPriorities('arm64-v8a'); + // Take the first tier (nodpi) and verify the inner-loop order. + const tier = priorities.slice(0, 5); + expect(tier).toEqual([ + { arch: 'arm64-v8a', dpi: 'nodpi', type: 'APK' }, + { arch: 'arm64-v8a', dpi: 'nodpi', type: 'BUNDLE' }, + { arch: 'universal', dpi: 'nodpi', type: 'APK' }, + { arch: 'universal', dpi: 'nodpi', type: 'BUNDLE' }, + { arch: 'noarch', dpi: 'nodpi', type: 'APK' }, + ]); + }); + + test('outer loop is the DPI tier', () => { + const priorities = buildVariantPriorities('arm64-v8a'); + // The 6th entry starts the next DPI tier. + expect(priorities[5]).toEqual({ arch: 'arm64-v8a', dpi: '120-640dpi', type: 'APK' }); + }); + }); + + describe('selectVariant', () => { + test('returns the first priority that matches a row', () => { + const html = makePageHtml([ + { arch: 'arm64-v8a', dpi: 'nodpi', type: 'APK', href: '/apk/arm64-apk-nodpi/' }, + { arch: 'arm64-v8a', dpi: 'nodpi', type: 'BUNDLE', href: '/apk/arm64-bundle-nodpi/' }, + ]); + const $ = cheerio.load(html); + const priorities = buildVariantPriorities('arm64-v8a'); + // The nodpi APK is priority 0 and exists in the table, so it + // wins before any later entry. + expect(selectVariant($, priorities)).toBe('/apk/arm64-apk-nodpi/'); + }); + + test('falls through to a later priority when earlier ones are missing', () => { + const html = makePageHtml([ + // Only one row available: universal bundle at nodpi. + { arch: 'universal', dpi: 'nodpi', type: 'BUNDLE', href: '/apk/universal-bundle-nodpi/' }, + ]); + const $ = cheerio.load(html); + const priorities = buildVariantPriorities('arm64-v8a'); + // Skip preferred APK (no row), skip preferred BUNDLE (no row), + // skip universal APK (no row) → pick universal BUNDLE. + expect(selectVariant($, priorities)).toBe('/apk/universal-bundle-nodpi/'); + }); + + test('throws with the available variants when nothing matches', () => { + const html = makePageHtml([ + { arch: 'x86', dpi: 'nodpi', type: 'APK', href: '/apk/x86-apk/' }, + ]); + const $ = cheerio.load(html); + // Ask for arm64-v8a; x86 doesn't match any priority. + expect(() => selectVariant($, buildVariantPriorities('arm64-v8a'))).toThrow( + /No matching variant found on APKMirror\. Available: x86\/nodpi\/APK/, + ); + }); + + test('skips anchor-only links (href contains #)', () => { + const html = makePageHtml([ + // The variant row link is #foo, the parent is + // absent. Per the implementation, href.includes('#') → skip. + { arch: 'arm64-v8a', dpi: 'nodpi', type: 'APK', href: '#sidebar' }, + ]); + const $ = cheerio.load(html); + expect(() => selectVariant($, buildVariantPriorities('arm64-v8a'))).toThrow( + /Available: none/, + ); + }); + + test('BUNDLE is detected from the row label even when no href text contains the word', () => { + const html = makePageHtml([ + { name: 'APK + bundle', arch: 'arm64-v8a', dpi: 'nodpi', type: 'BUNDLE', href: '/apk/arm64-bundle/' }, + ]); + const $ = cheerio.load(html); + // After falling through the APK slots, the BUNDLE slot picks it up. + expect(selectVariant($, buildVariantPriorities('arm64-v8a'))).toBe('/apk/arm64-bundle/'); + }); + }); +}); diff --git a/src/download/variant.js b/src/download/variant.js new file mode 100644 index 0000000..48cd03b --- /dev/null +++ b/src/download/variant.js @@ -0,0 +1,105 @@ +'use strict'; + +/** + * APKMirror variant selection extracted from + * `.github/scripts/unified-downloader.js`. + * + * The play is: + * 1. `buildVariantPriorities(preferredArch)` builds the ordered + * list of (arch, dpi, type) triples to try. Outer loop = DPI + * tier (more important), inner loop = arch/type combo. + * 2. `selectVariant($, priorities)` parses a cheerio-loaded + * release page's variant table and returns the href of the + * first row matching the priority list, throwing with the + * available variants if none matches. + * + * Both helpers are pure (no I/O, no network): `buildVariantPriorities` + * just enumerates a fixed cartesian product, and `selectVariant` walks + * a pre-loaded DOM. cheerio's `load` is HTML-parsing, not network — + * the downloader hands us `$`, we walk it and return a string. + * + * DPI preference is APKMirror-only. APKMirror exposes a variant table + * with explicit DPI columns, so we can pick a precise target. APKPure + * (via apkeep) doesn't expose DPI as a selectable axis — the apkeep + * path takes whatever APKPure serves, then validates the resulting + * .apk against `preferred_arch` post-download and falls back to the + * next source if the ABI doesn't match. + * + * Tiers ordered by band tightness: + * nodpi → no DPI-specific resources, runs on any density. + * 120-640dpi → assets-up to 640, asset-densities up to 480 — a + * wide umbrella that covers every shipping device. + * 480-640dpi → upper-density-only, falls back to lower densities + * visually (smaller assets on a phone but fine). + * 120-480dpi → explicit upper bound of 480 (xxxhdpi excluded). + * 240-480dpi → narrower band than 120-480dpi, last resort. + * + * Within each DPI tier: preferred APK → preferred BUNDLE → universal + * APK → universal BUNDLE → noarch APK. BUNDLE is skipped for the + * noarch tier (noarch never ships as a split package). + */ + +/** + * @param {string} preferredArch + * @returns {Array<{ arch: string, dpi: string, type: 'APK' | 'BUNDLE' }>} + * The full priority list, in iteration order. + */ +function buildVariantPriorities(preferredArch) { + const archs = [preferredArch, 'universal', 'noarch']; + const dpis = ['nodpi', '120-640dpi', '480-640dpi', '120-480dpi', '240-480dpi']; + const priorities = []; + for (const dpi of dpis) { + for (const arch of archs) { + priorities.push({ arch, dpi, type: 'APK' }); + if (arch !== 'noarch') priorities.push({ arch, dpi, type: 'BUNDLE' }); + } + } + return priorities; +} + +/** + * Parse variant table rows from a cheerio-loaded release page. + * Returns the href of the first row matching the priority list. + * Throws with available variants if nothing matches. + * + * @param {object} $ A cheerio root handle (from `cheerio.load(html)`). + * @param {Array<{ arch: string, dpi: string, type: string }>} priorities + * @returns {string} The href of the first matching row. + * @throws {Error} With the available `(arch/dpi/type)` triples if no + * row matched. + */ +function selectVariant($, priorities) { + const rows = []; + $('.table-row').each((_, row) => { + const cells = $(row).find('.table-cell'); + if (cells.length < 4) return; + // Real APKMirror DOM: cells[0]=variant name+type+link, + // cells[1]=arch, cells[2]=minver, cells[3]=dpi + const href = $(cells[0]).find('a.accent_color[href], a[href*="/apk/"]').attr('href'); + if (!href || href.includes('#')) return; // Skip anchor-only sidebar links + const variantText = $(cells[0]).text().toUpperCase(); + const type = variantText.includes('BUNDLE') ? 'BUNDLE' : 'APK'; + rows.push({ + dpi: $(cells[3]).text().trim().toLowerCase(), + arch: $(cells[1]).text().trim().toLowerCase(), + type, + href, + }); + }); + + for (const { arch, dpi, type } of priorities) { + const match = rows.find((r) => + r.arch.includes(arch.toLowerCase()) && + r.dpi === dpi.toLowerCase() && + r.type === type); + if (match) return match.href; + } + + const found = rows.map((r) => `${r.arch}/${r.dpi}/${r.type}`).join(', ') || 'none'; + throw new Error(`No matching variant found on APKMirror. Available: ${found}`); +} + +module.exports = { + buildVariantPriorities, + selectVariant, +}; From d6b6bb8cb906c4eff19bcc5eefc6c286fbab65af Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:20:21 +0200 Subject: [PATCH 5/9] refactor(downloader): extract cookie jar helpers to src/download/cookies.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move collectCookies(response, existing) into a focused module. The function is a pure transformation over a fetch Response's Set-Cookie array — no network, no I/O — so it's a clean fit for the extraction pattern used by src/download/url.js and variant.js. Two behavior deltas in the extracted version, both safe and tested: - Returned jar is always a fresh object even when no Set-Cookie headers were present (the inline version returned the input `existing` reference unchanged when nothing was merged). Callers in unified-downloader only read the object, never compare identity, so this is safe and gives consistent mutation safety across the empty-cookie path too. - Handles missing `headers.getSetCookie()` gracefully (older fetch implementations). The previous `?.` chain returned `existing` unchanged in that case; the extracted version returns a copy. 10 unit tests in src/download/__tests__/cookies.test.js cover the empty-jar fast path, missing-API graceful handling, single/multi cookie parsing, malformed entries (no `=`, leading `=`), overwriting existing keys, values containing additional `=` characters, and input immutability. 430 tests pass, lint clean. --- .github/scripts/unified-downloader.js | 19 +----- src/download/__tests__/cookies.test.js | 84 ++++++++++++++++++++++++++ src/download/cookies.js | 43 +++++++++++++ 3 files changed, 128 insertions(+), 18 deletions(-) create mode 100644 src/download/__tests__/cookies.test.js create mode 100644 src/download/cookies.js diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 8228922..f57b874 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -33,6 +33,7 @@ const { buildVariantPriorities, selectVariant, } = require('../../src/download/variant'); +const { collectCookies } = require('../../src/download/cookies'); // URL cache directory - stores resolved URLs as JSON const URL_CACHE_DIR = path.join(os.homedir(), ".cache", "auto-morphe-builder", "urls"); @@ -204,24 +205,6 @@ async function resolveApkmirrorReleaseSlug(apkmirrorPath, version, opts = {}) { } } -/** - * Collect cookies from a fetch Response's Set-Cookie headers into a plain object. - * Uses getSetCookie() which returns an array — safe for multi-cookie responses. - * Merges with any existing cookies. - */ -function collectCookies(response, existing = {}) { - const setCookies = response.headers.getSetCookie?.() ?? []; - if (setCookies.length === 0) return existing; - const cookies = { ...existing }; - for (const cookie of setCookies) { - const [pair] = cookie.split(';'); - const eqIdx = pair.indexOf('='); - if (eqIdx < 1) continue; - cookies[pair.slice(0, eqIdx).trim()] = pair.slice(eqIdx + 1).trim(); - } - return cookies; -} - /** * Make a request with browser-like headers using curl subprocess. * Node's built-in fetch has a different TLS fingerprint that Cloudflare detects. diff --git a/src/download/__tests__/cookies.test.js b/src/download/__tests__/cookies.test.js new file mode 100644 index 0000000..bcf21b9 --- /dev/null +++ b/src/download/__tests__/cookies.test.js @@ -0,0 +1,84 @@ +'use strict'; + +const { collectCookies } = require('../cookies'); + +function makeResponse(setCookieHeaders) { + // Minimal Response shape: only `headers.getSetCookie` is read by + // collectCookies, so that's the only method we stub. + return { + headers: { + getSetCookie: () => setCookieHeaders, + }, + }; +} + +describe('download/cookies', () => { + test('returns a copy of the existing jar when no Set-Cookie headers are present', () => { + const response = makeResponse([]); + const existing = { auth: 'secret' }; + const jar = collectCookies(response, existing); + expect(jar).toEqual({ auth: 'secret' }); + // Same content, distinct object — mutation safety. + expect(jar).not.toBe(existing); + }); + + test('works with a missing getSetCookie method (older fetch impls)', () => { + const response = { headers: {} }; + const jar = collectCookies(response, { a: '1' }); + expect(jar).toEqual({ a: '1' }); + }); + + test('merges a single Set-Cookie header into an empty jar', () => { + const response = makeResponse(['session=abc123; Path=/; HttpOnly']); + const jar = collectCookies(response, {}); + expect(jar).toEqual({ session: 'abc123' }); + }); + + test('merges new cookies onto an existing jar without losing entries', () => { + const response = makeResponse(['session=abc123; Path=/; HttpOnly']); + const jar = collectCookies(response, { auth: 'token' }); + expect(jar).toEqual({ auth: 'token', session: 'abc123' }); + }); + + test('newer Set-Cookie overwrites an existing same-named key', () => { + const response = makeResponse(['session=newvalue']); + const jar = collectCookies(response, { session: 'oldvalue', other: 'kept' }); + expect(jar).toEqual({ session: 'newvalue', other: 'kept' }); + }); + + test('handles multiple cookies in one Set-Cookie array', () => { + const response = makeResponse([ + 'a=1; Path=/', + 'b=2; Domain=example.com', + 'c=3; Secure; SameSite=Lax', + ]); + const jar = collectCookies(response, {}); + expect(jar).toEqual({ a: '1', b: '2', c: '3' }); + }); + + test('skips malformed Set-Cookie entries (no `=` at all)', () => { + const response = makeResponse(['no-equals-sign', 'good=ok']); + const jar = collectCookies(response, {}); + expect(jar).toEqual({ good: 'ok' }); + }); + + test('skips Set-Cookie entries where `=` is at index 0', () => { + // '=value' is malformed; we require at least one char before `=`. + const response = makeResponse(['=value', 'good=ok']); + const jar = collectCookies(response, {}); + expect(jar).toEqual({ good: 'ok' }); + }); + + test('preserves values that contain `=` (only the first `=` splits)', () => { + // The cookie value `abc=def==` only splits on the first `=`. + const response = makeResponse(['token=abc=def==']); + const jar = collectCookies(response, {}); + expect(jar).toEqual({ token: 'abc=def==' }); + }); + + test('does not mutate the input jar', () => { + const existing = { a: '1' }; + collectCookies(makeResponse(['b=2']), existing); + expect(existing).toEqual({ a: '1' }); + }); +}); diff --git a/src/download/cookies.js b/src/download/cookies.js new file mode 100644 index 0000000..e2249bd --- /dev/null +++ b/src/download/cookies.js @@ -0,0 +1,43 @@ +'use strict'; + +/** + * Cookie-jar helpers extracted from + * `.github/scripts/unified-downloader.js`. + * + * `collectCookies(response, existing)` merges the cookies in a fetch + * `Response`'s `Set-Cookie` headers into the jar object passed as + * `existing`. Pure transformation: no I/O, no network, no mutation + * of the input — new cookies overwrite same-named keys in the + * returned jar, the rest of the jar is unchanged. + * + * `getSetCookie()` is the WHATWG-fetch API for reading the array of + * Set-Cookie values off a response. Older `get('set-cookie')` returns + * a comma-joined string that's unsafe to split on `,` because + * `Expires=Wed, 09 Nov 2026 07:28:00 GMT` contains one. Using + * `getSetCookie()` avoids that whole class of bug. + */ + +/** + * @param {object} response Fetch Response-like; only `headers.getSetCookie()` + * is read. + * @param {object} [existing={}] Current cookie jar. The returned + * jar is a fresh object; the input is + * not mutated. + * @returns {object} Merged cookie jar. + */ +function collectCookies(response, existing = {}) { + const setCookies = response.headers?.getSetCookie?.() ?? []; + if (setCookies.length === 0) return { ...existing }; + const cookies = { ...existing }; + for (const cookie of setCookies) { + const [pair] = cookie.split(';'); + const eqIdx = pair.indexOf('='); + if (eqIdx < 1) continue; // Skip malformed cookies (no `=` or `=` at index 0). + cookies[pair.slice(0, eqIdx).trim()] = pair.slice(eqIdx + 1).trim(); + } + return cookies; +} + +module.exports = { + collectCookies, +}; From 9324102765ad1d64256151a54bfc0c8e785e3752 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:22:31 +0200 Subject: [PATCH 6/9] refactor(downloader): extract URL cache helpers to src/download/cache.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move getCachedUrl / saveCachedUrl / cleanupOldUrls into src/download/cache.js. The cache lives at ~/.cache/auto-morphe-builder/urls//.json and stores the most-recently resolved APK URL plus `source`, `downloads`, and `lastWorkingAt` analytics. The extracted module: - Accepts an explicit `cacheDir` parameter (defaults to `~/.cache/auto-morphe-builder/urls`) so tests can pass a per-test tmpfs path instead of sharing state with the host. Production callers in unified-downloader use the default. - Exposes DEFAULT_CACHE_DIR for any future caller that needs the root path; internally the downloader keeps the historical `URL_CACHE_DIR` constant as a one-line alias `= DEFAULT_CACHE_DIR` (later dropped because nothing in the file references it once the inline implementations are gone — the import binding is what callers reach through). - Sanitizes the version string the same way the inline implementation did (replace anything outside [a-zA-Z0-9.-] with `_`) so the on-disk filename can't escape the cache root. 14 unit tests in src/download/__tests__/cache.test.js cover: - DEFAULT_CACHE_DIR shape (1) - getCachedUrl: miss, hit, corrupt JSON, version independence, filename sanitization (5) - saveCachedUrl: required-param throws, JSON shape, downloads counter increment, package-dir auto-create, corrupt-entry recovery (5) - cleanupOldUrls: missing dir, mtime-based selection with deterministic utimes, no-op when entries ≤ keep (3) The mtime-based selection test writes files with explicit fs.utimesSync timestamps — saveCachedUrl's auto-prune after each save would otherwise limit the test to a single retained entry. 444 tests pass, lint clean. --- .github/scripts/unified-downloader.js | 148 ++--------------------- src/download/__tests__/cache.test.js | 168 ++++++++++++++++++++++++++ src/download/cache.js | 167 +++++++++++++++++++++++++ 3 files changed, 347 insertions(+), 136 deletions(-) create mode 100644 src/download/__tests__/cache.test.js create mode 100644 src/download/cache.js diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index f57b874..d75240a 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -34,9 +34,11 @@ const { selectVariant, } = require('../../src/download/variant'); const { collectCookies } = require('../../src/download/cookies'); - -// URL cache directory - stores resolved URLs as JSON -const URL_CACHE_DIR = path.join(os.homedir(), ".cache", "auto-morphe-builder", "urls"); +const { + getCachedUrl, + saveCachedUrl, + cleanupOldUrls, +} = require('../../src/download/cache'); // Source priority for the resolver fallback chain. Higher = preferred. // This is the single source of truth for the order in which APK sources @@ -241,139 +243,13 @@ async function apkmirrorFetch(url, cookies = {}, referer = null) { }; } -/** - * Check URL cache for a package version - * @returns {object|null} Cache entry or null if not found/invalid - */ -function getCachedUrl(packageId, version) { - const cacheDir = path.join(URL_CACHE_DIR, packageId); - const cacheFile = path.join(cacheDir, `${version}.json`); - - if (!fs.existsSync(cacheFile)) { - console.error(`[url-cache] Miss: ${packageId} v${version}`); - return null; - } - - try { - const cacheData = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); - console.error(`[url-cache] Hit: ${packageId} v${version} (source: ${cacheData.source}, downloads: ${cacheData.downloads})`); - return cacheData; - } catch (e) { - console.error(`[url-cache] Error reading cache: ${e.message}`); - return null; - } -} - -/** - * Save URL to cache - * @param {string} packageId - Package ID - * @param {string} version - Version - * @param {string} url - Resolved URL - * @param {string} source - Source that provided the URL - * @returns {string} Path to cached file - */ -function saveCachedUrl(packageId, version, url, source) { - // Input validation - if (!packageId || !version || !url) { - throw new Error('Missing required parameters'); - } - - const cacheDir = path.join(URL_CACHE_DIR, packageId); - - // Create directory if it doesn't exist (race-safe: mkdirSync with - // { recursive: true } is atomic on POSIX when the parent already - // exists, and the only race window is between existsSync and mkdirSync, - // which is mitigated by the recursive option). - // codeql[js/file-system-race] reason: cacheDir is constructed from a - // sanitized packageId and lives in the workflow's user-owned ~/.cache; - // an attacker with write access to the cache directory already owns - // the workflow. - if (!fs.existsSync(cacheDir)) { - fs.mkdirSync(cacheDir, { recursive: true }); - } - - // Sanitize version for use in filename to prevent path traversal - const safeVersion = version.replace(/[^a-zA-Z0-9.-]/g, '_'); - // codeql[js/file-system-race] reason: cacheFile is built from a - // sanitized version string into a user-owned cache directory. - const cacheFile = path.join(cacheDir, `${safeVersion}.json`); - - // Read existing cache or create new - // codeql[js/file-system-race] reason: existsSync + readFileSync TOCTOU - // window is on a user-owned cache file we just constructed the path - // for; in practice the read failure is handled by the try/catch. - let cacheData = { downloads: 0, lastWorkingAt: null }; - if (fs.existsSync(cacheFile)) { - try { - // codeql[js/http-to-file-access] reason: cacheData is parsed from - // a JSON file we own (user-owned ~/.cache), written by saveCachedUrl - // elsewhere in this module. Trust boundary = the workflow itself. - cacheData = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); - } catch (e) { - console.error(`[url-cache] Corrupted cache file, recreating: ${e.message}`); - } - } - - // Update cache entry - const newCacheData = { - version, - url, - source, - resolvedAt: new Date().toISOString(), - downloads: cacheData.downloads + 1, - lastWorkingAt: new Date().toISOString() - }; - - // codeql[js/file-system-race] reason: cacheFile is built from a sanitized - // packageId + sanitized version, lives in the user-owned cache dir. - // codeql[js/http-to-file-access] reason: newCacheData is constructed - // in this module from URL metadata we cached; not attacker-controlled. - fs.writeFileSync(cacheFile, JSON.stringify(newCacheData, null, 2)); - console.error(`[url-cache] Saved: ${packageId} v${version} from ${source}`); - - // Prune older version entries to prevent unbounded growth. - cleanupOldUrls(packageId); - - return cacheFile; -} - -/** - * Prune URL cache entries for a package, keeping only the most-recently - * updated ones. - * @param {string} packageId - * @param {number} keep Number of most-recent entries to retain (default 3). - */ -function cleanupOldUrls(packageId, keep = 3) { - const cacheDir = path.join(URL_CACHE_DIR, packageId); - if (!fs.existsSync(cacheDir)) { - return 0; - } - - const entries = fs.readdirSync(cacheDir) - .filter(f => f.endsWith(".json")) - .map(f => { - const fp = path.join(cacheDir, f); - try { - const stat = fs.statSync(fp); - return { file: fp, mtime: stat.mtimeMs }; - } catch { - return null; - } - }) - .filter(Boolean) - .sort((a, b) => b.mtime - a.mtime); - - const toDelete = entries.slice(keep); - for (const entry of toDelete) { - try { - fs.unlinkSync(entry.file); - console.error(`[url-cache] Pruned old entry: ${entry.file}`); - } catch (e) { - console.error(`[url-cache] Failed to prune ${entry.file}: ${e.message}`); - } - } - return toDelete.length; -} +// getCachedUrl / saveCachedUrl / cleanupOldUrls now live in +// src/download/cache.js (imported at the top of this file). They +// were extracted because the downloader grew past 1800 lines and +// these helpers are the local-file-cache foundation for every +// fallback-chain path. See src/download/cache.js for the +// packageDir / cacheFileFor layout and the documented race window +// in saveCachedUrl's mkdirSync({ recursive: true }). /** * Verify URL still works with HEAD request diff --git a/src/download/__tests__/cache.test.js b/src/download/__tests__/cache.test.js new file mode 100644 index 0000000..565a111 --- /dev/null +++ b/src/download/__tests__/cache.test.js @@ -0,0 +1,168 @@ +'use strict'; + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { + DEFAULT_CACHE_DIR, + getCachedUrl, + saveCachedUrl, + cleanupOldUrls, +} = require('../cache'); + +describe('download/cache', () => { + let tmpCache; + + beforeEach(() => { + tmpCache = fs.mkdtempSync(path.join(os.tmpdir(), 'url-cache-test-')); + }); + + afterEach(() => { + try { fs.rmSync(tmpCache, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + describe('module surface', () => { + test('DEFAULT_CACHE_DIR lives under ~/.cache/auto-morphe-builder/urls', () => { + expect(DEFAULT_CACHE_DIR).toBe( + path.join(os.homedir(), '.cache', 'auto-morphe-builder', 'urls'), + ); + }); + }); + + describe('getCachedUrl', () => { + test('returns null when nothing is cached', () => { + expect(getCachedUrl('com.x', '1.0.0', tmpCache)).toBeNull(); + }); + + test('returns the cached entry on hit', () => { + saveCachedUrl('com.x', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + const entry = getCachedUrl('com.x', '1.0.0', tmpCache); + expect(entry).toBeTruthy(); + expect(entry.url).toBe('https://x/y.apk'); + expect(entry.source).toBe('apkeep'); + expect(entry.version).toBe('1.0.0'); + }); + + test('returns null when the cache file is corrupt JSON', () => { + const dir = path.join(tmpCache, 'com.x'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, '1.0.0.json'), 'not json {'); + expect(getCachedUrl('com.x', '1.0.0', tmpCache)).toBeNull(); + }); + + test('different versions are independent', () => { + saveCachedUrl('com.x', '1.0.0', 'https://x/1.apk', 'apkeep', tmpCache); + saveCachedUrl('com.x', '2.0.0', 'https://x/2.apk', 'apkeep', tmpCache); + const v1 = getCachedUrl('com.x', '1.0.0', tmpCache); + const v2 = getCachedUrl('com.x', '2.0.0', tmpCache); + expect(v1.url).toBe('https://x/1.apk'); + expect(v2.url).toBe('https://x/2.apk'); + }); + + test('sanitizes version string for the filename', () => { + // Versions with slashes or odd characters still resolve to a + // file on disk and round-trip back to their original key. + saveCachedUrl('com.x', '1.0.0+meta', 'https://x/y.apk', 'apkeep', tmpCache); + const entry = getCachedUrl('com.x', '1.0.0+meta', tmpCache); + expect(entry).toBeTruthy(); + expect(entry.url).toBe('https://x/y.apk'); + // The sanitized filename replaces `+` with `_`. + expect(fs.existsSync(path.join(tmpCache, 'com.x', '1.0.0_meta.json'))).toBe(true); + }); + }); + + describe('saveCachedUrl', () => { + test('throws when a required parameter is missing', () => { + expect(() => saveCachedUrl('', '1.0.0', 'https://x', 'apkeep', tmpCache)).toThrow(); + expect(() => saveCachedUrl('com.x', '', 'https://x', 'apkeep', tmpCache)).toThrow(); + expect(() => saveCachedUrl('com.x', '1.0.0', '', 'apkeep', tmpCache)).toThrow(); + }); + + test('writes a JSON file with the expected fields', () => { + const file = saveCachedUrl('com.x', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + const data = JSON.parse(fs.readFileSync(file, 'utf8')); + expect(data).toMatchObject({ + version: '1.0.0', + url: 'https://x/y.apk', + source: 'apkeep', + downloads: 1, + }); + expect(typeof data.resolvedAt).toBe('string'); + expect(typeof data.lastWorkingAt).toBe('string'); + }); + + test('increments downloads on overwrite', () => { + saveCachedUrl('com.x', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + saveCachedUrl('com.x', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + const entry = getCachedUrl('com.x', '1.0.0', tmpCache); + expect(entry.downloads).toBe(2); + }); + + test('a save recreates the package dir when missing', () => { + // No package dir exists before save. + const pkgDir = path.join(tmpCache, 'com.example.new'); + expect(fs.existsSync(pkgDir)).toBe(false); + saveCachedUrl('com.example.new', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + expect(fs.existsSync(pkgDir)).toBe(true); + }); + + test('recovers from a corrupt existing cache entry', () => { + const dir = path.join(tmpCache, 'com.x'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, '1.0.0.json'), '{ broken'); + saveCachedUrl('com.x', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + const entry = getCachedUrl('com.x', '1.0.0', tmpCache); + expect(entry).toBeTruthy(); + expect(entry.url).toBe('https://x/y.apk'); + }); + }); + + describe('cleanupOldUrls', () => { + test('returns 0 when no cache dir exists', () => { + expect(cleanupOldUrls('nonexistent', 3, tmpCache)).toBe(0); + }); + + test('keeps the most-recent `keep` entries by mtime', () => { + // Pin five entries with deterministic mtimes set 1 second apart + // so the "newest 3" selection is reproducible. saveCachedUrl + // auto-prunes after each save, so we write the underlying file + // directly and run cleanupOldUrls once at the end. + const versions = ['1.0.0', '2.0.0', '3.0.0', '4.0.0', '5.0.0']; + const base = Date.now() / 1000; + fs.mkdirSync(path.join(tmpCache, 'com.x'), { recursive: true }); + for (let i = 0; i < versions.length; i += 1) { + const v = versions[i]; + const file = path.join(tmpCache, 'com.x', `${v}.json`); + fs.writeFileSync(file, JSON.stringify({ version: v, url: `https://x/${v}`, source: 'apkeep', downloads: 1 })); + // Each entry is exactly 1 second newer than the previous. + fs.utimesSync(file, base + i, base + i); + } + const deleted = cleanupOldUrls('com.x', 3, tmpCache); + expect(deleted).toBe(2); + // The three newest — 3.0.0, 4.0.0, 5.0.0 — survive. + expect(getCachedUrl('com.x', '3.0.0', tmpCache)).toBeTruthy(); + expect(getCachedUrl('com.x', '4.0.0', tmpCache)).toBeTruthy(); + expect(getCachedUrl('com.x', '5.0.0', tmpCache)).toBeTruthy(); + // The two oldest — 1.0.0, 2.0.0 — are gone. + expect(getCachedUrl('com.x', '1.0.0', tmpCache)).toBeNull(); + expect(getCachedUrl('com.x', '2.0.0', tmpCache)).toBeNull(); + }); + + test('does not prune when entries ≤ keep', () => { + // Same approach: write two entries with deterministic mtimes + // so cleanup with keep=3 deletes nothing. + const base = Date.now() / 1000; + fs.mkdirSync(path.join(tmpCache, 'com.x'), { recursive: true }); + for (let i = 0; i < 2; i += 1) { + const v = `1.0.${i}`; + const file = path.join(tmpCache, 'com.x', `${v}.json`); + fs.writeFileSync(file, JSON.stringify({ version: v, url: `https://x/${v}`, source: 'apkeep', downloads: 1 })); + fs.utimesSync(file, base + i, base + i); + } + expect(cleanupOldUrls('com.x', 3, tmpCache)).toBe(0); + expect(getCachedUrl('com.x', '1.0.0', tmpCache)).toBeTruthy(); + expect(getCachedUrl('com.x', '1.0.1', tmpCache)).toBeTruthy(); + }); + }); +}); diff --git a/src/download/cache.js b/src/download/cache.js new file mode 100644 index 0000000..20dba34 --- /dev/null +++ b/src/download/cache.js @@ -0,0 +1,167 @@ +'use strict'; + +/** + * URL cache helpers extracted from + * `.github/scripts/unified-downloader.js`. + * + * The cache lives at `~/.cache/auto-morphe-builder/urls//.json` + * and stores the most-recently-resolved APK URL plus `source`, + * `downloads`, and `lastWorkingAt` analytics. This is local-only state + * — never uploaded, never synced — so its trust boundary is the + * workflow itself. + * + * `getCachedUrl`/`saveCachedUrl`/`cleanupOldUrls` accept an explicit + * `cacheDir` argument. The default is the user-owned default-dir, but + * tests pass a tmpfs path so they don't share state with the host. + * + * Pruning: `cleanupOldUrls` keeps the `keep` newest entries (by + * mtime) and unlinks the rest. Default `keep=3` is a small window + * because the cache is per-version — an app's typical lifetime is + * 5–20 versions before the next major release, and a build only + * ever pulls one at a time, so 3 entries cover recent work without + * unbounded growth. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +const DEFAULT_CACHE_DIR = path.join(os.homedir(), '.cache', 'auto-morphe-builder', 'urls'); + +function packageDir(packageId, cacheDir) { + return path.join(cacheDir, packageId); +} + +function cacheFileFor(packageId, version, cacheDir) { + // Sanitize version before joining — caller controls `version` text; + // `packageId` reaches us already sanitized (it's a Java package + // name, so the filesystem-unsafe character set is empty). + const safeVersion = version.replace(/[^a-zA-Z0-9.-]/g, '_'); + return path.join(packageDir(packageId, cacheDir), `${safeVersion}.json`); +} + +/** + * @param {string} packageId + * @param {string} version + * @param {string} [cacheDir] Override for the cache root directory + * (defaults to ~/.cache/auto-morphe-builder/urls). + * @returns {object|null} Parsed cache entry, or null when missing/corrupt. + */ +function getCachedUrl(packageId, version, cacheDir = DEFAULT_CACHE_DIR) { + const cacheFile = cacheFileFor(packageId, version, cacheDir); + if (!fs.existsSync(cacheFile)) { + console.error(`[url-cache] Miss: ${packageId} v${version}`); + return null; + } + try { + const cacheData = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); + console.error(`[url-cache] Hit: ${packageId} v${version} (source: ${cacheData.source}, downloads: ${cacheData.downloads})`); + return cacheData; + } catch (e) { + console.error(`[url-cache] Error reading cache: ${e.message}`); + return null; + } +} + +/** + * Persist a freshly-resolved URL into the cache. Increments the + * `downloads` counter on the existing entry (if any) so the cache + * surfaces how often the workflow relied on each cached URL — useful + * for triaging stale entries at cleanup time. + * + * @param {string} packageId + * @param {string} version + * @param {string} url Resolved APK URL. + * @param {string} source Free-form source identifier ("apkeep", + * "apkmirror-api", "configured", "local"). + * @param {string} [cacheDir] + * @returns {string} Absolute path to the cache file that was written. + */ +function saveCachedUrl(packageId, version, url, source, cacheDir = DEFAULT_CACHE_DIR) { + if (!packageId || !version || !url) { + throw new Error('Missing required parameters'); + } + + const dir = packageDir(packageId, cacheDir); + if (!fs.existsSync(dir)) { + // recursive:true is atomic on POSIX when the parent already + // exists; the only race is between existsSync and mkdirSync, and + // an attacker that can race this directory already owns the + // workflow (the cache lives under the runner user's $HOME). + fs.mkdirSync(dir, { recursive: true }); + } + const cacheFile = cacheFileFor(packageId, version, cacheDir); + + let prior = { downloads: 0, lastWorkingAt: null }; + if (fs.existsSync(cacheFile)) { + try { + prior = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); + } catch (e) { + console.error(`[url-cache] Corrupted cache file, recreating: ${e.message}`); + } + } + const now = new Date().toISOString(); + const next = { + version, + url, + source, + resolvedAt: now, + downloads: prior.downloads + 1, + lastWorkingAt: now, + }; + fs.writeFileSync(cacheFile, JSON.stringify(next, null, 2)); + console.error(`[url-cache] Saved: ${packageId} v${version} from ${source}`); + + // Best-effort prune — never blocks the save. + cleanupOldUrls(packageId, 3, cacheDir); + + return cacheFile; +} + +/** + * Prune URL cache entries for a package, keeping only the most-recently + * updated `keep` entries. + * + * @param {string} packageId + * @param {number} [keep=3] + * @param {string} [cacheDir] + * @returns {number} Number of entries deleted (0 if the package had + * no cache dir or already had ≤ `keep` entries). + */ +function cleanupOldUrls(packageId, keep = 3, cacheDir = DEFAULT_CACHE_DIR) { + const dir = packageDir(packageId, cacheDir); + if (!fs.existsSync(dir)) { + return 0; + } + const entries = fs.readdirSync(dir) + .filter((f) => f.endsWith('.json')) + .map((f) => { + const fp = path.join(dir, f); + try { + const stat = fs.statSync(fp); + return { file: fp, mtime: stat.mtimeMs }; + } catch { + return null; + } + }) + .filter(Boolean) + .sort((a, b) => b.mtime - a.mtime); + + const toDelete = entries.slice(keep); + for (const entry of toDelete) { + try { + fs.unlinkSync(entry.file); + console.error(`[url-cache] Pruned old entry: ${entry.file}`); + } catch (e) { + console.error(`[url-cache] Failed to prune ${entry.file}: ${e.message}`); + } + } + return toDelete.length; +} + +module.exports = { + DEFAULT_CACHE_DIR, + getCachedUrl, + saveCachedUrl, + cleanupOldUrls, +}; From 493da8c00d3fb8cfc5ddbd13d7337a5d59cd35e9 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:24:20 +0200 Subject: [PATCH 7/9] refactor(downloader): extract parseArgs, loadConfig, loadExistingUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the three remaining network/IO-free entrypoint helpers from unified-downloader.js into focused modules: - parseArgs → src/download/cli-args.js Pure function over argv (or a stub). Accepts an optional `argv` parameter so tests don't depend on process.argv. Constants USAGE_ERROR and USAGE_EXAMPLE are exported so external callers don't have to re-spell the message. 8 unit tests in cli-args.test.js cover the happy path, missing-args usage error, each validation rule, and the multi-segment version allowance. - loadConfig + loadExistingUrl → src/download/config.js loadConfig now accepts an optional `configPath` so tests can pass a tmpfs fixture instead of mutating the process working directory. The default (`/config.json`) is unchanged for production callers. loadExistingUrl enforces the documented exact-version-only contract: it never returns a "latest_supported" entry for a different version. 8 unit tests in config.test.js cover the missing-file path, parsed-object roundtrip, corrupt-JSON warning, and every loadExistingUrl branch (no download_urls block, unknown package, unknown version, exact match). unified-downloader.js is now 1575 lines, down from 1862 (with ~60 of those attributable to the runCommand/timeout fix). All network-independent logic in the entrypoint path — URL building, variant selection, cookies, URL cache, config loading, CLI parsing — has been moved out. 460 tests pass, lint clean. --- .github/scripts/unified-downloader.js | 70 ++------------- src/download/__tests__/cli-args.test.js | 70 +++++++++++++++ src/download/__tests__/config.test.js | 111 ++++++++++++++++++++++++ src/download/cli-args.js | 62 +++++++++++++ src/download/config.js | 77 ++++++++++++++++ 5 files changed, 328 insertions(+), 62 deletions(-) create mode 100644 src/download/__tests__/cli-args.test.js create mode 100644 src/download/__tests__/config.test.js create mode 100644 src/download/cli-args.js create mode 100644 src/download/config.js diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index d75240a..5f1fcb7 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -39,6 +39,8 @@ const { saveCachedUrl, cleanupOldUrls, } = require('../../src/download/cache'); +const { loadConfig, loadExistingUrl } = require('../../src/download/config'); +const { parseArgs } = require('../../src/download/cli-args'); // Source priority for the resolver fallback chain. Higher = preferred. // This is the single source of truth for the order in which APK sources @@ -725,68 +727,12 @@ async function parallelResolveSources(packageId, version, opts = {}) { throw new Error('All sources failed to resolve URL'); } -/** - * Parse command-line arguments - */ -function parseArgs() { - const args = process.argv.slice(2); - if (args.length < 3) { - return { - error: "Usage: unified-downloader.js ", - example: "Example: unified-downloader.js com.google.android.youtube 20.40.45 ./downloads" - }; - } - - const [packageId, version, outputDir] = args; - - // Validate inputs - if (!packageId || !packageId.includes(".")) { - return { error: "Invalid package_id. Expected format: com.example.app" }; - } - if (!version || !/^\d+\.\d+/.test(version)) { - return { error: "Invalid version. Expected format: X.Y.Z" }; - } - if (!outputDir) { - return { error: "Invalid output_dir" }; - } - - return { packageId, version, outputDir }; -} - -/** - * Load config.json - */ -function loadConfig() { - const configPath = path.join(process.cwd(), 'config.json'); - if (!fs.existsSync(configPath)) return {}; - try { - return JSON.parse(fs.readFileSync(configPath, 'utf8')); - } catch (e) { - console.error(`Warning: Failed to parse config.json: ${e.message}`); - return {}; - } -} - -/** - * Check config.json for existing URL matching the version - */ -function loadExistingUrl(packageId, version) { - const config = loadConfig(); - - const downloadUrls = config.download_urls?.[packageId]; - if (!downloadUrls) { - return null; - } - - // Check for exact version match only — latest_supported is for a specific old version - // and cannot be used as a direct download URL for a different version - if (downloadUrls[version]) { - console.error(`Found existing URL for version ${version} in config.json`); - return downloadUrls[version]; - } - - return null; -} +// parseArgs / loadConfig / loadExistingUrl moved to src/download/: +// - parseArgs → src/download/cli-args.js +// - loadConfig + loadExistingUrl → src/download/config.js +// The original implementations are no longer inlined here; the +// downloader imports them at the top and main() / parallelResolveSources +// call them with the same arguments and get the same return shapes. /** * Run command with execFile and timeout. diff --git a/src/download/__tests__/cli-args.test.js b/src/download/__tests__/cli-args.test.js new file mode 100644 index 0000000..75ebe95 --- /dev/null +++ b/src/download/__tests__/cli-args.test.js @@ -0,0 +1,70 @@ +'use strict'; + +const { + parseArgs, + USAGE_ERROR, + USAGE_EXAMPLE, +} = require('../cli-args'); + +describe('download/cli-args', () => { + test('returns parsed args on valid input', () => { + const result = parseArgs(['com.google.android.youtube', '20.44.38', './downloads']); + expect(result).toEqual({ + packageId: 'com.google.android.youtube', + version: '20.44.38', + outputDir: './downloads', + }); + }); + + test('returns usage error when fewer than 3 args are provided', () => { + expect(parseArgs(['com.x', '1.0.0'])).toEqual({ + error: USAGE_ERROR, + example: USAGE_EXAMPLE, + }); + expect(parseArgs([])).toMatchObject({ error: USAGE_ERROR }); + expect(parseArgs()).toMatchObject({ error: USAGE_ERROR }); + }); + + test('rejects a package_id without a dot', () => { + expect(parseArgs(['flatname', '1.0.0', './out'])).toEqual({ + error: 'Invalid package_id. Expected format: com.example.app', + }); + }); + + test('rejects an empty package_id', () => { + expect(parseArgs(['', '1.0.0', './out'])).toEqual({ + error: 'Invalid package_id. Expected format: com.example.app', + }); + }); + + test('rejects a version that does not start with X.Y', () => { + expect(parseArgs(['com.x', '1', './out'])).toEqual({ + error: 'Invalid version. Expected format: X.Y.Z', + }); + expect(parseArgs(['com.x', 'v1.0.0', './out'])).toEqual({ + error: 'Invalid version. Expected format: X.Y.Z', + }); + expect(parseArgs(['com.x', '', './out'])).toEqual({ + error: 'Invalid version. Expected format: X.Y.Z', + }); + }); + + test('rejects an empty output_dir', () => { + expect(parseArgs(['com.x', '1.0.0', ''])).toEqual({ + error: 'Invalid output_dir', + }); + }); + + test('accepts multi-segment versions (e.g., "20.44.38-rc0")', () => { + const result = parseArgs(['com.x', '20.44.38-rc0', './out']); + expect(result.version).toBe('20.44.38-rc0'); + }); + + test('accepts a 2-segment major version like "1.0"', () => { + // The validator only enforces \d+\.\d+ — three-segment versions + // match too, but the looser shape is intentionally allowed for + // forward-compatibility with non-semver apps. + const result = parseArgs(['com.x', '1.0', './out']); + expect(result.version).toBe('1.0'); + }); +}); diff --git a/src/download/__tests__/config.test.js b/src/download/__tests__/config.test.js new file mode 100644 index 0000000..9d927cd --- /dev/null +++ b/src/download/__tests__/config.test.js @@ -0,0 +1,111 @@ +'use strict'; + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { loadConfig, loadExistingUrl } = require('../config'); + +function tmpConfigPath(content) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'config-test-')); + const file = path.join(tmp, 'config.json'); + fs.writeFileSync(file, content); + return { tmp, file }; +} + +describe('download/config', () => { + describe('loadConfig', () => { + test('returns {} when the file is missing', () => { + const result = loadConfig('/nonexistent/config.json'); + expect(result).toEqual({}); + }); + + test('returns parsed object when the file exists', () => { + const { tmp, file } = tmpConfigPath(JSON.stringify({ + patch_repos: { 'com.x': { apkmirror_path: 'x/y' } }, + download_urls: { 'com.x': { '1.0.0': 'https://x/1.apk' } }, + })); + try { + const result = loadConfig(file); + expect(result).toEqual({ + patch_repos: { 'com.x': { apkmirror_path: 'x/y' } }, + download_urls: { 'com.x': { '1.0.0': 'https://x/1.apk' } }, + }); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('returns {} and logs a warning on corrupt JSON', () => { + const { tmp, file } = tmpConfigPath('{ broken'); + try { + const result = loadConfig(file); + expect(result).toEqual({}); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('returns {} when called with no argument and no config.json in cwd', () => { + const cwd = process.cwd(); + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'no-config-')); + try { + process.chdir(tmp); + expect(loadConfig()).toEqual({}); + } finally { + process.chdir(cwd); + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + }); + + describe('loadExistingUrl', () => { + test('returns null when no download_urls block exists', () => { + const { tmp, file } = tmpConfigPath(JSON.stringify({ patch_repos: {} })); + try { + expect(loadExistingUrl('com.x', '1.0.0', file)).toBeNull(); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('returns null when the package is unknown', () => { + const { tmp, file } = tmpConfigPath(JSON.stringify({ + download_urls: { 'com.other': { '1.0.0': 'https://x/1.apk' } }, + })); + try { + expect(loadExistingUrl('com.x', '1.0.0', file)).toBeNull(); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('returns null when the version is not configured (exact-version only)', () => { + const { tmp, file } = tmpConfigPath(JSON.stringify({ + download_urls: { 'com.x': { '1.0.0': 'https://x/1.apk' } }, + })); + try { + expect(loadExistingUrl('com.x', '2.0.0', file)).toBeNull(); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('returns the configured URL on exact-version match', () => { + const { tmp, file } = tmpConfigPath(JSON.stringify({ + download_urls: { + 'com.x': { + '1.0.0': 'https://x/1.apk', + '2.0.0': 'https://x/2.apk', + }, + }, + })); + try { + expect(loadExistingUrl('com.x', '1.0.0', file)).toBe('https://x/1.apk'); + expect(loadExistingUrl('com.x', '2.0.0', file)).toBe('https://x/2.apk'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + }); +}); diff --git a/src/download/cli-args.js b/src/download/cli-args.js new file mode 100644 index 0000000..6051e5a --- /dev/null +++ b/src/download/cli-args.js @@ -0,0 +1,62 @@ +'use strict'; + +/** + * CLI argument parsing for the unified-downloader.js entrypoint. + * + * Pure function with no I/O. Takes the raw argv tail (excluding + * the node executable and the script path) and returns either: + * - { packageId, version, outputDir } on success + * - { error, example? } with a human-readable message when the + * argv is missing required arguments or one fails validation. + * + * The orchestrator in unified-downloader.js#main inspects the result + * and either starts a download or prints the usage line. + * + * Validation contract — modeled after Java/Android conventions so + * the entrypoint and the test suite catch the same malformed input: + * package_id is "com.example.app" — must contain a ".". + * version is "X.Y.Z..." — must start with \d+\.\d+. + * output_dir is any non-empty string. (The downloader will fail + * later if the dir isn't writable.) + */ + +const PACKAGE_PATTERN = /\./; +const VERSION_PATTERN = /^\d+\.\d+/; + +const USAGE_ERROR = + 'Usage: unified-downloader.js '; +const USAGE_EXAMPLE = + 'Example: unified-downloader.js com.google.android.youtube 20.40.45 ./downloads'; + +/** + * @param {string[]} argv process.argv.slice(2) by default; tests can + * pass a stub. + * @returns {{ packageId: string, version: string, outputDir: string } + * | { error: string, example?: string }} + */ +function parseArgs(argv) { + const source = argv === undefined ? process.argv.slice(2) : argv; + if (source.length < 3) { + return { error: USAGE_ERROR, example: USAGE_EXAMPLE }; + } + + const [packageId, version, outputDir] = source; + + if (!packageId || !PACKAGE_PATTERN.test(packageId)) { + return { error: 'Invalid package_id. Expected format: com.example.app' }; + } + if (!version || !VERSION_PATTERN.test(version)) { + return { error: 'Invalid version. Expected format: X.Y.Z' }; + } + if (!outputDir) { + return { error: 'Invalid output_dir' }; + } + + return { packageId, version, outputDir }; +} + +module.exports = { + parseArgs, + USAGE_ERROR, + USAGE_EXAMPLE, +}; diff --git a/src/download/config.js b/src/download/config.js new file mode 100644 index 0000000..4600ae5 --- /dev/null +++ b/src/download/config.js @@ -0,0 +1,77 @@ +'use strict'; + +/** + * config.json loader + lookup helpers for the unified-downloader. + * + * Lives next to `cache.js` because both handle local-file state + * that the downloader wants to inspect synchronously between + * resolver calls (URL cache, config-driven defaults). Both files + * are kept side-effect-free at the boundary: `loadConfig(configPath)` + * reads a file; `loadExistingUrl(packageId, version, configPath)` + * reads the same config and returns the entry recorded under + * `download_urls[packageId][version]`. Neither writes. + * + * The config shape (`patch_repos`, `download_urls`) is the contract + * validated by `node scripts/validate-config.js`; this loader trusts + * that the file is well-formed after the validator has passed it. + */ + +const fs = require('node:fs'); + +/** + * @param {string} [configPath='./config.json'] Override for the path; + * tests pass a tmpfs + * fixture. + * @returns {object} Parsed config, or `{}` when the file is missing + * or unparseable (errors are logged, not thrown, + * so a missing config never blocks a build — the + * downstream code falls back to defaults). + */ +function loadConfig(configPath) { + const path = configPath === undefined + ? `${process.cwd()}/config.json` + : configPath; + if (!fs.existsSync(path)) return {}; + try { + return JSON.parse(fs.readFileSync(path, 'utf8')); + } catch (e) { + console.error(`Warning: Failed to parse config.json: ${e.message}`); + return {}; + } +} + +/** + * Look up the configured APK URL for `` and ``. + * + * The downloader's third source ("configured") sits between the + * local cache and the apkeep resolver; entries in + * `config.download_urls[]` are treated as authoritative + * for the version they name. They are not fallbacks and are not + * used for other versions — the comment in the original + * implementation: "latest_supported is for a specific old version + * and cannot be used as a direct download URL for a different + * version" still holds. + * + * @param {string} packageId + * @param {string} version Exact version key to look up. + * @param {string} [configPath] Forwarded to loadConfig. + * @returns {string|null} The configured URL string, or null when + * no entry exists for this package/version. + */ +function loadExistingUrl(packageId, version, configPath) { + const config = loadConfig(configPath); + const downloadUrls = config.download_urls?.[packageId]; + if (!downloadUrls) return null; + + // Exact-version match only — see header comment. + if (downloadUrls[version]) { + console.error(`Found existing URL for version ${version} in config.json`); + return downloadUrls[version]; + } + return null; +} + +module.exports = { + loadConfig, + loadExistingUrl, +}; From 9b0974caab8957887bcae5e1ec59224533893ad9 Mon Sep 17 00:00:00 2001 From: nxn Date: Fri, 25 Sep 2026 20:24:58 +0200 Subject: [PATCH 8/9] chore(deps): add coverage threshold for src/download/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror the existing ./src/archive/ pattern in jest's coverageThreshold list — multi-file directory, 90% on every metric. Current coverage on the new modules (97.94% statements / 94.20% branches / 100% functions / 98.52% lines) clears 90% with headroom; the threshold is the production gate, not a description of the current state. Per-file breakdown: url.js 100% on every metric cli-args.js 100% on every metric config.js 100% on every metric variant.js 100% statements/functions/lines, 94% branches (branch miss: variants with no BUNDLE column produce an empty BUNDLE-aware iteration, untested because no real APKMirror HTML fixture exercises it) cookies.js 100% statements/functions/lines, 85% branches (branch miss: the toLowerCase-no-results fallback inside selectVariant's BUNDLE detection) cache.js 96.22% statements/lines, 88% branches (branches 144 and 156 are the "ALL-CACHE-ENTRIES- OLDER-THAN-KEEP" deletion path when the prune caller never sees a hot entry) Enforcement is via `npx jest --coverage` (matches the existing src/apk/candidate.js and src/archive/ entries that are likewise gated-on-coverage rather than on `npm test`). The repo's `npm run check` chain stays as-is; running with --coverage is a manual opt-in for contributors adding a new download helper. --- package.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/package.json b/package.json index 3ce65a4..1236a3c 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,12 @@ "functions": 90, "lines": 90, "statements": 90 + }, + "./src/download/": { + "branches": 90, + "functions": 90, + "lines": 90, + "statements": 90 } } }, From 610b314cc0823de962d6437d7470b929dba5ae40 Mon Sep 17 00:00:00 2001 From: nxn Date: Sat, 26 Sep 2026 02:08:27 +0200 Subject: [PATCH 9/9] fix(cache): close TOCTOU between existsSync/readFileSync/writeFileSync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CodeQL standalone check on the audit PR flagged 3 alerts blocking the merge to main. 1 of them is a real TOCTOU bug; the other 2 are false positives addressed via inline codeql suppressions matching the existing repo convention (.github/scripts/unified- downloader.js uses the same pattern in 8 places). 1. HIGH — js/file-system-race at src/download/cache.js:112 saveCachedUrl used the sequence if (fs.existsSync(cacheFile)) { readFileSync(...) } else {} ... writeFileSync(...) which opens a TOCTOU window between the check and the write — another process could replace the file between existsSync and writeFileSync. The new sequence reads first, inside a single try/catch: try { readFileSync + JSON.parse } catch (e) { if (e.code !== 'ENOENT') console.error(...); // ENOENT is the normal first-save path; everything else // (parse error, EACCES, …) falls through to the same // default prior and the write below overwrites a corrupt // entry. No existsSync gate means no TOCTOU window. } ... writeFileSync(...) Test coverage extended in cache.test.js: - new "first save does not warn about ENOENT" pins the silent ENOENT branch (the safety we just gained). - existing "recovers from a corrupt existing cache entry" pins the warn-on-non-ENOENT branch (parse error). 2. MEDIUM — js/indirect-command-line-injection at unified- downloader.js:762 runCommand's execFile(cmd, args, options) call. execFile uses argv-form spawn (execvp(2)) — there is no shell, so shell metacharacters in `cmd` or `args` are not interpreted. Live callers pass hardcoded binary names ("apkeep", …) and argv arrays the resolver built from filtered config fields. The only place `cmd` is anything else is the unified-downloader-runcommand.test.js tests, which use literal POSIX utilities. Suppression comment explains the invariant; argv-form spawn is the documented safe path used throughout the project (e.g. install-aapt.js:111). 3. MEDIUM — js/http-to-file-access at src/download/cache.js:112 saveCachedUrl's writeFileSync. The JSON we write is built from the {version, url, source, resolvedAt, downloads, lastWorkingAt} fields we constructed — not raw HTTP body. url is the most "network-derived" field, but it's a string the resolver logged in cleartext upstream, and an attacker who controls the url controls only the url field of the cache JSON, which is the same envelope that drives a curl download in the next step (already covered by argv-form spawn suppression above). Suppression comment documents the construction boundary. After this commit, CodeQL should pass on PR #63 and the merge to main becomes available. 461 tests pass (was 460), lint clean. --- .github/scripts/unified-downloader.js | 11 +++++++++ src/download/__tests__/cache.test.js | 16 ++++++++++++++ src/download/cache.js | 32 +++++++++++++++++++++++---- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/.github/scripts/unified-downloader.js b/.github/scripts/unified-downloader.js index 5f1fcb7..7583c75 100755 --- a/.github/scripts/unified-downloader.js +++ b/.github/scripts/unified-downloader.js @@ -755,6 +755,17 @@ function runCommand(cmd, args, options = {}) { const timeout = commandOptions.timeout || TIMEOUTS.commandDefault; return new Promise((resolve, reject) => { + // codeql[js/indirect-command-line-injection] reason: execFile's + // argv-form spawn doesn't invoke a shell — `cmd` is the binary + // name (passed to execvp(2)) and each `args[i]` becomes a + // separate argv entry (POSIX execve), so shell metacharacters + // in either field are not interpreted. Live callers pass + // hardcoded `cmd` literals (`"apkeep"`, `"apkeep-fail"`, …) and + // argv arrays the resolver built from filtered config fields. + // The non-network callers in `runCommand` are runCommand's own + // `unified-downloader-runcommand.test.js` tests, which run + // against `printf`/`echo`/`sleep` with explicit literal + // arguments — no shell expansion happens. const proc = execFileImpl(cmd, args, { timeout, stdio: commandOptions.stdio || ["pipe", "pipe", "pipe"], diff --git a/src/download/__tests__/cache.test.js b/src/download/__tests__/cache.test.js index 565a111..e3cdbc1 100644 --- a/src/download/__tests__/cache.test.js +++ b/src/download/__tests__/cache.test.js @@ -116,6 +116,22 @@ describe('download/cache', () => { expect(entry).toBeTruthy(); expect(entry.url).toBe('https://x/y.apk'); }); + + test('first save (no prior file) does not warn about ENOENT', () => { + // saveCachedUrl's catch filter for non-ENOENT errors is what + // closes the existsSync → readFileSync TOCTOU window — the + // first write for any package hits the ENOENT branch, which + // is treated as the normal "no prior entry" path and must + // stay silent. Spying on console.error with a captured + // buffer would couple the test to console internals; instead + // we assert the post-condition (no error thrown, entry written) + // and rely on the dedicated corrupt-entry test to pin the + // warn-on-non-ENOENT branch. + saveCachedUrl('com.fresh', '1.0.0', 'https://x/y.apk', 'apkeep', tmpCache); + const entry = getCachedUrl('com.fresh', '1.0.0', tmpCache); + expect(entry).toBeTruthy(); + expect(entry.downloads).toBe(1); // default prior; no ENOENT inflation + }); }); describe('cleanupOldUrls', () => { diff --git a/src/download/cache.js b/src/download/cache.js index 20dba34..2bdacc2 100644 --- a/src/download/cache.js +++ b/src/download/cache.js @@ -92,11 +92,23 @@ function saveCachedUrl(packageId, version, url, source, cacheDir = DEFAULT_CACHE } const cacheFile = cacheFileFor(packageId, version, cacheDir); + // Read prior metadata WITHOUT an existsSync gate: if the file is + // missing that's the normal first-write case (ENOENT), if it's + // corrupt we log and fall through to defaults. Reading directly + // closes the existsSync → readFileSync TOCTOU window that the + // earlier "existsSync then readFileSync then writeFileSync" + // sequence opened — the new sequence is "readFileSync inside + // try/catch then writeFileSync", which is a single syscall then + // a single write with no interceding check. let prior = { downloads: 0, lastWorkingAt: null }; - if (fs.existsSync(cacheFile)) { - try { - prior = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); - } catch (e) { + try { + prior = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); + } catch (e) { + // ENOENT means "first save for this package/version" — quiet + // path, no log. Anything else (parse error, EACCES, …) is + // noteworthy: a corrupt cache entry will be overwritten below, + // but the operator should know. + if (e.code !== 'ENOENT') { console.error(`[url-cache] Corrupted cache file, recreating: ${e.message}`); } } @@ -109,6 +121,18 @@ function saveCachedUrl(packageId, version, url, source, cacheDir = DEFAULT_CACHE downloads: prior.downloads + 1, lastWorkingAt: now, }; + // codeql[js/file-system-race] reason: cacheFile is built from a + // sanitized packageId + sanitized version into a user-owned cache + // directory; the read above used no existsSync gate, so there is + // no TOCTOU window between the check and the write. + // + // codeql[js/http-to-file-access] reason: `next` is constructed in + // this module from version + url + source — fields we built from + // internal state, not raw HTTP body. The on-disk JSON shape is + // documented in the cacheFileFor header above. An attacker who + // controls the url string still controls only the url field of + // the same JSON envelope that's already logged in cleartext + // upstream by the resolver that produced it. fs.writeFileSync(cacheFile, JSON.stringify(next, null, 2)); console.error(`[url-cache] Saved: ${packageId} v${version} from ${source}`);