Looks like there is no further feedback on the below. @Yann: Care to commit https://bz.apache.org/bugzilla/attachment.cgi?id=39832 ? I would then commit my stuff below.
Regards Rüdiger On 8/20/24 5:20 PM, Ruediger Pluem wrote: > > > On 8/20/24 2:32 PM, Eric Covener wrote: >> On Tue, Aug 20, 2024 at 8:12 AM Ruediger Pluem <rpl...@apache.org> wrote: >>> >>> >>> >>> On 8/20/24 1:44 PM, Eric Covener wrote: >>>> On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem <rpl...@apache.org> wrote: >>>>> >>>>> >>>>> >>>>> On 8/19/24 2:14 PM, Eric Covener wrote: >>>>>>> >>>>>>> Maybe reverting r757427 >>>>>>> (https://svn.apache.org/viewvc?view=revision&revision=757427) fixes >>>>>>> this. >>>>>>> My new patch sofar: >>>>>>> >>>>>>> Index: modules/mappers/mod_rewrite.c >>>>>>> =================================================================== >>>>>>> --- modules/mappers/mod_rewrite.c (revision 1920017) >>>>>>> +++ modules/mappers/mod_rewrite.c (working copy) >>>>>>> @@ -4527,20 +4527,6 @@ >>>>>>> * ourself). >>>>>>> */ >>>>>>> if (p->flags & RULEFLAG_PROXY) { >>>>>>> - /* For rules evaluated in server context, the mod_proxy fixup >>>>>>> - * hook can be relied upon to escape the URI as and when >>>>>>> - * necessary, since it occurs later. If in directory context, >>>>>>> - * the ordering of the fixup hooks is forced such that >>>>>>> - * mod_proxy comes first, so the URI must be escaped here >>>>>>> - * instead. See PR 39746, 46428, and other headaches. */ >>>>>>> - if (ctx->perdir && (p->flags & RULEFLAG_NOESCAPE) == 0) { >>>>>>> - char *old_filename = r->filename; >>>>>>> - >>>>>>> - r->filename = ap_escape_uri(r->pool, r->filename); >>>>>>> - rewritelog(r, 2, ctx->perdir, "escaped URI in per-dir >>>>>>> context " >>>>>>> - "for proxy, %s -> %s", old_filename, >>>>>>> r->filename); >>>>>>> - } >>>>>>> - >>>>>>> fully_qualify_uri(r); >>>>>>> >>>>>>> rewritelog(r, 2, ctx->perdir, "forcing proxy-throughput with >>>>>>> %s", >>>>>>> @@ -5392,12 +5378,17 @@ >>>>>>> return HTTP_FORBIDDEN; >>>>>>> } >>>>>>> >>>>>>> - /* make sure the QUERY_STRING and >>>>>>> - * PATH_INFO parts get incorporated >>>>>>> + if (rulestatus == ACTION_NOESCAPE) { >>>>>>> + apr_table_setn(r->notes, "proxy-nocanon", "1"); >>>>>>> + } >>>>>> >>>>>> This seems to preserve 2.4.58 behavior of [P,NE] in per-dir. >>>>>> We should re-document NE in this context though >>>>> >>>>> I am not sure what you mean here. >>>> >>>> Sorry, I originally had a copy of the description from the manual. >>>> Current doc for [NE] talks about internal redirects only. >>> >>> But people seem to use [P,NE] e.g >>> https://bz.apache.org/bugzilla/show_bug.cgi?id=69260 >>> Shouldn't we ensure that it does what people expect at least in 2.4.x? >> >> True. I guess we can say e.g. "When used with [P], [NE] prevents >> mod_proxy from escaping the substitution. mod_rewrite does not >> implicitly escape the substitution when the [P] flag is used" >> >>> What else should people do? proxy-nocanon is a note and not an environment >>> variable. Hence I guess >>> they have no other option with a RewriteRule. >> >> I didn't appreciate the note-vs-envvar, good point. >> >>> >>>> >>>>> >>>>>> >>>>>>> + >>>>>>> + /* make sure the QUERY_STRING gets incorporated, but only >>>>>>> + * if we do not escape the target. Otherwise the mod_proxy >>>>>>> + * canon handler will take care to do this. >>>>>> >>>>>> nit: I don't think we will ever escape the target later though [in >>>>>> mod_rewrite]. >>>>> >>>>> What the comment wants to say is that we only need to add the >>>>> QUERY_STRING if we do no escape the target. In case we escape the >>>>> target the QUERY_STRING gets added by the canon handler. >>>> >>>> Right, but I think it's misleading because there is no more escaping >>>> to come in mod_rewrite along this path (much less any controlled by >>>> [NE]) >>> >>> How about the below instead? >>> >>> /* make sure the QUERY_STRING gets incorporated, but only >>> * if we do not escape the target. Otherwise the mod_proxy >>> * canon handler will take care to add the QUERY_STRING >> >> >> /* make sure the QUERY_STRING gets incorporated in the case [NE] was >> specified >> * on the Proxy rule. We are preventing mod_proxy canon handler from >> * incorporating r->args as well as escaping the URL. >> */ >> >>> >>>> >>>>> >>>>>> >>>>>> >>>>>>> * (r->path_info was already appended by the >>>>>>> * rewriting engine because of the per-dir context!) >>>>>>> */ >>>>>>> - if (r->args != NULL) { >>>>>>> + if ((r->args != NULL) && (rulestatus == ACTION_NOESCAPE)) { >>>>>>> /* see proxy_http:proxy_http_canon() */ >>>>>>> r->filename = apr_pstrcat(r->pool, r->filename, >>>>>>> "?", r->args, NULL); >>>>>> >>>>>> nit: It might be more clear here to check (or memoize) proxy-nocanon >>>>>> directly, for the same confusion as above. >>>>>> >>>>> >>>>> You mean replace (rulestatus == ACTION_NOESCAPE) with >>>>> apr_table_get(r->notes, "proxy-nocanon") ? >>>> >>>> I think so, or alternatively remove any significance of [NE] for proxy. >>>> >>> >>> I will do so and post a new version of the patch here. > > > New version of patch: > > Index: modules/mappers/mod_rewrite.c > =================================================================== > --- modules/mappers/mod_rewrite.c (revision 1920017) > +++ modules/mappers/mod_rewrite.c (working copy) > @@ -4527,20 +4527,6 @@ > * ourself). > */ > if (p->flags & RULEFLAG_PROXY) { > - /* For rules evaluated in server context, the mod_proxy fixup > - * hook can be relied upon to escape the URI as and when > - * necessary, since it occurs later. If in directory context, > - * the ordering of the fixup hooks is forced such that > - * mod_proxy comes first, so the URI must be escaped here > - * instead. See PR 39746, 46428, and other headaches. */ > - if (ctx->perdir && (p->flags & RULEFLAG_NOESCAPE) == 0) { > - char *old_filename = r->filename; > - > - r->filename = ap_escape_uri(r->pool, r->filename); > - rewritelog(r, 2, ctx->perdir, "escaped URI in per-dir context " > - "for proxy, %s -> %s", old_filename, r->filename); > - } > - > fully_qualify_uri(r); > > rewritelog(r, 2, ctx->perdir, "forcing proxy-throughput with %s", > @@ -5089,7 +5075,7 @@ > } > if ((r->args != NULL) > && ((r->proxyreq == PROXYREQ_PROXY) > - || (rulestatus == ACTION_NOESCAPE))) { > + || apr_table_get(r->notes, "proxy-nocanon"))) { > /* see proxy_http:proxy_http_canon() */ > r->filename = apr_pstrcat(r->pool, r->filename, > "?", r->args, NULL); > @@ -5392,13 +5378,18 @@ > return HTTP_FORBIDDEN; > } > > - /* make sure the QUERY_STRING and > - * PATH_INFO parts get incorporated > + if (rulestatus == ACTION_NOESCAPE) { > + apr_table_setn(r->notes, "proxy-nocanon", "1"); > + } > + > + /* make sure the QUERY_STRING gets incorporated in the case > + * [NE] was specified on the Proxy rule. We are preventing > + * mod_proxy canon handler from incorporating r->args as well > + * as escaping the URL. > * (r->path_info was already appended by the > * rewriting engine because of the per-dir context!) > */ > - if (r->args != NULL) { > - /* see proxy_http:proxy_http_canon() */ > + if ((r->args != NULL) && apr_table_get(r->notes, > "proxy-nocanon")) { > r->filename = apr_pstrcat(r->pool, r->filename, > "?", r->args, NULL); > } > > > Regards > > Rüdiger > >
