diff --git a/Makefile b/Makefile index 9ac6e388..bd6294b7 100644 --- a/Makefile +++ b/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) . diff --git a/src/common/http/http.cc b/src/common/http/http.cc index 0f3a6a79..cbfd100c 100644 --- a/src/common/http/http.cc +++ b/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> Http::DecodeQueryData( std::multimap result; std::vector parts; boost::split(parts, query, boost::is_any_of("&")); + spdlog::trace("{} decode query: {}", __func__, query); for (auto part : parts) { std::vector 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)); diff --git a/test/common/http/http_test.cc b/test/common/http/http_test.cc index 7ad4ab95..20a8fc33 100644 --- a/test/common/http/http_test.cc +++ b/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 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 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 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());