I note that although PHP considers a filepath like '/home/somebody/junk' or 'sites/default/files/junk' to be a correct argument/streamwrapper for all file operations, the code in file_unmanaged_copy() thinks this is wrong. But unless we want to be way different from PHP's intent, it's not wrong.

Also, this code emits an error message but then doesn't return false to indicate an error.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Makes sense to me, although this definitely needs sign-off from drewish, pwolanin, or quicksketch. In the meantime in A $destnation without... it should be "destination". :)

aaron’s picture

i believe that we do not expose the standard php file stream in drupal, instead implementing (by default) public:// and private:// to enforce a standard folder path that will be available to modules regardless of the site configuration. if someone wanted to write outside of those folders, the best bet would be to implement a new stream wrapper.

good catch on the return value for that section though; appears to be an oversight.

aaron’s picture

Category: bug » feature

unless one of the other maintainers believes otherwise, i think that this would be an api change, as it could open a can of worms to allow direct access to the php file server.

pwolanin’s picture

Status: Needs review » Needs work

Right - we blocked this on purpose, and I agree that this would be an API change and possible security issue. What's the use case?

drewish’s picture

Status: Needs work » Needs review

hum... i don't specifically remember this decision. i can see both sides of it it seems like you should be able to copy some file from /tmp/import/foo to public://mymodule/foo but i can see some merit to only allow streams because if you want non-stream wrappers you could always just call copy()...

drewish’s picture

Category: feature » bug

putting this back to but because of the missing return. i think that needs to be fixed in any case.

aaron’s picture

Category: bug » feature

i agree (that the missing return needs to be fixed), but that's a separate (and easier) issue.

aaron’s picture

Category: feature » bug
rfay’s picture

Title: A filepath is a valid stream; should be accepted in filehandling » Invalid scheme fails to halt processing of a stream request

Thanks for your weigh-in. I opened #876114: Normal stream schemes are not supported in Drupal. Why? as a spin-off, and would appreciate you all making a final decision there.

Changing this issue to focus only on the failure to handle the error correctly.

rfay’s picture

pwolanin points out in IRC that general use of streams has been a security issue. Without this patch, one certainly can access file:///etc/passwd without any trouble.

pwolanin’s picture

I think the real bug fix is just this.

rfay’s picture

This one has an slightly improved comment: Assert an *allowed scheme*.

pwolanin’s picture

well, not allowed is not valid...

In any case - should be RTBC if the tests are happy (though perhaps worrying that we have no test coverage for this).

Status: Needs review » Needs work

The last submitted patch, drupal.invalid_scheme_874326_12.patch, failed testing.

pwolanin’s picture

hmm, looks like something was expecting to be able to use a bare path?

I fear there is still substantial internal inconsistency in the file handling.

rfay’s picture

So as we fix this, one question: Shouldn't we have in each of these failure situations: watchdog() for the admin (to fix their site) as well as drupal_set_message() (to notify the user their action failed)

pwolanin’s picture

Yes, it would seem so, but we need to inspect the code to see where the problems are.

rfay’s picture

Priority: Normal » Critical
Issue tags: +Security

Shucks, I think this is probably a security issue, since the intent of the system is not to allow direct file access (or access via other schemes). Currently, access to /etc/passwd, file:///etc/passwd, ftp://ftp@example.com, and lots of other things is fully allowed.

rfay’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Here is a patch for *one* of the #fails I chased down on the plane. It will fail with many more.

Status: Needs review » Needs work

The last submitted patch, drupal.invalid_scheme_874326_19.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
9.36 KB

This one might get them.

This also adds watchdog() calls (for the administrator) and outputs the actual path, instead of the streamwrapper.
I changed the drupal_set_message() calls to *not* output the destination path, as that could be a security issue, disclosing the filesystem structure.

Damien Tournoud’s picture

For the love of Drupal, could we remove those drupal_set_message() and properly use trigger_error() here?

rfay’s picture

Well, I like the idea of trigger_error(), but have quite a number of problems with it.

First, here's the code I used:

  if (!$destination_scheme || !file_stream_wrapper_valid_scheme($destination_scheme)) {
    trigger_error(t('File %file could not be copied, because the destination %destination is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.', array('%file' => $original_source, '%destination' => drupal_realpath($destination))), E_USER_WARNING);
    return FALSE;
  }

