-
Notifications
You must be signed in to change notification settings - Fork 392
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
Serving static files when using the built-in webserver. #4
Conversation
From PHP's documentation: > If a PHP file is given on the command line when the web server is started it is treated as a "router" script. The script is run at the start of each HTTP request. If this script returns FALSE, then the requested resource is returned as-is. Otherwise the script's output is returned to the browser. In order to serve the static files from webroot directory it is necessary to `return false` on the router script and let the webserver serve it.
@@ -21,6 +21,11 @@ | |||
// for built-in server | |||
if (php_sapi_name() === 'cli-server') { | |||
$_SERVER['PHP_SELF'] = '/' . basename(__FILE__); | |||
|
|||
$filename = __DIR__ . '/' . $_SERVER['REQUEST_URI']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't prevent local file inclusion or directory traversal. We should fail to handle any requests that include ..
or the %2c
in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make that %2e
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would not be a problem since you are not supposed to use it on production, where this kind of attack usually happens.
I ended up doing something similar to what AssetDispatcher
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supposed to, but some uneducated person somewhere will do it. And we'll be the person they blame.
You branch is called 3.0-webserver, but the merge target is |
The app folder is also "3.0-app", so most likely the target of the PR was left on default - which is master - by accident. |
The |
|
||
$url = urldecode($_SERVER['REQUEST_URI']); | ||
$file = __DIR__ . $url; | ||
if (strpos($url, '..') === false && strpos($url, '.') !== false && file_exists($file) && is_file($file)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't is_file() include file_exists() already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, will fix it up.
@markstory You made me wonder. How will be the versioning on this repository? Starting with |
It will start on 3.0 :) |
Ignore my previous comments about branches + versions, I'm a tool. I totally didn't notice that this was the cakephp/app repo. Sorry for the confusion. |
Any more thoughts on this? |
Serving static files when using the built-in webserver.
When using built-in webserver static files are not served, instead exceptions are thrown:
The current
AssetDispatcher
only tries to serve themed and plugins' assets.From PHP's documentation:
In order to serve the static files from webroot directory it is necessary to
return false
on the router script and let the webserver serve it.