From d8fb698f07141c1e48cab3404b4641dc6d8532f7 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 21 Aug 2017 16:24:51 +0200 Subject: [PATCH] SONAR-9283 harden Props against non trimmed strings --- .../main/java/org/sonar/process/Props.java | 25 +++-- .../java/org/sonar/process/PropsTest.java | 101 ++++++++++++------ 2 files changed, 88 insertions(+), 38 deletions(-) diff --git a/server/sonar-process/src/main/java/org/sonar/process/Props.java b/server/sonar-process/src/main/java/org/sonar/process/Props.java index b029fabd01d..fd9c896089f 100644 --- a/server/sonar-process/src/main/java/org/sonar/process/Props.java +++ b/server/sonar-process/src/main/java/org/sonar/process/Props.java @@ -19,13 +19,11 @@ */ package org.sonar.process; -import org.apache.commons.lang.StringUtils; - -import javax.annotation.CheckForNull; -import javax.annotation.Nullable; - import java.io.File; import java.util.Properties; +import javax.annotation.CheckForNull; +import javax.annotation.Nullable; +import org.apache.commons.lang.StringUtils; public class Props { @@ -43,7 +41,7 @@ public class Props { @CheckForNull public String value(String key) { - String value = properties.getProperty(key); + String value = valueImpl(key); if (value != null && encryption.isEncrypted(value)) { value = encryption.decrypt(value); } @@ -112,9 +110,22 @@ public class Props { } public void setDefault(String key, String value) { - String s = properties.getProperty(key); + String s = valueImpl(key); if (StringUtils.isBlank(s)) { properties.setProperty(key, value); } } + + @CheckForNull + private String valueImpl(String key) { + String value = properties.getProperty(key); + if (value == null) { + return null; + } + String trimmed = value.trim(); + if (trimmed.isEmpty()) { + return null; + } + return trimmed; + } } diff --git a/server/sonar-process/src/test/java/org/sonar/process/PropsTest.java b/server/sonar-process/src/test/java/org/sonar/process/PropsTest.java index 29e6acd4159..8c7d21a6567 100644 --- a/server/sonar-process/src/test/java/org/sonar/process/PropsTest.java +++ b/server/sonar-process/src/test/java/org/sonar/process/PropsTest.java @@ -19,47 +19,83 @@ */ package org.sonar.process; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.io.File; import java.io.IOException; import java.util.Properties; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; +@RunWith(DataProviderRunner.class) public class PropsTest { @Rule public TemporaryFolder temp = new TemporaryFolder(); + @Rule + public ExpectedException expectedException = ExpectedException.none(); @Test - public void value() { + @UseDataProvider("beforeAndAfterBlanks") + public void value(String blankBefore, String blankAfter) { Properties p = new Properties(); - p.setProperty("foo", "bar"); + p.setProperty("foo", blankBefore + "bar" + blankAfter); + p.setProperty("blank", blankBefore + blankAfter); Props props = new Props(p); assertThat(props.value("foo")).isEqualTo("bar"); assertThat(props.value("foo", "default value")).isEqualTo("bar"); + assertThat(props.value("blank")).isNull(); + assertThat(props.value("blank", "default value")).isEqualTo("default value"); assertThat(props.value("unknown")).isNull(); assertThat(props.value("unknown", "default value")).isEqualTo("default value"); + } + + @Test + @UseDataProvider("beforeAndAfterBlanks") + public void nonNullValue(String blankBefore, String blankAfter) { + Properties p = new Properties(); + p.setProperty("foo", blankBefore + "bar" + blankAfter); + Props props = new Props(p); assertThat(props.nonNullValue("foo")).isEqualTo("bar"); - try { - props.nonNullValue("other"); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("Missing property: other"); - } } @Test - public void valueAsInt() { + public void nonNullValue_throws_IAE_on_non_existing_key() { + Props props = new Props(new Properties()); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Missing property: other"); + + props.nonNullValue("other"); + } + + @Test + @UseDataProvider("beforeAndAfterBlanks") + public void nonNullValue_throws_IAE_on_existing_key_with_blank_value(String blankBefore, String blankAfter) { Properties p = new Properties(); - p.setProperty("foo", "33"); - p.setProperty("blank", ""); + p.setProperty("blank", blankBefore + blankAfter); + Props props = new Props(p); + + expectedException.expect(IllegalArgumentException.class); + + props.nonNullValue("blank"); + } + + @Test + @UseDataProvider("beforeAndAfterBlanks") + public void valueAsInt(String blankBefore, String blankAfter) { + Properties p = new Properties(); + p.setProperty("foo", blankBefore + "33" + blankAfter); + p.setProperty("blank", blankBefore + blankAfter); Props props = new Props(p); assertThat(props.valueAsInt("foo")).isEqualTo(33); @@ -71,9 +107,10 @@ public class PropsTest { } @Test - public void valueAsInt_not_integer() { + @UseDataProvider("beforeAndAfterBlanks") + public void valueAsInt_not_integer(String blankBefore, String blankAfter) { Properties p = new Properties(); - p.setProperty("foo", "bar"); + p.setProperty("foo", blankBefore + "bar" + blankAfter); Props props = new Props(p); try { @@ -85,28 +122,20 @@ public class PropsTest { } @Test - public void booleanOf() { + @UseDataProvider("beforeAndAfterBlanks") + public void booleanOf(String blankBefore, String blankAfter) { Properties p = new Properties(); - p.setProperty("foo", "True"); - p.setProperty("bar", "false"); + p.setProperty("foo", blankBefore + "True" + blankAfter); + p.setProperty("bar", blankBefore + "false" + blankAfter); Props props = new Props(p); assertThat(props.valueAsBoolean("foo")).isTrue(); assertThat(props.valueAsBoolean("bar")).isFalse(); + assertThat(props.valueAsBoolean("foo", false)).isTrue(); + assertThat(props.valueAsBoolean("bar", true)).isFalse(); assertThat(props.valueAsBoolean("unknown")).isFalse(); - } - - @Test - public void booleanOf_default_value() { - Properties p = new Properties(); - p.setProperty("foo", "true"); - p.setProperty("bar", "false"); - Props props = new Props(p); - assertThat(props.valueAsBoolean("unset", false)).isFalse(); assertThat(props.valueAsBoolean("unset", true)).isTrue(); - assertThat(props.valueAsBoolean("foo", false)).isTrue(); - assertThat(props.valueAsBoolean("bar", true)).isFalse(); } @Test @@ -162,4 +191,14 @@ public class PropsTest { assertThat(e).hasMessage("Property other_path is not set"); } } + + @DataProvider + public static Object[][] beforeAndAfterBlanks() { + return new Object[][] { + {"", ""}, + {" ", ""}, + {"", " "}, + {" ", " "}, + }; + } } -- 2.39.5