385338 - Bugzilla should display application/zip contents (possibly using jar:x!/ links in attachment editing)

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement, P3)

Description

[timeless]

try to load a zip file in the attachment editor. you get this rude message:

Attachment is not viewable in your browser because its MIME type (application/zip) is not one that your browser is able to display.

Download the attachment.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/2007053004 Minefield/3.0a5pre

Expected results:
<object src="jar:attachment.cgi?id=269183&action=view!/">
          <p><b>
            Attachment is not viewable in your browser because its MIME type 
            (application/zip) is not one that your browser is 
            able to display.
          </b></p>
          <p><b>
            <a href="attachment.cgi?id=269183">Download the attachment</a>.
          </b></p>
</object>

Well, something like that.

Comment 1

[Max Kanat-Alexander]

This isn't a bad idea. I've never seen what that actually looks like, though--does somebody have a screen shot?

Comment 2

[Frédéric Buclin]

What does jar: do?

Comment 4

[Brian Murrell]

I want also to be able to view and download components of the zip/tar/targeezee, not just edit the components.  In fact view/download should easier than edit as one doesn't have to put the extracted file back into the composite container.

Comment 5

[timeless]

Attachment: jar:https://bugzilla.mozilla.org/attachment.cgi?id=269183&action=view!/

Comment 6

[timeless]

Attachment: i wonder if this works

fwiw, you pretty much need seamonkey to test (firefox optimized away support for this feature ...).

Comment 7

[Max Kanat-Alexander]

So this feature doesn't work in the vast majority of web browsers?

Comment 8

[timeless]

you mean it doesn't work with ie? yeah.

actually, it does work with firefox. i was having trouble because i kept using the wrong urls.

Comment 9

[Max Kanat-Alexander]

Okay, it if works with firefox, then it's definitely an acceptable feature. :-)

Comment 10

[Frédéric Buclin]

Comment on attachment 271456 [details] [diff] [review]
i wonder if this works

>Index: mozilla/webtools/bugzilla/attachment.cgi

>+# Returns 1 if the parameter is a content-type representing a zip archive
>+sub isZipArchive

I want it as a method of Bugzilla::Attachment. I'm moving routines outside attachment.cgi, so don't add new ones here.


>+  return 0 unless $contenttype =~ m{^application/(?:x-|)(.*)};
>+  return 1 if $1 =~ /^(?:zip|xpinstall|java-archive|jar)/;
>+  return 0;

Please write this as a single regexp to avoid 3 consecutive return statements.


Note that this patch will only works with Firefox 3. It doesn't work with Firefox 2. But the advantage is that it doesn't depend on any external Perl module, which I like.

Comment 11

[timeless]

Attachment: modularity

Comment 12

[Frédéric Buclin]

Comment on attachment 273379 [details] [diff] [review]
modularity

>Index: attachment.cgi

>     foreach my $a (@$attachments) {
>+        $a->{'iszip'} = $a->isZipArchive;
>+        $a->{'isviewable'} = $a->isViewable;
>     }

Don't do that! Now that we have methods, we shouldn't alter the internal variables anymore. They are already set. So this loop should go away.


>+  my $iszip = $attachment->isZipArchive;
>+  my $isviewable = !$attachment->isurl && $attachment->isViewable;

>+  $vars->{'iszip'} = $iszip;
>   $vars->{'isviewable'} = $isviewable; 

We pass the attachment object to the template. It doesn't make sense to store its attributes in separate variables. The template already can access them easily. So both hash keys above should go away now that isViewable() is a method.



>Index: Bugzilla/Attachment.pm

