Bug 85553 – CSP: Eval isn't blocked in about:blank subframes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

ContentSecurityOrigin shouldn't be RefCounted

[Adam Barth]

Created attachment 140123 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140123 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        I don't think this has any observable effects, so I'm not sure how to
> +        write a test for it.

You should explain better what the backpointer is used for and what the presumable effects of this change are, even if you don't believe they're necessarily observable or easily testable.

[Adam Barth]

    if (!m_scriptExecutionContext->isDocument())
        return;

    m_scriptExecutionContext->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message);

        PingLoader::reportContentSecurityPolicyViolation(frame, m_reportURLs[i], report);

(It's also used a bunch when parsing/enforcing a policy, but that all happens before it gets shared.)

I think the worst thing that can happen is that the console message shows up in the console fro the wrong frame or that the PingLoader uses the wrong frame as the context for the report network request (e.g., maybe the referrer would end up wrong on that request).

[Adam Barth]

Comment on attachment 140123 [details]
Patch

I think I figured out how to test it.  I don't think the sandbox directive is properly applied to the subframe...  Now I need to figure out how to make that into a test.

[Adam Barth]

Actually, that isn't testable either because sandbox automatically inherits into subframes!  Hum...

[Adam Barth]

I figured out a way to test it, because I'm amazing.

[Adam Barth]

Created attachment 140143 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140143 [details]
Patch

Woohoo!

[WebKit Review Bot]

Comment on attachment 140143 [details]
Patch

Clearing flags on attachment: 140143

Committed r116066: <http://trac.webkit.org/changeset/116066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrey Kosyakov]

I'm marking eval-blocked-in-about-blank-iframe.html as SLOW in debug based on this:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Feval-blocked-in-about-blank-iframe.html

[Adam Barth]

I think the test just has a bug.  It shouldn't be slow.

[Andrey Kosyakov]

(In reply to comment #12)
> I think the test just has a bug.  It shouldn't be slow.

yup -- this actually happened to be timeout, not just slow (I already updated the expectations)

[Adam Barth]

I think it's a race.  Will fix shortly.

[Adam Barth]

Created attachment 140274 [details]
Fix race condition in test

[Eric Seidel (no email)]

Comment on attachment 140274 [details]
Fix race condition in test

LGTM.

[WebKit Review Bot]

Comment on attachment 140274 [details]
Fix race condition in test

Clearing flags on attachment: 140274

Committed r116132: <http://trac.webkit.org/changeset/116132>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.