Bug 37228

Summary: Allow white listing access from origin to local origin
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: Page LoadingAssignee: Erik Arvidsson <arv>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Changed to RefPtr none

Description Erik Arvidsson 2010-04-07 12:07:56 PDT
Allow white listing access from domain to local files
Comment 1 Erik Arvidsson 2010-04-07 13:06:29 PDT
Created attachment 52774 [details]
Patch
Comment 2 Adam Barth 2010-04-07 13:24:15 PDT
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?
Comment 3 Erik Arvidsson 2010-04-07 14:16:17 PDT
Created attachment 52780 [details]
Patch
Comment 4 Erik Arvidsson 2010-04-07 14:19:27 PDT
(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 5 Adam Barth 2010-04-07 14:22:46 PDT
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.
Comment 6 Adam Barth 2010-04-07 14:23:26 PDT
> This is how other tests do it. I only ran the test on Mac though.

Ah, ok.  Good.  :)
Comment 7 Erik Arvidsson 2010-04-07 14:51:33 PDT
Created attachment 52785 [details]
Changed to RefPtr
Comment 8 Adam Barth 2010-04-07 15:35:11 PDT
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?
Comment 9 Erik Arvidsson 2010-04-07 15:40:02 PDT
(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 10 WebKit Commit Bot 2010-04-07 15:59:36 PDT
Comment on attachment 52785 [details]
Changed to RefPtr

Clearing flags on attachment: 52785

Committed r57238: <http://trac.webkit.org/changeset/57238>
Comment 11 WebKit Commit Bot 2010-04-07 15:59:41 PDT
All reviewed patches have been landed.  Closing bug.