Content-Disposition parser does not require presence of ";" between params

RESOLVED FIXED in mozilla16

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Julian Reschke, Assigned: Julian Reschke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Apparently, the parser doesn't require ";" between parameters.

(Testcase to follow)
Julian,  what's your proposal?  That we keep the current behavior (and restore test coverage for it), or no longer permit it?   The latter is of course a change on the web, so we'd want to make sure we don't break anything unless we think it's merited.
(In reply to Jason Duell (:jduell) from comment #1)
> Julian,  what's your proposal?  That we keep the current behavior (and
> restore test coverage for it), or no longer permit it?   The latter is of
> course a change on the web, so we'd want to make sure we don't break
> anything unless we think it's merited.

We should follow the spec, or recommend that the spec be changed if following the spec would break the web.

My understanding is that there are other bugs with the parsing of this header and/or similar headers. I would rather have a patch that implements a "correct" parser in a sane way, that may be useful for headers with similar syntax, than a quick fix for this particular bug.
(Assignee)

Comment 3

6 years ago
Optimally, we fix the bug. 

Will write a test case and compare with other UAs. We also need to keep in mind that the parser is used for other things, such as Content-Type within HTML.

Mid-term, we should write a robust header field tokenizer and use it everywhere.
(Assignee)

Comment 4

5 years ago
(In reply to Julian Reschke from comment #0)
> Apparently, the parser doesn't require ";" between parameters.
> 
> (Testcase to follow)

http://greenbytes.de/tech/tc2231/#attmissingdelim
(Assignee)

Comment 5

5 years ago
Created attachment 631277 [details] [diff] [review]
Proposed patch, incl test case

1) Modifies the parser to stop processing once a semicolon is missing *between* parameters (this aligns the behavior with other UAs, see <http://greenbytes.de/tech/tc2231/#attmissingdelim>)

2) Does not modify the behavior for space-in-token; see <http://greenbytes.de/tech/tc2231/#attmissingdelim2>; Firefox behavior is strictly better than the one observed in other browsers.

3) Does not modify the behavior for semicolon missing between disposition type and parameters, see <http://greenbytes.de/tech/tc2231/#attmissingdelim3>; will treat this as a separate bug.

4) Updates the test cases related to this bug, and fixes another one where the delimiter was indeed missing.
Assignee: nobody → julian.reschke
Attachment #631277 - Flags: review?(jduell.mcbugs)
Comment on attachment 631277 [details] [diff] [review]
Proposed patch, incl test case

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

Looks good, and Try is green:

  https://tbpl.mozilla.org/?tree=Try&rev=ebb09a8c6538

::: netwerk/test/unit/test_MIME_params.js
@@ +397,4 @@
>     "attachment", "basic"],
>  
>    // Bug 732369: Content-Disposition parser does not require presence of ";" between params
> +  // optimally, this would no even return the disposition type "attachment"  

s/no/not/
Attachment #631277 - Flags: review?(jduell.mcbugs) → review+
thanks Julian!

 https://hg.mozilla.org/integration/mozilla-inbound/rev/fce24c917529
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/fce24c917529
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.