Re: svn commit: r1876674 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http2/h2_request.c server/protocol.c

[Ruediger Pluem <rp...@apache.org>]

On 4/17/20 6:47 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Apr 17 16:47:42 2020
> New Revision: 1876674
> 
> URL: http://svn.apache.org/viewvc?rev=1876674&view=rev
> Log:
> core, h2: common ap_parse_request_line() and ap_check_request_header() code.
> 
> Extract parsing/validation code from read_request_line() and ap_read_request()
> into ap_parse_request_line() and ap_check_request_header() helpers such that
> mod_http2 can validate its HTTP/1 request with the same/configured policy.
> 
> Modified:
>     httpd/httpd/trunk/include/ap_mmn.h
>     httpd/httpd/trunk/include/http_protocol.h
>     httpd/httpd/trunk/modules/http2/h2_request.c
>     httpd/httpd/trunk/server/protocol.c
> 

> 
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1876674&r1=1876673&r2=1876674&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Fri Apr 17 16:47:42 2020

> @@ -1444,50 +1526,13 @@ request_rec *ap_read_request(conn_rec *c
>          }
>      }
>  
> -    /* update what we think the virtual host is based on the headers we've
> -     * now read. may update status.
> -     */
> -    strict_host_check = (conf->strict_host_check == AP_CORE_CONFIG_ON);
> -    access_status = ap_update_vhost_from_headers_ex(r, strict_host_check);
> -    if (strict_host_check && access_status != HTTP_OK) { 
> -         if (r->server == ap_server_conf) { 
> -             ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10156)
> -                           "Requested hostname '%s' did not match any ServerName/ServerAlias "
> -                           "in the global server configuration ", r->hostname);
> -         } else { 
> -             ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10157)
> -                           "Requested hostname '%s' did not match any ServerName/ServerAlias "
> -                           "in the matching virtual host (default vhost for "
> -                           "current connection is %s:%u)", 
> -                           r->hostname, r->server->defn_name, r->server->defn_line_number);
> -         }
> -         goto die_early;
> -    }
> -    if (r->status != HTTP_OK) { 
> +    if (!ap_check_request_header(r)) {
>          access_status = r->status;
>          goto die_early;

This means we now die_early where we did not before and may not reply with custom error pages configured for the vhost.

>      }
>  
> -    if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1, 1)))
> -        || ((r->proto_num == HTTP_VERSION(1, 1))
> -            && !apr_table_get(r->headers_in, "Host"))) {
> -        /*
> -         * Client sent us an HTTP/1.1 or later request without telling us the
> -         * hostname, either with a full URL or a Host: header. We therefore
> -         * need to (as per the 1.1 spec) send an error.  As a special case,
> -         * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain
> -         * a Host: header, and the server MUST respond with 400 if it doesn't.
> -         */
> -        ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
> -                      "client sent HTTP/1.1 request without hostname "
> -                      "(see RFC2616 section 14.23): %s", r->uri);
> -        access_status = HTTP_BAD_REQUEST;
> -        goto die_early;
> -    }
> -
>      /* we may have switched to another server */
>      r->per_dir_config = r->server->lookup_defaults;
> -    conf = ap_get_core_module_config(r->server->module_config);
>  
>      /* Toggle to the Host:-based vhost's timeout mode to fetch the
>       * request body and send the response body, if needed.

Regards

Rüdiger

Re: svn commit: r1876674 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http2/h2_request.c server/protocol.c

[Yann Ylavic <yl...@gmail.com>]

On Fri, Apr 17, 2020 at 9:59 PM Ruediger Pluem
>
> On 4/17/20 6:47 PM, ylavic
> >
> > -    if (r->status != HTTP_OK) {
> > +    if (!ap_check_request_header(r)) {
> >          access_status = r->status;
> >          goto die_early;
>
> This means we now die_early where we did not before and may not reply with custom error pages configured for the vhost.

See my reply for r1876664, die_early will also call ap_die() finally
so custom error pages are still in the place.

There are two semantic changes compared to before r1876664 (the first
commit) though, first there are cases where we previously called
ap_send_error_response() only while now we ap_die() always (almost).
It did not make sense to me to call ap_die() on read_request_line()
failure but not later, if ErrorDocument is relevant with request line
errors I think it is for later ones.
Second, this commit (r1876674) checks the Expect header in
ap_check_request_header(), that is before post_read_request hooks,
while we cheched (and failed) after that before. I think this change
makes sense too.

Regards,
Yann.