Bug 110831 – Plug-ins from third-party domains in the top frame can still set LSOs

• Status: NEW
• Component: WebKit Misc.

[Vicki Pfau]

Since r127513, plug-ins from third-party contexts have been forced into private browsing mode; however, this patch only defines a third-party context as a subframe; an embed to a third-party domain in the main frame will not be forced into private browsing mode.

Changing this to force third-party domain embeds, however, will break upload sites e.g. for Flash games that use CDNs or subdomains to ensure cookies are safe, such as Newgrounds and Kongregate.

[Vicki Pfau]

<rdar://problem/12610535>

[Vicki Pfau]

Created attachment 190179 [details]
Patch

[Maciej Stachowiak]

That's worrisome. How bad do these sites break? We should probably discuss this offline.

[Darin Adler]

Comment on attachment 190179 [details]
Patch

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

Code seems OK, but a bit confusing.

> Source/WebCore/ChangeLog:15
> +        (WebCore):

Please remove bogus change log lines.

> Source/WebCore/ChangeLog:17
> +        (SecurityOrigin):

Please remove bogus change log lines.

> Source/WebCore/page/SecurityOrigin.cpp:386
> +bool SecurityOrigin::canAccessPluginStorage(const WebCore::KURL &url) const

The prefix and formatting here is wrong. Should be:

    const KURL& url

> Source/WebCore/page/SecurityOrigin.cpp:395
> +    if (!url.isValid())
> +        return true;

Seems strange that the create function doesn’t do this check.

> Source/WebCore/page/SecurityOrigin.cpp:398
> +    RefPtr<SecurityOrigin> origin = create(url);
> +    return canAccessStorage(origin.get());

No need for the local variable. Reads better as:

    return canAccessStorage(create(url).get());

Also, wouldn’t it be better to call canAccessPuginStorage here instead of canAccessStorage?

> Source/WebCore/page/SecurityOrigin.h:153
> +    bool canAccessPluginStorage(const KURL&) const;

To me it seems like this argument needs a name. Not at all clear what this URL represents.

> LayoutTests/http/tests/security/cross-origin-same-frame-plugin-expected.txt:2
> +CONSOLE MESSAGE: line 11: false
> +CONSOLE MESSAGE: line 12: false

These sure are cryptic. Could you improve the test to make it clearer what these are about?

> LayoutTests/platform/mac-wk2/http/tests/security/cross-origin-same-frame-plugin-expected.txt:2
> +CONSOLE MESSAGE: line 11: true
> +CONSOLE MESSAGE: line 12: false

These sure are cryptic. Could you improve the test to make it clearer what these are about?