Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FPM: Always use script name in PHP_SELF if cgi.discard_path = 1 and cgi.fix_pathinfo = 0 #11025

Open
bukka opened this issue Apr 6, 2023 · 2 comments
Assignees

Comments

@bukka
Copy link
Member

bukka commented Apr 6, 2023

Description

Currently if PATH_INFO set, then it is used in PHP_SELF if cgi.fix_pathinfo is disabled. This makes sense if the executable is used from PATH_TRANSLATED which is default behaviour. However if cgi.discard_path is enabled, it uses SCRIPT_FILENAME executable instead. This however does not match the documentation which states that PHP_SELF is relative path to the executable because PATH_INFO has nothing to do with executable path.

This was first mentioned in https://bugs.php.net/bug.php?id=68053 and this issue is an extraction of that problem (the primary concern won't be fixed due to a significant BC break). Even though this could be considered as a bug, it is not that critical and it has some BC break concern. It means it should be treated more as a feature change to improve the current behaviour and should go only to master and be mentioned in UPGRADING.

@bukka
Copy link
Member Author

bukka commented Apr 6, 2023

Just comment for myself. The fix should be pretty much just

diff --git a/sapi/fpm/fpm/fpm_main.c b/sapi/fpm/fpm/fpm_main.c
index 64ef27dade..61c24b7569 100644
--- a/sapi/fpm/fpm/fpm_main.c
+++ b/sapi/fpm/fpm/fpm_main.c
@@ -1310,7 +1310,7 @@ static void init_request_info(void)
                        }
                } else {
                        /* pre 4.3 behaviour, shouldn't be used but provides BC */
-                       if (env_path_info) {
+                       if (!CGIG(discard_path) && env_path_info) {
                                SG(request_info).request_uri = env_path_info;
                        } else {
                                SG(request_info).request_uri = env_script_name;

and some changes in the tests. I'm still working on better coverage so will address it once all the tests are added.

@bukka bukka self-assigned this Apr 7, 2023
@danog
Copy link
Contributor

danog commented Nov 21, 2023

@bukka I feel this is a more general issue, as PHP_SELF shouldn't contain the PATH_INFO in any case, regardless of the value of fix_pathinfo, but it is specifically appended if fix_pathinfo is true: https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants