[tobyhede]

Requested by Toby

When Proxy rewrites a simple Query on 2.2 to encrypt a statement, it rebuilds the SQL from the statements it kept.
A SET, a DISCARD, or a statement sent after mapping was disabled used to be left out, so PostgreSQL never ran it (for example SET search_path TO app; INSERT INTO encrypted ... sent only the INSERT).
#481 fixed the code: every passthrough path in query_handler now keeps its statement.
No test covered a passthrough statement beside an encrypted one, so this PR adds two unit tests to postgresql/tests.rs.
Each guards different passthrough paths, so neither is a duplicate of the other:

• statements_beside_an_encrypted_statement_are_forwarded_in_order: SET search_path, an encrypted INSERT, SELECT id FROM users and DISCARD PLANS are forwarded in order, and the plaintext is not. It guards the push for a statement that is not type checked (SET, DISCARD) and the push for a type-checked statement with no encrypted column (SELECT id).
• a_statement_after_mapping_is_disabled_is_forwarded_beside_an_encrypted_statement: after SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = true, the later INSERT is forwarded unchanged beside the encrypted one. It guards the mapping-disabled push. Forwarding that plaintext is the intended effect of UNSAFE_DISABLE_MAPPING.

Tests only, so no changelog entry.

Validation

• Mutations: removing each of the three transformed_statements.push lines in the passthrough paths makes the test that guards it fail. All tests pass on release/2.2.
• cargo test -p cipherstash-proxy --lib (271 passed), cargo clippy -p cipherstash-proxy --all-targets --all-features -- -D warnings and cargo fmt --check.

Acknowledgment

By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.

🤖 Generated with Claude Code

[freshtonic]

Reviewed head def4ec7b against release/2.2 (fef0f29d). Approved: the two tests are correct and guard the two #481 fixes. S1 and the nits below can go in this PR or later.

Important follow-up: when I traced the other passthrough paths in query_handler, I found a fail-open on 2.2 that is older than this PR. It does not block this PR, but it needs its own ticket and fix soon.

Important follow-up: fail-open on 2.2 (not caused by this PR)

The type-check fallback forwards the original Query, together with any statement in it that needs encryption.

• The code is frontend.rs:538-557. When a statement fails the type check and mapping errors are off, query_handler returns Ok(None), so Proxy forwards the client's Query unchanged. Mapping errors are off by default (CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS).
• This also forwards every other statement in the Query as the client wrote it, including an INSERT into an encrypted column. I confirmed this with a scratch test in postgresql/tests.rs (Connection::encrypted(), default config):
  • SELECT * FROM no_such_table; INSERT INTO users (email) VALUES ('a@example.com') is forwarded unchanged. Proxy encrypts nothing, and no error is sent to the client.
  • INSERT INTO users (email) VALUES ('a@example.com'); SELECT * FROM no_such_table is also forwarded unchanged. Proxy encrypts the INSERT first (encrypted_with == [None]) and then discards the result.
• In my test, PostgreSQL would fail the Query, because no_such_table does not exist. In production, the failing statement can be one that PostgreSQL runs but Proxy cannot type check, for example a read of a table that Proxy has not loaded. PostgreSQL then runs the INSERT with the plaintext. A plain string is not a valid eql_v2_encrypted literal, so PostgreSQL often rejects the value. But the plaintext is always in the SQL that reaches the database, and a value in the shape of a record literal is stored.
• The same code was on the 2.2 base before #481 (4facf298), so this is not a regression from the recent backports.
• main has a batch rule for this: "A fallback forwards the whole Query as the client wrote it, so it must not carry a statement that needs mapping". On 2.2, the equivalent is to refuse the Query when another statement in it needs encryption, and also to refuse a write that falls back while encrypted columns are configured.
• Please open a ticket for this and fix it in a separate PR, with priority.

Should fix

S1. One passthrough path still has no test. These tests guard the mapping_disabled push (frontend.rs:527) and the push for a statement that is not type checked (:536). They do not guard the push for a statement that is type checked but uses no encrypted column (:620). When I removed that push, both tests still passed, and so did the full suite. Then INSERT INTO users (email) VALUES (…); SELECT id FROM users sends only the INSERT. Add such a statement to statements_beside_an_encrypted_statement_are_forwarded_in_order.

Nits

• a_statement_after_mapping_is_disabled_is_forwarded_beside_an_encrypted_statement asserts that a plaintext INSERT into the encrypted users.email column is forwarded. That is the documented effect of UNSAFE_DISABLE_MAPPING. But a reader who sees a test that requires plaintext in an encrypted column can think that it is a bug. Add a one-line comment that this is the intended unsafe behaviour.
• The PR description says "Both tests fail with #481's two transformed_statements.push lines removed". That is true when both lines are removed, but each test guards only one line: the first test guards :536, and the second guards :527. Say this, so that no one deletes one test as a duplicate.

What I checked and found correct

• Mutations. When I removed the push at :527, the mapping-disabled test failed. When I removed the push at :536, the ordering test failed.
• The assertions. forwarded_query requires exactly one forwarded Q message. Both tests check that the plaintext of the encrypted INSERT is not in it, and that no error was sent to the client. The prefix and suffix checks prove that the order is kept.

What I ran

• cargo test -p cipherstash-proxy --lib: 270 of 271 passed. The one failure, config::tandem::tests::prometheus_config, is the environment race. The config:: tests with --test-threads=1: 42 of 42 passed.
• The scratch test and the mutations above. I reverted all changes after the runs.
• CI: all 4 checks pass.

[tobyhede]

Thanks @freshtonic. I pushed a2bac0e, which addresses the review:

• S1: the ordering test now includes SELECT id FROM users. With the type-checked passthrough push removed, the test fails, because only SET;INSERT;DISCARD is forwarded.
• Nits: each test now has a doc comment naming the passthrough path it guards. The mapping-disabled test also says that forwarding the plaintext is the intended effect of UNSAFE_DISABLE_MAPPING. The description now says which test guards which push.
• Fail-open in the type-check fallback: I filed a high-priority ticket, which includes your two repros and main's batch rule as the target. It will be fixed in a separate PR against release/2.2.

---

Generated by Claude Code