Apply CSP to chrome: and about: pages

Apply CSP to chrome: and about: pages

Apply Content Security Policy to the chrome://credits page, and the other pages handled by browser_about_handler.cc while we're at it.  Move inline JS out of several html files.  Move top-level page generation to the about_handler.cc, with tracked_objects.cc/hisogram.cc generating markup fragments to be included.  This keeps this files agnostic to CSP issues.  Also fix an output encoding issue in the <title> of some pages.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90788

[Tom Sepez, 2011-06-24 00:34:50]

about_credits.tmpl is a problem; too large it seems to review here, but when I
generate it from licenses.py, it *is* different in terms of the licenses
present.  Hmm.

[abarth-chromium, 2011-06-24 04:45:18]

This patch is great.  I'm really glad we're adding these policies to some of
this code!

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc#ol...
base/metrics/histogram.cc:1069: output->append(" - " + query);
I can haz XSS?

[Tom Sepez, 2011-06-24 17:14:22]

> base/metrics/histogram.cc:1069: output->append(" - " + query);
> I can haz XSS?

Nearly.  Get %3c here, not the ideal &lt;, but not < either.

[Tom Sepez, 2011-06-24 17:55:54]

Adding reviewers.  jar for histogram, tracked objects; evan/brett for
browser_about_handler, arv for /resources.  Thanks heaps.

[Tom Sepez, 2011-06-24 18:02:13]

Adding mark for about_credits templates.

[Evan Martin, 2011-06-24 18:05:32]

I hope you don't mind, I updated your code review to have a one-line summary
description

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc#ol...
base/metrics/histogram.cc:1069: output->append(" - " + query);
On 2011/06/24 04:45:18, abarth wrote:
> I can haz XSS?

Well, given that you can't link to these pages, you'd have to paste in the URL,
in which case you're vulnerable to other pasting attacks.  ;)

http://codereview.chromium.org/7215034/diff/7002/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7215034/diff/7002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:762: if (!query.empty())
curly on same line as if

[Tom Sepez, 2011-06-24 18:07:27]

> curly on same line as if
Done.

[abarth-chromium, 2011-06-24 18:07:56]

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

http://codereview.chromium.org/7215034/diff/7002/base/metrics/histogram.cc#ol...
base/metrics/histogram.cc:1069: output->append(" - " + query);
On 2011/06/24 18:05:32, Evan Martin wrote:
> Well, given that you can't link to these pages, you'd have to paste in the
URL,
> in which case you're vulnerable to other pasting attacks.  ;)

We've fixed the pasting attacks we know of (at least in the address bar) in one
of our recent releases.

[Tom Sepez, 2011-06-27 17:33:31]

adding estade in place of arv.

[jar (doing other things), 2011-06-27 18:50:19]

The changes LGTM, but it is hard to tell if you got it right, and this level of
code does not have great (any?) unit tests.  

Can you verify that for each about: page you hacked, that:
a) The page still renders;
b) That query strings are handled as they were (and if not, provide an
explanation for why the functionality was lost).

http://codereview.chromium.org/7215034/diff/18021/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7215034/diff/18021/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:192: }  // namespace
I find this jumping in and out of annonymous namespaces confusing (it is hard to
tell what has external linkage, and what doesn't).

Can you see a way to reorder to make this easier to follow?  Perhaps put the
class definition at a different point?  Other??  Should it be a todo, so that
you don't overcomplicate this CL?

http://codereview.chromium.org/7215034/diff/18021/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1255: namespace {
Where is the matching curly?

[Tom Sepez, 2011-06-27 19:20:19]

Yes, I've checked manually that each page renders as I've been converting this,
doing chromeos=1 builds where needed, and I checked queries to about
historgrams/about objects still work.

I will clean up the anonymous namespace.

[Evan Martin, 2011-06-28 17:19:18]

base LGTM