fix(backend-native): require @cubejs-infra/post-installer ^0.1.2 to drop decompress - #11580
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11580 +/- ##
==========================================
- Coverage 84.05% 79.52% -4.53%
==========================================
Files 257 484 +227
Lines 81582 99739 +18157
Branches 0 3672 +3672
==========================================
+ Hits 68576 79322 +10746
- Misses 13006 19869 +6863
- Partials 0 548 +548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
decompress ^4.2.1 carries a critical advisory (arbitrary file write via path
traversal) and reached every Cube install through a chain with exactly one
requirer at each hop:
cubejs-backend-native -> @cubejs-infra/post-installer@^0.0.7
-> @cubejs-backend/shared@0.33.20 -> decompress@^4.2.1
`^0.0.7` is a caret on a 0.0.x version, i.e. exactly 0.0.7, and shared was pinned
exactly too, so nothing could float past it. post-installer 0.1.2 repins shared
to a release that dropped decompress; `^0.1.2` spans >=0.1.2 <0.2.0 and can
reach it.
Lockfile regenerated against the published 0.1.2: decompress@^4.2.1 and
@cubejs-backend/shared@0.33.20 both gone (only decompress-response remains, an
unrelated got dependency). The new @octokit/* entries are expected —
post-installer gained github_artifact:// support via octokit in 0.1.0, which the
old ^0.0.7 pin predated.
Behaviour verified against this package's own resources config (plain host+path,
not github_artifact://): 0.0.7, 0.1.1 and the published 0.1.2 all exit 0 and
construct the identical native-<platform>-<arch>-<libc>-<libpython>.tar.gz URL.
The node >=20 floor post-installer now declares is already this package's own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c19a8c6 to
0b0e1b2
Compare
|
Claude finished @bsod90's task in 1m 24s —— View job Review: looks good to merge — 1 low-severity note, plus a title/diff mismatch to reconcileFull review
Verification against the checked-out head (
|
| Claim in the PR body | Result |
|---|---|
decompress@^4.2.1 gone from yarn.lock |
✅ only decompress-response@^6.0.0 remains (unrelated, got) |
@cubejs-backend/shared@0.33.20 gone |
✅ replaced by 1.7.20 (pulled by post-installer) |
post-installer resolves to 0.1.2 |
✅ yarn.lock:2799-2807 |
No other requirer of decompress in-repo |
✅ no package.json outside node_modules declares it |
Node >=20 floor is not a new constraint |
✅ cubejs-backend-native already declares it, and every other packages/* manifest surveyed declares >=20.0.0 uniformly |
The reasoning in the description holds up: a root resolutions override would fix only this lockfile and would not reach anyone installing @cubejs-backend/native, so bumping the declared dependency is the right lever. Nice write-up of the resolution chain, too — the ^0.0.7-is-really-an-exact-pin detail is the crux and it's stated correctly.
Findings
Version mismatch between title and diff (needs reconciling). The PR title and branch name say 0.2.0; the checked-out commit declares "@cubejs-infra/post-installer": "^0.1.2" and the lockfile resolves 0.1.2. The body consistently describes 0.1.2, so most likely the title is stale — but I could not fetch a newer head than 0b0e1b2 in this environment, so if a 0.2.0 bump commit was pushed after that, it isn't covered by this review. Flagged inline on packages/cubejs-backend-native/package.json:55.
Low — install-time surface widens slightly. ^0.0.7 was an exact pin by npm caret rules; ^0.1.2 is >=0.1.2 <0.2.0. Consumers installing without a lockfile will now float across future 0.1.x releases of a package that runs a postinstall binary downloader. Deliberate and defensible (it's what lets shared float forward), but ~0.1.2 would get the same advisory fix with a tighter range. Detail inline.
Informational — duplicate shared copy. post-installer 0.1.2 pins @cubejs-backend/shared at exactly 1.7.20 while this package depends on 1.7.21, so installs carry two copies. Not a regression (previously the second copy was 0.33.20), and it will resolve itself whenever post-installer repins — just noting it will drift by one patch on every Cube release.
Not reviewed
No test/documentation changes are expected or needed for a dependency bump; codecov confirms no coverage delta. I did not run an install to reproduce the lockfile — the verification above is static analysis of the committed yarn.lock.
| "@cubejs-backend/cubesql": "1.7.21", | ||
| "@cubejs-backend/shared": "1.7.21", | ||
| "@cubejs-infra/post-installer": "^0.0.7" | ||
| "@cubejs-infra/post-installer": "^0.1.2" |
There was a problem hiding this comment.
The declared range here is ^0.1.2, but the PR title and branch both say 0.2.0. Worth reconciling one or the other before merge — if a 0.2.0 of post-installer is what you actually intend to ship against, this line (and the lockfile) still point at the 0.1.x line.
Separate, minor note on the range semantics: ^0.0.7 was effectively an exact pin (npm's caret on 0.0.x allows nothing else), whereas ^0.1.2 resolves to >=0.1.2 <0.2.0. So downstream consumers installing @cubejs-backend/native fresh (no lockfile) will now float across future 0.1.x patch releases of a package whose whole job is to run a postinstall script that downloads and unpacks a binary. That's a deliberate and reasonable trade — it's exactly what lets shared float off decompress — but if you want to keep the install-time surface pinned, ~0.1.2 or an exact 0.1.2 gets the same security win without the float.
|
Converting back to draft — I found a problem with this change that the PR description above does not reflect. This trades a critical for a high rather than eliminating it
That is what Second, unexplained issue
So "my change broke it" and "v1.7.21 broke it" are both consistent with the evidence, and I am not going to assert either. Job logs are not retrievable from my environment, which is why this is unresolved rather than diagnosed. Suggested directionThe durable fix is in Leaving this in draft rather than deleting it: the range change here is still a prerequisite for any version of the fix, since |
|
Claude finished @bsod90's task in 2m 16s —— View job Re-review: the draft-blocking premise doesn't hold — this PR is a strict improvement, not a critical→high tradeFull review — 0 blocking, 1 correction to the draft rationale, 2 notes on the suggested follow-up
The correction:
|
| master | this PR | |
|---|---|---|
extract-zip@2.0.1 in yarn.lock |
✅ present (yarn.lock:13676) |
✅ present (yarn.lock:13634) — unchanged |
decompress@^4.2.1 |
present | gone |
direct dep on shared from backend-native |
1.7.21 |
1.7.21 (identical) |
So the extract-zip chain in npm audit reproduces on master with ^0.0.7 too — the audit just attributes it to post-installer → shared because that's one path among several, not the only one. security/snyk should be failing on master for the same reason. Net effect of this PR is critical removed, high unchanged, which is the outcome the original description claimed. The description was incomplete about extract-zip existing, but not wrong about what this PR does.
That removes the reason to hold this in draft. The extract-zip work is genuinely worth doing — but it's independent of this change and shouldn't block it.
Two notes on the suggested follow-up (dropping extract-zip from shared)
The zip branch probably does have a live caller — check before removing. In-repo callers of downloadAndExtractFile all take non-zip paths, so the hypothesis looks good at first glance:
| caller | archive |
|---|---|
packages/cubejs-backend-maven/src/maven.ts:89 |
.tar.gz |
packages/cubejs-databricks-jdbc-driver/src/installer.ts:16 |
skipExtract: true (bare .jar) |
packages/cubejs-testing/test/bin/download-dataset.ts:9 |
fixtures |
But extractArchive is a public export of @cubejs-backend/shared (packages/cubejs-backend-shared/src/index.ts:20), so in-repo callers aren't the whole caller set — and the relevant out-of-repo caller is the one this PR just upgraded to. post-installer 0.1.x's github_artifact:// support downloads GitHub Actions artifacts, and the artifact-download API serves those only as ZIP. Removing the zip branch would plausibly break exactly the feature that 0.1.0 added. Worth reading post-installer's source before assuming the branch is dead.
The advisory's exact attack is already covered by a passing test. packages/cubejs-backend-shared/test/http-utils.test.ts:219-234 is GHSA-jmr9-qjv8-65gv's scenario verbatim — a zip symlink entry pointing outside the target, followed by an entry written through it — and it asserts extract-zip rejects it:
await expect(extractArchive(archive, targetDir())).rejects.toThrow(/out of bound path/i);
expect(fs.existsSync(path.join(outside, 'PWNED.txt'))).toBe(false);with the Zip Slip case at :190 alongside it. Both run against the 2.0.1 that's in the lockfile. If those pass in CI, the advisory isn't reachable through this call site, and the stronger response to snyk is a documented exception citing these tests rather than churning the dependency. I could not run them here — this checkout has no node_modules — so treat that as "the test exists and asserts containment," not "I observed it green."
Build / unit (24.x, 3.13)
Can't attribute it either. The Build run for this head (32060741796) is still in progress as of this review, and no failed run is recorded on the PR right now. Your reasoning for not asserting a cause was the right call; re-running against current master is the cheapest way to separate "this change" from "v1.7.21".
Prior threads
Re-affirmed the existing unresolved thread on packages/cubejs-backend-native/package.json:55 — still applies as written: the line declares ^0.1.2 while the title and branch say 0.2.0, and the ^0.0.7→^0.1.2 range widening note is unchanged. Not re-posting it.
Removes the last
decompressrequirer from this repo — and because it changes the published dependency graph, from every Cube installation rather than just ours.decompress ^4.2.1carries a critical advisory (arbitrary file write via path traversal) and reached us through a chain with exactly one requirer at every hop:@cubejs-infra/post-installer@0.1.2(cube-js/post-installer#2, released) repinssharedto 1.7.20, which droppeddecompress.^0.1.2spans>=0.1.2 <0.2.0and can reach it — unlike the old^0.0.7, where the caret on a0.0.xversion pinned exactly one release.Lockfile effect
Regenerated against the published 0.1.2:
decompress@^4.2.1@cubejs-backend/shared@0.33.20@cubejs-infra/post-installerNet −152 lines. The only remaining
decompress*match isdecompress-response, an unrelatedgotdependency.The new
@octokit/*entries are expected, not incidental: post-installer gainedgithub_artifact://support via octokit in 0.1.0, which the old^0.0.7pin predated.Verified
resourcesconfig (plainhost+path, notgithub_artifact://): the published 0.1.2 exits 0 and constructs the identical URL to 0.0.7 —releases/download/v<version>/native-<platform>-<arch>-<libc>-<libpython>.tar.gz. Checked 0.0.7, 0.1.1 and 0.1.2 side by side.decompress,-tar,-tarbz2,-targz,-unzip); 0.1.2 pulls 0.packages/cubejs-backend-native/package.jsonalready declares"engines": {"node": ">=20.0.0"}, which is what post-installer 0.1.2 now requires.Why not a
resolutionsoverrideAn override in this repo's root would fix only this lockfile; it does not propagate to anyone installing
@cubejs-backend/native, who would still resolvepost-installer@0.0.7 → shared@0.33.20 → decompress. Changing the declared dependency is the only thing that reaches downstream users.