26154 2009-06-02 23:27:20 -0700 SecurityOrigin::createFromDatabaseIdentifier should handle _'s in the hostname 2009-06-04 12:29:41 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED FIXED P3 Normal --- 1 jorlow webkit-unassigned darin oldest_to_newest 124104 0 jorlow 2009-06-02 23:27:20 -0700 As Adam Barth mentioned: > There are a bunch of problems with > SecurityOrigin::databaseIdentifier(), not the least of which is that > "_" can't really be used as a delimiter because some intranet host > names contain the character in their names. I'd rather we didn't used > databaseIdentifier() and had a single string representation we used > everywhere. It seems like a good first step towards a fix is to search for only the first and the last _ and assume everything in between is the host name. This would choke on any protocols/schemes with an underscore in it, but the old code would have as well, and it's not clear to me how to handle this gracefully. The original thread: http://lists.macosforge.org/pipermail/webkit-dev/2009-June/008038.html http://lists.macosforge.org/pipermail/webkit-dev/2009-June/008043.html 124105 1 30891 jorlow 2009-06-02 23:33:45 -0700 Created attachment 30891 A potential fix. This is the fix I proposed in the thread, but I have no idea how to create a test for it. Any ideas? 124153 2 30891 darin 2009-06-03 09:05:10 -0700 Comment on attachment 30891 A potential fix. > + WARNING: NO TEST CASES ADDED OR CHANGED This is something you're supposed to delete if you either add a test case or decide you can't add one. You shouldn't submit patches with this in them. We don't want to check this warning into the change log! > + // Ensure there were at least 2 seperator characters. Some hostnames on intranets have > + // underscores in them, so we'll assume that any additional underscores are part of the host. Typo in this comment ("seperator"). Also, we use only one space after periods in our comments. Seems OK to land this without a test. To make a test we'd have to change run-webkit-tests to set up the Apache server to run with a hostname that had an underscore in it. Not impossible, but perhaps difficult. It would also be good to have tests for the existing behavior, for example, illegal database identifiers that fall into the failure case. r=me 124162 3 jorlow 2009-06-03 10:04:28 -0700 (In reply to comment #2) > (From update of attachment 30891 [review]) > > + WARNING: NO TEST CASES ADDED OR CHANGED > > This is something you're supposed to delete if you either add a test case or > decide you can't add one. You shouldn't submit patches with this in them. We > don't want to check this warning into the change log! I left that in as a warning to my reviewer. I assumed it'd be taken out by whomever checked it in. > > + // Ensure there were at least 2 seperator characters. Some hostnames on intranets have > > + // underscores in them, so we'll assume that any additional underscores are part of the host. > > Typo in this comment ("seperator"). Also, we use only one space after periods > in our comments. Ugh. That's going to take some getting used to. :-) > Seems OK to land this without a test. To make a test we'd have to change > run-webkit-tests to set up the Apache server to run with a hostname that had an > underscore in it. Not impossible, but perhaps difficult. > > It would also be good to have tests for the existing behavior, for example, > illegal database identifiers that fall into the failure case. It does seem like a pain to test. Now that I think about it, even if I wrote a LayoutTest, there's really no good way to verify the output beyond it not crashing. It's really too bad there's no way to write unittests in WebKit since this is basically a perfect example of what they're really useful for. Anyway, would you like me to fix the ChangeLog + 2 spaces issue and send another patch, or did you + it since a committer can easily do that? 124199 4 darin 2009-06-03 12:27:10 -0700 (In reply to comment #3) > (In reply to comment #2) > > > + WARNING: NO TEST CASES ADDED OR CHANGED > > > > This is something you're supposed to delete if you either add a test case or > > decide you can't add one. You shouldn't submit patches with this in them. We > > don't want to check this warning into the change log! > > I left that in as a warning to my reviewer. I assumed it'd be taken out by > whomever checked it in. OK. That's not how it is supposed to work. The only thing a committer should have to do is add the name of the reviewer and update the date. > > > + // Ensure there were at least 2 seperator characters. Some hostnames on intranets have > > > + // underscores in them, so we'll assume that any additional underscores are part of the host. > > > > Typo in this comment ("seperator"). Also, we use only one space after periods > > in our comments. > > Ugh. That's going to take some getting used to. :-) Join the debate! http://en.wikipedia.org/wiki/French_spacing http://www.chicagomanualofstyle.org/CMS_FAQ/OneSpaceorTwo/OneSpaceorTwo02.html http://www.webword.com/reports/period.html http://en.wikipedia.org/wiki/Wikipedia_talk:Manual_of_Style_archive_(spaces_after_a_full_stop/period) > > Seems OK to land this without a test. To make a test we'd have to change > > run-webkit-tests to set up the Apache server to run with a hostname that had an > > underscore in it. Not impossible, but perhaps difficult. > > > > It would also be good to have tests for the existing behavior, for example, > > illegal database identifiers that fall into the failure case. > > It's really too bad there's no way to write unittests in WebKit > since this is basically a perfect example of what they're really useful for. There's no reason for you to settle for the status quo. Everything about our testing infrastructure was created by someone at one point to solve a problem like the one you just mentioned. You could be the person who made this possible. Exposing some particular function to JavaScript is all it takes to make a particular unit test in our existing framework; that has been done many times. There's an experimental feature in the Mac DumpRenderTree that lets JavaScript call any Objective-C method, too. You could invent a new approach. Maybe you could bring unit tests as you know them from other projects to this project. Please don't assume that someone else is going to fix this! > Anyway, would you like me to fix the ChangeLog + 2 spaces issue and send > another patch, or did you + it since a committer can easily do that? It'd be kind for you to do that. Really depends on the committer how they feel about doing such minor chores. 124202 5 30921 jorlow 2009-06-03 13:00:12 -0700 Created attachment 30921 Fixed spacing in comment + ChangeLog issues (In reply to comment #4) > (In reply to comment #3) > > It's really too bad there's no way to write unittests in WebKit > > since this is basically a perfect example of what they're really useful for. > > There's no reason for you to settle for the status quo. Everything about our > testing infrastructure was created by someone at one point to solve a problem > like the one you just mentioned. You could be the person who made this > possible. Exposing some particular function to JavaScript is all it takes to > make a particular unit test in our existing framework; that has been done many > times. There's an experimental feature in the Mac DumpRenderTree that lets > JavaScript call any Objective-C method, too. You could invent a new approach. > Maybe you could bring unit tests as you know them from other projects to this > project. > > Please don't assume that someone else is going to fix this! Created https://bugs.webkit.org/show_bug.cgi?id=26171 > > Anyway, would you like me to fix the ChangeLog + 2 spaces issue and send > > another patch, or did you + it since a committer can easily do that? > > It'd be kind for you to do that. Really depends on the committer how they feel > about doing such minor chores. Done. 124210 6 jorlow 2009-06-03 13:50:32 -0700 Interesting. I can't set the review flag on my updated patch. Is that because it already has a +? Anyhow, I think it's ready to commit. Thanks! 124214 7 darin 2009-06-03 14:23:08 -0700 (In reply to comment #6) > Interesting. I can't set the review flag on my updated patch. Is that because > it already has a +? I don't know why. 124349 8 bfulgham 2009-06-04 11:46:26 -0700 Assign for landing. 30891 2009-06-02 23:33:45 -0700 2009-06-03 13:00:12 -0700 A potential fix. parse.patch text/plain 1759 jorlow SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDM3OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDYtMDIgIEplcmVteSBPcmxvdyAgPGpvcmxvd0BjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg V0FSTklORzogTk8gVEVTVCBDQVNFUyBBRERFRCBPUiBDSEFOR0VECisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI2MTU0CisgICAgICAgIEFsbG93IHVu ZGVyc2NvcmVzIGluIHRoZSBob3N0bmFtZXMgd2UgcGFyc2Ugb3V0IG9mIGRhdGFiYXNlSWRlbnRp ZmllcnMuCisgICAgICAgIFRoaXMgY29kZSBpcyB1c2VkIGZvciBIVE1MIDUgZGF0YWJhc2Ugc3Vw cG9ydC4KKworICAgICAgICAqIHBhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwOgorICAgICAgICAoV2Vi Q29yZTo6U2VjdXJpdHlPcmlnaW46OmNyZWF0ZUZyb21EYXRhYmFzZUlkZW50aWZpZXIpOgorCiAy MDA5LTA2LTAyICBEYXJpbiBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBEYXZpZCBIeWF0dC4KSW5kZXg6IFdlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcAkocmV2aXNp b24gNDQzNjEpCisrKyBXZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0yNDYsMTIgKzI0NiwxMyBAQCBQYXNzUmVmUHRyPFNlY3VyaXR5T3JpZ2luPiBTZWN1 cml0eU9yaWdpCiAgICAgICAgIHJldHVybiBjcmVhdGUoS1VSTCgpKTsKICAgICAgICAgCiAgICAg Ly8gTWFrZSBzdXJlIHRoZXJlJ3MgYSBzZWNvbmQgc2VwYXJhdG9yCi0gICAgaW50IHNlcGFyYXRv cjIgPSBkYXRhYmFzZUlkZW50aWZpZXIuZmluZChTZXBhcmF0b3JDaGFyYWN0ZXIsIHNlcGFyYXRv cjEgKyAxKTsKKyAgICBpbnQgc2VwYXJhdG9yMiA9IGRhdGFiYXNlSWRlbnRpZmllci5yZXZlcnNl RmluZChTZXBhcmF0b3JDaGFyYWN0ZXIpOwogICAgIGlmIChzZXBhcmF0b3IyID09IC0xKQogICAg ICAgICByZXR1cm4gY3JlYXRlKEtVUkwoKSk7CiAgICAgICAgIAotICAgIC8vIE1ha2Ugc3VyZSB0 aGVyZSdzIG5vdCBhIHRoaXJkIHNlcGFyYXRvcgotICAgIGlmIChkYXRhYmFzZUlkZW50aWZpZXIu cmV2ZXJzZUZpbmQoU2VwYXJhdG9yQ2hhcmFjdGVyKSAhPSBzZXBhcmF0b3IyKQorICAgIC8vIEVu c3VyZSB0aGVyZSB3ZXJlIGF0IGxlYXN0IDIgc2VwZXJhdG9yIGNoYXJhY3RlcnMuICBTb21lIGhv c3RuYW1lcyBvbiBpbnRyYW5ldHMgaGF2ZQorICAgIC8vIHVuZGVyc2NvcmVzIGluIHRoZW0sIHNv IHdlJ2xsIGFzc3VtZSB0aGF0IGFueSBhZGRpdGlvbmFsIHVuZGVyc2NvcmVzIGFyZSBwYXJ0IG9m IHRoZSBob3N0LgorICAgIGlmIChzZXBhcmF0b3IxICE9IHNlcGFyYXRvcjIpCiAgICAgICAgIHJl dHVybiBjcmVhdGUoS1VSTCgpKTsKICAgICAgICAgCiAgICAgLy8gTWFrZSBzdXJlIHRoZSBwb3J0 IHNlY3Rpb24gaXMgYSB2YWxpZCBwb3J0IG51bWJlciBvciBkb2Vzbid0IGV4aXN0Cg== 30921 2009-06-03 13:00:12 -0700 2009-06-03 14:22:57 -0700 Fixed spacing in comment + ChangeLog issues parse.patch text/plain 1707 jorlow SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDM3OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDktMDYtMDIgIEplcmVteSBPcmxvdyAgPGpvcmxvd0BjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI2MTU0CisgICAgICAgIEFs bG93IHVuZGVyc2NvcmVzIGluIHRoZSBob3N0bmFtZXMgd2UgcGFyc2Ugb3V0IG9mIGRhdGFiYXNl SWRlbnRpZmllcnMuCisgICAgICAgIFRoaXMgY29kZSBpcyB1c2VkIGZvciBIVE1MIDUgZGF0YWJh c2Ugc3VwcG9ydC4KKworICAgICAgICAqIHBhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwOgorICAgICAg ICAoV2ViQ29yZTo6U2VjdXJpdHlPcmlnaW46OmNyZWF0ZUZyb21EYXRhYmFzZUlkZW50aWZpZXIp OgorCiAyMDA5LTA2LTAyICBEYXJpbiBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAg ICBSZXZpZXdlZCBieSBEYXZpZCBIeWF0dC4KSW5kZXg6IFdlYkNvcmUvcGFnZS9TZWN1cml0eU9y aWdpbi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcAko cmV2aXNpb24gNDQzNjEpCisrKyBXZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC0yNDYsMTIgKzI0NiwxMyBAQCBQYXNzUmVmUHRyPFNlY3VyaXR5T3JpZ2lu PiBTZWN1cml0eU9yaWdpCiAgICAgICAgIHJldHVybiBjcmVhdGUoS1VSTCgpKTsKICAgICAgICAg CiAgICAgLy8gTWFrZSBzdXJlIHRoZXJlJ3MgYSBzZWNvbmQgc2VwYXJhdG9yCi0gICAgaW50IHNl cGFyYXRvcjIgPSBkYXRhYmFzZUlkZW50aWZpZXIuZmluZChTZXBhcmF0b3JDaGFyYWN0ZXIsIHNl cGFyYXRvcjEgKyAxKTsKKyAgICBpbnQgc2VwYXJhdG9yMiA9IGRhdGFiYXNlSWRlbnRpZmllci5y ZXZlcnNlRmluZChTZXBhcmF0b3JDaGFyYWN0ZXIpOwogICAgIGlmIChzZXBhcmF0b3IyID09IC0x KQogICAgICAgICByZXR1cm4gY3JlYXRlKEtVUkwoKSk7CiAgICAgICAgIAotICAgIC8vIE1ha2Ug c3VyZSB0aGVyZSdzIG5vdCBhIHRoaXJkIHNlcGFyYXRvcgotICAgIGlmIChkYXRhYmFzZUlkZW50 aWZpZXIucmV2ZXJzZUZpbmQoU2VwYXJhdG9yQ2hhcmFjdGVyKSAhPSBzZXBhcmF0b3IyKQorICAg IC8vIEVuc3VyZSB0aGVyZSB3ZXJlIGF0IGxlYXN0IDIgc2VwZXJhdG9yIGNoYXJhY3RlcnMuIFNv bWUgaG9zdG5hbWVzIG9uIGludHJhbmV0cyBoYXZlCisgICAgLy8gdW5kZXJzY29yZXMgaW4gdGhl bSwgc28gd2UnbGwgYXNzdW1lIHRoYXQgYW55IGFkZGl0aW9uYWwgdW5kZXJzY29yZXMgYXJlIHBh cnQgb2YgdGhlIGhvc3QuCisgICAgaWYgKHNlcGFyYXRvcjEgIT0gc2VwYXJhdG9yMikKICAgICAg ICAgcmV0dXJuIGNyZWF0ZShLVVJMKCkpOwogICAgICAgICAKICAgICAvLyBNYWtlIHN1cmUgdGhl IHBvcnQgc2VjdGlvbiBpcyBhIHZhbGlkIHBvcnQgbnVtYmVyIG9yIGRvZXNuJ3QgZXhpc3QK