Fix "api_key" possibly getting stripped from inside URLs.

We want to strip the "api_key" query parameter, but our regex wasn't
quite correct, so it was possible "api_key" was getting stripped from
the URL in unexpected situations, like when the string "api_key" just
happened to be somewhere inside the query string (for example, it would
end up stripping "?foo=api_key" or ?api_key_foo=bar").

This fixes the issue by fixing the regex for stripping query parameters
to more properly detect individual query string boundaries (so only
"api_key=foo" should be stripped, but other instances of "api_key"
should be left alone).

Author: GUI

CHANGELOG.md

@@ -1,5 +1,11 @@
 # API Umbrella Change Log
 
+## Unreleased
+
+### Fixed
+
+- **Fix URL handling for query strings containing "api\_key":** It was possible that API Umbrella was stripping the string "api\_key" from inside URLs before passing requests to the API backend in some unexpected cases. The `api_key` query parameter should still be stripped, but other instances of "api\_key" elsewhere in the URL (for example as a value, like `?foo=api_key`), are now retained.
+
 ## 0.14.4 (2017-07-15)
 
 This update contains one important fix for v0.14.3. Upgrading is recommended if you are currently running v0.14.3.

src/api-umbrella/proxy/utils.lua

@@ -248,7 +248,7 @@ function _M.remove_arg(original_args, remove)
     -- matter, but in general it just seems safer to default to doing less
     -- changes to the query string).
     local _, gsub_err
-    args, _, gsub_err = gsub(args, "\\b" .. remove .. "=?[^&]*(&|$)", "")
+    args, _, gsub_err = gsub(args, "(?<=^|&)" .. remove .. "(?:=[^&]*)?(?:&|$)", "", "jo")
     if gsub_err then
       ngx.log(ngx.ERR, "regex error: ", gsub_err)
     end

test/proxy/request_rewriting/test_strips_api_keys.rb

@@ -43,6 +43,7 @@ def test_strips_api_key_from_middle_of_query
     assert_response_code(200, response)
     data = MultiJson.load(response.body)
     assert_equal({ "test" => "value", "foo" => "bar" }, data["url"]["query"])
+    assert_equal("http://127.0.0.1/info/?test=value&foo=bar", data["raw_url"])
   end
 
   def test_strips_repeated_api_key_in_query
@@ -100,6 +101,41 @@ def test_retains_basic_auth_if_api_key_passed_by_other_means
     assert(data["headers"]["authorization"])
   end
 
+  def test_retains_api_key_string_in_values
+    response = Typhoeus.get("http://127.0.0.1:9080/api/info/?search=api_key", http_options)
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal({ "search" => "api_key" }, data["url"]["query"])
+  end
+
+  def test_retains_api_key_string_in_values_with_prefix
+    response = Typhoeus.get("http://127.0.0.1:9080/api/info/?search=foo_api_key", http_options)
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal({ "search" => "foo_api_key" }, data["url"]["query"])
+  end
+
+  def test_retains_api_key_string_in_values_with_suffix
+    response = Typhoeus.get("http://127.0.0.1:9080/api/info/?search=api_key_foo", http_options)
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal({ "search" => "api_key_foo" }, data["url"]["query"])
+  end
+
+  def test_retains_api_key_string_in_param_with_prefix
+    response = Typhoeus.get("http://127.0.0.1:9080/api/info/?foo_api_key=bar", http_options)
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal({ "foo_api_key" => "bar" }, data["url"]["query"])
+  end
+
+  def test_retains_api_key_string_in_param_with_suffix
+    response = Typhoeus.get("http://127.0.0.1:9080/api/info/?api_key_foo=bar", http_options)
+    assert_response_code(200, response)
+    data = MultiJson.load(response.body)
+    assert_equal({ "api_key_foo" => "bar" }, data["url"]["query"])
+  end
+
   def test_preserves_query_string_order
     response = Typhoeus.get("http://127.0.0.1:9080/api/info/?ccc=foo&aaa=bar&api_key=#{self.api_key}&b=test", http_options)
     assert_response_code(200, response)