CVS Account Request: http://drupal.org/node/994286 (status RTBC)
Sandbox: http://drupal.org/sandbox/lava/1074760
Description:

I am applying for a GIT account to publish and maintain a new Drupal module written by the Electronic Frontier Foundation. I have been working with Tim Jones from the Electronic Frontier Foundation on a module called MyTube . MyTube was written in 2008 as a way of serving Flash content or other remote embeds without putting users' privacy at risk with things like Flash cookies or remote referrers . Unfortunately, the project had not been worked on since early 2008, was not very configurable, and only worked for Drupal 5 (what EFF uses). As a Drupal 6 administrator at opensource.osu.edu, who was interested in running it on our website, I picked up the project, fixed a few bugs, made some improvements, made it compatible with Drupal 6, and converted it from using nodeapi into an input filter for flexibility purposes. I then backported my changes to Drupal 5 and passed the code onto EFF . The current version is located at http://opensource.osu.edu/mytube and also in our git repository at https://opensource.cse.ohio-state.edu/git/swaneybr/mytube.git .

MyTube is an input filter (it used to use hook_nodeapi in original version) that replaces all embed, object, and iframe tags with locally-hosted thumbnails that, when clicked, run a JQuery script that swaps the thumbnail out with the original embed code. Effectively, until the user clicks play, no remote request is made to the remote site hosting the Flash (or whatever else) content, so the user cannot receive referrer headers or Flash cookies until opting in. Below each thumbnail is a (configurable) disclaimer warning users about the privacy risks in loading the content. The disclaimer includes a (configurable) link to a page about privacy concerns for Flash content, which by default goes to https://www.eff.org/deeplinks/2008/02/embedded-video-and-your-privacy and also has a link to the source of the video, with the link text being the top-level domain. This ensures visitors can know what they are clicking to load before they load it. In summary, MyTube can be thought of as a site-wide Flashblock . To alleviate the NoScript-esque inconvenience this may pose to some users, especially those who don't understand or care about the privacy implications of some video-hosting services, MyTube also automatically modifies embed code for a number of websites so it autoplays when loaded, and then locates and downloads the thumbnail for that embed, so the appearance and experience is roughly the same as if the embed code itself were pre-loaded with the page. See an example at http://opensource.osu.edu/node/299 . In this way, unlike with NoScript and the like, the user only needs to click ONCE to play the video for many video-sharing websites (I am continuing to add more as I find them). If the source is not supported, a default thumbnail is displayed instead, and all thumbnails (default or not) are overlayed with a play.png symbol. In such unsupported cases, users can also add a thumb= atttribute to the embed code to specify their own thumbnail. Wherever the thumbnail may be from, it will automatically be resized to the size of the embed, or (if so-configured) the embed resized to an administrator-set width and height. Currently, thumbnails and autoplay are supported for YouTube, Metacafe, and Vimeo, and autoplay alone is supported for a handful of other locations.

What I hope to accomplish by submitting this to drupal.org is 2 things: (1) I want MyTube to automatically find updates, especially if (god forbid) there were some security vulnerability in our code, and (2) more users will know about and feel comfortable deploying MyTube. If it's adopted on a large scale it will be highly beneficial to end users who are mostly unaware of the massive amounts of data user-profiling and marketing companies are collecting about them, but potentially also beneficial to some companies (such as security vendors) as a PR move to show they care about your privacy. While naïve visitors will not notice anything happening, the ones who know and care about online privacy will appreciate that visiting a page with a single YouTube embed will not be added to a central database somewhere. I have spoken with the technical staff at EFF about hosting it here, and Tim Jones has agreed to sign on as a co-maintainer for the project, while I plan on being the primary maintainer. MyTube is licensed under the GNU General Public License version 2 or 3 (currently a license file is included for both, but I can remove them if it's against your terms). If you would like to contact Tim, let me know; his primary e-mail address has changed since leaving EFF. If you wish to speak with someone at EFF about the project, their main contact is now Chris Contelini , but he has not contributed any code to MyTube.

I shamefully admit I am new to Drupal development, or web development in general, but to the best of my knowledge the code submitted complies with your security best practices and coding standards, and API calls are made in place of standard PHP libraries wherever possible. As far as I know, there is no other server-side protection that does what MyTube does, not just within Drupal, but anywhere. There are a couple of end-user tools in the form of add-ons for Firefox, including Flashblock and NoScript. While these are more secure for security-conscious users, and they work for all websites, MyTube will work for all** visitors of the website regardless of what security plugins they may or may not have.

