php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #52835 readfile sample allows directory download
Submitted: 2010-09-14 10:29 UTC Modified: 2010-09-15 19:58 UTC
From: kittya at breyfamily dot net Assigned: aharvey (profile)
Status: Wont fix Package: Documentation problem
PHP Version: 5.3.3 OS:
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: kittya at breyfamily dot net
New email:
PHP Version: OS:

 

 [2010-09-14 10:29 UTC] kittya at breyfamily dot net
Description:
------------
For the readfile function, sample 1, "Forcing a download using readfile()" uses file_exists to check whether $file is valid. However, if there is a directory by the name of $file, the sample code causes PHP to try to download the directory, which will likely be meaningless to the client.

Expected result:
----------------
The sample should use is_file.

Actual result:
--------------
The sample currently uses file_exists.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2010-09-14 12:25 UTC] aharvey@php.net
-Status: Open +Status: Assigned -Assigned To: +Assigned To: aharvey
 [2010-09-14 12:35 UTC] aharvey@php.net
-Status: Assigned +Status: Wont fix
 [2010-09-14 12:35 UTC] aharvey@php.net
Actually, on second thoughts, I'm going to leave this as it is. It's an obviously contrived example and is_file() isn't necessarily sufficient anyway, since it doesn't cover links and special files.

Closing won't fix.
 [2010-09-15 01:26 UTC] kittya at breyfamily dot net
The example may not be so contrived after all. I found the example when I was looking for code to do just what the example does. Before submitting the documentation problem report, I used the example almost verbatim in a project:
http://sermonsontheweb.svn.sourceforge.net/viewvc/sermonsontheweb/trunk/Web/ChurchWebSite/Sermons/Download.php?revision=199&view=markup
I noticed the problem when performing security testing.

There are more elegant solutions with rewrites, but the approach in the example is simple and very portable.

Regarding is_file, perhaps links and special files cases will be OK. If links would be followed, and the stream from special files would be read, that would be appropriate in most scenarios.
 [2010-09-15 05:27 UTC] aharvey@php.net
Don't get me wrong, it's certainly possible to make the example work with links and special files as well.

My concern is that we're then complicating an example that's specifically demonstrating the usage of readfile() with a Content-Disposition header -- down to having an explicit line at the top defining $file as something that's obviously the name of a file, not a directory. I just think any potential change would only make the intent of the example less clear than it already is.
 [2010-09-15 19:58 UTC] kittya at breyfamily dot net
I see what you mean about the hard coded file name being contrived. Still, taking the example at face value, it's saying if you put a file in some exact location on the server, this code will let you download it. Hence the example checks that the file really exists. I don't think it muddies the intent for the example to also check that what lives at the location is really a file.

Since it doesn't add any complexity to the example, I would get as close as you can to demonstrating best practice.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Dec 27 03:01:28 2024 UTC