FileChooserSettings::acceptMIMETypes contains MIME type strings in wrong format. We had better drop invalid MIME type strings.
Created attachment 110981 [details] Patch
Comment on attachment 110981 [details] Patch Nice and simple! I'm glad you were able to reuse some existing code for this.
Comment on attachment 110981 [details] Patch Thank you for midnight quick review!
Comment on attachment 110981 [details] Patch Attachment 110981 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10067454
Created attachment 110985 [details] Patch for landing
Created attachment 110990 [details] Patch for landing 2 Windows build fix
RFC 2616 HTTP/1.1 Media Types Section and relevant parts of the grammar: http://tools.ietf.org/html/rfc2616#section-3.7 SP = <US-ASCII SP, space (32)> HT = <US-ASCII HT, horizontal-tab (9)> CTL = <any US-ASCII control character (octets 0 - 31) and DEL (127)> token = 1*<any CHAR except CTLs or separators> separators = "(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "\" | <"> | "/" | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT media-type = type "/" subtype *( ";" parameter ) type = token subtype = token It looks like isTokenCharacter may be a little generous in validating a MIME type: bool ContentTypeParser::isTokenCharacter(UChar c) { return isASCII(c) && c > ' ' && c != '"' && c != '(' && c != ')' && c != ',' && c != '/' && (c < ':' || c > '@') && (c < '[' || c > ']'); } It looks to me like this would allow some non-token characters, like: '{' (separator 0x7B) '}' (separator 0x7D) DEL (CTL 0x7F) But I don't know how important this would be to fix. And maybe I'm actually missing something and this is okay.
Comment on attachment 110990 [details] Patch for landing 2 Attachment 110990 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10076816
(In reply to comment #7) > RFC 2616 HTTP/1.1 Media Types Section and relevant parts of the grammar: > http://tools.ietf.org/html/rfc2616#section-3.7 > > SP = <US-ASCII SP, space (32)> > HT = <US-ASCII HT, horizontal-tab (9)> > CTL = <any US-ASCII control character (octets 0 - 31) and DEL (127)> > token = 1*<any CHAR except CTLs or separators> > separators = "(" | ")" | "<" | ">" | "@" > | "," | ";" | ":" | "\" | <"> > | "/" | "[" | "]" | "?" | "=" > | "{" | "}" | SP | HT > > media-type = type "/" subtype *( ";" parameter ) > type = token > subtype = token > > It looks like isTokenCharacter may be a little generous in validating a MIME type: > > bool ContentTypeParser::isTokenCharacter(UChar c) > { > return isASCII(c) && c > ' ' && c != '"' && c != '(' && c != ')' && c != ',' && c != '/' && (c < ':' || c > '@') && (c < '[' || c > ']'); > } > > It looks to me like this would allow some non-token characters, like: > > '{' (separator 0x7B) > '}' (separator 0x7D) > DEL (CTL 0x7F) > > But I don't know how important this would be to fix. And maybe I'm > actually missing something and this is okay. It seems RFC 2045 and RFC 2616 are not compatible. From RFC 2045: token := 1*<any (US-ASCII) CHAR except SPACE, CTLs, or tspecials> tspecials := "(" / ")" / "<" / ">" / "@" / "," / ";" / ":" / "\" / <"> "/" / "[" / "]" / "?" / "=" ; Must be in quoted-string, ; to use within parameter values I'll update the patch so that it doesn't use ContentTypeParser.
Created attachment 111406 [details] Patch 2 Do not use ContentTypeParser
Comment on attachment 111406 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=111406&action=review > Source/WebCore/html/HTMLInputElement.cpp:1310 > +static bool isRFC2616TokenCharacter(UChar ch) Since this function is used only in one place, it would be better to mark it inline for compilers that aren’t as aggressive about inlining such things. > Source/WebCore/html/HTMLInputElement.cpp:1312 > + return isASCII(ch) && ch > ' ' && ch != '"' && ch != '(' && ch != ')' && ch != ',' && ch != '/' && (ch < ':' || ch > '@') && (ch < '[' || ch > ']') && ch != '{' && ch != '}' && ch != 0x7f; If there is a speed issue at all, a 128-byte array is a much faster way to implement this after the isASCII test. > Source/WebCore/html/HTMLInputElement.cpp:1334 > + if (j != slashPosition && !isRFC2616TokenCharacter(trimmedMimeType[j])) { You would get slightly faster performance if you reversed the && here.
Comment on attachment 111406 [details] Patch 2 I also think it would be nice to put the bulk of the HTMLInputElement::acceptMIMETypes changes into a new function, isValidMIMEType(), but that is totally up to you. I think it would be much easier to read, and someone later on might look for MIME type validation and find the function.
Comment on attachment 111406 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=111406&action=review Darrin, Joseph, thank you for the comments. I'll follow your comments except the following one, and land the patch. >> Source/WebCore/html/HTMLInputElement.cpp:1312 >> + return isASCII(ch) && ch > ' ' && ch != '"' && ch != '(' && ch != ')' && ch != ',' && ch != '/' && (ch < ':' || ch > '@') && (ch < '[' || ch > ']') && ch != '{' && ch != '}' && ch != 0x7f; > > If there is a speed issue at all, a 128-byte array is a much faster way to implement this after the isASCII test. acceptMIMETypes() is called once only when a file chooser dialog is opened. I think the current code is enough.
Landed: http://trac.webkit.org/changeset/97918