Skip to content

Commit

Permalink
[3QAbWQds] apoc.export.arrow.all ignores export file config (neo4j-co…
Browse files Browse the repository at this point in the history 
…ntrib#349)

* [3QAbWQds] apoc.export.arrow.all ignores export file config

* [3QAbWQds] Added ExportArrowSecurityTest and removed some unnecessary tests
  • Loading branch information
@vga91
vga91 authored Apr 5, 2023
1 parent e8a4a7c commit 89ad884
Show file tree
Hide file tree
Showing 5 changed files with 406 additions and 66 deletions.
5 changes: 3 additions & 2 deletions common/src/main/java/apoc/ApocConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ public class ApocConfig extends LifecycleAdapter {
));
private static final String DEFAULT_PATH = ".";
private static final String CONFIG_DIR = "config-dir=";
public static final String EXPORT_TO_FILE_ERROR = "Export to files not enabled, please set apoc.export.file.enabled=true in your apoc.conf.\n" +
"Otherwise, if you are running in a cloud environment without filesystem access, use the `{stream:true}` config and null as a 'file' parameter to stream the export back to your client.";
public static final String EXPORT_NOT_ENABLED_ERROR = "Export to files not enabled, please set apoc.export.file.enabled=true in your apoc.conf.";
public static final String EXPORT_TO_FILE_ERROR = EXPORT_NOT_ENABLED_ERROR +
"\nOtherwise, if you are running in a cloud environment without filesystem access, use the `{stream:true}` config and null as a 'file' parameter to stream the export back to your client.";

private final Config neo4jConfig;
private final Log log;
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/apoc/export/arrow/ExportArrowService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,14 @@

import java.util.stream.Stream;

import static apoc.ApocConfig.APOC_EXPORT_FILE_ENABLED;
import static apoc.ApocConfig.EXPORT_NOT_ENABLED_ERROR;
import static apoc.ApocConfig.apocConfig;

