1276820 - bmoattachments.org sends malformed content-type header

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Michael[tm] Smith [:sideshowbarker]]

https://bugzilla.mozilla.org/attachment.cgi?id=8758032&action=edit just has a literal "text/html" as the content-type, but https://bug1276161.bmoattachments.org/attachment.cgi?id=8758032 serves that attachment with a 'Content-Type:text/html; name="canvas-dashed-line.html"; charset=' header (that is, with an invalid name= param added, and with an empty charset= param).

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Confirm this. Empty value for parameter here is invalid per HTTP spec.

Comment 2

[David Lawrence [:dkl]]

Attachment: 1276820_1.patch

We provide the empty charset= due to not knowing whether the attachment content is utf8 or not and also we add it to force Apache not to add it directly.

The rest I backported from upstream trunk which will fix the issue with the filename being added to content-type incorrectly.

dkl

Comment 3

[Michael[tm] Smith [:sideshowbarker]]

Please consider at least doing this instead:

  Content-Type: text/html; charset=''

That is, with an empty quoted string as the charset parameter value instead of no value at all.

> We provide the empty charset=

Are you sure the way it's being done currently is actually conformant syntax?

As Xidorn noted in comment #1 it seems like it might not be.

The relevant part of the HTTP spec, https://tools.ietf.org/html/rfc7231#section-3.1.1.2 says:

  parameter      = token "=" ( token / quoted-string )

...and in https://tools.ietf.org/html/rfc7230#section-3.2.6 says:

  token          = 1*tchar
  tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                 / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                 / DIGIT / ALPHA
                 ; any VCHAR, except delimiters

So if I'm reading that right, for HTTP, if a media-type parameter is specified, at least one character must be specified for its value. Or maybe you can specify what is effectively the empty string but if so it's only allowed if it's quoted; that is:

  Content-Type: text/html; charset=''

> due to not knowing whether the attachment content is utf8 or not

If it’s not known to be utf8, then it’d seem the right way to convey that is to drop the charset param completely.

> and also we add it to force Apache not to add it directly

If Apache is adding it directly, that seems like something broken in Apache that should be fixed or patched rather than having the system send a broken/malformed HTTP header in every response.

Comment 4

[David Lawrence [:dkl]]

Attachment: 1276820_2.patch

(In reply to Michael[tm] Smith from comment #3)
> So if I'm reading that right, for HTTP, if a media-type parameter is
> specified, at least one character must be specified for its value. Or maybe
> you can specify what is effectively the empty string but if so it's only
> allowed if it's quoted; that is:
> 
>   Content-Type: text/html; charset=''

fair enough. new patch with charset=''.

dkl

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8759302 [details] [diff] [review]
1276820_2.patch

Review of attachment 8759302 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 6

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   acba90e..557410f  master -> master