Bug 194664

Summary: REGRESSION (r240446): Storage Access API does not handle domains properly
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, commit-queue, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=194791
Bug Depends on:    
Bug Blocks: 193199    
Attachments:
Description Flags
Patch none

Brent Fulgham
Reported 2019-02-14 12:49:17 PST
During my refactoring of the ResourceLoadStatistics code, I introduced two bugs: (1) I neglected to be consistent in my use of 'primaryDomain', causing some Storage Access API code paths to store approves under one domain (e.g., 'www.example.com'), while checking status under the eTLD+1 (e.g., 'example.com'). The exact string matching requirement caused these to get missed. (2) I used a move operator before a final set of copies of domain names, leading to some empty strings being passed to Storage Access API calls. Both issues are corrected in this patch.
Attachments
Patch (30.57 KB, patch)
2019-02-14 12:56 PST, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
John Wilander
Comment 1 2019-02-14 12:55:18 PST
This is another example of why I really should introduce a registrableDomain class so we can get type safety to save us. We’re using it in so many places now – ITP, SameSite cookies, partitioning, Ad Click Attribution, and PSON (afaik). I might do this next week.
Radar WebKit Bug Importer
Comment 2 2019-02-14 12:55:37 PST
<rdar://problem/48084628>
Brent Fulgham
Comment 3 2019-02-14 12:56:16 PST
Created attachment 362051 [details] Patch
Alex Christensen
Comment 4 2019-02-14 13:02:36 PST
Comment on attachment 362051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362051&action=review > Source/WebKit/ChangeLog:17 > + Both issues are corrected in this patch. Are there any tests to make sure we don't do this again?
John Wilander
Comment 5 2019-02-14 13:21:10 PST
(In reply to Alex Christensen from comment #4) > Comment on attachment 362051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362051&action=review > > > Source/WebKit/ChangeLog:17 > > + Both issues are corrected in this patch. > > Are there any tests to make sure we don't do this again? The problem is that our layout tests don’t support subdomains. That’s why we missed this once earlier. Type safety should fix it.
WebKit Commit Bot
Comment 6 2019-02-14 17:55:32 PST
Comment on attachment 362051 [details] Patch Clearing flags on attachment: 362051 Committed r241574: <https://trac.webkit.org/changeset/241574>
WebKit Commit Bot
Comment 7 2019-02-14 17:55:34 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.