refactor(table names): handle unsafe table names - #174
Conversation
…mes are mostly safe now (need to look again with fresh eyes) and test coverage is back up to required min
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/query/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMoves table selection to ChangesTable-Name Safety and API Endpoint Refactor
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
|
github-actions
Bot
added
go
coderabbitai
Bot
added
documentation
github-actions
Bot
removed
documentation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the system to support arbitrary ClickHouse table names by implementing a robust encoding mechanism for NATS subjects and switching to parameterized queries for ClickHouse inserts. These changes effectively mitigate SQL injection risks while allowing for flexible table naming conventions. Additionally, the test suite has been updated to ensure high coverage and verify the new safety measures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
internal/query/builder_test.go (1)
26-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing test coverage for backtick escaping in table names.
The
Buildfunction now escapes backticks viastrings.ReplaceAll(table, "", "``")`, but there's no test verifying this escaping works correctly. Add a test case for table names containing backticks to ensure the double-backtick escaping produces valid SQL.✅ Proposed test case
func TestBuild_TableNameWithBacktick(t *testing.T) { t.Parallel() sq := &StructuredQuery{Columns: []string{"page"}, Limit: 10} result, err := Build("my`table", sq, testSchema(), 0) require.NoError(t, err) assert.Equal(t, "SELECT page FROM `my``table` LIMIT 10", result.SQL) }Note: You'll need to adjust
testSchema()or create a schema that matches the table name being tested, or the test will need to use a schema with a matchingNamefield if that's validated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/query/builder_test.go` around lines 26 - 40, Add a unit test that verifies Build correctly escapes backticks in table names: create TestBuild_TableNameWithBacktick which calls Build("my`table", sq, testSchema(), 0) with a simple StructuredQuery (e.g., Columns: []string{"page"}, Limit: 10), require no error and assert the SQL equals "SELECT page FROM `my``table` LIMIT 10"; ensure the testSchema used contains a matching table Name or adjust/create a test schema entry so Build accepts the table name.internal/ingest/bento_test.go (1)
210-240: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTest name is now misleading: rename to reflect new pass-through behavior.
The function is named
TestJsInput_Read_UnsafeTableNameDroppedbut the test now asserts the opposite—that unsafe table names are not dropped (assert.False(t, badMsg.doubleAcked)). Rename to something likeTestJsInput_Read_UnsafeTableNamePassesThroughorTestJsInput_Read_UnsafeTableNameNotDroppedto accurately describe the tested behavior.♻️ Suggested rename
-func TestJsInput_Read_UnsafeTableNameDropped(t *testing.T) { +func TestJsInput_Read_UnsafeTableNamePassesThrough(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ingest/bento_test.go` around lines 210 - 240, Rename the test function TestJsInput_Read_UnsafeTableNameDropped to reflect the new pass-through behavior (e.g., TestJsInput_Read_UnsafeTableNamePassesThrough or TestJsInput_Read_UnsafeTableNameNotDropped) and update any references or comments; ensure the function signature (func TestJsInput_Read_UnsafeTableNamePassesThrough(t *testing.T)) and the file's test mentions match the new name, and keep assertions around jsInput.Read, badMsg.doubleAcked, goodMsg.doubleAcked, and MetaGet checks unchanged.internal/ingest/bento.go (1)
86-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIssue
#151requirement not fully addressed: empty-table-name rejections still bypass DLQ.The PR claims to close Issue
#151, which explicitly requires routing empty-table-name rejections through the DLQ (subjectdlq.__rejected__) with anx-wh-drop-reasonheader. This code path still logs and DoubleAcks without publishing to the DLQ stream.Per the issue objectives: "Route both unsafe-table-name and empty-table-name rejection paths through the existing DLQ."
🔧 Suggested approach
+ const dlqRejectedSubject = "dlq.__rejected__" + // Reject messages with no table name. if raw.TableName == "" { slog.ErrorContext(msgCtx, "rejecting message: empty table_name") + // Route to DLQ per Issue `#151` requirements + if _, pubErr := js.Publish(msgCtx, dlqRejectedSubject, m.Data(), + jetstream.WithMsgHeader("x-wh-drop-reason", "empty-table-name")); pubErr != nil { + slog.ErrorContext(msgCtx, "DLQ publish failed for empty table_name", "error", pubErr) + } if doubleAckErr := m.DoubleAck(msgCtx); doubleAckErr != nil { slog.WarnContext(msgCtx, "double ack failed for dropped message", "error", doubleAckErr) } continue }Note: This requires access to the JetStream context within
jsInput.Read. You may need to injectjsas a field onjsInputor restructure the rejection path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ingest/bento.go` around lines 86 - 94, The empty-table-name branch in jsInput.Read currently logs and DoubleAcks (raw.TableName == "" -> slog.ErrorContext + m.DoubleAck) but does not publish to the DLQ; change this to route the rejection through the same DLQ used for unsafe-table-name by publishing the message to subject "dlq.__rejected__" with header "x-wh-drop-reason" set to an appropriate reason, then DoubleAck; to do this, ensure the jsInput struct has access to the JetStream context (add a js field or otherwise pass js into jsInput.Read), use that js context to PublishMsg/Publish with headers for the DLQ, and replace the current slog.ErrorContext + m.DoubleAck-only flow in the raw.TableName == "" branch with the DLQ publish + DoubleAck sequence mirroring the unsafe-table-name handling.tests/integration/dlq_test.go (1)
68-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
query.EncodeTablefor DLQ subject construction.Line 62 correctly uses
query.EncodeTable(table)for the ingest subject, but line 68 constructs the DLQ subject using the raw table name. According to the PR objectives and review stack context, NATS subjects (including DLQ subjects) should use encoded table names for consistency.🔧 Proposed fix
- dlqSubject := "dlq." + table + dlqSubject := "dlq." + query.EncodeTable(table)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/dlq_test.go` at line 68, The DLQ subject is built from the raw table name (dlqSubject := "dlq." + table) but should use the encoded table like the ingest subject; update the dlqSubject construction to use query.EncodeTable(table) (i.e., prefix "dlq." to the encoded table value) so DLQ subjects consistently match the ingest subject encoding.internal/api/stream_test.go (2)
207-207: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
assertJSONErrorResponsefor internal API error-path assertions.These error-path checks currently use
testutil.AssertJSONContains; the package-local helper provides stronger header assertions for API errors.Based on learnings: “In WaveHouse tests under internal/api/**/*_test.go, use the package-local helper
assertJSONErrorResponse(t, w)for HTTP error-path assertions… it also assertsContent-TypeandX-Content-Type-Optionsheaders.”Also applies to: 243-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/stream_test.go` at line 207, Replace the error-path assertions that call testutil.AssertJSONContains(t, w, http.StatusBadRequest, ...) with the package-local helper assertJSONErrorResponse(t, w); specifically locate usages of testutil.AssertJSONContains in internal/api/stream_test.go (e.g., the call with map[string]any{"error": tc.errBody} and the other occurrence around the noted range) and change them to assertJSONErrorResponse(t, w) so the test also asserts Content-Type and X-Content-Type-Options headers for API error responses.
195-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win“empty after url decode” case is not testing a decode-to-empty input.
Line 195 duplicates the missing-table scenario (
table == "") instead of a percent-encoded value that decodes to empty/invalid. This weakens the regression test intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/stream_test.go` at line 195, Replace the duplicated test case that currently uses an empty string for the "empty after url decode" case with a percent-encoded value that decodes to an empty/invalid value (for example "%00" or another encoding your URL decoder treats as empty) so the test actually verifies the decode-to-empty scenario; update the test case input (the second element of the tuple) from "" to the chosen percent-encoded string and keep the expected error string ("missing required query parameter: table") or adjust it if your decoder returns a different error for decoded-empty values, ensuring the tuple with description "empty after url decode" is the one changed rather than duplicating the plain-empty case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/ingest.go`:
- Around line 40-42: url.PathUnescape errors are currently ignored causing
malformed percent-encodings (e.g. "%ZZ") to be accepted; change the logic so
that when url.PathUnescape(table) returns an error you respond with HTTP 400 and
return immediately (following the same error-handling pattern used elsewhere in
this handler), e.g. check the error from url.PathUnescape and call the handler's
bad-request response path using the same mechanism as other checks instead of
falling back to the raw table value.
In `@internal/api/stream_ws.go`:
- Around line 54-58: The handler currently rejects connections when
r.URL.Query().Get("table") == "" which conflates a missing table parameter
(should be allowed) with an explicit empty value (?table=) (should be rejected);
change the check to inspect presence via r.URL.Query() so: detect whether the
"table" key exists and, if it exists, verify its first value is non-empty,
calling writeJSONError(w, http.StatusBadRequest, "invalid table name") only for
the explicit-empty case; allow the flow to continue when the "table" key is
absent so clients can connect with no initial subscriptions.
In `@tests/e2e/sdk/ingest.test.ts`:
- Around line 317-321: The test currently comments out assertions for the policy
rejection (variable badRes in tests/e2e/sdk/ingest.test.ts), leaving the
rejection path unverified; restore the assertion lines
(expect(badRes.error).not.toBeNull(); expect(badRes.error!.status).toBe(403);)
and ensure the test setup enforces the policy requiring country=US so the
request with country=GB returns the expected error, or if the policy behavior is
still broken, mark the test as skipped (test.skip) and open a follow-up issue to
re-enable it later; target the badRes usage and the commented expect lines to
implement this change.
---
Outside diff comments:
In `@internal/api/stream_test.go`:
- Line 207: Replace the error-path assertions that call
testutil.AssertJSONContains(t, w, http.StatusBadRequest, ...) with the
package-local helper assertJSONErrorResponse(t, w); specifically locate usages
of testutil.AssertJSONContains in internal/api/stream_test.go (e.g., the call
with map[string]any{"error": tc.errBody} and the other occurrence around the
noted range) and change them to assertJSONErrorResponse(t, w) so the test also
asserts Content-Type and X-Content-Type-Options headers for API error responses.
- Line 195: Replace the duplicated test case that currently uses an empty string
for the "empty after url decode" case with a percent-encoded value that decodes
to an empty/invalid value (for example "%00" or another encoding your URL
decoder treats as empty) so the test actually verifies the decode-to-empty
scenario; update the test case input (the second element of the tuple) from ""
to the chosen percent-encoded string and keep the expected error string
("missing required query parameter: table") or adjust it if your decoder returns
a different error for decoded-empty values, ensuring the tuple with description
"empty after url decode" is the one changed rather than duplicating the
plain-empty case.
In `@internal/ingest/bento_test.go`:
- Around line 210-240: Rename the test function
TestJsInput_Read_UnsafeTableNameDropped to reflect the new pass-through behavior
(e.g., TestJsInput_Read_UnsafeTableNamePassesThrough or
TestJsInput_Read_UnsafeTableNameNotDropped) and update any references or
comments; ensure the function signature (func
TestJsInput_Read_UnsafeTableNamePassesThrough(t *testing.T)) and the file's test
mentions match the new name, and keep assertions around jsInput.Read,
badMsg.doubleAcked, goodMsg.doubleAcked, and MetaGet checks unchanged.
In `@internal/ingest/bento.go`:
- Around line 86-94: The empty-table-name branch in jsInput.Read currently logs
and DoubleAcks (raw.TableName == "" -> slog.ErrorContext + m.DoubleAck) but does
not publish to the DLQ; change this to route the rejection through the same DLQ
used for unsafe-table-name by publishing the message to subject
"dlq.__rejected__" with header "x-wh-drop-reason" set to an appropriate reason,
then DoubleAck; to do this, ensure the jsInput struct has access to the
JetStream context (add a js field or otherwise pass js into jsInput.Read), use
that js context to PublishMsg/Publish with headers for the DLQ, and replace the
current slog.ErrorContext + m.DoubleAck-only flow in the raw.TableName == ""
branch with the DLQ publish + DoubleAck sequence mirroring the unsafe-table-name
handling.
In `@internal/query/builder_test.go`:
- Around line 26-40: Add a unit test that verifies Build correctly escapes
backticks in table names: create TestBuild_TableNameWithBacktick which calls
Build("my`table", sq, testSchema(), 0) with a simple StructuredQuery (e.g.,
Columns: []string{"page"}, Limit: 10), require no error and assert the SQL
equals "SELECT page FROM `my``table` LIMIT 10"; ensure the testSchema used
contains a matching table Name or adjust/create a test schema entry so Build
accepts the table name.
In `@tests/integration/dlq_test.go`:
- Line 68: The DLQ subject is built from the raw table name (dlqSubject :=
"dlq." + table) but should use the encoded table like the ingest subject; update
the dlqSubject construction to use query.EncodeTable(table) (i.e., prefix "dlq."
to the encoded table value) so DLQ subjects consistently match the ingest
subject encoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7798eeb3-9051-4d8e-b247-5cc443b3ff87
📒 Files selected for processing (18)
Makefileinternal/api/ingest.gointernal/api/schema.gointernal/api/stream_sse.gointernal/api/stream_test.gointernal/api/stream_ws.gointernal/api/structured_query.gointernal/ingest/bento.gointernal/ingest/bento_test.gointernal/query/builder.gointernal/query/builder_test.gointernal/query/ident.gointernal/testutil/testutil.goscripts/orchestrator/main.gotests/e2e/compose.yamltests/e2e/sdk/ingest.test.tstests/e2e/sdk/query.test.tstests/integration/dlq_test.go
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a more flexible table name handling strategy by replacing strict regex validation with percent-encoding for NATS subjects and utilizing ClickHouse's identifier parameterization for SQL queries. It also enables event deduplication by default and adds extensive E2E tests for policy enforcement and special characters. Review feedback identifies a regression where the optional table parameter in the WebSocket handler was incorrectly made mandatory and emphasizes the need to maintain regex-based validation for table identifiers to satisfy security requirements. Furthermore, the reviewer recommended expanding test coverage to ensure that both empty parameters and invalid identifiers are handled correctly.
github-actions
Bot
added
the
dependencies
|
/gemini review |
coderabbitai
Bot
added
documentation
2 participants
This PR changes parts of our codebase to use the ClickHouse HTTP client (part of #171) in order to allow ANY legal ClickHouse table names (so basically anything) to use query parameters so that even clear SQL injections as a table name are allowed. Additionally, we change how table names are stored to be safe for NATS internally.
Due to the changes above, test coverage dropped below the threshold, so significant portions of this PR are to get it to the threshold again.
Related Issues
Closes #151, #167