From f8d50b0bfb209309e1bef9ca8ca5f2fc3f8d8c15 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Sat, 4 Jul 2026 15:29:30 -0500 Subject: [PATCH] fix(strict_provenance): gate update1 inside make() (target + key consistency) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2.3 post-release audit found update1 was a fully ungated write path under strict_provenance: inside make(), SideTable.update1({...}) silently mutated any table past the provenance boundary (empirically demonstrated) while the same write via insert is blocked. The asymmetry was unintended: the boundary is 'write only to self (and Parts)', and update1 is a write. update1 now runs the same two checks as insert, before the existence check so a provenance violation raises as such: - assert_write_allowed(self, verb='update1 on') — target must be the make() target or one of its Parts; blocked-update message reads 'update1 on is not permitted inside make() for ...'. - assert_row_key_allowed(row, action='update') — key columns overlapping the current make() key must match; mismatch message reads 'updated row's ... Updates must be consistent with the key being populated.' Both gate functions gain wording parameters; the insert-path messages are byte-identical to before. No-op outside make() or with the flag off — update1 behavior outside strict mode is unchanged (pinned by a new test). Tests: blocked update1 on another table (value verified untouched), blocked key-mismatched update1 on self, allowed key-consistent update1 on self, and default-off parity. Full strict suite 12/12; test_update1 4/4. Companion docs update (write-enforcement table + limits) rides in the datajoint-docs post-audit reconciliation PR. --- src/datajoint/provenance.py | 48 +++++-- src/datajoint/table.py | 12 ++ tests/integration/test_strict_provenance.py | 151 ++++++++++++++++++++ 3 files changed, 196 insertions(+), 15 deletions(-) diff --git a/src/datajoint/provenance.py b/src/datajoint/provenance.py index 8f196194f..b2858f7c0 100644 --- a/src/datajoint/provenance.py +++ b/src/datajoint/provenance.py @@ -125,12 +125,13 @@ def assert_read_allowed(query_expression) -> None: ) -def assert_write_allowed(target_table) -> None: +def assert_write_allowed(target_table, verb: str = "insert into") -> None: """ - Verify the *target* of an insert is allowed under the active strict-make context. + Verify the *target* of a write is allowed under the active strict-make context. - Called from ``Table.insert`` after the existing ``_allow_insert`` check and - before any rows are materialized. No-op when no strict-make context is active. + Called from ``Table.insert`` (after the existing ``_allow_insert`` check and + before any rows are materialized) and from ``Table.update1`` (before the + UPDATE is issued). No-op when no strict-make context is active. Allowed targets: @@ -140,6 +141,14 @@ def assert_write_allowed(target_table) -> None: as rows are materialized, so this gate never consumes the caller's ``rows`` iterable — a one-shot generator must survive to reach ``insert``. + Parameters + ---------- + target_table : Table + The table being written to. + verb : str + Phrase naming the write operation in the error message + (``"insert into"`` or ``"update1 on"``). + Raises ``DataJointError`` if the target is not permitted. """ ctx = _active_strict_make.get() @@ -168,21 +177,29 @@ def assert_write_allowed(target_table) -> None: if target_name not in target_set: raise DataJointError( - f"strict_provenance=True: insert into {target_name!r} is not permitted " + f"strict_provenance=True: {verb} {target_name!r} is not permitted " f"inside make() for {make_target.full_table_name!r}. Only the target " f"table and its Part tables may be written." ) -def assert_row_key_allowed(row) -> None: +def assert_row_key_allowed(row, action: str = "insert") -> None: """ - Verify a single insert row's key columns match the active ``make()`` key. + Verify a single written row's key columns match the active ``make()`` key. - Called per row from ``Table._insert_rows`` as rows are materialized, so the + Called per row from ``Table._insert_rows`` as rows are materialized (so the check sees a concrete row without the write gate having to consume the - caller's ``rows`` iterable. No-op when no strict-make context is active or - when ``row`` is not a dict (numpy records / bare sequences carry no field - names to check by — same as the previous behavior). + caller's ``rows`` iterable), and from ``Table.update1`` with the update row. + No-op when no strict-make context is active or when ``row`` is not a dict + (numpy records / bare sequences carry no field names to check by — same as + the previous behavior). + + Parameters + ---------- + row : dict + The row being written. + action : str + ``"insert"`` or ``"update"`` — selects the error-message wording. Raises ``DataJointError`` on a mismatch. """ @@ -192,15 +209,16 @@ def assert_row_key_allowed(row) -> None: if not isinstance(row, dict): return _make_target, _allowed_tables, key = ctx - _check_row_key(row, key) + _check_row_key(row, key, action) -def _check_row_key(row: dict, current_key: dict) -> None: +def _check_row_key(row: dict, current_key: dict, action: str = "insert") -> None: """Raise if any row attribute overlapping with the current key has a different value.""" + past, plural = {"insert": ("inserted", "Inserts"), "update": ("updated", "Updates")}[action] for k, v in current_key.items(): if k in row and row[k] != v: raise DataJointError( - f"strict_provenance=True: inserted row's {k!r}={row[k]!r} does not " - f"match the current make() key's {k!r}={v!r}. Inserts must be " + f"strict_provenance=True: {past} row's {k!r}={row[k]!r} does not " + f"match the current make() key's {k!r}={v!r}. {plural} must be " f"consistent with the key being populated." ) diff --git a/src/datajoint/table.py b/src/datajoint/table.py index 4f44ffbf6..a09337c6c 100644 --- a/src/datajoint/table.py +++ b/src/datajoint/table.py @@ -551,6 +551,18 @@ def update1(self, row): raise DataJointError("Attribute `%s` not found." % next(k for k in row if k not in self.heading.names)) except StopIteration: pass # ok + + # Strict-provenance write gate. No-op outside make() or when the config + # flag is off. Same boundary as insert: the target must be the make() + # target or one of its Parts, and the row's key columns must match the + # current key. Placed before the existence check so a provenance + # violation raises as such rather than as "no matching entry". + # See src/datajoint/provenance.py. + from .provenance import assert_row_key_allowed, assert_write_allowed + + assert_write_allowed(self, verb="update1 on") + assert_row_key_allowed(row, action="update") + if len(self.restriction): raise DataJointError("Update cannot be applied to a restricted table.") key = {k: row[k] for k in self.primary_key} diff --git a/tests/integration/test_strict_provenance.py b/tests/integration/test_strict_provenance.py index 5def0c960..4d6124dfe 100644 --- a/tests/integration/test_strict_provenance.py +++ b/tests/integration/test_strict_provenance.py @@ -320,3 +320,154 @@ def make(self, key): # No strict_mode fixture — default-off DerivedLegacy.populate() assert (DerivedLegacy & {"subject_id": 1}).fetch1("val") == 0 + + +def test_strict_blocks_update1_on_other_table(prefix, connection_test, strict_mode): + """update1 is a write: under strict mode, updating a table that is not + self or one of its Parts raises, and the target row is left unmodified. + Regression for the ungated-update1 hole found in the 2.3 post-release audit.""" + schema = dj.Schema(f"{prefix}_strict_update1_other", connection=connection_test) + + @schema + class Subject(dj.Lookup): + definition = """ + subject_id : int32 + """ + contents = [(1,)] + + @schema + class SideResult(dj.Manual): + definition = """ + side_id : int32 + --- + val : int32 + """ + + SideResult.insert1({"side_id": 1, "val": 100}) + + captured: list[Exception] = [] + + @schema + class Derived(dj.Computed): + definition = """ + -> Subject + --- + val : int32 + """ + + def make(self, key): + try: + SideResult.update1({"side_id": 1, "val": 999}) + except DataJointError as e: + captured.append(e) + self.insert1({**key, "val": 1}) + + Derived.populate() + assert len(captured) == 1 + assert "update1 on" in str(captured[0]) + assert "not permitted" in str(captured[0]) + # The side table must be untouched. + assert (SideResult & {"side_id": 1}).fetch1("val") == 100 + + +def test_strict_blocks_update1_with_mismatched_key(prefix, connection_test, strict_mode): + """update1 on self with key columns that disagree with the current make() + key raises the key-consistency error (before any existence check).""" + schema = dj.Schema(f"{prefix}_strict_update1_key", connection=connection_test) + + @schema + class Subject(dj.Lookup): + definition = """ + subject_id : int32 + """ + contents = [(1,)] + + captured: list[Exception] = [] + + @schema + class Derived(dj.Computed): + definition = """ + -> Subject + --- + val : int32 + """ + + def make(self, key): + self.insert1({**key, "val": 1}) + try: + # bogus key — must be rejected by the provenance key check, + # not by the "one existing entry" check + self.update1({"subject_id": 999, "val": 2}) + except DataJointError as e: + captured.append(e) + + Derived.populate() + assert len(captured) == 1 + assert "updated row's" in str(captured[0]) + assert "does not match the current make() key" in str(captured[0]) + + +def test_strict_update1_on_self_with_matching_key_allowed(prefix, connection_test, strict_mode): + """update1 on self with a key-consistent row is permitted under strict mode + (corrective update within the provenance boundary).""" + schema = dj.Schema(f"{prefix}_strict_update1_self", connection=connection_test) + + @schema + class Subject(dj.Lookup): + definition = """ + subject_id : int32 + """ + contents = [(1,)] + + @schema + class Derived(dj.Computed): + definition = """ + -> Subject + --- + val : int32 + """ + + def make(self, key): + self.insert1({**key, "val": 1}) + self.update1({**key, "val": 2}) + + Derived.populate() + assert (Derived & {"subject_id": 1}).fetch1("val") == 2 + + +def test_update1_unchanged_without_strict(prefix, connection_test): + """With strict_provenance off (default), update1 from inside make() behaves + as before — no gate fires.""" + schema = dj.Schema(f"{prefix}_update1_default_off", connection=connection_test) + + @schema + class Subject(dj.Lookup): + definition = """ + subject_id : int32 + """ + contents = [(1,)] + + @schema + class SideResult(dj.Manual): + definition = """ + side_id : int32 + --- + val : int32 + """ + + SideResult.insert1({"side_id": 1, "val": 100}) + + @schema + class DerivedLegacy(dj.Computed): + definition = """ + -> Subject + --- + val : int32 + """ + + def make(self, key): + SideResult.update1({"side_id": 1, "val": 200}) + self.insert1({**key, "val": 0}) + + DerivedLegacy.populate() + assert (SideResult & {"side_id": 1}).fetch1("val") == 200