Skip to content
Permalink
Browse files
[CONFIGURATION-760] Properties file using cyclical includes cause a S…
…tackOverflowError instead of detecting the misconfiguration. (#35)
  • Loading branch information
garydgregory committed Sep 12, 2019
1 parent a396507 commit d3c40d779168c09cacc22481c7b5b1bb69cf6c4e
Show file tree
Hide file tree
Showing 9 changed files with 186 additions and 23 deletions.
@@ -72,6 +72,9 @@
<action dev="ggregory" type="update" issue="CONFIGURATION-759" due-to="Gary Gregory">
Update Spring from 4.3.24.RELEASE to 4.3.25.RELEASE.
</action>
<action dev="ggregory" type="fix" issue="CONFIGURATION-760" due-to="Gary Gregory">
Properties file using cyclical includes cause a StackOverflowError instead of detecting the misconfiguration.
</action>
</release>
<release version="2.5" date="2019-05-23"
description="Minor release with new features and updated dependencies.">
@@ -27,6 +27,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -643,11 +644,12 @@ public Object clone()
*
* @param key the property key
* @param value the property value
* @param seenStack the stack of seen include URLs
* @return a flag whether this is a normal property
* @throws ConfigurationException if an error occurs
* @since 1.3
*/
boolean propertyLoaded(final String key, final String value)
boolean propertyLoaded(final String key, final String value, final Deque<URL> seenStack)
throws ConfigurationException
{
boolean result;
@@ -661,7 +663,7 @@ boolean propertyLoaded(final String key, final String value)
getListDelimiterHandler().split(value, true);
for (final String f : files)
{
loadIncludeFile(interpolate(f), false);
loadIncludeFile(interpolate(f), false, seenStack);
}
}
result = false;
@@ -676,7 +678,7 @@ else if (StringUtils.isNotEmpty(getIncludeOptional())
getListDelimiterHandler().split(value, true);
for (final String f : files)
{
loadIncludeFile(interpolate(f), true);
loadIncludeFile(interpolate(f), true, seenStack);
}
}
result = false;
@@ -1853,9 +1855,11 @@ private static boolean needsUnescape(final char ch)
*
* @param fileName the name of the file to load
* @param optional whether or not the {@code fileName} is optional
* @param seenStack Stack of seen include URLs
* @throws ConfigurationException if loading fails
*/
private void loadIncludeFile(final String fileName, final boolean optional) throws ConfigurationException
private void loadIncludeFile(final String fileName, final boolean optional, final Deque<URL> seenStack)
throws ConfigurationException
{
if (locator == null)
{
@@ -1881,11 +1885,8 @@ private void loadIncludeFile(final String fileName, final boolean optional) thro

if (url == null)
{
if (getIncludeListener() != null)
{
getIncludeListener().accept(new ConfigurationException(
"Cannot resolve include file " + fileName, new FileNotFoundException(fileName)));
}
getIncludeListener().accept(new ConfigurationException("Cannot resolve include file " + fileName,
new FileNotFoundException(fileName)));
}
else
{
@@ -1896,14 +1897,25 @@ private void loadIncludeFile(final String fileName, final boolean optional) thro
{
try
{
fh.load(url);
// Check for cycles
if (seenStack.contains(url))
{
throw new ConfigurationException(
String.format("Cycle detected loading %s, seen stack: %s", url, seenStack));
}
seenStack.add(url);
try
{
fh.load(url);
}
finally
{
seenStack.pop();
}
}
catch (ConfigurationException e)
{
if (getIncludeListener() != null)
{
getIncludeListener().accept(e);
}
getIncludeListener().accept(e);
}
}
finally
@@ -19,6 +19,8 @@
import java.io.IOException;
import java.io.Reader;
import java.io.Writer;
import java.net.URL;
import java.util.ArrayDeque;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -138,6 +140,9 @@ public class PropertiesConfigurationLayout implements EventListener<Configuratio
/** Stores the force single line flag. */
private boolean forceSingleLine;

/** Seen includes. */
private final ArrayDeque<URL> seenStack = new ArrayDeque<>();

/**
* Creates a new, empty instance of {@code PropertiesConfigurationLayout}.
*/
@@ -486,7 +491,7 @@ public void load(final PropertiesConfiguration config, final Reader in)
while (reader.nextProperty())
{
if (config.propertyLoaded(reader.getPropertyName(),
reader.getPropertyValue()))
reader.getPropertyValue(), seenStack))
{
final boolean contained = layoutData.containsKey(reader
.getPropertyName());
@@ -653,6 +658,7 @@ private PropertyLayoutData fetchLayoutData(final String key)
*/
private void clear()
{
seenStack.clear();
layoutData.clear();
setHeaderComment(null);
setFooterComment(null);
@@ -49,6 +49,7 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
@@ -73,8 +74,8 @@
import org.apache.commons.configuration2.io.FileHandler;
import org.apache.commons.configuration2.io.FileSystem;
import org.apache.commons.lang3.mutable.MutableObject;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
@@ -1147,8 +1148,82 @@ public void testIncludeLoadAllOnLoadException() throws Exception
}

@Test
@Ignore("PropertiesConfiguration does NOT detect cyclical references.")
public void testIncludeLoadAllCycliclaReference() throws Exception
public void testIncludeLoadCyclicalReferenceFail() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
final FileHandler handler = new FileHandler(pc);
handler.setBasePath(testBasePath);
handler.setFileName("include-cyclical-reference.properties");
try {
handler.load();
Assert.fail("Expected " + Configuration.class.getCanonicalName());
} catch (ConfigurationException e) {
// expected
// e.printStackTrace();
}
assertNull(pc.getString("keyA"));
}

@Test
public void testIncludeLoadCyclicalMultiStepReferenceFail() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
final FileHandler handler = new FileHandler(pc);
handler.setBasePath(testBasePath);
handler.setFileName("include-cyclical-root.properties");
try {
handler.load();
Assert.fail("Expected " + Configuration.class.getCanonicalName());
} catch (ConfigurationException e) {
// expected
// e.printStackTrace();
}
assertNull(pc.getString("keyA"));
}

@Test
public void testIncludeLoadCyclicalMultiStepReferenceIgnore() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
final FileHandler handler = new FileHandler(pc);
handler.setBasePath(testBasePath);
handler.setFileName("include-cyclical-root.properties");
handler.load();
assertEquals("valueA", pc.getString("keyA"));
}

@Test
public void testIncludeIncludeLoadCyclicalReferenceFail() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
final FileHandler handler = new FileHandler(pc);
handler.setBasePath(testBasePath);
handler.setFileName("include-include-cyclical-reference.properties");
try {
handler.load();
Assert.fail("Expected " + Configuration.class.getCanonicalName());
} catch (ConfigurationException e) {
// expected
// e.printStackTrace();
}
assertNull(pc.getString("keyA"));
}

