Closed
Bug 460008
Opened 16 years ago
Closed 15 years ago
View source on images needs some love
Categories
(addons.mozilla.org Graveyard :: Admin/Editor Tools, enhancement, P4)
addons.mozilla.org Graveyard
Admin/Editor Tools
Tracking
(Not tracked)
VERIFIED
FIXED
5.4
People
(Reporter: u278084, Assigned: u278084)
References
()
Details
Attachments
(2 files, 3 obsolete files)
84.70 KB,
image/jpeg
|
Details | |
5.36 KB,
patch
|
jbalogh
:
review+
|
Details | Diff | Splinter Review |
When you view source and navigate to an image, you get garbage. That needs to be fixed to show the image.
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
Comment 2•16 years ago
|
||
I love how you list "does not work with IE6" under Pro ;) However, what *does* it display in IE7?
(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•16 years ago
|
||
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?
(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.
This outta work in IE6+7, if they're important and all. It's basically comment 4 without fpassthru().
Comment 7•15 years ago
|
||
(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.
(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•15 years ago
|
||
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•15 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•15 years ago
|
||
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 12•15 years ago
|
||
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•15 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•15 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•15 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•15 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.
Comment 17•15 years ago
|
||
I'll drop in 5.4 so I remember to follow up with Cesar.
Priority: -- → P4
Target Milestone: --- → 5.4
Assignee | ||
Comment 18•15 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•15 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•15 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.
Updated•15 years ago
|
Attachment #413827 -
Flags: review?(jbalogh) → review+
Comment 21•15 years ago
|
||
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•15 years ago
|
||
I'm not sure if cesar is going to be around tonight, but this is r56873. Thanks for the patch, cesar!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/files/browse/31645; nice work, Cesar.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•