Split off from #938614: Downgrade required PHP version to 5.2.4.

In that issue we raised the PHP requirement from 5.2.0 to 5.2.4 so that we can ensure that remote stream wrappers can't be used as a PHP include when the allow_url_include php.ini setting is FALSE.

But currently, new stream wrappers defined by modules default as local stream wrappers, unless the module explicitly adds STREAM_WRAPPERS_REMOTE to the 'type' within hook_stream_wrappers().

This patch changes the default to be REMOTE, so that modules have to explicitly opt-in to STREAM_WRAPPERS_LOCAL in order to have their streams be more trusted.

This patch copies the relevant hunks from #938614-59: Downgrade required PHP version to 5.2.4, and adds a new hunk for documentation in hook_stream_wrappers().

Files: 
CommentFileSizeAuthor
#16 drupal-942690-16.patch1.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 35,617 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#9 drupal.stream_wrappers_default_remote.9.patch9.73 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,373 pass(es).
[ View ]
#6 drupal.stream_wrappers_default_remote.6.patch9.48 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,380 pass(es).
[ View ]
drupal.stream_wrappers_default_remote.patch6.46 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,278 pass(es).
[ View ]

Comments

+++ modules/system/system.api.php 15 Oct 2010 15:33:00 -0000
@@ -2310,18 +2315,35 @@ function hook_stream_wrappers() {
+    'youtube' => array(
+      'name' => t('YouTube video'),
+      'class' => 'MyModuleYouTubeStreamWrapper',
+      'description' => t('Video streamed from YouTube.'),
+      // A module implementing YouTube integration may decide to support using
+      // the YouTube API for uploading video, but here, we assume that this
+      // particular module only supports playing YouTube video.
+      'type' => STREAM_WRAPPERS_READ_VISIBLE,
+    ),

Something I don't like in general is that we have separate bits for STEAM_WRAPPERS_LOCAL and STREAM_WRAPPERS_REMOTE. So that means something can be both or neither. Does that make any sense? For example, in the above "youtube" example, neither is specified, since we're saying we want "remote" to be the default. But if something then calls file_get_stream_wrappers(STREAM_WRAPPERS_REMOTE), the "youtube" stream wrapper won't be returned. This is a pre-existing condition of HEAD, so I'm not sure to what extent this patch needs to solve this, but it seems like a WTF to me.

To clarify: I have some concerns that we could improve the DX, and a little concern about code readability, but probably chx is right that this is the only secure approach at this point for the stream wrappers.

Good point about the bit mask - makes more sense for a single position to be 0 or 1 for local versus remote.

@effulgentsia: Obviously, in the case for the youtube stream, STREAM_WRAPPERS_REMOTE would probably be better to be explicitly defined as best practice for your concern. For that matter, considering that WTF, what would be the disadvantages of simply requiring an explicit definition, rather than a default? Anyone implementing stream wrappers for Drupal really should be reading the friendly manual at this early stage of development, anyway.

Component:base system» file system

marking the component.

StatusFileSize
new9.48 KB
PASSED: [[SimpleTest]]: [MySQL] 26,380 pass(es).
[ View ]

Discussed more with chx in IRC. Here's a patch that completely removes the STREAM_WRAPPERS_REMOTE constant. Given that whether something is local or remote has security implications, let's head off the problems that would stem from people using both the STREAM_WRAPPERS_REMOTE and STREAM_WRAPPERS_LOCAL constants, in potentially inconsistent ways. Removing the constant also has the benefit of triggering PHP notices in all modules using that constant, which is a good thing, since given the API change inherent in this patch, we want those module developers evaluating their use of that constant. BC breaks suck, but security holes suck more.

aaron i find your faith in developers amusing.

Visual review of the patch looks good.

StatusFileSize
new9.73 KB
PASSED: [[SimpleTest]]: [MySQL] 26,373 pass(es).
[ View ]

+++ includes/file.inc 15 Oct 2010 20:37:21 -0000
@@ -90,12 +90,31 @@ define('FILE_STATUS_PERMANENT', 1);
+ * Examples:
+ * @code
+ *   // Retrieve information about all stream wrappers defined by
+ *   // hook_stream_wrappers() implementations.
+ *   $all_stream_wrappers = file_get_stream_wrappers();
+ *
+ *   // Retrieve information about all stream wrappers that have both
+ *   // STREAM_WRAPPERS_READ and STREAM_WRAPPERS_VISIBLE bits set.
+ *   $readable_and_visible_stream_wrappers = file_get_stream_wrappers(STREAM_WRAPPERS_READ | STREAM_WRAPPERS_VISIBLE);
+ *
+ *   // Retrieve information about all stream wrappers that are visible, but do
+ *   // not use local file storage.
+ *   $remote_stream_wrappers = array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_VISIBLE), file_get_stream_wrappers(STEAM_WRAPPERS_LOCAL));
+ * @endcode

