Do not invalidate cached resources upon error responses to unsafe methods (#7864)

Before this change, cached resources would be deleted when the server
replied to a request with an unsafe method (POST, PUT, etc.) with an
error response code. This patch changes that behavior so that it is only
deleted when the response has a successful response code.

This seeks to comply more narrowly with the wording of RFC 7234,
section-4.4:

   A cache MUST invalidate the effective request URI (Section 5.5 of
   [RFC7230]) when it receives a non-error response to a request with a
   method whose safety is unknown.

Note that we were technically in compliance with this before since we
always invalidated, regardless of the error response code. This,
however, was too broad in that we invalidated when we didn't need to.
Now we will only invalidate when the response code indicates a
successful response to the request with an unsafe method.

This also fixes our feature that caches responses to POST requests.

Author: bneradt

proxy/hdrs/HTTP.h

@@ -508,7 +508,7 @@ class HTTPHdr : public MIMEHdr
   void version_set(HTTPVersion version);
 
   const char *method_get(int *length);
-  int method_get_wksidx();
+  int method_get_wksidx() const;
   void method_set(const char *value, int length);
 
   URL *url_create(URL *url);
@@ -957,7 +957,7 @@ HTTPHdr::method_get(int *length)
 }
 
 inline int
-HTTPHdr::method_get_wksidx()
+HTTPHdr::method_get_wksidx() const
 {
   ink_assert(valid());
   ink_assert(m_http->m_polarity == HTTP_TYPE_REQUEST);

proxy/http/HttpTransact.cc

@@ -2085,6 +2085,14 @@ HttpTransact::OSDNSLookup(State *s)
       } else if (s->cache_lookup_result == CACHE_LOOKUP_MISS || s->cache_info.action == CACHE_DO_NO_ACTION) {
         TRANSACT_RETURN(SM_ACTION_API_OS_DNS, HandleCacheOpenReadMiss);
         // DNS lookup is done if the lookup failed and need to call Handle Cache Open Read Miss
+      } else if (s->cache_info.action == CACHE_PREPARE_TO_WRITE && s->http_config_param->cache_post_method == 1 &&
+                 s->method == HTTP_WKSIDX_POST) {
+        // By virtue of being here, we are intending to forward the request on
+        // to the server. If we marked this as CACHE_PREPARE_TO_WRITE and this
+        // is a POST request whose response we intend to write, then we have to
+        // proceed from here by calling the function that handles this as a
+        // miss.
+        TRANSACT_RETURN(SM_ACTION_API_OS_DNS, HandleCacheOpenReadMiss);
       } else {
         build_error_response(s, HTTP_STATUS_INTERNAL_SERVER_ERROR, "Invalid Cache Lookup result", "default");
         Log::error("HTTP: Invalid CACHE LOOKUP RESULT : %d", s->cache_lookup_result);
@@ -3093,7 +3101,8 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
   // fall through
   default:
     SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_HIT_SERVED);
-    if (s->method == HTTP_WKSIDX_GET || s->api_resp_cacheable == true) {
+    if (s->method == HTTP_WKSIDX_GET || (s->http_config_param->cache_post_method == 1 && s->method == HTTP_WKSIDX_POST) ||
+        s->api_resp_cacheable == true) {
       // send back the full document to the client.
       TxnDebug("http_trans", "[build_response_from_cache] Match! Serving full document.");
       s->cache_info.action = CACHE_DO_SERVE;
@@ -3323,7 +3332,7 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
   if (GET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP) == ' ') {
     SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_NOT_CACHED);
   }
-  // We do a cache lookup for DELETE and PUT requests as well.
+  // We do a cache lookup for some non-GET requests as well.
   // We must, however, not cache the responses to these requests.
   if (does_method_require_cache_copy_deletion(s->http_config_param, s->method) && s->api_req_cacheable == false) {
     s->cache_info.action = CACHE_DO_NO_ACTION;
@@ -4475,10 +4484,11 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     // cacheability of server response, and request method
     // precondition: s->cache_info.action is one of the following
     // CACHE_DO_UPDATE, CACHE_DO_WRITE, or CACHE_DO_DELETE
+    int const server_request_method = s->hdr_info.server_request.method_get_wksidx();
     if (s->api_server_response_no_store) {
       s->cache_info.action = CACHE_DO_NO_ACTION;
     } else if (s->api_server_response_ignore && server_response_code == HTTP_STATUS_OK &&
-               s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_HEAD) {
+               server_request_method == HTTP_WKSIDX_HEAD) {
       s->api_server_response_ignore = false;
       ink_assert(s->cache_info.object_read);
       base_response        = s->cache_info.object_read->response_get();
@@ -4495,7 +4505,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
       } else if (s->www_auth_content == CACHE_AUTH_STALE && server_response_code == HTTP_STATUS_UNAUTHORIZED) {
         s->cache_info.action = CACHE_DO_NO_ACTION;
       } else if (!cacheable) {
-        s->cache_info.action = CACHE_DO_DELETE;
+        if (HttpTransactHeaders::is_status_an_error_response(server_response_code) &&
+            !HttpTransactHeaders::is_method_safe(server_request_method)) {
+          // Only delete the cache entry if the response is successful. For
+          // unsuccessful responses, the transaction doesn't invalidate our
+          // entry. This behavior complies with RFC 7234, section 4.4 which
+          // stipulates that the entry only need be invalidated for non-error
+          // responses:
+          //
+          //    A cache MUST invalidate the effective request URI (Section 5.5 of
+          //    [RFC7230]) when it receives a non-error response to a request
+          //    with a method whose safety is unknown.
+          s->cache_info.action = CACHE_DO_NO_ACTION;
+        } else {
+          s->cache_info.action = CACHE_DO_DELETE;
+        }
       } else if (s->method == HTTP_WKSIDX_HEAD) {
         s->cache_info.action = CACHE_DO_DELETE;
       } else {
@@ -4513,7 +4537,19 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
       }
 
     } else if (s->cache_info.action == CACHE_DO_DELETE) {
-      // do nothing
+      if (!cacheable && HttpTransactHeaders::is_status_an_error_response(server_response_code) &&
+          !HttpTransactHeaders::is_method_safe(server_request_method)) {
+        // Only delete the cache entry if the response is successful. For
+        // unsuccessful responses, the transaction doesn't invalidate our
+        // entry. This behavior complies with RFC 7234, section 4.4 which
+        // stipulates that the entry only need be invalidated for non-error
+        // responses:
+        //
+        //    A cache MUST invalidate the effective request URI (Section 5.5 of
+        //    [RFC7230]) when it receives a non-error response to a request
+        //    with a method whose safety is unknown.
+        s->cache_info.action = CACHE_DO_NO_ACTION;
+      }
 
     } else {
       ink_assert(!("cache action inconsistent with current state"));
@@ -5920,6 +5956,18 @@ HttpTransact::is_cache_response_returnable(State *s)
     SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_METHOD);
     return false;
   }
