Copilot commented on code in PR #13221:
URL: https://github.com/apache/ignite/pull/13221#discussion_r3549471349
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -2587,6 +2595,47 @@ public static URL resolveSpringUrl(String springCfgPath)
throws IgniteCheckedExc
try {
url = new URL(springCfgPath);
+
+ URL cfgUrl = url;
+
+ String scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+
+ // Handle jar:<nested_url>!/path to avoid bypassing remote-scheme
checks.
+ if ("jar".equals(scheme)) {
+ String file = cfgUrl.getFile();
+
+ int sep = file.indexOf("!/");
+
+ if (sep > 0) {
+ try {
+ cfgUrl = new URL(file.substring(0, sep));
+ scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+ }
+ catch (MalformedURLException ignored) {
+ // No-op.
+ }
+ }
+ }
Review Comment:
The `jar:` unwrapping only peels a single layer, so a nested URL like
`jar:jar:http://...!/x!/y` would leave `scheme` as `jar` after the first unwrap
and bypass the remote-scheme block. To fully prevent bypasses, unwrap
repeatedly while the effective scheme is `jar` and the `!/` separator is
present.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -2587,6 +2595,47 @@ public static URL resolveSpringUrl(String springCfgPath)
throws IgniteCheckedExc
try {
url = new URL(springCfgPath);
+
+ URL cfgUrl = url;
+
+ String scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+
+ // Handle jar:<nested_url>!/path to avoid bypassing remote-scheme
checks.
+ if ("jar".equals(scheme)) {
+ String file = cfgUrl.getFile();
+
+ int sep = file.indexOf("!/");
+
+ if (sep > 0) {
+ try {
+ cfgUrl = new URL(file.substring(0, sep));
+ scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+ }
+ catch (MalformedURLException ignored) {
+ // No-op.
+ }
+ }
+ }
+
+ if (REMOTE_CFG_SCHEMES.contains(scheme)) {
+ if (ALWAYS_BLOCKED_CFG_SCHEMES.contains(scheme))
+ throw new IgniteCheckedException(
+ "Spring configuration URLs with scheme '" + scheme +
"' are always blocked " +
+ "due to security risk. Use HTTPS or a local
file/classpath reference instead. " +
+ "Provided host: " + cfgUrl.getHost()
+ );
+
+ boolean allowRemote =
Boolean.getBoolean(IgniteSystemProperties.IGNITE_ALLOW_REMOTE_SPRING_CFG_URL);
Review Comment:
For consistency with the rest of this class (and other Ignite code), prefer
`IgniteSystemProperties.getBoolean(...)` over `Boolean.getBoolean(...)` when
reading Ignite system properties.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -2587,6 +2595,47 @@ public static URL resolveSpringUrl(String springCfgPath)
throws IgniteCheckedExc
try {
url = new URL(springCfgPath);
+
+ URL cfgUrl = url;
+
+ String scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+
+ // Handle jar:<nested_url>!/path to avoid bypassing remote-scheme
checks.
+ if ("jar".equals(scheme)) {
+ String file = cfgUrl.getFile();
+
+ int sep = file.indexOf("!/");
+
+ if (sep > 0) {
+ try {
+ cfgUrl = new URL(file.substring(0, sep));
+ scheme = cfgUrl.getProtocol().toLowerCase(Locale.ROOT);
+ }
+ catch (MalformedURLException ignored) {
+ // No-op.
+ }
+ }
+ }
+
+ if (REMOTE_CFG_SCHEMES.contains(scheme)) {
+ if (ALWAYS_BLOCKED_CFG_SCHEMES.contains(scheme))
+ throw new IgniteCheckedException(
+ "Spring configuration URLs with scheme '" + scheme +
"' are always blocked " +
+ "due to security risk. Use HTTPS or a local
file/classpath reference instead. " +
+ "Provided host: " + cfgUrl.getHost()
+ );
Review Comment:
The FTP/FTPS error message advises using HTTPS, but HTTPS is also blocked by
default unless `ignite.spring.cfg.allowRemoteUrl` is enabled. The guidance
would be clearer if it recommended local file/classpath by default and
explicitly mentioned the flag for HTTP/HTTPS if remote loading is required.
##########
modules/clients/src/test/java/org/apache/ignite/internal/jdbc2/JdbcConnectionSelfTest.java:
##########
@@ -296,4 +306,47 @@ 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";
+ final String expMsg = scheme.startsWith("ftp") ? "always blocked"
: "Remote Spring configuration URLs";
+
+ GridTestUtils.assertThrows(
+ log,
+ new Callable<Object>() {
+ @Override public Object call() throws Exception {
+ try (Connection conn =
DriverManager.getConnection(url)) {
+ return conn;
+ }
+ }
+ },
+ SQLException.class,
+ expMsg
+ );
+ }
+ }
+
+ /**
+ * Test that JDBC cfg:// URL with remote HTTP location is allowed when
system property is set.
+ */
+ @Test
+ @WithSystemProperty(key = "ignite.spring.cfg.allowRemoteUrl", value =
"true")
Review Comment:
Use the shared system-property constant instead of duplicating the literal
key string, to keep this test aligned if the property name changes.
##########
modules/clients/src/test/java/org/apache/ignite/internal/jdbc2/JdbcConnectionSelfTest.java:
##########
@@ -296,4 +306,47 @@ 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";
+ final String expMsg = scheme.startsWith("ftp") ? "always blocked"
: "Remote Spring configuration URLs";
+
+ GridTestUtils.assertThrows(
+ log,
+ new Callable<Object>() {
+ @Override public Object call() throws Exception {
+ try (Connection conn =
DriverManager.getConnection(url)) {
+ return conn;
+ }
+ }
+ },
+ SQLException.class,
+ expMsg
+ );
+ }
+ }
+
+ /**
+ * Test that JDBC cfg:// URL with remote HTTP location is allowed when
system property is set.
+ */
+ @Test
+ @WithSystemProperty(key = "ignite.spring.cfg.allowRemoteUrl", value =
"true")
+ public void testRemoteHttpCfgUrlAllowedWhenFlagSet() {
+ final String url = CFG_URL_PREFIX +
"http://127.0.0.1:1/nonexistent.xml";;
+
+ try {
+ DriverManager.getConnection(url);
+ }
+ catch (SQLException e) {
+ assertFalse(
+ "Security exception should not be thrown when flag is enabled",
+ e.getMessage().contains("Remote Spring configuration URLs")
+ );
+ }
Review Comment:
`testRemoteHttpCfgUrlAllowedWhenFlagSet` does not fail if
`DriverManager.getConnection(url)` unexpectedly succeeds (no assertions after
the `try`). It should assert that a `SQLException` is thrown for the
nonexistent URL, and then verify that the thrown message is not the
security-block message.
##########
modules/core/src/test/java/org/apache/ignite/internal/util/IgniteUtilsSelfTest.java:
##########
@@ -1615,6 +1616,78 @@ 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")
Review Comment:
This test uses a string literal for the system property key, while this file
already uses `IgniteSystemProperties.*` constants for `@WithSystemProperty`.
Using the constant avoids typos and keeps tests aligned if the property name
changes.
##########
modules/core/src/test/java/org/apache/ignite/internal/util/IgniteUtilsSelfTest.java:
##########
@@ -1615,6 +1616,78 @@ 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() throws
IgniteCheckedException {
+ URL url =
IgniteUtils.resolveSpringUrl("http://127.0.0.1:1/nonexistent.xml";);
+
+ assertNotNull(url);
+ assertEquals("http", url.getProtocol());
+ }
+
+ /**
+ * Test that FTP is always blocked even when remote URL property is set.
+ */
+ @Test
+ @WithSystemProperty(key = "ignite.spring.cfg.allowRemoteUrl", value =
"true")
Review Comment:
Same as above: prefer using
`IgniteSystemProperties.IGNITE_ALLOW_REMOTE_SPRING_CFG_URL` instead of
duplicating the property string literal.
--
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]
