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

Fix bug #74205 PATH_INFO only partial support #2403

Open
wants to merge 3 commits into
base: master
from

Conversation

3 participants
@remicollet
Copy link
Contributor

commented Mar 4, 2017

I tried but failed to find why this "dot" breaks redirection, so I propose to clean it.
Notice: static files are managed in php_cli_server_dispatch

As this may introduce a behavior change, I only propose this for 7.2

Open for discussion.

See https://bugs.php.net/74205

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

@nikic

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

Looks like this was introduced in 8d46756, but it's not clear why, or how it is related to that change (which seems to be about slashes).

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

indeed.
@laruence as you're the author of 8d46756 can you please have a look (yes this is quite old)

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

Hmm... I see a case

/index.php
/img/foo.png

/img/bar.png should (perhaps) return a not found error.. will try to think if a better way is possible

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

A mnimal fix, allowing dot not in final part is

diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c
index 62e1601..a3ff6e8 100644
--- a/sapi/cli/php_cli_server.c
+++ b/sapi/cli/php_cli_server.c
@@ -1382,6 +1382,9 @@ static void php_cli_server_request_translate_vpath(php_cli_server_request *reque
        }
        q = request->vpath + request->vpath_len;
        while (q > request->vpath) {
+               if (*q == '/') {
+                       break;
+               }
                if (*q-- == '.') {
                        is_static_file = 1;
                        break;

remicollet added some commits Mar 5, 2017

Fix bug #74205 Built-in server have only PATH_INFO partial support
Fix bug #74061 Built-in server assumes path with dot is a file

Add a -P (--redirect) option which allow to unconditionally redirect
all not found file to index.php, with PATH_INFO set
@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2017

After a bit more thinking on this, and lot of try I think we cannot have a safe way to known if you should redirect or not.

Another exampple /foo/index.php/bar ?

So, here is another proposal (which should be very simple)

  • without new --redirect option, current behavior is unchanged, redirect only occurs if no dot in URI
  • with new --redirect option, everything not found is redirected to index.php (and PATH_INFO is set)

This new option is designed to be a real equiv to Apache rules:

RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^ index.php [QSA,L]

See the new test for this change.

@krakjoe krakjoe added the Bugfix label Mar 6, 2017

@nikic nikic added Enhancement and removed Bugfix labels Mar 11, 2017

@nikic

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

Isn't --redirect effectively equivalent to specifying a routing script with a if (file_exists) return false; check at the top?

@remicollet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2017

@nikic not exactly, a possible router script will be (a bit ugly)

<?php
if ($_SERVER['SCRIPT_FILENAME'] !== basename(__FILE__)) {
	return false;
}
$_SERVER['PATH_INFO']       = $_SERVER['SCRIPT_NAME'];
$_SERVER['SCRIPT_FILENAME'] = __DIR__ . "/index.php";
$_SERVER['SCRIPT_NAME']     = "/index.php";
require $_SERVER['SCRIPT_FILENAME'];
return true;

BTW, the reason why I try to fix reported bug is that documentation states:

"URI requests are served from the current working directory where PHP was started, unless the -t option is used to specify an explicit document root. If a URI request does not specify a file, then either index.php or index.html in the given directory are returned. If neither file exists, the lookup for index.php and index.html will be continued in the parent directory and so on until one is found or the document root has been reached. If an index.php or index.html is found, it is returned and $_SERVER['PATH_INFO'] is set to the trailing part of the URI. Otherwise a 404 response code is returned."

Which only works if no dot in called URI.

The wording "URI request does not specify a file" is a bit confusing...

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

@remicollet bump, can I ask that you either merge or close this as appropriate please ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.