neo4j-contrib · Lojjs · Dec 1, 2022 · Dec 1, 2022
diff --git a/core/src/main/java/apoc/ApocConfig.java b/core/src/main/java/apoc/ApocConfig.java
@@ -10,6 +10,7 @@
 import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.apache.commons.configuration2.ex.ConversionException;
 import org.neo4j.configuration.Config;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.neo4j.configuration.GraphDatabaseSettings;
 import org.neo4j.dbms.api.DatabaseManagementService;
 import org.neo4j.graphdb.GraphDatabaseService;
@@ -128,8 +129,11 @@ public ApocConfig(Config neo4jConfig, LogService log, GlobalProcedures globalPro
     }
 
     // use only for unit tests
-    public ApocConfig() {
-        this.neo4jConfig = null;
+    public ApocConfig(Config neo4jConfig) {
+        this.neo4jConfig = neo4jConfig;
+        if (neo4jConfig != null) {
+            this.blockedIpRanges = neo4jConfig.get( GraphDatabaseInternalSettings.cypher_ip_blocklist);
+        }
         this.log = NullLog.getInstance();
         this.databaseManagementService = null;
         theInstance = this;

diff --git a/core/src/main/java/apoc/util/FileUtils.java b/core/src/main/java/apoc/util/FileUtils.java
@@ -32,6 +32,7 @@
 import static apoc.ApocConfig.APOC_IMPORT_FILE_ALLOW__READ__FROM__FILESYSTEM;
 import static apoc.ApocConfig.apocConfig;
 import static apoc.util.Util.ERROR_BYTES_OR_STRING;
+import static apoc.util.Util.REDIRECT_LIMIT;
 import static apoc.util.Util.readHttpInputStream;
 
 /**
@@ -71,7 +72,7 @@ public StreamConnection getStreamConnection(String urlAddress, Map<String, Objec
                 case http:
                 case https:
                 case gs:
-                    return readHttpInputStream(urlAddress, headers, payload);
+                    return readHttpInputStream(urlAddress, headers, payload, REDIRECT_LIMIT);
                 default:
                     try {
                         return new StreamConnection.FileStreamConnection(URI.create(urlAddress));

diff --git a/core/src/main/java/apoc/util/Util.java b/core/src/main/java/apoc/util/Util.java
@@ -101,6 +101,7 @@
 import static apoc.export.cypher.formatter.CypherFormatterUtils.formatToString;
 import static apoc.util.DateFormatUtil.getOrCreate;
 import static java.lang.String.format;
+import static java.net.HttpURLConnection.HTTP_NOT_MODIFIED;
 import static org.eclipse.jetty.util.URIUtil.encodePath;
 
 /**
@@ -113,6 +114,7 @@ public class Util {
     public static final String REL_COUNT = "MATCH ()-->() RETURN count(*) as result";
     public static final String COMPILED = "interpreted"; // todo handle enterprise properly
     public static final String ERROR_BYTES_OR_STRING = "Only byte[] or url String allowed";
+    public static final int REDIRECT_LIMIT = 10;
 
     public static String labelString(List<String> labelNames) {
         return labelNames.stream().map(Util::quote).collect(Collectors.joining(":"));
@@ -330,25 +332,27 @@ public static URLConnection openUrlConnection(String url, Map<String, Object> he
         URL src = new URL(url);
         URLConnection con = src.openConnection();
         con.setRequestProperty("User-Agent", "APOC Procedures for Neo4j");
-        if (headers != null) {
-            Object method = headers.get("method");
-            if (method != null && con instanceof HttpURLConnection) {
+       if (con instanceof HttpURLConnection) {
                 HttpURLConnection http = (HttpURLConnection) con;
-                http.setRequestMethod(method.toString());
-                http.setChunkedStreamingMode(1024*1024);
-                http.setInstanceFollowRedirects(true);
+                http.setInstanceFollowRedirects(false);
+            if (headers != null) {
+                Object method = headers.get("method");
+                if (method != null) {
+                    http.setRequestMethod(method.toString());
+                    http.setChunkedStreamingMode(1024 * 1024);
+                }
+                headers.forEach((k, v) -> con.setRequestProperty(k, v == null ? "" : v.toString()));
             }
-            headers.forEach((k,v) -> con.setRequestProperty(k, v == null ? "" : v.toString()));
         }
-//        con.setDoInput(true);
+
         con.setConnectTimeout(apocConfig().getInt("apoc.http.timeout.connect",10_000));
         con.setReadTimeout(apocConfig().getInt("apoc.http.timeout.read",60_000));
         return con;
     }
 
     public static boolean isRedirect(HttpURLConnection con) throws IOException {
-        int code = con.getResponseCode();
-        boolean isRedirectCode = code >= 300 && code < 400;
+ int responseCode = con.getResponseCode();
+        boolean isRedirectCode = responseCode >= 300 && responseCode <= 307 && responseCode != 306 && responseCode != HTTP_NOT_MODIFIED;
         if (isRedirectCode) {
             URL location = new URL(con.getHeaderField("Location"));
             String oldProtocol = con.getURL().getProtocol();
@@ -407,7 +411,7 @@ private static CountingInputStream getStreamCompressedFile(String urlAddress, Ma
         return new CountingInputStream(stream, sc.getLength());
     }
 
-    private static StreamConnection getStreamConnection(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
+    public static StreamConnection getStreamConnection(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
         return FileUtils.SupportedProtocols
                 .from(urlAddress)
                 .getStreamConnection(urlAddress, headers, payload);
@@ -427,14 +431,17 @@ private static InputStream getFileStreamIntoCompressedFile(InputStream is, Strin
         return null;
     }
 
-    public static StreamConnection readHttpInputStream(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
+    public static StreamConnection readHttpInputStream(String urlAddress, Map<String, Object> headers, String payload, int redirectLimit) throws IOException {
         ApocConfig.apocConfig().checkReadAllowed(urlAddress);
         URLConnection con = openUrlConnection(urlAddress, headers);
         writePayload(con, payload);
         String newUrl = handleRedirect(con, urlAddress);
         if (newUrl != null && !urlAddress.equals(newUrl)) {
             con.getInputStream().close();
-            return readHttpInputStream(newUrl, headers, payload);
+            if (redirectLimit == 0) {
+                throw new IOException("Redirect limit exceeded");
+            }
+            return readHttpInputStream(newUrl, headers, payload, --redirectLimit);
         }
 
         return new StreamConnection.UrlStreamConnection(con);

diff --git a/core/src/test/java/apoc/load/XmlTest.java b/core/src/test/java/apoc/load/XmlTest.java
@@ -4,12 +4,16 @@
 import apoc.util.CompressionAlgo;
 import apoc.util.TestUtil;
 import apoc.xml.XmlTestUtils;
+import inet.ipaddr.IPAddressString;
+import junit.framework.TestCase;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang.exception.ExceptionUtils;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.graphdb.ResourceIterator;
 import org.neo4j.internal.helpers.collection.Iterables;
@@ -41,7 +45,8 @@ public class XmlTest {
     @Rule
     public DbmsRule db = new ImpermanentDbmsRule()
             .withSetting(ApocSettings.apoc_import_file_enabled, true)
-            .withSetting(ApocSettings.apoc_import_file_use__neo4j__config, false);
+            .withSetting(ApocSettings.apoc_import_file_use__neo4j__config, false)
+            .withSetting( GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString("127.0.0.0/8")));
 
     @Rule
     public ExpectedException thrown = ExpectedException.none();
@@ -195,6 +200,18 @@ public void testLoadXmlXpathReturnBookFromBookTitle () {
                 });
     }
 
+    @Test
+    public void testLoadXmlWithBlockedIP () {
+        QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
+                () -> testCall(db,
+                        "CALL apoc.load.xml('http://127.0.0.0/fake.xml') yield value as result",
+                        map(),
+                        (r) -> {}
+                )
+        );
+        TestCase.assertTrue(e.getMessage().contains("access to /127.0.0.0 is blocked via the configuration property unsupported.dbms.cypher_ip_blocklist"));
+    }
+
     @Test
     public void testLoadXmlXpathBooKsFromGenre () {
         testResult(db, "CALL apoc.load.xml('" + TestUtil.getUrlFileName("xml/books.xml") + "', '/catalog/book[genre=\"Computer\"]') yield value as result",

diff --git a/core/src/test/java/apoc/util/UtilIT.java b/core/src/test/java/apoc/util/UtilIT.java
@@ -1,61 +1,57 @@
 package apoc.util;
 
 import apoc.ApocConfig;
+import inet.ipaddr.IPAddressString;
+import junit.framework.TestCase;
 import org.apache.commons.io.IOUtils;
-import org.junit.After;
+import org.junit.Assert;
 import org.junit.Assume;
-import org.junit.Before;
-import org.junit.Ignore;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.AfterEach;
+import org.neo4j.configuration.Config;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
 
 import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.net.URL;
 import java.nio.charset.Charset;
+import java.util.ArrayList;
+import java.util.List;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
-@Ignore
 public class UtilIT {
-
-    @Rule
-    public TestName testName = new TestName();
-
     private GenericContainer httpServer;
 
-    private static final String WITH_URL_LOCATION = "WithUrlLocation";
-    private static final String WITH_FILE_LOCATION = "WithFileLocation";
-
-    @Before
-    public void setUp() throws Exception {
-        new ApocConfig();  // empty test configuration, ensure ApocConfig.apocConfig() can be used
-        TestUtil.ignoreException(() -> {
-            httpServer = new GenericContainer("alpine")
-                    .withCommand("/bin/sh", "-c", String.format("while true; do { echo -e 'HTTP/1.1 301 Moved Permanently\\r\\nLocation: %s'; echo ; } | nc -l -p 8000; done",
-                            testName.getMethodName().endsWith(WITH_URL_LOCATION) ? "http://www.google.com" : "file:/etc/passwd"))
-                    .withExposedPorts(8000);
-            httpServer.waitingFor(Wait.forHttp("/")
-                    .forStatusCode(301));
-            httpServer.start();
-        }, Exception.class);
+    private GenericContainer setUpServer(Config neo4jConfig, String redirectURL) {
+        new ApocConfig(neo4jConfig);
+        GenericContainer httpServer = new GenericContainer("alpine")
+                .withCommand("/bin/sh", "-c", String.format("while true; do { echo -e 'HTTP/1.1 301 Moved Permanently\\r\\nLocation: %s'; echo ; } | nc -l -p 8000; done",
+                        redirectURL))
+                .withExposedPorts(8000);
+        httpServer.start();
         Assume.assumeNotNull(httpServer);
         Assume.assumeTrue(httpServer.isRunning());
+        return httpServer;
     }
 
-    @After
+    @AfterEach
     public void tearDown() {
-        if (httpServer != null) {
+        if (httpServer != null)
+        {
             httpServer.stop();
         }
     }
 
     @Test
     public void redirectShouldWorkWhenProtocolNotChangesWithUrlLocation() throws IOException {
+        httpServer = setUpServer(null, "http://www.google.com");
         // given
-        String url = getServerUrl();
+        String url = getServerUrl(httpServer);
 
         // when
         String page = IOUtils.toString(Util.openInputStream(url, null, null, null), Charset.forName("UTF-8"));
@@ -64,11 +60,84 @@ public void redirectShouldWorkWhenProtocolNotChangesWithUrlLocation() throws IOE
         assertTrue(page.contains("<title>Google</title>"));
     }
 
+    @Test
+    public void redirectWithBlockedIPsWithUrlLocation() {
+        List<IPAddressString> blockedIPs = List.of(new IPAddressString("127.168.0.1/8"));
+
+        Config neo4jConfig = mock(Config.class);
+        when(neo4jConfig.get(GraphDatabaseInternalSettings.cypher_ip_blocklist)).thenReturn(blockedIPs);
+
+        httpServer = setUpServer(neo4jConfig, "http://127.168.0.1");
+        String url = getServerUrl(httpServer);
+
+        IOException e = Assert.assertThrows(IOException.class,
+                () -> Util.openInputStream(url, null, null, null)
+        );
+        TestCase.assertTrue(e.getMessage().contains("access to /127.168.0.1 is blocked via the configuration property unsupported.dbms.cypher_ip_blocklist"));
+    }
+
+    @Test
+    public void redirectWithProtocolUpgradeIsAllowed() throws IOException {
+        List<IPAddressString> blockedIPs = List.of(new IPAddressString("127.168.0.1/8"));
+
+        Config neo4jConfig = mock(Config.class);
+        when(neo4jConfig.get(GraphDatabaseInternalSettings.cypher_ip_blocklist)).thenReturn(blockedIPs);
+
+        httpServer = setUpServer(neo4jConfig, "https://www.google.com");
+        String url = getServerUrl(httpServer);
+
+        // when
+        String page = IOUtils.toString(Util.openInputStream(url, null, null, null), Charset.forName("UTF-8"));
+        // then
+        assertTrue(page.contains("<title>Google</title>"));
+    }
+
+    @Test
+    public void redirectWithProtocolDowngradeIsNotAllowed() throws IOException {
+        HttpURLConnection mockCon = mock(HttpURLConnection.class);
+        when(mockCon.getResponseCode()).thenReturn(302);
+        when(mockCon.getHeaderField("Location")).thenReturn("http://127.168.0.1");
+        when(mockCon.getURL()).thenReturn(new URL("https://127.0.0.0"));
+
+        RuntimeException e = Assert.assertThrows(RuntimeException.class,
+                () -> Util.isRedirect(mockCon)
+        );
+
+        TestCase.assertTrue(e.getMessage().contains("The redirect URI has a different protocol: http://127.168.0.1"));
+    }
+
+    @Test
+    public void shouldFailForExceedingRedirectLimit() {
+        Config neo4jConfig = mock(Config.class);
+
+        httpServer = setUpServer(neo4jConfig, "https://127.0.0.0");
+        String url = getServerUrl(httpServer);
+
+        ArrayList<GenericContainer> servers = new ArrayList<>();
+        for (int i = 1; i <= 10; i++) {
+            GenericContainer server = setUpServer(neo4jConfig, url);
+            servers.add(server);
+            url = getServerUrl(server);
+        }
+
+        String finalUrl = url;
+        IOException e = Assert.assertThrows(IOException.class,
+                () -> Util.openInputStream(finalUrl, null, null, null)
+        );
+
+        TestCase.assertTrue(e.getMessage().contains("Redirect limit exceeded"));
+
+        for (GenericContainer server : servers) {
+            server.stop();
+        }
+    }
+
     @Test(expected = RuntimeException.class)
     public void redirectShouldThrowExceptionWhenProtocolChangesWithFileLocation() throws IOException {
         try {
+            httpServer = setUpServer(null, "file:/etc/passwd");
             // given
-            String url = getServerUrl();
+            String url = getServerUrl(httpServer);
 
             // when
             Util.openInputStream(url, null, null, null);
@@ -79,7 +148,7 @@ public void redirectShouldThrowExceptionWhenProtocolChangesWithFileLocation() th
         }
     }
 
-    private String getServerUrl() {
+    private String getServerUrl(GenericContainer httpServer) {
         return String.format("http://%s:%s", httpServer.getContainerIpAddress(), httpServer.getMappedPort(8000));
     }
 }