// FIXME: This function doesn't comply with RFC 6266. // For example, this function doesn't handle the interaction between " and ; // that arises from quoted-string, nor does this function properly unquote // attribute values. Further this function appears to process parameter names // in a case-sensitive manner. (There are likely other bugs as well.) String filenameFromHTTPContentDisposition(const String& value)
Created attachment 141362 [details] Patch
Comment on attachment 141362 [details] Patch Attachment 141362 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12672251
Comment on attachment 141362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141362&action=review > Source/WebCore/platform/network/HTTPParsers.cpp:222 > + for (int i = 0; i <= value.length(); i++) { > + // Try to find filename in strings which are separated by not-double-quoted ';'. > + if (value[i] == ';' || i == value.length()) { > + // Skip finding filename if there is not a '=' in the string. > + if (equalPos > semicolonPos) { > + size_t keyPos = semicolonPos + 1; > + String key = value.substring(keyPos, equalPos - keyPos).stripWhiteSpace(); > + if (key.lower() == "filename") { > + String filename = value.substring(equalPos + 1, i - equalPos -1).stripWhiteSpace(); > + if (filename[0] == '"' && filename[filename.length() -1] == '"') > + filename = filename.substring(1, filename.length() -2); > + return filename; > + } > + } > + semicolonPos = i; > + } else if (value[i] == '"') { > + // Skip double-quoted ';'. > + size_t secondQuotePos = value.find('"', i + 1); > + if (secondQuotePos != notFound) > + i = secondQuotePos; > + } else if (value[i] == '=' && equalPos <= semicolonPos) // equanlPos points to the first '=' when parsing filename. > + equalPos = i; Seems array bounds overflow. You may access value[value.length()]
Comment on attachment 141362 [details] Patch Attachment 141362 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12663665
Comment on attachment 141362 [details] Patch You need to at least make it compile on other ports. Also have you checked there is not an older WebKit bug tracking this? Have you checked your interpretation of this RFC compliance with other browsers?
Comment on attachment 141362 [details] Patch This is not a correct implementation of this parser. For example, you're not dealing with \ escaping of " in quoted-string.
I would caution reviewers against reviewing patches in this bug without a detailed understanding of the Content-Disposition header. Parsing it correctly is difficult.
Created attachment 141650 [details] Patch
(In reply to comment #5) > (From update of attachment 141362 [details]) > You need to at least make it compile on other ports. Also have you checked there is not an older WebKit bug tracking this? Have you checked your interpretation of this RFC compliance with other browsers? I can't find an older Webkit bug tracking this. Google chrome and safari don't have this problem. Seems they don't use this function.
Comment on attachment 141650 [details] Patch I'm sorry I'm not being more helpful, but your algorithm isn't correct. You might find this implementation to be a useful guide: http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_content_disposition.cc?view=markup Even that implementation isn't 100% correct, but it's much closer than what's in this patch.
We still have this FIXME: https://searchfox.org/wubkat/source/Source/WebCore/platform/network/HTTPParsers.cpp#331