fix query route safe clickhouse error codes#1268
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughUpdates ClickHouse error classification by marking codes 43 and 47 as unsafe for analytics query handling and adds end-to-end tests covering ILLEGAL_TYPE_OF_ARGUMENT and UNKNOWN_IDENTIFIER error cases. No exported signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR expands the set of "safe" ClickHouse error codes in the internal analytics query route by adding codes Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /analytics/query] --> B[Execute ClickHouse Query]
B -->|Success| C[Return rows + query_id]
B -->|Error| D[getSafeClickhouseErrorMessage]
D --> E{Is error code in\nSAFE_CODES?}
E -->|Yes - codes 43,47,62,\n159,164,158,396,636| F[Return raw ClickHouse message]
E -->|No| G{Is error code in\nUNSAFE_CODES?}
G -->|Yes - codes 36,60,497| H[Return generic message]
G -->|No - unknown code| I[captureError + generic message]
H --> J[Throw KnownErrors.AnalyticsQueryError]
F --> J
I --> J
Last reviewed commit: "fix query route safe..." |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts (1)
1548-1579: Consider adding an explicit production-path masking assertion for code 47.This test currently validates development-mode output. A focused follow-up test for production-mode behavior would lock in the key security guarantee (generic masked error for unsafe codes).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts` around lines 1548 - 1579, Add a sibling test to "does not leak column names from restricted tables via unknown identifier (code 47)" that invokes runQuery (or the same test helper) with production-mode semantics and asserts the response is a generic masked ANALYTICS_QUERY_ERROR without the ClickHouse suggestion text ("Maybe you meant" or real column names); use the same query ("SELECT qurey FROM system.query_log"), reuse stripQueryId(expect) for normalization, and assert the body.error/body.details do not contain "Maybe you meant" or "query" but instead contain the generic masked message expected in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts`:
- Around line 1548-1579: Add a sibling test to "does not leak column names from
restricted tables via unknown identifier (code 47)" that invokes runQuery (or
the same test helper) with production-mode semantics and asserts the response is
a generic masked ANALYTICS_QUERY_ERROR without the ClickHouse suggestion text
("Maybe you meant" or real column names); use the same query ("SELECT qurey FROM
system.query_log"), reuse stripQueryId(expect) for normalization, and assert the
body.error/body.details do not contain "Maybe you meant" or "query" but instead
contain the generic masked message expected in production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90f3ba1a-6841-457a-b188-0eed9f3f1b33
📒 Files selected for processing (2)
apps/backend/src/app/api/latest/internal/analytics/query/route.tsapps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts
N2D4
approved these changes
Mar 21, 2026
N2D4
approved these changes
Mar 21, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts (1)
1558-1622: Add one production-path assertion for the no-leak guarantee.These cases currently assert test-mode behavior (which still includes full ClickHouse text). Please add a production-mode assertion (or a route-level unit test around
getSafeClickhouseErrorMessage) verifying codes 43/47 return only the default sanitized message and do not include query/table/column details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts` around lines 1558 - 1622, Tests assert development-mode ClickHouse error text leakage for codes 43 and 47 but lack a production-path assertion that getSafeClickhouseErrorMessage sanitizes those errors; add a new assertion that simulates production mode (or calls getSafeClickhouseErrorMessage directly) for the two cases (arrayJoin/code 43 and unknown identifier/code 47) and verify the returned message is the default sanitized message without any query/table/column details, referencing the test cases in analytics-query.test.ts and the helper function getSafeClickhouseErrorMessage to locate where to add the production-mode check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts`:
- Around line 1558-1622: Tests assert development-mode ClickHouse error text
leakage for codes 43 and 47 but lack a production-path assertion that
getSafeClickhouseErrorMessage sanitizes those errors; add a new assertion that
simulates production mode (or calls getSafeClickhouseErrorMessage directly) for
the two cases (arrayJoin/code 43 and unknown identifier/code 47) and verify the
returned message is the default sanitized message without any query/table/column
details, referencing the test cases in analytics-query.test.ts and the helper
function getSafeClickhouseErrorMessage to locate where to add the
production-mode check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b7be848-901a-4bd7-b58e-58c5ed340a52
📒 Files selected for processing (2)
apps/backend/src/app/api/latest/internal/analytics/query/route.tsapps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/app/api/latest/internal/analytics/query/route.ts
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Tests