Closed Bug 1276820 Opened 4 years ago Closed 3 years ago

bmoattachments.org sends malformed content-type header

Categories

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

Production
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mike, Assigned: dkl)

References

()

Details

Attachments

(2 files)

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).
Confirm this. Empty value for parameter here is invalid per HTTP spec.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: attach-and-request → nobody
Component: Attachments & Requests → General
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Version: unspecified → Production
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Attached patch 1276820_1.patchSplinter Review
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
Attachment #8758411 - Flags: review?(dylan)
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.
Attached patch 1276820_2.patchSplinter Review
(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
Attachment #8759302 - Flags: review?(dylan)
Attachment #8758411 - Flags: review?(dylan)
Comment on attachment 8759302 [details] [diff] [review]
1276820_2.patch

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

r=dylan
Attachment #8759302 - Flags: review?(dylan) → review+
To https://github.com/mozilla-bteam/bmo.git
   acba90e..557410f  master -> master
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
See Also: → 1297243
You need to log in before you can comment on or make changes to this bug.