Skip to content

Commit 4a263f1

Browse files
gem-neo4jLojjs
gem-neo4j
and
Lojjs
authored
[eOxJMLMv] Checks redirects for blocked IPs instead of just final location. (neo4j#240)
Also fixes ignored test for URL to file redirect. Co-authored-by: louise <louise.soderstrom@neo4j.com>
1 parent 2c61008 commit 4a263f1

File tree

6 files changed

+147
-52
lines changed

6 files changed

+147
-52
lines changed

‎common/src/main/java/apoc/ApocConfig.java

+5-2
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,11 @@ public ApocConfig(Config neo4jConfig, LogService log, GlobalProcedures globalPro
105105
}
106106

107107
// use only for unit tests
108-
public ApocConfig() {
109-
this.neo4jConfig = null;
108+
public ApocConfig(Config neo4jConfig) {
109+
this.neo4jConfig = neo4jConfig;
110+
if (neo4jConfig != null) {
111+
this.blockedIpRanges = neo4jConfig.get(GraphDatabaseInternalSettings.cypher_ip_blocklist);
112+
}
110113
this.log = NullLog.getInstance();
111114
this.databaseManagementService = null;
112115
theInstance = this;

‎common/src/main/java/apoc/util/FileUtils.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import static apoc.ApocConfig.APOC_IMPORT_FILE_ALLOW__READ__FROM__FILESYSTEM;
3333
import static apoc.ApocConfig.apocConfig;
3434
import static apoc.util.Util.ERROR_BYTES_OR_STRING;
35+
import static apoc.util.Util.REDIRECT_LIMIT;
3536
import static apoc.util.Util.readHttpInputStream;
3637

3738
/**
@@ -50,7 +51,7 @@ public static StreamConnection getStreamConnection(SupportedProtocols protocol,
5051
case http:
5152
case https:
5253
case gs:
53-
return readHttpInputStream(urlAddress, headers, payload);
54+
return readHttpInputStream(urlAddress, headers, payload, REDIRECT_LIMIT);
5455
default:
5556
try {
5657
return new StreamConnection.FileStreamConnection(URI.create(urlAddress));

‎common/src/main/java/apoc/util/Util.java

+21-13
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@
106106
import static apoc.export.cypher.formatter.CypherFormatterUtils.formatToString;
107107
import static apoc.util.DateFormatUtil.getOrCreate;
108108
import static java.lang.String.format;
109+
import static java.net.HttpURLConnection.HTTP_NOT_MODIFIED;
109110
import static org.neo4j.configuration.GraphDatabaseSettings.SYSTEM_DATABASE_NAME;
110111
import static org.eclipse.jetty.util.URIUtil.encodePath;
111112

@@ -120,6 +121,8 @@ public class Util {
120121
public static final String COMPILED = "interpreted"; // todo handle enterprise properly
121122
public static final String ERROR_BYTES_OR_STRING = "Only byte[] or url String allowed";
122123

124+
public static final int REDIRECT_LIMIT = 10;
125+
123126
public static String labelString(List<String> labelNames) {
124127
return labelNames.stream().map(Util::quote).collect(Collectors.joining(":"));
125128
}
@@ -336,25 +339,27 @@ public static URLConnection openUrlConnection(String url, Map<String, Object> he
336339
URL src = new URL(url);
337340
URLConnection con = src.openConnection();
338341
con.setRequestProperty("User-Agent", "APOC Procedures for Neo4j");
339-
if (headers != null) {
340-
Object method = headers.get("method");
341-
if (method != null && con instanceof HttpURLConnection) {
342+
if (con instanceof HttpURLConnection) {
342343
HttpURLConnection http = (HttpURLConnection) con;
343-
http.setRequestMethod(method.toString());
344-
http.setChunkedStreamingMode(1024*1024);
345-
http.setInstanceFollowRedirects(true);
344+
http.setInstanceFollowRedirects(false);
345+
if (headers != null) {
346+
Object method = headers.get("method");
347+
if (method != null) {
348+
http.setRequestMethod(method.toString());
349+
http.setChunkedStreamingMode(1024 * 1024);
350+
}
351+
headers.forEach((k, v) -> con.setRequestProperty(k, v == null ? "" : v.toString()));
346352
}
347-
headers.forEach((k,v) -> con.setRequestProperty(k, v == null ? "" : v.toString()));
348353
}
349-
// con.setDoInput(true);
354+
350355
con.setConnectTimeout(apocConfig().getInt("apoc.http.timeout.connect",10_000));
351356
con.setReadTimeout(apocConfig().getInt("apoc.http.timeout.read",60_000));
352357
return con;
353358
}
354359

355360
public static boolean isRedirect(HttpURLConnection con) throws IOException {
356-
int code = con.getResponseCode();
357-
boolean isRedirectCode = code >= 300 && code < 400;
361+
int responseCode = con.getResponseCode();
362+
boolean isRedirectCode = responseCode >= 300 && responseCode <= 307 && responseCode != 306 && responseCode != HTTP_NOT_MODIFIED;
358363
if (isRedirectCode) {
359364
URL location = new URL(con.getHeaderField("Location"));
360365
String oldProtocol = con.getURL().getProtocol();
@@ -413,7 +418,7 @@ private static CountingInputStream getStreamCompressedFile(String urlAddress, Ma
413418
return new CountingInputStream(stream, sc.getLength());
414419
}
415420

416-
private static StreamConnection getStreamConnection(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
421+
public static StreamConnection getStreamConnection(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
417422
return FileUtils.getStreamConnection( FileUtils.from( urlAddress), urlAddress, headers, payload);
418423
}
419424

@@ -431,14 +436,17 @@ private static InputStream getFileStreamIntoCompressedFile(InputStream is, Strin
431436
return null;
432437
}
433438

434-
public static StreamConnection readHttpInputStream(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
439+
public static StreamConnection readHttpInputStream(String urlAddress, Map<String, Object> headers, String payload, int redirectLimit) throws IOException {
435440
ApocConfig.apocConfig().checkReadAllowed(urlAddress);
436441
URLConnection con = openUrlConnection(urlAddress, headers);
437442
writePayload(con, payload);
438443
String newUrl = handleRedirect(con, urlAddress);
439444
if (newUrl != null && !urlAddress.equals(newUrl)) {
440445
con.getInputStream().close();
441-
return readHttpInputStream(newUrl, headers, payload);
446+
if (redirectLimit == 0) {
447+
throw new IOException("Redirect limit exceeded");
448+
}
449+
return readHttpInputStream(newUrl, headers, payload, --redirectLimit);
442450
}
443451

444452
return new StreamConnection.UrlStreamConnection(con);

‎common/src/test/java/apoc/util/UtilIT.java

+99-30
Original file line numberDiff line numberDiff line change
@@ -1,50 +1,45 @@
11
package apoc.util;
22

33
import apoc.ApocConfig;
4+
import inet.ipaddr.IPAddressString;
5+
import junit.framework.TestCase;
46
import org.apache.commons.io.IOUtils;
5-
import org.junit.After;
7+
import org.junit.Assert;
68
import org.junit.Assume;
7-
import org.junit.Before;
8-
import org.junit.Ignore;
9-
import org.junit.Rule;
109
import org.junit.Test;
11-
import org.junit.rules.TestName;
10+
import org.junit.jupiter.api.AfterEach;
11+
import org.neo4j.configuration.Config;
12+
import org.neo4j.configuration.GraphDatabaseInternalSettings;
1213
import org.testcontainers.containers.GenericContainer;
13-
import org.testcontainers.containers.wait.strategy.Wait;
1414

1515
import java.io.IOException;
16+
import java.net.HttpURLConnection;
17+
import java.net.URL;
1618
import java.nio.charset.Charset;
19+
import java.util.ArrayList;
20+
import java.util.List;
1721

1822
import static org.junit.Assert.assertEquals;
1923
import static org.junit.Assert.assertTrue;
24+
import static org.mockito.Mockito.mock;
25+
import static org.mockito.Mockito.when;
2026

2127
public class UtilIT {
22-
23-
@Rule
24-
public TestName testName = new TestName();
25-
2628
private GenericContainer httpServer;
2729

28-
private static final String WITH_URL_LOCATION = "WithUrlLocation";
29-
private static final String WITH_FILE_LOCATION = "WithFileLocation";
30-
31-
@Before
32-
public void setUp() {
33-
new ApocConfig(); // empty test configuration, ensure ApocConfig.apocConfig() can be used
34-
TestUtil.ignoreException(() -> {
35-
httpServer = new GenericContainer("alpine")
36-
.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",
37-
testName.getMethodName().endsWith(WITH_URL_LOCATION) ? "http://www.google.com" : "file:/etc/passwd"))
38-
.withExposedPorts(8000);
39-
httpServer.waitingFor(Wait.forHttp("/")
40-
.forStatusCode(301));
41-
httpServer.start();
42-
}, Exception.class);
30+
private GenericContainer setUpServer(Config neo4jConfig, String redirectURL) {
31+
new ApocConfig(neo4jConfig);
32+
GenericContainer httpServer = new GenericContainer("alpine")
33+
.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",
34+
redirectURL))
35+
.withExposedPorts(8000);
36+
httpServer.start();
4337
Assume.assumeNotNull(httpServer);
4438
Assume.assumeTrue(httpServer.isRunning());
39+
return httpServer;
4540
}
4641

47-
@After
42+
@AfterEach
4843
public void tearDown() {
4944
if (httpServer != null) {
5045
httpServer.stop();
@@ -53,8 +48,42 @@ public void tearDown() {
5348

5449
@Test
5550
public void redirectShouldWorkWhenProtocolNotChangesWithUrlLocation() throws IOException {
51+
httpServer = setUpServer(null, "http://www.google.com");
5652
// given
57-
String url = getServerUrl();
53+
String url = getServerUrl(httpServer);
54+
55+
// when
56+
String page = IOUtils.toString(Util.openInputStream(url, null, null, null), Charset.forName("UTF-8"));
57+
58+
// then
59+
assertTrue(page.contains("<title>Google</title>"));
60+
}
61+
62+
@Test
63+
public void redirectWithBlockedIPsWithUrlLocation() {
64+
List<IPAddressString> blockedIPs = List.of(new IPAddressString("127.168.0.1/8"));
65+
66+
Config neo4jConfig = mock(Config.class);
67+
when(neo4jConfig.get(GraphDatabaseInternalSettings.cypher_ip_blocklist)).thenReturn(blockedIPs);
68+
69+
httpServer = setUpServer(neo4jConfig, "http://127.168.0.1");
70+
String url = getServerUrl(httpServer);
71+
72+
IOException e = Assert.assertThrows(IOException.class,
73+
() -> Util.openInputStream(url, null, null, null)
74+
);
75+
TestCase.assertTrue(e.getMessage().contains("access to /127.168.0.1 is blocked via the configuration property internal.dbms.cypher_ip_blocklist"));
76+
}
77+
78+
@Test
79+
public void redirectWithProtocolUpgradeIsAllowed() throws IOException {
80+
List<IPAddressString> blockedIPs = List.of(new IPAddressString("127.168.0.1/8"));
81+
82+
Config neo4jConfig = mock(Config.class);
83+
when(neo4jConfig.get(GraphDatabaseInternalSettings.cypher_ip_blocklist)).thenReturn(blockedIPs);
84+
85+
httpServer = setUpServer(neo4jConfig, "https://www.google.com");
86+
String url = getServerUrl(httpServer);
5887

5988
// when
6089
String page = IOUtils.toString(Util.openInputStream(url, null, null, null), Charset.forName("UTF-8"));
@@ -63,12 +92,52 @@ public void redirectShouldWorkWhenProtocolNotChangesWithUrlLocation() throws IOE
6392
assertTrue(page.contains("<title>Google</title>"));
6493
}
6594

66-
@Ignore
95+
@Test
96+
public void redirectWithProtocolDowngradeIsNotAllowed() throws IOException {
97+
HttpURLConnection mockCon = mock(HttpURLConnection.class);
98+
when(mockCon.getResponseCode()).thenReturn(302);
99+
when(mockCon.getHeaderField("Location")).thenReturn("http://127.168.0.1");
100+
when(mockCon.getURL()).thenReturn(new URL("https://127.0.0.0"));
101+
102+
RuntimeException e = Assert.assertThrows(RuntimeException.class,
103+
() -> Util.isRedirect(mockCon)
104+
);
105+
106+
TestCase.assertTrue(e.getMessage().contains("The redirect URI has a different protocol: http://127.168.0.1"));
107+
}
108+
109+
@Test
110+
public void shouldFailForExceedingRedirectLimit() {
111+
Config neo4jConfig = mock(Config.class);
112+
113+
httpServer = setUpServer(neo4jConfig, "https://127.0.0.0");
114+
String url = getServerUrl(httpServer);
115+
116+
ArrayList<GenericContainer> servers = new ArrayList<>();
117+
for (int i = 1; i <= 10; i++) {
118+
GenericContainer server = setUpServer(neo4jConfig, url);
119+
servers.add(server);
120+
url = getServerUrl(server);
121+
}
122+
123+
String finalUrl = url;
124+
IOException e = Assert.assertThrows(IOException.class,
125+
() -> Util.openInputStream(finalUrl, null, null, null)
126+
);
127+
128+
TestCase.assertTrue(e.getMessage().contains("Redirect limit exceeded"));
129+
130+
for (GenericContainer server : servers) {
131+
server.stop();
132+
}
133+
}
134+
67135
@Test(expected = RuntimeException.class)
68136
public void redirectShouldThrowExceptionWhenProtocolChangesWithFileLocation() throws IOException {
69137
try {
138+
httpServer = setUpServer(null, "file:/etc/passwd");
70139
// given
71-
String url = getServerUrl();
140+
String url = getServerUrl(httpServer);
72141

73142
// when
74143
Util.openInputStream(url, null, null, null);
@@ -79,7 +148,7 @@ public void redirectShouldThrowExceptionWhenProtocolChangesWithFileLocation() th
79148
}
80149
}
81150

82-
private String getServerUrl() {
151+
private String getServerUrl(GenericContainer httpServer) {
83152
return String.format("http://%s:%s", httpServer.getContainerIpAddress(), httpServer.getMappedPort(8000));
84153
}
85154
}

‎core/src/main/java/apoc/load/Xml.java

+2-4
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@
4444
import java.io.IOException;
4545
import java.io.InputStream;
4646
import java.io.StringReader;
47-
import java.net.URL;
48-
import java.net.URLConnection;
4947
import java.nio.charset.Charset;
5048
import java.util.ArrayDeque;
5149
import java.util.ArrayList;
@@ -64,6 +62,7 @@
6462
import static apoc.util.CompressionConfig.COMPRESSION;
6563
import static apoc.util.FileUtils.getInputStreamFromBinary;
6664
import static apoc.util.Util.ERROR_BYTES_OR_STRING;
65+
import static apoc.util.Util.getStreamConnection;
6766

6867
public class Xml {
6968

@@ -164,8 +163,7 @@ private XMLStreamReader getXMLStreamReader(Object urlOrBinary, XmlImportConfig c
164163
String url = (String) urlOrBinary;
165164
apocConfig.checkReadAllowed(url);
166165
url = FileUtils.changeFileUrlIfImportDirectoryConstrained(url);
167-
URLConnection urlConnection = new URL(url).openConnection();
168-
inputStream = urlConnection.getInputStream();
166+
inputStream = getStreamConnection(url, null, null).getInputStream();
169167
} else if (urlOrBinary instanceof byte[]) {
170168
inputStream = getInputStreamFromBinary((byte[]) urlOrBinary, config.getCompressionAlgo());
171169
} else {

‎core/src/test/java/apoc/load/XmlTest.java

+18-2
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55
import apoc.util.collection.Iterables;
66
import apoc.util.collection.Iterators;
77
import apoc.xml.XmlTestUtils;
8+
import inet.ipaddr.IPAddressString;
9+
import junit.framework.TestCase;
810
import org.apache.commons.io.FileUtils;
911
import org.apache.commons.lang.exception.ExceptionUtils;
12+
import org.junit.Assert;
1013
import org.junit.Before;
1114
import org.junit.Rule;
1215
import org.junit.Test;
16+
import org.neo4j.configuration.GraphDatabaseInternalSettings;
1317
import org.neo4j.graphdb.QueryExecutionException;
1418
import org.neo4j.graphdb.ResourceIterator;
1519
import org.neo4j.test.rule.DbmsRule;
@@ -41,8 +45,8 @@ public class XmlTest {
4145
public static final String FILE_SHORTENED = "src/test/resources/xml/humboldt_soemmering01_1791.TEI-P5-shortened.xml";
4246

4347
@Rule
44-
public DbmsRule db = new ImpermanentDbmsRule();
45-
48+
public DbmsRule db = new ImpermanentDbmsRule()
49+
.withSetting(GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString("127.0.0.0/8")));
4650

4751
@Before
4852
public void setUp() {
@@ -195,6 +199,18 @@ public void testLoadXmlXpathReturnBookFromBookTitle () {
195199
});
196200
}
197201

202+
@Test
203+
public void testLoadXmlWithBlockedIP () {
204+
QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
205+
() -> testCall(db,
206+
"CALL apoc.load.xml('http://127.0.0.0/fake.xml') yield value as result",
207+
map(),
208+
(r) -> {}
209+
)
210+
);
211+
TestCase.assertTrue(e.getMessage().contains("access to /127.0.0.0 is blocked via the configuration property internal.dbms.cypher_ip_blocklist"));
212+
}
213+
198214
@Test
199215
public void testLoadXmlXpathBooKsFromGenre () {
200216
testResult(db, "CALL apoc.load.xml('" + TestUtil.getUrlFileName("xml/books.xml") + "', '/catalog/book[genre=\"Computer\"]') yield value as result",

0 commit comments

Comments
 (0)