98945 – Web Inspector: Whitelist safe styles for 'console.log('%c...', ...)'.

RESOLVED FIXED 98945

Web Inspector: Whitelist safe styles for 'console.log('%c...', ...)'.

https://bugs.webkit.org/show_bug.cgi?id=98945

Summary Web Inspector: Whitelist safe styles for 'console.log('%c...', ...)'.

Mike West

Reported 2012-10-10 12:28:50 PDT

69401 just landed, bringing with it support for styling console messages via '%c' in the same way as Firebug. We probably don't want to let _all_ style through, however.

Attachments
Patch (8.75 KB, patch) 2012-10-10 14:09 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch (8.79 KB, patch) 2012-10-11 05:16 PDT, Mike West	no flags	Details Formatted Diff Diff
Indentation. (8.53 KB, patch) 2012-10-11 05:18 PDT, Mike West	no flags	Details Formatted Diff Diff
Enumerated whitelist. (10.74 KB, patch) 2012-10-14 11:43 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch (8.37 KB, patch) 2012-10-15 03:42 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch for landing. (8.19 KB, patch) 2012-10-16 05:13 PDT, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Mike West

Comment 1 2012-10-10 14:09:33 PDT

Created attachment 168070 [details] Patch

Mike West

Comment 2 2012-10-10 14:12:08 PDT

