[7.8] SQL: fix handling of escaped chars in JDBC connection string (#…

…58429) (#58976)

SQL: fix handling of escaped chars in JDBC connection string (#58429)

This commit fixes an issue emerging when the connection string URI
contains escaped characters.

The original URI is pre-parsed in order to re-assemble a new URI having
the optional elements filled in with defaults. The new URI has been
using however the unescaped query and fragment parts. So if these
contained any escaped `&` or `=` (such as in the password option value),
the unescaping would reveal them and make them later interfere with the
options parsing.

The commit changes that, so that the new URI be built from the unescaped
"raw" parts of the original URI.

(cherry picked from commit 94eb5a0)

docs/reference/sql/endpoints/jdbc.asciidoc

@@ -51,9 +51,9 @@ Once registered, the driver understands the following syntax as an URL:
 
 ["source","text",subs="attributes"]
 ----
-jdbc:es://[[http|https]://]?[host[:port]]?/[prefix]?[\?[option=value]&]*
+jdbc:[es|elasticsearch]://[[http|https]://]?[host[:port]]?/[prefix]?[\?[option=value]&]*
 ----
-`jdbc:es://`:: Prefix. Mandatory.
+`jdbc:[es|elasticsearch]://`:: Prefix. Mandatory.
 
 `[[http|https]://]`:: Type of HTTP connection to make. Possible values are
 `http` (default) or `https`. Optional.
@@ -154,7 +154,7 @@ To put all of it together, the following URL:
 jdbc:es://http://server:3456/?timezone=UTC&page.size=250
 ----
 
-Opens up a {es-sql} connection to `server` on port `3456`, setting the JDBC connection timezone to `UTC` and its pagesize to `250` entries.
+opens up a {es-sql} connection to `server` on port `3456`, setting the JDBC connection timezone to `UTC` and its pagesize to `250` entries.
 
 === API usage
 

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcConfiguration.java

@@ -10,6 +10,7 @@
 import org.elasticsearch.xpack.sql.client.StringUtils;
 
 import java.net.URI;
+import java.net.URLDecoder;
 import java.sql.DriverPropertyInfo;
 import java.time.ZoneId;
 import java.util.ArrayList;
@@ -37,6 +38,7 @@
 //TODO: beef this up for Security/SSL
 public class JdbcConfiguration extends ConnectionConfiguration {
     static final String URL_PREFIX = "jdbc:es://";
+    static final String URL_FULL_PREFIX = "jdbc:elasticsearch://";
     public static URI DEFAULT_URI = URI.create("http://localhost:9200/");
 
 
@@ -127,6 +129,8 @@ private static URI parseUrl(String u) throws JdbcSQLException {
     private static String removeJdbcPrefix(String connectionString) throws JdbcSQLException {
         if (connectionString.startsWith(URL_PREFIX)) {
             return connectionString.substring(URL_PREFIX.length());
+        } else if (connectionString.startsWith(URL_FULL_PREFIX)) {
+            return connectionString.substring(URL_FULL_PREFIX.length());
         } else {
             throw new JdbcSQLException("Expected [" + URL_PREFIX + "] url, received [" + connectionString + "]");
         }
@@ -143,8 +147,10 @@ private static Properties parseProperties(URI uri, String u) throws JdbcSQLExcep
                     if (args.size() != 2) {
                         throw new JdbcSQLException("Invalid parameter [" + param + "], format needs to be key=value");
                     }
+                    final String key = URLDecoder.decode(args.get(0), "UTF-8").trim();
+                    final String val = URLDecoder.decode(args.get(1), "UTF-8");
                     // further validation happens in the constructor (since extra properties might be specified either way)
-                    props.setProperty(args.get(0).trim(), args.get(1).trim());
+                    props.setProperty(key, val);
                 }
             }
         } catch (JdbcSQLException e) {
@@ -208,7 +214,9 @@ public boolean indexIncludeFrozen() {
     }
 
     public static boolean canAccept(String url) {
-        return (StringUtils.hasText(url) && url.trim().startsWith(JdbcConfiguration.URL_PREFIX));
+        String u = url.trim();
+        return (StringUtils.hasText(u) &&
+            (u.startsWith(JdbcConfiguration.URL_PREFIX) || u.startsWith(JdbcConfiguration.URL_FULL_PREFIX)));
     }
 
     public DriverPropertyInfo[] driverPropertyInfo() {

x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcConfigurationTests.java

@@ -12,6 +12,7 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.URLEncoder;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.sql.DriverManager;
@@ -27,6 +28,8 @@
 import static org.elasticsearch.xpack.sql.client.ConnectionConfiguration.PAGE_TIMEOUT;
 import static org.elasticsearch.xpack.sql.client.ConnectionConfiguration.PROPERTIES_VALIDATION;
 import static org.elasticsearch.xpack.sql.client.ConnectionConfiguration.QUERY_TIMEOUT;
+import static org.elasticsearch.xpack.sql.jdbc.JdbcConfiguration.URL_FULL_PREFIX;
+import static org.elasticsearch.xpack.sql.jdbc.JdbcConfiguration.URL_PREFIX;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 
@@ -43,131 +46,145 @@ public void testInvalidUrl() {
     }
 
     public void testJustThePrefix() throws Exception {
-       Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es:"));
-       assertEquals("Expected [jdbc:es://] url, received [jdbc:es:]", e.getMessage());
+        Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es:"));
+        assertEquals("Expected [jdbc:es://] url, received [jdbc:es:]", e.getMessage());
     }
 
     public void testJustTheHost() throws Exception {
         assertThat(ci("jdbc:es://localhost").baseUri().toString(), is("http://localhost:9200/"));
+        assertThat(ci("jdbc:elasticsearch://localhost").baseUri().toString(), is("http://localhost:9200/"));
+    }
+
+    private static String jdbcPrefix() {
+        return randomFrom(URL_PREFIX, URL_FULL_PREFIX);
     }
 
     public void testHostAndPort() throws Exception {
-        assertThat(ci("jdbc:es://localhost:1234").baseUri().toString(), is("http://localhost:1234/"));
+        assertThat(ci(jdbcPrefix() + "localhost:1234").baseUri().toString(), is("http://localhost:1234/"));
+    }
+
+    public void testPropertiesEscaping() throws Exception {
+        String pass = randomUnicodeOfLengthBetween(1, 500);
+        String encPass = URLEncoder.encode(pass, "UTF-8").replace("+", "%20");
+        String url = jdbcPrefix() + "test?password=" + encPass;
+        JdbcConfiguration ci = ci(url);
+        assertEquals(pass, ci.authPass());
     }
 
     public void testTrailingSlashForHost() throws Exception {
-        assertThat(ci("jdbc:es://localhost:1234/").baseUri().toString(), is("http://localhost:1234/"));
+        assertThat(ci(jdbcPrefix() + "localhost:1234/").baseUri().toString(), is("http://localhost:1234/"));
     }
 
     public void testMultiPathSuffix() throws Exception {
-        assertThat(ci("jdbc:es://a:1/foo/bar/tar").baseUri().toString(), is("http://a:1/foo/bar/tar"));
+        assertThat(ci(jdbcPrefix() + "a:1/foo/bar/tar").baseUri().toString(), is("http://a:1/foo/bar/tar"));
     }
 
     public void testV6Localhost() throws Exception {
-        assertThat(ci("jdbc:es://[::1]:54161/foo/bar").baseUri().toString(), is("http://[::1]:54161/foo/bar"));
+        assertThat(ci(jdbcPrefix() + "[::1]:54161/foo/bar").baseUri().toString(), is("http://[::1]:54161/foo/bar"));
     }
 
     public void testDebug() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://a:1/?debug=true");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "a:1/?debug=true");
         assertThat(ci.baseUri().toString(), is("http://a:1/"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.debugOut(), is("err"));
     }
 
     public void testDebugOut() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://a:1/?debug=true&debug.output=jdbc.out");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "a:1/?debug=true&debug.output=jdbc.out");
         assertThat(ci.baseUri().toString(), is("http://a:1/"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.debugOut(), is("jdbc.out"));
     }
 
     public void testDebugFlushAlways() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://a:1/?debug=true&debug.flushAlways=false");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "a:1/?debug=true&debug.flushAlways=false");
         assertThat(ci.baseUri().toString(), is("http://a:1/"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.flushAlways(), is(false));
 
-        ci = ci("jdbc:es://a:1/?debug=true&debug.flushAlways=true");
+        ci = ci(jdbcPrefix() + "a:1/?debug=true&debug.flushAlways=true");
         assertThat(ci.baseUri().toString(), is("http://a:1/"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.flushAlways(), is(true));
 
-        ci = ci("jdbc:es://a:1/?debug=true");
+        ci = ci(jdbcPrefix() + "a:1/?debug=true");
         assertThat(ci.baseUri().toString(), is("http://a:1/"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.flushAlways(), is(false));
     }
 
     public void testTypeInParam() throws Exception {
-        Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://a:1/foo/bar/tar?debug=true&debug.out=jdbc.out"));
+        Exception e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "a:1/foo/bar/tar?debug=true&debug.out=jdbc.out"));
         assertEquals("Unknown parameter [debug.out]; did you mean [debug.output]", e.getMessage());
     }
 
     public void testDebugOutWithSuffix() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://a:1/foo/bar/tar?debug=true&debug.output=jdbc.out");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "a:1/foo/bar/tar?debug=true&debug.output=jdbc.out");
         assertThat(ci.baseUri().toString(), is("http://a:1/foo/bar/tar"));
         assertThat(ci.debug(), is(true));
         assertThat(ci.debugOut(), is("jdbc.out"));
     }
 
     public void testHttpWithSSLEnabledFromProperty() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://test?ssl=true");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "test?ssl=true");
         assertThat(ci.baseUri().toString(), is("https://test:9200/"));
     }
 
     public void testHttpWithSSLEnabledFromPropertyAndDisabledFromProtocol() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://http://test?ssl=true");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "http://test?ssl=true");
         assertThat(ci.baseUri().toString(), is("https://test:9200/"));
     }
 
     public void testHttpWithSSLEnabledFromProtocol() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://https://test:9200");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "https://test:9200");
         assertThat(ci.baseUri().toString(), is("https://test:9200/"));
     }
 
     public void testHttpWithSSLEnabledFromProtocolAndProperty() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://https://test:9200?ssl=true");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "https://test:9200?ssl=true");
         assertThat(ci.baseUri().toString(), is("https://test:9200/"));
     }
 
     public void testHttpWithSSLDisabledFromProperty() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://test?ssl=false");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "test?ssl=false");
         assertThat(ci.baseUri().toString(), is("http://test:9200/"));
     }
 
     public void testHttpWithSSLDisabledFromPropertyAndProtocol() throws Exception {
-        JdbcConfiguration ci = ci("jdbc:es://http://test?ssl=false");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "http://test?ssl=false");
         assertThat(ci.baseUri().toString(), is("http://test:9200/"));
     }
 
     public void testHttpWithSSLDisabledFromPropertyAndEnabledFromProtocol() throws Exception {
-        Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://https://test?ssl=false"));
+        Exception e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "https://test?ssl=false"));
         assertEquals("Cannot enable SSL: HTTPS protocol being used in the URL and SSL disabled in properties", e.getMessage());
     }
 
     public void testValidatePropertiesDefault() {
-        Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://test:9200?pagee.size=12"));
+        Exception e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "test:9200?pagee.size=12"));
         assertEquals("Unknown parameter [pagee.size]; did you mean [page.size]", e.getMessage());
 
-        e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://test:9200?foo=bar"));
+        e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "test:9200?foo=bar"));
         assertEquals("Unknown parameter [foo]; did you mean [ssl]", e.getMessage());
     }
 
     public void testValidateProperties() {
-        Exception e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://test:9200?pagee.size=12&validate.properties=true"));
+        Exception e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "test:9200?pagee.size=12&validate.properties=true"));
         assertEquals("Unknown parameter [pagee.size]; did you mean [page.size]", e.getMessage());
 
-        e = expectThrows(JdbcSQLException.class, () -> ci("jdbc:es://test:9200?&validate.properties=true&something=some_value"));
+        e = expectThrows(JdbcSQLException.class, () -> ci(jdbcPrefix() + "test:9200?&validate.properties=true&something=some_value"));
         assertEquals("Unknown parameter [something]; did you mean []", e.getMessage());
 
         Properties properties  = new Properties();
         properties.setProperty(PROPERTIES_VALIDATION, "true");
-        e = expectThrows(JdbcSQLException.class, () -> JdbcConfiguration.create("jdbc:es://test:9200?something=some_value", properties, 0));
+        e = expectThrows(JdbcSQLException.class,
+            () -> JdbcConfiguration.create(jdbcPrefix() + "test:9200?something=some_value", properties, 0));
         assertEquals("Unknown parameter [something]; did you mean []", e.getMessage());
     }
 
     public void testNoPropertiesValidation() throws SQLException {
-        JdbcConfiguration ci = ci("jdbc:es://test:9200?pagee.size=12&validate.properties=false");
+        JdbcConfiguration ci = ci(jdbcPrefix() + "test:9200?pagee.size=12&validate.properties=false");
         assertEquals(false, ci.validateProperties());
 
         // URL properties test
@@ -177,8 +194,9 @@ public void testNoPropertiesValidation() throws SQLException {
         long pageTimeout = randomNonNegativeLong();
         int pageSize = randomIntBetween(0, Integer.MAX_VALUE);
 
-        ci = ci("jdbc:es://test:9200?validate.properties=false&something=some_value&query.timeout=" + queryTimeout + "&connect.timeout="
-                + connectTimeout + "&network.timeout=" + networkTimeout + "&page.timeout=" + pageTimeout + "&page.size=" + pageSize);
+        ci = ci(jdbcPrefix() + "test:9200?validate.properties=false&something=some_value&query.timeout=" + queryTimeout
+                + "&connect.timeout=" + connectTimeout + "&network.timeout=" + networkTimeout + "&page.timeout=" + pageTimeout
+                + "&page.size=" + pageSize);
         assertEquals(false, ci.validateProperties());
         assertEquals(queryTimeout, ci.queryTimeout());
         assertEquals(connectTimeout, ci.connectTimeout());
@@ -196,7 +214,7 @@ public void testNoPropertiesValidation() throws SQLException {
         properties.put(PAGE_SIZE, Integer.toString(pageSize));
 
         // also putting validate.properties in URL to be overriden by the properties value
-        ci = JdbcConfiguration.create("jdbc:es://test:9200?validate.properties=true&something=some_value", properties, 0);
+        ci = JdbcConfiguration.create(jdbcPrefix() + "test:9200?validate.properties=true&something=some_value", properties, 0);
         assertEquals(false, ci.validateProperties());
         assertEquals(queryTimeout, ci.queryTimeout());
         assertEquals(connectTimeout, ci.connectTimeout());
@@ -210,7 +228,7 @@ public void testTimoutOverride() throws Exception {
         properties.setProperty(CONNECT_TIMEOUT, "3"); // Should be overridden
         properties.setProperty(PAGE_TIMEOUT, "4");
 
-        String url = "jdbc:es://test?connect.timeout=1&page.timeout=2";
+        String url = jdbcPrefix() + "test?connect.timeout=1&page.timeout=2";
 
         // No properties
         JdbcConfiguration ci = JdbcConfiguration.create(url, null, 0);
@@ -235,7 +253,7 @@ public void testSSLPropertiesInUrl() throws Exception {
         allProps.putAll(urlPropMap);
         String sslUrlProps = urlPropMap.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&"));
 
-        assertSslConfig(allProps, ci("jdbc:es://test?" + sslUrlProps.toString()).sslConfig());
+        assertSslConfig(allProps, ci(jdbcPrefix() + "test?" + sslUrlProps.toString()).sslConfig());
     }
 
     public void testSSLPropertiesInUrlAndProperties() throws Exception {
@@ -258,7 +276,7 @@ public void testSSLPropertiesInUrlAndProperties() throws Exception {
         Properties allProps = new Properties();
         allProps.putAll(urlPropMap);
         allProps.putAll(propMap);
-        assertSslConfig(allProps, JdbcConfiguration.create("jdbc:es://test?" + sslUrlProps.toString(), props, 0).sslConfig());
+        assertSslConfig(allProps, JdbcConfiguration.create(jdbcPrefix() + "test?" + sslUrlProps.toString(), props, 0).sslConfig());
     }
 
     public void testSSLPropertiesOverride() throws Exception {
@@ -295,7 +313,7 @@ public void testDriverConfigurationWithSSLInURL() {
         });
 
         try {
-            DriverManager.getDriver("jdbc:es://test?" + sslUrlProps);
+            DriverManager.getDriver(jdbcPrefix() + "test?" + sslUrlProps);
         } catch (SQLException sqle) {
             fail("Driver registration should have been successful. Error: " + sqle);
         }
@@ -347,12 +365,12 @@ static void assertSslConfig(Properties allProperties, SslConfig sslConfig) throw
     }
 
     private void assertJdbcSqlExceptionFromUrl(String wrongSetting, String correctSetting) {
-        String url = "jdbc:es://test?" + wrongSetting + "=foo";
+        String url = jdbcPrefix() + "test?" + wrongSetting + "=foo";
         assertJdbcSqlException(wrongSetting, correctSetting, url, null);
     }
 
     private void assertJdbcSqlExceptionFromProperties(String wrongSetting, String correctSetting) {
-        String url = "jdbc:es://test";
+        String url = jdbcPrefix() + "test";
         Properties props = new Properties();
         props.put(wrongSetting, correctSetting);
         assertJdbcSqlException(wrongSetting, correctSetting, url, props);

x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/UriUtils.java

@@ -18,12 +18,26 @@ private UriUtils() {
      */
     public static URI parseURI(String connectionString, URI defaultURI) {
         final URI uri = parseWithNoScheme(connectionString);
+        // Repack the connection string with provided default elements - where missing from the original string - and reparse into a URI.
         final String path = "".equals(uri.getPath()) ? defaultURI.getPath() : uri.getPath();
-        final String query = uri.getQuery() == null ? defaultURI.getQuery() : uri.getQuery();
+        final String rawQuery = uri.getQuery() == null ? defaultURI.getRawQuery() : uri.getRawQuery();
+        final String rawFragment = uri.getFragment() == null ? defaultURI.getRawFragment() : uri.getRawFragment();
         final int port = uri.getPort() < 0 ? defaultURI.getPort() : uri.getPort();
         try {
-            return new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), port, path, query, defaultURI.getFragment());
+            // The query part is attached in original "raw" format, to preserve the escaping of characters. This is needed since any
+            // escaped query structure characters (`&` and `=`) wouldn't remain escaped when passed back through the URI constructor
+            // (since they are legal in the query part), and that would later interfere with correctly parsing the attributes.
+            // And same with escaped `#` chars in the fragment part.
+            String connStr = new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), port, path, null, null).toString();
+            if (StringUtils.hasLength(rawQuery)) {
+                connStr += "?" + rawQuery;
+            }
+            if (StringUtils.hasLength(rawFragment)) {
+                connStr += "#" + rawFragment;
+            }
+            return new URI(connStr);
         } catch (URISyntaxException e) {
+            // should only happen if the defaultURI is malformed
             throw new IllegalArgumentException("Invalid connection configuration [" + connectionString + "]: " + e.getMessage(), e);
         }
     }