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

Removing magic file handling from responses #11921

Closed
1 of 3 tasks
ndm2 opened this issue Apr 12, 2018 · 4 comments
Closed
1 of 3 tasks

Removing magic file handling from responses #11921

ndm2 opened this issue Apr 12, 2018 · 4 comments
Assignees
Milestone

Comments

@ndm2
Copy link
Contributor

ndm2 commented Apr 12, 2018

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 4.0.0

I've seen this over and over again, people try to send files and accidentally pass non-absolute paths (filenames only, relative paths, ...) to Response::file()/withFile(), thus having CakePHP do it's "magic", trying to look up the file in APP, which by default is the src folder.

Even if auto-resolving non-absolute paths would be deemed useful, who would actually want to serve files from the src folder? I would like to suggest to remove this behavior in 4.0, completely that is, ie. make withFile() require an absolute path, and fail hard for anything else.

Even though the current behavior is documented in the API docs, it's IMHO an unnecessary boobytrap, and people seem to step in to it all the time.

@saeideng saeideng added this to the 4.0.0 milestone Apr 12, 2018
@saeideng
Copy link
Member

saeideng commented Apr 12, 2018

which by default is the src folder.

yes, cake uses APP constant

cakephp/src/Http/Response.php

Lines 2397 to 2414 in 40ec1f4

protected function validateFile($path)
{
if (strpos($path, '../') !== false || strpos($path, '..\\') !== false) {
throw new NotFoundException(__d('cake', 'The requested file contains `..` and will not be read.'));
}
if (!is_file($path)) {
$path = APP . $path;
}
$file = new File($path);
if (!$file->exists() || !$file->readable()) {
if (Configure::read('debug')) {
throw new NotFoundException(sprintf('The requested file %s was not found or not readable', $path));
}
throw new NotFoundException(__d('cake', 'The requested file was not found'));
}
return $file;

@ADmad
Copy link
Member

ADmad commented Apr 12, 2018

make withFile() require an absolute path, and fail hard for anything else.

👍

@markstory
Copy link
Member

We should add a deprecation warning for this scenario now.

@markstory markstory self-assigned this Apr 12, 2018
markstory added a commit that referenced this issue Apr 14, 2018
Files being used with Response::withFile() should be absolute. Prefixing
with the application path is often seen as unwanted magic.

Refs #11921
@markstory
Copy link
Member

Closing as the deprecation has been added.

@saeideng saeideng modified the milestones: 4.0.0, 3.6.0 Apr 14, 2018
markstory added a commit that referenced this issue Apr 14, 2018
Relative paths assume that cwd is 'safe'. Instead we should require
people to be more explicit in their paths.

Refs #11921
Refs #11926
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

4 participants