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. > > Regards > > Rüdiger > > -- Eric Covener cove...@gmail.com