Security harden stream wrappers by defaulting them as remote

+++ 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.

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.

+++ 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.

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.