Bug 85553

Summary: CSP: Eval isn't blocked in about:blank subframes
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: caseq, eric, japhet, mkwst, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 53572    
Attachments:
Description Flags
Patch
none
Patch
none
Fix race condition in test none

Description Adam Barth 2012-05-03 16:16:11 PDT
ContentSecurityOrigin shouldn't be RefCounted
Comment 1 Adam Barth 2012-05-03 16:19:44 PDT
Created attachment 140123 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-03 16:23:53 PDT
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.
Comment 3 Adam Barth 2012-05-03 16:28:40 PDT
    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).
Comment 4 Adam Barth 2012-05-03 16:35:14 PDT
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.
Comment 5 Adam Barth 2012-05-03 16:36:01 PDT
Actually, that isn't testable either because sandbox automatically inherits into subframes!  Hum...
Comment 6 Adam Barth 2012-05-03 17:47:09 PDT
I figured out a way to test it, because I'm amazing.
Comment 7 Adam Barth 2012-05-03 18:45:51 PDT
Created attachment 140143 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-05-03 19:04:42 PDT
Comment on attachment 140143 [details]
Patch

Woohoo!
Comment 9 WebKit Review Bot 2012-05-03 20:44:24 PDT
Comment on attachment 140143 [details]
Patch

Clearing flags on attachment: 140143

Committed r116066: <http://trac.webkit.org/changeset/116066>
Comment 10 WebKit Review Bot 2012-05-03 20:44:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Andrey Kosyakov 2012-05-04 00:04:39 PDT
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
Comment 12 Adam Barth 2012-05-04 10:11:58 PDT
I think the test just has a bug.  It shouldn't be slow.
Comment 13 Andrey Kosyakov 2012-05-04 10:15:52 PDT
(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)
Comment 14 Adam Barth 2012-05-04 10:21:41 PDT
I think it's a race.  Will fix shortly.
Comment 15 Adam Barth 2012-05-04 10:52:12 PDT
Created attachment 140274 [details]
Fix race condition in test
Comment 16 Eric Seidel (no email) 2012-05-04 11:00:58 PDT
Comment on attachment 140274 [details]
Fix race condition in test

LGTM.
Comment 17 WebKit Review Bot 2012-05-04 11:37:20 PDT
Comment on attachment 140274 [details]
Fix race condition in test

Clearing flags on attachment: 140274

Committed r116132: <http://trac.webkit.org/changeset/116132>
Comment 18 WebKit Review Bot 2012-05-04 11:37:25 PDT
All reviewed patches have been landed.  Closing bug.