feat(server-core): Redact query values in log events - #11854
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
github-actions
Bot
added
docs
|
Claude finished @MazterQyou's task in 7m 11s —— View job Review complete — solid, well-tested design; 1 high, 3 medium, 5 low. 9 inline comments posted.Full review
What's goodThe design choice that makes this work is putting the wrapper outermost, so the telemetry, dev-server and agent wrappers all receive the redacted copy — and stamping Findings
Test gaps worth closing: a backslash-escaped literal (#2), non-plain-object Not verifiedI could not run the test suite or the linter — |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11854 +/- ##
==========================================
+ Coverage 80.37% 80.40% +0.02%
==========================================
Files 500 501 +1
Lines 106573 106780 +207
Branches 3885 3908 +23
==========================================
+ Hits 85658 85856 +198
- Misses 20369 20376 +7
- Partials 546 548 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3ef78db to
9ff783c
Compare
|
Claude finished @MazterQyou's task in 6m 14s —— View job Re-review of the updated design — 1 high, 2 medium, 2 low; 4 new inline comments, 8 prior threads resolved, 2 re-affirmedFull review
What changed since the last round, and what it fixedThe redesign is the right one. Moving SQL redaction out of the hand-rolled JS lexer and into Resolved 8 of my 10 prior threads as addressed. Two remain open and still apply — I have not re-posted them:
New findings
Test gaps that follow from the above: no Rust case for a non- Not verified
|
9ff783c to
27d7c68
Compare
|
Claude finished @MazterQyou's task in 7m 19s —— View job Re-review of the redesign — 1 high, 2 medium, 2 low; 5 new inline comments, 4 prior threads resolved, 2 re-affirmed
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | High | shim.rs:467 |
Cube SQL Error bypasses redaction entirely. handle_connection_error → SessionLogger::error → NodeBridgeLogger → apiGateway.log carries props["query"], built from the raw statement in query_engine.rs:216/:244 and parser_pg.rs:281. The JS lexer used to cover this incidentally; nothing does now. Same comment: SensitiveDataSanitizer is the weaker predecessor of redact_value and the two now coexist, with the misleadingly-named sanitizedQuery prop next to the unredacted one. |
| 2 | Med | config/mod.rs:219 |
env_optparse panics on a value bool::from_str rejects, and this is the first Rust read of CUBEJS_DEV_MODE. CUBEJS_DEV_MODE=TRUE works today (JS asBoolStrict lower-cases) and now crashes cubesql at startup; CUBEJS_LOG_REDACTION=1 likewise. |
| 3 | Med | statement.rs:887 |
visit_expressions_mut reaches ast::Value inside expressions only; literals stored as bare String (ShowStatementFilter::Like, Statement::Comment) are not covered, while the docstring says "every string literal, in every statement kind". Also: value-level redaction takes syntax strings too (DATE_TRUNC('redacted', …), INTERVAL 'redacted'), which is safe but undocumented. |
| 4 | Low | PreAggregationLoader.ts:55 |
Guard and destructure disagree on shape: isFetchTablesKey accepts a string head, const [[sql]] then yields the string's first character. Unreachable under the local QueryKey type — which also makes the branch dead, as its predecessor was. |
| 5 | Low | log-redaction.ts:4 |
The module header states "Cube generates it with placeholders" as a fact; unsafeValue() inlining and date ranges make it a constraint, and finding 1 is the case where the other half of the sentence does not hold either. |
Not a finding, just noted: redact_sql_literals parses each statement a second time purely for logging, on the Query/Parse hot path. Cheap next to planning, but it is now per-message and unconditional when redaction is on — worth one measurement on a wide BI query.
Test gaps that follow: no case for the Cube SQL Error props path (finding 1), none for a SHOW … LIKE filter (finding 3), and none for PreAggregationLoader's introspection branch (finding 4, though the honest fix there is to delete it).
Not verified
yarn, node -e and cargo are all outside this session's allowed commands and the workspace has no node_modules, so nothing here was executed — log-redaction.test.ts, index.test.ts and test_redact_sql_literals were reviewed by reading. Finding 1 was derived by tracing the call chain (shim.rs:557 → telemetry/mod.rs:84 → logger.rs:33 → sql-server.ts:50) and the two with_meta sites; worth confirming with a deliberately unsupported statement against a local instance before acting. Finding 2 rests on env-var's asBoolStrict lower-casing its input, which I could not run to confirm. The serde_json exponent question above is unresolved for the same reason.
8d131ea to
46ae5cd
Compare
|
Claude finished @MazterQyou's task in 4m 58s —— View job Re-review — 1 high; 1 inline comment, both of my prior threads resolved
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | High | gateway.ts:576 |
The twin is built from req.body.query destructured at line 548, before cubeSqlRequestSchema.validate — and this runs in the catch that validation failure lands in. {} or {"query": 42} reaches cx.argument::<JsString>(0)?, which throws out of the catch; userAsyncHandler's .catch(next) turns a 400 Invalid query format into a 500 and the User Error event is never logged. On by default in production. Same shape when loadNative() itself throws. |
Folded into that comment: log-redaction.test.ts:32 pins the drop using exactly the payload that now has a twin, so the fixture no longer describes a live producer.
What I checked and found sound
- Every SQL API event still carries its twin, and only where it belongs. Both
SpanId::newsites chainwith_redacted_query_key(shim.rs:459-466,node_export.rs:316-323);transport.rs:681-688insertsredactedQueryonly when the payload has aquery, and everylog_load_statesite now passesspan_id.query_key—shim.rs:296/394/558/2035/2062,query_engine.rs:113/298,node_export.rs:294/342/606/636.grep '"sql":'acrosscubesqlandcubejs-backend-nativefinds no raw-SQL payload outside the two span constructors and the tests. handle_connection_errorstates its condition once now (shim.rs:523-533): onequerybinding gated onlog_redaction(), onematchproducing both twins,log_propscarrying them whileerr_responsekeeps the originals for the client that sent the statement.redacted_errorisNonewhen props carry noquery, which is the data-source-error case the docs already scope out.redact_error_message's two needles — raw text first, then each statement re-printed — cover both entry points' shapes, with the lower-cased/comment-carrying fixture and the multi-statement parser error pinning the case that used to miss. An emptyquerymakescontains("")true but the replacement is empty too; a query with no literal returns its original text, so the replace is a no-op.- Wrapper placement.
withLogRedactionwraps the raw sink atserver.ts:201, beforeinitAgentand both telemetry wrappers, so the agent collector forwards the original and Query History keeps the statement as sent; the telemetry sites now readparams.redactedError ?? params.error(:303,:332), matching what they already did forsanitizedQuery. Nothing readsthis.options.loggerbehind the wrapper — the onlyoptions.loggercall sites (QueryCache.ts:883,QueryQueue.ts:137,CompilerApi.ts:148) all trace back tothis.logger. redact_literalsovervisit_expressions_mutreachesSET/INSERT/PREPARE/EXECUTEandExpr::TypedString, each with a case; returning the original text when nothing was redacted keeps comments and spacing (the/* Tableau */fixture). The docstring scopes itself to "every string literal an expression carries" and names theSHOW … LIKE/COMMENT ONgap.- Dev-mode parity between
env.ts:216andconfig/mod.rs:164is unchanged (NODE_ENV !== 'production' || CUBEJS_DEV_MODE),env_parse_boolwarns and defaults,Config::test()pinslog_redaction: false, and the threeNODE_ENVcases plus both explicit values are pinned inlog-redaction.test.ts:237-273. The twoindex.test.tscases cover the wrapper end to end under both modes. QueryCache.payloadForLogis non-mutating, handles theinlineTables === undefinedcase that is every non-lambda query, and covers all three spreads; thecsvRowsrule inlog-redaction.ts:78stays as the backstop.holdsValues/PARAMETER_KEYS,isSqlWithParamsdecliningorder/dateRange, the cycle and shared-reference memo, the class-instance copy and the non-mutation case all still hold on re-read.config.mdxnow tells a custom-loggerauthor what arrives redacted, which was the gap on that side.
Not verified
yarn / jest could not run (no node_modules in the checkout) and cargo is unavailable, so log-redaction.test.ts, index.test.ts and the Rust tests were reviewed by reading. The finding was derived by tracing gateway.ts:548-580 against cubeSqlRequestSchema (query.js:214-219) and userAsyncHandler (:125-129); curl -XPOST localhost:4000/cubejs-api/v1/cubesql -d '{}' with CUBEJS_LOG_REDACTION=true would show it directly.
• branch server-core/redact-query-values •
46ae5cd to
67eb193
Compare
|
Claude finished @MazterQyou's task in 5m 42s —— View job Re-review — 2 low; 2 inline comments, my last open thread resolved
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | gateway.ts:2509 |
The bare catch { return undefined } makes a permanently-unloadable native module indistinguishable from a malformed body: every /v1/cubesql error logs query: { sql: 'redacted' } forever with nothing saying why. One this.log line keeps the degradation observable without changing the outcome. |
| 2 | Low | environment-variables.mdx:1391, index.mdx:153 |
The docs name one exception (unparsed statement logged as received) but not the second outcome — a statement with no twin is dropped wholesale, not literal-replaced. That is the reachable case from finding 1 and from a malformed body. |
What I checked and found sound
- Every SQL API event still carries its twin. Both
SpanId::newsites chainwith_redacted_query_key(shim.rs:459-467,node_export.rs:316-324),redacted_query_key(sql, true)always returnsSome(an unparsed statement falls back to its own text), andtransport.rs:681-689insertsredactedQueryonly when the payload has aquery. Walked everylog_load_statesite —shim.rs:293/345/373/388/551/2028/2055,query_engine.rs:107/191/262/295,node_export.rs:289/337/601/650— and each one that carries a statement readsspan_id.query_key, so each gets its twin. The threeData Query Statusevents carry noqueryand correctly get none. - The two entry points now agree on errors.
shim.rs:523-533computes both twins from oneprops["query"]binding gated once onlog_redaction();SQL API ErrorgetsredactedErrorinline (:545) andredactedQueryfromtransport.rs;log_propscarries both intologger.errorwhileerr_response(:562-570) keeps the originals for the client that sent the statement.node_export.rs:634-645does the same for itsCube SQL Error, with the config resolved once per query. - Every
CompilationErrorthat quotes a statement has a needle. Checked the parser too:parser_pg.rs:281attachesquerymeta for the multi-statement case, and bothCompilationError::SqlParsersites (:211,:249) attachoriginal_query— so theUnable to parsesnippets fall inside the already-documented unparsed-statement exception rather than being a separate gap.redact_error_message's raw-then-reprinted needle covers both entry points' shapes; the lower-cased/comment-carrying fixture pins the case that used to miss. - Borrow shape in
handle_connection_error:queryis anOption<&String>intopropswhose borrow ends at thematch, so the laterprops.map(...)move is fine, and both earlierprops.clone()calls are gone. holdsValuesstill has its producer.PreAggregationLoaderis out of the diff now, andqueryOptions(:533) still setsqueryKeyMd5itself — so the'queryKeyMd5' in nodegate keys off something the producer owns, and the pre-aggregation build payload'svaluesare still reached.getCacheHashand the logged hash cannot drift, since nothing shared touches either.- The drop rule has no false-positive target. Grepped
packages/*/srcfor a loggedqueryobject carryingsql: onlygateway.ts:572.PreAggregationLoader.ts:901'squery: { sql, params }goes toclient.unload, not a logger.sanitizeQueryForLogging(undefined)yields{}, which has nosqland is untouched. QueryCache.payloadForLogis non-mutating (shallow spread, fresh object per table), handles theinlineTables === undefinedcase that is every non-lambda query, and covers all three spreads (:673,:741,:812); the queue key is[query, values]with noinlineTables, so the rows never reachQueryQueue's events either. ThecsvRowsrule inlog-redaction.ts:78stays as the backstop.- Config parity.
env.ts:216andconfig/mod.rs:164both computeNODE_ENV !== 'production' || CUBEJS_DEV_MODE, matchingOptsHandler.isDevMode();env_parse_boolwarns and defaults rather than panicking;Config::test()pinslog_redaction: false; and the native bridge buildsConfig::default()(config.rs:130), so both processes read the same env rather than one of them silently landing on a test default. - Wrapper placement.
withLogRedactionwraps the raw sink atserver.ts:201, beforeinitAgentand both telemetry wrappers, so the agent endpoint and Query History keep the statement as sent; the telemetry sites readparams.redactedError ?? params.error(:303,:332), matching what they already did forsanitizedQuery. redact_literalsovervisit_expressions_mutreachesSET/INSERT/PREPARE/EXECUTEandExpr::TypedString, each with a case; returning the original text when nothing was redacted keeps comments and spacing (the/* Tableau */fixture). The docstring scopes itself to "every string literal an expression carries" and names theSHOW … LIKE/COMMENT ONgap.- The test mock for
@cubejs-backend/nativeuses aProxyrather than a spread — correct, since the module's exports are non-enumerable getters, and the comment says exactly why.
Not verified
yarn / jest could not run (no node_modules in the checkout) and cargo is unavailable, so log-redaction.test.ts, index.test.ts and the Rust tests were reviewed by reading. Finding 1 is a code-path observation needing no runtime confirmation; finding 2 is editorial.
67eb193 to
8863cad
Compare
|
Claude finished @MazterQyou's task in 3m 49s —— View job Re-review — 2 medium, 1 low; 3 inline comments, both of my prior threads resolved
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Med | env.ts:224 |
The native gate turns off the half that never needed native. Filter values, values/params/query_values, [sql, params] tuples and csvRows are all pure JS; only the SQL API twin calls into the module — and on such a platform the SQL API cannot run at all (sql-server.ts:79-81, env.ts:2049). A production musl deployment now logs REST filter values in the clear by default. |
| 2 | Med | env.ts:226 |
isNativeSupported() → detectLibc() → spawnSync('getconf', …) (platform.ts:12), and getEnv does not memoise, so redactedSqlForLog forks a process synchronously on every /v1/cubesql error. defaultOn is also computed before the variable is read, so an explicit CUBEJS_LOG_REDACTION pays for it too. |
| 3 | Low | gateway.ts:2521 |
Redaction Unavailable fires per failing request rather than once, and carries no req.context, so it has no requestId tying it to the User Error beside it. |
What I checked and found sound
- The lazy native import is genuinely lazy.
redactSqlLiteralscallsloadNative()at call time (js/index.ts:383-387), andloadedNativeis cached, so importing it at the top ofgateway.tsdoes not force a load on an unsupported platform. That is what makes thetryinredactedSqlForLogthe right place for the failure. - The two new gateway tests cover the pair the last round was missing: a malformed body keeps its 400 and still logs
User Errorwith no twin, and a failing statement logsredactedQuerybeside the original. TheProxy-not-spread mock is correct for a module whose exports are non-enumerable getters, and the comment says exactly why. - The env matrix is complete: production, three dev-mode shapes, both explicit values, and the new unsupported-platform case with the force-on override, all in
log-redaction.test.ts:231-280.jest.spyOn(platform, 'isNativeSupported')does intercept, sinceenv.tsimports the binding rather than destructuring it at load. - Rust side unchanged and still sound from the previous rounds:
env_parse_boolwarns and defaults rather than panicking,dev_modematchesOptsHandler.isDevMode()(config/mod.rs:164-165againstenv.ts:223),Config::test()pinslog_redaction: false,transport.rs:681-689insertsredactedQueryonly when the payload has aquery, andredact_error_message's raw-then-reprinted needle covers both entry points' shapes. - The sink's contract holds.
withLogRedactionwraps the raw sink atserver.ts:201, beforeinitAgentand both telemetry wrappers, so the agent endpoint keeps the statement as sent; the telemetry sites readparams.redactedError ?? params.error(:303,:332), matching what they already did forsanitizedQuery. TheCOMPANIONSswap is top-level, which is where both twins arrive. holdsValues/PARAMETER_KEYS, thecsvRowsbackstop,QueryCache.payloadForLog, the cycle and shared-reference memo, the class-instance copy and the non-mutation case all still hold on re-read.
Threads
Resolved both of my own. The one open thread is @ovr's on log-redaction.ts — its subject (queryKeyMd5, canonicalJson, the getCacheHash divergence) is out of the diff entirely and PreAggregationLoader is untouched, but it is a human thread, so I left it to close.
Not verified
yarn / jest could not run (no node_modules in the checkout) and cargo is unavailable, so log-redaction.test.ts, the new index.test.ts cases and the Rust tests were reviewed by reading. Findings 1 and 2 were derived by tracing isNativeSupported / detectLibc and the getEnv('logRedaction') call sites (server.ts:201, gateway.ts:2513); a CUBEJS_LOG_REDACTION=true run with strace -f -e trace=clone on a few /v1/cubesql errors would show the spawns directly.
• branch server-core/redact-query-values •
8863cad to
ced365f
Compare
|
Claude finished @MazterQyou's task in 4m 15s —— View job Re-review — no new findings; all three of my prior threads resolved
|
ced365f to
71988da
Compare
|
Claude finished @MazterQyou's task in 3m 51s —— View job Re-review — 1 low; 1 inline comment, no threads of mine left open
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | gateway.ts:2517 |
Two of the three ways into that catch never touch native: assertApiScope (:556) and cubeSqlRequestSchema.validate (:558) throw before execSql. On a platform where loadNative() throws, those turn a clean 403 / 400 plus a logged User Error into a generic 500 with no event — the one line that would have named the missing module is the one that disappears. |
What I checked and found sound
- Every SQL API event carries its twin, and only where it belongs. Both
SpanId::newsites chainwith_redacted_query_key(shim.rs:459-466,node_export.rs:316-322),redacted_query_key(sql, true)always returnsSome(an unparsed statement falls back to its own text), andtransport.rs:681-689insertsredactedQueryonly when the payload already has aquery. Walked everylog_load_statesite —shim.rs:293/345/373/388/551/2028/2055,query_engine.rs:107/191/262/295,node_export.rs:289/337/601/650— and each one carrying a statement readsspan_id.query_key; theData Query Status/Plan Rewriteevents carry noqueryand correctly get no twin.grep '"sql":'acrosscubesqlandcubejs-backend-nativefinds no raw-SQL payload outside the two span constructors and the tests. handle_connection_errorstates its condition once (shim.rs:523-533): oneprops["query"]binding gated onlog_redaction(), onematchproducing both twins.SQL API ErrorgetsredactedErrorinline andredactedQueryfromtransport.rs;log_propscarries both intologger.error, whileerr_responsestill builds{message}\nQUERY: {query}from the originals for the client that sent the statement.redact_error_message's two needles — raw text first, then each statement re-printed — cover both entry points' shapes. An emptyquerymakescontains("")true but the replacement is empty too, soreplaceis a no-op; a query with no literal returns its own text, so the replace is a no-op; an unparsed query returns the message unchanged, the documented exception.redact_literalsovervisit_expressions_mutreachesSET/INSERT/PREPARE/EXECUTEandExpr::TypedString, each with a case intest_redact_sql_literals; returning the original text when nothing was redacted keeps comments and spacing (the/* Tableau */fixture). The docstring scopes itself to "every string literal an expression carries" and names theSHOW … LIKE/COMMENT ONgap.SensitiveDataSanitizeris gone in favour ofredacted_statement, sosanitizedQueryand the logged text are one pass.- Wrapper placement.
withLogRedactionwraps the raw sink atserver.ts:201, beforeinitAgentand both telemetry wrappers, so the agent endpoint keeps the statement as sent; the telemetry sites readparams.redactedError ?? params.error(:303,:332), matching what they already did forsanitizedQuery. - The drop rule has no false-positive target.
'sql' in querycatches the malformed-body shapes the type guard deliberately leaves untwinned, and the only producer of a loggedqueryobject carryingsqlisgateway.ts:572—PreAggregationLoader.ts:901'squery: { sql, params }goes toclient.unload, not a logger. - Config parity.
env.ts:216-226andconfig/mod.rs:164-165both computeNODE_ENV !== 'production' || CUBEJS_DEV_MODE, matchingOptsHandler.isDevMode(); the native gate is gone fromenv.ts, so the pure-JS half of redaction stays on where the module cannot load;env_parse_boolwarns and defaults rather than panicking;Config::test()pinslog_redaction: false. QueryCache.payloadForLogis non-mutating, handles theinlineTables === undefinedcase that is every non-lambda query, and covers all three spreads (:673,:741,:812); thecsvRowsrule inlog-redaction.ts:78stays as the backstop.holdsValues/PARAMETER_KEYS,isSqlWithParamsdecliningorder/dateRange, the cycle and shared-reference memo, the class-instance copy, theNODE_ENVmatrix and the two gateway tests (malformed body keeps its 400 and its event; a failing statement logsredactedQuerybeside the original) all still hold on re-read. Docs read as separate rules and now name both the agent endpoint and the "logged asredacted" outcome.
Threads
All of my own threads are resolved; nothing to resolve this round. The one open thread is @ovr's on log-redaction.ts — its subject (queryKeyMd5, canonicalJson, the getCacheHash divergence) is out of the diff entirely and PreAggregationLoader is untouched, but it is a human thread, so I left it to close.
Not verified
yarn / jest could not run (the checkout has no node_modules) and cargo is unavailable, so log-redaction.test.ts, the index.test.ts cases and the Rust tests were reviewed by reading. The finding was derived by reading the catch at gateway.ts:564-580 against loadNative() (js/index.ts:175-194) and userAsyncHandler; it needs no runtime confirmation.
• branch server-core/redact-query-values •
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
71988da to
0bcca51
Compare
|
Claude finished @MazterQyou's task in 1m 31s —— View job Re-review — no new findings; my last open thread is resolved
|
2 participants
Check List
Description of Changes Made
This PR redacts filter values, SQL parameters and SQL API string literals in log output, controlled by
CUBEJS_LOG_REDACTION, while events sent to Cube Cloud keep the original values.A SQL API statement is redacted in cubesql on the parsed statement (string literals replaced, numbers and placeholders kept) and attached beside the original as
redactedQuery; an error message that quotes the statement gets aredactedErrortwin. The log sink swaps the twins in. A statement that cannot be parsed has no twin and is logged as received.CUBEJS_LOG_REDACTIONdefaults to on, except in development mode (NODE_ENVnotproduction, orCUBEJS_DEV_MODE=true), where runnable SQL in the console is wanted. cubesql reads the same variable.Related tests are included.
Notes
redactedQueryandredactedError; unknown fields are ignored there. No change is needed in cube-runtime or Cube Cloud.sanitizedQueryin error telemetry now uses the same redaction pass: marker is'redacted'(was'[REPLACED]'), and short strings and'true'/'false'are no longer exempt.Executing SQLandStreaming SQLno longer log the rows of lambda inline tables, only their name and columns, regardless of the switch.