127421 2014-01-22 03:24:13 -0800 [curl] Improve realm string parsing in WWW-Authenticate headers 2014-01-30 10:57:48 -0800 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 davidsz webkit-unassigned bfulgham commit-queue galpeter oldest_to_newest 970652 0 davidsz 2014-01-22 03:24:13 -0800 The realm string contains quotes at the beginning and end - this is the opposite of the libsoup implementation. Furthermore, if the header is concatenated from two or more another headers, it contains more incorrect part. For example, if the header is: WWW-Authenticate: Basic realm="First realm", Basic realm="Second realm" realm string will be: "First realm", Basic realm="Second realm" 970654 1 221852 davidsz 2014-01-22 03:26:58 -0800 Created attachment 221852 Proposed patch 970728 2 221852 bfulgham 2014-01-22 09:54:16 -0800 Comment on attachment 221852 Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=221852&action=review Looks good. I have a couple of minor comments for your consideration. > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:351 > String authHeader = response.httpHeaderField("WWW-Authenticate"); I just noticed that this could probably be const, since we aren't modifying it. > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:358 > + if (realm.startsWith('"') && realm.endsWith('"') && realm.length() > 1) What happens if we get the input ""? Are we supposed to create a protection space for the "" realm? Or should we be bailing out early? > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:359 > + realm = realm.substring(1, realm.length()-2); This might be clearer if it were wrapped up as a little function: static void removeLeadingAndTrailingQuotes(String& value) ... 971090 3 221966 davidsz 2014-01-23 02:36:06 -0800 Created attachment 221966 Proposed patch II. Thanks for your comments! > What happens if we get the input ""? Are we supposed to create a protection space for the "" realm? Or should we be bailing out early? I think we should accept the empty string as realm, because the most popular browsers accept it too. 973805 4 221966 bfulgham 2014-01-30 10:30:09 -0800 Comment on attachment 221966 Proposed patch II. r=me 973820 5 221966 commit-queue 2014-01-30 10:57:46 -0800 Comment on attachment 221966 Proposed patch II. Clearing flags on attachment: 221966 Committed r163091: <http://trac.webkit.org/changeset/163091> 973821 6 commit-queue 2014-01-30 10:57:48 -0800 All reviewed patches have been landed. Closing bug. 221852 2014-01-22 03:26:58 -0800 2014-01-23 03:23:47 -0800 Proposed patch www-auth-parse.diff text/plain 1864 davidsz ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA1MDM3OTkzLi42OGI1ZDViIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTQtMDEtMjIgIFN6YWJvbGNzIERhdmlkICA8ZGF2aWRzekBpbmYudS1zemVnZWQuaHU+ CisKKyAgICAgICAgW2N1cmxdIEltcHJvdmUgcmVhbG0gc3RyaW5nIHBhcnNpbmcgaW4gV1dXLUF1 dGhlbnRpY2F0ZSBoZWFkZXJzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMjc0MjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBUaGUgcmVhbG0gc3RyaW5nIGNvbnRhaW5zIHF1b3RlcyBhdCB0aGUgYmVnaW5u aW5nIGFuZCBlbmQgLSB0aGlzIGlzIHRoZQorICAgICAgICBvcHBvc2l0ZSBvZiB0aGUgbGlic291 cCBpbXBsZW1lbnRhdGlvbi4gRnVydGhlcm1vcmUsIGlmIHRoZSBoZWFkZXIgaXMKKyAgICAgICAg Y29uY2F0ZW5hdGVkIGZyb20gdHdvIG9yIG1vcmUgYW5vdGhlciBoZWFkZXJzLCBpdCBjb250YWlu cyBtb3JlIGluY29ycmVjdCBwYXJ0LgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9jdXJs L1Jlc291cmNlSGFuZGxlTWFuYWdlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpnZXRQcm90ZWN0 aW9uU3BhY2UpOgorCiAyMDE0LTAxLTIyICBNaWhuZWEgT3ZpZGVuaWUgIDxtaWhuZWFAYWRvYmUu Y29tPgogCiAgICAgICAgIFtDU1NSZWdpb25zXSBJbmNvcnJlY3QgbGF5b3V0IG9mIGEgcmVnaW9u IHBzZXVkbyBjaGlsZHJlbgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0 d29yay9jdXJsL1Jlc291cmNlSGFuZGxlTWFuYWdlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9uZXR3b3JrL2N1cmwvUmVzb3VyY2VIYW5kbGVNYW5hZ2VyLmNwcAppbmRleCAzM2Q3ZjQ1 Li5kMGY4NmEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL2N1 cmwvUmVzb3VyY2VIYW5kbGVNYW5hZ2VyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9uZXR3b3JrL2N1cmwvUmVzb3VyY2VIYW5kbGVNYW5hZ2VyLmNwcApAQCAtMzUxLDggKzM1MSwx MyBAQCBzdGF0aWMgYm9vbCBnZXRQcm90ZWN0aW9uU3BhY2UoQ1VSTCogaCwgY29uc3QgUmVzb3Vy Y2VSZXNwb25zZSYgcmVzcG9uc2UsIFByb3RlYwogICAgIFN0cmluZyBhdXRoSGVhZGVyID0gcmVz cG9uc2UuaHR0cEhlYWRlckZpZWxkKCJXV1ctQXV0aGVudGljYXRlIik7CiAgICAgY29uc3QgU3Ry aW5nIHJlYWxtU3RyaW5nID0gInJlYWxtPSI7CiAgICAgaW50IHJlYWxtUG9zID0gYXV0aEhlYWRl ci5maW5kKHJlYWxtU3RyaW5nKTsKLSAgICBpZiAocmVhbG1Qb3MgPiAwKQorICAgIGlmIChyZWFs bVBvcyA+IDApIHsKICAgICAgICAgcmVhbG0gPSBhdXRoSGVhZGVyLnN1YnN0cmluZyhyZWFsbVBv cyArIHJlYWxtU3RyaW5nLmxlbmd0aCgpKTsKKyAgICAgICAgcmVhbG0gPSByZWFsbS5sZWZ0KHJl YWxtLmZpbmQoJywnKSk7CisKKyAgICAgICAgaWYgKHJlYWxtLnN0YXJ0c1dpdGgoJyInKSAmJiBy ZWFsbS5lbmRzV2l0aCgnIicpICYmIHJlYWxtLmxlbmd0aCgpID4gMSkKKyAgICAgICAgICAgIHJl YWxtID0gcmVhbG0uc3Vic3RyaW5nKDEsIHJlYWxtLmxlbmd0aCgpLTIpOworICAgIH0KIAogICAg IFByb3RlY3Rpb25TcGFjZVNlcnZlclR5cGUgc2VydmVyVHlwZSA9IFByb3RlY3Rpb25TcGFjZVNl cnZlckhUVFA7CiAgICAgaWYgKHByb3RvY29sID09ICJodHRwcyIpCg== 221966 2014-01-23 02:36:06 -0800 2014-01-30 10:57:46 -0800 Proposed patch II. www-auth-parse.diff text/plain 2363 davidsz ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3YmU5YTQ2Li41NGQ5NzFmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTQtMDEtMjMgIFN6YWJvbGNzIERhdmlkICA8ZGF2aWRzekBpbmYudS1zemVnZWQuaHU+ CisKKyAgICAgICAgW2N1cmxdIEltcHJvdmUgcmVhbG0gc3RyaW5nIHBhcnNpbmcgaW4gV1dXLUF1 dGhlbnRpY2F0ZSBoZWFkZXJzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMjc0MjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBUaGUgcmVhbG0gc3RyaW5nIGNvbnRhaW5zIHF1b3RlcyBhdCB0aGUgYmVnaW5u aW5nIGFuZCBlbmQgLSB0aGlzIGlzIHRoZQorICAgICAgICBvcHBvc2l0ZSBvZiB0aGUgbGlic291 cCBpbXBsZW1lbnRhdGlvbi4gRnVydGhlcm1vcmUsIGlmIHRoZSBoZWFkZXIgaXMKKyAgICAgICAg Y29uY2F0ZW5hdGVkIGZyb20gdHdvIG9yIG1vcmUgYW5vdGhlciBoZWFkZXJzLCBpdCBjb250YWlu cyBtb3JlIGluY29ycmVjdCBwYXJ0LgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9jdXJs L1Jlc291cmNlSGFuZGxlTWFuYWdlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpyZW1vdmVMZWFk aW5nQW5kVHJhaWxpbmdRdW90ZXMpOgorICAgICAgICAoV2ViQ29yZTo6Z2V0UHJvdGVjdGlvblNw YWNlKToKKwogMjAxNC0wMS0yMyAgTMOhc3psw7MgTGFuZ8OzICA8bGxhbmdvLnUtc3plZ2VkQHBh cnRuZXIuc2Ftc3VuZy5jb20+CiAKICAgICAgICAgUmFuZ2Ugc2hvdWxkIGJlIGNvbnN0cnVjdGFi bGUuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL2N1cmwvUmVz b3VyY2VIYW5kbGVNYW5hZ2VyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsv Y3VybC9SZXNvdXJjZUhhbmRsZU1hbmFnZXIuY3BwCmluZGV4IDEzMjZkZGYuLjNmN2VlN2EgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY3VybC9SZXNvdXJjZUhh bmRsZU1hbmFnZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY3Vy bC9SZXNvdXJjZUhhbmRsZU1hbmFnZXIuY3BwCkBAIC0zMjIsNiArMzIyLDEzIEBAIHN0YXRpYyBi b29sIGlzQXBwZW5kYWJsZUhlYWRlcihjb25zdCBTdHJpbmcgJmtleSkKICAgICByZXR1cm4gZmFs c2U7CiB9CiAKK3N0YXRpYyB2b2lkIHJlbW92ZUxlYWRpbmdBbmRUcmFpbGluZ1F1b3RlcyhTdHJp bmcmIHZhbHVlKQoreworICAgIHVuc2lnbmVkIGxlbmd0aCA9IHZhbHVlLmxlbmd0aCgpOworICAg IGlmICh2YWx1ZS5zdGFydHNXaXRoKCciJykgJiYgdmFsdWUuZW5kc1dpdGgoJyInKSAmJiBsZW5n dGggPiAxKQorICAgICAgICB2YWx1ZSA9IHZhbHVlLnN1YnN0cmluZygxLCBsZW5ndGgtMik7Cit9 CisKIHN0YXRpYyBib29sIGdldFByb3RlY3Rpb25TcGFjZShDVVJMKiBoLCBjb25zdCBSZXNvdXJj ZVJlc3BvbnNlJiByZXNwb25zZSwgUHJvdGVjdGlvblNwYWNlJiBwcm90ZWN0aW9uU3BhY2UpCiB7 CiAgICAgQ1VSTGNvZGUgZXJyOwpAQCAtMzQ4LDExICszNTUsMTQgQEAgc3RhdGljIGJvb2wgZ2V0 UHJvdGVjdGlvblNwYWNlKENVUkwqIGgsIGNvbnN0IFJlc291cmNlUmVzcG9uc2UmIHJlc3BvbnNl LCBQcm90ZWMKIAogICAgIFN0cmluZyByZWFsbTsKIAotICAgIFN0cmluZyBhdXRoSGVhZGVyID0g cmVzcG9uc2UuaHR0cEhlYWRlckZpZWxkKCJXV1ctQXV0aGVudGljYXRlIik7CisgICAgY29uc3Qg U3RyaW5nIGF1dGhIZWFkZXIgPSByZXNwb25zZS5odHRwSGVhZGVyRmllbGQoIldXVy1BdXRoZW50 aWNhdGUiKTsKICAgICBjb25zdCBTdHJpbmcgcmVhbG1TdHJpbmcgPSAicmVhbG09IjsKICAgICBp bnQgcmVhbG1Qb3MgPSBhdXRoSGVhZGVyLmZpbmQocmVhbG1TdHJpbmcpOwotICAgIGlmIChyZWFs bVBvcyA+IDApCisgICAgaWYgKHJlYWxtUG9zID4gMCkgewogICAgICAgICByZWFsbSA9IGF1dGhI ZWFkZXIuc3Vic3RyaW5nKHJlYWxtUG9zICsgcmVhbG1TdHJpbmcubGVuZ3RoKCkpOworICAgICAg ICByZWFsbSA9IHJlYWxtLmxlZnQocmVhbG0uZmluZCgnLCcpKTsKKyAgICAgICAgcmVtb3ZlTGVh ZGluZ0FuZFRyYWlsaW5nUXVvdGVzKHJlYWxtKTsKKyAgICB9CiAKICAgICBQcm90ZWN0aW9uU3Bh Y2VTZXJ2ZXJUeXBlIHNlcnZlclR5cGUgPSBQcm90ZWN0aW9uU3BhY2VTZXJ2ZXJIVFRQOwogICAg IGlmIChwcm90b2NvbCA9PSAiaHR0cHMiKQo=