On Tue, Aug 20, 2024 at 8:12 AM Ruediger Pluem <[email protected]> wrote:
>
>
>
> On 8/20/24 1:44 PM, Eric Covener wrote:
> > On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem <[email protected]> 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.
>
> Regards
>
> Rüdiger
>
>
--
Eric Covener
[email protected]
