Fix url encoding handling for code parameter. (#206)

* fix url encoding handling for code parameter.

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>

* todo for the oidc lib.

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>

Makefile

@@ -21,6 +21,9 @@ compose:
 docker: build
 	rm -rf build_release && mkdir -p build_release && cp -r bazel-bin/ build_release && docker build . -f build/Dockerfile.runner -t $(IMAGE)
 
+docker.push: docker
+  docker push $(IMAGE)
+
 docker-from-scratch:
 	docker build -f build/Dockerfile.builder -t authservice:$(USER) .
 

src/common/http/http.cc

@@ -54,6 +54,25 @@ bool IsFormDataSafeCharacter(const char character) {
   return IsUrlSafeCharacter(character) || (character == '+');
 }
 
+// We found that at least accounts.google.com[1] could contain `/` in the
+// authorization code, as part of OAuth callback URl query param.
+// such as `..../?code=4/Abc38.
+// RFC 6749[2] specifies "code" could be `VSCHAR`, containing all printable
+// ascii chars. RFC 3986[3] about URI specifies that:
+// If a reserved character is found in a URI component and
+//  no delimiting role is known for that character, then it must be
+//  interpreted as representing the data octet corresponding to that
+//  character's encoding in US-ASCII.
+// [1]
+// https://developers.google.com/identity/protocols/oauth2/openid-connect#sendauthrequest
+// [2] https://datatracker.ietf.org/doc/html/rfc6749#appendix-A.11.
+// [3] https://www.rfc-editor.org/rfc/rfc3986#page-12
+// TODO(incfly): move to a separate library specific for OIDC instead of putting
+// in http lib.
+bool IsOIDCCodeSafeCharacter(const char character) {
+  return IsUrlSafeCharacter(character) || (character == '/');
+}
+
 std::string SafeEncode(absl::string_view in, SafeCharacterFunc IsSafe) {
   std::stringstream builder;
   for (auto character : in) {
@@ -143,18 +162,28 @@ absl::optional<std::multimap<std::string, std::string>> Http::DecodeQueryData(
   std::multimap<std::string, std::string> result;
   std::vector<std::string> parts;
   boost::split(parts, query, boost::is_any_of("&"));
+  spdlog::trace("{} decode query: {}", __func__, query);
   for (auto part : parts) {
     std::vector<std::string> pair;
     boost::split(pair, part, boost::is_any_of("="));
     if (pair.size() != 2) {
       return absl::nullopt;
     }
-    auto escaped_key = SafeDecode(pair[0], IsUrlSafeCharacter);
+    SafeCharacterFunc checker = IsUrlSafeCharacter;
+    auto escaped_key = SafeDecode(pair[0], checker);
     if (!escaped_key.has_value()) {
+      spdlog::error("{} decode query fail at the query pair {}, key part.",
+                    __func__, part);
       return absl::nullopt;
     }
-    auto escaped_value = SafeDecode(pair[1], IsUrlSafeCharacter);
+    // If the key is the "code", then we use a different checker function.
+    if (pair[0] == "code") {
+      checker = IsOIDCCodeSafeCharacter;
+    }
+    auto escaped_value = SafeDecode(pair[1], checker);
     if (!escaped_value.has_value()) {
+      spdlog::error("{} decode query fail at the query pair {}, value part.",
+                    __func__, part);
       return absl::nullopt;
     }
     result.insert(std::make_pair(*escaped_key, *escaped_value));

test/common/http/http_test.cc

@@ -25,6 +25,13 @@ struct {
 } query_test_case = {.raw = R"RAW(cde=456%207&state=abc%20123)RAW",
                      .encoded = {{"cde", "456 7"}, {"state", "abc 123"}}};
 
+struct {
+  const char *raw;
+  const std::multimap<absl::string_view, absl::string_view> encoded;
+} query_oidc_code_case = {
+    .raw = R"RAW(code=4/fod_AB8&state=abc%20123)RAW",
+    .encoded = {{"code", "4/fod_AB8"}, {"state", "abc 123"}}};
+
 struct {
   const char *raw;
   const std::multimap<absl::string_view, absl::string_view> encoded;
@@ -66,6 +73,24 @@ TEST(Http, EncodeQueryData) {
   }
 }
 
+// Checks that the OIDC callback "?code=x/xxx", code parameter can contains
+// valid characters, such as "/".
+TEST(Http, DecodeQueryDataOIDCCode) {
+  auto decoded =
+      Http::DecodeQueryData(R"RAW(code=4/fod_AB8&state=abc%2F123)RAW");
+  // param "code" is not encoding "/", but the param "state" is handled as usual
+  // URL encoding.
+  std::multimap<absl::string_view, absl::string_view> encoded = {
+      {"code", "4/fod_AB8"}, {"state", "abc/123"}};
+  ASSERT_TRUE(decoded.has_value());
+  ASSERT_EQ(encoded.size(), decoded->size());
+  for (auto val : encoded) {
+    auto iter = decoded->find(val.first.data());
+    ASSERT_TRUE(iter != decoded->end());
+    ASSERT_EQ(iter->second, val.second);
+  }
+}
+
 TEST(Http, DecodeFormData) {
   auto result = Http::DecodeFormData(form_test_case.raw);
   EXPECT_TRUE(result.has_value());