When 'eval' and 'eval'-like constructs are blocked by a 'script-src' Content Security Policy directive, JSC currently throws an exception reading "Eval is disabled". This isn't bad, but it would be nice if that message included more detail about what exactly had gone wrong. I'd like to see a string more along the lines of <Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: "script-src 'unsafe-inline'">.
Created attachment 159100 [details] Patch
Created attachment 159125 [details] Resetting tests.
Hey Geoffrey, would you mind taking a look at the patch I've just uploaded? It's a cleaned up version of the one you skimmed yesterday that pipes an error message down from ContentSecurityPolicy to the JSGlobalObject for use in thrown exceptions. Adam, if you wouldn't mind peeking at the CSP bits as well, I'd appreciate it. :) Thanks!
Comment on attachment 159125 [details] Resetting tests. View in context: https://bugs.webkit.org/attachment.cgi?id=159125&action=review Patch looks generally good. I have some small style comments, and a question about security policy policy. > Source/WebCore/page/ContentSecurityPolicy.cpp:807 > + String message = makeString("Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: \"", directives->operativeDirective(directives->m_scriptSrc.get())->text(), "\".\n"); The proper tense here is "violated", not "violates", since "Refused" is in the past tense. > Source/WebCore/page/ContentSecurityPolicy.cpp:1353 > +String ContentSecurityPolicy::evalDisabledErrorMessage() const > +{ > + for (size_t i = 0; i < m_policies.size(); ++i) { This is a linear search each time we make a new document. Is m_policies guaranteed to be very small? Why did you choose the message from the first non-eval policy? What if there are more non-eval policies? > Source/WebCore/page/ContentSecurityPolicy.cpp:1354 > + if (!m_policies[i].get()->allowEval(0, SuppressReport)) No need for .get() on a smart pointer: operator-> does the right thing. > Source/WebCore/page/ContentSecurityPolicy.cpp:1355 > + return m_policies[i].get()->evalDisabledErrorMessage(); Ditto.
Comment on attachment 159125 [details] Resetting tests. View in context: https://bugs.webkit.org/attachment.cgi?id=159125&action=review Thanks for your comments! I've addressed them in a patch I'll upload in a moment. CSP discussion inline below: >> Source/WebCore/page/ContentSecurityPolicy.cpp:807 >> + String message = makeString("Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: \"", directives->operativeDirective(directives->m_scriptSrc.get())->text(), "\".\n"); > > The proper tense here is "violated", not "violates", since "Refused" is in the past tense. Hrm. Yeah. I see your point, however evaluating a string as JavaScript continues to be a violation. The policy is still in effect. Also, I don't like the way "violated" sounds. :) I've changed it entirely to avoid this discussion of tense, and replace it with another: "Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive". WDYT? >> Source/WebCore/page/ContentSecurityPolicy.cpp:1353 >> + for (size_t i = 0; i < m_policies.size(); ++i) { > > This is a linear search each time we make a new document. Is m_policies guaranteed to be very small? > > Why did you choose the message from the first non-eval policy? What if there are more non-eval policies? m_policies has one entry per Content Security Policy HTTP header. We expect to only have one, but it's possible for misconfigured sets of servers and proxies to end up with more than one. A site with any more than one or two headers would be an outlier. I choose the first one simply because it's trivial. :) We deal with multiple headers by ensuring that all of them allow whatever action is being taken. If any of the policies block 'eval()', for instance, we don't have to check the rest of them: it's blocked. >> Source/WebCore/page/ContentSecurityPolicy.cpp:1354 >> + if (!m_policies[i].get()->allowEval(0, SuppressReport)) > > No need for .get() on a smart pointer: operator-> does the right thing. Ah. Cool. :)
Created attachment 159159 [details] ggaren's feedback.
Friendly ping, ggaren. :) Would you mind taking a look at this patch? Thanks!
(In reply to comment #7) > Friendly ping, ggaren. :) > > Would you mind taking a look at this patch? > > Thanks! Ping. :)
Hey Geoffrey, it seems like you're pretty booked up these days. I haven't had much luck tracking down an alternate reviewer the few times I've tried on IRC (probably due to massive time zone differences). Perhaps you could recommend someone else who might be able to review this patch? Thanks!
Created attachment 162831 [details] Rebasing onto ToT.
Created attachment 163662 [details] Rebase onto ToT.
Hey Sam, Adam suggested you as a good reviewer for this sort of change. Would you mind taking a look at this patch? Thanks!
Comment on attachment 163662 [details] Rebase onto ToT. Attachment 163662 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13819864
Comment on attachment 163662 [details] Rebase onto ToT. r=me
Please fix before landing: /usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SinkDocument.o): in function vtable for WebCore::SinkDocument:SinkDocument.cpp(.data.rel.ro._ZTVN7WebCore12SinkDocumentE+0x188): error: undefined reference to 'WebCore::Document::disableEval()' /usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SinkDocument.o): in function vtable for WebCore::SinkDocument:SinkDocument.cpp(.data.rel.ro._ZTVN7WebCore12SinkDocumentE+0x218): error: undefined reference to 'non-virtual thunk to WebCore::Document::disableEval()' /usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SharedWorkerContext.o): in function vtable for WebCore::SharedWorkerContext:SharedWorkerContext.cpp(.data.rel.ro._ZTVN7WebCore19SharedWorkerContextE+0x28): error: undefined reference to 'WebCore::WorkerContext::disableEval()'
Created attachment 164211 [details] Rebased.
Comment on attachment 164211 [details] Rebased. Thanks, much appreciated! Latest iteration of the patch looks green. CQ?
Comment on attachment 164211 [details] Rebased. Rejecting attachment 164211 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 50>At revision 154708. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13859330
(In reply to comment #18) > (From update of attachment 164211 [details]) > Rejecting attachment 164211 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > Last 500 characters of output: > Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > 50>At revision 154708. > > ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > > ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > Updating webkit projects from gyp files... > > Full output: http://queues.webkit.org/results/13859330 Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog' at /usr/lib/git-core/git-svn line 570 My favourite kind of merge conflict. :(
Created attachment 164229 [details] rebasing.
(In reply to comment #20) > Created an attachment (id=164229) [details] > rebasing. Try again (or have I actually missed something in the ChangeLog)?
Comment on attachment 164229 [details] rebasing. r=me
Comment on attachment 164229 [details] rebasing. Clearing flags on attachment: 164229 Committed r128670: <http://trac.webkit.org/changeset/128670>
All reviewed patches have been landed. Closing bug.