Improve redirect handling

* Do not permit a 302 to be cached if a redirect is in progress. * Could be more permissive if we remove the check for `HTTP_STATUS_MOVED_TEMPORARILY` in `HttpTransact::is_response_cacheable()` * Refactor HttpSM::is_redirect_required() to make it easier to call when state is incomplete.
elsloo · Jan 9, 2024 · 88659cc · 88659cc
1 parent 46a6570
commit 88659cc
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 4 deletions.
diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h
@@ -258,6 +258,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
 
   bool is_private() const;
   bool is_redirect_required();
+  bool is_redirect_required_for_status(HTTPStatus *status);
 
   /// Get the protocol stack for the inbound (client, user agent) connection.
   /// @arg result [out] Array to store the results

diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc
@@ -8499,19 +8499,33 @@ HttpSM::is_private() const
   return res;
 }
 
-// check to see if redirection is enabled and less than max redirections tries or if a plugin enabled redirection
+// convenience method for callers that do not want to provide a status
 inline bool
 HttpSM::is_redirect_required()
+{
+  return is_redirect_required_for_status(nullptr);
+}
+
+// check to see if redirection is enabled and less than max redirections tries or if a plugin enabled redirection
+inline bool
+HttpSM::is_redirect_required_for_status(HTTPStatus *status)
 {
   bool redirect_required = (enable_redirection && (redirection_tries < t_state.txn_conf->number_of_redirections) &&
                             !HttpTransact::is_fresh_cache_hit(t_state.cache_lookup_result));
 
   SMDbg(dbg_ctl_http_redirect, "redirect_required: %u", redirect_required);
 
   if (redirect_required == true) {
-    HTTPStatus status = t_state.hdr_info.client_response.status_get();
+    HTTPStatus s;
+
+    if (status != nullptr) {
+      s = *status;
+    } else {
+      s = t_state.hdr_info.client_response.status_get();
+    }
+
     // check to see if the response from the origin was a 301, 302, or 303
-    switch (status) {
+    switch (s) {
     case HTTP_STATUS_MULTIPLE_CHOICES:   // 300
     case HTTP_STATUS_MOVED_PERMANENTLY:  // 301
     case HTTP_STATUS_MOVED_TEMPORARILY:  // 302

diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc
@@ -6260,6 +6260,11 @@ HttpTransact::is_response_cacheable(State *s, HTTPHdr *request, HTTPHdr *respons
     return false;
   }
 
+  // don't cache a 302 when a redirect is being chased
+  if (response_code == HTTP_STATUS_MOVED_TEMPORARILY && s->state_machine->is_redirect_required_for_status(&response_code)) {
+    return false;
+  }
+
   // check if cache control overrides default cacheability
   int indicator;
   indicator = response_cacheable_indicated_by_cc(response, s->cache_control.ignore_server_no_cache);

diff --git a/tests/gold_tests/redirect/redirect_post.test.py b/tests/gold_tests/redirect/redirect_post.test.py
@@ -78,7 +78,7 @@
     'map http://127.0.0.1:{0} http://127.0.0.1:{1}'.format(ts.Variables.port, redirect_serv1.Variables.Port))
 
 tr = Test.AddTestRun()
-tr.Processes.Default.Command = 'touch largefile.txt && truncate largefile.txt -s 50M && curl -H "Expect: " -i http://127.0.0.1:{0}/redirect1 -F "filename=@./largefile.txt" && rm -f largefile.txt'.format(
+tr.Processes.Default.Command = 'touch largefile.txt && truncate -s 50M largefile.txt && curl -H "Expect: " -i http://127.0.0.1:{0}/redirect1 -F "filename=@./largefile.txt" && rm -f largefile.txt'.format(
     ts.Variables.port)
 tr.Processes.Default.StartBefore(ts)
 tr.Processes.Default.StartBefore(redirect_serv1)