>+sub isViewable
>+{

In Attachment.pm, methods are of_this_form() insteadOfThisForm(). Please update it accordingly.
Nit: also, keep the braket on the same line as the method name.


>+    if ((($contenttype =~ m{^application/vnd\.mozilla\.}) ||
>+        $attachment->isZipArchive) &&
>+        ($cgi->user_agent() =~ m{Gecko/}))

Wow. So many parens that it's almost unreadable. Only the ($foo || $bar) ones are useful.
Moreover, $attachment should be $self, and you forgot to define $cgi in this method, so Bugzilla crashes.


>+sub isZipArchive
>+{

Same comment as above. This method should be named is_zip_archive.


>+    return $contenttype =~
>+      m{^application/(?:x-|)(:zip|xpinstall|java-archive|jar)};

I just attached a .zip file on Bugzilla, and Firefox 2 set the MIME type to application/octet-stream. Isn't Firefox able to set it to one of those above?
Moreover, (:zip|...) is incorrect. You probably meant (?:zip|...) ?



>Index: template/en/default/attachment/edit.html.tmpl

>+[% IF iszip %]

With my previous comments, this becomes [% IF attachment.is_zip_archive %]


>+            <iframe id="zipFrame"
>+                    src="jar:attachment.cgi?id=[% attachment.id %]!/"
>+                    type="[% attachment.contenttype FILTER html %]">
>+          [% END %]
>           <iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;">
>             <b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
>             <a href="attachment.cgi?id=[% attachment.id %]">View the attachment on a separate page</a>.</b>
>           </iframe>
>+          [% IF iszip %]
>+            </iframe>
>+          [% END %]

This completely breaks the UI. Also, is it intentional to have one <iframe> included in another one? I don't think so.

Comment 13

[Frédéric Buclin]

Oh, and also move is_viewable() and is_zip_archive() (or why not is_archive(), it's shorter) to other is_foo() methods in Attachment.pm.

Comment 14

[timeless]

Attachment: rearrange things

Comment 15

[Frédéric Buclin]

Comment on attachment 273466 [details] [diff] [review]
rearrange things

>Index: attachment.cgi

Changes in attachment.cgi are no longer needed. I moved isViewable() into Attachment.pm in bug 392175. Your current patch contains two errors about this move anyway.



>Index: Bugzilla/Attachment.pm

>+sub is_viewable {

>+    if (($contenttype =~ m{^application/vnd\.mozilla\.}
>+         || $attachment->is_zip)
>+        && $cgi->user_agent() =~ m{Gecko/}) {
>+        return $self->{is_viewable} = 1;
>+    }

Firefox 2 isn't able to use jar: to view the content of archives, so despite it's based on Gecko, a zip archive is not viewable for Firefox 2.



>Index: template/en/default/attachment/edit.html.tmpl

>+[% IF attachment.is_zip %]
>+  function viewZip()
>+    {
>+      switchToMode('zip');
>+    }
>   function viewRaw()

The template doesn't compile. There is a missing [% END %].


>-      [% ELSIF isviewable %]
>+      [% ELSIF attachment.is_viewable %]

This change is incorrect as isviewable was also taking is_url into account. But this has already been addressed in bug 392175.


>+            <iframe id="zipFrame"
>+                    src="jar:attachment.cgi?id=[% attachment.id %]!/"
>+                    type="[% attachment.contenttype FILTER html %]"
>+                    style="height: 400px; width: 100%;">

Could archives be affected by bug 38862?

Comment 16

[timeless]

Attachment: merge

Adding support for recognizing bad geckos can be done later. It really shouldn't block this feature.

In my preliminary testing, it doens't work here either:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060329 Firefox/1.6a1

(I was told it would)

As to your questions about what that other thing will affect, it *should* break the edit attachment as comment feature, but it should not affect this because this is entirely handled by the browser and doesn't involve any web page processing of the content (it's just changing the url of the iframe or something like that).

Comment 17

[Frédéric Buclin]

Comment on attachment 276747 [details] [diff] [review]
merge

FYI, is_viewable() is already in Attachment.pm, and your template doesn't compile because you have an [% IF %] alone with no [% END %].

As I said earlier, attaching such obviously broken patches again and again is disrespectful to the reviewer.

Comment 18

[timeless]

Attachment: merged

lpsolit: knowingly bitrotting someone else's patch that's in *your* queue is not respectful either.

please consider the work i do as template contributions. in the end it's in bugzilla's best interest for *someone* not necessarily me to finish the patches and get them committed.

You need to stop treating everyone as if they have infinite time to deal with your review comments. The goal of bugzilla should be to pull in improvements whenever possible, taking unfinished ideas and polishing them.

Not sitting on a pile of bug reports.

Comment 19

[Dave Miller [:justdave]]

(In reply to comment #18)
> You need to stop treating everyone as if they have infinite time to deal with
> your review comments. The goal of bugzilla should be to pull in improvements
> whenever possible, taking unfinished ideas and polishing them.

As lofty as a goal that is (and I agree, it's a good goal) we really don't have the manpower to act that way right now.  You need to get buy-in from someone to clean up your stuff if you want it committed, otherwise be content to consider it submitted ideas and wait until someone comes along with time to work on it (in which case you should reassign the bug to the default assignee instead of yourself so we know someone else needs to clean up if you're not going to clean them up yourself).

We'll probably jump all over ourselves to help clean up for a newbie submitting a patch who isn't sure how to clean up on their own, but the fact of the matter is that you aren't a newbie, and you've been here for years, and your attitude really makes people want to ignore you sometimes.  It's going to be hard to get people to clean up after someone who's rude all the time.  I've known you long enough to know you're a pretty straight-forward guy who just says what's on his mind most of the time, and you're not intentionally being rude.  But you still need to make an attempt to be a little courteous in your communication once in a while, especially if you want help getting your patches cleaned up enough to commit them.  Instead of saying "*someone* not necessarily me should finish this" as if we should have known we needed to clean it up for you, try saying "I'm a bit busy right now, would someone else be willing to take over getting this cleaned up and committed?".  Bet that goes a long way.  Otherwise we tend to have the attitude that we don't want to intrude on your work or steal your glory. ;)

Comment 20

[timeless]

gah. i don't want glory. i want a better Bugzilla. and yet, here lpsolit specifically stepped on me. I want Bugzilla to work better at places I go, and I want to be able to sell Bugzilla to places so I'm not stuck with something worse.

Comment 21

[Greg Hendricks]

Comment on attachment 279714 [details] [diff] [review]
merged

Global symbol "$attachment" requires explicit package name at Bugzilla/Attachment.pm line 328.

Comment 22

[timeless]

Attachment: corrected

Comment 23

[Frédéric Buclin]

Comment on attachment 286894 [details] [diff] [review]
corrected

>Index: template/en/default/attachment/edit.html.tmpl

>+                  style="height: 400px; width: 100%;[% hide_viewframe %]">

Doesn't pass tests:

t/008filter..........NOK 70
#   Failed test '(en/default) template/en/default/attachment/edit.html.tmpl has unfiltered directives:
#   344: hide_viewframe
# --ERROR'

Comment 24

[timeless]

Attachment: filtered

Comment 25

[Frédéric Buclin]

Comment on attachment 302996 [details] [diff] [review]
filtered

The patch doesn't work. When clicking the "View contents of attachment" button, all other buttons disappear and clicking on it a second time throws an error. Moreover, Firefox 3 refuses to show the content of the zip file, arguing that opening such a file type is unsafe. And there is no button in the UI to workaround this. If Firefox 3 definitely prevents such an action, we will have to mark this bug as WONTFIX.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Firefox 3 allows JAR URIs to work if the content type of the zip file being viewed is application/java-archive or application/x-jar. You need to be aware of the XSS possibilities that entails, but there are existing Bugzilla bugs on that and this doesn't make them any worse.

Comment 27

[Frédéric Buclin]

I attached a zip file, and it has been detected as application/zip. Why is there a XSS risk? Isn't Firefox supposed to list files in the zip file, nothing more? I would expect to get a safe list of files.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

The XSS risk is that some sites might allow you to upload arbitrary zip files, without realizing that that effectively allows people to arbitrarily upload HTML/JS that can be linked to using the jar: protocol. See bug 369814 for details.

The fix for that bug was to whitelist only those two MIME types, under the assumption that site administrators sending JAR files with that content type understand the risks.