SOLR-5971: Fix error 'Illegal character in query' when proxying request

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1715615 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -398,6 +398,9 @@ Bug Fixes
 * SOLR-8280: Fixed bug in SimilarityFactory initialization that prevented SolrCoreAware factories -- such
   as SchemaSimilarityFactory -- from functioning properly with managed schema features. (hossman)
 
+* SOLR-5971: Fix error 'Illegal character in query' when proxying request.
+  (Uwe Schindler, Ishan Chattopadhyaya, Eric Bus)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

@@ -517,11 +517,7 @@ public class HttpSolrCall {
     HttpEntity httpEntity = null;
     boolean success = false;
     try {
-      String urlstr = coreUrl;
-
-      String queryString = req.getQueryString();
-
-      urlstr += queryString == null ? "" : "?" + queryString;
+      String urlstr = coreUrl + queryParams.toQueryString();
 
       boolean isPostOrPutRequest = "POST".equals(req.getMethod()) || "PUT".equals(req.getMethod());
       if ("GET".equals(req.getMethod())) {
@@ -565,8 +561,8 @@ public class HttpSolrCall {
 
         // We pull out these two headers below because they can cause chunked
         // encoding issues with Tomcat
-        if (header != null && !header.getName().equals(TRANSFER_ENCODING_HEADER)
-            && !header.getName().equals(CONNECTION_HEADER)) {
+        if (header != null && !header.getName().equalsIgnoreCase(TRANSFER_ENCODING_HEADER)
+            && !header.getName().equalsIgnoreCase(CONNECTION_HEADER)) {
           resp.addHeader(header.getName(), header.getValue());
         }
       }

solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java

@@ -0,0 +1,92 @@
+package org.apache.solr.cloud;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.File;
+import java.net.URL;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.junit.Test;
+
+@SuppressSSL
+public class TestRequestForwarding extends SolrTestCaseJ4 {
+
+  private MiniSolrCloudCluster solrCluster;
+
+  @Override
+  public void setUp() throws Exception {
+    super.setUp();
+    System.setProperty("solr.test.sys.prop1", "propone");
+    System.setProperty("solr.test.sys.prop2", "proptwo");
+    solrCluster = new MiniSolrCloudCluster(3, createTempDir(), buildJettyConfig("/solr"));
+    File configDir = getFile("solr").toPath().resolve("collection1/conf").toFile();
+    solrCluster.uploadConfigDir(configDir, "conf1");
+  }
+
+  @Override
+  public void tearDown() throws Exception {
+    solrCluster.shutdown();
+    System.clearProperty("solr.test.sys.prop1");
+    System.clearProperty("solr.test.sys.prop2");
+
+    super.tearDown();
+  }
+
+  @Test
+  public void testMultiCollectionQuery() throws Exception {
+    createCollection("collection1", "conf1");
+    // Test against all nodes (two of them host the collection, one of them will 
+    // forward the query)
+    for (JettySolrRunner jettySolrRunner : solrCluster.getJettySolrRunners()) {
+      String queryStrings[] = {
+          "q=cat%3Afootball%5E2", // URL encoded 
+          "q=cat:football^2" // No URL encoding, contains disallowed character ^
+      };
+      for (String q: queryStrings) {
+        try {
+          URL url = new URL(jettySolrRunner.getBaseUrl().toString()+"/collection1/select?"+q);
+          url.openStream(); // Shouldn't throw any errors
+        } catch (Exception ex) {
+          throw new RuntimeException("Query '" + q + "' failed, ",ex);
+        }
+      }
+    }
+  }
+
+  private void createCollection(String name, String config) throws Exception {
+    CollectionAdminResponse response;
+    CollectionAdminRequest.Create create = new CollectionAdminRequest.Create();
+    create.setConfigName(config);
+    create.setCollectionName(name);
+    create.setNumShards(2);
+    create.setReplicationFactor(1);
+    create.setMaxShardsPerNode(1);
+    response = create.process(solrCluster.getSolrClient());
+    
+    if (response.getStatus() != 0 || response.getErrorMessages() != null) {
+      fail("Could not create collection. Response" + response.toString());
+    }
+    ZkStateReader zkStateReader = solrCluster.getSolrClient().getZkStateReader();
+    AbstractDistribZkTestBase.waitForRecoveriesToFinish(name, zkStateReader, false, true, 100);
+  }
+}

solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java

@@ -271,7 +271,7 @@ public class ConcurrentUpdateSolrClient extends SolrClient {
           requestParams.set(CommonParams.VERSION, client.parser.getVersion());
 
           method = new HttpPost(client.getBaseURL() + "/update"
-              + ClientUtils.toQueryString(requestParams, false));
+              + requestParams.toQueryString());
           method.setEntity(template);
           method.addHeader("User-Agent", HttpSolrClient.AGENT);
           method.addHeader("Content-Type", contentType);

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java

@@ -46,7 +46,6 @@ import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.request.RequestWriter;
-import org.apache.solr.client.solrj.util.ClientUtils;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
@@ -344,7 +343,7 @@ public class HttpSolrClient extends SolrClient {
       if (streams != null) {
         throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "GET can't send streams!");
       }
-      return new HttpGet(basePath + path + ClientUtils.toQueryString(wparams, false));
+      return new HttpGet(basePath + path + wparams.toQueryString());
     }
 
     if (SolrRequest.METHOD.POST == request.getMethod() || SolrRequest.METHOD.PUT == request.getMethod()) {
@@ -367,7 +366,7 @@ public class HttpSolrClient extends SolrClient {
         // send server list and request list as query string params
         ModifiableSolrParams queryParams = calculateQueryParams(this.queryParams, wparams);
         queryParams.add(calculateQueryParams(request.getQueryParams(), wparams));
-        String fullQueryUrl = url + ClientUtils.toQueryString(queryParams, false);
+        String fullQueryUrl = url + queryParams.toQueryString();
         HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
             new HttpPost(fullQueryUrl) : new HttpPut(fullQueryUrl);
         if (!isMultipart) {
@@ -424,9 +423,9 @@ public class HttpSolrClient extends SolrClient {
       }
       // It is has one stream, it is the post body, put the params in the URL
       else {
-        String pstr = ClientUtils.toQueryString(wparams, false);
+        String fullQueryUrl = url + wparams.toQueryString();
         HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ?
-            new HttpPost(url + pstr) : new HttpPut(url + pstr);
+            new HttpPost(fullQueryUrl) : new HttpPut(fullQueryUrl);
 
         // Single stream as body
         // Using a loop just to get the first one

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

@@ -21,7 +21,6 @@ import org.apache.solr.common.SolrDocument;
 import org.apache.solr.common.SolrInputDocument;
 import org.apache.solr.common.SolrInputField;
 import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.Base64;
 import org.apache.solr.common.util.ContentStream;
 import org.apache.solr.common.util.ContentStreamBase;
@@ -31,12 +30,10 @@ import org.apache.solr.common.util.XML;
 import java.io.IOException;
 import java.io.StringWriter;
 import java.io.Writer;
-import java.net.URLEncoder;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
-import java.util.Iterator;
 import java.util.Map;
 import java.util.Map.Entry;
 
@@ -209,37 +206,6 @@ public class ClientUtils
     return sb.toString();
   }
 
-  public static String toQueryString( SolrParams params, boolean xml ) {
-    StringBuilder sb = new StringBuilder(128);
-    try {
-      String amp = xml ? "&" : "&";
-      boolean first=true;
-      Iterator<String> names = params.getParameterNamesIterator();
-      while( names.hasNext() ) {
-        String key = names.next();
-        String[] valarr = params.getParams( key );
-        if( valarr == null ) {
-          sb.append( first?"?":amp );
-          sb.append(key);
-          first=false;
-        }
-        else {
-          for (String val : valarr) {
-            sb.append( first? "?":amp );
-            sb.append(key);
-            if( val != null ) {
-              sb.append('=');
-              sb.append( URLEncoder.encode( val, "UTF-8" ) );
-            }
-            first=false;
-          }
-        }
-      }
-    }
-    catch (IOException e) {throw new RuntimeException(e);}  // can't happen
-    return sb.toString();
-  }
-
   /** Constructs a slices map from a collection of slices and handles disambiguation if multiple collections are being queried simultaneously */
   public static void addSlices(Map<String,Slice> target, String collectionName, Collection<Slice> slices, boolean multiCollection) {
     for (Slice slice : slices) {

solr/solrj/src/java/org/apache/solr/common/params/MapSolrParams.java

@@ -17,12 +17,8 @@
 
 package org.apache.solr.common.params;
 
-import org.apache.solr.common.util.StrUtils;
-
-import java.util.Arrays;
 import java.util.Iterator;
 import java.util.Map;
-import java.io.IOException;
 
 /**
  *
@@ -61,28 +57,4 @@ public class MapSolrParams extends SolrParams {
 
   public Map<String,String> getMap() { return map; }
 
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder(128);
-    try {
-      boolean first=true;
-
-      for (Map.Entry<String,String> entry : map.entrySet()) {
-        String key = entry.getKey();
-        Object val = entry.getValue();
-        if (val instanceof String[]) {
-          String[] strings = (String[]) val;
-          val =  StrUtils.join(Arrays.asList(strings),',');
-        }
-        if (!first) sb.append('&');
-        first=false;
-        sb.append(key);
-        sb.append('=');
-        StrUtils.partialURLEncodeVal(sb, val==null ? "" : String.valueOf(val));
-      }
-    }
-    catch (IOException e) {throw new RuntimeException(e);}  // can't happen
-
-    return sb.toString();
-  }
 }

solr/solrj/src/java/org/apache/solr/common/params/ModifiableSolrParams.java

@@ -17,8 +17,6 @@
 
 package org.apache.solr.common.params;
 
-import java.io.IOException;
-import java.net.URLEncoder;
 import java.util.LinkedHashMap;
 import java.util.Iterator;
 import java.util.Map;
@@ -191,29 +189,4 @@ public class ModifiableSolrParams extends SolrParams
   public String[] getParams(String param) {
     return vals.get( param );
   }
-
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder(128);
-    try {
-      boolean first=true;
-
-      for (Map.Entry<String,String[]> entry : vals.entrySet()) {
-        String key = entry.getKey();
-        String[] valarr = entry.getValue();
-        for (String val : valarr) {
-          if (!first) sb.append('&');
-          first=false;
-          sb.append(key);
-          sb.append('=');
-          if( val != null ) {
-            sb.append( URLEncoder.encode( val, "UTF-8" ) );
-          }
-        }
-      }
-    }
-    catch (IOException e) {throw new RuntimeException(e);}  // can't happen
-
-    return sb.toString();
-  }
 }

solr/solrj/src/java/org/apache/solr/common/params/MultiMapSolrParams.java

@@ -17,12 +17,9 @@
 
 package org.apache.solr.common.params;
 
-import org.apache.solr.common.util.StrUtils;
-
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
-import java.io.IOException;
 
 /**
  *
@@ -80,30 +77,6 @@ public class MultiMapSolrParams extends SolrParams {
 
   public Map<String,String[]> getMap() { return map; }
 
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder(128);
-    try {
-      boolean first=true;
-
-      for (Map.Entry<String,String[]> entry : map.entrySet()) {
-        String key = entry.getKey();
-        String[] valarr = entry.getValue();
-
-        for (String val : valarr) {
-          if (!first) sb.append('&');
-          first=false;
-          sb.append(key);
-          sb.append('=');
-          StrUtils.partialURLEncodeVal(sb, val==null ? "" : val);
-        }
-      }
-    }
-    catch (IOException e) {throw new RuntimeException(e);}  // can't happen
-
-    return sb.toString();
-  }
-
   /** Returns a MultiMap view of the SolrParams as efficiently as possible.  The returned map may or may not be a backing implementation. */
   public static Map<String,String[]> asMultiMap(SolrParams params) {
     return asMultiMap(params, false);
@@ -134,5 +107,4 @@ public class MultiMapSolrParams extends SolrParams {
     }
   }
 
-
 }

solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java

@@ -22,7 +22,11 @@ import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.apache.solr.common.util.StrUtils;
 
+import java.io.IOException;
 import java.io.Serializable;
+import java.io.UnsupportedEncodingException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -385,4 +389,54 @@ public abstract class SolrParams implements Serializable {
     }
     return sink;
   }
+  
+  /** Returns this SolrParams as a properly URL encoded string, starting with {@code "?"}, if not empty. */
+  public String toQueryString() {
+    try {
+      final String charset = StandardCharsets.UTF_8.name();
+      final StringBuilder sb = new StringBuilder(128);
+      boolean first = true;
+      for (final Iterator<String> it = getParameterNamesIterator(); it.hasNext();) {
+        final String name = it.next(), nameEnc = URLEncoder.encode(name, charset);
+        for (String val : getParams(name)) {
+          sb.append(first ? '?' : '&').append(nameEnc).append('=').append(URLEncoder.encode(val, charset));
+          first = false;
+        }
+      }
+      return sb.toString();
+    } catch (UnsupportedEncodingException e) {
+      // impossible!
+      throw new AssertionError(e);
+    }
+  }
+  
+  /** Like {@link #toQueryString()}, but only replacing enough chars so that
+   * the URL may be unambiguously pasted back into a browser.
+   * This method can be used to properly log query parameters without
+   * making them unreadable.
+   * <p>
+   * Characters with a numeric value less than 32 are encoded.
+   * &amp;,=,%,+,space are encoded.
+   */
+  @Override
+  public String toString() {
+    final StringBuilder sb = new StringBuilder(128);
+    try {
+      boolean first=true;
+      for (final Iterator<String> it = getParameterNamesIterator(); it.hasNext();) {
+        final String name = it.next();
+        for (String val : getParams(name)) {
+          if (!first) sb.append('&');
+          first=false;
+          StrUtils.partialURLEncodeVal(sb, name);
+          sb.append('=');
+          StrUtils.partialURLEncodeVal(sb, val);
+        }
+      }
+      return sb.toString();
+    } catch (IOException e) {
+      // impossible!
+      throw new AssertionError(e);
+    }
+  }
 }