28262 2009-08-13 09:33:13 -0700 SecurityOrigin::createFromDatabaseIdentifier contains bugs 2009-08-13 11:21:43 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 steveblock benm andreip eric jorlow steveblock oldest_to_newest 139873 0 steveblock 2009-08-13 09:33:13 -0700 SecurityOrigin::createFromDatabaseIdentifier contains the following two bugs. Line 286 When testing that the identifier contains at least two separators, the test is reversed. As a result, identifiers with at least two separators are erroneously rejected. This was introduced in http://trac.webkit.org/changeset/44423. Test case ... INPUT: "http_webkit.org_80" ACTUAL: SecurityOrigin(KURL()) (input rejected) EXPECTED: SecurityOrigin(KURL("http://webkit.org:80")) Line 292 When testing that the port is either a valid integer or absent, the logic is incorrect. Instead, the logic tests that the port is a valid integer or present. Hence identifiers where the port is absent are erroneously rejected and those where the port is present but invalid are erroneously accepted. This was introduced in http://trac.webkit.org/changeset/29386. Test cases ... INPUT: "http_webkit.org_" ACTUAL: SecurityOrigin(KURL()) (input rejected) EXPECTED: SecurityOrigin(KURL("http://webkit.org:0")) INPUT: "http_webkit.org_string" ACTUAL: SecurityOrigin(KURL("http://webkit.org:0")) EXPECTED: SecurityOrigin(KURL()) (input rejected) 139889 1 jorlow 2009-08-13 09:54:41 -0700 I'll take a look. 139891 2 34753 steveblock 2009-08-13 09:55:16 -0700 Created attachment 34753 Patch 1 for bug 28262 Is there a way to add low-level tests for this kind of functionality? 139897 3 jorlow 2009-08-13 10:04:11 -0700 (In reply to comment #2) > Created an attachment (id=34753) [details] > Patch 1 for bug 28262 > > Is there a way to add low-level tests for this kind of functionality? Not really. And, in fact, I created a bug suggesting that we do find a way. This class seems trivial to unittests, but ther'es no framework at the moment. (Only LayoutTests.) 139905 4 34753 darin 2009-08-13 10:11:59 -0700 Comment on attachment 34753 Patch 1 for bug 28262 The best way to make tests for this is to figure out what actual effect the bugs would have. It's OK to have them not tested if there's no obvious way to do so. Building a lower level unit test machinery would allow testing things like this, but would have other costs that may not be acceptable. Find to just land this as-is. r=me 139908 5 steveblock 2009-08-13 10:15:59 -0700 Assigning to benm for landing. 139942 6 34753 eric 2009-08-13 11:21:39 -0700 Comment on attachment 34753 Patch 1 for bug 28262 Clearing flags on attachment: 34753 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/SecurityOrigin.cpp Committed r47221 M WebCore/ChangeLog M WebCore/page/SecurityOrigin.cpp r47221 = ce243898041701dab28e73a78b800546022b62dc (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/47221 139943 7 eric 2009-08-13 11:21:43 -0700 All reviewed patches have been landed. Closing bug. 34753 2009-08-13 09:55:16 -0700 2009-08-13 11:21:39 -0700 Patch 1 for bug 28262 fixSecurityOriginBug.txt text/plain 1922 steveblock SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NzE5NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDgtMTMgIFN0ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEZpeGVzIGEgY291cGxlIG9mIGJ1Z3MgaW4gU2VjdXJpdHlPcmlnaW46OmNyZWF0ZUZyb21EYXRh YmFzZUlkZW50aWZpZXIuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0yODI2MgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBwb3NzaWJsZS4KKworICAgICAg ICAqIHBhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2VjdXJpdHlP cmlnaW46OmNyZWF0ZUZyb21EYXRhYmFzZUlkZW50aWZpZXIpOgorCiAyMDA5LTA4LTEzICBNYXJr IFJvd2UgIDxtcm93ZUBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2ZXJ0IHI0NzE4NSwgdGhlIGZp eCBmb3IgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yODE4NT4sIGFz IGl0IGJyb2tlIHRoZQpJbmRleDogV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcAo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBXZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwCShyZXZpc2lvbiA0NzE5 NikKKysrIFdlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTI4MSwxNSArMjgxLDE2IEBAIFBhc3NSZWZQdHI8U2VjdXJpdHlPcmlnaW4+IFNlY3VyaXR5T3Jp Z2kKICAgICBpZiAoc2VwYXJhdG9yMiA9PSAtMSkKICAgICAgICAgcmV0dXJuIGNyZWF0ZShLVVJM KCkpOwogICAgICAgICAKLSAgICAvLyBFbnN1cmUgdGhlcmUgd2VyZSBhdCBsZWFzdCAyIHNlcGVy YXRvciBjaGFyYWN0ZXJzLiBTb21lIGhvc3RuYW1lcyBvbiBpbnRyYW5ldHMgaGF2ZQorICAgIC8v IEVuc3VyZSB0aGVyZSB3ZXJlIGF0IGxlYXN0IDIgc2VwYXJhdG9yIGNoYXJhY3RlcnMuIFNvbWUg aG9zdG5hbWVzIG9uIGludHJhbmV0cyBoYXZlCiAgICAgLy8gdW5kZXJzY29yZXMgaW4gdGhlbSwg c28gd2UnbGwgYXNzdW1lIHRoYXQgYW55IGFkZGl0aW9uYWwgdW5kZXJzY29yZXMgYXJlIHBhcnQg b2YgdGhlIGhvc3QuCi0gICAgaWYgKHNlcGFyYXRvcjEgIT0gc2VwYXJhdG9yMikKKyAgICBpZiAo c2VwYXJhdG9yMSA9PSBzZXBhcmF0b3IyKQogICAgICAgICByZXR1cm4gY3JlYXRlKEtVUkwoKSk7 CiAgICAgICAgIAogICAgIC8vIE1ha2Ugc3VyZSB0aGUgcG9ydCBzZWN0aW9uIGlzIGEgdmFsaWQg cG9ydCBudW1iZXIgb3IgZG9lc24ndCBleGlzdAogICAgIGJvb2wgcG9ydE9rYXk7CiAgICAgaW50 IHBvcnQgPSBkYXRhYmFzZUlkZW50aWZpZXIucmlnaHQoZGF0YWJhc2VJZGVudGlmaWVyLmxlbmd0 aCgpIC0gc2VwYXJhdG9yMiAtIDEpLnRvSW50KCZwb3J0T2theSk7Ci0gICAgaWYgKCFwb3J0T2th eSAmJiBzZXBhcmF0b3IyICsgMSA9PSBzdGF0aWNfY2FzdDxpbnQ+KGRhdGFiYXNlSWRlbnRpZmll ci5sZW5ndGgoKSkpCisgICAgYm9vbCBwb3J0QWJzZW50ID0gKHNlcGFyYXRvcjIgPT0gc3RhdGlj X2Nhc3Q8aW50PihkYXRhYmFzZUlkZW50aWZpZXIubGVuZ3RoKCkpIC0gMSk7CisgICAgaWYgKCEo cG9ydE9rYXkgfHwgcG9ydEFic2VudCkpCiAgICAgICAgIHJldHVybiBjcmVhdGUoS1VSTCgpKTsK ICAgICAKICAgICBpZiAocG9ydCA8IDAgfHwgcG9ydCA+IDY1NTM1KQo=