Skip to content

fix: refuse bytes against a text column on Postgres (#785) - #834

Merged
0x054 merged 3 commits into
mainfrom
sdd/785-secretbytes-text-column-refusal
Oct 30, 2026
Merged

0x054 merged 3 commits into
mainfrom
sdd/785-secretbytes-text-column-refusal

Conversation

@0x054

@0x054 0x054 commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #785

What

On Postgres, an operation that would move bytes into a text column (or text into a bytea column) now raises DataError before any statement is sent, instead of storing hex text. The per-table catalog records each column's live storage class from its existing probe (typcategory), so matching columns cost no new round trip. Covers save, bulk_create, instance update, query update(), and fetches that select the column (root, projection of it, include() hops). SQLite is unaffected. The 0.26 upgrade note describes the refusal.

Verify

Ran the ticket's Verify block on 3ae15e8 (merged with origin/main cf1ee43): pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgres passed (Postgres 18); cargo test -q --no-default-features --features testing catalog passed. Full suite before the merge: 6435 passed, 1 failed (test_postponed_annotations, fixed on main by #831, passes after the merge).
TDD: the test file was written alongside the implementation, not run red first (the red state is the issue's reported corruption); GREEN 9/9 new tests on sqlite+postgres.
Verify block corrected: yes: cargo test -q -p ferro-orm catalog does not link; ran cargo test -q --no-default-features --features testing catalog (the CI form).

Interfaces

Interfaces changed: none to the ticket's Produces; TableCatalog also gains storage_type: HashMap<String, String> (live type name, for the message). StorageClass::Other columns are absent from the map rather than stored.

Laws

  • AGENTS.md#I-6: refuses loudly with the fix named; no warning/skip path
  • AGENTS.md#I-3: n/a: no unwrap added; errors via PyResult
  • AGENTS.md#I-4: Rust change covered by pytest bridge tests
  • AGENTS.md#I-10: n/a: CHANGELOG untouched
  • AGENTS.md#I-1, I-2, I-7, I-8: n/a: no DDL, hydration or docs-example change

Notes for the reviewer

Outside Files owned: src/errors.rs (data_error constructor, there is no Python-side change so exceptions.py is untouched) and src/state.rs (model_label_for_table, for the model name in the message). A projection through a relation (key.vault.secret) is checked against the hop table catalog; aggregates are not checked. The refusal also applies to .sql() rendering of a fetch since it shares build_fetch.

🤖 Generated with Claude Code

0x054 and others added 2 commits October 30, 2026 14:26
…mn on Postgres (#785)

The per-table catalog records each column's live storage class; writes and
fetches that cross the text/bytes boundary raise DataError before sending.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@0x054 0x054 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec: FAIL

  • Important · [ticket#Acceptance criteria] · src/operations.rs:3675 at 6a46b66 · Missing · The fetch check only looks at projected columns whose path is empty. A projection that reaches the mismatched column through a relation is not checked and still corrupts the value without an error. Reproduced on Postgres 18 at this head: Vault.secret declared SecretBytes, live varchar holding '\xff006b', and Key(vault FK). await Key.select(lambda key: key.vault.secret).all() returns Row(secret=SecretBytes(...)) whose get_secret_value() is b'\\xff006b', while Vault.get(1) refuses. The ticket says "a fetch whose selection includes such a column (... a projection of it, a hop that loads it)" must refuse. The PR body lists this as a limit, but the ticket gives no Narrows line for it. Fix: for a Column with a non-empty path, resolve the table through join_plan.prefix_table, then check it against plan.hop_registrations and that table's cached catalog. populate_hop_bind_context has already loaded that catalog, so this adds no round trip. Add a test for it.
    Cannot verify: none

Accepted as declared: src/errors.rs (data_error) and src/state.rs (model_label_for_table) fall outside Files owned, and both are needed for the message. TableCatalog.storage_type is an extra field, needed for the live type name in the text. StorageClass::Other exists but is never stored, because a missing key reads as Other, which is equivalent for every reader. .sql() of a refused fetch also refuses, because it shares build_fetch and so stays consistent with the rendered-statement parity pin. The DataError text matches the ticket's Interfaces line exactly (test_save_refuses_and_leaves_the_table_unchanged checks it with ==). The hop catalog read at src/operations.rs:3683 is a cache hit, since populate_hop_bind_context (:409) already loaded every join-plan table. tests/test_catalog_cache.py passes and the new happy-path count assert holds.

Quality: FAIL

  • Important · [AGENTS.md#I-6] · src/operations.rs:3675 at 6a46b66 · The PR body's Laws line claims "I-6: refuses loudly ... no warning/skip path", but it also documents "a projection that traverses a relation ... not checked" as a known limit. That limit is a silent-corruption path (see the Spec finding), which is the "documented residual risk" I-6 forbids. The fix is the same as in the Spec finding.

Verify: PR head 6a46b66 passed (pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgres: 36 passed, all 8 Postgres-marked new tests ran on Postgres 18; cargo test -q --no-default-features --features testing catalog passed). The ticket's cargo test -q -p ferro-orm catalog fails with "package ID specification did not match", so the PR's Verify block corrected: yes is accurate.
Trial merge: not run (base unchanged; the head already contains origin/main cf1ee43f)

…rage class (#785)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@0x054 0x054 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: 6a46b66..3ae15e8

  • Spec finding (relation-path projection unchecked, src/operations.rs:3675): ADDRESSED. At src/operations.rs:3671-3696 at 3ae15e8, a projected column with a non-empty path is resolved through join_plan.prefix_table and plan.hop_registrations and checked against the hop table's cached catalog. My Postgres 18 reproduction (Key.select(lambda key: key.vault.secret).all() over a varchar hop column) now raises DataError: Vault.secret is declared bytea but the live column is varchar; .... The new test_projection_through_a_relation_checks_the_hop_column[varchar|bytea] covers both the refusal and the matching case.
  • Quality finding (I-6 Laws claim contradicted by the documented limit): ADDRESSED. The relation-projection limit is gone from the code and from the PR body, so the I-6 line now holds.

No new Critical or Important problems in the fix diff. The hop catalog read at :3694 hits the cache, because populate_hop_bind_context loaded every join-plan table, and tests/test_catalog_cache.py still passes.

Spec: PASS

Cannot verify: none

Quality: PASS

Verify: PR head 3ae15e8 passed (pytest tests/test_storage_class_mismatch.py tests/test_secret_fields.py tests/test_catalog_cache.py --db-backends=sqlite,postgres: 38 passed, 10 new-file tests on Postgres 18; cargo test -q --no-default-features --features testing catalog passed)
Trial merge: not run (base unchanged; merge base with origin/main is cf1ee43f)

@0x054
0x054 merged commit eddd177 into main Oct 30, 2026
11 checks passed
@0x054
0x054 deleted the sdd/785-secretbytes-text-column-refusal branch October 30, 2026 18:55
@cursor cursor Bot mentioned this pull request Nov 2, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SecretBytes saved into a 0.25 varchar column on Postgres is stored as escaped text with no error

1 participant