@Test
public void testIncludeIncludeLoadCyclicalReferenceIgnore() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
final FileHandler handler = new FileHandler(pc);
handler.setBasePath(testBasePath);
handler.setFileName("include-include-cyclical-reference.properties");
handler.load();
assertEquals("valueA", pc.getString("keyA"));
}

@Test
public void testIncludeLoadCyclicalReferenceIgnore() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
@@ -1231,7 +1306,7 @@ public void testPropertyLoaded() throws ConfigurationException
{
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
conf.propertyLoaded("layoutLoadedProperty", "yes");
conf.propertyLoaded("layoutLoadedProperty", "yes", null);
assertEquals("Layout's load() was called", 0, layout.loadCalls);
assertEquals("Property not added", "yes", conf.getString("layoutLoadedProperty"));
}
@@ -1244,7 +1319,8 @@ public void testPropertyLoadedInclude() throws ConfigurationException
{
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClasspath.properties,testEqual.properties");
conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClasspath.properties,testEqual.properties",
new ArrayDeque<>());
assertEquals("Layout's load() was not correctly called", 2, layout.loadCalls);
assertFalse("Property was added", conf.containsKey(PropertiesConfiguration.getInclude()));
}
@@ -1259,7 +1335,7 @@ public void testPropertyLoadedIncludeNotAllowed() throws ConfigurationException
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
conf.setIncludesAllowed(false);
conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClassPath.properties,testEqual.properties");
conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClassPath.properties,testEqual.properties", null);
assertEquals("Layout's load() was called", 0, layout.loadCalls);
assertFalse("Property was added", conf.containsKey(PropertiesConfiguration.getInclude()));
}
@@ -26,6 +26,8 @@
import java.io.Reader;
import java.io.StringReader;
import java.io.StringWriter;
import java.net.URL;
import java.util.Deque;
import java.util.Iterator;

import org.apache.commons.configuration2.convert.DefaultListDelimiterHandler;
@@ -830,12 +832,12 @@ static class LayoutTestConfiguration extends PropertiesConfiguration
* load() call on the layout is invoked.
*/
@Override
boolean propertyLoaded(final String key, final String value)
boolean propertyLoaded(final String key, final String value, Deque<URL> seenStack)
throws ConfigurationException
{
if (builder == null)
{
return super.propertyLoaded(key, value);
return super.propertyLoaded(key, value, seenStack);
}
if (PropertiesConfiguration.getInclude().equals(key))
{
@@ -0,0 +1,16 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include = src/test/resources/include-cyclical-root.properties
keyA = valueA
@@ -0,0 +1,16 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include = src/test/resources/include-cyclical-step.properties
keyA = valueA
@@ -0,0 +1,16 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include = src/test/resources/include-cyclical-back-to-root.properties
keyA = valueA
@@ -0,0 +1,16 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include = src/test/resources/include-cyclical-reference.properties
keyA = valueA

0 comments on commit d3c40d7

Please sign in to comment.