wenzhenghu commented on code in PR #66006:
URL: https://github.com/apache/doris/pull/66006#discussion_r3654108622
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2390,6 +2394,40 @@ public String getOriginStmtInString() {
return "";
}
+ private String getStmtForLogging(String stmt) {
+ if (stmt == null || !(parsedStmt instanceof LogicalPlanAdapter)) {
+ return stmt;
+ }
+ // Internal export outfile tasks use an empty origin SQL, so audit
masking must skip reparsing here.
+ if (stmt.isEmpty()) {
+ return stmt;
+ }
+ LogicalPlan logicalPlan = ((LogicalPlanAdapter)
parsedStmt).getLogicalPlan();
+ if (!(logicalPlan instanceof NeedAuditEncryption)) {
+ return stmt;
+ }
+ return ((NeedAuditEncryption) logicalPlan).geneEncryptionSQL(stmt);
+ }
+
+ private String getStmtForLoggingBeforeParse() {
+ if (originStmt == null || originStmt.originStmt == null) {
+ return null;
+ }
+ // Empty SQL cannot produce a valid parse tree for audit masking, so
keep the original text.
+ if (originStmt.originStmt.isEmpty()) {
+ return originStmt.originStmt;
+ }
+ try {
+ LogicalPlan logicalPlan = new
NereidsParser().parseSingle(originStmt.originStmt);
+ if (!(logicalPlan instanceof NeedAuditEncryption)) {
+ return originStmt.originStmt;
+ }
+ return ((NeedAuditEncryption)
logicalPlan).geneEncryptionSQL(originStmt.originStmt);
+ } catch (Exception e) {
+ return originStmt.originStmt;
Review Comment:
Thanks, agreed. For this PR scope, returning the raw statement on pre-parse
masking failure is not acceptable because it can leak ai.api_key. The pre-parse
path now fails closed to /* masked statement unavailable */ instead of falling
back to the original SQL, and there is a unit test for the parse-failure case.
Fixed in 3207a315876.
##########
fe/fe-core/src/test/java/org/apache/doris/qe/StmtExecutorTest.java:
##########
@@ -472,4 +484,74 @@ public void
testShouldDisableCloudVersionCacheOnRetryForE230() {
connectContext.getSessionVariable().cloudTableVersionCacheTtlMs =
originalTableTtl;
}
}
+
+ @Test
+ public void testEmptyOriginStmtSkipsAuditMaskingReparse() throws Exception
{
+ org.apache.doris.nereids.trees.plans.logical.LogicalPlan logicalPlan =
Mockito.mock(
+ org.apache.doris.nereids.trees.plans.logical.LogicalPlan.class,
+ Mockito.withSettings().extraInterfaces(
+
org.apache.doris.nereids.trees.plans.commands.NeedAuditEncryption.class));
+ Mockito.doThrow(new AssertionError("empty SQL should not trigger audit
masking reparse"))
+
.when((org.apache.doris.nereids.trees.plans.commands.NeedAuditEncryption)
logicalPlan)
+ .geneEncryptionSQL("");
+
+ org.apache.doris.analysis.StatementBase parsedStmt = new
org.apache.doris.nereids.glue.LogicalPlanAdapter(
+ logicalPlan, new org.apache.doris.nereids.StatementContext());
+ parsedStmt.setOrigStmt(new OriginStatement("", 0));
+ StmtExecutor executor = new StmtExecutor(connectContext, parsedStmt);
+
+ // Empty internal SQL must bypass audit masking reparsing in both
logging paths.
+ Method getStmtForLogging =
StmtExecutor.class.getDeclaredMethod("getStmtForLogging", String.class);
+ getStmtForLogging.setAccessible(true);
+ Assertions.assertEquals("", getStmtForLogging.invoke(executor, ""));
+
+ Method getStmtForLoggingBeforeParse =
StmtExecutor.class.getDeclaredMethod("getStmtForLoggingBeforeParse");
+ getStmtForLoggingBeforeParse.setAccessible(true);
+ Assertions.assertEquals("",
getStmtForLoggingBeforeParse.invoke(executor));
+ }
+
+ @Test
+ public void testNeedAuditEncryptionStatementLogsMaskedSql() throws
Exception {
+ boolean originalPrintRequest =
Config.enable_print_request_before_execution;
+ Config.enable_print_request_before_execution = true;
+ try (TestLogAppender appender =
TestLogAppender.attach(StmtExecutor.class)) {
+ connectContext.getState().reset();
+ StmtExecutor stmtExecutor = new StmtExecutor(connectContext,
CREATE_AI_RESOURCE_SQL);
+ stmtExecutor.execute();
+
+
Assertions.assertFalse(appender.contains(org.apache.logging.log4j.Level.INFO,
"sk-test-secret"));
+
Assertions.assertTrue(appender.contains(org.apache.logging.log4j.Level.INFO,
"*XXX"));
+ } finally {
+ Config.enable_print_request_before_execution =
originalPrintRequest;
+ }
+ connectContext.getState().reset();
+ StmtExecutor showExecutor = new StmtExecutor(connectContext, "");
+ showExecutor.execute();
+ Assertions.assertEquals(QueryState.MysqlStateType.OK,
connectContext.getState().getStateType());
+ }
+
+ @Test
+ public void testAlterResourceSuccessLogDoesNotPrintResourceObject() throws
Exception {
+ createResource(CREATE_AI_RESOURCE_SQL);
Review Comment:
Thanks. This is a test-isolation concern rather than the credential-leak
issue this PR targets, so it does not affect the scoped problem being fixed
here. The current test revision also no longer shares a fixed resource name.
--
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]