(In reply to comment #1) > Created an attachment (id=168070) [details] > Patch As a first pass at a reasonable whitelist, this patch allows 'color' and 'font', as well as 'background[-*]', 'border[-*]', 'font[-*]', 'margin[-*]', 'padding[-*]', 'text[-*]', '-webkit-background[-*]', '-webkit-border[-*]', '-webkit-font[-*]', '-webkit-margin[-*]', '-webkit-padding[-*]', and '-webkit-text[-*]'. We might want to adjust those. WDYT?

Pavel Feldman

Comment 3 2012-10-11 04:34:16 PDT

Comment on attachment 168070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168070&action=review > Source/WebCore/inspector/front-end/ConsoleMessage.js:455 > + if (buffer.style.hasOwnProperty(x) && isWhitelistedStyle(x)) You will get hundreds of them, it is better to iterate the whitelist instead. > Source/WebCore/inspector/front-end/ConsoleMessage.js:460 > + function isWhitelistedStyle(s) { { on the next line > Source/WebCore/inspector/front-end/ConsoleMessage.js:464 > + var prefixes = ["background", "border", "font", "margin", "padding", "text", "webkitBackground", "webkitBorder", "webkitFont", "webkitMargin", "webkitPadding", "webkitText"]; You don't need to check the prefixes - getting the shorthand "border" should generate proper value for you. > LayoutTests/inspector/console/console-format-style-whitelist.html:2 > + <head> We never indent in the inspector tests.

Mike West

Comment 4 2012-10-11 05:16:19 PDT

Created attachment 168197 [details] Patch

Mike West

Comment 5 2012-10-11 05:18:44 PDT

Created attachment 168199 [details] Indentation.

Mike West

Comment 6 2012-10-11 05:19:57 PDT

Comment on attachment 168070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168070&action=review Thanks for taking a look. Comments inline. >> Source/WebCore/inspector/front-end/ConsoleMessage.js:455 >> + if (buffer.style.hasOwnProperty(x) && isWhitelistedStyle(x)) > > You will get hundreds of them, it is better to iterate the whitelist instead. How about iterating the set items? I'll upload a new patch for you to take a look at. >> Source/WebCore/inspector/front-end/ConsoleMessage.js:464 >> + var prefixes = ["background", "border", "font", "margin", "padding", "text", "webkitBackground", "webkitBorder", "webkitFont", "webkitMargin", "webkitPadding", "webkitText"]; > > You don't need to check the prefixes - getting the shorthand "border" should generate proper value for you. I don't think this is enough. Based on some quick experimentation, it doesn't look like '-webkit-font-feature-settings', for example, shows up in the 'font' property's value. Neither does 'text-rendering: optimizeLegibility;' show up in 'text'.

Pavel Feldman

Comment 7 2012-10-11 06:35:41 PDT

> I don't think this is enough. Based on some quick experimentation, it doesn't look like '-webkit-font-feature-settings', for example, shows up in the 'font' property's value. Neither does 'text-rendering: optimizeLegibility;' show up in 'text'. This makes me think "don't use these properties" :)

Mike West

Comment 8 2012-10-11 06:40:59 PDT

(In reply to comment #7) > > I don't think this is enough. Based on some quick experimentation, it doesn't look like '-webkit-font-feature-settings', for example, shows up in the 'font' property's value. Neither does 'text-rendering: optimizeLegibility;' show up in 'text'. > > This makes me think "don't use these properties" :) *shrug* I'm not saying they're totally essential, they're not. Still, 'display' and 'position' (and probably 'overflow' and 'word-wrap' and others I can't think of) could actually break the layout. These couldn't. I'd rather expose as many properties as possible if we don't have a good reason not to.

Pavel Feldman

Comment 9 2012-10-11 09:00:41 PDT

> These couldn't. I'd rather expose as many properties as possible if we don't have a good reason not to. They surely could: box properties (border, padding, margin) can significantly affect the layout. I don't think we need to be creative here, we just need to provide a way to log colorful / bold / underlined messages for the user's convenience. We don't want pages to abuse this API and log("%cFoo", "font-size:1000px") in order to effectively block user interaction by means on console.

Mike West

Comment 10 2012-10-11 09:19:49 PDT

(In reply to comment #9) > > These couldn't. I'd rather expose as many properties as possible if we don't have a good reason not to. > > They surely could: box properties (border, padding, margin) can significantly affect the layout. I don't think we need to be creative here, we just need to provide a way to log colorful / bold / underlined messages for the user's convenience. > > We don't want pages to abuse this API and log("%cFoo", "font-size:1000px") in order to effectively block user interaction by means on console. Well, a malicious page can spam the console right now by looping calls to console.log(). I don't think this patch makes it much easier to make it tough for a developer to work. :) In any event, it sounds like you're suggesting simply walking over [background, border, color, font, margin, padding] (and maybe 'text-decoration'?), which I think is a fine compromise. If developers would like other properties, adding them to the whitelist would be trivial. I'll spin a new patch for you tonightish. Thanks!

Mike West

Comment 11 2012-10-11 10:20:15 PDT

(In reply to comment #10) > I'll spin a new patch for you tonightish. Thanks! Actually, now that I'm looking at it again, it doesn't look like other properties fold together either. For example, given: var x = document.createElement('span'); x.setAttribute('style', 'margin-left: 10px;'); The value of 'x.style.margin' is ''. It does work well in the other direction, however: setting 'margin: 10px;' expands out into 'x.style.marginLeft' and so on. It's still probably more performant to list out all the attributes we care about and iterate over them, it's just going to be a larger list than expected.

Mike West

Comment 12 2012-10-14 11:43:49 PDT

Created attachment 168586 [details] Enumerated whitelist.

Pavel Feldman

Comment 13 2012-10-15 03:08:24 PDT

Comment on attachment 168586 [details] Enumerated whitelist. View in context: https://bugs.webkit.org/attachment.cgi?id=168586&action=review > Source/WebCore/inspector/front-end/ConsoleMessage.js:548 > + for (var i = 0; i < buffer.style.length; i++) { Nice, so you now only walk through a handful of defined properties. Can you use border-, text-, stroke-, etc masks then? > Source/WebCore/inspector/front-end/ConsoleMessage.js:550 > + if (styleWhitelist[property] === true) if (styleWhitelist[property])

Mike West

Comment 14 2012-10-15 03:42:22 PDT

Created attachment 168669 [details] Patch

Pavel Feldman

Comment 15 2012-10-16 02:10:19 PDT

Comment on attachment 168669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168669&action=review > Source/WebCore/inspector/front-end/ConsoleMessage.js:463 > + if (property === "color" || property === "font") No need to special case these: font will match below, color could be also added below - it'll break when there is a new color-* something introduced :) > Source/WebCore/inspector/front-end/ConsoleMessage.js:468 > + if (property.substr(0, prefixes[i].length) == prefixes[i]) property.startsWith(prefixes[i])

Mike West

Comment 16 2012-10-16 05:13:24 PDT

Created attachment 168926 [details] Patch for landing.

WebKit Review Bot

Comment 17 2012-10-16 06:13:04 PDT

Comment on attachment 168926 [details] Patch for landing. Clearing flags on attachment: 168926 Committed r131448: <http://trac.webkit.org/changeset/131448>

WebKit Review Bot

Comment 18 2012-10-16 06:13:08 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Web Inspector (Deprecated)

Assignee

Mike West

Reported

2012-10-10 12:28 PDT

Modified

2012-10-16 06:13 PDT History

CC List

10 users Show

URL

Keywords

Depends on

69401

Blocks

Dependencies

tree graph