Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow quoted empty string MIME type parameter values #79

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 10, 2018

As pointed out in the issue, this is also allowed by RFC 7230 (HTTP).

Tests: ...

Fixes #78.


Preview | Diff

annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 10, 2018
@annevk
Copy link
Member Author

annevk commented Sep 10, 2018

I've informed the bugs linked from #36 (comment) about this proposed change.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes don't seem to pass the test in https://github.com/web-platform-tests/wpt/pull/12925/files. In particular the parsed version still has charset = GBK. This is probably because of the

mimeType’s parameters[parameterName] does not exist

condition which is still present. So I think this is a test bug; the spec changes might still be right.

mimesniff.bs Outdated
@@ -393,7 +393,8 @@ these steps:
<li><p>Append U+003D (=) to <var>serialization</var>.

<li>
<p>If <var>value</var> does not solely contain <a>HTTP token code points</a>:
<p>If <var>value</var> does not solely contain <a>HTTP token code points</a> or is the empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"or value is the empty string" for clarity?

@annevk
Copy link
Member Author

annevk commented Sep 10, 2018

Well, now the charset parameter would be present (containing the quoted empty string value), so why would it be updated with GBK?

@domenic
Copy link
Member

domenic commented Sep 10, 2018

Oh, right, I misread the order. Then I guess the issue must be on the parsing side. I will step through and see if I implemented it wrong or if the algorithm is missing something.

@domenic
Copy link
Member

domenic commented Sep 10, 2018

The bug was in my implementation; I did not account for "soley" working on zero-length strings.

The following tests are now failing:

 ● mime-types.json › x/x;test

    Expected value to equal:
      "x/x"
    Received:
      "x/x;test=\"\""

  ● mime-types.json › x/x;x=

    Expected value to equal:
      "x/x"
    Received:
      "x/x;x=\"\""

  ● mime-types.json › x/x;x=     (x/x;x=\t)

    Expected value to equal:
      "x/x"
    Received:
      "x/x;x=\"\""

Assuming that it's correct to change those tests, then the spec change LGTM.

@annevk
Copy link
Member Author

annevk commented Sep 10, 2018

That does not look correct as none of those inputs contain quotes and therefore should be unchanged. Is your implementation available somewhere?

@domenic
Copy link
Member

domenic commented Sep 10, 2018

jsdom/whatwg-mimetype#12

@domenic
Copy link
Member

domenic commented Sep 10, 2018

What happens is you remove trailing ASCII whitespace so x/x;x=\t becomes x/x;x. Then when we get to 11.7, position is past the end of input, so we skip to 11.8 without ever executing the "continue". Thus we add the empty string as a value since we don't bail out before 11.8.

@bathos
Copy link

bathos commented Sep 10, 2018

@domenic I think you meant "becomes x/x;x=" — I see the path you’re describing though. Would this be better solved with a flag ("set wasQuoted to true"), or is that usually considered undesirable?

@annevk
Copy link
Member Author

annevk commented Sep 11, 2018

@bathos flags are certainly fair game, but I found a way to do it without (and also reduce overall amount of indenting).

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed works, nice!

As pointed out in the issue, this is also allowed by RFC 7230 (HTTP).

Tests: web-platform-tests/wpt#12925.

Fixes #78.
@annevk annevk force-pushed the annevk/mime-type-empty-string-parameter-value branch from ee00986 to dce4b86 Compare September 14, 2018 07:22
@annevk annevk merged commit 190c18a into master Sep 14, 2018
@annevk annevk deleted the annevk/mime-type-empty-string-parameter-value branch September 14, 2018 07:24
annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 14, 2018
domenic added a commit to jsdom/whatwg-mimetype that referenced this pull request Sep 14, 2018
domenic added a commit to jsdom/whatwg-mimetype that referenced this pull request Sep 14, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 18, 2018
…stonly

Automatic update from web-platform-testsMIME Sniffing: quoted empty string

See whatwg/mimesniff#79.

--

wpt-commits: 8a062c149e4581de0cd92cb51f316247e9514e5d
wpt-pr: 12925
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 19, 2018
…stonly

Automatic update from web-platform-testsMIME Sniffing: quoted empty string

See whatwg/mimesniff#79.

--

wpt-commits: 8a062c149e4581de0cd92cb51f316247e9514e5d
wpt-pr: 12925
kisg pushed a commit to paul99/webkit-mips that referenced this pull request Nov 13, 2018
https://bugs.webkit.org/show_bug.cgi?id=191388

Patch by Rob Buis <rbuis@igalia.com> on 2018-11-12
Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Include improved expected test result and updated mime-type test:
whatwg/mimesniff#79

* web-platform-tests/mimesniff/mime-types/charset-parameter.window-expected.txt:
* web-platform-tests/mimesniff/mime-types/parsing.any-expected.txt:
* web-platform-tests/mimesniff/mime-types/parsing.any.worker-expected.txt:
* web-platform-tests/mimesniff/mime-types/resources/mime-types.json:
* web-platform-tests/xhr/overridemimetype-blob-expected.txt:

Source/WebCore:

According to RFC 2045 and https://mimesniff.spec.whatwg.org/#parsing-a-mime-type empty
quoted strings are acceptable for Content-Type parameter values. They
are accepted by Firefox and Chrome implementations as well.

Test: web-platform-tests/xhr/overridemimetype-blob.html

* platform/network/ParsedContentType.cpp:
(WebCore::parseToken):
(WebCore::parseQuotedString):
(WebCore::parseContentType):
* platform/network/ParsedContentType.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@238124 268f45cc-cd09-0410-ab3c-d52691b4dbfc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…stonly

Automatic update from web-platform-testsMIME Sniffing: quoted empty string

See whatwg/mimesniff#79.

--

wpt-commits: 8a062c149e4581de0cd92cb51f316247e9514e5d
wpt-pr: 12925

UltraBlame original commit: 2f175e80c76f64ac22bf09d3e02b6ebcab13c7ca
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…stonly

Automatic update from web-platform-testsMIME Sniffing: quoted empty string

See whatwg/mimesniff#79.

--

wpt-commits: 8a062c149e4581de0cd92cb51f316247e9514e5d
wpt-pr: 12925

UltraBlame original commit: 2f175e80c76f64ac22bf09d3e02b6ebcab13c7ca
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…stonly

Automatic update from web-platform-testsMIME Sniffing: quoted empty string

See whatwg/mimesniff#79.

--

wpt-commits: 8a062c149e4581de0cd92cb51f316247e9514e5d
wpt-pr: 12925

UltraBlame original commit: 2f175e80c76f64ac22bf09d3e02b6ebcab13c7ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants