219358 - attachments with MIME type "application/xhtml+xml" cannot be viewed in the edit page

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Steffen Wilberg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030914 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030914 Firebird/0.6.1+

Attachments with MIME type "application/xhtml+xml" are not displayed in the edit
page.

Reproducible: Always

Steps to Reproduce:
1. Click on "Edit" next to an attachment which has the MIME type
"application/xhtml+xml"

Actual Results:  
On the right side of the edit page, an error is displayed: "Attachment cannot be
viewed because its MIME type is not either text/*, image/*, or
application/vnd.mozilla.*. Download the attachment instead."

Expected Results:  
Bugzilla should display the attachment.

Comment 1

[Dave Miller [:justdave]]

*** Bug 219513 has been marked as a duplicate of this bug. ***

Comment 2

[Gervase Markham [:gerv]]

Moving to Bugzilla product - this is why I didn't find this bug before filing
the dupe. It's a Bugzilla bug, because this MIME type can only be displayed
inline on certain browsers, and so need special support.

Gerv

Comment 3

[Dave Miller [:justdave]]

idea: since other than UserAgent sniffing (which isn't terribly reliable)
there's not an easy way to figure out who can support what, how about if we
display the iframe regardless, but pass a different action than "view" (like
"inline").  If it's a type we think they can display, it just falls through to
"view" anyway. If it's not a type we think all browsers support, that action can
display an empty page within the IFRAME that explains it something like this:

This attachment is of type <mime-type>, which is not universally viewable in all
browsers.  If you think your browser should be able to view it, <a>view it
anyway</a>.  NOTE: If your browser really can't view it, this will probably
result in a file download dialog.

which then does action=view within the IFRAME when you click the "view it
anyway" link.

Comment 4

[Gervase Markham [:gerv]]

That sounds a bit clunky...

As the web moves more to XHTML, XHTML attachments will become the norm. At what
stage will we swap over to just displaying it, or will we have this intermediate
screen for evermore?

On the other hand, if you just want to request a review or change a flag,
currently you have to load the whole patch, even if it's big. Is there an
argument for having this feature for all patches? Or all patches over a certain
size?

Gerv

Comment 5

[Boris Zbarsky [:bzbarsky]]

> there's not an easy way to figure out who can support what

There's the "Accept:" header....

Comment 6

[Gervase Markham [:gerv]]

Yeah, how would that be for a plan? If the attachment is of an explicit type
mentioned in the Accept: header, then display it inline.

Mozilla's Accept header is:
text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,image/jpeg,image/gif;q=0.2,*/*;q=0.1

Changing to this algorithm, for Mozilla at least, would mean 
- we stopped displaying inline text/*, where * is not html, xml or plain
- we stopped displaying inline image/*, where * is not png, jpeg or gif
- we stopped displaying inline application/vnd.mozilla.*
- we started displaying inline application/xml,application/xhtml+xml

However, IE's Accept: header is far less useful, so this probably wouldn't work
for IE.

How about the current set, _plus_ any explicitly mentioned in the Accept: header?

Gerv

Comment 7

[Steffen Wilberg]

Our accept header says */* at the end, i.e. it accepts any content, it just
prefers other content types (higher "q"). Any browser I know has got an unknown
content type dialog, e.g. IE displays a download dialog when served with
application/xhtml+xml.

So why don't we just let the browser decide what to do with it, display it, use
a plugin, or show the unknown content dialog?

At least we should keep the current set, _plus_ any explicitly mentioned in the
Accept header, as Gerv suggested. Because if we don't display */*, and only the
text subcategories xml, text and plain, we'd stop displaying e.g. text/css.

Comment 8

[Gervase Markham [:gerv]]

> So why don't we just let the browser decide what to do with it, display it, use
> a plugin, or show the unknown content dialog?

Because people going to the Edit Attachment screen don't necessarily want to
download the attachment. So, we don't want to serve it to them unless it's an
iframe-able type.

> At least we should keep the current set, _plus_ any explicitly mentioned in the
> Accept header, as Gerv suggested.

That's sounding like the best plan to me. I'll whip up a patch.

Gerv

Comment 9

[Dave Miller [:justdave]]