+  // We may be caching responses to methods other than GET, such as POST. Make
+  // sure that our cached resource has a method that matches the incoming
+  // requests's method. If not, then we cannot reply with the cached resource.
+  // That is, we cannot reply to an incoming GET request with a response to a
+  // previous POST request.
+  int const client_request_method = s->hdr_info.client_request.method_get_wksidx();
+  int const cached_request_method = s->cache_info.object_read->request_get()->method_get_wksidx();
+  if (client_request_method != cached_request_method) {
+    SET_VIA_STRING(VIA_CACHE_RESULT, VIA_IN_CACHE_NOT_ACCEPTABLE);
+    SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_METHOD);
+    return false;
+  }
   // If cookies in response and no TTL set, we do not cache the doc
   if ((s->cache_control.ttl_in_cache <= 0) &&
       do_cookies_prevent_caching(static_cast<int>(s->txn_conf->cache_responses_to_cookies), &s->hdr_info.client_request,

proxy/http/HttpTransactHeaders.cc

@@ -87,9 +87,17 @@ HttpTransactHeaders::is_this_method_supported(int the_scheme, int the_method)
 bool
 HttpTransactHeaders::is_method_safe(int method)
 {
+  // See RFC 7231, section 4.2.1.
   return (method == HTTP_WKSIDX_GET || method == HTTP_WKSIDX_OPTIONS || method == HTTP_WKSIDX_HEAD || method == HTTP_WKSIDX_TRACE);
 }
 
+bool
+HttpTransactHeaders::is_status_an_error_response(HTTPStatus response_code)
+{
+  auto const comparable_response_code = static_cast<unsigned int>(response_code);
+  return (comparable_response_code >= 400) && (comparable_response_code <= 599);
+}
+
 bool
 HttpTransactHeaders::is_method_idempotent(int method)
 {

proxy/http/HttpTransactHeaders.h

@@ -56,6 +56,7 @@ class HttpTransactHeaders
   static bool downgrade_request(bool *origin_server_keep_alive, HTTPHdr *outgoing_request);
   static bool is_method_safe(int method);
   static bool is_method_idempotent(int method);
+  static bool is_status_an_error_response(HTTPStatus response_code);
 
   static void generate_and_set_squid_codes(HTTPHdr *header, char *via_string, HttpTransact::SquidLogInfo *squid_codes);
 

tests/gold_tests/cache/cache-request-method.test.py

@@ -0,0 +1,63 @@
+'''
+Verify correct caching behavior with respect to request method.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+Test.Summary = '''
+Verify correct caching behavior with respect to request method.
+'''
+
+# Test 0: Verify correct POST response handling when caching POST responses is
+# disabled.
+ts = Test.MakeATSProcess("ts")
+replay_file = "replay/post_with_post_caching_disabled.replay.yaml"
+server = Test.MakeVerifierServerProcess("server0", replay_file)
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http.*|cache.*',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    # Caching of POST responses is disabled by default. Verify default behavior
+    # by leaving it unconfigured.
+    # 'proxy.config.http.cache.post_method': 0,
+})
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server.Variables.http_port)
+)
+tr = Test.AddTestRun("Verify correct with POST response caching disabled.")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.AddVerifierClientProcess("client0", replay_file, http_ports=[ts.Variables.port])
+
+# Test 1: Verify correct POST response handling when caching POST responses is
+# enabled.
+ts = Test.MakeATSProcess("ts-cache-post")
+replay_file = "replay/post_with_post_caching_enabled.replay.yaml"
+server = Test.MakeVerifierServerProcess("server1", replay_file)
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http.*|cache.*',
+    'proxy.config.http.insert_age_in_response': 0,
+    'proxy.config.http.cache.post_method': 1,
+})
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server.Variables.http_port)
+)
+tr = Test.AddTestRun("Verify correct with POST response caching enabled.")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.AddVerifierClientProcess("client1", replay_file, http_ports=[ts.Variables.port])