View source on images needs some love

VERIFIED FIXED in 5.4

Status

enhancement
P4
normal
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: cesar, Assigned: cesar)

Tracking

unspecified

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

11 years ago
Posted image example
When you view source and navigate to an image, you get garbage. That needs to be fixed to show the image.
Assignee

Comment 1

11 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → cdolivei.bugzilla
Status: NEW → ASSIGNED
I love how you list "does not work with IE6" under Pro ;)

However, what *does* it display in IE7?
Assignee

Comment 3

11 years ago
(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.
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?
Assignee

Comment 5

10 years ago
(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.
Assignee

Comment 6

10 years ago
Posted patch A cross-browser solution (obsolete) — Splinter Review
This outta work in IE6+7, if they're important and all. It's basically comment 4 without fpassthru().
(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.
Assignee

Comment 8

10 years ago
(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 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 <clouserw@mozilla.com>
>  *   Justin Scott <fligtar@gmail.com>
>  *   Frederic Wenzel <fwenzel@mozilla.com>
>+ *   Cesar Oliveira  <a.sacred.line@gmailc.om>

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 <fwenzel@mozilla.com>
>+ *    Cesar Oliveira  <a.sacred.line@gmailc.om>
>  *
>  * 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/*.
Assignee

Comment 10

10 years ago
(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.
Assignee

Comment 11

10 years ago
Posted patch patch v2 + fixes (obsolete) — Splinter Review
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! ;)
Attachment #343680 - Attachment is obsolete: true
Attachment #372071 - Attachment is obsolete: true
Attachment #372479 - Flags: review?(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!
Attachment #372479 - Flags: review?(jbalogh) → review-
Assignee

Comment 13

10 years ago
(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

10 years ago
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

10 years ago
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
Assignee

Comment 16

10 years ago
(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.
I'll drop in 5.4 so I remember to follow up with Cesar.
Priority: -- → P4
Target Milestone: --- → 5.4
Assignee

Comment 18

10 years ago
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.
Attachment #372479 - Attachment is obsolete: true
Attachment #413827 - Flags: review?(jbalogh)

Comment 19

10 years ago
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.
Assignee

Comment 20

10 years ago
(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.
Attachment #413827 - Flags: review?(jbalogh) → review+
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!
I'm not sure if cesar is going to be around tonight, but this is r56873.  Thanks for the patch, cesar!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.