diff --git a/docs/root/configuration/http/http_filters/csrf_filter.rst b/docs/root/configuration/http/http_filters/csrf_filter.rst index a5e9e87e508ef..b36afbf72f11e 100644 --- a/docs/root/configuration/http/http_filters/csrf_filter.rst +++ b/docs/root/configuration/http/http_filters/csrf_filter.rst @@ -28,9 +28,8 @@ a request originated from the same host: When the filter is evaluating a request, it ensures both pieces of information are present and compares their values. If the source origin is missing or the origins do not match the request is rejected. The exception to this being if the source origin has been -added to the policy as valid. Because CSRF attacks specifically target state-changing -requests, the filter only acts on HTTP requests that have a state-changing method -(POST, PUT, etc.). +added to the policy as valid. The filter acts on both HTTP requests that have a state-changing method and +non-state-changing method for safety. .. note:: Due to differing functionality between browsers this filter will determine diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index 742695feb1088..6c99f3d9750b0 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -26,16 +26,6 @@ using RcDetails = ConstSingleton; namespace { constexpr absl::string_view NullOrigin{"null"}; -bool isModifyMethod(const Http::RequestHeaderMap& headers) { - const absl::string_view method_type = headers.getMethodValue(); - if (method_type.empty()) { - return false; - } - const auto& method_values = Http::Headers::get().MethodValues; - return (method_type == method_values.Post || method_type == method_values.Put || - method_type == method_values.Delete || method_type == method_values.Patch); -} - std::string hostAndPort(const absl::string_view absolute_url) { Http::Utility::Url url; if (!absolute_url.empty()) { @@ -102,10 +92,6 @@ Http::FilterHeadersStatus CsrfFilter::decodeHeaders(Http::RequestHeaderMap& head return Http::FilterHeadersStatus::Continue; } - if (!isModifyMethod(headers)) { - return Http::FilterHeadersStatus::Continue; - } - const auto source_origin = sourceOriginValue(headers); if (source_origin.empty()) { config_->stats().missing_source_origin_.inc(); diff --git a/test/extensions/filters/http/csrf/csrf_filter_integration_test.cc b/test/extensions/filters/http/csrf/csrf_filter_integration_test.cc index c13fbbd762a19..7f50d7df9c08f 100644 --- a/test/extensions/filters/http/csrf/csrf_filter_integration_test.cc +++ b/test/extensions/filters/http/csrf/csrf_filter_integration_test.cc @@ -108,24 +108,24 @@ TEST_P(CsrfFilterIntegrationTest, TestCsrfDisabled) { EXPECT_EQ(response->headers().getStatusValue(), "200"); } -TEST_P(CsrfFilterIntegrationTest, TestNonMutationMethod) { +TEST_P(CsrfFilterIntegrationTest, TestOriginMismatch) { config_helper_.prependFilter(CSRF_FILTER_ENABLED_CONFIG); Http::TestRequestHeaderMapImpl headers = {{ - {":method", "GET"}, + {":method", "PUT"}, {":path", "/"}, {":scheme", "http"}, {"origin", "http://cross-origin"}, {"host", "test-origin"}, }}; - const auto& response = sendRequestAndWaitForResponse(headers); + const auto& response = sendRequest(headers); EXPECT_TRUE(response->complete()); - EXPECT_EQ(response->headers().getStatusValue(), "200"); + EXPECT_EQ(response->headers().getStatusValue(), "403"); } -TEST_P(CsrfFilterIntegrationTest, TestOriginMismatch) { +TEST_P(CsrfFilterIntegrationTest, TestEnforcesGet) { config_helper_.prependFilter(CSRF_FILTER_ENABLED_CONFIG); Http::TestRequestHeaderMapImpl headers = {{ - {":method", "PUT"}, + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {"origin", "http://cross-origin"}, diff --git a/test/extensions/filters/http/csrf/csrf_filter_test.cc b/test/extensions/filters/http/csrf/csrf_filter_test.cc index 51d44717fa340..3fe653e20c43f 100644 --- a/test/extensions/filters/http/csrf/csrf_filter_test.cc +++ b/test/extensions/filters/http/csrf/csrf_filter_test.cc @@ -97,21 +97,6 @@ class CsrfFilterTest : public testing::Test { Http::TestRequestTrailerMapImpl request_trailers_; }; -TEST_F(CsrfFilterTest, RequestWithNonMutableMethod) { - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "GET"}, {"host", "localhost"}, {":scheme", "http"}}; - - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data_, false)); - Http::MetadataMap metadata_map{{"metadata", "metadata"}}; - EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_.decodeMetadata(metadata_map)); - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers_)); - - EXPECT_EQ(0U, config_->stats().missing_source_origin_.value()); - EXPECT_EQ(0U, config_->stats().request_invalid_.value()); - EXPECT_EQ(0U, config_->stats().request_valid_.value()); -} - TEST_F(CsrfFilterTest, RequestWithoutOriginAndWithoutDestination) { Http::TestRequestHeaderMapImpl request_headers{{":method", "PUT"}, {":scheme", "http"}};