Bug 26154 – SecurityOrigin::createFromDatabaseIdentifier should handle _'s in the hostname

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

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

[Jeremy Orlow]

Created attachment 30891 [details]
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?

[Darin Adler]

Comment on attachment 30891 [details]
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

[Jeremy Orlow]

(In reply to comment #2)
> (From update of attachment 30891 [details] [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?

[Darin Adler]

(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.

[Jeremy Orlow]

Created attachment 30921 [details]
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.

[Jeremy Orlow]

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!

[Darin Adler]

(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.

[Brent Fulgham]

Assign for landing.