Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Sep 27, 2024 at 2:48 PM Eric Covener wrote: > > Feel free to ask if you want to clarify some bits in FPM or its tests. I > will also try to find some time to test this patch (most likely next week) > and later I plan to do some integration tests which could potentially run > daily against trunk (it's a bit longer term plan though). > > Hi Jakub, do you have a result from current/recent trunk vs 2.4.62 w/ > the added tests? I think if we are improving relative to 2.4.62 it > might be better to at least get this stuff released, even if it still > disrupts some things from the historical behavior (that are likely > busted in 2.4.62 too) > Hi Eric, apology for the long delay. I have been quite busy with some unrelated PHP security isseus and also wanted to get my FPM integration tool working with httpd which I finally menaged to do. I created few SetHandler using tests in https://github.com/wstool/wst-php-fpm/blob/bf390dd1dda196ea9339b4d3d26caf5fbf306226/spec/instances/httpd-proxy-fcgi-handler-basic.yaml and they are all good when testing https://github.com/apache/httpd/pull/470 . That PR looks good to me and should be safe to merge IMHO. I would like to do some UDS testing next week (I got it as a priority now so should really happen next week - not like last time... :) ) but I wouldn't expect issues there too. In terms of https://github.com/apache/httpd/pull/489 , I will need to think about it a bit more. It took me a bit to get my head around the current code and still need a bit more time to fully understand it. I will also need a bit more time to produce some integration tests for ProxyPass and ProxyPassMatch. But as I said I have got it as my main FPM prioirity so it should not hopefully take too long. Kind regards, Jakub
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
> Feel free to ask if you want to clarify some bits in FPM or its tests. I will > also try to find some time to test this patch (most likely next week) and > later I plan to do some integration tests which could potentially run daily > against trunk (it's a bit longer term plan though). Hi Jakub, do you have a result from current/recent trunk vs 2.4.62 w/ the added tests? I think if we are improving relative to 2.4.62 it might be better to at least get this stuff released, even if it still disrupts some things from the historical behavior (that are likely busted in 2.4.62 too)
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
Hello,
I'm PHP-FPM maintainer. We got actually report about this as well so just
went through this.
On Sat, Aug 3, 2024 at 7:35 PM Eric Covener wrote:
> On Fri, Aug 2, 2024 at 12:19 PM Yann Ylavic wrote:
> >
> > On Fri, Aug 2, 2024 at 3:26 PM Eric Covener wrote:
> > >
> > > On Fri, Aug 2, 2024 at 9:10 AM Yann Ylavic
> wrote:
> > > >
> > > > On Fri, Aug 2, 2024 at 1:06 PM Eric Covener
> wrote:
> > > > >
> > > > > > Yeah, if not under DocumentRoot I don't see how ProxyPass could
> work,
> > > > > > but SetHandler should since it's following the whole request
> > > > > > processing to resolve the filesystem r->filename?
> > > > >
> > > > > In the examples I am seeing spot-checking google results, people
> who
> > > > > use ProxyPass + FPM hard-code the document root (or wherever the
> stuff
> > > > > is) in the 2nd parameter. This includes our own manual and the
> > > > > unofficial confluence wiki.
> > > >
> > > > Ah ok, I think we can do something like this:
> > > > if (rconf->need_dirwalk) {
> > > > const char *docroot = ap_document_root(r);
> > > > if (strncmp(docroot, r->filename, strlen(docroot)) != 0)
> {
> > > > r->proxyreq = PROXYREQ_NONE;
> > > > ap_core_translate(r);
> > > > }
> > > > ap_directory_walk(r);
> > > > }
> > > > ?
> > >
> > > I think users could be happily humming along with some other absolute
> > > filesystem path in the ProxyPass 2nd arg and would now see it
> > > docroot-prefixed.
> > > Are you trying to make the ProxyPass+FPM vars better because they will
> > > no longer be fixed up by apache_was_here side effects? I think it is
> > > probably bes to just retain/restore some of the historical bogus
> > > values if MAY_BE_FPM -- maybe doing them at the last moment where we'd
> > > do the above.
> >
> > Maybe this:
> > if (rconf) {
> > if (!rconf->need_dirwalk) {
> > r->filename = rconf->filename;
> > }
> > else {
> > char *saved_uri = r->uri;
> > char *saved_path_info = r->path_info;
> > int i = 0;
> >
> > /* Try to find the script without DocumentRoot than with */
> > do {
> > r->path_info = NULL;
> > r->filename = r->uri = rconf->filename;
> > if (i) {
> > r->proxyreq = PROXYREQ_NONE;
> > ap_core_translate(r);
> > r->proxyreq = PROXYREQ_REVERSE;
> > }
> > ap_directory_walk(r);
> > } while (r->finfo.filetype != APR_REG && ++i < 2);
> >
> > /* If no actual script was found, fall back to the "proxy:"
> > * SCRIPT_FILENAME dealt with below or by php-fpm directly.
> > */
> > if (i == 2) {
> > r->path_info = saved_path_info;
> > r->filename = proxy_filename;
> > }
> > /* Restore REQUEST_URI in any case */
> > r->uri = saved_uri;
> > }
> > }
> > ?
>
> Looks good to me, curious what you are thinking for changes-entry or
> is this more for a semblance of sanity in this area of code if we have
> to come back to it?
>
> This seems to (at least):
> - fix slightly nonsensical combo of SetHandler + proxy-fcgi-pathinfo=full
> - allows ProxyPass + proxy-fcgi-pathinfo=full to not repeat the
> document root in the directive
>
> I don't see a ton of references to proxy-fcgi-pathinfo in the wild. It
> was kind of pre-emptive as I remember along with the
> ProxyFCGISetEnvIf. But one the few I found (on php.net) said "Don't
> use ProxyPassMatch. Use SetHandler."
>
>
I don't think this is correct as we definitely have users using
ProxyPassMatch and it mostly just works for their use cases.
> We could also consider deprecating the use of proxypass specifically
> with FPM (where the balancer scenario is kind of moot).
>
I wouldn't deprecate proxypass with FPM. Note that the most usual setup is
just a single file (index.php) getting the actual path in path info where
proxypass is just fine. WordPress is a bit different but think that works
too. So deprecating something that would be helpful for absolute minority
of users does not make much sense to me. It might be better to just
document the limitations.
In terms of logic, the only part that we decode is pathinfo if it's taken
from proxy pass because there is no PATH_INFO env that we would use
otherwise. For such case, we need to take it from SCRIPT_FILENAME which is
encoded so we need to decode it. All of this is just a horrible piece of
code that accumulated over the years (probably my least favourity part of
FPM really) so any changes can easily break things and would be good idea
to test it properly. I added some tests that show the expectation that we
have which can be found here:
https://github.com/php/php-src/tree/master/sapi/fpm/tests (all of those
fcgi-*-apache*).
F
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 12:19 PM Yann Ylavic wrote:
>
> On Fri, Aug 2, 2024 at 3:26 PM Eric Covener wrote:
> >
> > On Fri, Aug 2, 2024 at 9:10 AM Yann Ylavic wrote:
> > >
> > > On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> > > >
> > > > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > > > but SetHandler should since it's following the whole request
> > > > > processing to resolve the filesystem r->filename?
> > > >
> > > > In the examples I am seeing spot-checking google results, people who
> > > > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > > > is) in the 2nd parameter. This includes our own manual and the
> > > > unofficial confluence wiki.
> > >
> > > Ah ok, I think we can do something like this:
> > > if (rconf->need_dirwalk) {
> > > const char *docroot = ap_document_root(r);
> > > if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> > > r->proxyreq = PROXYREQ_NONE;
> > > ap_core_translate(r);
> > > }
> > > ap_directory_walk(r);
> > > }
> > > ?
> >
> > I think users could be happily humming along with some other absolute
> > filesystem path in the ProxyPass 2nd arg and would now see it
> > docroot-prefixed.
> > Are you trying to make the ProxyPass+FPM vars better because they will
> > no longer be fixed up by apache_was_here side effects? I think it is
> > probably bes to just retain/restore some of the historical bogus
> > values if MAY_BE_FPM -- maybe doing them at the last moment where we'd
> > do the above.
>
> Maybe this:
> if (rconf) {
> if (!rconf->need_dirwalk) {
> r->filename = rconf->filename;
> }
> else {
> char *saved_uri = r->uri;
> char *saved_path_info = r->path_info;
> int i = 0;
>
> /* Try to find the script without DocumentRoot than with */
> do {
> r->path_info = NULL;
> r->filename = r->uri = rconf->filename;
> if (i) {
> r->proxyreq = PROXYREQ_NONE;
> ap_core_translate(r);
> r->proxyreq = PROXYREQ_REVERSE;
> }
> ap_directory_walk(r);
> } while (r->finfo.filetype != APR_REG && ++i < 2);
>
> /* If no actual script was found, fall back to the "proxy:"
> * SCRIPT_FILENAME dealt with below or by php-fpm directly.
> */
> if (i == 2) {
> r->path_info = saved_path_info;
> r->filename = proxy_filename;
> }
> /* Restore REQUEST_URI in any case */
> r->uri = saved_uri;
> }
> }
> ?
Looks good to me, curious what you are thinking for changes-entry or
is this more for a semblance of sanity in this area of code if we have
to come back to it?
This seems to (at least):
- fix slightly nonsensical combo of SetHandler + proxy-fcgi-pathinfo=full
- allows ProxyPass + proxy-fcgi-pathinfo=full to not repeat the
document root in the directive
I don't see a ton of references to proxy-fcgi-pathinfo in the wild. It
was kind of pre-emptive as I remember along with the
ProxyFCGISetEnvIf. But one the few I found (on php.net) said "Don't
use ProxyPassMatch. Use SetHandler."
We could also consider deprecating the use of proxypass specifically
with FPM (where the balancer scenario is kind of moot).
--
Eric Covener
cove...@gmail.com
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:26 PM Eric Covener wrote:
>
> On Fri, Aug 2, 2024 at 9:10 AM Yann Ylavic wrote:
> >
> > On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> > >
> > > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > > but SetHandler should since it's following the whole request
> > > > processing to resolve the filesystem r->filename?
> > >
> > > In the examples I am seeing spot-checking google results, people who
> > > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > > is) in the 2nd parameter. This includes our own manual and the
> > > unofficial confluence wiki.
> >
> > Ah ok, I think we can do something like this:
> > if (rconf->need_dirwalk) {
> > const char *docroot = ap_document_root(r);
> > if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> > r->proxyreq = PROXYREQ_NONE;
> > ap_core_translate(r);
> > }
> > ap_directory_walk(r);
> > }
> > ?
>
> I think users could be happily humming along with some other absolute
> filesystem path in the ProxyPass 2nd arg and would now see it
> docroot-prefixed.
> Are you trying to make the ProxyPass+FPM vars better because they will
> no longer be fixed up by apache_was_here side effects? I think it is
> probably bes to just retain/restore some of the historical bogus
> values if MAY_BE_FPM -- maybe doing them at the last moment where we'd
> do the above.
Maybe this:
if (rconf) {
if (!rconf->need_dirwalk) {
r->filename = rconf->filename;
}
else {
char *saved_uri = r->uri;
char *saved_path_info = r->path_info;
int i = 0;
/* Try to find the script without DocumentRoot than with */
do {
r->path_info = NULL;
r->filename = r->uri = rconf->filename;
if (i) {
r->proxyreq = PROXYREQ_NONE;
ap_core_translate(r);
r->proxyreq = PROXYREQ_REVERSE;
}
ap_directory_walk(r);
} while (r->finfo.filetype != APR_REG && ++i < 2);
/* If no actual script was found, fall back to the "proxy:"
* SCRIPT_FILENAME dealt with below or by php-fpm directly.
*/
if (i == 2) {
r->path_info = saved_path_info;
r->filename = proxy_filename;
}
/* Restore REQUEST_URI in any case */
r->uri = saved_uri;
}
}
?
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919628)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -141,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -181,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -359,16 +365,43 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *proxy_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+if (!rconf->need_dirwalk) {
+r->filename = rconf->filename;
+}
+else {
+char *saved_uri = r->uri;
+char *saved_path_info = r->path_info;
+int i = 0;
+
+/* Try to find the script without DocumentRoot than with */
+do {
+r->path_info = NULL;
+r->filename = r->uri = rconf->filename;
+if (i) {
+r->proxyreq = PROXYREQ_NONE;
+ap_core_translate(r);
+r->proxyreq = PROXYREQ_REVERSE
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 9:10 AM Yann Ylavic wrote:
>
> On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> >
> > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > but SetHandler should since it's following the whole request
> > > processing to resolve the filesystem r->filename?
> >
> > In the examples I am seeing spot-checking google results, people who
> > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > is) in the 2nd parameter. This includes our own manual and the
> > unofficial confluence wiki.
>
> Ah ok, I think we can do something like this:
> if (rconf->need_dirwalk) {
> const char *docroot = ap_document_root(r);
> if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> r->proxyreq = PROXYREQ_NONE;
> ap_core_translate(r);
> }
> ap_directory_walk(r);
> }
> ?
I think users could be happily humming along with some other absolute
filesystem path in the ProxyPass 2nd arg and would now see it
docroot-prefixed.
Are you trying to make the ProxyPass+FPM vars better because they will
no longer be fixed up by apache_was_here side effects? I think it is
probably bes to just retain/restore some of the historical bogus
values if MAY_BE_FPM -- maybe doing them at the last moment where we'd
do the above.
--
Eric Covener
cove...@gmail.com
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:10 PM Yann Ylavic wrote:
>
> On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
> >
> > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > > but SetHandler should since it's following the whole request
> > > processing to resolve the filesystem r->filename?
> >
> > In the examples I am seeing spot-checking google results, people who
> > use ProxyPass + FPM hard-code the document root (or wherever the stuff
> > is) in the 2nd parameter. This includes our own manual and the
> > unofficial confluence wiki.
>
> Ah ok, I think we can do something like this:
> if (rconf->need_dirwalk) {
> const char *docroot = ap_document_root(r);
> if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
> r->proxyreq = PROXYREQ_NONE;
> ap_core_translate(r);
> }
> ap_directory_walk(r);
> }
> ?
Full patch (v2).
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919628)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -141,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -181,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -359,18 +365,26 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *saved_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+r->filename = rconf->filename;
+
+/* To fix/set r->filename/path_info, do now what ProxyPass skipped */
+if (rconf->need_dirwalk) {
+const char *docroot = ap_document_root(r);
+if (strncmp(r->filename, docroot, strlen(docroot)) != 0) {
+r->proxyreq = PROXYREQ_NONE;
+ap_core_translate(r);
+}
+ap_directory_walk(r);
+}
}
-
-/* Strip proxy: prefixes */
-if (r->filename) {
+else if (r->filename) {
char *newfname = NULL;
+/* Strip proxy: prefixes */
+
if (!strncmp(r->filename, "proxy:balancer://", 17)) {
newfname = apr_pstrdup(r->pool, r->filename+17);
}
@@ -401,6 +415,9 @@ static apr_status_t send_environment(proxy_conn_re
ap_add_common_vars(r);
ap_add_cgi_vars(r);
+r->filename = saved_filename;
+r->proxyreq = PROXYREQ_REVERSE;
+
/* XXX are there any FastCGI specific env vars we need to send? */
/* Give admins final option to fine-tune env vars */
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 1:06 PM Eric Covener wrote:
>
> > Yeah, if not under DocumentRoot I don't see how ProxyPass could work,
> > but SetHandler should since it's following the whole request
> > processing to resolve the filesystem r->filename?
>
> In the examples I am seeing spot-checking google results, people who
> use ProxyPass + FPM hard-code the document root (or wherever the stuff
> is) in the 2nd parameter. This includes our own manual and the
> unofficial confluence wiki.
Ah ok, I think we can do something like this:
if (rconf->need_dirwalk) {
const char *docroot = ap_document_root(r);
if (strncmp(docroot, r->filename, strlen(docroot)) != 0) {
r->proxyreq = PROXYREQ_NONE;
ap_core_translate(r);
}
ap_directory_walk(r);
}
?
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
> Yeah, if not under DocumentRoot I don't see how ProxyPass could work, > but SetHandler should since it's following the whole request > processing to resolve the filesystem r->filename? In the examples I am seeing spot-checking google results, people who use ProxyPass + FPM hard-code the document root (or wherever the stuff is) in the 2nd parameter. This includes our own manual and the unofficial confluence wiki.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 11:33 AM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 6:02 AM Eric Covener wrote: > > > > On Thu, Aug 1, 2024 at 9:22 PM Yann Ylavic wrote: > > > > > > > > For this how about this attached patch? > > > > With it I can get the correct env vars (I think), and since we'd not > > > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > > > "apache_was_there" and work straight with the raw vars? I'm probably > > > > having a sweet dream :) > > > > Just in case.. > > > > > > PS: the script needs to exist in the DOCUMENT_ROOT for this to work, > > > but that's how php-fpm works I suppose. > > > > This is somewhat over my head (despite writing and forgetting some of > > those fcgi kludges) but tell me if I am close. > > > > - proxy-fcgi-pathinfo was only meant to be used with ProxyPass, not > > SetHandler, but this is not explicit in the code. > > Yeah, I didn't change that part. > > > - The current code to generate SCRIPT_FILENAME (supposed to be a > > filename) and PATH_INFO probably doesn't work for the ProxyPass path > > where it's actually needed. This is because r->filename won't even be > > docroot-prefixed if mod_proxy handles translate_name early. > > Correct. > > > - Your addition gets it to at least work for stuff that is under the > > DocumentRoot, as the directory walk will now split into r->filename > > and r->path_info based on what was on disk > > Yeah, if not under DocumentRoot I don't see how ProxyPass could work, > but SetHandler should since it's following the whole request > processing to resolve the filesystem r->filename? > > > - Your addition also causes all ProxyPass configs that didn't tell us > > non-FPM backend type explicitly to act like proxy-fcgi-pathinfo=full > > (dirwalk). > > Yeah, not really thought deeply but I don't get what ProxyPass without > proxy-fcgi-pathinfo is meant for.. > > > > > Does the latest patch still leave us with proxy:fcgi:// in the env for > > FPM or Unknown or is it part of the dream scenario? > > No more proxy:fcgi://, that's the bet :) And we probably can stop blocking '?' then.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 6:02 AM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 9:22 PM Yann Ylavic wrote: > > > > > > For this how about this attached patch? > > > With it I can get the correct env vars (I think), and since we'd not > > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > > "apache_was_there" and work straight with the raw vars? I'm probably > > > having a sweet dream :) > > > Just in case.. > > > > PS: the script needs to exist in the DOCUMENT_ROOT for this to work, > > but that's how php-fpm works I suppose. > > This is somewhat over my head (despite writing and forgetting some of > those fcgi kludges) but tell me if I am close. > > - proxy-fcgi-pathinfo was only meant to be used with ProxyPass, not > SetHandler, but this is not explicit in the code. Yeah, I didn't change that part. > - The current code to generate SCRIPT_FILENAME (supposed to be a > filename) and PATH_INFO probably doesn't work for the ProxyPass path > where it's actually needed. This is because r->filename won't even be > docroot-prefixed if mod_proxy handles translate_name early. Correct. > - Your addition gets it to at least work for stuff that is under the > DocumentRoot, as the directory walk will now split into r->filename > and r->path_info based on what was on disk Yeah, if not under DocumentRoot I don't see how ProxyPass could work, but SetHandler should since it's following the whole request processing to resolve the filesystem r->filename? > - Your addition also causes all ProxyPass configs that didn't tell us > non-FPM backend type explicitly to act like proxy-fcgi-pathinfo=full > (dirwalk). Yeah, not really thought deeply but I don't get what ProxyPass without proxy-fcgi-pathinfo is meant for.. > > Does the latest patch still leave us with proxy:fcgi:// in the env for > FPM or Unknown or is it part of the dream scenario? No more proxy:fcgi://, that's the bet :)
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 5:22 AM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 9:12 PM Yann Ylavic wrote: > > > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > > > So we probably should keep encoding r->filename with ProxyPass, and > > > come back to my previous patch which skipped it only for SetHandler? > > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > > GENERIC" we don't send the "proxy:scheme://host" part and > > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > > > So I did this in r1919629. > > Just to level-set, IIUC: > > - 2.4.59 encoded SCRIPT_FILENAME for ProxyPass but not SetHandler > didn't and this same fragile stuff in php-fpm is CVE-2024-38473 > - 2.4.60 started encoding these, for CVE-2024-38473 + general safety > - PR69203 reports a few different symptoms where SCRIPT_FILENAME has > URL-encoded file names that are actually spaces, utf-8, etc. > - https://github.com/apache/httpd/pull/470 undoes the re-encoding > introduced in 2.4.60 but makes sure controls and '?' aren't in the > SCRIPT_FILENAME (for CVE-2024-38473 and related funny business). Yeah correct, just '?' is not forbidden when !FCGI_MAY_BE_FPM because in this case SCRIPT_FILENAME is not the "proxy:" URL (https://github.com/apache/httpd/commit/cab0bfbb2645bb8f689535e5e2834e2dbc23f5a5 applies). > > So this sounds reasonable to me without upsetting the fragile link > between php-fpm and proxy_fcgi. > > > > But it's going to be an endless issue if we can't fix or align > > > ProxyPass and SetHandler because of workarounds there, we have to > > > remain bug compatible.. > > I wonder does ProxyPass just not work with php-fpm and these > spaces/utf-8 scenarios? Possibly because php-fpm seems to do some decoding itself when it thinks apache_was_there with ProxyPass, but I didn't test nor fully follow the code. > > > For this how about this attached patch? > > With it I can get the correct env vars (I think), and since we'd not > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > "apache_was_there" and work straight with the raw vars? I'm probably > > having a sweet dream :) > > It sounds the most rational, but it also sounds very similar to the > breakage the t/modules/proxy_fcgi.t points to here: > https://github.com/apache/httpd/commit/cab0bfbb2645bb8f689535e5e2834e2dbc23f5a5 Not really the same because we don't just skip the "proxy:scheme://host" for SCRIPT_FILENAME, we really "resolve" the rest through ap_core_translate() (which adds DOCUMENT_ROOT) and ap_directory_walk() (which splits, provided the script exists). Eg. by requesting "/sympa/index.php/blah" I get: (gdb) dump_table r->subprocess_env [11] 'DOCUMENT_ROOT'='/var/www/htdocs' [0x556c4e98] [16] 'SCRIPT_FILENAME'='/var/www/htdocs/sympa/index.php' [0x7fffe8007378] [22] 'REQUEST_URI'='/sympa/index.php/blah' [0x7fffe8007cc8] [23] 'SCRIPT_NAME'='/sympa/index.php' [0x7fffe8007ce0] [24] 'PATH_INFO'='/blah' [0x7fffe80067ef] [25] 'PATH_TRANSLATED'='/var/www/htdocs/blah' [0x7fffe8007d10] > > As the thread talks about, maybe we can get them to accept some other > way to signal Apache. Or with the real SCRIPT_FILENAME get them to not think apache_was_there..
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 9:22 PM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 3:12 AM Yann Ylavic wrote: > > > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > > > So we probably should keep encoding r->filename with ProxyPass, and > > > come back to my previous patch which skipped it only for SetHandler? > > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > > GENERIC" we don't send the "proxy:scheme://host" part and > > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > > > So I did this in r1919629. > > > > > > > > But it's going to be an endless issue if we can't fix or align > > > ProxyPass and SetHandler because of workarounds there, we have to > > > remain bug compatible.. > > > > For this how about this attached patch? > > With it I can get the correct env vars (I think), and since we'd not > > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > > "apache_was_there" and work straight with the raw vars? I'm probably > > having a sweet dream :) > > Just in case.. > > PS: the script needs to exist in the DOCUMENT_ROOT for this to work, > but that's how php-fpm works I suppose. This is somewhat over my head (despite writing and forgetting some of those fcgi kludges) but tell me if I am close. - proxy-fcgi-pathinfo was only meant to be used with ProxyPass, not SetHandler, but this is not explicit in the code. - The current code to generate SCRIPT_FILENAME (supposed to be a filename) and PATH_INFO probably doesn't work for the ProxyPass path where it's actually needed. This is because r->filename won't even be docroot-prefixed if mod_proxy handles translate_name early. - Your addition gets it to at least work for stuff that is under the DocumentRoot, as the directory walk will now split into r->filename and r->path_info based on what was on disk - Your addition also causes all ProxyPass configs that didn't tell us non-FPM backend type explicitly to act like proxy-fcgi-pathinfo=full (dirwalk). Does the latest patch still leave us with proxy:fcgi:// in the env for FPM or Unknown or is it part of the dream scenario? I ran out of time before being able to apply it and run it through tests.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 9:12 PM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > So we probably should keep encoding r->filename with ProxyPass, and > > come back to my previous patch which skipped it only for SetHandler? > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > GENERIC" we don't send the "proxy:scheme://host" part and > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > So I did this in r1919629. Just to level-set, IIUC: - 2.4.59 encoded SCRIPT_FILENAME for ProxyPass but not SetHandler didn't and this same fragile stuff in php-fpm is CVE-2024-38473 - 2.4.60 started encoding these, for CVE-2024-38473 + general safety - PR69203 reports a few different symptoms where SCRIPT_FILENAME has URL-encoded file names that are actually spaces, utf-8, etc. - https://github.com/apache/httpd/pull/470 undoes the re-encoding introduced in 2.4.60 but makes sure controls and '?' aren't in the SCRIPT_FILENAME (for CVE-2024-38473 and related funny business). So this sounds reasonable to me without upsetting the fragile link between php-fpm and proxy_fcgi. > > But it's going to be an endless issue if we can't fix or align > > ProxyPass and SetHandler because of workarounds there, we have to > > remain bug compatible.. I wonder does ProxyPass just not work with php-fpm and these spaces/utf-8 scenarios? > For this how about this attached patch? > With it I can get the correct env vars (I think), and since we'd not > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > "apache_was_there" and work straight with the raw vars? I'm probably > having a sweet dream :) It sounds the most rational, but it also sounds very similar to the breakage the t/modules/proxy_fcgi.t points to here: https://github.com/apache/httpd/commit/cab0bfbb2645bb8f689535e5e2834e2dbc23f5a5 As the thread talks about, maybe we can get them to accept some other way to signal Apache.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 3:12 AM Yann Ylavic wrote: > > On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote: > > > > So we probably should keep encoding r->filename with ProxyPass, and > > come back to my previous patch which skipped it only for SetHandler? > > Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType > > GENERIC" we don't send the "proxy:scheme://host" part and > > SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? > > So I did this in r1919629. > > > > > But it's going to be an endless issue if we can't fix or align > > ProxyPass and SetHandler because of workarounds there, we have to > > remain bug compatible.. > > For this how about this attached patch? > With it I can get the correct env vars (I think), and since we'd not > send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag > "apache_was_there" and work straight with the raw vars? I'm probably > having a sweet dream :) > Just in case.. PS: the script needs to exist in the DOCUMENT_ROOT for this to work, but that's how php-fpm works I suppose.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Fri, Aug 2, 2024 at 12:18 AM Yann Ylavic wrote:
>
> So we probably should keep encoding r->filename with ProxyPass, and
> come back to my previous patch which skipped it only for SetHandler?
> Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType
> GENERIC" we don't send the "proxy:scheme://host" part and
> SCRIPT_NAME/FILENAME are supposed to be the real decoded paths?
So I did this in r1919629.
>
> But it's going to be an endless issue if we can't fix or align
> ProxyPass and SetHandler because of workarounds there, we have to
> remain bug compatible..
For this how about this attached patch?
With it I can get the correct env vars (I think), and since we'd not
send a "proxy:" SCRIPT_FILENAME anymore, php-fpm would not flag
"apache_was_there" and work straight with the raw vars? I'm probably
having a sweet dream :)
Just in case..
Index: modules/proxy/mod_proxy_fcgi.c
===
--- modules/proxy/mod_proxy_fcgi.c (revision 1919623)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -29,6 +29,7 @@ typedef struct {
typedef struct {
int need_dirwalk;
+char *filename;
} fcgi_req_config_t;
/* We will assume FPM, but still differentiate */
@@ -119,8 +142,10 @@ static int proxy_fcgi_canon(request_rec *r, char *
rconf = apr_pcalloc(r->pool, sizeof(fcgi_req_config_t));
ap_set_module_config(r->request_config, &proxy_fcgi_module, rconf);
}
+rconf->filename = apr_pstrcat(r->pool, "/", url, NULL);
-if (NULL != (pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo"))) {
+pathinfo_type = apr_table_get(r->subprocess_env, "proxy-fcgi-pathinfo");
+if (pathinfo_type) {
/* It has to be on disk for this to work */
if (!strcasecmp(pathinfo_type, "full")) {
rconf->need_dirwalk = 1;
@@ -159,6 +184,9 @@ static int proxy_fcgi_canon(request_rec *r, char *
"set r->path_info to %s", r->path_info);
}
}
+else if (FCGI_MAY_BE_FPM(dconf) && !from_handler) {
+rconf->need_dirwalk = 1;
+}
return OK;
}
@@ -337,12 +365,17 @@ static apr_status_t send_environment(proxy_conn_re
int next_elem, starting_elem;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module);
+char *saved_filename = r->filename;
-if (rconf && rconf->need_dirwalk) {
-char *saved_filename = r->filename;
-r->filename = r->uri;
-ap_directory_walk(r);
-r->filename = saved_filename;
+if (rconf) {
+if (rconf->filename) {
+r->filename = rconf->filename;
+}
+if (rconf->need_dirwalk) {
+r->proxyreq = PROXYREQ_NONE;
+ap_core_translate(r);
+ap_directory_walk(r);
+}
}
/* Strip proxy: prefixes */
@@ -379,6 +412,9 @@ static apr_status_t send_environment(proxy_conn_re
ap_add_common_vars(r);
ap_add_cgi_vars(r);
+r->filename = saved_filename;
+r->proxyreq = PROXYREQ_REVERSE;
+
/* XXX are there any FastCGI specific env vars we need to send? */
/* Give admins final option to fine-tune env vars */
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 9:53 PM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 2:47 PM Yann Ylavic wrote: > > > > On Thu, Aug 1, 2024 at 7:57 PM Eric Covener wrote: > > > > > > On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > > > > > > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > > > > > > > But does it leave the splitting problem with decoded %3F? > > > > > > > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > > > > does never contain the query-string in the first place, so any '?' in > > > > there (hence in SCRIPT_FILENAME) is part of the actual file path > > > > (which we'd encode for proxying any other scheme than fcgi). And the > > > > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > > > > decoded uri-path they have to be consistent and consider that > > > > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > > > > in ... QUERY_STRING). > > > > > > Just to recap, FPM doesn't want to find the query it in > > > SCRIPT_FILENAME, it wants to toss it away because it used to > > > accidentally end up in there (via mod_rewrite?) But this is where the > > > mismatch between what we've walked/mapped/authorized and what will be > > > executed is. > > > > If FPM wants a decoded SCRIPT_FILENAME but no '?' character? > > Decoding a path with %3f will inevitably give '?', even though it's > > still the path, why would FPM decode it as a URL and find a query > > string in there? > > I think as a workaround for what we can (or used to?) send: > https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1043 > It also means an actual file with a literal question mark cannot be > run through php-fpm. OK, so php-fpm will parse the given "proxy:" SCRIPT_FILENAME as an URL, trimming the query-string, and determine that "apache_was_there". So there's never been a way to pass a decoded path with '?' to php-fpm using SetHandler (which before the latest changes never re-encoded the filename). So we probably can just forbid '?' like controls. But reading that code (wow..), php-fpm is also able to differentiate ProxyPass vs Sethandler and do things like: https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1172 which %-decode the PATH_INFO, supposedly extracted from SCRIPT_FILENAME. So we probably should keep encoding r->filename with ProxyPass, and come back to my previous patch which skipped it only for SetHandler? Possibly FCGI_MAY_BE_FPM() only too because for "ProxyFCGIBackendType GENERIC" we don't send the "proxy:scheme://host" part and SCRIPT_NAME/FILENAME are supposed to be the real decoded paths? But it's going to be an endless issue if we can't fix or align ProxyPass and SetHandler because of workarounds there, we have to remain bug compatible.. At some point we'll have to coordinate with them to remove that "apache_was_there"..
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 2:47 PM Yann Ylavic wrote: > > On Thu, Aug 1, 2024 at 7:57 PM Eric Covener wrote: > > > > On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > > > > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > > > > > But does it leave the splitting problem with decoded %3F? > > > > > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > > > does never contain the query-string in the first place, so any '?' in > > > there (hence in SCRIPT_FILENAME) is part of the actual file path > > > (which we'd encode for proxying any other scheme than fcgi). And the > > > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > > > decoded uri-path they have to be consistent and consider that > > > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > > > in ... QUERY_STRING). > > > > Just to recap, FPM doesn't want to find the query it in > > SCRIPT_FILENAME, it wants to toss it away because it used to > > accidentally end up in there (via mod_rewrite?) But this is where the > > mismatch between what we've walked/mapped/authorized and what will be > > executed is. > > If FPM wants a decoded SCRIPT_FILENAME but no '?' character? > Decoding a path with %3f will inevitably give '?', even though it's > still the path, why would FPM decode it as a URL and find a query > string in there? I think as a workaround for what we can (or used to?) send: https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1043 It also means an actual file with a literal question mark cannot be run through php-fpm. -- Eric Covener cove...@gmail.com
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 7:57 PM Eric Covener wrote: > > On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > > > But does it leave the splitting problem with decoded %3F? > > > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > > does never contain the query-string in the first place, so any '?' in > > there (hence in SCRIPT_FILENAME) is part of the actual file path > > (which we'd encode for proxying any other scheme than fcgi). And the > > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > > decoded uri-path they have to be consistent and consider that > > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > > in ... QUERY_STRING). > > Just to recap, FPM doesn't want to find the query it in > SCRIPT_FILENAME, it wants to toss it away because it used to > accidentally end up in there (via mod_rewrite?) But this is where the > mismatch between what we've walked/mapped/authorized and what will be > executed is. If FPM wants a decoded SCRIPT_FILENAME but no '?' character? Decoding a path with %3f will inevitably give '?', even though it's still the path, why would FPM decode it as a URL and find a query string in there?
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 1:37 PM Yann Ylavic wrote: > > On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > > > But does it leave the splitting problem with decoded %3F? > > Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename > does never contain the query-string in the first place, so any '?' in > there (hence in SCRIPT_FILENAME) is part of the actual file path > (which we'd encode for proxying any other scheme than fcgi). And the > '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the > decoded uri-path they have to be consistent and consider that > SCRIPT_FILENAME is nothing else than a path (no query-string, which is > in ... QUERY_STRING). Just to recap, FPM doesn't want to find the query it in SCRIPT_FILENAME, it wants to toss it away because it used to accidentally end up in there (via mod_rewrite?) But this is where the mismatch between what we've walked/mapped/authorized and what will be executed is. What about, for now, just failing it here as if it were a ctl? If more users come out of the woodwork, at least we will have some concrete examples of how it can currently end up this way w/o malicious input.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 5:51 PM Eric Covener wrote: > > But does it leave the splitting problem with decoded %3F? Yeah but I'm not sure that it's _our_ problem, a "proxy:" r->filename does never contain the query-string in the first place, so any '?' in there (hence in SCRIPT_FILENAME) is part of the actual file path (which we'd encode for proxying any other scheme than fcgi). And the '?' will be in SCRIPT_NAME/PATH_INFO/etc too. If the scripts want the decoded uri-path they have to be consistent and consider that SCRIPT_FILENAME is nothing else than a path (no query-string, which is in ... QUERY_STRING). There is the "proxy-noquery" ->notes which seems to influence whether SCRIPT_FILENAME should include the query-string or not, but that's probably for the "proxy-nocanon" case which uses r->unparsed_uri. And at that "nocanon" point I also think that the users have to keep the pieces.. There is also send_environment() which will strip any query-string (supposedly) from r->filename if it matches r->args (because "Query string in environment only", meaning QUERY_STRING I guess). That's guarded by !FCGI_MAY_BE_FPM though (i.e. never), because FPM seems to have some expectations/workarounds around SCRIPT_FILENAME to address ... who knows. So in this whack-a-mole game I'm afraid we'll have to have an opt-out for every single thing we are supposed to fix :/
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Thu, Aug 1, 2024 at 10:58 AM Yann Ylavic wrote: > > On Wed, Jul 31, 2024 at 7:34 PM Eric Covener wrote: > > > > On Tue, Jul 23, 2024 at 5:35 AM Yann Ylavic wrote: > > > > > > On Wed, Jul 17, 2024 at 6:22 PM wrote: > > > > > > > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > > > > > > > --- Comment #6 from Yann Ylavic --- > > > > Created attachment 39817 > > > > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > > > > Proxy FCGI nocanon from SetHandler > > > > > > I'm not sure how we should proceed here, this patch would avoid > > > encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not > > > "ProxyPass fcgi:..." which looks awkward. SetHandler is the > > > recommended/most used way to proxy fcgi which is probably why people > > > start noticing now that we do the same as with ProxyPass, but I don't > > > see why they should be different in this regard.. > > > > > > If SCRIPT_FILENAME should be decoded (per the spec) I wonder if > > > proxy_fcgi_canon() should not encode at all, or maybe only when > > > FCGI_MAY_BE_FPM() (so to have an opt-out)? > > > > > And like in the above patch forbid controls still but not space/tab, WDYT? > > > > Based on the bug and the japanese path, maybe set the bar even lower > > and just ratchet it all the way back to the character we know is > > problematic? > > So I went with r1919620 which makes the most sense for me. > This will forbid control characters (hence allow for space only, not > tab anymore) but mainly will *not* re-encode r->filename, irrespective > of ProxyPass vs SetHandler (I think that if users want the encoding we > should have an opt with ProxyFCGIBackendType, ProxyPass vs SetHandler > makes no sense here..). WDYT? Makes sense, even the japanese example in the bug is UTF-8. But does it leave the splitting problem with decoded %3F? -- Eric Covener cove...@gmail.com
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Wed, Jul 31, 2024 at 7:34 PM Eric Covener wrote: > > On Tue, Jul 23, 2024 at 5:35 AM Yann Ylavic wrote: > > > > On Wed, Jul 17, 2024 at 6:22 PM wrote: > > > > > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > > > > > --- Comment #6 from Yann Ylavic --- > > > Created attachment 39817 > > > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > > > Proxy FCGI nocanon from SetHandler > > > > I'm not sure how we should proceed here, this patch would avoid > > encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not > > "ProxyPass fcgi:..." which looks awkward. SetHandler is the > > recommended/most used way to proxy fcgi which is probably why people > > start noticing now that we do the same as with ProxyPass, but I don't > > see why they should be different in this regard.. > > > > If SCRIPT_FILENAME should be decoded (per the spec) I wonder if > > proxy_fcgi_canon() should not encode at all, or maybe only when > > FCGI_MAY_BE_FPM() (so to have an opt-out)? > > > And like in the above patch forbid controls still but not space/tab, WDYT? > > Based on the bug and the japanese path, maybe set the bar even lower > and just ratchet it all the way back to the character we know is > problematic? So I went with r1919620 which makes the most sense for me. This will forbid control characters (hence allow for space only, not tab anymore) but mainly will *not* re-encode r->filename, irrespective of ProxyPass vs SetHandler (I think that if users want the encoding we should have an opt with ProxyFCGIBackendType, ProxyPass vs SetHandler makes no sense here..). WDYT? Regards; Yann.
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Tue, Jul 23, 2024 at 5:35 AM Yann Ylavic wrote: > > On Wed, Jul 17, 2024 at 6:22 PM wrote: > > > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > > > --- Comment #6 from Yann Ylavic --- > > Created attachment 39817 > > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > > Proxy FCGI nocanon from SetHandler > > I'm not sure how we should proceed here, this patch would avoid > encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not > "ProxyPass fcgi:..." which looks awkward. SetHandler is the > recommended/most used way to proxy fcgi which is probably why people > start noticing now that we do the same as with ProxyPass, but I don't > see why they should be different in this regard.. > > If SCRIPT_FILENAME should be decoded (per the spec) I wonder if > proxy_fcgi_canon() should not encode at all, or maybe only when > FCGI_MAY_BE_FPM() (so to have an opt-out)? > And like in the above patch forbid controls still but not space/tab, WDYT? Based on the bug and the japanese path, maybe set the bar even lower and just ratchet it all the way back to the character we know is problematic?
Re: [Bug 69203] proxy error from php-fpm backend for paths containing spaces
On Wed, Jul 17, 2024 at 6:22 PM wrote: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=69203 > > --- Comment #6 from Yann Ylavic --- > Created attachment 39817 > --> https://bz.apache.org/bugzilla/attachment.cgi?id=39817&action=edit > Proxy FCGI nocanon from SetHandler I'm not sure how we should proceed here, this patch would avoid encoding SCRIPT_FILENAME for "SetHandler proxy:fcgi:..." but not "ProxyPass fcgi:..." which looks awkward. SetHandler is the recommended/most used way to proxy fcgi which is probably why people start noticing now that we do the same as with ProxyPass, but I don't see why they should be different in this regard.. If SCRIPT_FILENAME should be decoded (per the spec) I wonder if proxy_fcgi_canon() should not encode at all, or maybe only when FCGI_MAY_BE_FPM() (so to have an opt-out)? And like in the above patch forbid controls still but not space/tab, WDYT? Regards; Yann.