public class ExportArrowService {

public static final String EXPORT_TO_FILE_ARROW_ERROR = EXPORT_NOT_ENABLED_ERROR +
"\nOtherwise, if you are running in a cloud environment without filesystem access, use the apoc.export.arrow.stream.* procedures to stream the export back to your client.";
private final GraphDatabaseService db;
private final Pools pools;
private final TerminationGuard terminationGuard;
Expand All @@ -34,6 +40,11 @@ public Stream<ByteArrayResult> stream(Object data, ArrowConfig config) {
}

public Stream<ProgressInfo> file(String fileName, Object data, ArrowConfig config) {
// we cannot use apocConfig().checkWriteAllowed(..) because the error is confusing
// since it says "... use the `{stream:true}` config", but with arrow procedures the streaming mode is implemented via different procedures
if (!apocConfig().getBoolean(APOC_EXPORT_FILE_ENABLED)) {
throw new RuntimeException(EXPORT_TO_FILE_ARROW_ERROR);
}
if (data instanceof Result) {
return new ExportResultFileStrategy(fileName, db, pools, terminationGuard, logger).export((Result) data, config);
} else {
Expand Down
131 changes: 67 additions & 64 deletions core/src/test/java/apoc/export/ExportCoreSecurityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.neo4j.configuration.GraphDatabaseSettings;
import org.neo4j.graphdb.GraphDatabaseService;
import org.neo4j.graphdb.QueryExecutionException;
import org.neo4j.graphdb.Result;
import org.neo4j.test.rule.DbmsRule;
Expand All @@ -27,8 +28,10 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
import java.util.stream.Collectors;

import static apoc.util.TestUtil.assertError;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

Expand Down Expand Up @@ -59,12 +62,16 @@ public static void setUp() {
setFileExport(false);
}

private static void setFileExport(boolean allowed) {
public static void setFileExport(boolean allowed) {
ApocConfig.apocConfig().setProperty(ApocConfig.APOC_EXPORT_FILE_ENABLED, allowed);
}

private static Collection<String[]> data(Map<String, List<String>> apocProcedureArguments) {
return APOC_EXPORT_PROCEDURE_NAME
return data(apocProcedureArguments, APOC_EXPORT_PROCEDURE_NAME);
}

public static Collection<String[]> data(Map<String, List<String>> apocProcedureArguments, List<String> procedureNames) {
return procedureNames
.stream()
.flatMap(method -> apocProcedureArguments
.entrySet()
Expand All @@ -80,7 +87,7 @@ public static class TestIllegalFSAccess {
private final String apocProcedure;

public TestIllegalFSAccess(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final Map<String, List<String>> APOC_PROCEDURE_ARGUMENTS = Map.of(
Expand Down Expand Up @@ -117,7 +124,7 @@ public void testIllegalFSAccessExport() {
() -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {})
);

TestUtil.assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure);
assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure);
}
}

Expand All @@ -126,7 +133,7 @@ public static class TestIllegalExternalFSAccess {
private final String apocProcedure;

public TestIllegalExternalFSAccess(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
Expand Down Expand Up @@ -162,7 +169,7 @@ public void testIllegalExternalFSAccessExport() {
Result::resultAsString);
fail(message);
} catch (Exception e) {
TestUtil.assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure);
assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure);
} finally {
setFileExport(false);
}
Expand All @@ -174,7 +181,7 @@ public static class TestPathTraversalAccess {
private final String apocProcedure;

public TestPathTraversalAccess(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final String case1 = "'file:///%2e%2e%2f%2f%2e%2e%2f%2f%2e%2e%2f%2f%2e%2e%2f%2fapoc/test.txt'";
Expand All @@ -183,7 +190,7 @@ public TestPathTraversalAccess(String exportMethod, String exportMethodType, Str
private static final String case4 = "'tests/../../test.txt'";
private static final String case5 = "'tests/..//..//test.txt'";

private static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5);
public static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5);

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
"query", cases.stream().map(
Expand All @@ -207,14 +214,7 @@ public static Collection<String[]> data() {

@Test
public void testPathTraversal() {
setFileExport(true);

QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {})
);

TestUtil.assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure);
setFileExport(false);
assertPathTraversalError(db, "CALL " + apocProcedure);
}
}

Expand All @@ -229,7 +229,7 @@ public static class TestPathTraversalIsNormalised {
private final String apocProcedure;

public TestPathTraversalIsNormalised(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final String case1 = "'file://%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2f/apoc/test.txt'";
Expand All @@ -239,7 +239,7 @@ public TestPathTraversalIsNormalised(String exportMethod, String exportMethodTyp
private static final String case5 = "'file://" + directory.getAbsolutePath() + "//..//..//..//..//apoc/test.txt'";
private static final String case6 = "'file:///%252e%252e%252f%252e%252e%252f%252e%252e%252f%252e%252e%252f/apoc/test.txt'";

private static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5, case6);
public static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5, case6);

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
"query", cases.stream().map(
Expand All @@ -263,14 +263,9 @@ public static Collection<String[]> data() {

@Test
public void testPathTraversal() {
setFileExport(true);

QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {})
assertPathTraversalError(db, "CALL " + apocProcedure,
e -> TestCase.assertTrue(e.getMessage().contains("apoc/test.txt (No such file or directory)"))
);

TestCase.assertTrue(e.getMessage().contains("apoc/test.txt (No such file or directory)"));
setFileExport(false);
}
}

Expand All @@ -284,7 +279,7 @@ public static class TestPathTraversalIsNormalisedWithinDirectory {
private final String apocProcedure;

public TestPathTraversalIsNormalisedWithinDirectory(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final String case1 = "'file:///..//..//..//..//apoc//..//..//..//..//test.txt'";
Expand All @@ -296,7 +291,7 @@ public TestPathTraversalIsNormalisedWithinDirectory(String exportMethod, String
private static final String case7 = "'test.txt'";
private static final String case8 = "'file:///..//..//..//..//test.txt'";

private static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5, case6, case7, case8);
public static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5, case6, case7, case8);

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
"query", cases.stream().map(
Expand All @@ -320,15 +315,9 @@ public static Collection<String[]> data() {

@Test
public void testPathTraversal() {
setFileExport(true);
assertPathTraversalWithoutErrors(db, apocProcedure, directory,
(r) -> assertTrue(((String) r.get("file")).contains("test.txt")));

TestUtil.testCall(db, "CALL " + apocProcedure,
(r) -> assertTrue(((String) r.get("file")).contains("test.txt"))
);

File f = new File(directory.getAbsolutePath() + "/test.txt");
TestCase.assertTrue(f.exists());
TestCase.assertTrue(f.delete());
}
}

Expand All @@ -343,13 +332,13 @@ public static class TestPathTraversalIsWithSimilarDirectoryName {
private final String apocProcedure;

public TestPathTraversalIsWithSimilarDirectoryName(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final String case1 = "'../imported/test.txt'";
private static final String case2 = "'tests/../../imported/test.txt'";

private static final List<String> cases = Arrays.asList(case1, case2);
public static final List<String> cases = Arrays.asList(case1, case2);

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
"query", cases.stream().map(
Expand All @@ -373,15 +362,7 @@ public static Collection<String[]> data() {

@Test
public void testPathTraversal() {
setFileExport(true);

QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {})
);

TestUtil.assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure);

setFileExport(false);
assertPathTraversalError(db, "CALL " + apocProcedure);
}
}

Expand All @@ -397,7 +378,7 @@ public static class TestPathTraversAllowedWithinDirectory {
private final String apocProcedure;

public TestPathTraversAllowedWithinDirectory(String exportMethod, String exportMethodType, String exportMethodArguments) {
this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
this.apocProcedure = getApocProcedure(exportMethod, exportMethodType, exportMethodArguments);
}

private static final String case1 = "'file:///../import/../import//..//tests/test.txt'";
Expand All @@ -406,7 +387,7 @@ public TestPathTraversAllowedWithinDirectory(String exportMethod, String exportM
private static final String case4 = "'file:///tests/test.txt'";
private static final String case5 = "'tests/test.txt'";

private static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5);
public static final List<String> cases = Arrays.asList(case1, case2, case3, case4, case5);

private static final Map<String, List<String>> METHOD_ARGUMENTS = Map.of(
"query", cases.stream().map(
Expand All @@ -430,15 +411,8 @@ public static Collection<String[]> data() {

@Test
public void testPathTraversal() {
setFileExport(true);

TestUtil.testCall(db, "CALL " + apocProcedure,
(r) -> assertTrue(((String) r.get("file")).contains("tests/test.txt"))
);

File f = new File(subDirectory.getAbsolutePath() + "/test.txt");
TestCase.assertTrue(f.exists());
TestCase.assertTrue(f.delete());
assertPathTraversalWithoutErrors(db, apocProcedure, subDirectory,
(r) -> assertTrue(((String) r.get("file")).contains("tests/test.txt")));
}
}

Expand All @@ -450,17 +424,46 @@ public void testIllegalFSAccessExportCypherSchema() {
QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, String.format("CALL " + apocProcedure, "'./hello', {}"), (r) -> {})
);
TestUtil.assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure);
assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure);
}

@Test
public void testIllegalExternalFSAccessExportCypherSchema() {
setFileExport(true);
QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, String.format("CALL " + apocProcedure, "'../hello', {}"), (r) -> {})
);
TestUtil.assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure);
setFileExport(false);
assertPathTraversalError(db, String.format("CALL " + apocProcedure, "'../hello', {}"),
e -> assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure));
}
}

