Re: svn commit: r1756560 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS include/ap_mmn.h modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h

[Marion & Christophe JAILLET]

Hi,


Le 17/08/2016 à 01:17, yla...@apache.org a écrit :

Author: ylavic
Date: Tue Aug 16 23:17:46 2016
New Revision: 1756560

URL: http://svn.apache.org/viewvc?rev=1756560=rev
Log:
Merge r1746207 from trunk:

mod_dav: Add support for childtags to dav_error.

Submitted by: minfrin
Reviewed by: minfrin, jim, ylavic

Modified:
 httpd/httpd/branches/2.4.x/   (props changed)
 httpd/httpd/branches/2.4.x/CHANGES
 httpd/httpd/branches/2.4.x/STATUS
 httpd/httpd/branches/2.4.x/include/ap_mmn.h
 httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.c
 httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.h

[...]

Modified: httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.c?rev=1756560=1756559=1756560=diff
==
--- httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.c (original)
+++ httpd/httpd/branches/2.4.x/modules/dav/main/mod_dav.c Tue Aug 16 23:17:46 
2016
@@ -364,16 +364,33 @@ static int dav_error_response_tag(reques
  ap_rputs(" xmlns:m=\"http://apache.org/dav/xmlns\";, r);
  }
  
-if (err->namespace != NULL) {

-ap_rprintf(r,
-   " xmlns:C=\"%s\">" DEBUG_CR
-   "" DEBUG_CR,
-   err->namespace, err->tagname);
+if (err->childtags) {
+if (err->namespace != NULL) {
+ap_rprintf(r,
+" xmlns:C=\"%s\">" DEBUG_CR
+"%s" DEBUG_CR,
+err->namespace,
+err->tagname, err->childtags, err->tagname);
+}
+else {
+ap_rprintf(r,
+">" DEBUG_CR
+"%s" DEBUG_CR,

Shouldn't this be:
"%s" DEBUG_CR,
in order to close the D tag?


+err->tagname, err->childtags, err->tagname);
+}
  }
  else {
-ap_rprintf(r,
-   ">" DEBUG_CR
-   "" DEBUG_CR, err->tagname);
+if (err->namespace != NULL) {
+ap_rprintf(r,
+" xmlns:C=\"%s\">" DEBUG_CR
+"" DEBUG_CR,
+err->namespace, err->tagname);
+}
+else {
+ap_rprintf(r,
+">" DEBUG_CR
+"" DEBUG_CR, err->tagname);
+}
  }
  
  /* here's our mod_dav specific tag: */

[...

CJ

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel 
antivirus Avast.
https://www.avast.com/antivirus

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

[William A Rowe Jr]

On Aug 16, 2016 4:39 PM, "Yann Ylavic"  wrote:
>
> On Tue, Aug 16, 2016 at 8:11 PM,   wrote:
> > Author: wrowe
> > Date: Tue Aug 16 18:11:14 2016
> > New Revision: 1756540
> >
> > URL: http://svn.apache.org/viewvc?rev=1756540=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).

Agreed, I'll note it. Was thinking this was an internal config but it is in
a published header, as you note.

> > 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.

Will fix that along with MMN.

> > Modified: httpd/httpd/trunk/server/core.c
> > URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1756540=1756539=1756540=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?

IMO, no. I anticipated we might someday add Require1.1 as a third
alternative, as you alluded to below.

> > -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
> = looks more extensible (to me)...

It appeared that both exclusivity tests were borked because the value was
first set, ergo the old bit was unset, and then a meaningless test occurred
(doh).

Allow0.9 or Require1.0 is more readily apparent in perusing someone else's
config files. We have too many uses of generic (and case sensitive? Ick)
keywords that makes it harder to spot misconfiguration. Do you really want
to grep for the token and the directive name, or an arbitrary search for
min= which has a number of contextual meanings?

> 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] ?

I'm open to suggestions, what about HTTPProtocolOptions (option list)?

> > Modified: httpd/httpd/trunk/server/protocol.c
> > URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1756540=1756539=1756540=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;
> > + 

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

[Yann Ylavic]

On Tue, Aug 16, 2016 at 8:11 PM,   wrote:
> Author: wrowe
> Date: Tue Aug 16 18:11:14 2016
> New Revision: 1756540
>
> URL: http://svn.apache.org/viewvc?rev=1756540=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=1756539=1756540=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
= 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=1756539=1756540=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, , )
>  && (ap_cstr_casecmp("http", http) == 0)

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


Regards,
Yann.

Re: svn commit: r1756531 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Roy T. Fielding]

> On Aug 16, 2016, at 9:51 AM, Eric Covener  wrote:
> 
> On Tue, Aug 16, 2016 at 12:26 PM, Roy T. Fielding  wrote:
>> It used to be that we always log INFO because we only use it for noting
>> configuration details.  Has that changed?
> 
> You're probably thinking of the special handling of NOTICE level, so n/a here.

Oh, right. Brain fart.

Roy

Re: svn commit: r1756531 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Eric Covener]

On Tue, Aug 16, 2016 at 12:26 PM, Roy T. Fielding  wrote:
> It used to be that we always log INFO because we only use it for noting
> configuration details.  Has that changed?

You're probably thinking of the special handling of NOTICE level, so n/a here.

Re: svn commit: r1756531 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

[Roy T. Fielding]

> On Aug 16, 2016, at 9:21 AM, yla...@apache.org wrote:
> 
> Author: ylavic
> Date: Tue Aug 16 16:21:13 2016
> New Revision: 1756531
> 
> URL: http://svn.apache.org/viewvc?rev=1756531=rev
> Log:
> Follow up to r1750392: reduce AH03408 level to INFO as suggested by wrowe/jim.

It used to be that we always log INFO because we only use it for noting
configuration details.  Has that changed?

Roy

Re: svn commit: r1756186 - in /httpd/httpd/trunk: include/ modules/http/ modules/proxy/

[Yann Ylavic]

On Mon, Aug 15, 2016 at 2:46 PM, William A Rowe Jr  wrote:
> On Aug 14, 2016 3:35 PM, "Yann Ylavic"  wrote:
>>
>> > Why the change in log level?
>>
>> Reading a response before any request was sent looks quite severe to
>> me, a warning might better alert the admin about some (backend)
>> misbehaviour/response splitting, while a debug could stay unnoticed.
>
> If this is their own back end (and not in a forward proxy scenario) info
> level should be sufficient for daily operation.

Agreed, changed in r1756531, thanks for the suggestion.