Bug 98945

Summary:

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

Product:

WebKit

Reporter:

Mike West <mkwst>

Component:

Web Inspector (Deprecated)

Assignee:

Mike West <mkwst>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

69401

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Indentation.	none
Enumerated whitelist.	none
Patch	none
Patch for landing.	none

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.