Copilot commented on code in PR #19236:
URL: https://github.com/apache/pinot/pull/19236#discussion_r3791401268
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -119,14 +125,21 @@ public String toJsonString(Object object) {
private JsonNode toJsonRecursive(JsonNode node) {
if (node.isObject()) {
node.fieldNames().forEachRemaining(field -> {
- if (_patterns.stream().anyMatch(pattern ->
pattern.matcher(field).matches())) {
+ JsonNode fieldValue = node.get(field);
+ boolean preservePlaceholder = _useDefaultCredentialPolicy &&
fieldValue.isTextual()
+ &&
TableConfigRedactionUtils.isUnresolvedPlaceholder(fieldValue.textValue());
Review Comment:
This preservation check is too broad because `isUnresolvedPlaceholder`
accepts any string that starts with `${` and ends with `}`. For example,
`${IGNORED} https://user:[email protected]}` is treated as one placeholder
and returned verbatim, bypassing the structured URI/assignment redaction added
below. Require the entire value to be exactly one syntactically valid
placeholder before skipping redaction, and add a mixed-placeholder/text
regression test.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java:
##########
@@ -65,8 +65,8 @@ public static <T extends BaseJsonConfig> T
applyConfigWithEnvVariablesAndSystemP
return (T) JsonUtils.jsonNodeToObject(jsonNode,
configTemplate.getClass());
} catch (IOException e) {
throw new RuntimeException(String
- .format("Unable to read JsonConfig to class [%s] after applying
environment variables, jsonConfig is: '%s'.",
- configTemplate.getClass().getName(), jsonNode.toString()), e);
+ .format("Unable to read JsonConfig to class [%s] after applying
environment variables.",
+ configTemplate.getClass().getName()), e);
Review Comment:
The resolved JSON body is no longer in the outer message, but the Jackson
`IOException` is still retained as the cause and can include the rejected value
(for example, an invalid numeric/enum value). The new test does not cover this
because the secret maps to a valid `String` while a separate ordinary value
triggers the failure. Sanitize or omit the cause as the other diagnostic
changes in this PR do, and make the failing value itself the secret sentinel in
the regression test.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -66,11 +68,14 @@ public final class Obfuscator {
private final String _maskedValue;
private final List<Pattern> _patterns;
+ private final boolean _useDefaultCredentialPolicy;
/// Obfuscator with default behavior matching (ignore case) "secret",
"password", and "token" suffixes. Masks any
/// values with '\*\*\*\*\*'
public Obfuscator() {
- this(DEFAULT_MASKED_VALUE, DEFAULT_PATTERNS);
+ _maskedValue = DEFAULT_MASKED_VALUE;
+ _patterns = DEFAULT_PATTERNS;
+ _useDefaultCredentialPolicy = true;
Review Comment:
The public constructor documentation above still says the default only
matches `secret`, `password`, and `token` suffixes, but this constructor now
also applies the shared sensitive-name and structured-text/URI policy. Update
the Javadoc so callers understand that ordinary textual leaves may now be
transformed as well.
--
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]
