Summary: | Allow white listing access from origin to local origin | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||
Component: | Page Loading | Assignee: | Erik Arvidsson <arv> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Erik Arvidsson
2010-04-07 12:07:56 PDT
Created attachment 52774 [details]
Patch
Comment on attachment 52774 [details]
Patch
I like the approach. Two comments:
301 if (OriginAccessWhiteList* list = originAccessMap().get(documentOrigin->toString())) {
302 PassRefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url);
303 for (size_t i = 0; i < list->size(); ++i) {
304 if (list->at(i).matchesOrigin(*targetOrigin))
305 return true;
306 }
307 }
This looks like copy/paste code. Can we abstract that into a private method?
+var localImageLocation = layoutTestController.pathToLocalResource('file:///tmp/LayoutTests/http/tests/security/resources/compass.jpg');
Will this file URL work on all platforms (e.g., Windows)? Is this how other tests do this?
Created attachment 52780 [details]
Patch
(In reply to comment #2) > (From update of attachment 52774 [details]) > I like the approach. Two comments: > > 301 if (OriginAccessWhiteList* list = > originAccessMap().get(documentOrigin->toString())) { > 302 PassRefPtr<SecurityOrigin> targetOrigin = > SecurityOrigin::create(url); > 303 for (size_t i = 0; i < list->size(); ++i) { > 304 if (list->at(i).matchesOrigin(*targetOrigin)) > 305 return true; > 306 } > 307 } > > This looks like copy/paste code. Can we abstract that into a private method? Done. I was initially reluctant to do this since it requires us to always create the targetOrigin SecuritOrigin even if the list is empty but I don't think this is an issue since we only get here for local files and creating a SecurityOrigin seems pretty cheap since it is done a lot already. > > +var localImageLocation = > layoutTestController.pathToLocalResource('file:///tmp/LayoutTests/http/tests/security/resources/compass.jpg'); > > Will this file URL work on all platforms (e.g., Windows)? Is this how other > tests do this? This is how other tests do it. I only ran the test on Mac though. Comment on attachment 52780 [details] Patch + bool isAccessWhiteListed(const SecurityOrigin& targetOrigin) const; We should be passing around SecurityOrigin as a SecurityOrigin* not a const SecurityOrigin& because its a pointer type, not a reference type. What was the resolution on +var localImageLocation = layoutTestController.pathToLocalResource('file:///tmp/LayoutTests/http/tests/security/resources/compass.jpg'); ? + PassRefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); This should be a RefPtr. PassRefPtr is only for parameter. You might find it useful to read this documentation: http://webkit.org/coding/RefPtr.html Other than that, this patch is looking good. > This is how other tests do it. I only ran the test on Mac though.
Ah, ok. Good. :)
Created attachment 52785 [details]
Changed to RefPtr
Comment on attachment 52785 [details]
Changed to RefPtr
Great. Thanks for the patch.
+ const SecurityOrigin*
We don't usually use const pointers as parameters, but I see that there's already an example of this in the file. Would you be willing to make a followup patch (in a new bug) that removes all the examples in this file?
(In reply to comment #8) > (From update of attachment 52785 [details]) > Great. Thanks for the patch. > > + const SecurityOrigin* > > We don't usually use const pointers as parameters, but I see that there's > already an example of this in the file. Would you be willing to make a > followup patch (in a new bug) that removes all the examples in this file? OK. Will do. Thanks for the review and the pointer to the PassRefPtr/RefPtr doc. Comment on attachment 52785 [details] Changed to RefPtr Clearing flags on attachment: 52785 Committed r57238: <http://trac.webkit.org/changeset/57238> All reviewed patches have been landed. Closing bug. |