librarian trusts user browser supplied mime type for bug attachments (can set type to text/html though it should be text/plain)

The librarian is serving those attachments with the mime type it was told to. So we need to work out why it was uploaded as text/html in the first place.

What bug is that file attached to?

it's bug #34668

The mimetype is easily changeable by clicking the "edit" link in the attachments portlet on the right, which takes you to https://launchpad.net/distros/ubuntu/+source/clisp/+bug/34668/attachments/7592.

I think the bug here is that the +addattachment page doesn't ask for a mimetype, so has to rely on what the browser guesses the file's mimetype is. This is a Malone bug, rather than a Librarian bug.

Either we should ask for a mime type when adding the attachment, or we should improve the guessing of the mime type.

Just a note; I've run into this a few times when people attach a ".debdiff" file.

I see this fairly regularly as well. It just happened with the logfile attachment in bug 126797.

Is the file type provided by the browser or guessed by launchpad?

We trust the mime type sent by the browser. So at the moment it is garbage in, garbage out.

If it is only a case of text/plain being sent as text/html, can we special case this in the Librarian? I can't think of a use case where we *want* to store HTML in the Librarian and have it served up as HTML. So we can make the Librarian serve HTML mime types as text/plain or better yet store them in the database as text/plain on upload. I think we need to do this one day anyway, as if the Librarian starts doing authentication it will become a source of attacks.

On Tue, Aug 21, 2007 at 04:04:52AM -0000, Stuart Bishop wrote:
> We trust the mime type sent by the browser. So at the moment it is
> garbage in, garbage out.

I wonder what's confusing Firefox, then. These files don't look like HTML
to file(1), for example.

Alexander, can you tell us how the detection works?

> If it is only a case of text/plain being sent as text/html, can we
> special case this in the Librarian? I can't think of a use case where we
> *want* to store HTML in the Librarian and have it served up as HTML. So
> we can make the Librarian serve HTML mime types as text/plain or better
> yet store them in the database as text/plain on upload. I think we need
> to do this one day anyway, as if the Librarian starts doing
> authentication it will become a source of attacks.

It seems like it should be possible to store HTML in the librarian and
record it as such, though I agree that it doesn't make sense to serve it
that way.

--
 - mdz

On Tue, Aug 21, 2007 at 08:36:45AM +0100, Matt Zimmerman wrote:
> On Tue, Aug 21, 2007 at 04:04:52AM -0000, Stuart Bishop wrote:
> > We trust the mime type sent by the browser. So at the moment it is
> > garbage in, garbage out.
>
> I wonder what's confusing Firefox, then. These files don't look like HTML
> to file(1), for example.
>
> Alexander, can you tell us how the detection works?

I think its a mix from file-extension and file-introspection .... for
details I would have to investigate ... which I didn't because read
below ...

>
> > If it is only a case of text/plain being sent as text/html, can we
> > special case this in the Librarian? I can't think of a use case where we
> > *want* to store HTML in the Librarian and have it served up as HTML. So
> > we can make the Librarian serve HTML mime types as text/plain or better
> > yet store them in the database as text/plain on upload. I think we need
> > to do this one day anyway, as if the Librarian starts doing
> > authentication it will become a source of attacks.
>

It looks like that this is not true. What I did: I downloaded your
gdm.log from bug 126797 ... uploaded the .log file to some random test
bug 121740 and captured the headers.

And see: launchpad thinks its text/html ... but the headers log reveals
that firefox sends the gdm.log mime-part as text/x-log (e.g. no
text/html) ... so I suspect that launchpad falls back to text/html if
it doesn't know about the mimetype send by the browser (however just
guessing).

FYI, attached the captured header log.

> It seems like it should be possible to store HTML in the librarian and
> record it as such, though I agree that it doesn't make sense to serve it
> that way.
>

IMHO, all mime-types in the text/* hierachy should just be served as
text/plain ... I might miss some corner cases where we want
something different though.

 - Alexander

 �c��

The debdiff case was dealt with recently in bug 229040, but it looks like a more general fix is necessary. The problem, afaict, is in the `zope.app.content_types.text_type` function, which does some really awful guessing:

def text_type(s):
    s = s.strip()
    # Yuk. See if we can figure out the type by content.
    if s.lower().startswith('<html>') or '</' in s:
        return 'text/html'
    elif s.startswith('<?xml'):
        return 'text/xml'
    else:
        return 'text/plain'

We could call `z.a.content_types.guess_content_type` with an explicit default, then `text_type` will never be called. Some people may rely on the fact that real XML and HTML files are currently detected, so a refined version of `text_type` may be needed instead.

On Mon, Jul 07, 2008 at 10:14:04AM -0000, Gavin Panella wrote:
> The debdiff case was dealt with recently in bug 229040, but it looks
> like a more general fix is necessary. The problem, afaict, is in the
> `zope.app.content_types.text_type` function, which does some really
> awful guessing:
>
> def text_type(s):
> s = s.strip()
> # Yuk. See if we can figure out the type by content.
> if s.lower().startswith('<html>') or '</' in s:
> return 'text/html'
> elif s.startswith('<?xml'):
> return 'text/xml'
> else:
> return 'text/plain'
>
> We could call `z.a.content_types.guess_content_type` with an explicit
> default, then `text_type` will never be called. Some people may rely on
> the fact that real XML and HTML files are currently detected, so a
> refined version of `text_type` may be needed instead.

I think simply replacing the first test with something a bit less liberal,
e.g.:

s = s.strip().lower()
if (s.startswith('<html') or s.startswith('<!doctype html') or
    s.startswith('<title') or s.startswith('<head') ):

would be fine. FYI, this example code reflects what file(1) does today,
which seems reasonable enough.

A more comprehensive option might be to validate the file using an HTML
parser, which I'm sure is around somewhere in launchpad already.

--
 - mdz

Pushing to next milestone, since python-magic still isn't installed on the servers.

In Hardy, XHTML content is reported as simply text/xml, which breaks a few things. I set about implementing a sniffer for XML files to try and return something more convincing. It works, on Hardy.

However, on Intrepid, the behaviour of things based on libmagic seems to be quite different from Hardy. In particular, bug 285031 is causing serious issues. I'm suspending work on this until I understand the situation more, and until Intrepid moves out of beta.

Content sniffing is -evil-. I'm inclined to wontfix this and say that browsers should supply better metadata. That may be impractical, so we could allow folk to set the mime type explicitly after upload.