KSQL-15030 | Address misc items for ksql - #11063
Open
Vedarth Sharma (VedarthConfluent) wants to merge 3 commits into
Open
KSQL-15030 | Address misc items for ksql#11063Vedarth Sharma (VedarthConfluent) wants to merge 3 commits into
Vedarth Sharma (VedarthConfluent) wants to merge 3 commits into
Conversation
…ry protobuf state and redact OAuth secret on toString Two critical security / data-corruption fixes. (1) RowFormat.PROTOBUF was a JVM-singleton enum constant whose anonymous body declared mutable `transient ConnectSchema connectSchema;` and `transient KsqlConnectSerializer<Struct> serializer;` instance fields. Every metadataRow() wrote them; every dataRow() read them. Two concurrent push queries on application/vnd.ksql.v1+protobuf race on the singleton: query B's metadataRow overwrites schema and serializer, then query A's later dataRow either throws (column-count mismatch on `new Struct(B's schema)`) or - worse - serializes A's rows under B's protobuf schema, returning bytes that decode incorrectly on the client. Convert the per-query state out of the enum. RowFormat is now a factory; each constant overrides newFormatter() to return a fresh RowFormatter instance with its own per-writer state. JsonStreamedRowResponseWriter calls rowFormat.newFormatter() once at construction and stores the resulting formatter, so every concurrent writer has an isolated state and protobuf rows are always serialized under their owning query's schema. (2) ClientSecretIdpConfig.toString() included idpClientSecret verbatim. Any log statement that concats the config - LOG.info( "config={}", cfg), throw new RuntimeException("bad cfg: " + cfg), String.valueOf(cfg) - leaked the OAuth client secret in plaintext to log files. Redact the secret in toString() to "[REDACTED]" (still distinguishes null from set for differential debugging). The getter is untouched so legitimate callers still receive the cleartext. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
…xecutorTest to new RowFormatter API The previous commit converted RowFormat from a stateful enum into a factory that hands out fresh per-writer RowFormatter instances. Two existing tests in RestTestExecutorTest still called the now-removed abstract methods directly via the enum constant (RowFormat.PROTOBUF.metadataRow / .dataRow), which broke compilation of the functional-tests module and surfaced in CI as the CP Jar Build CI Gating failure. Switch the tests to obtain a single RowFormatter from RowFormat.PROTOBUF.newFormatter() and call metadataRow / dataRow on that formatter. Reusing the same instance across the metadata + data calls preserves the existing test semantics (the PROTOBUF formatter caches the Connect schema between metadataRow and dataRow). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r-writer protobuf RowFormatter isolation Adds a test that pins the contract from the P0 fix in this PR: each call to RowFormat.PROTOBUF.newFormatter() must produce a distinct RowFormatter, and the per-instance ConnectSchema and serializer state must not leak between formatters. The test drives two formatters with two different LogicalSchemas (one column vs two columns), then exercises dataRow on each with matching row widths. Under the previous implementation - where the PROTOBUF enum constant cached connectSchema/serializer in fields shared across the JVM - the second metadataRow would clobber the first's schema, and the next f1.dataRow() would throw a column-count mismatch or, worse, serialize under the wrong schema. Co-Authored-By: Claude Opus 4.7 (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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
What behavior do you want to change, why, how does your patch achieve the changes?
Address misc items for ksql
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist