SOLR-18345: ClientUtils.encodeLocalParamVal fix for \, ', "

changelog/unreleased/SOLR-18345-encodeLocalParamVal.yml

@@ -0,0 +1,10 @@
+title: >
+  SolrJ ClientUtils.encodeLocalParamVal() can produce lossy/invalid encodings with a backslash or leading quotes.
+  Affects faceting with a custom facet response key.
+  Affects the SQL module for LIKE queries.
+type: fixed
+authors:
+  - name: David Smiley
+links:
+  - name: SOLR-18345
+    url: https://issues.apache.org/jira/browse/SOLR-18345

solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java

@@ -236,8 +236,19 @@ public static String encodeLocalParamVal(String val) {
     int len = val.length();
     if (0 == len) return "''"; // quoted empty string
 
+    // Note: QueryParsing#parseLocalParams's peek() (used to check for a '=' or the closing quote
+    // char) skips leading whitespace as a side effect, so an unquoted empty value would silently
+    // absorb the whitespace meant to separate it from the next local param, corrupting parsing of
+    // everything that follows. Quoting sidesteps this entirely.
+
     int i = 0;
-    if (len > 0 && val.charAt(0) != '$') {
+    char first = val.charAt(0);
+    // A leading '$' would be read back as a param dereference, and a leading quote char would be
+    // read back as the start of a quoted string (StrParser#getQuotedString accepts both ' and "
+    // as delimiters); both must be quoted regardless of the rest of the value.
+    if (first == '$' || first == '\'' || first == '"') {
+      // leave i == 0 so the quoting branch below is taken
+    } else {
       for (; i < len; i++) {
         char ch = val.charAt(i);
         if (Character.isWhitespace(ch) || ch == '}') break;
@@ -246,12 +257,16 @@ public static String encodeLocalParamVal(String val) {
 
     if (i >= len) return val;
 
-    // We need to enclose in quotes... but now we need to escape
+    // We need to enclose in quotes... but now we need to escape.  Both the quote delimiter itself
+    // and a literal backslash must be escaped: StrParser#getQuotedString treats any '\' as the
+    // start of an escape sequence when reading a quoted value, so an un-escaped '\' here would be
+    // silently consumed (or worse, combined with the following char into an unintended escape like
+    // \n) when the value is parsed back.
     StringBuilder sb = new StringBuilder(val.length() + 4);
     sb.append('\'');
     for (i = 0; i < len; i++) {
       char ch = val.charAt(i);
-      if (ch == '\'') {
+      if (ch == '\'' || ch == '\\') {
         sb.append('\\');
       }
       sb.append(ch);

solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java

@@ -16,12 +16,14 @@
  */
 package org.apache.solr.client.solrj.util;
 
+import org.apache.lucene.tests.util.TestUtil;
 import org.apache.solr.SolrTestCase;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.HealthCheckRequest;
 import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
-import org.apache.solr.client.solrj.request.XMLRequestWriter;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.search.QueryParsing;
 import org.junit.Test;
 
 /**
@@ -37,6 +39,49 @@ public void testEscapeQuery() {
     assertEquals("h\\~\\!", ClientUtils.escapeQueryChars("h~!"));
   }
 
+  // FYI also tested via org.apache.solr.common.params.SolrParamTest.testLocalParamRoundTripParsing
+  public void testEncodeLocalParamValRoundTrip() throws Exception {
+    // Values that require quoting (whitespace, '}', or a leading '$') must round-trip through
+    // Solr's own local-params reader, in particular values containing a literal backslash or
+    // single quote.
+    assertRoundTrips("");
+    assertRoundTrips("'leadingQuote");
+    assertRoundTrips("\"leadingDoubleQuote");
+    assertRoundTrips("plain");
+    assertRoundTrips("has space");
+    assertRoundTrips("trailing}brace");
+    assertRoundTrips("has'quote and space");
+    assertRoundTrips("has\\backslash and space");
+    assertRoundTrips("both\\'kinds together");
+    assertRoundTrips("$dollarPrefixed");
+    assertRoundTrips("$dollarPrefixed with space");
+    assertRoundTrips("$\\'mix of everything");
+
+    for (int i = 0; i < 100; i++) {
+      assertRoundTrips(TestUtil.randomUnicodeString(random()));
+    }
+  }
+
+  private void assertRoundTrips(String original) throws Exception {
+    String encoded = ClientUtils.encodeLocalParamVal(original);
+    String txt = "{!key=" + encoded + "}";
+    ModifiableSolrParams target = new ModifiableSolrParams();
+    QueryParsing.parseLocalParams(txt, 0, target, null);
+    assertEquals(
+        "encodeLocalParamVal(" + original + ") -> " + encoded + " did not round-trip",
+        original,
+        target.get("key"));
+
+    // Also confirm the encoded value doesn't swallow whatever follows it.
+    String txtFollowedByAnother = "{!key=" + encoded + " next=followed}";
+    ModifiableSolrParams targetFollowedByAnother = new ModifiableSolrParams();
+    QueryParsing.parseLocalParams(txtFollowedByAnother, 0, targetFollowedByAnother, null);
+    assertEquals(
+        "encodeLocalParamVal(" + original + ") -> " + encoded + " swallowed the next local param",
+        "followed",
+        targetFollowedByAnother.get("next"));
+  }
+
   @Test
   public void testDeterminesWhenToUseDefaultCollection() {
     final var noDefaultNeededRequest = new CollectionAdminRequest.List();
@@ -55,7 +100,6 @@ public void testDeterminesWhenToUseDefaultCollection() {
 
   @Test
   public void testUrlBuilding() throws Exception {
-    final var rw = new XMLRequestWriter();
     // Simple case, non-collection request
     {
       final var request = new HealthCheckRequest();