In IRC, chx suggested reworking this documentation to not have inline code comments inside a PHP docblock, so this patch does that.

+++ includes/stream_wrappers.inc 15 Oct 2010 20:37:21 -0000
@@ -65,9 +63,9 @@ define('STREAM_WRAPPERS_VISIBLE', 0x0010
/**
- * Stream wrapper type flag -- visible, readable and writeable.
+ * Stream wrapper type flag -- hidden, readable and writeable using local files.
  */
-define('STREAM_WRAPPERS_WRITE_VISIBLE', STREAM_WRAPPERS_READ | STREAM_WRAPPERS_WRITE | STREAM_WRAPPERS_VISIBLE);
+define('STREAM_WRAPPERS_LOCAL_HIDDEN', STREAM_WRAPPERS_LOCAL | STREAM_WRAPPERS_HIDDEN);
/**
  * Stream wrapper type flag -- visible and read-only.
@@ -75,9 +73,23 @@ define('STREAM_WRAPPERS_WRITE_VISIBLE',
@@ -75,9 +73,23 @@ define('STREAM_WRAPPERS_WRITE_VISIBLE',
define('STREAM_WRAPPERS_READ_VISIBLE', STREAM_WRAPPERS_READ | STREAM_WRAPPERS_VISIBLE);
/**
+ * Stream wrapper type flag -- visible, readable and writeable.
+ */
+define('STREAM_WRAPPERS_WRITE_VISIBLE', STREAM_WRAPPERS_READ | STREAM_WRAPPERS_WRITE | STREAM_WRAPPERS_VISIBLE);
+

#6 makes it look like STREAM_WRAPPERS_WRITE_VISIBLE is being changed, but it's not, so this patch cleans that up.

Powered by Dreditor.

Status:Needs review» Reviewed & tested by the community

This is originally chx's patch, so he's not RTBC'ing it. But, he's approved it, pwolanin has approved it (#2, #3, #8), and in IRC, heyrocker didn't object to it, so I think it's worth RTBC'ing and putting in front of Dries and webchick.

i like this approach. thanks for the work!

Status:Reviewed & tested by the community» Needs work

webchick asks for a test. Write a test wrapper (or just steal it from http://drupal.org/files/issues/928104-1-implement-dswi.patch :D ) register it without a type, ini_set allow_url_fopen to false if it's not false yet (do not do anything if it's false, it's neither necessary nor feasible) try to open a test:// file and then detect the error you are rewarded with. Simpletest tests catch errors so it must be possible (grep banana modules/simpletest/tests/error.test).

Status:Needs work» Fixed

Committed to CVS HEAD. Thanks.

Priority:Critical» Normal
Status:Fixed» Needs work

Well, the test is still necessary

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Okay, let's write a test!

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 35,617 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Well, I didn't get too far. I found it very hard to write a test for code that already works.
Interesting note, the following patch gave the following error: stream_wrapper_unregister() [function.stream-wrapper-unregister]: Unable to unregister protocol dummy://

(That was me trying to recreate the code before the fix.)

Issue tags:+Needs tests

Neat!

Status:Needs work» Needs review

Status:Needs review» Needs work

Whoops, I just realized that you reverted back from this patch. That was confusing.

Yeah, I wasn't kidding when I said I didn't get very far :)