fix: close serialization RCE, 5 lossy-payload defects, and 4 t.all() defects

[hussainsultan]

Fixes ten defects found by an implementation evaluation of this repo. Every one was reproduced with a working proof-of-concept before being fixed, and each has a regression test — none of these paths had coverage.

Full suite: 1266 passed, 1 skipped, 11 xfailed, 4 xpassed, 0 failed. 29 tests added.

1. Arbitrary code execution from a tagged model (critical)

("fn", module, qualname) imported any module and handed the result to Call.resolve(), which calls it. ("call", ("fn","builtins","eval"), ...) was a working RCE, and the import side effect fired at deserialize time, before anything resolved.

This is reachable, not theoretical: the xorq.from_tag_node entry point in pyproject.toml routes any bsl-tagged expression through this code automatically, and git catalogs store the resolver trees as plain editable YAML. Anyone with write access to a catalog gets code execution in every reader's process. The v2.0 refactor removed pickle's format but kept pickle's capability.

Callables are now restricted to the ibis/xorq/operator module roots, validated on both sides — an author fails when serializing rather than a reader when loading. The resolved object is checked too, because a qualname is a getattr chain: ("fn","ibis.util","os.system") and ("fn","ibis.expr.api","builtins.eval") both start in a trusted module and walk out of it. Both are covered by tests.

Sizing the allowlist was empirical — instrumenting the full suite shows real models emit exactly two callables ever, ifelse and _finish_searched_case, so the restriction costs nothing. trust_callable_module() is the documented opt-in for anyone who needs more.

2. Five silent-wrong serialization defects

Each reproduced end-to-end through the public to_tagged/from_tagged API:

• thaw() collapsed multi-argument calls. ("just", x) resolver nodes look exactly like the tuple-of-pairs encoding of a dict, so only the last survived: substr(0, 2) came back as substr(2), isin([x, y]) as isin([y]). Group-by results changed with no error. Values under a *_struct key are now opaque. The corruption was read-side only, so this repairs existing payloads with no format change.
• freeze() stringified every non-scalar constant. Dates came back as strings; Decimal and np.int64 came back as type errors from the query compiler. Constants now carry a typed encoding, and freeze() raises rather than coercing.
• Name replay beat the serialized expression. aggregate(n=lambda t: t.a.max()) replayed the model's n = a.sum() when the names collided (3 → 6). Bare references are now recorded explicitly, so name replay — which is what makes them fan-out safe — is no longer inferred from a name collision.
• No version gate. bsl_version was written and never read, so a v1.0 payload (pickled expressions, a format no longer read at all) and a dimension with no readable expression both degraded to raw column references: amount = _.amount * 1.1 came back as amount, with plausible numbers and no error. Both are refused now.
• _rebind_to_backend repointed every DatabaseTable at one backend without checking. Joining two same-schema databases read every column from whichever came first — prod's data silently replaced by staging's. The fix hinges on an exact signal: from_ibis() mints a new Backend wrapper per call but reuses the caller's connection object, while a different database has a different one. Rebinding is now gated on connection identity, so duplicate wrappers still unify and distinct databases fail in the engine instead of merging. A near-duplicate copy of the rebind logic was folded into the shared primitive.

3. t.all() / percent-of-total

t.all(x) means "x over the whole filtered dataset, ignoring the group by". Four code paths implemented it differently and three returned a sum of per-group values instead — which still looks like a share, so nothing surfaced.

• IbisCalcScope.all only routed an exact Field reference to the real totals table, so t.all(m) and t.all(m * 1) disagreed; for a mean measure the second summed the per-group means. Measure references inside an expression are now rebound to the totals table.
• The post-aggregation chain (aggregate().order_by().mutate()) only has the grouped rows, so its total can only be a window sum — exact for SUM/COUNT, meaningless for a mean, median, distinct count or ratio, where it disagreed with the identical calc-measure formula (0.167 vs 0.5). Non-additive references are now refused, naming the spelling that works; additive ones are unchanged.
• Two integer operands truncated: total / t.all(total) returned 0 because xorq's DataFusion does integer division. Integral totals are cast, matching why the calc path gives its virtual columns a float schema.
• On a join_many model with only calc measures requested, the pre-agg fallback returned tbl.aggregate({}) — an aggregate with zero columns — dropping the group keys and every calc spec, surfacing much later as Schema and number of arrays unequal from arrow. It now aggregates with its group keys, and refuses (naming the fix) when a calc builds its reduction inline and so has no fan-out-safe base.

Notes for review

• Three changes are deliberately fail-loud rather than best-effort: unsupported payload versions, non-additive post-agg totals, and inline reductions in totals under join_many. Each replaces a silently wrong number, and each error message carries the working alternative. test_from_xorq_with_tagged_table was updated because it pinned the silent-degrade behavior.
• Non-additivity is classified by resolving each measure against its root's raw table; measures that can't be classified are omitted rather than assumed non-additive, so unclassifiable cases keep their current behavior instead of failing on a guess.
• Two findings from the evaluation did not hold up under a corrected harness (df.sort_values(k)[col][0] is label-based indexing and was reading an arbitrary row): the reported declaration-order dependence of percent-of-total, and a general t.all() × join_many crash. The underlying defects were real but narrower than first described. Tests now compare full result dicts, with data where the correct and sum-of-group-values answers cannot coincide.

Left alone as out of scope: the ops.py monolith, the unenforced ruff select set, and convert.py's dead Semantic*Op handlers.

🤖 Generated with Claude Code