Fix percent encoding in query component

include/network/uri/detail/encode.hpp

@@ -29,24 +29,34 @@ inline CharT hex_to_letter(CharT in) {
   return in;
 }
 
+template <class charT, class OutputIterator>
+void percent_encode(charT in, OutputIterator &out) {
+  out++ = '%';
+  out++ = hex_to_letter((in >> 4) & 0x0f);
+  out++ = hex_to_letter(in & 0x0f);
+}
+
+template <class charT>
+bool is_unreserved(charT in) {
+  return ((in >= 'a') && (in <= 'z')) ||
+         ((in >= 'A') && (in <= 'Z')) ||
+         ((in >= '0') && (in <= '9')) ||
+         (in == '-') ||
+         (in == '.') ||
+         (in == '_') ||
+         (in == '~');
+}
+
 template <class charT, class OutputIterator>
 void encode_char(charT in, OutputIterator &out, const char *ignore = "") {
-  if (((in >= 'a') && (in <= 'z')) ||
-      ((in >= 'A') && (in <= 'Z')) ||
-      ((in >= '0') && (in <= '9')) ||
-      (in == '-') ||
-      (in == '.') ||
-      (in == '_') ||
-      (in == '~')) {
+  if (is_unreserved(in)) {
     out++ = in;
   } else {
     auto first = ignore, last = ignore + std::strlen(ignore);
     if (std::find(first, last, in) != last) {
       out++ = in;
     } else {
-      out++ = '%';
-      out++ = hex_to_letter((in >> 4) & 0x0f);
-      out++ = hex_to_letter(in & 0x0f);
+      percent_encode(in, out);
     }
   }
 }
@@ -106,6 +116,17 @@ OutputIterator encode_query(InputIterator first, InputIterator last,
   return out;
 }
 
+template <typename InputIterator, typename OutputIterator>
+OutputIterator encode_query_component(InputIterator first, InputIterator last,
+                                      OutputIterator out) {
+  auto it = first;
+  while (it != last) {
+    detail::encode_char(*it, out, "/?");
+    ++it;
+  }
+  return out;
+}
+
 template <typename InputIterator, typename OutputIterator>
 OutputIterator encode_fragment(InputIterator first, InputIterator last,
                                OutputIterator out) {

include/network/uri/uri_builder.hpp

@@ -199,22 +199,15 @@ class uri_builder {
   uri_builder &clear_query();
 
   /**
-   * \brief Adds a new query to the uri_builder.
+   * \brief Adds a new query key/value pair to the uri_builder.
    * \param key The query key.
    * \param value The query value.
    * \returns \c *this
    */
   template <typename Key, typename Value>
-    uri_builder &append_query_key_value_pair(const Key &key, const Value &value) {
-    if (!query_) {
-      query_ = string_type();
-    }
-    else {
-      query_->append("&");
-    }
-    string_type query_pair = detail::translate(key) + "=" + detail::translate(value);
-    network::uri::encode_query(std::begin(query_pair), std::end(query_pair),
-                               std::back_inserter(*query_));
+  uri_builder &append_query_key_value_pair(const Key &key, const Value &value) {
+    append_query_key_value_pair(detail::translate(key),
+                                detail::translate(value));
     return *this;
   }
 
@@ -253,6 +246,7 @@ class uri_builder {
   void set_authority(string_type authority);
   void set_path(string_type path);
   void append_query(string_type query);
+  void append_query_key_value_pair(string_type key, string_type value);
   void set_fragment(string_type fragment);
 
   optional<string_type> scheme_, user_info_, host_, port_, path_, query_,

src/uri_builder.cpp

@@ -122,6 +122,19 @@ void uri_builder::append_query(string_type query) {
                              std::back_inserter(*query_));
 }
 
+void uri_builder::append_query_key_value_pair(string_type key, string_type value) {
+  if (!query_) {
+    query_ = string_type();
+  } else {
+    query_->push_back('&');
+  }
+  detail::encode_query_component(std::begin(key), std::end(key),
+                                 std::back_inserter(*query_));
+  query_->push_back('=');
+  detail::encode_query_component(std::begin(value), std::end(value),
+                                 std::back_inserter(*query_));
+}
+
 uri_builder &uri_builder::clear_query() {
   query_ = network::nullopt;
   return *this;

test/uri_builder_test.cpp

@@ -794,7 +794,45 @@ TEST(builder_test, construct_from_uri_bug_116) {
   const network::uri b("http://b.com");
   a = b;
 
-  network::uri_builder ub(a);  // ASAN reports heap-use-after-free here
+  network::uri_builder ub(a);
   const network::uri c(ub.uri());
   ASSERT_FALSE(c.has_port()) << c.string();
 }
+
+TEST(builder_test, append_query_key_value_pair_encodes_equals_sign) {
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "="));
+  ASSERT_EQ(network::string_view("%3D"), ub.uri().query_begin()->second);
+}
+
+TEST(builder_test, append_query_key_value_pair_encodes_number_sign) {
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "#"));
+  ASSERT_EQ(network::string_view("%23"), ub.uri().query_begin()->second);
+}
+
+TEST(builder_test, append_query_key_value_pair_encodes_percent_sign) {
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "%"));
+  ASSERT_EQ(network::string_view("%25"), ub.uri().query_begin()->second);
+}
+
+TEST(builder_test, append_query_key_value_pair_encodes_ampersand) {
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "&"));
+  ASSERT_EQ(network::string_view("%26"), ub.uri().query_begin()->second);
+}
+
+TEST(builder_test, append_query_key_value_pair_does_not_encode_slash) {
+  // https://tools.ietf.org/html/rfc3986#section-3.4
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "/"));
+  ASSERT_EQ(network::string_view("/"), ub.uri().query_begin()->second);
+}
+
+TEST(builder_test, append_query_key_value_pair_does_not_encode_qmark) {
+  // https://tools.ietf.org/html/rfc3986#section-3.4
+  network::uri_builder ub(network::uri("http://example.com"));
+  ASSERT_NO_THROW(ub.append_query_key_value_pair("q", "?"));
+  ASSERT_EQ(network::string_view("?"), ub.uri().query_begin()->second);
+}

