460008 - View source on images needs some love

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, enhancement, P4)

Description

[u278084]

Attachment: example

When you view source and navigate to an image, you get garbage. That needs to be fixed to show the image.

Comment 1

[u278084]

Attachment: v1

I think the best solution is to encode the binary as base64 and display it as a data url. This has a few pros and cons:

Cons : Doesn't work in IE7
Pros : Works in Safari, Firefox, Opera, maybe IE8. Doesn't work in IE6

But it's editor tools, so that's really not important :)
I used the filename's extension to figure out the mime type. It's a cheap hack. There are other ways (exif, finfo) but require external libraries to be compiled that I don't think is worth the additional trouble.

Comment 2

[Fred Wenzel [:wenzel]]

I love how you list "does not work with IE6" under Pro ;)

However, what *does* it display in IE7?

Comment 3

[u278084]

(In reply to comment #2)
> However, what *does* it display in IE7?

It shows up as a broken image. The same you would get if the image was not found.

Comment 4

[Fred Wenzel [:wenzel]]

Wouldn't it be better to just display an <img.../> tag and point the src to a new action in the same controller that will fpassthru() the file in question?

Comment 5

[u278084]

(In reply to comment #4)
> Wouldn't it be better to just display an <img.../> tag and point the src to a
> new action in the same controller that will fpassthru() the file in question?

It wouldn't make a lot of sense. All images are in a xpi (or in a jar in the xpi), so we'll have to extract to disk first. Also, data urls seem to work in IE8. yayz.

Comment 6

[u278084]

Attachment: A cross-browser solution

This outta work in IE6+7, if they're important and all. It's basically comment 4 without fpassthru().

Comment 7

[Fred Wenzel [:wenzel]]

(In reply to comment #6)
> Created an attachment (id=372071) [details]
> A cross-browser solution
> 
> This outta work in IE6+7, if they're important and all. It's basically comment
> 4 without fpassthru().

Now, I am not sure I understand you quite right: In comment 5 you say it doesn't make sense and in comment 6 you implement it anyway? Hmmmmmm.

Comment 8

[u278084]

(In reply to comment #7)
> Now, I am not sure I understand you quite right: In comment 5 you say it
> doesn't make sense and in comment 6 you implement it anyway? Hmmmmmm.

Well, yeah. I was half-way through trying comment 4 without thinking when I realized that fpassthru() would cause so many read/writes to disk, it would probably be worse. I then figured that, if we used a controller to get images and set the right headers, it would at least work in browsers that don't support data urls.

Sorry. I make things complicated and confusing.

Comment 9

[Jeff Balogh (:jbalogh)]

Comment on attachment 372071 [details] [diff] [review]
A cross-browser solution

>Index: site/app/controllers/files_controller.php
>===================================================================
>--- site/app/controllers/files_controller.php	(revision 24199)
>+++ site/app/controllers/files_controller.php	(working copy)
>@@ -24,6 +24,7 @@
>  *   Wil Clouser <[e-mail]>
>  *   Justin Scott <[e-mail]>
>  *   Frederic Wenzel <[e-mail]>
>+ *   Cesar Oliveira  <[e-mail]>

Typo or spam avoidance tricks?


>+        } else if (in_array($ext, array('png', 'gif', 'jpg', 'jpeg', 'bmp', 'ico'))) {
>+            $this->publish('maybeImage', true);

Since you're using this same array down below, could we put it in a variable somewhere?

Maybe IMAGE_EXTENSIONS in config/constants.php?

>         }
>         $this->publish('filename', $file);
>         $this->publish('contents', $contents);
>         $this->render('view', 'ajax');
>     }
> 
>+    function _fetch($path, $file, $addontype) {
>+        $file = html_entity_decode(urldecode($file), ENT_QUOTES, 'UTF-8');
>+        $contents = $this->_get_contents($path, $file, $addontype);
>+        $ext = mb_strtolower(pathinfo($file, PATHINFO_EXTENSION));
>+        /* text/plain causes Mozilla to do content-sniffing */
>+        $contenttype = 'text/plain';
>+
>+        if ($contents === false) {
>+            $contents = '';
>+        } else if (in_array($ext, array('png', 'gif', 'jpg', 'jpeg', 'bmp', 'ico'))) {
>+            switch ($ext) {
>+                case 'png' :
>+                    $contenttype = 'image/png';
>+                    break;
>+                case 'gif' :
>+                    $contenttype = 'image/gif';
>+                    break;
>+                case 'jpg' :
>+                case 'jpeg' :
>+                    $contenttype = 'image/jpeg';
>+                    break;
>+                case 'bmp' :
>+                    $contenttype = 'image/bmp';
>+                    break;
>+                case 'ico' :
>+                    $contenttype = 'image/vnd.microsoft.icon';
>+                    break;
>+            }

Or instead of an array of extensions, it could map extensions to content types?

IMAGE_TYPES = array('png' => 'image/png', ...);

} else if (array_key_exists($ext, IMAGE_TYPES)) {
    $content_type = IMAGE_TYPES[$ext];
}

I'm a sucker for moving data out of code.  :)

>+        }
>+
>+        header('Content-Type: '.$contenttype);
>+        print $contents;
>+
>+        $this->autoRender = false;
>+    }
>+
>     function diff($file_id) {
>         $this->Amo->clean($file_id);
>         
>Index: site/app/views/files/view.thtml
>===================================================================
>--- site/app/views/files/view.thtml	(revision 24199)
>+++ site/app/views/files/view.thtml	(working copy)
>@@ -21,6 +21,7 @@
>  *
>  * Contributor(s):
>  *    Frederic Wenzel <[e-mail]>
>+ *    Cesar Oliveira  <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>@@ -44,7 +45,9 @@
>         echo $line;
>     }
> }
>-else {
>+else if (isset($maybeImage)) {
>+    echo '<img src="/'.$this->params['url']['url'].'?fetch='.$filename.'"/>';

I think you need to use $html->url, or even $html->image, else the link won't work for me on jbalogh.khan.../amo/site/*.

Comment 10

[u278084]

(In reply to comment #9)
> Typo or spam avoidance tricks?
Typo.

> I'm a sucker for moving data out of code.  :)
That's a good point. I made those changes.

> I think you need to use $html->url, or even $html->image, else the link won't
> work for me on jbalogh.khan.../amo/site/*.
Ok. I wasn't sure how to make the link. But $html->url seems to be what I'm looking for.

Comment 11

[u278084]

Attachment: patch v2 + fixes

I like this solution better than using data urls. So I am obsoleting my first patch. It incorporates the fixes from comment 9. Thanks for volunteering to review my patch! ;)

Comment 12

[Jeff Balogh (:jbalogh)]

Comment on attachment 372479 [details] [diff] [review]
patch v2 + fixes


>@@ -102,6 +103,9 @@
>         if (!empty($_GET['view'])) {
>             $this->_view($path, $_GET['view'], $addontype);
>             return;
>+        } else if (!empty($_GET['fetch'])) {
>+            $this->_fetch($path, $_GET['fetch'], $addontype);
>+            return;
>         }
> 
>         $files = array();
>@@ -158,16 +162,41 @@
>     function _view($path, $file, $addontype) {
>         $file = html_entity_decode(urldecode($file), ENT_QUOTES, 'UTF-8');
>         $contents = $this->_get_contents($path, $file, $addontype);
>+        $ext = mb_strtolower(pathinfo($file, PATHINFO_EXTENSION));
> 
>+        global $image_types;
>         if (is_bool($contents) && $contents == false) {
>             $this->flash(_('error_file_notfound'), '/');
>             return;
>+        } else if (array_key_exists($ext, $image_types)) {
>+            $this->publish('maybeImage', true);
>         }
>         $this->publish('filename', $file);
>         $this->publish('contents', $contents);
>         $this->render('view', 'ajax');
>     }
> 
>+    /* Handles files that require special cases (eg. images) */
>+    function _fetch($path, $file, $addontype) {
>+        $file = html_entity_decode(urldecode($file), ENT_QUOTES, 'UTF-8');

I didn't know what those decode functions were supposed to do, so I looked them up.  On http://us2.php.net/urldecode there's a big warning: The superglobals $_GET and $_REQUEST  are already decoded. Using urldecode() on an element in $_GET or $_REQUEST could have unexpected and dangerous results.

$file here is coming from $_GET['fetch'].  Could you add a couple tests showing what those decodes are supposed to do, and what weird cases they help with?


>-else {
>+else if (isset($maybeImage)) {
>+    echo '<img src="' . $html->url('/files/browse/'.$this->params['pass'][0].'/?fetch='.$filename) . '"/>';
>+} else {
>     echo $contents;
> }
> echo '</pre>';

Can you add a couple tests for the new features as well?  One to check that $maybeImage is being set when it should (and that the URI is correct), and another that makes sure a ?fetch URL returns the right thing.  Thanks!

Comment 13

[u278084]

(In reply to comment #12)
> I didn't know what those decode functions were supposed to do, so I looked them
> up.  On http://us2.php.net/urldecode there's a big warning: The superglobals
> $_GET and $_REQUEST  are already decoded. Using urldecode() on an element in
> $_GET or $_REQUEST could have unexpected and dangerous results.
> 
> $file here is coming from $_GET['fetch'].  Could you add a couple tests showing
> what those decodes are supposed to do, and what weird cases they help with?

Good point. I took it directly from the _view() code, and the urldecode() was added as a consequence of bug 460991. I talked to Wil about it, and he can't remember the reason for including it (likely because the bug was fixed last November). So I'm a bit stuck on whether to leave it in or take it out. CC'ing Wil to see if he has any additional comments. I'm leaning towards taking it out. I just can't figure out why it is added, but I also can't figure out how it can be really dangerous.

Comment 14

[:Gijs (he/him)]

I was about to file this and found I wasn't exactly the first. What needs to happen to get this landed and fixed? It's past my annoyance threshold by now (took longer because I review fewer add-ons than Cesar, probably ;-) ).

Comment 15

[fcp]

Comment on attachment 372479 [details] [diff] [review]
patch v2 + fixes

>-else {
>+else if (isset($maybeImage)) {
>+    echo '<img src="' . $html->url('/files/browse/'.$this->params['pass'][0].'/?fetch='.$filename) . '"/>';
>+} else {
>     echo $contents;
> }
> echo '</pre>';

I am not reviewing the patch, but I just realized that this part is seemingly putting <img> in <pre>, which is not allowed according to the XHTML 1.0 spec (or the HTML 4.01 spec either).
http://www.w3.org/TR/xhtml1/#prohibitions

Comment 16

[u278084]

(In reply to comment #14)
> I was about to file this and found I wasn't exactly the first. What needs to
> happen to get this landed and fixed? It's past my annoyance threshold by now
> (took longer because I review fewer add-ons than Cesar, probably ;-) ).

Heh. It is just missing tests. Specifically, the one regarding using urldecode on a $_GET.

(In reply to comment #15)
> I am not reviewing the patch, but I just realized that this part is seemingly
> putting <img> in <pre>, which is not allowed according to the XHTML 1.0 spec
> (or the HTML 4.01 spec either).
> http://www.w3.org/TR/xhtml1/#prohibitions

Thanks, I will make that change for the next patch.

Comment 17

[Wil Clouser [:clouserw]]

I'll drop in 5.4 so I remember to follow up with Cesar.

Comment 18

[u278084]

Attachment: brought back from the dead

Bit rot was gnarly.

I removed the urldecode that I used, mainly because I couldn't find out why we used it before. Removed the image tag from inside the pre tag. I grew really frustrated last time I tried to make test cases for these, so I am not including it.

Comment 19

[fcp]

Comment on attachment 413827 [details] [diff] [review]
brought back from the dead

(This is not a review of the patch.)  In view.thtml, can $filename contain symbols such as “&” and “#”?  If so, they do not seem to be handled correctly.  This may be a separate issue from this bug.

Comment 20

[u278084]

(In reply to comment #19)
> (From update of attachment 413827 [details] [diff] [review])
> (This is not a review of the patch.)  In view.thtml, can $filename contain
> symbols such as “&” and “#”?  If so, they do not seem to be handled correctly. 
> This may be a separate issue from this bug.

Yes it can. I haven't tried it though. Our data sanitization is pretty unpredictable sometimes, so I'm not sure if it's a concern.

Comment 21

[Jeff Balogh (:jbalogh)]

Comment on attachment 413827 [details] [diff] [review]
brought back from the dead

>Index: controllers/files_controller.php
>+        header('Content-Type: '.$contenttype);
>+    	$this->publish('contents', $contents, false);
>+	    $this->render('fetch', 'ajax');
>+    }
>+

There's a tab and other weird indentation there.

Otherwise, it looks good.  Thanks!

Comment 22

[Wil Clouser [:clouserw]]

I'm not sure if cesar is going to be around tonight, but this is r56873.  Thanks for the patch, cesar!

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/files/browse/31645; nice work, Cesar.