Bug 28262 - SecurityOrigin::createFromDatabaseIdentifier contains bugs
Summary: SecurityOrigin::createFromDatabaseIdentifier contains bugs
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ben Murdoch
Depends on:
Reported: 2009-08-13 09:33 PDT by Steve Block
Modified: 2009-08-13 11:21 PDT (History)
4 users (show)

See Also:

Patch 1 for bug 28262 (1.88 KB, patch)
2009-08-13 09:55 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-08-13 09:33:13 PDT
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)
Comment 1 Jeremy Orlow 2009-08-13 09:54:41 PDT
I'll take a look.
Comment 2 Steve Block 2009-08-13 09:55:16 PDT
Created attachment 34753 [details]
Patch 1 for bug 28262

Is there a way to add low-level tests for this kind of functionality?
Comment 3 Jeremy Orlow 2009-08-13 10:04:11 PDT
(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.)
Comment 4 Darin Adler 2009-08-13 10:11:59 PDT
Comment on attachment 34753 [details]
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
Comment 5 Steve Block 2009-08-13 10:15:59 PDT
Assigning to benm for landing.
Comment 6 Eric Seidel (no email) 2009-08-13 11:21:39 PDT
Comment on attachment 34753 [details]
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
Comment 7 Eric Seidel (no email) 2009-08-13 11:21:43 PDT
All reviewed patches have been landed.  Closing bug.