For security issues, MyTube does not currently access the database in any way other than the variable_get function. The primary security concern is that it is an input filter, and should handle user input in a safe and careful manner. While the removal of unsafe or potentially unsafe content is normally handled by the HTML Filter, users must technically be allowed (either by disabling this filter or whitelisting the appropriate tags) to submit embed, object, param, or iframe tags. Therefore, I consider any instance where such tags can pass through and automatically load (in any fashion) a security vulnerability. There has been one such vulnerability in the original version, which I can describe in more detail on request, but calling the HTML Corrector before MyTube processes the code will mitigate the issue (MyTube therefore calls said filter before invoking its own methods). Relevant embed tags can be selectively allowed or disallowed, and MyTube will process only what the user is allowed to use. In the event of a security vulnerability, if a fix is not immediately available, it can be mitigated simply by removing embed/iframe/object/param tags from the HTML Filter whitelist, and MyTube will pose zero threat. Simply disabling MyTube without revoking this access will allow everything to pass through unfiltered, but disabling MyTube will not create any benefit (or problems for that matter) if the tags are also removed from the whitelist. The only other issue I am aware of, which is minor but I'm considering mitigating anyway, is that in rare isolated cases MyTube has failed to detect where a video source is located, and therefore not display a link in the disclaimer. I'm considering adding code to handle these embeds differently, warning users that the code appears malformed and may be malicious, but this would probably fall more in line with a new feature. MyTube was originally written by Tim with the intention that it would be run in a tightly-managed environment, where everyone posting content trusts one another (or a single-user blog), but I hope to make it safe enough so administrators can safely allow many relatively anonymous users to post embed content without it every automatically launching for anybody without their explicit per-case consent.

Comments

siliconmeadow’s picture

Status: Active » Reviewed & tested by the community

Further to #994286: lavagolemking [lava], I've reviewed and tested this module.

skaught’s picture

I just took a quick peek through you code. i haven't actually installed it, but i just thought i'ld make a quick recommendation..

