[pull] master from cube-js:master - #661
Merged
Merged
Conversation
A measure multiplied by a fan-out join is read through the keys subquery: a DISTINCT grid of the query dimensions plus the primary key of the measure's cube, re-joined to that cube by the key so the measure aggregates once per row. The primary key was projected unconditionally, next to the query dimensions, so a query that asks for that key as a dimension got two columns under one alias and the re-join's reference to it became ambiguous — Postgres rejects the query with `column reference "..." is ambiguous`. Project the key once when it plays both roles. The re-join still resolves to the surviving column: it looks the column up by member symbol, not by alias, so a key projected under a dimension's alias is found too. Covered by a planner integration test against Postgres and a schema-compiler test on the reported model, green under both planners. Fixes #11455 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… logos and favicon (#11537) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…11532) * fix(shared,templates): replace the unmaintained decompress package `decompress@4.2.1` is the newest release and carries two archive-extraction advisories with **no patched version**: - GHSA-mp2f-45pm-3cg9 (CRITICAL) — extraction can create files and links outside of the target directory - GHSA-h39j-r5qq-r9mm — Arbitrary File Write via Archive Extraction (Zip Slip) The package is dead, so there is nothing to bump to. `@xhmikosr/decompress` (the maintained fork) is not usable here: every version is ESM-only, while both consumers compile to CommonJS via `tsconfig.base.json` and declare `node >=20.0.0`, and Node 20 cannot `require()` ESM. Replaced with `tar` (already used throughout this repo) plus `extract-zip` for zip, both CommonJS and both advisory-clean. **`cubejs-templates`** was trivial: `PackageFetcher` passed `decompressTargz()` explicitly, so it only ever handled gzipped tars. `tar.x` is a direct equivalent, and this drops two dependencies. **`cubejs-backend-shared`** needed a real implementation. `downloadAndExtractFile` is public API (`index.ts` re-exports `./http-utils`) and was format-agnostic, so the new `extractArchive` dispatches on **magic bytes** rather than filename — not a style preference: `streamWithProgress` saves to `crypto.randomBytes(16).toString('hex')` with no extension at all, which is exactly why `decompress` sniffed content too. gzip (`1f 8b`) and plain tar (`ustar` at offset 257) go to `tar.x`; zip (`PK`) goes to `extract-zip`. `.tar.bz2` support is dropped deliberately — nothing in this repository produces one and it would mean another dependency. It throws a named error rather than failing obscurely, as does an unrecognised file. Every in-repo caller is unaffected: maven downloads `.tar.gz`, the testing fixtures are `.tar.gz`, and the Databricks JDBC driver passes `skipExtract: true` so it never reached this path. Verification is by hostile archive rather than version numbers, since resisting traversal is the entire point. Nine new tests in `test/http-utils.test.ts` cover a tar entry escaping via `..`, a tar entry with an absolute path, a tar symlink pointing outside the target, the genuine Zip Slip case, all three supported formats round-tripping, and both rejection paths. The zip fixtures are built byte-by-byte (stored, no deflate) on purpose: `archiver` silently rewrites `../ZIP_PWNED.txt` to `ZIP_PWNED.txt`, which made an earlier version of that test extract a benign archive and pass for the wrong reason. No new dev dependency was needed. 401 tests pass in `cubejs-backend-shared`; both packages typecheck and lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shared,templates): mkdirp the target, pin tar options, witness the fixtures Review follow-ups. **`extractArchive`'s contract depended on the archive format.** `extract-zip` creates its target but `tar.x` throws `CwdError` when it is missing — and callers cannot know the format in advance, that being the whole point of magic-byte dispatch. Now calls `mkdirpSync(cwd)` first, matching what the removed `decompress` did. No in-repo caller was affected (`downloadAndExtractFile` mkdirp'd already), but this is exported API. **Two tar defaults were unintended changes.** `preserveOwner` defaults to true when running as root — the normal case inside the Cube image — which would apply whatever uid/gid the tarball recorded, where the previous implementation always wrote as the current user. And unsafe entries are *dropped with a warning*, not an error, so an archive of nothing but `../evil` would extract to nothing and resolve successfully, leaving the caller to fail later on a confusing missing-file error. Both tar call sites now pass `preserveOwner: false` plus an `onwarn` that reports through `internalExceptions` (a `console.warn` in templates, which has no Sentry channel). Verified the hook fires: `TAR_ENTRY_ERROR: path contains '..'`. **The tar fixtures had the same silent-sanitisation exposure the zip fixture was hand-rolled to avoid.** Their only assertion was a negative, so if a future tar normalised the name assigned in `onWriteEntry`, the fixture would quietly become benign and the tests would keep passing while proving nothing. All three now read the stored entry names back with `tar.t` and assert the hostile name survived into the archive, and the `..` case gained a benign sibling so a pass distinguishes "dropped the bad entry" from "extracted nothing at all". That witness immediately corrected a wrong belief: tar stores an absolute path **verbatim** and strips the leading `/` when extracting, not when writing. The comments claiming otherwise are fixed in both packages. **Nits.** The zip magic comment contained literal control bytes (`\x03\x04`, `\x05\x06`) rather than the escape sequences intended — a heredoc ate them — so it rendered as `"PK", or "PK"`. Replaced with prose, and the file is now verified free of control characters. `Buffer.slice` -> `subarray`. The doc blocks lost their advisory archaeology, keeping the load-bearing parts: why dispatch is by magic bytes, and what the extraction guarantees are. The v7-tar gap (no `ustar` magic to detect it by) is now recorded alongside bzip2. 401 tests pass in cubejs-backend-shared (9 in the new file); both packages typecheck and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shared): decouple benign tar warnings from internalExceptions, test zip symlinks Review follow-ups, both of which corrected something. **`internalExceptions` can exit the process.** It calls `process.exit(1)` under `CUBEJS_INTERNAL_EXCEPTIONS=exit` (`errors.ts:10`), and tar warns about plenty of benign conditions besides traversal — unsupported entry types such as fifos and devices, `TAR_ENTRY_INVALID`, failed utime/chown. Routing all of them there meant one odd entry in a third-party tarball could take the process down mid-download, where before it extracted and carried on. `onwarn` now sends only `TAR_ENTRY_ERROR` — the code tar uses for a rejected path, verified as what it actually emits for a `..` entry — through `internalExceptions`, and `console.warn`s everything else. Under the opt-in `exit` setting a genuine traversal attempt still fails hard, which is what that setting asks for. While confirming this: `internalExceptions` only `console.error`s, it has no Sentry channel, so an earlier claim of mine that this routing "reaches Sentry" was wrong. The reason to distinguish the two paths is severity and the exit behaviour, not delivery. **The zip backend had no symlink test**, and it is the more interesting half: a symlink entry's *name* is perfectly clean, so only a check on the resolved destination catches the entry written through it afterwards. Added one, which required teaching the hand-rolled `writeZip` to set external attributes (the unix mode lives in the high 16 bits, and the shift needs `>>> 0` or it overflows into a negative int32). `extract-zip@2.0.1` passes: it creates the symlink inside the target, then throws `Out of bound path` on the entry written through it, and nothing lands outside. So it does resolve destinations rather than only string-matching names, and the guarantee stated in `extractArchive`'s doc block now holds for the symlink case as well as the name case — demonstrated rather than asserted. 402 tests pass (10 in this file); lint and both typechecks clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(shared): make the zip fixture a faithful unix producer; correct a comment Two review nits. The hand-rolled zip declared `version made by = 20`, whose high byte is the host system — 0, meaning MS-DOS. A zip that genuinely records a symlink comes from a unix producer and carries 3 there. It worked only because `extract-zip@2.0.1` reads `externalFileAttributes >> 16` unconditionally without consulting the host byte, so the test did exercise the real path — but the fixture was asserting against a producer shape no real archiver emits, and a reasonable future hardening (gating the symlink branch on host 3) would have broken it for a fixture reason rather than a behavioural one. Same principle that motivated hand-rolling the archive at all: it should be what an attacker would actually hand you. The `tarOptions` comment described `TAR_ENTRY_ERROR` as "the code tar uses for a rejected path", which is narrower than what the branch catches. Measured it rather than reasoning about it: a read-only target raises `TAR_ENTRY_ERROR` once per entry, exactly as a `..` name does, so per-entry write failures land on the same side of the split. That is the right side — a half-extracted install is a real failure and escalating it is what the opt-in `exit` setting asks for — but the comment now says so instead of implying the branch is traversal-only. 10 tests pass; lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(shared): assert containment positively instead of a loop that cannot fail Review nit, and a correct one. The absolute-path test walked `readdirSync(target, { recursive: true })` asserting every entry resolved under `target` — but those entries are relative paths *of that directory*, so `path.resolve(target, entry)` is under `target` by construction. The loop could never fail: had tar honoured the absolute name, the escaped file simply would not have appeared in the listing at all. It also used `startsWith` on a resolved prefix, which is the classic sibling-directory trap (`/tmp/x/target-evil` satisfies a `/tmp/x/target` prefix check) — harmless here only because the loop was vacuous anyway. Replaced with the assertion the loop was pretending to be: the entry exists at its absolute path re-rooted under the target. That can actually fail, and it holds the test to the same standard as the `..` fixture — a positive assertion distinguishing "contained" from "dropped", rather than a negative one that cannot tell them apart. 10 tests pass; lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )