From c837d6e1aba7a7d6d9783139943ecef7cf7e01e2 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 17 Dec 2020 16:18:15 -0500 Subject: [PATCH 1/2] oauth: properly stop filter chain when a response was sent Related to #14416. Signed-off-by: Raul Gutierrez Segales --- source/extensions/filters/http/oauth2/filter.cc | 14 +++++++------- test/extensions/filters/http/oauth2/filter_test.cc | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index d1b3654457be..aa1396ec9302 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -212,12 +212,12 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he Http::Utility::Url state_url; if (!state_url.initialize(state, false)) { sendUnauthorizedResponse(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } // Avoid infinite redirect storm if (config_->redirectPathMatcher().match(state_url.pathAndQueryParams())) { sendUnauthorizedResponse(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } Http::ResponseHeaderMapPtr response_headers{ Http::createHeaderMap( @@ -283,7 +283,7 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he config_->stats().oauth_unauthorized_rq_.inc(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } // At this point, we *are* on /_oauth. We believe this request comes from the authorization @@ -292,14 +292,14 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he const auto query_parameters = Http::Utility::parseQueryString(path_str); if (query_parameters.find(queryParamsError()) != query_parameters.end()) { sendUnauthorizedResponse(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } // if the data we need is not present on the URL, stop execution if (query_parameters.find(queryParamsCode()) == query_parameters.end() || query_parameters.find(queryParamsState()) == query_parameters.end()) { sendUnauthorizedResponse(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } auth_code_ = query_parameters.at(queryParamsCode()); @@ -308,7 +308,7 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he Http::Utility::Url state_url; if (!state_url.initialize(state_, false)) { sendUnauthorizedResponse(); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } Formatter::FormatterImpl formatter(config_->redirectUri()); @@ -359,7 +359,7 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap response_headers->setLocation(new_path); decoder_callbacks_->encodeHeaders(std::move(response_headers), true, SIGN_OUT); - return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + return Http::FilterHeadersStatus::StopIteration; } void OAuth2Filter::onGetAccessTokenSuccess(const std::string& access_code, diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index fae1d1676efb..6117684f0f80 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -174,7 +174,7 @@ TEST_F(OAuth2Test, RequestSignout) { }; EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); } @@ -255,7 +255,7 @@ TEST_F(OAuth2Test, OAuthErrorNonOAuthHttpCallback) { EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); } @@ -281,7 +281,7 @@ TEST_F(OAuth2Test, OAuthErrorQueryString) { EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); EXPECT_CALL(decoder_callbacks_, encodeData(_, true)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); EXPECT_EQ(scope_.counterFromString("test.oauth_failure").value(), 1); @@ -309,7 +309,7 @@ TEST_F(OAuth2Test, OAuthCallbackStartsAuthentication) { EXPECT_CALL(*oauth_client_, asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", "https://traffic.example.com" + TEST_CALLBACK)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); } @@ -421,7 +421,7 @@ TEST_F(OAuth2Test, OAuthTestInvalidUrlInStateQueryParam) { EXPECT_CALL(*validator_, token()).WillRepeatedly(ReturnRef(legit_token)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), false)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); } @@ -455,7 +455,7 @@ TEST_F(OAuth2Test, OAuthTestCallbackUrlInStateQueryParam) { EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_response_headers), false)); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, false)); Http::TestRequestHeaderMapImpl final_request_headers{ @@ -584,7 +584,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { "https://traffic.example.com" + TEST_CALLBACK)); // Invoke the callback logic. As a side effect, state_ will be populated. - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(second_request_headers, false)); EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value()); From 3a76aa6849715cabb5d14c9cb6d6398c77491437 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 17 Dec 2020 17:06:39 -0500 Subject: [PATCH 2/2] Fix test Signed-off-by: Raul Gutierrez Segales --- test/extensions/filters/http/oauth2/filter_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 6117684f0f80..bac0b139ac4c 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -309,7 +309,7 @@ TEST_F(OAuth2Test, OAuthCallbackStartsAuthentication) { EXPECT_CALL(*oauth_client_, asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", "https://traffic.example.com" + TEST_CALLBACK)); - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, filter_->decodeHeaders(request_headers, false)); } @@ -564,7 +564,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); // This represents the beginning of the OAuth filter. - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(first_request_headers, false)); // This represents the callback request from the authorization server. @@ -584,7 +584,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { "https://traffic.example.com" + TEST_CALLBACK)); // Invoke the callback logic. As a side effect, state_ will be populated. - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, filter_->decodeHeaders(second_request_headers, false)); EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value());