httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1756540 - in /httpd/httpd/trunk: docs/manual/mod/core.xml modules/http/http_filters.c server/core.c server/protocol.c server/vhost.c
Date Tue, 16 Aug 2016 21:39:43 GMT
On Tue, Aug 16, 2016 at 8:11 PM,  <wrowe@apache.org> wrote:
> Author: wrowe
> Date: Tue Aug 16 18:11:14 2016
> New Revision: 1756540
>
> URL: http://svn.apache.org/viewvc?rev=1756540&view=rev
> Log:
> Rename the previously undocumented HTTPProtocol directive
> to EnforceHTTPProtocol, and invert the default behavior
> to strictly observe RFC 7230 unless otherwise configured.
> And Document This.
>
> The relaxation option is renamed 'Unsafe'. 'Strict' is no
> longer case sensitive. 'min=0.9|1.0' is now the verbose
> 'Allow0.9' or 'Require1.0' case-insenstive grammer. The
> exclusivity tests have been modified to detect conflicts.
>
> The 'strict,log' option failed to enforce strict conformance,
> and has been removed. Unsafe, informational logging is possible
> in any loadable module, after the request data is unsafely
> accepted.

This probably requires a MMN bump (possibly major w.r.t trunk).

>
[]
>
> Modified:
>     httpd/httpd/trunk/docs/manual/mod/core.xml
>     httpd/httpd/trunk/modules/http/http_filters.c
>     httpd/httpd/trunk/server/core.c
>     httpd/httpd/trunk/server/protocol.c
>     httpd/httpd/trunk/server/vhost.c

Looks like changes to include/http_core.h are missing, at least
AP_HTTP_CONFORMANCE_UNSAFE is undefined for now.

>
[]
>
> Modified: httpd/httpd/trunk/server/core.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1756540&r1=1756539&r2=1756540&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Tue Aug 16 18:11:14 2016
> @@ -4011,37 +4011,39 @@ static const char *set_protocols_honor_o
[]
> +static const char *set_enforce_http_protocol(cmd_parms *cmd, void *dummy,
> +                                             const char *arg)
>  {
[]
> +    if ((conf->http09_enable & AP_HTTP09_ENABLE) &&
> +        (conf->http09_enable & AP_HTTP09_DISABLE)) {
> +        return "EnforceHttpProtocol 'Allow0.9' and 'Require1.0'"
> +               " are mutually exclusive";
> +    }

So why define two separate keywords for those?
Wouldn't "Forbid0.9" be simpler?

>
[]
> -AP_INIT_ITERATE("HttpProtocol", set_http_protocol, NULL, RSRC_CONF,
> -              "'min=0.9' (default) or 'min=1.0' to allow/deny HTTP/0.9; "
> -              "'liberal', 'strict', 'strict,log-only'"),
> +AP_INIT_ITERATE("EnforceHttpProtocol", set_enforce_http_protocol, NULL, RSRC_CONF,
> +              "'Allow0.9' or 'Require1.0' (default) to allow or deny HTTP/0.9; "
> +              "'Unsafe' or 'Strict' (default) to process incorrect requests"),

The original min= was naturally mutually exclusive, and the original
<option>= looks more extensible (to me)...

Also, since this directive will mainly be used to relax HTTP protocol
(wrt RFC 7230) for compatibility reasons, EnforceHttpProtocol may not
be appropriate.

How about HttpProtocolPolicy [Strict|Unsafe] min=[0.9|1.0|1.1] ?

>
[]
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1756540&r1=1756539&r2=1756540&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Tue Aug 16 18:11:14 2016
> @@ -687,13 +686,11 @@ static int read_request_line(request_rec
>          if (strict) {
>              ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>                            "Invalid protocol '%s'", r->protocol);
> -            if (enforce_strict) {
> -                r->proto_num = HTTP_VERSION(1,0);
> -                r->protocol  = "HTTP/1.0";
> -                r->connection->keepalive = AP_CONN_CLOSE;
> -                r->status = HTTP_BAD_REQUEST;
> -                return 0;
> -            }
> +            r->proto_num = HTTP_VERSION(1,0);
> +            r->protocol  = "HTTP/1.0";
> +            r->connection->keepalive = AP_CONN_CLOSE;
> +            r->status = HTTP_BAD_REQUEST;
> +            return 0;
>          }
>          if (3 == sscanf(r->protocol, "%4s/%u.%u", http, &major, &minor)
>              && (ap_cstr_casecmp("http", http) == 0)

While at it, "HTTP" (caps) looks a good strict candidate here :)


Regards,
Yann.

Mime
View raw message