[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 ---