Bug 85553 - CSP: Eval isn't blocked in about:blank subframes
: CSP: Eval isn't blocked in about:blank subframes
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Adam Barth
:
Depends on:
Blocks: 53572
  Show dependency treegraph
 
Reported: 2012-05-03 16:16 PDT by Adam Barth
Modified: 2012-05-04 11:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.81 KB, patch)
2012-05-03 16:19 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2012-05-03 18:45 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Fix race condition in test (1.55 KB, patch)
2012-05-04 10:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 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.