Your javascript should be added in as a Behavior (such as: http://groups.drupal.org/node/20318 - best link i can find quickly).

As well, your .info needs some attention. We don't actually know which drupal version it's suppose to be used with.

l@va’s picture

As well, your .info needs some attention. We don't actually know which drupal version it's suppose to be used with.

I think this is a problem with Git. The version I submitted is here (I committed the exact same folder to both places) and I haven't changed that file since following the instructions here. If the git repository is filtering out the line that indicates the version of Drupal it's supposed to go with, then I don't know what else I can do.

I am not sure what you are asking of the JavaScript. Each thumbnail activates a JQuery script that replaces the thumbnail with the original embed code submitted by the content submitter.

dave reid’s picture

Status: Reviewed & tested by the community » Needs work

I'm seeing potential security holes in a basic scan of the module:

 122 /**
 123  * Implementation of hook_filter_tips
 124  */
 125 function mytube_filter_tips($delta, $format, $long = FALSE) {
 126   if (!$long) {
 127     return t("Embeds like YouTube and iframes will be prevented from grabbing visitors' data without their permission.");
 128   }
 129   else {
 130     $privacy_url = variable_get("mytube_url", "");
 131     if ($privacy_url == "") {
 132       $privacy_url="https://www.eff.org/deeplinks/2008/02/embedded-video-and-your-privacy";
 133     }
 134     $privacy_text = variable_get("mytube_text", "");
 135     if ($privacy_text == "") {
 136       $privacy_text = t("This embed will serve content from") . " <em><a rel=\"nofollow\" href=\"\$complete_embed_url\" onclick=\"return false;\">\$top-level_domain</a></em><br />";
 137     }
 138     else {
 139       $privacy_text = str_replace("!embed", "\$complete_embed_url", $privacy_text);
 140       $privacy_text = str_replace("!domain", "\$top-level_domain", $privacy_text);
 141     }

If someone were to add malicious text in the mytube_text variable (which is exposed in the admin UI), there is no filtering performed on it. Please make sure to review http://drupal.org/writing-secure-code and test out 'unsafe' strings in your module's variables like <script>alert('XSS!');</script> to make sure they do not get executed.

skaught’s picture

Status: Needs review » Needs work

try it as:

; $Id: MyTube.info  Exp $

maybe it's a syntax issue...i'm new to git myself. as far as i know, we don't actual need to include this line anymore (although i can't promise that's true.) that line is actually a left over from CVS. i'm use to SubVersion, which doesn't need use it eaither, but in general it is handy for keeping track one's own code progression...

as for the javascript, its about how we are 'suppose to' integrate jQuery into drupal. it's a method that will allow other prototype base javascript frameworks (or open prototyping) using the '$() function' to co-run (plays well with others). a better link: http://drupal.org/node/205296

i'm actually going through this whole process myself right now: http://drupal.org/node/1078580 i first say you entry and thought i had competition....my modules a youtube input filter. but not for 'the old swf embed method'

cheers.

l@va’s picture

Status: Needs work » Needs review

If someone were to add malicious text in the mytube_text variable (which is exposed in the admin UI), there is no filtering performed on it. Please make sure to review http://drupal.org/writing-secure-code and test out 'unsafe' strings in your module's variables like

alert('XSS!');

to make sure they do not get executed.

I don't see this as a particularly serious issue, because that user would have to have "administer mytube" permissions, which would typically only be assigned to an administrator. If you're worried about an administrator injecting malicious code into your site, then you probably shouldn't be giving them administrative privileges. MyTube is an input filter. As such, giving users the ability to configure it is right up there with giving said users "administer filters" permissions. I cannot think of a single case where a user can't compromise the integrity of your site if they have that permission. Nonetheless, that choice is best left up to site owners (maybe there is some obscure implementation where this would be the case...), and the issue in question is fixed for all fields in the MyTube administration page. Update has been deployed.

maybe it's a syntax issue...

As of the above commit, it's showing the correct mytube.info file for me. I'm thinking whatever happened was a temporary issue.

as for the javascript, its about how we are 'suppose to' integrate jQuery into drupal. it's a method that will allow other prototype base javascript frameworks (or open prototyping) to using he '$() function' to co-run (plays well with others). a better link: http://drupal.org/node/205296

Javascript is now using Drupal behaviors.

dave reid’s picture

FYI You cannot easily just make exceptions for your own administer permissions. Those have to be accepted by the security team. There are plenty of 'administer [foo]' permissions that could potentially output XSS (core's own 'administer taxonomy' permission included), but you still have to filter on output.

sreynen’s picture

Hi l@va,

A few issues I noticed:

  • ";$Id" and "// $Id$" no longer do anything on Drupal.org. That's a CVS thing, and we're on Git now.
  • _mytube_add_param ($url, $param, $embed) has a space between the function name and open parenthesis; space should be removed.
  • Most of _mytube_download() should be replaced with drupal_http_request().
  • "// FIXME:" That should be fixed.
  • Though it's not exactly a bug, I'd encourage you to use double quotes less, as all that escaping makes the code harder to read. For single variables, you don't need quotes at all. For regular expressions or markup without variables, single quotes avoid the need to escape included double quotes.
siliconmeadow’s picture

Hi l@va - what can we do to get you fired up to get this moving again? Mytube is a very worthwhile would certainly be popular if available on d.o as a first class module.

l@va’s picture

The changes are coming; I'm just a bit busy running for student government right now, being committed to 3 places at a given time and all. I'm wrapping up elections after this week, so it shouldn't be long before I get back into it.

l@va’s picture

Status: Needs work » Needs review

";$Id" and "// $Id$" no longer do anything on Drupal.org. That's a CVS thing, and we're on Git now.

Removed.

_mytube_add_param ($url, $param, $embed) has a space between the function name and open parenthesis; space should be removed.

Fixed.

Most of _mytube_download() should be replaced with drupal_http_request().

Most of the function body has been replaced. Calls to _mytube_download() and _mytube_download_thumb() need to stay. _mytube_download() returns a path to the file it downloads, while drupal_http_request() returns the file itself in $result->data (the return value is horribly documented, by the way). The cURL processes are now completely replaced though, and MyTube no longer depends on the php-curl library, but the functionality provided by _mytube_download() cannot be fully replaced by drupal_http_request().

"// FIXME:" That should be fixed.

On closer inspection, it doesn't appear that MetaCafé provides embed code for 3rd-party embeds, so users are unlikely to post such content instead of just taking the YouTube embed. Were somebody to do this, using some debugging tools on MetaCafé's website, and were somebody to also have posted the equivalent YouTube embed, then Drupal would have 2 separate but identical thumbnails downloaded from the same place for that particular video. I have decided not to pursue this any further, since it doesn't break anything.

Though it's not exactly a bug, I'd encourage you to use double quotes less, as all that escaping makes the code harder to read. For single variables, you don't need quotes at all. For regular expressions or markup without variables, single quotes avoid the need to escape included double quotes.

Most instances of double quotes where this is an issue have been replaced with single quotes.

sreynen’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. I did open a minor issue to update the documentation to match your more recent changes, #1129418: References to php5-curl in INSTALL.txt and README.txt are no longer relevant.

rfay’s picture

Status: Reviewed & tested by the community » Fixed

Git vetted User access granted. Thanks for your contributions, and future contributions, and for your patience during this process! Please make sure to help review other applications.

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

Component: new project application » distribution/profile