Skip to content

Commit

Permalink
[FLINK-24679][conf] Hide secret values from parser error messages
Browse files Browse the repository at this point in the history
  • Loading branch information
zentol authored Nov 21, 2022
1 parent 2eeb8fe commit 3d1c43c
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,11 @@ public <T> Optional<T> getOptional(ConfigOption<T> option) {
}
} catch (Exception e) {
throw new IllegalArgumentException(
String.format(
"Could not parse value '%s' for key '%s'.",
rawValue.map(Object::toString).orElse(""), option.key()),
GlobalConfiguration.isSensitive(option.key())
? String.format("Could not parse value for key '%s'.", option.key())
: String.format(
"Could not parse value '%s' for key '%s'.",
rawValue.map(Object::toString).orElse(""), option.key()),
e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ static Map<String, String> convertToProperties(Object o) {
pair -> {
if (pair.size() != 2) {
throw new IllegalArgumentException(
"Could not parse pair in the map " + pair);
"Map item is not a key-value pair (missing ':'?)");
}
})
.collect(Collectors.toMap(a -> a.get(0), a -> a.get(1)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,7 @@ private static Configuration loadYAMLResource(File file) {
+ file
+ ":"
+ lineNo
+ ": \""
+ line
+ "\"");
+ ": Line is not a key-value pair (missing space after ':'?)");
continue;
}

Expand All @@ -217,9 +215,7 @@ private static Configuration loadYAMLResource(File file) {
+ file
+ ":"
+ lineNo
+ ": \""
+ line
+ "\"");
+ ": Key or value was empty");
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@

package org.apache.flink.configuration;

import org.apache.flink.util.ExceptionUtils;
import org.apache.flink.util.InstantiationUtil;
import org.apache.flink.util.TestLogger;

import org.assertj.core.api.Assertions;
import org.junit.Test;

import java.time.Duration;
Expand All @@ -30,6 +32,7 @@
import java.util.Map;
import java.util.stream.Collectors;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.junit.Assert.assertArrayEquals;
Expand Down Expand Up @@ -455,6 +458,44 @@ public void testMapRemovePrefix() {
assertFalse(cfg.containsKey(MAP_PROPERTY_2));
}

@Test
public void testListParserErrorDoesNotLeakSensitiveData() {
ConfigOption<List<String>> secret =
ConfigOptions.key("secret").stringType().asList().noDefaultValue();

Assertions.assertThat(GlobalConfiguration.isSensitive(secret.key())).isTrue();

final Configuration cfg = new Configuration();
// missing closing quote
cfg.setString(secret.key(), "'secret_value");

assertThatThrownBy(() -> cfg.get(secret))
.isInstanceOf(IllegalArgumentException.class)
.satisfies(
e ->
Assertions.assertThat(ExceptionUtils.stringifyException(e))
.doesNotContain("secret_value"));
}

@Test
public void testMapParserErrorDoesNotLeakSensitiveData() {
ConfigOption<Map<String, String>> secret =
ConfigOptions.key("secret").mapType().noDefaultValue();

Assertions.assertThat(GlobalConfiguration.isSensitive(secret.key())).isTrue();

final Configuration cfg = new Configuration();
// malformed map representation
cfg.setString(secret.key(), "secret_value");

assertThatThrownBy(() -> cfg.get(secret))
.isInstanceOf(IllegalArgumentException.class)
.satisfies(
e ->
Assertions.assertThat(ExceptionUtils.stringifyException(e))
.doesNotContain("secret_value"));
}

// --------------------------------------------------------------------------------------------
// Test classes
// --------------------------------------------------------------------------------------------
Expand Down

0 comments on commit 3d1c43c

Please sign in to comment.