Bug 152489 - Allow JavaScript to iterate over plugins for local SecurityOrigins
Summary: Allow JavaScript to iterate over plugins for local SecurityOrigins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-21 12:43 PST by Brent Fulgham
Modified: 2015-12-22 11:38 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.56 KB, patch)
2015-12-21 12:47 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (4.82 KB, patch)
2015-12-22 10:05 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2015-12-22 10:29 PST, Brent Fulgham
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-12-21 12:43:27 PST
This bug refines Bug 151783 slightly based on Darin's suggestion: "Iā€™d really prefer that this security check be done with SecurityOrigin function rather than a direct call to isLocalFile.".

This patch revises the code slightly to do just that.
Comment 1 Brent Fulgham 2015-12-21 12:47:15 PST
Created attachment 267761 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-12-21 14:06:36 PST
Comment on attachment 267761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267761&action=review

> Source/WebCore/page/Page.cpp:532
> +    DocumentLoader* documentLoader = mainFrame().loader().documentLoader();
> +    if (!documentLoader)
> +        return false;

There wasn't a null check for documentLoader before, but there is one now. Can a regression test be added for this change?

> Source/WebCore/page/Page.cpp:534
> +    RefPtr<SecurityOrigin> origin(SecurityOrigin::create(documentLoader->url()));

SecurityOrigin::create returns a Ref, so please don't make a RefPtr out of it. I think that this is the right way to write this:

Ref<SecurityOrigin> origin = SecurityOrigin::create(documentLoader->url());

But more importantly, I think that we should use the actual security origin for the document, not a one reconstructed from its URL. I don't know of an observable difference, but calling securityOrigin() on the document feels like a better thing to do.
Comment 3 Brent Fulgham 2015-12-22 09:57:06 PST
(In reply to comment #2)
> Comment on attachment 267761 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267761&action=review
> 
> > Source/WebCore/page/Page.cpp:532
> > +    DocumentLoader* documentLoader = mainFrame().loader().documentLoader();
> > +    if (!documentLoader)
> > +        return false;
> 
> There wasn't a null check for documentLoader before, but there is one now.
> Can a regression test be added for this change?

After looking at some of the other uses of documentLoader here (and elsewhere), I noticed that many of them are doing null checks, so I added one here. I'm not sure how to create a test that hits this code with a nullptr documentLoader.

> > Source/WebCore/page/Page.cpp:534
> > +    RefPtr<SecurityOrigin> origin(SecurityOrigin::create(documentLoader->url()));
> 
> SecurityOrigin::create returns a Ref, so please don't make a RefPtr out of
> it. I think that this is the right way to write this:
> 
> Ref<SecurityOrigin> origin = SecurityOrigin::create(documentLoader->url());

Done.

> But more importantly, I think that we should use the actual security origin
> for the document, not a one reconstructed from its URL. I don't know of an
> observable difference, but calling securityOrigin() on the document feels
> like a better thing to do.

I can certainly do that. However, it looks like securityOrigin() is not guaranteed to return a value, so I think we should use the securityOrigin if it exists, otherwise we have to generate one from the URL.
Comment 4 Brent Fulgham 2015-12-22 10:05:20 PST
Created attachment 267790 [details]
Patch
Comment 5 Alexey Proskuryakov 2015-12-22 10:08:08 PST
Comment on attachment 267790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267790&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:197
> +    } else if (DocumentLoader* documentLoader = page->mainFrame().loader().documentLoader()) {

Do we use this idiom anywhere else? I would expect that if document->securityOrigin() returns null, then there isn't a security origin for us to use, so we should not fall back to the URL.
Comment 6 Brent Fulgham 2015-12-22 10:12:07 PST
(In reply to comment #5)
> Comment on attachment 267790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267790&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:197
> > +    } else if (DocumentLoader* documentLoader = page->mainFrame().loader().documentLoader()) {
> 
> Do we use this idiom anywhere else? I would expect that if
> document->securityOrigin() returns null, then there isn't a security origin
> for us to use, so we should not fall back to the URL.

I have to admit I cannot find a single place where we don't just trust the return value of document()->securityOrigin(). It must be 'guaranteed' to exist.

I'll remove that fallback case.
Comment 7 Brent Fulgham 2015-12-22 10:29:55 PST
Created attachment 267791 [details]
Patch
Comment 8 Brent Fulgham 2015-12-22 11:38:12 PST
Committed r194367: <http://trac.webkit.org/changeset/194367>