Second, here is the output to the screen:

User warning: File <em class="placeholder">temporary://file4dkBgA</em> could not be copied, because the destination <em class="placeholder"></em> is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper. in file_unmanaged_copy() (line 660 of /home/rfay/workspace/d7git/includes/file.inc).

The watchdog entry is identical, except that its type is 'php' instead of 'file':

So my questions are:

  • When did trigger_error() become best practice and where can I find out more about that?
  • Note that placeholders are not handled correctly (or at least % placeholders)
  • Using this technique exposes the file structure of the server to an end user. Are we OK with that?
  • The watchdog() shows up as a 'user warning' of type 'php', instead of type 'file', which is what we would prefer, I'd think
  • [edit] The watchdog() created this way is untranslatable; Watchdogs are supposed to be translated at the last moment.
rfay’s picture

Chatted with DamZ about #23 in IRC. It seems that without adding a new drupal_trigger_error() we can't resolve the issues there.

IMO, given the nature of the way we're trying to finesse user and watchdog reporting, the existing patch in #21 might be the best approach for now.

rfay’s picture

rfay’s picture

Assigned: Unassigned » rfay
rfay’s picture

Assigned: rfay » Unassigned
Status: Needs review » Needs work
Issue tags: +API change

Bringing over the issues in #866756: file_directory_path('private') returns FALSE by default which were:
- writing to private:// when private is misconfigured allowed writing arbitrary locations on the filesystem
- Writing a bare filepath was allowed (should be resolved already by the previous patches in this issue)

Talking with pwolanin at CPH sprint. Here's our proposal:

1. Remove file_directory_path(). Yes, it's an API change. But leaving it in can only make broken contrib.
2. Add file_get_default_uri() to replace the legitimate part of file_directory_path. This would return the URL of the default scheme, as in public://.
3. file_directory_temp() -> _file_directory_temp(). Change comments to make explicitly private. (API Change also)
4. Add tests to
- Make sure that a bare filepath can't be used to read or write or prepare_directory.
- Make sure that a misconfigured private doesn't result in ability to write arbitrary path.

pwolanin’s picture

After discussion with quicksketch, we will not remove file_directory_path(), but we shoudl make sure that any public functions generally require URIs.

rfay’s picture

Assigned: Unassigned » rfay

So we decided that removing file_directory_path() just disrupted too many things, would break too much contrib. Instead we'll refocus on the actual intent: Modules must use URIs, not filepaths, with the Drupal File API.. Therefore, verify that the key API functions (*_copy, *_move, etc.) throw an exception if they are used with a bare filepath. Or at least that they fail.

So instead the plan is:

1. Make sure the primary file API functions fail with a bare (non-URI) filepath.
2. Write tests that:
- Make sure that a bare filepath can't be used to read or write or prepare_directory.
- Make sure that a misconfigured private doesn't result in ability to write arbitrary path.

pwolanin will work on the changes; rfay will work on the new tests.

rfay’s picture

There are some pretty significant problems with the whole decision path we're on here.

1. There is lots of code that uses barepaths, both in core and contrib. One of the nastiest is in simpletest, which saves away the actual "original files" directory and then creates a new directory. How is it supposed to use "public://" for operations like that?

2. Files that are in the modules or the themes directory, for example, need to be accessible. If we disallow file api access to barepaths, then there is no clean way to do anything using file api with a file in a module directory, for example.

I'm not entirely sure that we're doing the right thing here, and perhaps we should even reconsider #876114: Normal stream schemes are not supported in Drupal. Why?.

chx’s picture

Edit: Under no circumstances we are disabling the use of bare paths this late in the cycle. Edit: so #1 is correct and #11 is a far fetched API change that absolutely should not happen. I retract what I said.

rfay’s picture

Assigned: rfay » Unassigned
aaron’s picture

1. There is lots of code that uses barepaths, both in core and contrib. One of the nastiest is in simpletest, which saves away the actual "original files" directory and then creates a new directory. How is it supposed to use "public://" for operations like that?

As I recall, didn't we create a simpletest:// stream wrapper for file tests? In conversations, we talked about creating it in the fly for that purpose, in a subfolder of the public file location.

2. Files that are in the modules or the themes directory, for example, need to be accessible. If we disallow file api access to barepaths, then there is no clean way to do anything using file api with a file in a module directory, for example.

