Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 9/11/24 5:59 PM, Yann Ylavic wrote: > On Mon, Sep 9, 2024 at 12:20 PM Ruediger Pluem wrote: >> >> 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. > > Done (r1920570). > > I suppose we need a single backport proposal for both PRs 69235 and 69260? I marked 69260 as a duplicate of 69235, but still mentioned it in the changelog for the sake of referencing the discussion on both PR's. > If so maybe amend "changes-entries/pr69235.txt" with your follow up commit? Done in r1920572. Backport would be 1920570, 1920571, 1920572 then. Regards Rüdiger
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Wed, Sep 11, 2024 at 5:59 PM Yann Ylavic wrote: > > On Mon, Sep 9, 2024 at 12:20 PM Ruediger Pluem wrote: > > > > 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. > > Done (r1920570). > > I suppose we need a single backport proposal for both PRs 69235 and 69260? > If so maybe amend "changes-entries/pr69235.txt" with your follow up commit? Nevermind, just saw that PR 69260 is now a duplicate. > > Regards; > Yann.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Mon, Sep 9, 2024 at 12:20 PM Ruediger Pluem wrote: > > 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. Done (r1920570). I suppose we need a single backport proposal for both PRs 69235 and 69260? If so maybe amend "changes-entries/pr69235.txt" with your follow up commit? Regards; Yann.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
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 wrote:
>>>
>>>
>>>
>>> On 8/20/24 1:44 PM, Eric Covener wrote:
On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem 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() */
>>>
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 8/20/24 2:32 PM, Eric Covener wrote:
> On Tue, Aug 20, 2024 at 8:12 AM Ruediger Pluem wrote:
>>
>>
>>
>> On 8/20/24 1:44 PM, Eric Covener wrote:
>>> On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem 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-n
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Tue, Aug 20, 2024 at 8:12 AM Ruediger Pluem wrote:
>
>
>
> On 8/20/24 1:44 PM, Eric Covener wrote:
> > On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem 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.
> >
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 8/20/24 1:44 PM, Eric Covener wrote:
> On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem 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?
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.
>
>>
>>>
+
+/* 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
>
>>
>>>
>>>
* (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
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Tue, Aug 20, 2024 at 4:41 AM Ruediger Pluem 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.
>
> >
> >> +
> >> +/* 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])
>
> >
> >
> >> * (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.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
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.
>
>> +
>> +/* 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.
>
>
>> * (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") ?
Regards
Rüdiger
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Mon, Aug 19, 2024 at 8:14 AM 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
>
> > +
> > +/* 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].
>
>
> > * (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.
Or maybe not remember we set it, as they could have proxy-nocanon set directly.
--
Eric Covener
cove...@gmail.com
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
>
> 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
> +
> +/* 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].
> * (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.
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 8/19/24 11:24 AM, Ruediger Pluem wrote:
>
>
> On 8/7/24 2:09 PM, Ruediger Pluem wrote:
>>
>>
>> On 7/31/24 7:03 PM, Yann Ylavic wrote:
>>> On Wed, Jul 31, 2024 at 6:57 PM wrote:
https://bz.apache.org/bugzilla/show_bug.cgi?id=69235
--- Comment #2 from Yann Ylavic ---
Created attachment 39832
--> https://bz.apache.org/bugzilla/attachment.cgi?id=39832&action=edit
mod_proxy fixup after mod_rewrite's
>>>
>>> What could be the issue with mod_proxy fixing up / canonicalizing all
>>> mod_rewrite [P] URLs (including perdir)?
>>> Looks sensible to me..
>>>
>>
>> Just to reconfirm the current state:
>>
>> 1. If we have a server level rewrite rule with a [P] flag then the mod_proxy
>> fixup is run on it.
>> 2. If we have a directory level rewrite rule with a [P] flag then the
>> mod_proxy fixup is not applied to this rewritten URL as the
>> URL is rewritten after the mod_proxy fixup was run.
>>
>> The original commit r98707
>> (https://github.com/apache/httpd/commit/6f1e0d8307e0c874938b2a9881373ddb8564c84c)
>> seems to be a fix for
>> PR16368 (https://bz.apache.org/bugzilla/show_bug.cgi?id=16368).
>> I guess the different behavior is currently needed because of
>> https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5392-L5401
>>
>> In the directory context case we add the query string to r->filename whereas
>> we don't in the server context (at least not for
>> reverse proxies):
>>
>> https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5087-L5093
>>
>> The original code for the directory case seems to be there since ages:
>> https://github.com/apache/httpd/commit/e3e87d34a0280b4e88c87b86b715d2c710ffb7ec
>>
>> Maybe we should apply the same conditions for adding the query string in
>> directory context like in server context?
>>
>> I guess r->proxyreq == PROXYREQ_PROXY is always false in the directory
>> context which leaves us with the additional condition of
>> rulestatus == ACTION_NOESCAPE.
>>
>> So something like
>>
>> Index: modules/mappers/mod_rewrite.c
>> ===
>> --- modules/mappers/mod_rewrite.c(revision 1919056)
>> +++ modules/mappers/mod_rewrite.c(working copy)
>> @@ -5303,12 +5303,13 @@
>> return HTTP_FORBIDDEN;
>> }
>>
>> -/* make sure the QUERY_STRING and
>> - * PATH_INFO parts get incorporated
>> +/* 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.
>> * (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);
>>
>>
>> Plus the above patch proposed in PR 69235.
>
> Further possible issues not yet analyzed, just as a heads up:
>
> https://github.com/apache/httpd/blob/fe4ade610c750d63c3bbfe0d07c5a37f2d5cb9f0/modules/mappers/mod_rewrite.c#L4530-L4542
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, "pro
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 8/7/24 2:09 PM, Ruediger Pluem wrote:
>
>
> On 7/31/24 7:03 PM, Yann Ylavic wrote:
>> On Wed, Jul 31, 2024 at 6:57 PM wrote:
>>>
>>> https://bz.apache.org/bugzilla/show_bug.cgi?id=69235
>>>
>>> --- Comment #2 from Yann Ylavic ---
>>> Created attachment 39832
>>> --> https://bz.apache.org/bugzilla/attachment.cgi?id=39832&action=edit
>>> mod_proxy fixup after mod_rewrite's
>>
>> What could be the issue with mod_proxy fixing up / canonicalizing all
>> mod_rewrite [P] URLs (including perdir)?
>> Looks sensible to me..
>>
>
> Just to reconfirm the current state:
>
> 1. If we have a server level rewrite rule with a [P] flag then the mod_proxy
> fixup is run on it.
> 2. If we have a directory level rewrite rule with a [P] flag then the
> mod_proxy fixup is not applied to this rewritten URL as the
> URL is rewritten after the mod_proxy fixup was run.
>
> The original commit r98707
> (https://github.com/apache/httpd/commit/6f1e0d8307e0c874938b2a9881373ddb8564c84c)
> seems to be a fix for
> PR16368 (https://bz.apache.org/bugzilla/show_bug.cgi?id=16368).
> I guess the different behavior is currently needed because of
> https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5392-L5401
>
> In the directory context case we add the query string to r->filename whereas
> we don't in the server context (at least not for
> reverse proxies):
>
> https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5087-L5093
>
> The original code for the directory case seems to be there since ages:
> https://github.com/apache/httpd/commit/e3e87d34a0280b4e88c87b86b715d2c710ffb7ec
>
> Maybe we should apply the same conditions for adding the query string in
> directory context like in server context?
>
> I guess r->proxyreq == PROXYREQ_PROXY is always false in the directory
> context which leaves us with the additional condition of
> rulestatus == ACTION_NOESCAPE.
>
> So something like
>
> Index: modules/mappers/mod_rewrite.c
> ===
> --- modules/mappers/mod_rewrite.c (revision 1919056)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -5303,12 +5303,13 @@
> return HTTP_FORBIDDEN;
> }
>
> -/* make sure the QUERY_STRING and
> - * PATH_INFO parts get incorporated
> +/* 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.
> * (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);
>
>
> Plus the above patch proposed in PR 69235.
Further possible issues not yet analyzed, just as a heads up:
https://github.com/apache/httpd/blob/fe4ade610c750d63c3bbfe0d07c5a37f2d5cb9f0/modules/mappers/mod_rewrite.c#L4530-L4542
Regards
Rüdiger
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On 7/31/24 7:03 PM, Yann Ylavic wrote:
> On Wed, Jul 31, 2024 at 6:57 PM wrote:
>>
>> https://bz.apache.org/bugzilla/show_bug.cgi?id=69235
>>
>> --- Comment #2 from Yann Ylavic ---
>> Created attachment 39832
>> --> https://bz.apache.org/bugzilla/attachment.cgi?id=39832&action=edit
>> mod_proxy fixup after mod_rewrite's
>
> What could be the issue with mod_proxy fixing up / canonicalizing all
> mod_rewrite [P] URLs (including perdir)?
> Looks sensible to me..
>
Just to reconfirm the current state:
1. If we have a server level rewrite rule with a [P] flag then the mod_proxy
fixup is run on it.
2. If we have a directory level rewrite rule with a [P] flag then the mod_proxy
fixup is not applied to this rewritten URL as the
URL is rewritten after the mod_proxy fixup was run.
The original commit r98707
(https://github.com/apache/httpd/commit/6f1e0d8307e0c874938b2a9881373ddb8564c84c)
seems to be a fix for
PR16368 (https://bz.apache.org/bugzilla/show_bug.cgi?id=16368).
I guess the different behavior is currently needed because of
https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5392-L5401
In the directory context case we add the query string to r->filename whereas we
don't in the server context (at least not for
reverse proxies):
https://github.com/apache/httpd/blob/3e87e2ba980f6ba2a47c5d860ba6545087b35b21/modules/mappers/mod_rewrite.c#L5087-L5093
The original code for the directory case seems to be there since ages:
https://github.com/apache/httpd/commit/e3e87d34a0280b4e88c87b86b715d2c710ffb7ec
Maybe we should apply the same conditions for adding the query string in
directory context like in server context?
I guess r->proxyreq == PROXYREQ_PROXY is always false in the directory context
which leaves us with the additional condition of
rulestatus == ACTION_NOESCAPE.
So something like
Index: modules/mappers/mod_rewrite.c
===
--- modules/mappers/mod_rewrite.c (revision 1919056)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -5303,12 +5303,13 @@
return HTTP_FORBIDDEN;
}
-/* make sure the QUERY_STRING and
- * PATH_INFO parts get incorporated
+/* 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.
* (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);
Plus the above patch proposed in PR 69235.
Regards
Rüdiger
Re: [Bug 69235] unable to connect to backend with existing rewrite rules.
On Wed, Jul 31, 2024 at 6:57 PM wrote: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69235 > > --- Comment #2 from Yann Ylavic --- > Created attachment 39832 > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39832&action=edit > mod_proxy fixup after mod_rewrite's What could be the issue with mod_proxy fixing up / canonicalizing all mod_rewrite [P] URLs (including perdir)? Looks sensible to me..