public static String getApocProcedure(String exportMethod, String exportMethodType, String exportMethodArguments) {
return "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")";
}

public static void assertPathTraversalError(GraphDatabaseService db, String query, Consumer<Exception> exceptionConsumer) {
setFileExport(true);

QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
() -> TestUtil.testCall(db, query, (r) -> {})
);

exceptionConsumer.accept(e);

setFileExport(false);
}

public static void assertPathTraversalWithoutErrors(GraphDatabaseService db, String apocProcedure, File directory, Consumer<Map<String, Object>> consumer) {
setFileExport(true);

TestUtil.testCall(db, "CALL " + apocProcedure, consumer);

File f = new File(directory.getAbsolutePath() + "/test.txt");
TestCase.assertTrue(f.exists());
TestCase.assertTrue(f.delete());
}

public static void assertPathTraversalError(GraphDatabaseService db, String apocProcedure) {
assertPathTraversalError(db, apocProcedure,
e -> assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure)
);
}

}
19 changes: 19 additions & 0 deletions core/src/test/java/apoc/export/arrow/ArrowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import apoc.util.JsonUtil;
import apoc.util.TestUtil;
import com.fasterxml.jackson.core.JsonProcessingException;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
Expand Down Expand Up @@ -95,6 +96,10 @@ public class ArrowTest {
public static void beforeClass() {
db.executeTransactionally("CREATE (f:User {name:'Adam',age:42,male:true,kids:['Sam','Anna','Grace'], born:localdatetime('2015-05-18T19:32:24.000'), place:point({latitude: 13.1, longitude: 33.46789, height: 100.0})})-[:KNOWS {since: 1993, bffSince: duration('P5M1.5D')}]->(b:User {name:'Jim',age:42})");
TestUtil.registerProcedure(db, ExportArrow.class, LoadArrow.class, Graphs.class, Meta.class);
}

@Before
public void before() {
apocConfig().setProperty(APOC_IMPORT_FILE_ENABLED, true);
apocConfig().setProperty(APOC_EXPORT_FILE_ENABLED, true);
}
Expand Down Expand Up @@ -247,6 +252,20 @@ public void testFileRoundtripArrowGraph() {

@Test
public void testStreamRoundtripArrowAll() {
testStreamRoundtripAllCommon();
}

@Test
public void testStreamRoundtripArrowAllWithImportExportConfsDisabled() {
// disable both export and import configs
apocConfig().setProperty(APOC_IMPORT_FILE_ENABLED, false);
apocConfig().setProperty(APOC_EXPORT_FILE_ENABLED, false);

// should work regardless of the previous config
testStreamRoundtripAllCommon();
}

private void testStreamRoundtripAllCommon() {
// given - when
final byte[] byteArray = db.executeTransactionally("CALL apoc.export.arrow.stream.all() YIELD value AS byteArray ",
Map.of(),
Expand Down
Loading

0 comments on commit 89ad884

Please sign in to comment.