test/uri_encoding_test.cpp

@@ -10,15 +10,15 @@
 
 
 TEST(uri_encoding_test, encode_user_info_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_user_info(std::begin(unencoded), std::end(unencoded),
 				 std::back_inserter(instance));
   ASSERT_EQ("%21%23%24%26%27%28%29%2A%2B%2C%2F:%3B%3D%3F%40%5B%5D", instance);
 }
 
 TEST(uri_encoding_test, encode_host_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_host(std::begin(unencoded), std::end(unencoded),
 			    std::back_inserter(instance));
@@ -34,31 +34,31 @@ TEST(uri_encoding_test, encode_ipv6_host) {
 }
 
 TEST(uri_encoding_test, encode_port_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_port(std::begin(unencoded), std::end(unencoded),
 			    std::back_inserter(instance));
   ASSERT_EQ("%21%23%24%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D", instance);
 }
 
 TEST(uri_encoding_test, encode_path_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_path(std::begin(unencoded), std::end(unencoded),
 			    std::back_inserter(instance));
   ASSERT_EQ("%21%23%24%26%27%28%29%2A%2B%2C/%3A;=%3F@%5B%5D", instance);
 }
 
 TEST(uri_encoding_test, encode_query_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_query(std::begin(unencoded), std::end(unencoded),
 			     std::back_inserter(instance));
   ASSERT_EQ("%21%23%24&%27%28%29%2A%2B%2C/%3A;=%3F@%5B%5D", instance);
 }
 
 TEST(uri_encoding_test, encode_fragment_iterator) {
-  const std::string unencoded("!#$&\'()*+,/:;=?@[]");
+  const std::string unencoded("!#$&'()*+,/:;=?@[]");
   std::string instance;
   network::uri::encode_fragment(std::begin(unencoded), std::end(unencoded),
 				std::back_inserter(instance));
@@ -70,7 +70,7 @@ TEST(uri_encoding_test, decode_iterator) {
   std::string instance;
   network::uri::decode(std::begin(encoded), std::end(encoded),
 		       std::back_inserter(instance));
-  ASSERT_EQ("!#$&\'()*+,/:;=?@[]", instance);
+  ASSERT_EQ("!#$&'()*+,/:;=?@[]", instance);
 }
 
 TEST(uri_encoding_test, decode_iterator_error_1) {