fix: process request body once at EOS and correct status/filter bugs

src/mod_security3.c

@@ -145,6 +145,8 @@ static msc_t *create_tx_context(request_rec *r) {
     }
 
     msr->r = r;
+    msr->request_body_processed = 0;  /* Initialize flag */
+
     unique_id = getenv("UNIQUE_ID");
     if (unique_id != NULL && strlen(unique_id) > 0) {
         msr->t = msc_new_transaction_with_id(msc_apache->modsec,
@@ -365,17 +367,17 @@ static int hook_request_late(request_rec *r)
     /* Find the transaction context and make sure
      * we are supposed to proceed.
      */
-#ifdef REQUEST_EARLY
     msr = retrieve_tx_context(r);
-#else
-    msr = create_tx_context(r);
-#endif
     if (msr == NULL)
     {
-        /* If we can't find the context that probably means it's
-         * a subrequest that was not initiated from the outside.
+        /* Context should have been created by hook_insert_filter,
+         * but create it now if it doesn't exist for some reason.
          */
-        return DECLINED;
+        msr = create_tx_context(r);
+        if (msr == NULL)
+        {
+            return DECLINED;
+        }
     }
 
 #ifdef LATE_CONNECTION_PROCESS
@@ -400,7 +402,37 @@ static int hook_request_late(request_rec *r)
 #endif
 
 
+    /* Set up to read the request body.
+     * This is necessary to trigger the input filter which buffers the body.
+     */
+    int rc = ap_setup_client_block(r, REQUEST_CHUNKED_ERROR);
+    if (rc != OK)
+    {
+        return rc;
+    }
+
+    /* If there's a request body, read it to trigger the input filter */
+    if (ap_should_client_block(r))
+    {
+        char buffer[HUGE_STRING_LEN];
+        apr_off_t len;
+
+        /* Read body using the simpler ap_get_client_block API
+         * This should trigger our input filter for each chunk */
+        while ((len = ap_get_client_block(r, buffer, sizeof(buffer))) > 0)
+        {
+            /* The input filter intercepts this and appends to ModSecurity */
+            /* We don't need to do anything with the data here */
+        }
+    }
+
+    /* Process request body.
+     * The input filter has buffered body data during ap_get_brigade above.
+     * Now we process it. This handler can properly return HTTP status codes
+     * for interventions, unlike the input filter.
+     */
     msc_process_request_body(msr->t);
+
     it = process_intervention(msr->t, r);
     if (it != N_INTERVENTION_STATUS)
     {
@@ -448,11 +480,15 @@ static void hook_insert_filter(request_rec *r)
 {
     msc_t *msr = NULL;
 
-    /* Find the transaction context first. */
+    /* Find the transaction context, or create it if it doesn't exist yet. */
     msr = retrieve_tx_context(r);
     if (msr == NULL)
     {
-        return;
+        msr = create_tx_context(r);
+        if (msr == NULL)
+        {
+            return;
+        }
     }
 
 #if 1
@@ -571,7 +607,9 @@ static void msc_register_hooks(apr_pool_t *pool)
     /* still, we don't have location configuration yet. */
     ap_hook_process_connection(hook_connection_early, NULL, NULL, APR_HOOK_FIRST);
 
-    ap_hook_fixups(hook_request_late, fixups_beforeme_list, NULL, APR_HOOK_REALLY_FIRST);
+    /* Register as handler to read request body in the proper phase
+     * Don't use fixups - body reading must happen in handler phase */
+    ap_hook_handler(hook_request_late, NULL, NULL, APR_HOOK_REALLY_FIRST);
 
     /* Lets add the remaining hooks */
     ap_hook_insert_filter(hook_insert_filter, NULL, NULL, APR_HOOK_FIRST);

src/mod_security3.h

@@ -51,6 +51,7 @@ typedef struct
 {
     request_rec *r;
     Transaction *t;
+    int request_body_processed;  /* Flag to track if body was processed */
 } msc_t;
 
 

src/msc_filters.c

@@ -19,18 +19,16 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut,
     {
         ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_NOERRNO, 0, f->r->server,
                 "ModSecurity: Internal Error: msr is null in input filter.");
-        ap_remove_output_filter(f);
+        ap_remove_input_filter(f);
         return send_error_bucket(msr, f, HTTP_INTERNAL_SERVER_ERROR);
     }
 
     pbbTmp = apr_brigade_create(r->pool, c->bucket_alloc);
-    if (APR_BRIGADE_EMPTY(pbbTmp))
-    {
-        ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes);
 
-        if (mode == AP_MODE_EATCRLF || ret != APR_SUCCESS)
-            return ret;
-    }
+    ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes);
+
+    if (mode == AP_MODE_EATCRLF || ret != APR_SUCCESS)
+        return ret;
 
     while (!APR_BRIGADE_EMPTY(pbbTmp))
     {
@@ -43,6 +41,11 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut,
 
         if (APR_BUCKET_IS_EOS(pbktIn))
         {
+            /* Mark that we've buffered the complete request body */
+            /* The actual processing and intervention handling will be done
+             * by hook_request_late, which can properly return HTTP status codes */
+            msr->request_body_processed = 1;
+
             APR_BUCKET_REMOVE(pbktIn);
             APR_BRIGADE_INSERT_TAIL(pbbOut, pbktIn);
             break;
@@ -54,16 +57,8 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut,
             return ret;
         }
 
+        /* Append body chunk - processing will happen in hook_request_late */
         msc_append_request_body(msr->t, data, len);
-        it = process_intervention(msr->t, r);
-        if (it != N_INTERVENTION_STATUS)
-        {
-            ap_remove_output_filter(f);
-            return send_error_bucket(msr, f, it);
-        }
-
-        // FIXME: Now we should have the body. Is this sane?
-        msc_process_request_body(msr->t);
 
         pbktOut = apr_bucket_heap_create(data, len, 0, c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(pbbOut, pbktOut);
@@ -132,7 +127,16 @@ apr_status_t output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in)
         {
             const char *data;
             apr_size_t len;
-            apr_bucket_read(pbktIn, &data, &len, APR_BLOCK_READ);
+            apr_status_t rv;
+
+            rv = apr_bucket_read(pbktIn, &data, &len, APR_BLOCK_READ);
+            if (rv != APR_SUCCESS)
+            {
+                ap_log_error(APLOG_MARK, APLOG_ERR, rv, f->r->server,
+                    "ModSecurity: Error reading response body bucket");
+                return rv;
+            }
+
             msc_append_response_body(msr->t, data, len);
         }
         msc_process_response_body(msr->t);

src/msc_utils.c

@@ -21,7 +21,8 @@ apr_status_t send_error_bucket(msc_t *msr, ap_filter_t *f, int status)
     apr_bucket_brigade *brigade = NULL;
     apr_bucket *bucket = NULL;
 
-    /* Set the status line explicitly for the error document */
+    /* Set both status code and status line */
+    f->r->status = status;
     f->r->status_line = ap_get_status_line(status);
 
     brigade = apr_brigade_create(f->r->pool, f->r->connection->bucket_alloc);