SOLR-13181: param macro expansion could throw (#1877)

...StringIndexOutOfBoundsException on bad syntax
* failOnMissingParams: should have been returning null (failing) on bad syntax cases

Co-authored-by: Christine Poerschke <cpoerschke@apache.org>

solr/CHANGES.txt

@@ -244,6 +244,10 @@ Bug Fixes
 * SOLR-14821: {!terms} docValuesTermsFilterTopLevel method now works with single-valued strings (Anatolii Siuniaev
   via Jason Gerlowski)
 
+* SOLR-13181: macro expansion of parameters could result in a StringIndexOutOfBoundsException.
+  Also, params with macros will be processed more strictly by LTR FeatureWeight.
+  (David Smiley, Cesar Rodriguez, Christine Poerschke)
+
 Other Changes
 ---------------------
 

solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java

@@ -119,8 +119,8 @@ public class MacroExpander {
     int start = 0;  // start of the unprocessed part of the string
     StringBuilder sb = null;
     for (;;) {
+      assert idx >= start;
       idx = val.indexOf(macroStart, idx);
-      int matchedStart = idx;
 
       // check if escaped
       if (idx > 0) {
@@ -135,18 +135,19 @@ public class MacroExpander {
         }
       }
       else if (idx < 0) {
-        if (sb == null) return val;
-        sb.append(val.substring(start));
-        return sb.toString();
+        break;
       }
 
       // found unescaped "${"
-      idx += macroStart.length();
+      final int matchedStart = idx;
 
-      int rbrace = val.indexOf('}', idx);
+      int rbrace = val.indexOf('}', matchedStart + macroStart.length());
       if (rbrace == -1) {
         // no matching close brace...
-        continue;
+        if (failOnMissingParams) {
+          return null;
+        }
+        break;
       }
 
       if (sb == null) {
@@ -154,14 +155,14 @@ public class MacroExpander {
       }
 
       if (matchedStart > 0) {
-        sb.append(val.substring(start, matchedStart));
+        sb.append(val, start, matchedStart);
       }
 
       // update "start" to be at the end of ${...}
-      start = rbrace + 1;
+      idx = start = rbrace + 1;
 
-      // String inbetween = val.substring(idx, rbrace);
-      StrParser parser = new StrParser(val, idx, rbrace);
+      // String in-between braces
+      StrParser parser = new StrParser(val, matchedStart + macroStart.length(), rbrace);
       try {
         String paramName = parser.getId();
         String defVal = null;
@@ -187,13 +188,19 @@ public class MacroExpander {
         }
 
       } catch (SyntaxError syntaxError) {
+        if (failOnMissingParams) {
+          return null;
+        }
         // append the part we would have skipped
-        sb.append( val.substring(matchedStart, start) );
-        continue;
+        sb.append(val, matchedStart, start);
       }
+    } // loop idx
 
+    if (sb == null) {
+      return val;
     }
-
+    sb.append(val, start, val.length());
+    return sb.toString();
   }
 
 

solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java

@@ -16,14 +16,14 @@
  */
 package org.apache.solr.request.macro;
 
-import java.util.Map;
+import java.util.Collections;
 import java.util.HashMap;
-
+import java.util.Map;
 
 import org.apache.solr.SolrTestCase;
 import org.junit.Test;
 
-/*
+/**
  * Tests for the MacroExpander
  */
 public class TestMacroExpander extends SolrTestCase {
@@ -121,14 +121,13 @@ public class TestMacroExpander extends SolrTestCase {
     request.put("expr", new String[] {"${one_ref}"}); // expr is for streaming expressions, no replacement by default
     request.put("one_ref",new String[] {"one"});
     request.put("three_ref",new String[] {"three"});
-    @SuppressWarnings({"rawtypes"})
-    Map expanded = MacroExpander.expand(request);
-    assertEquals("zero", ((String[])expanded.get("fq"))[0]);
-    assertEquals("one", ((String[])expanded.get("fq"))[1]);
-    assertEquals("two", ((String[]) expanded.get("fq"))[2]);
-    assertEquals("three", ((String[]) expanded.get("fq"))[3]);
+    Map<String, String[]> expanded = MacroExpander.expand(request);
+    assertEquals("zero", expanded.get("fq")[0]);
+    assertEquals("one", expanded.get("fq")[1]);
+    assertEquals("two", expanded.get("fq")[2]);
+    assertEquals("three", expanded.get("fq")[3]);
 
-    assertEquals("${one_ref}", ((String[])expanded.get("expr"))[0]);
+    assertEquals("${one_ref}", expanded.get("expr")[0]);
   }
 
   @Test
@@ -143,15 +142,36 @@ public class TestMacroExpander extends SolrTestCase {
     String oldVal = System.getProperty("StreamingExpressionMacros","false");
     System.setProperty("StreamingExpressionMacros", "true");
     try {
-      @SuppressWarnings({"rawtypes"})
-      Map expanded = MacroExpander.expand(request);
-      assertEquals("zero", ((String[])expanded.get("fq"))[0]);
-      assertEquals("one", ((String[])expanded.get("fq"))[1]);
-      assertEquals("two", ((String[]) expanded.get("fq"))[2]);
-      assertEquals("three", ((String[]) expanded.get("fq"))[3]);
-      assertEquals("one", ((String[])expanded.get("expr"))[0]);
+      Map<String, String[]> expanded = MacroExpander.expand(request);
+      assertEquals("zero", expanded.get("fq")[0]);
+      assertEquals("one", expanded.get("fq")[1]);
+      assertEquals("two", expanded.get("fq")[2]);
+      assertEquals("three", expanded.get("fq")[3]);
+      assertEquals("one", expanded.get("expr")[0]);
     } finally {
       System.setProperty("StreamingExpressionMacros", oldVal);
     }
   }
+
+  @Test
+  public void testUnbalanced() { // SOLR-13181
+    final Map<String, String[]> request = Collections.singletonMap("answer", new String[]{ "42" });
+    final MacroExpander meSkipOnMissingParams = new MacroExpander(request);
+    final MacroExpander meFailOnMissingParams = new MacroExpander(request, true);
+    assertEquals("${noClose", meSkipOnMissingParams.expand("${noClose"));
+    assertEquals("42 ${noClose", meSkipOnMissingParams.expand("${answer} ${noClose"));
+    assertEquals("42 ${noClose fooBar", meSkipOnMissingParams.expand("${answer} ${noClose fooBar"));
+    assertNull(meFailOnMissingParams.expand("${noClose"));
+    assertNull(meFailOnMissingParams.expand("${answer} ${noClose"));
+    assertNull(meFailOnMissingParams.expand("${answer} ${noClose fooBar"));
+
+    assertEquals("${${b}}", meSkipOnMissingParams.expand("${${b}}"));
+    assertNull(meFailOnMissingParams.expand("${${b}}"));
+
+    // Does not register as a syntax failure, although may subjectively look like it.
+    //   Consequently, the expression is replaced with nothing in default mode.
+    //   It'd be nice if there was a mode to leave un-resolved macros as-is when they don't resolve.
+    assertEquals("preamble ", meSkipOnMissingParams.expand("preamble ${exp${bad}"));
+    assertNull(meFailOnMissingParams.expand("preamble ${exp${bad}"));
+  }
 }