Copilot commented on code in PR #13221:
URL: https://github.com/apache/ignite/pull/13221#discussion_r3481546847
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -2600,6 +2608,31 @@ public static URL resolveSpringUrl(String springCfgPath)
throws IgniteCheckedExc
try {
url = new URL(springCfgPath);
+
+ String scheme = url.getProtocol().toLowerCase();
+
+ if (REMOTE_CFG_SCHEMES.contains(scheme)) {
+ // FTP is always blocked — unencrypted, susceptible to MITM
+ if (ALWAYS_BLOCKED_CFG_SCHEMES.contains(scheme))
+ throw new IgniteCheckedException(
+ "Spring configuration URLs with scheme '" + scheme +
"' are always blocked " +
+ "due to security risk (unencrypted transfer, MITM
vulnerability). " +
+ "Use HTTPS or a local file/classpath reference
instead. " +
+ "Provided host: " + url.getHost()
+ );
+
+ // HTTP/HTTPS blocked by default, allowed via system property
+ boolean allowRemote =
Boolean.getBoolean(IgniteSystemProperties.IGNITE_ALLOW_REMOTE_SPRING_CFG_URL);
+
+ if (!allowRemote)
+ throw new IgniteCheckedException(
+ "Remote Spring configuration URLs (http/https) are not
allowed by default " +
+ "to prevent remote code execution via
attacker-controlled Spring XML. " +
+ "Provided host: " + url.getHost() + ". " +
+ "To allow remote URLs set system property: -D" +
+
IgniteSystemProperties.IGNITE_ALLOW_REMOTE_SPRING_CFG_URL + "=true"
+ );
+ }
Review Comment:
Remote URL blocking can be bypassed with nested URL schemes such as
`jar:http(s)://...!/cfg.xml` because the current check only inspects the
top-level protocol. Also, `toLowerCase()` should use a fixed locale to avoid
locale-dependent behavior in security checks, and the FTP/FTPS message
currently claims "unencrypted transfer" even for FTPS.
##########
modules/core/src/main/java/org/apache/ignite/IgniteSystemProperties.java:
##########
@@ -1967,6 +1967,15 @@ public final class IgniteSystemProperties extends
IgniteCommonsSystemProperties
@SystemProperty(value = "Packages list to expose in configuration view")
public static final String IGNITE_CONFIGURATION_VIEW_PACKAGES =
"IGNITE_CONFIGURATION_VIEW_PACKAGES";
+
+ /**
+ * System property to allow remote HTTP/HTTPS URLs when loading Spring XML
configuration.
+ * Remote URLs are blocked by default to prevent RCE via
attacker-controlled Spring XML.
+ * FTP is always blocked regardless of this property due to MITM risk.
+ */
Review Comment:
The Javadoc says only FTP is always blocked, but the implementation blocks
both FTP and FTPS (see `ALWAYS_BLOCKED_CFG_SCHEMES`). The property description
should match actual behavior to avoid misleading users.
##########
modules/core/src/test/java/org/apache/ignite/internal/util/IgniteUtilsSelfTest.java:
##########
@@ -1615,6 +1615,85 @@ public void testLongToBytes() {
}
}
+ /**
+ * Test that remote HTTPS URL in Spring cfg is blocked by default.
+ */
+ @Test
+ public void testResolveSpringUrlBlocksHttpsByDefault() {
+ assertThrows(log, () -> {
+
IgniteUtils.resolveSpringUrl("https://attacker.example.com/evil.xml";);
+ return null;
+ }, IgniteCheckedException.class, "Remote Spring configuration URLs");
+ }
+
+ /**
+ * Test that remote FTP URL in Spring cfg is blocked by default.
+ */
+ @Test
+ public void testResolveSpringUrlBlocksFtpByDefault() {
+ assertThrows(log, () -> {
+
IgniteUtils.resolveSpringUrl("ftp://attacker.example.com/evil.xml";);
+ return null;
+ }, IgniteCheckedException.class, "always blocked");
+ }
+
+ /**
+ * Test that remote HTTP URL in Spring cfg is blocked by default
+ * and error message contains guidance on how to enable remote URLs.
+ */
+ @Test
+ public void testResolveSpringUrlBlocksHttpByDefault() {
+ try {
+
IgniteUtils.resolveSpringUrl("http://attacker.example.com/evil.xml";);
+ fail("Expected IgniteCheckedException");
+ }
+ catch (IgniteCheckedException e) {
+ assertTrue(
+ "Error message should contain system property name",
+
e.getMessage().contains(IgniteSystemProperties.IGNITE_ALLOW_REMOTE_SPRING_CFG_URL)
+ );
+ assertFalse(
+ "Error message should not contain full URL to avoid credential
leak",
+ e.getMessage().contains("http://attacker.example.com/evil.xml";)
+ );
+ assertTrue(
+ "Error message should contain host",
+ e.getMessage().contains("attacker.example.com")
+ );
+ }
+ }
+
+ /**
+ * Test that remote HTTP URL is allowed when system property is set.
+ */
+ @Test
+ @WithSystemProperty(key = "ignite.spring.cfg.allowRemoteUrl", value =
"true")
+ public void testResolveSpringUrlAllowsHttpWhenPropertySet() {
+ // Should not throw — validation passes when flag is true.
+ // Will throw MalformedURLException or connection error, not our
security check.
+ try {
+ IgniteUtils.resolveSpringUrl("http://127.0.0.1:1/nonexistent.xml";);
+ }
+ catch (IgniteCheckedException e) {
+ assertFalse(
+ "Should not throw security exception when flag is enabled",
+ e.getMessage().contains("Remote Spring configuration URLs")
+ );
+ }
Review Comment:
This test catches and effectively ignores `IgniteCheckedException` unless it
matches a specific substring, which can hide regressions (with the flag
enabled, `resolveSpringUrl` should not throw at all for a syntactically valid
URL). Also, `resolveSpringUrl` does not perform network access, so the comment
about connection errors is misleading.
##########
modules/clients/src/test/java/org/apache/ignite/internal/jdbc2/JdbcConnectionSelfTest.java:
##########
@@ -296,4 +306,46 @@ public void testSqlHints() throws Exception {
assertTrue(((JdbcConnection)conn).skipReducerOnUpdate());
}
}
-}
+
+ /**
+ * Test that JDBC cfg:// URL with remote HTTP, HTTPS, and FTP location is
blocked.
+ */
+ @Test
+ public void testRemoteCfgUrlsAreBlocked() {
+ for (String scheme : Arrays.asList("http", "https", "ftp", "ftps")) {
+ final String url = CFG_URL_PREFIX + scheme +
"://attacker.example.com/evil.xml";
+
+ GridTestUtils.assertThrows(
+ log,
+ new Callable<Object>() {
+ @Override public Object call() throws Exception {
+ try (Connection conn =
DriverManager.getConnection(url)) {
+ return conn;
+ }
+ }
+ },
+ SQLException.class,
+ null
+ );
+ }
+ }
Review Comment:
Passing `null` as the expected message means this test doesn't verify that
failures are caused by the new remote-URL blocking (it could fail for unrelated
reasons). Please assert an expected substring per scheme so the test validates
the security behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
