xiangfu0 commented on code in PR #18411:
URL: https://github.com/apache/pinot/pull/18411#discussion_r3208543142
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -461,19 +493,35 @@ public String validateConfig(String tableConfigsStr,
@QueryParam("validationTypesToSkip") @Nullable String typesToSkip,
@Context HttpHeaders httpHeaders,
@Context Request request) {
Pair<TableConfigs, Map<String, Object>> tableConfigsAndUnrecognizedProps;
+ JsonNode tableConfigsJson;
try {
tableConfigsAndUnrecognizedProps =
JsonUtils.stringToObjectAndUnrecognizedProperties(tableConfigsStr,
TableConfigs.class);
+ tableConfigsJson = JsonUtils.stringToJsonNode(tableConfigsStr);
} catch (IOException e) {
throw new ControllerApplicationException(LOGGER,
String.format("Invalid TableConfigs json string: %s. Reason: %s",
tableConfigsStr, e.getMessage()),
Review Comment:
This still reflects the entire `tableConfigsStr` into both the 400 response
and the controller log line (`ControllerApplicationException` logs 4xx
messages). `TableConfigs` payloads can carry stream credentials, so a malformed
`/tableConfigs/validate` request now leaks secrets into logs. Please match the
`addConfig()` scrub here and keep only `e.getMessage()`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -837,9 +873,11 @@ public ObjectNode checkTableConfig(String tableConfigStr,
@QueryParam("validationTypesToSkip") @Nullable String typesToSkip,
@Context HttpHeaders httpHeaders,
@Context Request request) {
Pair<TableConfig, Map<String, Object>>
tableConfigAndUnrecognizedProperties;
+ JsonNode tableConfigJson;
try {
tableConfigAndUnrecognizedProperties =
JsonUtils.stringToObjectAndUnrecognizedProperties(tableConfigStr,
TableConfig.class);
+ tableConfigJson = JsonUtils.stringToJsonNode(tableConfigStr);
} catch (IOException e) {
String msg = String.format("Invalid table config json string: %s.
Reason: %s", tableConfigStr, e.getMessage());
Review Comment:
Same leak on `/tables/validate`: parse failures log the full raw table
config via `ControllerApplicationException`, including any stream config
credentials in the request body. The new PR already scrubbed similar error
paths elsewhere, so this validate endpoint needs the same treatment.
--
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]
