elharo commented on code in PR #123:
URL: https://github.com/apache/maven-shared-io/pull/123#discussion_r3720106630
##########
src/main/java/org/apache/maven/shared/io/location/FileLocatorStrategy.java:
##########
@@ -19,18 +19,70 @@
package org.apache.maven.shared.io.location;
import java.io.File;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Objects;
import org.apache.maven.shared.io.logging.MessageHolder;
/**
* file locator strategy.
*
+ * <p>
+ * The location specification is normalized before use, so <code>..</code> and
<code>.</code> elements do not remain in
+ * the resolved file. If the location specification comes from an untrusted
source, use
+ * {@link #FileLocatorStrategy(File)} to give a base directory. The strategy
then resolves relative specifications
+ * against that base directory and refuses to resolve a file outside of it.
+ * </p>
*/
public class FileLocatorStrategy implements LocatorStrategy {
+ private final File baseDirectory;
+
+ /**
+ * Create a strategy that resolves any file, and resolves a relative
specification against the current directory.
+ */
+ public FileLocatorStrategy() {
+ this.baseDirectory = null;
+ }
+
+ /**
+ * Create a strategy that only resolves files in the given base directory.
+ *
+ * @param baseDirectory the directory that contains the files to resolve;
a relative specification is resolved
Review Comment:
not grammatically parallel
##########
src/main/java/org/apache/maven/shared/io/location/FileLocatorStrategy.java:
##########
@@ -19,18 +19,70 @@
package org.apache.maven.shared.io.location;
import java.io.File;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Objects;
import org.apache.maven.shared.io.logging.MessageHolder;
/**
* file locator strategy.
*
+ * <p>
Review Comment:
write this by hand. This should describe the class, not the bug.
##########
src/main/java/org/apache/maven/shared/io/location/FileLocatorStrategy.java:
##########
@@ -19,18 +19,70 @@
package org.apache.maven.shared.io.location;
import java.io.File;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Objects;
import org.apache.maven.shared.io.logging.MessageHolder;
/**
* file locator strategy.
*
+ * <p>
+ * The location specification is normalized before use, so <code>..</code> and
<code>.</code> elements do not remain in
+ * the resolved file. If the location specification comes from an untrusted
source, use
+ * {@link #FileLocatorStrategy(File)} to give a base directory. The strategy
then resolves relative specifications
+ * against that base directory and refuses to resolve a file outside of it.
+ * </p>
*/
public class FileLocatorStrategy implements LocatorStrategy {
+ private final File baseDirectory;
+
+ /**
+ * Create a strategy that resolves any file, and resolves a relative
specification against the current directory.
+ */
+ public FileLocatorStrategy() {
+ this.baseDirectory = null;
+ }
+
+ /**
+ * Create a strategy that only resolves files in the given base directory.
+ *
+ * @param baseDirectory the directory that contains the files to resolve;
a relative specification is resolved
+ * against this directory, and a specification that points
outside of it is refused.
+ */
+ public FileLocatorStrategy(File baseDirectory) {
+ this.baseDirectory = Objects.requireNonNull(baseDirectory,
"baseDirectory");
+ }
+
/** {@inheritDoc} */
public Location resolve(String locationSpecification, MessageHolder
messageHolder) {
- File file = new File(locationSpecification);
+ Objects.requireNonNull(locationSpecification, "locationSpecification");
+
+ File file;
+ try {
+ Path path = Paths.get(locationSpecification);
+
+ if (baseDirectory != null) {
+ path = baseDirectory.toPath().resolve(path);
+ }
+
+ file = path.normalize().toFile();
+ } catch (InvalidPathException e) {
+ messageHolder.addMessage("File: " + locationSpecification + " is
not a valid path.");
+
+ return null;
+ }
+
+ if (baseDirectory != null && !isInBaseDirectory(file)) {
+ messageHolder.addMessage(
Review Comment:
why not just throw an exception here?
##########
src/main/java/org/apache/maven/shared/io/location/FileLocatorStrategy.java:
##########
@@ -19,18 +19,70 @@
package org.apache.maven.shared.io.location;
import java.io.File;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Objects;
import org.apache.maven.shared.io.logging.MessageHolder;
/**
* file locator strategy.
*
+ * <p>
+ * The location specification is normalized before use, so <code>..</code> and
<code>.</code> elements do not remain in
+ * the resolved file. If the location specification comes from an untrusted
source, use
+ * {@link #FileLocatorStrategy(File)} to give a base directory. The strategy
then resolves relative specifications
+ * against that base directory and refuses to resolve a file outside of it.
+ * </p>
*/
public class FileLocatorStrategy implements LocatorStrategy {
+ private final File baseDirectory;
+
+ /**
+ * Create a strategy that resolves any file, and resolves a relative
specification against the current directory.
+ */
+ public FileLocatorStrategy() {
+ this.baseDirectory = null;
+ }
+
+ /**
+ * Create a strategy that only resolves files in the given base directory.
+ *
+ * @param baseDirectory the directory that contains the files to resolve;
a relative specification is resolved
+ * against this directory, and a specification that points
outside of it is refused.
+ */
+ public FileLocatorStrategy(File baseDirectory) {
+ this.baseDirectory = Objects.requireNonNull(baseDirectory,
"baseDirectory");
+ }
+
/** {@inheritDoc} */
public Location resolve(String locationSpecification, MessageHolder
messageHolder) {
- File file = new File(locationSpecification);
+ Objects.requireNonNull(locationSpecification, "locationSpecification");
+
+ File file;
+ try {
+ Path path = Paths.get(locationSpecification);
+
+ if (baseDirectory != null) {
+ path = baseDirectory.toPath().resolve(path);
+ }
+
+ file = path.normalize().toFile();
+ } catch (InvalidPathException e) {
+ messageHolder.addMessage("File: " + locationSpecification + " is
not a valid path.");
+
+ return null;
Review Comment:
This null return needs to be well documented, if it's done at all. Am=n
exception feels more appropriate
--
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]
