ctubbsii commented on a change in pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#discussion_r525525497
##########
File path:
core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
##########
@@ -140,8 +140,8 @@ public Buildable withOverrides(Map<String,String>
overrides) {
return this;
}
- @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
- justification = "location of props is specified by an admin")
+ @SuppressFBWarnings(value = {"URLCONNECTION_SSRF_FD", "PATH_TRAVERSAL_IN"},
+ justification = "location of props is specified by an admin, path
provided by test")
Review comment:
Is this suppression necessary on this method? This method doesn't look
like it changed. It might be covered by the suppression elsewhere.
##########
File path:
start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java
##########
@@ -86,10 +86,10 @@ public static String getAccumuloProperty(String
propertyName, String defaultValu
return defaultValue;
}
try {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new
Parameters().properties().setURL(accumuloConfigUrl));
- PropertiesConfiguration config = propsBuilder.getConfiguration();
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(accumuloConfigUrl.getFile())) {
+ config.getLayout().load(config, reader);
Review comment:
I wasn't aware of the `read()` method before. Apparently, we can just
call `config.read(reader)` here. I also wasn't aware of `FileHandler` before,
which I see is being used when saving files.
I think we should try to be consistent. We should either use:
```java
var config = PropertiesConfiguration();
try (var reader = new FileReader()) {
config.read(reader);
}
// and
try (var writer = new FileWriter()) {
config.write(writer);
}
```
OR
```java
var config = new PropertiesConfiguration();
var fileHandler = new FileHandler(config);
fileHandler.load(file);
// and
fileHandler.save(file);
```
Ultimately, they will both do the same thing, but I'm not sure which one is
cleaner, since we still need to handle exceptions. Either way, but we should
consistently use one pattern or the other.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
