6452 2006-01-09 08:54:03 -0800 KURL::appendEscapingBadChars() doesn't know about %u-escaping. 2019-02-06 09:03:07 -0800 1 1 1 Unclassified WebKit DOM 420+ Mac OS X 10.4 RESOLVED FIXED P2 Normal --- 1 grubba webkit-unassigned ap cdumez oldest_to_newest 28056 0 grubba 2006-01-09 08:54:03 -0800 Sending a %u-escaped string (eg as generated by escape() for unicode data) to XMLHttpRequest::open() will cause any %u's to be escaped an extra time (ie the webserver will receive %25u2014 instead of %u2014). The likely culprit seems to be WebCore/kwq/KWQKURL.mm::appendEscapingBadChars(), which doesn't know about %u-style escaping. A patch similar to the following ought to fix the problem: --- KWQKURL.mm.orig Mon Jan 9 17:43:02 2006 +++ KWQKURL.mm Mon Jan 9 17:46:04 2006 @@ -981,6 +981,16 @@ *p++ = c; *p++ = *str++; *p++ = *str++; + } else if (c == '%' && strEnd - str >= 5 && + (str[0] == 'u' || str[0] == 'U') && + isHexDigit(str[0]) && isHexDigit(str[1]) && + isHexDigit(str[2]) && isHexDigit(str[3])) { + *p++ = c; + *p++ = *str++; + *p++ = *str++; + *p++ = *str++; + *p++ = *str++; + *p++ = *str++; } else if (c == '?') { *p++ = c; } else { 28130 1 ap 2006-01-10 05:14:22 -0800 Do you have a test case that behaves differently in Safari and Firefox (or Internet Explorer)? The problem is not entirely clear to me from the description, sorry. 28134 2 grubba 2006-01-10 05:47:26 -0800 I've verified that my javascript code works as expected in Firefox 1.5. It's hard to do a portable test case, since the typical use is in AJAX code, but here's an extract: index.js: var isSafari = navigator.appVersion.match("AppleWebKit"); function xmlhttpRequest(filepath, callback) { var xmlhttp; if(window.XMLHttpRequest) { // Mozilla, Safari, ... xmlhttp = new XMLHttpRequest(); } else if (window.ActiveXObject) { // IE xmlhttp = new ActiveXObject("Microsoft.XMLHTTP"); } xmlhttp.open("GET", filepath, true); xmlhttp.onreadystatechange = function() { var value = callback(xmlhttp); return value; }; xmlhttp.send(null) } function getResponseXML(xmlhttp) { var doc = document.importNode(xmlhttp.responseXML.documentElement, true); if(isSafari) { // Safari doesn't initialise all node attributes (name, class etc.) // properly without this workaround. doc.innerHTML = doc.innerHTML; } return doc; } function editGetFields(tag_list, data) { for(var i = 0; i < tag_list.length; i++) { var name = tag_list.item(i).getAttribute("name"); if(name) { data[name] = tag_list.item(i).value; } } } function editTextSave(button, close) { var div_edit = document.getElementById("document-editor"); var document_id = div_edit.getAttribute("name"); var article_id = getArticleId(div_edit); var article_tr = document.getElementById("article-open-" + article_id); var data = new Object; editGetFields(div_edit.getElementsByTagName("input"), data); editGetFields(div_edit.getElementsByTagName("textarea"), data); var callback = function(xmlhttp) { if(xmlhttp.readyState != 4) return; var table_db = getResponseXML(xmlhttp); var tr_db = table_db.getElementsByTagName("tr").item(0); article_tr.parentNode.replaceChild(tr_db, article_tr); if(close) { displayArticleList(); } } xmlhttpRequest("actions/edit-text-save.xml?" + "article_id="+escape(article_id) + "&" + "document_id="+escape(document_id) + "&" + "finished="+escape(data.finished) + "&" + "headline="+escape(data.headline) + "&" + "subheadline="+escape(data.subheadline) + "&" + "keywords="+escape(data.keywords) + "&" + "blurb="+escape(data.blurb) + "&" + "byline="+escape(data.byline) + "&" + "notes="+escape(data.notes) + "&" + "story="+escape(data.story), callback); return false; } If one of data.x contains a unicode character outside ISO-8859-1 (like eg \u2014 (EM_DASH)), escape() above will encode it with %u-style encoding (eg %u2014). This is as expected. However when the resulting request is received by the http server the DWIM in XMLHttpRequest::open() will have quoted it an extra time (to eg %25u2014) when sent by Safari: ceylon.roxen.com - - [09/Jan/2006:17:15:44 +0100] "GET /es/actions/edit-text-sav e.xml?article_id=441&document_id=324&finished=on&headline=Katastrofhj%E4lp%20_%2 0ny%20exportsatsning&subheadline=&keywords=&blurb=&byline=&notes=&story=%E5%E4%F 6%20%25uACA1%25uAC94%25uAC84%0A%0AKatastrofhj%E4lp%20%25u2014%20ny%20exportsatsn ing%20%E5%E4%F6%20%BFSpanska%20fr%E5gor%3F%2075%20%25u2031%20%25u260E%0A%0AStora %20katastrofer%20skapar%20lidande%20och%20n%F6d%20%25u2014%20men%20ocks%E5%20exp [...] Note that there are several cases of double-escaped unicode characters above. But not with Firefox: shipon.roxen.com - - [09/Jan/2006:16:23:33 +0100] "GET /es/actions/edit-text-sav e.xml?article_id=441&document_id=324&finished=on&headline=Katastrofhj%E4lp%20_%2 0ny%20exportsatsning&subheadline=&keywords=&blurb=&byline=&notes=&story=%u6CE8%u 6587%u65B9%u6CD5%u304C%u3072%u3068%u76EE%u3067%u308F%u304B%u308B%u30AC%u30A4%u30 C9%u30C4%u30A2%u30FC%u306F%u3053%u3061%u3089%u3002%0A%u260E%20%E5%E4%F6%2075%20% [...] Above unicode characters have been escaped a single time (as expected). 28162 3 ap 2006-01-10 09:52:55 -0800 Yes, I can see this with a simple test html and tcpflow. Would you like to submit the patch for review (by adding ChangeLog notes, attaching the patch as a file and setting a review flag)? 28163 4 5599 grubba 2006-01-10 10:01:50 -0800 Created attachment 5599 Suggested patch for KWQKURL.mm::appendEscapingBadChars() 28166 5 5600 grubba 2006-01-10 10:15:39 -0800 Created attachment 5600 Now with ChangeLog entry. 28169 6 ap 2006-01-10 10:33:06 -0800 My test case: <http://nypop.com/~ap/webkit/xmlhttp-url-encoding.html> (too lazy to make one that doesn't require tcpflow, and no idea about how to make an automated one). 28194 7 5599 darin 2006-01-10 15:08:06 -0800 Comment on attachment 5599 Suggested patch for KWQKURL.mm::appendEscapingBadChars() First, thanks for submitting this patch. It seems quite important to fix this bug! But I don't think this fix is correct. I think we should either be escaping all % characters in the string or not escaping any. It doesn't seem right to decide whether to escape the % character based on whether it's followed by u201F (for example) or not. If we shouldn't be escaping "%" characters in this case, then we need to change KURL behavior, perhaps adding a parameter if we don't always want that behavior. 28217 8 5600 darin 2006-01-10 19:49:46 -0800 Comment on attachment 5600 Now with ChangeLog entry. OK, but my review comment stands (see comment 7 above). I don't think this is the correct fix. 28231 9 grubba 2006-01-11 01:39:04 -0800 I agree that having DWIM's like appendEscapingBadChars() is generally a bad idea to begin with, but now that it's there, it should either be fixed or removed entirely (which might break code that doesn't perform escaping properly, and thus relies on it being there). The alternative of escaping all "%" characters would break compatibility with eg Firefox even more. 28232 10 ap 2006-01-11 05:05:32 -0800 I have updated the test case a bit to show more different cases. In the test, both MSIE and Firefox never escape % characters (MSIE doesn't even escape non-ASCII characters, actually). Looks like the DWIM code first appeared in a massive KURL rewrite between revisions 1905 and 1906; so it's not entirely obvious if it serves any purpose. 32499 11 6486 ap 2006-02-14 12:21:52 -0800 Created attachment 6486 don't escape % characters I don't really see how to check all code paths that end up in appendEscapingBadChars()... There is a regression test included in the patch, and "everything else" still seems to work fine - which is admittedly less than a perfect evidence. 32541 12 6486 mjs 2006-02-14 18:05:24 -0800 Comment on attachment 6486 don't escape % characters Based on your test case I think never escaping % is the right thing. I think I am the one who did the rewrite and I didn't have a specific reason for escaping %. r=me 57107 13 brettw 2007-09-26 13:25:54 -0700 Note that IE7 changes this behavior. It rejects all invalid escape sequences and will not load the URL. This includes the %uXXXX form. 1502907 14 lforschler 2019-02-06 09:03:07 -0800 Mass moving XML DOM bugs to the "DOM" Component. 5599 2006-01-10 10:01:50 -0800 2006-01-10 15:08:06 -0800 Suggested patch for KWQKURL.mm::appendEscapingBadChars() KWQKURL.mm.patch text/plain 631 grubba LS0tIEtXUUtVUkwubW0ub3JpZwlUdWUgSmFuIDEwIDE4OjU2OjM3IDIwMDYKKysrIEtXUUtVUkwu bW0JVHVlIEphbiAxMCAxODo1ODozOCAyMDA2CkBAIC05ODEsNiArOTgxLDE2IEBACiAgICAgICAg ICAgICAgICAgKnArKyA9IGM7CiAgICAgICAgICAgICAgICAgKnArKyA9ICpzdHIrKzsKICAgICAg ICAgICAgICAgICAqcCsrID0gKnN0cisrOworCSAgICB9IGVsc2UgaWYgKGMgPT0gJyUnICYmIHN0 ckVuZCAtIHN0ciA+PSA1ICYmCisJCSAgICAgICAoc3RyWzBdID09ICd1JyB8fCBzdHJbMF0gPT0g J1UnKSAmJgorCQkgICAgICAgaXNIZXhEaWdpdChzdHJbMF0pICYmIGlzSGV4RGlnaXQoc3RyWzFd KSAmJgorCQkgICAgICAgaXNIZXhEaWdpdChzdHJbMl0pICYmIGlzSGV4RGlnaXQoc3RyWzNdKSkg eworCSAgICAgICAgKnArKyA9IGM7CisJICAgICAgICAqcCsrID0gKnN0cisrOworCSAgICAgICAg KnArKyA9ICpzdHIrKzsKKwkgICAgICAgICpwKysgPSAqc3RyKys7CisJICAgICAgICAqcCsrID0g KnN0cisrOworCSAgICAgICAgKnArKyA9ICpzdHIrKzsKICAgICAgICAgICAgIH0gZWxzZSBpZiAo YyA9PSAnPycpIHsKICAgICAgICAgICAgICAgICAqcCsrID0gYzsKICAgICAgICAgICAgIH0gZWxz ZSB7Cg== 5600 2006-01-10 10:15:39 -0800 2006-02-14 12:21:52 -0800 Now with ChangeLog entry. WebCore.patch text/plain 1062 grubba LS0tIFdlYkNvcmUvQ2hhbmdlTG9nLm9yaWcJVHVlIEphbiAxMCAxOTowNzowOSAyMDA2CisrKyBX ZWJDb3JlL0NoYW5nZUxvZwlUdWUgSmFuIDEwIDE5OjEwOjQ0IDIwMDYKQEAgLTEsMyArMSw5IEBA CisyMDA2LTAxLTEwICBIZW5yaWsgR3J1YmJzdHL2bSAgPGdydWJiYUBncnViYmEub3JnPgorCisJ KiBrd3EvS1dRS1VSTC5tbToKKwlLSU86OmFwcGVuZEVzY2FwaW5nQmFkQ2hhcnMoKSBlcnJvbmVv dXNseSBlc2NhcGVkICV1IHNlcXVlbmNlcyBhbgorCWV4dHJhIHRpbWUgKGh0dHA6Ly9idWd6aWxs YS5vcGVuZGFyd2luLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjQ1MikuCisKIDIwMDYtMDEtMDcgIEFu ZGVycyBDYXJsc3NvbiAgPGFuZGVyc2NhQG1hYy5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkg TWFjaWVqLgotLS0gV2ViQ29yZS9rd3EvS1dRS1VSTC5tbS5vcmlnCVR1ZSBKYW4gMTAgMTg6NTY6 MzcgMjAwNgorKysgV2ViQ29yZS9rd3EvS1dRS1VSTC5tbQlUdWUgSmFuIDEwIDE4OjU4OjM4IDIw MDYKQEAgLTk4MSw2ICs5ODEsMTYgQEAKICAgICAgICAgICAgICAgICAqcCsrID0gYzsKICAgICAg ICAgICAgICAgICAqcCsrID0gKnN0cisrOwogICAgICAgICAgICAgICAgICpwKysgPSAqc3RyKys7 CisJICAgIH0gZWxzZSBpZiAoYyA9PSAnJScgJiYgc3RyRW5kIC0gc3RyID49IDUgJiYKKwkJICAg ICAgIChzdHJbMF0gPT0gJ3UnIHx8IHN0clswXSA9PSAnVScpICYmCisJCSAgICAgICBpc0hleERp Z2l0KHN0clswXSkgJiYgaXNIZXhEaWdpdChzdHJbMV0pICYmCisJCSAgICAgICBpc0hleERpZ2l0 KHN0clsyXSkgJiYgaXNIZXhEaWdpdChzdHJbM10pKSB7CisJICAgICAgICAqcCsrID0gYzsKKwkg ICAgICAgICpwKysgPSAqc3RyKys7CisJICAgICAgICAqcCsrID0gKnN0cisrOworCSAgICAgICAg KnArKyA9ICpzdHIrKzsKKwkgICAgICAgICpwKysgPSAqc3RyKys7CisJICAgICAgICAqcCsrID0g KnN0cisrOwogICAgICAgICAgICAgfSBlbHNlIGlmIChjID09ICc/JykgewogICAgICAgICAgICAg ICAgICpwKysgPSBjOwogICAgICAgICAgICAgfSBlbHNlIHsK 6486 2006-02-14 12:21:52 -0800 2006-02-14 18:05:24 -0800 don't escape % characters 6452r2_patch.txt text/plain 2531 ap SW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZW5jb2RpbmcvcGVyY2VudC1lc2NhcGluZy5odG1sCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZW5jb2RpbmcvcGVyY2VudC1lc2NhcGluZy5o dG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFzdC9lbmNvZGluZy9wZXJjZW50LWVz Y2FwaW5nLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMjMgQEAKKzxodG1sPgorPGhlYWQ+ Cis8bWV0YSBodHRwLWVxdWl2PSJjb250ZW50LXR5cGUiIGNvbnRlbnQ9InRleHQvaHRtbDsgY2hh cnNldD11dGYtOCI+Cis8dGl0bGU+UGVyY2VudCBlc2NhcGluZzwvdGl0bGU+Cis8L2hlYWQ+Cis8 Ym9keT4KKzxwPlRlc3QgZm9yIDxhIGhyZWY9Imh0dHA6Ly9idWd6aWxsYS5vcGVuZGFyd2luLm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9NjQ1MiI+YnVnIDY0NTI8L2E+IC0gCitLSU86OmFwcGVuZEVzY2Fw aW5nQmFkQ2hhcnMoKSBzaG91bGQgbmV2ZXIgZXNjYXBlIHBlcmNlbnQgY2hhcmFjdGVycy48L3A+ Cis8Zm9ybSBhY3Rpb249J3Jlc291cmNlcy8lMjUyNSV1MDQzNSAwICV4eCUyNSUldWxpa2UuaHRt bCcgbmFtZT1mPgorIDxpbnB1dCB0eXBlPWhpZGRlbiBuYW1lPXEgdmFsdWU9Jyc+Cis8L2Zvcm0+ Cis8c2NyaXB0PgorCisgIGlmICh3aW5kb3cubGF5b3V0VGVzdENvbnRyb2xsZXIpIHsKKyAgICBs YXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7CisgICAgbGF5b3V0VGVzdENvbnRyb2xs ZXIud2FpdFVudGlsRG9uZSgpOworICB9CisKKyAgZG9jdW1lbnQuZi5zdWJtaXQoKTsKKworPC9z Y3JpcHQ+Cis8L2JvZHk+Cis8L2h0bWw+CkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2VuY29kaW5n L3Jlc291cmNlcy8lMjUldTA0MzUgMCAleHglJSV1bGlrZS5odG1sCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExh eW91dFRlc3RzL2Zhc3QvZW5jb2RpbmcvcmVzb3VyY2VzLyUyNSV1MDQzNSAwICV4eCUlJXVsaWtl Lmh0bWwgMCAleHglJSV1bGlrZS5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFz dC9lbmNvZGluZy9yZXNvdXJjZXMvJTI1JXUwNDM1IDAgJXh4JSUldWxpa2UuaHRtbCAwICV4eCUl JXVsaWtlLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTQgQEAKKzxodG1sPgorPGJvZHkg b25sb2FkPSJqYXZhc2NyaXB0OgorICAvLyBXaW5JRSBhbmQgRmlyZWZveCB1c2UgdGhlIHNhbWUg VVJMLCBidXQgaW4gV2luSUUsIGRvY3VtZW50LlVSTCByZXR1cm5zIHVuZXNjYXBlZCB0ZXh0Cisg IGlmIChkb2N1bWVudC5VUkwubWF0Y2goJyUyNS4qaHRtbCcpID09ICclMjUyNSV1MDQzNSUyMDAl MjAleHglMjUlJXVsaWtlLmh0bWwnIHx8IAorICAgICAgZG9jdW1lbnQuVVJMLm1hdGNoKCclMjUu Kmh0bWwnKSA9PSAnJTI1JXUwNDM1IDAgJXh4JSUldWxpa2UuaHRtbCcpCisgICAgIGRvY3VtZW50 LmdldEVsZW1lbnRCeUlkKCdyZXN1bHQnKS5maXJzdENoaWxkLm5vZGVWYWx1ZSA9ICdTVUNDRVNT JzsKKworICBsYXlvdXRUZXN0Q29udHJvbGxlci5ub3RpZnlEb25lKCk7CisiPgorPHA+VGVzdCBm b3IgPGEgaHJlZj0iaHR0cDovL2J1Z3ppbGxhLm9wZW5kYXJ3aW4ub3JnL3Nob3dfYnVnLmNnaT9p ZD02NDUyIj5idWcgNjQ1MjwvYT4gLSAKK0tJTzo6YXBwZW5kRXNjYXBpbmdCYWRDaGFycygpIHNo b3VsZCBuZXZlciBlc2NhcGUgcGVyY2VudCBjaGFyYWN0ZXJzLjwvcD4KKzxwIGlkPXJlc3VsdD5G QUlMVVJFPC9wPgorPC9ib2R5PgorPC9odG1sPgpJbmRleDogV2ViQ29yZS9rd3EvS1dRS1VSTC5t bQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2t3cS9LV1FLVVJMLm1tCShyZXZpc2lvbiAxMjc5MSkK KysrIFdlYkNvcmUva3dxL0tXUUtVUkwubW0JKHdvcmtpbmcgY29weSkKQEAgLTgxOCwxMSArODE4 LDcgQEAgc3RhdGljIHZvaWQgYXBwZW5kRXNjYXBpbmdCYWRDaGFycyhjaGFyKgogICAgIHdoaWxl IChzdHIgPCBzdHJFbmQpIHsKICAgICAgICAgdW5zaWduZWQgY2hhciBjID0gKnN0cisrOwogICAg ICAgICBpZiAoaXNCYWRDaGFyKGMpKSB7Ci0gICAgICAgICAgICBpZiAoYyA9PSAnJScgJiYgc3Ry RW5kIC0gc3RyID49IDIgJiYgaXNIZXhEaWdpdChzdHJbMF0pICYmIGlzSGV4RGlnaXQoc3RyWzFd KSkgewotICAgICAgICAgICAgICAgICpwKysgPSBjOwotICAgICAgICAgICAgICAgICpwKysgPSAq c3RyKys7Ci0gICAgICAgICAgICAgICAgKnArKyA9ICpzdHIrKzsKLSAgICAgICAgICAgIH0gZWxz ZSBpZiAoYyA9PSAnPycpIHsKKyAgICAgICAgICAgIGlmIChjID09ICclJyB8fCBjID09ICc/Jykg ewogICAgICAgICAgICAgICAgICpwKysgPSBjOwogICAgICAgICAgICAgfSBlbHNlIHsKICAgICAg ICAgICAgICAgICAqcCsrID0gJyUnOwo=