morningman opened a new pull request, #70366: URL: https://github.com/apache/doris/pull/70366
### Background Several FE features rewrite the text of a statement by cutting the original SQL string at token positions from the parser. Audit masking replaces credential properties and passwords with `*XXX` before the statement reaches `fe.audit.log`, `__internal_schema.audit_log` and the FE log; CREATE / ALTER VIEW and CREATE MATERIALIZED VIEW expand `*` and qualify names in the definition they store; the parser reads `/*+ ... */` hints from comment tokens. The lexer counts its input by Unicode code point, while a Java `String` holds a supplementary character, such as an emoji, as two `char`s. ### What problem does this PR solve? Issue Number: None Related PR: #47595 (introduced the bug in audit masking), #32743 (introduced the bug in view rewriting), #70338 (its review found the masking leak) PR Type: Bugfix Problem Summary: After each supplementary character in a statement, these token positions point one `char` too early, so the text is cut at the wrong place. A credential after or inside such characters stays partly or fully in clear text in the audit log, CREATE VIEW fails with a syntax error, CREATE MATERIALIZED VIEW stores a broken query, and a hint after such a character is silently ignored. This PR converts the positions to `String` offsets, or reads the text through the token, at the five places that cut the original SQL. Tests are 89% of the diff. ### Details - **Change map:** production code +33 -33 in 3 files; tests +240 in 5 files (FE unit tests and one regression suite); generated `.out` +16. (1) `BaseViewInfo.rewriteSql`, shared by audit masking, views and MTMV, converts its positions; the sync-MV builder drops its private copy of it, and `rewriteProjectsToUserDefineAlias` replaces the select list through it. (2) `NereidsParser.getHintMap` reads hints from the token text. (3) `NereidsParser.tryParseExplainPlan` converts the start of the EXPLAIN PLAN body. - **Frequency:** Always, when a character above U+FFFF (an emoji, CJK Extension B) comes before or inside the rewritten part, for example a catalog COMMENT of emoji in front of `'s3.secret_key' = 'secret'`. - **Root cause:** `CaseInsensitiveStream.fromString` keeps ANTLR's code-point stream for any input with a surrogate (before #67452 every input used `CharStreams.fromString`), so `Token.getStartIndex()` and `getStopIndex()` count code points. The five places in the Change map used them as `String` indexes. - **Why existing tests missed it:** Unit tests: the masking (`EncryptSQLTest`, `RepositoryAuditEncryptionTest`), view and MTMV (`CreateViewTest`, `CreateMTMVCommandTest`) and hint tests (`LeanTokenModeTest`) use only BMP text, where both kinds of position coincide; `CaseInsensitiveStreamTest` uses an emoji but checks only the stream's own positions. Regression: `audit/test_external_catalog_credential_masking` and the `ddl_p0/create_view_nereids` suites have no supplementary characters, and the masking suite only checks that the whole credential is absent, which a leaked tail passes. - **Fix:** Convert where the text is cut. `rewriteSql` converts each position with `String.offsetByCodePoints`, walking the sorted positions once so the cost stays linear; `rewriteProjectsToUserDefineAlias` reads expressions through the token's input stream; `getHintMap` reads the hint from the token text, as `removeCommentAndTrimBlank` does. Changing the stream to report `String` positions was rejected: #67452 made code-point positions part of `CaseInsensitiveStream`'s contract, and `CaseInsensitiveStreamTest` pins them. - **Other paths and branches:** Fixed here: the 19 masking visitors of `LogicalPlanBuilderForEncryption` with every caller of `geneEncryptionSQL` (audit log, FE statement log, `_query_plan`) and the streaming insert job; the view, MTMV and sync-MV rewrites. Not affected: the other uses of token positions read text through the token's own stream (such as `getOriginSql`) or only compare positions with each other. branch-3.1, branch-4.0, branch-4.1 and branch-4.2 have the same code. Introduced by #32743 (views), #47595 (masking), #33699 (sync MV) and #33262 (EXPLAIN PLAN); hints since at least #39590. ### Testing - New unit tests fail on master `f1bc8b3690c` and pass with the fix, run locally: `EncryptSQLTest#testEncryptionAfterSupplementaryCharacters`, through `MysqlConnectProcessor` to the audit event (emoji inside a credential, in a catalog COMMENT, in the first of two property clauses, in a user name before `PASSWORD(...)`; master left `secret`, `abc`, `123` and `123456` in clear text); `CreateViewTest#testCreateViewRewritesAfterSupplementaryCharacters` (master: `mismatched input '`e`'`); `CreateMTMVCommandTest#testCreateMTMVRewritesQuerySqlAfterSupplementaryCharacters` (master kept ``SELECT '😀😀' A`e` e`internal`...``); `SupplementaryCharacterOffsetTest` (master dropped the hint, cut the EXPLAIN PLAN body to `AN SELECT 100`, and rewrote the sync MV to `avg_stateg(k)))`). - New regression suite `ddl_p0/create_view_nereids/test_create_view_supplementary_chars`: fails without the fix with the same syntax error and passes with it; the `.out` is generated by the script. Run locally on one FE and one BE, an older master FE build with the three changed classes swapped in. - 27 existing FE UT classes around the change (467 tests) pass. Reverting each part of the fix alone, moving the end of a replaced span by one, or not advancing the position cursor each fails a new test; the off-by-one also fails existing ASCII cases. - By hand on that cluster, without the fix: the secret stayed in `audit_log`, `/* 😀 */ select /*+ SET_VAR(query_timeout=1) */ sleep(3)` ran for 3 s, and an MTMV over `select '😀😀' as e, ...` was created but failed to refresh (`Nereids parse failed`). With the fix: `"s3.secret_key" = "*XXX")`, and the query times out after 1 s. ### Notes for Reviewers - `getHintMap` loses its `sql` parameter, which nothing reads any more; its three callers are in `NereidsParser`. - `rewriteProjectsToUserDefineAlias` replaces the select list through `rewriteSql` instead of `StringUtils.overlay`, so the conversion lives in one method. - Stored definitions are not repaired: an MTMV created by an affected version from such a query keeps its broken definition and has to be recreated. ### Release note Fix passwords and credentials left partly or fully unmasked in the audit log and the FE log when a supplementary character, such as an emoji, comes before or inside them. Also fix CREATE VIEW failing, CREATE MATERIALIZED VIEW storing a broken query, and hints such as SET_VAR being ignored after such a character; materialized views already created with a broken query must be recreated. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the fix, the tests, and this description - Human review: The author reviewed part of the changes. - Prompts: After the review bot on #70338 reported the masking leak, the author confirmed it is an existing engine defect shared by all masking paths and asked for a separate engine PR that fixes it, followed by a reply on #70338 with the link. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
