Add content-security-policy (CSP) to chrome://flags.

Add content-security-policy (CSP) to chrome://flags.

CSP provides a second line of defense against XSS flaws in the underlying page
by requiring all scripts to come from well-known external sources.  This CL
moves the JS formerly in flags.html into new file flags.js, and adds some JS
to attach handlers formerly set by inline onclick="" attributes.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89723

[Tom Sepez, 2011-06-14 22:00:39]

Please review.  Thanks heaps.

[abarth-chromium, 2011-06-14 22:08:16]

Looks good to me, but I didn't study the details.

[Evan Stade, 2011-06-18 00:37:25]

lgtm with a couple nits

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
chrome/browser/resources/flags.html:9: <meta http-equiv="X-WebKit-CSP"
content="object-src 'none'; script-src chrome://resources 'self' 'unsafe-eval'">
80 (I think you can just wrap strings in html like:

content="foo
         bar"

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
chrome/browser/resources/flags.html:11: body {
while you are at it, could you please move the style to a .css file?

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
chrome/browser/resources/flags.html:207: classs="experiment-select"
s/classs/class

also 4 space indent rather than 2 (I realize the rest of the file doesn't follow
this convention --- you could either fix the rest of the file or leave it alone)

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags.js
File chrome/browser/resources/flags.js (right):

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
chrome/browser/resources/flags.js:1: /**
license header

http://codereview.chromium.org/7148023/diff/1/chrome/browser/resources/flags....
chrome/browser/resources/flags.js:11: /* enabled is only set if the experiment
is single valued */
C++ style comments (//), final punctuation

http://codereview.chromium.org/7148023/diff/1/chrome/browser/ui/webui/chrome_...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

http://codereview.chromium.org/7148023/diff/1/chrome/browser/ui/webui/chrome_...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:22: void AddString(const
std::string& name, const string16& value);
I don't see much point in this function since it has a one-line implementation.
(Unless you plan to use it publicly. Do you?)

[Tom Sepez, 2011-06-20 17:43:56]

> 80 (I think you can just wrap strings in html like:
 
Sure. 80 is part of the C++ standard, probably doesn't hurt to apply it to HTML
as well.

> while you are at it, could you please move the style to a .css file?

Orthogonal to this CL; JS needs to be out of line for CSP, but inline CSS
doesn't pose the same risk, so I'd like to leave this for later.

 
> also 4 space indent rather than 2 (I realize the rest of the file doesn't
follow
> this convention --- you could either fix the rest of the file or leave it
alone)

Fixed this line, left rest of file alone.

> license header

Done.
 
> C++ style comments (//), final punctuation

Yep.  Pre-existing.  Cleaned up.


> I don't see much point in this function since it has a one-line
implementation. (Unless you plan to use it publicly. Do you?)

Yep, used by flags_ui.cc.  Prefer this to making localized_strings_ public.

[Evan Stade, 2011-06-20 17:45:58]

On Mon, Jun 20, 2011 at 10:43 AM,  <tsepez@chromium.org> wrote:
>
>> 80 (I think you can just wrap strings in html like:
>
> Sure. 80 is part of the C++ standard, probably doesn't hurt to apply it to
> HTML
> as well.
>
>> while you are at it, could you please move the style to a .css file?
>
> Orthogonal to this CL; JS needs to be out of line for CSP, but inline CSS
> doesn't pose the same risk, so I'd like to leave this for later.

Leave the code cleaner than when you found it. Moving the css to a new
file would be orthogonal to any cl.

>
>
>> also 4 space indent rather than 2 (I realize the rest of the file doesn't
>
> follow
>>
>> this convention --- you could either fix the rest of the file or leave it
>
> alone)
>
> Fixed this line, left rest of file alone.
>
>> license header
>
> Done.
>
>> C++ style comments (//), final punctuation
>
> Yep.  Pre-existing.  Cleaned up.
>
>
>> I don't see much point in this function since it has a one-line
>
> implementation. (Unless you plan to use it publicly. Do you?)
>
> Yep, used by flags_ui.cc.  Prefer this to making localized_strings_ public.
>
>
>
> http://codereview.chromium.org/7148023/
>

[Tom Sepez, 2011-06-20 18:08:22]

> Leave the code cleaner than when you found it. Moving the css to a new
> file would be orthogonal to any cl.

Done.  grit's flattenhtml re-inlines this anyways when putting flags.html into a
resource bundle.

[Evan Stade, 2011-06-20 18:50:28]

thanks. lgtm

http://codereview.chromium.org/7148023/diff/7005/chrome/browser/resources/fla...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/7148023/diff/7005/chrome/browser/resources/fla...
chrome/browser/resources/flags.html:10: script-src chrome://resources 'self'
'unsafe-eval'">
4 space indent for continuation lines