A suggestion I'd had earlier in the process was to create a read only stream to access deeper Drupal files, such as drupal://, that would read from the Drupal root.

aaron’s picture

the conceptualization of simpletest:// (not sure on the fly if that's what we actually implemented) was that we could create simpletest:// in a subfolder of public://, then reroute the public:// variable to use that folder, so that tests would work.

rfay’s picture

@aaron: There is no simpletest://, and there is no drupal:// or site:// or anything like that. We have public://, private://, and temporary://. So we have no general way of handling files except those that are in the transient-file areas. No way to address things that are in the module or theme or library directories.

So here's what's going on: Whatever the intent, we have allowed bare paths (which are a URI that uses the default scheme, file://) for the last year. They've been allowed in core. They've been allowed in contrib. Lots of things have been updated using the assumption that this would work. The comments on many File API functions say explicitly that barepaths work. We possibly made a mistake, but IMO we're going to have to allow all bare paths in D7 in the File API.

So here is what I think our practical choices are:

Option 1: Explicitly allow filepaths and explicitly allow URIs with schemes provided by Drupal (public, private, temporary, and any schemes implemented using hook_stream_wrappers()).

Option 2: Allow all URIs. This is the basic PHP approach to filehandling. Any URI implemented can be used with the same API. That would mean that all Drupal-implemented schemes would be supported, and it also would mean that barepaths could be supported, as the default scheme in a URI is file://. This is the generic approach to this, and it's the one that is intended by the entire PHP streamwrapper approach to file and device handling.

rfay’s picture

OK, discussion with DamZ, pwolanin, and webchick at the sprint came up with this proposed approach:

1. Take the existing #21, because it cleans up some things that need cleaning.

2. Allow all URIs (any scheme) and also barepaths in file_unmanaged_* and file_prepare_directory()

3. Disallow barepaths and non-Drupal schemes in managed file APIs like file_copy(), etc.

4. Completely remove file_get_directory_path() and replace it (where required, like simpletest) with equivalent functionality. This will break some contrib and be annoying.

rfay’s picture

This one is now dependent on important spinoff: #895308: Remove file_directory_path()

rfay’s picture

Status: Needs work » Needs review
FileSize
16.52 KB

This patch continues from #21 and is all I could get done before collapsing. It's not done, but it's a start. I probably won't be able to do anything more on this for the next week.

This will collide with #895308: Remove file_directory_path() depending on which goes first, so a reroll will be required.

Status: Needs review » Needs work

The last submitted patch, drupal.file_invalid_scheme_874326_38.patch, failed testing.

philbar’s picture

dopry’s picture

-1

The file_unmanaged_* api should only accept valid schemes and no schemes should be defaulted. Developers should be intentional in their use of the Drupal fileapi functions. If using bare paths is a requirement for them they should use the native php file functions. Please see my comments in the issue you opened regarding the why for this design. You're only going to make mapping these functions to object methods more difficult if you start coding outside the interfaces now.

aaron’s picture

simpletest creates dummy:// and points it to the original public://. from what i recall, public:// is then supposed to point to a newly created subfolder so that tests can be performed without overwriting things they shouldn't be touching. however, i can't find that in a quick look. the simpletest:// i recall is being used for mimetypes, but is not a real stream wrapper. maybe someone else knows better?

rfay’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

Rerolled #38 to account for the commit of #895308: Remove file_directory_path(). Need to see all those fun #fails again.

Status: Needs review » Needs work

The last submitted patch, drupal.file_invalid_scheme_874326_43.patch, failed testing.

aaron’s picture

+ * @param $uti
...
+ function file_valid_uri($uti) {

should be $uri of course

aaron’s picture

since we're talking about an API change anyway, it would be really nice to be able to do this (assuming the three provided stream wrappers were read-only):

file_unmanaged_copy('drupal://misc/favicon.ico', 'public://favicon.ico');
file_unmanaged_copy('modules://my_module/file.jpg', 'public://file.jpg');
file_unmanaged_copy('theme://images/bg.png', 'public://theme-images/bg.png');
aaron’s picture

seems like a less-intrusive, less-extensive change anyway, now that i think about it...

rfay’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

I agree with your #46, aaron, wholeheartedly. But I think we needed to do that a long time ago. D8.

Here's #45 $uti. Wow, it was late that night. That will fix about 900 of the #fails.

If everybody and the committers want three new schemes, I'm for it. But I really think we have to live with what we did at this point.

Status: Needs review » Needs work

The last submitted patch, drupal.invalid_scheme_874326_48.patch, failed testing.

rfay’s picture

Assigned: Unassigned » rfay

I think I've about got the #fails, but it's bedtime.

rfay’s picture

Status: Needs work » Needs review
FileSize
16.08 KB

Let's see how this one does. Note the controversial else{} in file_stream_wrapper_uri_normalize().

Status: Needs review » Needs work

The last submitted patch, drupal.file_invalid_scheme_874326_51.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
17.93 KB

Arghh. Lost some work that had already been done. This fixes that and then there's a missing drupal_realpath() in this one.

rfay’s picture

Issue Summary:

Although the switch from bare paths to everything-uses-URIs had been started a year ago when the streamwrappers patch (#517814: File API Stream Wrapper Conversion) was committed, it didn't quite get done due to the bug described in the initial post on this issue. And as a result of that there were a couple of discussions that didn't get completed.

Discussion 1 was about file_directory_path() and the fact that ordinary code should never need to know it, as they use streamwrapper format like public://, private://, etc. The outcome of that discussion was to completely remove file_directory_path() and that was done (already committed and controversial) in #895308: Remove file_directory_path() (#4 over there was committed in #12).

Discussion 2 was about whether bare paths were allowed. The decision was that bare paths (like "sites/all/modules/junk/junk.txt") are not allowed in the managed file API, but *are* allowed in the unmanaged file API, because of discussion 3 below.

Discussion 3 was about what kinds of streamwrappers should be allowed in what contexts. The decision taken was that only Drupal streamwrappers (public://, private://, temporary://, and module-provided wrappers through hook_stream_wrappers()) would be allowed in the managed API, but that all streamwrappers (including PHP's various file://, ftp://, http://, etc.) would be allowed in the unmanaged API. In the PHP world, a bare path use the default scheme of file://, so bare paths are allowed in the unmanaged API as well.

This patch tries to implement discussions 2 and 3 (#1 was already committed) and it adds some tests to make sure that the decisions have been implemented.

rfay’s picture

Assigned: rfay » Unassigned

Now assigned to "reviewers"

chx’s picture

Status: Needs review » Reviewed & tested by the community

This sounds quite a good summary and decisions I can support.

chx’s picture

Here is a longer followup: baring implementing #46 which would require a real lot of time (do we need all three of drupal:// module:// theme:// or rather just misc:// module:// theme:// and then what about library:// and what if you put something else in sites/all and whether we need a site:// for sites/$currentsitename... there is no end to this) unmanaged file API should keep supporting bare paths which is the equivalent of file:// in PHP and if we support that then it's plain silly not to support any other PHP stream wrapper.

On the other hand, managed file API is a Drupal API and should keep Drupal stuff. It's mostly about user uploaded files and as such Drupal wrappers are adequate.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Great work on this, as well as the summary. I agree with the direction taken in this patch. It aligns more closely with developer expectations, and still keeps the separation between managed and unmanaged files sane.

However, this no longer applies. Can I get a re-roll?

webchick’s picture

Issue tags: +Needs documentation

Oh, ok. The .rej file is only whitespaces, as I have been informed by chx. However, it really seems like this needs sign-off from pwolanin, aaron, dopry, or drewish.

webchick’s picture

Status: Needs work » Needs review
Issue tags: -Needs documentation

Sorry, that "Needs Documentation" was when I was going to commit it, before I read the entire issue. ;)

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs documentation

Visually reviewing the patch, this is indeed in line with the direction we agreed at Drupalcon. Patch looks good, though we may want to double check the test cases that are using drupal_realpath() in a follow-up issue.

For the future we need an issue for the change described in #46 for D8 (by implication only Drupal-provided schemes are allowed), plus #908730: File API functions should throw exceptions when failures happen

chx’s picture

Removed the failing whitespace hunks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, great.

Committed to HEAD.

rfay’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

rfay’s picture

Status: Closed (fixed) » Needs review
FileSize
1.09 KB

Discovered a little typo/error that results in a (very) bad watchdog message. This watchdog call just had the wrong parameter.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Good catch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

amontero’s picture