[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apache-httpd-dev
Subject:    [PATCH 62186] POST request getting logged as GET request
From:       Micha Lenk <micha () lenk ! info>
Date:       2018-03-29 19:09:59
Message-ID: 5a3d8774-4362-cd4d-6d6c-0534291f8726 () lenk ! info
[Download RAW message or body]

Hi Apache httpd committers,

I think I've found a bug which triggers under following conditions:

* Apache is configured to serve a local customized error page, e.g.
   using something like "ErrorDocument 404 /var/www/errors/404.html"

* Apache is configured to log the original request's method, e.g.
   using something like (please note, the "%<m" is the part that
   matters):
   CustomLog logs/mylog "%h %l %u %t \"%r\" %>s %b method=\"%<m\""

* HTTP request is a POST request (request body content doesn't matter)

* the request destination path results in a 404 Not Found error

If all these conditions are met, the request will get logged to
logs/mylog with a line ending with 'method="GET"', even though this was
a POST request.

For easier reproduction of this case, I've created a patch that extends
the Apache httpd test suite to cover this case. Please see the attached
file bz62186_httpd-test.patch.

An explanation of this behavior can be found in the code of ap_die_r(),
which explicitly sets r->method to "GET" and r->method_number to M_GET
before it is calling ap_internal_redirect(custom_response, r) to serve
the configured error document.

I've tried to fix this issue by taking a backup of the original
request's method and restoring it as soon as ap_internal_redirect()
returns (see attached patch bz62186_httpd_bugfix.patch). So far the
tests I've done are successful, i.e. the request is now correctly logged
as POST request.

I've filed this issue some days ago as
https://bz.apache.org/bugzilla/show_bug.cgi?id=62186 , but so far it
didn't get any comments yet. Could anybody please take a look?


Kind regards,
Micha


["bz62186_httpd-test.patch" (text/x-patch)]

--- t/apache/errordoc_method_logging.t	(nonexistent)
+++ t/apache/errordoc_method_logging.t	(working copy)
@@ -0,0 +1,34 @@ 
+use strict;
+use warnings FATAL => 'all';
+
+use Data::Dumper;
+use Apache::Test;
+use Apache::TestRequest;
+use Apache::TestUtil qw/t_cmp
+                        t_start_file_watch
+                        t_finish_file_watch/;
+
+Apache::TestRequest::module('error_document');
+
+plan tests => 3, need_lwp;
+
+{
+    t_start_file_watch 'method_log';
+
+    my $response = POST '/method_logging', content => 'does not matter';
+    chomp(my $content = $response->content);
+
+    ok t_cmp($response->code,
+             404,
+             'POST /method_logging, code');
+
+    ok t_cmp($content,
+             'Error 404 Test',
+             'POST /method/logging, content');
+
+    my @loglines = t_finish_file_watch 'method_log';
+    chomp @loglines;
+    ok t_cmp($loglines[0],
+             qr/"POST \/method_logging HTTP\/1.1" .* method="POST"/,
+             'POST /method/logging, log');
+}
--- t/conf/extra.conf.in	(revision 1826815)
+++ t/conf/extra.conf.in	(working copy)
@@ -742,7 +742,11 @@ 
 ## 
 <VirtualHost _default_:error_document>
     ErrorDocument 404 "per-server 404
-                                                                                     \
 +    <IfModule mod_log_config.c>
+        CustomLog logs/method_log "%h %l %u %t \"%r\" %>s %b method=\"%<m\""
+        CustomLog logs/access_log "%h %l %u %t \"%r\" %>s %b"
+    </IfModule>
+
     <Location /redefine>
         ErrorDocument 404 "per-dir 404
     </Location>
@@ -760,6 +764,10 @@ 
         ErrorDocument 404 default
     </Location>
 
+    <Location /method_logging>
+        ErrorDocument 404 /apache/errordoc/404.html
+    </Location>
+
     <Directory @DocumentRoot@/apache>
          ErrorDocument 404 "testing merge
     </Directory>
--- t/htdocs/apache/errordoc/404.html	(nonexistent)
+++ t/htdocs/apache/errordoc/404.html	(working copy)
@@ -0,0 +1, @@ 
+Error 404 Test


["bz62186_httpd_bugfix.patch" (text/x-patch)]

--- modules/http/http_request.c	(revision 1826989)
+++ modules/http/http_request.c	(working copy)
@@ -187,7 +187,8 @@ 
             apr_table_setn(r->headers_out, "Location", custom_response);
         }
         else if (custom_response[0] == '/') {
-            const char *error_notes;
+            const char *error_notes, *original_method;
+            int original_method_number;
             r->no_local_copy = 1;       /* Do NOT send HTTP_NOT_MODIFIED for
                                          * error documents! */
             /*
@@ -205,9 +206,13 @@ 
                                              "error-notes")) != NULL) {
                 apr_table_setn(r->subprocess_env, "ERROR_NOTES", error_notes);
             }
+            original_method = r->method;
+            original_method_number = r->method_number;
             r->method = "GET";
             r->method_number = M_GET;
             ap_internal_redirect(custom_response, r);
+            r->method = original_method;
+            r->method_number = original_method_number;
             return;
         }
         else {



[prev in list] [next in list] [prev in thread] [next in thread]