Conversation
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.
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.
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 <base64> 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).
…load/variant.js
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.
…ies.js
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.
Move getCachedUrl / saveCachedUrl / cleanupOldUrls into
src/download/cache.js. The cache lives at
~/.cache/auto-morphe-builder/urls/<packageId>/<version>.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.
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 (`<cwd>/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.
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| downloads: prior.downloads + 1, | ||
| lastWorkingAt: now, | ||
| }; | ||
| fs.writeFileSync(cacheFile, JSON.stringify(next, null, 2)); |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of change
fix:) — non-breaking change that restores intended behaviourfeat:) — non-breaking change that adds user-visible capabilityfeat:orfix:with!) — change that requires user actiondocs:) — no production-code changerefactor:) — no user-visible behaviour changechore:) — CI, dependencies, repo maintenancetest:) — test-only changesValidation
npm cisucceeds on a clean clonenpm testpasses (no skipped tests for new code)npm run lintpassesjq '.' config.json && jq '.' patches.jsonsucceedsactionlint .github/workflows/<file>.ymlshellcheck .github/scripts/pipeline/*.sh .github/scripts/pipeline/lib/*.shpython3 -c "import yaml; yaml.safe_load(open('.github/workflows/<file>.yml'))"docs/updated where the public surface changesSecurity review
KEYSTORE_BASE64,KEYSTORE_PASSWORD,KEY_PASSWORD,KEY_ALIAS, GitHub tokens, APKMirror creds)permissions:widening in any workflowpull_request_targettrigger and no new use of untrusted input in shell stepsactions/*versionspull_request_targetis not used; secrets are not exposed to fork PRsDocumentation
docs/configuration.md(ifconfig.jsonshape changes)docs/architecture.md(if the workflow graph changes)docs/release-process.md(if release tag format, pruning, or pinning changes)docs/troubleshooting.md(if a new failure mode is introduced)CONTRIBUTING.md(if commit message or PR conventions change)Additional notes