Bug 37228 – Allow white listing access from origin to local origin

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Erik Arvidsson]

Allow white listing access from domain to local files

[Erik Arvidsson]

Created attachment 52774 [details]
Patch

[Adam Barth]

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?

[Erik Arvidsson]

Created attachment 52780 [details]
Patch

[Erik Arvidsson]

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

[Adam Barth]

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.

[Adam Barth]

> This is how other tests do it. I only ran the test on Mac though.

Ah, ok.  Good.  :)

[Erik Arvidsson]

Created attachment 52785 [details]
Changed to RefPtr

[Adam Barth]

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?

[Erik Arvidsson]

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

[WebKit Commit Bot]

Comment on attachment 52785 [details]
Changed to RefPtr

Clearing flags on attachment: 52785

Committed r57238: <http://trac.webkit.org/changeset/57238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.