Last Comment Bug 732369 - Content-Disposition parser does not require presence of ";" between params
: Content-Disposition parser does not require presence of ";" between params
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Julian Reschke
:
Mentors:
Depends on:
Blocks: 609667
  Show dependency treegraph
 
Reported: 2012-03-02 04:00 PST by Julian Reschke
Modified: 2012-06-12 18:32 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch, incl test case (1.86 KB, patch)
2012-06-07 22:57 PDT, Julian Reschke
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Julian Reschke 2012-03-02 04:00:55 PST
Apparently, the parser doesn't require ";" between parameters.

(Testcase to follow)
Comment 1 Jason Duell [:jduell] (needinfo me) 2012-03-02 12:12:30 PST
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.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-02 13:51:56 PST
(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.
Comment 3 Julian Reschke 2012-03-02 14:16:28 PST
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.
Comment 4 Julian Reschke 2012-06-07 12:02:51 PDT
(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
Comment 5 Julian Reschke 2012-06-07 22:57:48 PDT
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.
Comment 6 Jason Duell [:jduell] (needinfo me) 2012-06-12 10:54:22 PDT
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/
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-06-12 10:55:19 PDT
thanks Julian!

 https://hg.mozilla.org/integration/mozilla-inbound/rev/fce24c917529
Comment 8 Matt Brubeck (:mbrubeck) 2012-06-12 18:32:12 PDT
https://hg.mozilla.org/mozilla-central/rev/fce24c917529

Note You need to log in before you can comment on or make changes to this bug.