<iframe sandbox="allowScripts"> just bit me, and should trigger a warning. Perhaps "'allowScripts' is an invalid sandbox flag." or "'allowScripts', 'allowForms', and 'allowSameOrigin' are invalid sandbox flags." Adam, would you rather I approached this by passing in something that let us write to the console from SecurityContext::parseSandboxPolicy, or by adding an out parameter to enable the caller to do the work?
I would go the out parameter approach, but it's up to you.
Created attachment 173690 [details] Patch
Here's a pass at the problem. Not entirely sure whether the StringBuilder is appropriate. WDYT, Adam?
Comment on attachment 173690 [details] Patch Attachment 173690 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14815278
Comment on attachment 173690 [details] Patch Attachment 173690 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14823084
Comment on attachment 173690 [details] Patch Attachment 173690 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14809532
Comment on attachment 173690 [details] Patch Attachment 173690 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14820232
Comment on attachment 173690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173690&action=review > Source/WebCore/dom/SecurityContext.cpp:94 > + unsigned numTokenErrors = 0; numTokenErrors -> numberOfTokenErrors > Source/WebCore/dom/SecurityContext.cpp:122 > + tokenErrors.append(", '"); append -> appendLiteral > Source/WebCore/dom/SecurityContext.cpp:134 > + if (numTokenErrors) > + invalidTokensErrorMessage = tokenErrors.toString() + ((numTokenErrors > 1) ? " are invalid sandbox flags." : " is an invalid sandbox flag."); You shouldn't call toString on the StringBuilder until you're done. Instead, you should append these last strings and then call toString at the end. The reason is that the pattern you have here creates an extra allocation and memcpy. The temporary string you create from calling StringBuilder::toString is a waste. > Source/WebCore/page/ContentSecurityPolicy.cpp:1663 > + String message = "Error while parsing the 'sandbox' Content Security Policy directive: " + invalidFlags; > + logToConsole(message); No need to name this temporary.
Created attachment 173713 [details] Take 2.
Comment on attachment 173713 [details] Take 2. View in context: https://bugs.webkit.org/attachment.cgi?id=173713&action=review > Source/WebCore/dom/SecurityContext.cpp:125 > + tokenErrors.appendLiteral("'"); I gave you bad advice. You can actually just make this tokenErrors.append('\''), which is slightly more efficient. > Source/WebCore/dom/SecurityContext.cpp:127 > + tokenErrors.appendLiteral("'"); Ditto > Source/WebCore/dom/SecurityContext.cpp:135 > + tokenErrors.append((numberOfTokenErrors > 1) ? " are invalid sandbox flags." : " is an invalid sandbox flag."); This would be more efficient if you had a real if statement and called appendLiteral separately for the two different constants. What's going on here is that appendLiteral uses a compiler trick to figure out the length of the string at compile-time, which means we don't need to call strlen on these constants. The way you've written this code, we can't figure out the length at compile time, so we fall back to strlen. Above, it's better to use character constants rather than strings because then we don't need loops or other nonsense to traverse a one-character string. None of this really matters in this patch. I just mention it so that you know for future patches where it might matter more.
(In reply to comment #10) > None of this really matters in this patch. I just mention it so that you know for future patches where it might matter more. It's good info! I appreciate you taking the time to explain. I'll spin up a new patch with these changes.
Created attachment 173723 [details] Patch
Created attachment 173729 [details] Patch for landing
Comment on attachment 173729 [details] Patch for landing Regressions: Unexpected text-only failures (3) fast/frames/sandboxed-iframe-attribute-parsing.html [ Failure ] fast/frames/sandboxed-iframe-parsing-space-characters.html [ Failure ] http/tests/security/sandboxed-iframe-modify-self.html [ Failure ]
Created attachment 174398 [details] Patch.
Comment on attachment 174398 [details] Patch. Clearing flags on attachment: 174398 Committed r134766: <http://trac.webkit.org/changeset/134766>
All reviewed patches have been landed. Closing bug.