[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152774820
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java
---
@@ -59,4 +61,53 @@ public void testEmptyFolderThrowsTableNotFound() throws
Exception {
throw ex;
}
}
+
+ @Test
+ public void testBackPath() throws Exception {
--- End diff --
Please split into two tests.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152772350
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
--- End diff --
Please use java doc.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152774087
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath,
String subpath) {
+Preconditions.checkArgument(!parent.isEmpty());
+Preconditions.checkArgument(!combinedPath.isEmpty());
--- End diff --
Please add message for pre-conditions so error message will be more clear.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152772486
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath,
String subpath) {
--- End diff --
This method can return void, since it never will return false. I see you
use this during testing but it can be tested with void type as well.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152773573
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
--- End diff --
Consider using primitive, we don't want unnecessary boxing / unboxing.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152773919
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -252,11 +252,15 @@ private static String buildPath(final String[] path,
final int folderIndex) {
return builder.toString();
}
- public static FileSelection create(final DrillFileSystem fs, final
String parent, final String path) throws IOException {
+ public static FileSelection create(final DrillFileSystem fs, final
String parent, final String path,
+ final boolean allowAccessOutsideWorkspace) throws IOException {
Stopwatch timer = Stopwatch.createStarted();
boolean hasWildcard = path.contains(WILD_CARD);
final Path combined = new Path(parent, removeLeadingSlash(path));
+if (!allowAccessOutsideWorkspace) {
+ checkBackPaths(parent, combined.toString(), path);
--- End diff --
I usually void using `toString` for `Path`, consider using
`combined.toUri().getPath()`.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862693
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath,
String subpath) {
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862665
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153318818
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java
---
@@ -59,4 +61,53 @@ public void testEmptyFolderThrowsTableNotFound() throws
Exception {
throw ex;
}
}
+
+ @Test
+ public void testBackPath() throws Exception {
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862722
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -359,15 +363,30 @@ private static Path handleWildCard(final String root)
{
}
}
- private static String removeLeadingSlash(String path) {
-if (path.charAt(0) == '/') {
+ public static String removeLeadingSlash(String path) {
+if (!path.isEmpty() && path.charAt(0) == '/') {
String newPath = path.substring(1);
return removeLeadingSlash(newPath);
} else {
return path;
}
}
+ // Check if the path is a valid sub path under the parent after removing
backpaths. Throw an exception if
+ // it is not
+ // We pass subpath in as a parameter only for the error message
+ public static boolean checkBackPaths(String parent, String combinedPath,
String subpath) {
+Preconditions.checkArgument(!parent.isEmpty());
+Preconditions.checkArgument(!combinedPath.isEmpty());
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862954
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
--- End diff --
I need to check if the value is not present (i.e. null). That will be the
case with all storage plugin configurations that have already been created.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r152862636
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java
---
@@ -252,11 +252,15 @@ private static String buildPath(final String[] path,
final int folderIndex) {
return builder.toString();
}
- public static FileSelection create(final DrillFileSystem fs, final
String parent, final String path) throws IOException {
+ public static FileSelection create(final DrillFileSystem fs, final
String parent, final String path,
+ final boolean allowAccessOutsideWorkspace) throws IOException {
Stopwatch timer = Stopwatch.createStarted();
boolean hasWildcard = path.contains(WILD_CARD);
final Path combined = new Path(parent, removeLeadingSlash(path));
+if (!allowAccessOutsideWorkspace) {
+ checkBackPaths(parent, combined.toString(), path);
--- End diff --
Done
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153337275
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,25 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
+ // field is a Boolean
(not boolean) so that we can
+ // assign a default
value if it is not defined in a
+ // storage plugin
config
public WorkspaceConfig(@JsonProperty("location") String location,
@JsonProperty("writable") boolean writable,
- @JsonProperty("defaultInputFormat") String
defaultInputFormat) {
+ @JsonProperty("defaultInputFormat") String
defaultInputFormat,
+ @JsonProperty("allowAccessOutsideWorkspace")
Boolean allowAccessOutsideWorkspace
+ ) {
this.location = location;
this.writable = writable;
this.defaultInputFormat = defaultInputFormat;
+//this.allowAccessOutsideWorkspace = allowAccessOutsideWorkspace !=
null ? allowAccessOutsideWorkspace : false ;
+this.allowAccessOutsideWorkspace = true;
--- End diff --
It seems we should not always set true...
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153336979
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
--- End diff --
As far I remember `@JsonProperty("allowAccessOutsideWorkspace") boolean
allowAccessOutsideWorkspace` will set false by default if value is not present
during deserialization. Could you please check?
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153388800
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
--- End diff --
Yes it would, I believe. But we want the value to be `true` for backward
compatibility. (This also addresses your next comment). So we need to know if
the value is missing. Can only do that with a non primitive AFAIK.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1050#discussion_r153436976
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceConfig.java
---
@@ -30,18 +30,24 @@
public class WorkspaceConfig {
/** Default workspace is a root directory which supports read, but not
write. */
- public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null);
+ public static final WorkspaceConfig DEFAULT = new WorkspaceConfig("/",
false, null, false);
private final String location;
private final boolean writable;
private final String defaultInputFormat;
-
+ private final Boolean allowAccessOutsideWorkspace; // allow access
outside the workspace by default. This
--- End diff --
1. Can we adjust the variable to be false by default, i.e. rename it to
`disallowAccessOutsideWorkspace`? Thus we'll be able to use primitive, right?
2. In the below code you always set `this.allowAccessOutsideWorkspace =
true;`, block with `this.allowAccessOutsideWorkspace =
allowAccessOutsideWorkspace != null ? allowAccessOutsideWorkspace : false ;` is
commented. I guess this is a mistake.
---
[GitHub] drill pull request #1050: DRILL-5964: Do not allow queries to access paths o...
Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/1050 ---
