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 <bugzi...@apache.org> wrote:
>>>>
>>>> https://bz.apache.org/bugzilla/show_bug.cgi?id=69235
>>>>
>>>> --- Comment #2 from Yann Ylavic <ylavic....@gmail.com> ---
>>>> 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, "proxy-nocanon", "1");
+ }
+
+ /* 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);
Of course tze one from PR 69235 needs to go on top.
Regards
Rüdiger
