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