Yeah, that was the reason I shied away from using the Accept header before, was
because Mozilla has */* in it, and we know serving */* in this situation isn't
reliable.  Keeping the existing set plus any *explicitly* (not wildcarded)
listed in the Accept header would probably work.  Or maybe a q-value of 0.2 or
better (since the */* as a q=0.1)

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This patch changes the behaviour to let through all text/*, image/*, and
application/vnd.mozilla.* to Mozilla. In addition, it lets through any type
explicitly mentioned (not by wildcard) in the Accept: header.

Gerv

Comment 11

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 135077 [details] [diff] [review]
Patch v.1

Requesting review from [e-mail]. 

Gerv

Comment 13

[Steffen Wilberg]

Comment on attachment 135077 [details] [diff] [review]
Patch v.1

It's a great honour for me to be requested for review! I have to note however
that I'm not a perl hacker. And I know nothing about the Bugzilla code. But
nevertheless, this code looks good. I've also tested this algorithm and it
works as it should.

>+  my $accept = join(",", $cgi->Accept());
Move this line down, above the if ($accept). We don't need to look at the
accept header if the contenttype is text/ or image/ or xul.

>+            Attachment cannot be viewed because its MIME type is not one that
>+            your browser is able to display.
We should tell the user the contenttype. We should also express that it's the
browser which makes the decision (by its accept header), not us. Something
like: "The MIME type of this attachment is [% contenttype %], which your
browser does not explicitly specify that it is able to display."

r=me.

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Fixed review comments.

Gerv

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 135141 [details] [diff] [review]
Patch v.2

Requesting r= and a= from justdave.

Gerv

Comment 16

[Joel Peshkin]

Gerv,

   How do we prevent XSS attacks in html or xml coded attachments??

Comment 17

[Dave Miller [:justdave]]

Joel: that's bug 38862.

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 135141 [details] [diff] [review]
Patch v.2

> Index: attachment.cgi
> @@ -423,6 +423,40 @@
> +  $contenttype =~ s/^\s+//;
> +  $contenttype =~ s/\s+$//;

Don't we have a trim() function in Bugzilla::Util that does this?

> +  # We assume we can view all text and image types  
> +  if ($contenttype =~ /^(text|image)\//) {
> +    return 1;
> +  }

This is probably a bad assumption.  For text it might be okay, but there's a
lot of types of images that can't be viewed in an iframe on a lot of browsers. 
Consider image/svg+xml on a non-svg-enabled browser.  It'll probably want to
download that.	Since just about every browser explicitly lists all the image
types they can do, we can probably just use the Accept header to look for image
types.	Do plugins (think QuickTime) add things to the Accept: header?

The rest looks good.

Comment 19

[Gervase Markham [:gerv]]

> Since just about every browser explicitly lists all the image types they can do,

That's sadly not true. For example, Mozilla doesn't advertise image/bmp. I can
see this generalisation could be a problem, but automatically attempting to
render image/* is the current behaviour, and doesn't seem to have caused
problems so far.

> Do plugins (think QuickTime) add things to the Accept: header?

I don't think so, no. At least, not in Mozilla.

Gerv

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 135141 [details] [diff] [review]
Patch v.2

OK, I'll give on that, since we're not changing it from the existing behavior. 
The trim() thing is a nit...  r= with that fixed

Comment 21

[Christian Reis]

Comment on attachment 135141 [details] [diff] [review]
Patch v.2

>Index: template/en/default/attachment/edit.html.tmpl
%]&amp;action=view">Download the attachment instead</a>.
>+            Attachment cannot be viewed because its MIME type 
>+            ([% contenttype FILTER html %]) is not one that your browser is 
>+            able to display.
>+          </b></p>

Maybe "... is not viewable in your browser." ?

Comment 22

[Gervase Markham [:gerv]]

Fixed.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.49; previous revision: 1.48
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.21; previous revision: 1.20
done

Gerv

Comment 23

[Dave Miller [:justdave]]

*** Bug 226030 has been marked as a duplicate of this bug. ***

Comment 24

[Frédéric Buclin]

*** Bug 217449 has been marked as a duplicate of this bug. ***