Fix filter_body plugin: deterministic request blocking and response body blocking

plugins/experimental/filter_body/filter_body.cc

@@ -101,6 +101,10 @@ struct TransformData {
   bool                      headers_added = false;
 };
 
+// Forward declaration - send_response_handler is used in execute_actions
+// but defined later in the file.
+int send_response_handler(TSCont contp, TSEvent event, void *edata);
+
 /**
  * @brief Case-insensitive substring search.
  *
@@ -439,10 +443,22 @@ execute_actions(TransformData *data, Rule const *rule, std::string const *matche
   if (rule->actions & ACTION_BLOCK) {
     data->blocked = true;
     TSHttpTxnStatusSet(data->txnp, TS_HTTP_STATUS_FORBIDDEN);
-    // Set error body so client gets a proper response
     char const *error_body = "Blocked by content filter";
     TSHttpTxnErrorBodySet(data->txnp, TSstrdup(error_body), strlen(error_body), TSstrdup("text/plain"));
-    Dbg(dbg_ctl, "Blocking request due to rule: %s", rule->name.c_str());
+    if (data->direction == Direction::REQUEST) {
+      // Add a SEND_RESPONSE_HDR hook to override the error response to 403.
+      // Only added here (when actually blocking) to avoid hook overhead on
+      // every transaction.  The transform zeros its output, creating a
+      // Content-Length mismatch at the origin.  The short timeout below makes
+      // ATS close the hanging origin connection quickly instead of waiting the
+      // default 30 seconds.
+      TSCont send_resp_contp = TSContCreate(send_response_handler, nullptr);
+      TSHttpTxnHookAdd(data->txnp, TS_HTTP_SEND_RESPONSE_HDR_HOOK, send_resp_contp);
+      TSHttpTxnHookAdd(data->txnp, TS_HTTP_TXN_CLOSE_HOOK, send_resp_contp);
+      TSHttpTxnConfigIntSet(data->txnp, TS_CONFIG_HTTP_TRANSACTION_NO_ACTIVITY_TIMEOUT_OUT, 1);
+    }
+    Dbg(dbg_ctl, "Blocking %s body due to rule: %s", data->direction == Direction::REQUEST ? "request" : "response",
+        rule->name.c_str());
   }
 }
 
@@ -611,19 +627,21 @@ transform_handler(TSCont contp, TSEvent event, void *edata ATS_UNUSED)
         }
 
         if (data->blocked) {
-          // Blocking action - complete the transform with zero output
-          // The 403 status we set will cause ATS to generate the error response
+          // Zero the transform output so the blocked body is not forwarded.
+          // This creates a Content-Length mismatch at the origin (headers
+          // advertised the original body size but 0 bytes arrive). The short
+          // no-activity timeout set in execute_actions causes ATS to close
+          // the hanging origin connection quickly and generate an error
+          // response. The SEND_RESPONSE_HDR hook then overrides it to 403.
           TSVIONBytesSet(data->output_vio, 0);
           TSVIOReenable(data->output_vio);
 
-          // Consume all remaining input
+          // Consume all remaining input so the client side completes cleanly
           int64_t const remaining = TSIOBufferReaderAvail(reader);
           if (remaining > 0) {
             TSIOBufferReaderConsume(reader, remaining);
           }
           TSVIONDoneSet(write_vio, TSVIONBytesGet(write_vio));
-
-          // Signal write complete
           TSContCall(TSVIOContGet(write_vio), TS_EVENT_VCONN_WRITE_COMPLETE, write_vio);
           return 0;
         }
@@ -747,6 +765,49 @@ response_handler(TSCont contp, TSEvent event, void *edata)
   return 0;
 }
 
+/**
+ * @brief Send response handler to enforce 403 for blocked requests.
+ *
+ * Called on TS_HTTP_SEND_RESPONSE_HDR_HOOK only when the request body
+ * transform has blocked a request (hook is added in execute_actions).
+ * The transform zeros its output, creating a Content-Length mismatch that
+ * causes ATS to generate an error response (typically 502). This hook
+ * overrides that error to a deterministic 403 Forbidden.
+ *
+ * @param[in] contp The continuation.
+ * @param[in] event The event type (SEND_RESPONSE_HDR or TXN_CLOSE).
+ * @param[in] edata The HTTP transaction.
+ * @return Always returns 0.
+ */
+int
+send_response_handler(TSCont contp, TSEvent event, void *edata)
+{
+  TSHttpTxn txnp = static_cast<TSHttpTxn>(edata);
+
+  if (event == TS_EVENT_HTTP_TXN_CLOSE) {
+    TSContDestroy(contp);
+    TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
+    return 0;
+  }
+
+  if (event == TS_EVENT_HTTP_SEND_RESPONSE_HDR) {
+    // This hook is only added when the request body was blocked, so we
+    // unconditionally override the response to 403.
+    TSMBuffer bufp;
+    TSMLoc    hdr_loc;
+    if (TSHttpTxnClientRespGet(txnp, &bufp, &hdr_loc) == TS_SUCCESS) {
+      Dbg(dbg_ctl, "Overriding response to 403 for blocked request");
+      TSHttpHdrStatusSet(bufp, hdr_loc, TS_HTTP_STATUS_FORBIDDEN);
+      TSHttpHdrReasonSet(bufp, hdr_loc, TSHttpHdrReasonLookup(TS_HTTP_STATUS_FORBIDDEN),
+                         strlen(TSHttpHdrReasonLookup(TS_HTTP_STATUS_FORBIDDEN)));
+      TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc);
+    }
+  }
+
+  TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
+  return 0;
+}
+
 /**
  * @brief Parse the YAML configuration file.
  *

tests/gold_tests/autest-site/ats_replay.test.ext

@@ -178,6 +178,18 @@ def ATSReplayTest(obj, replay_file: str):
         rc = server_config['return_code']
         server.ReturnCode = Any(*rc) if isinstance(rc, list) else rc
 
+    # Configure server log validation if specified.
+    server_log_validation = server_config.get('log_validation', {})
+    if server_log_validation:
+        for contains_entry in server_log_validation.get('contains', []):
+            expression = contains_entry['expression']
+            description = contains_entry.get('description', f'Verify server output contains: {expression}')
+            server.Streams.All += Testers.ContainsExpression(expression, description)
+        for excludes_entry in server_log_validation.get('excludes', []):
+            expression = excludes_entry['expression']
+            description = excludes_entry.get('description', f'Verify server output excludes: {expression}')
+            server.Streams.All += Testers.ExcludesExpression(expression, description)
+
     # ATS configuration.
     if not 'ats' in autest_config:
         raise ValueError(f"Replay file {replay_file} does not contain 'autest.ats' section")
@@ -201,6 +213,18 @@ def ATSReplayTest(obj, replay_file: str):
         rc = client_config['return_code']
         client.ReturnCode = Any(*rc) if isinstance(rc, list) else rc
 
+    # Configure client log validation if specified.
+    client_log_validation = client_config.get('log_validation', {})
+    if client_log_validation:
+        for contains_entry in client_log_validation.get('contains', []):
+            expression = contains_entry['expression']
+            description = contains_entry.get('description', f'Verify client output contains: {expression}')
+            client.Streams.All += Testers.ContainsExpression(expression, description)
+        for excludes_entry in client_log_validation.get('excludes', []):
+            expression = excludes_entry['expression']
+            description = excludes_entry.get('description', f'Verify client output excludes: {expression}')
+            client.Streams.All += Testers.ExcludesExpression(expression, description)
+
     if dns:
         ts.StartBefore(dns)
     ts.StartBefore(server)

tests/gold_tests/pluginTest/filter_body/config/filter_body_response_block.yaml

@@ -13,4 +13,3 @@ rules:
         - "SSN:"
         - "password:"
     action: [log, block]
-

tests/gold_tests/pluginTest/filter_body/filter_body.replay.yaml

@@ -26,10 +26,16 @@ autest:
 
   server:
     name: 'server'
+    process_config:
+      other_args: '--verbose diag'
+    log_validation:
+      excludes:
+        - expression: "FILTER_BODY_BLOCK_CANARY"
+          description: "Verify blocked request body never reached the origin"
 
   client:
     name: 'client'
-    # There's a race condition for when the connection is droped by ATS - this
+    # There's a race condition for when the connection is dropped by ATS - this
     # can result in either a 0 or 1 return code.
     return_code: [0, 1]
 
@@ -122,8 +128,12 @@ autest:
             description: "Verify response block rule matched"
       traffic_out:
         contains:
-          - expression: "Blocking request due to rule"
+          - expression: "Blocking request body due to rule"
             description: "Verify request blocking action was taken"
+          - expression: "Overriding response to 403 for blocked request"
+            description: "Verify SEND_RESPONSE_HDR hook enforced 403 status"
+          - expression: "Blocking response body due to rule"
+            description: "Verify response blocking action was taken"
           - expression: "Added header X-Security-Match"
             description: "Verify header was added for request"
           # Adding an internal response is not supported while streaming the body.
@@ -167,11 +177,10 @@ sessions:
   #############################################################################
   # Test 2: Request block - request with XXE pattern is blocked
   #
-  # When blocking request bodies, ATS closes the connection to the origin and
-  # the client either experiences simply a closed connection or, depending upon
-  # timeing, a 502 Bad Gateway response. The plugin cannot send a custom error
-  # response (like 403) because the request headers have already been sent to
-  # the origin by the time the body is inspected.
+  # The plugin detects the malicious pattern in the request body and blocks it.
+  # The transform aborts the origin-side VConn to prevent the body from being
+  # forwarded, and a SEND_RESPONSE_HDR hook overrides whatever error ATS
+  # generates (typically 502) to a deterministic 403.
   #############################################################################
   - transactions:
       - client-request:
@@ -182,10 +191,10 @@ sessions:
             fields:
               - [Host, request-block.example.com]
               - [Content-Type, "application/xml+plus_other_stuff"]
-              - [Content-Length, 49]
+              - [Content-Length, 74]
               - [uuid, request-block-test]
           content:
-            data: '<?xml version="1.0"?><!ENTITY xxe SYSTEM "file:">'
+            data: 'FILTER_BODY_BLOCK_CANARY <?xml version="1.0"?><!ENTITY xxe SYSTEM "file:">'
 
         server-response:
           status: 200
@@ -197,7 +206,7 @@ sessions:
             data: "OK"
 
         proxy-response:
-          status: 502
+          status: 403
 
   #############################################################################
   # Test 3: Request header - request passes, header added to server request
@@ -334,9 +343,10 @@ sessions:
           # don't expect to see any external headers added.
 
   #############################################################################
-  # Test 7: Response block - detect pattern and attempt blocking
-  # Note: Response blocking after streaming starts has limitations - the
-  # response will still return 200 but the body will be blocked.
+  # Test 7: Response block - block responses with sensitive data
+  # The response body is blocked (zeroed) when a pattern is detected. Because
+  # response headers (including Content-Length) are committed before the body
+  # is inspected, the client sees a Content-Length mismatch.
   #############################################################################
   - transactions:
       - client-request:
@@ -362,6 +372,5 @@ sessions:
           content:
             data: '{"name": "John", "SSN: 123-45-6789"}'
 
-        # Note that blocking happens after the 200 response headers are sent.
         proxy-response:
           status: 200