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

This reverts commit c75c797.

This change introduces a failure in the esi AuTest. I'll revert this now
to free up CI and circle back around on the patch later.

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() const;
+  int method_get_wksidx();
   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() const
+HTTPHdr::method_get_wksidx()
 {
   ink_assert(valid());
   ink_assert(m_http->m_polarity == HTTP_TYPE_REQUEST);

proxy/http/HttpTransact.cc

@@ -2085,14 +2085,6 @@ 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);
@@ -3101,8 +3093,7 @@ 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->http_config_param->cache_post_method == 1 && s->method == HTTP_WKSIDX_POST) ||
-        s->api_resp_cacheable == true) {
+    if (s->method == HTTP_WKSIDX_GET || 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;
@@ -3332,7 +3323,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 some non-GET requests as well.
+  // We do a cache lookup for DELETE and PUT 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;
@@ -4484,11 +4475,10 @@ 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 &&
-               server_request_method == HTTP_WKSIDX_HEAD) {
+               s->hdr_info.server_request.method_get_wksidx() == 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();
@@ -4505,21 +4495,7 @@ 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) {
-        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;
-        }
+        s->cache_info.action = CACHE_DO_DELETE;
       } else if (s->method == HTTP_WKSIDX_HEAD) {
         s->cache_info.action = CACHE_DO_DELETE;
       } else {
@@ -4537,19 +4513,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
       }
 
     } else if (s->cache_info.action == CACHE_DO_DELETE) {
-      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;
-      }
+      // do nothing
 
     } else {
       ink_assert(!("cache action inconsistent with current state"));
@@ -5956,18 +5920,6 @@ 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,17 +87,9 @@ 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,7 +56,6 @@ 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);