Apply content-security-policy to the HTML options page. This is a

Pre-requisites needed before applying content-security-policy to the HTML options page.  CSP is a second line of defense in case someone introduces an XSS in one of these pages.

The changes to jstemplate_builder are to allow the template scripts
to be served as resources where possible, and to return JS as well as
HTML in the one case where the script is to be dynamically generated.

The other changes are to combine the options javascript files into a
single large file resource and to serve it from under chrome://settings.
This satisfies the "no inline" rule and options.html can script src="" it.

I did put one file into the /shared directory, since it is a companion
to a piece of pre-existing template code alread in that directory.

TEST=chrome://settings page loads properly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85322

[Tom Sepez, 2011-05-10 21:01:53]

Please review.

Added Stuart and Evan as reviewers since they seem to have their fingers on the
options.html file, which is where the crux of the issue lies.

[abarth-chromium, 2011-05-10 21:11:45]

I wonder if this patch might be easier to review in smaller pieces, but I'll
leave that up to stuart and evan.  The breakdown I might use is:

1) Changing JSTemplate to support non-inline.
2) Moving the resources to be external.
3) Adding the security policy.

http://codereview.chromium.org/7003007/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/options_ui.cc (right):

http://codereview.chromium.org/7003007/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.cc:113:
scoped_refptr<RefCountedBytes> html_bytes(new RefCountedBytes);
It seems odd to call this html_bytes because sometimes it's JavaScript.  Maybe
response_bytes ?

http://codereview.chromium.org/7003007/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.cc:116: if (path == "strings")
I'm not sure whether this is the best name.  Maybe strings.json ?

http://codereview.chromium.org/7003007/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.cc:117: {
This { should be on the previous line per the style guide.

http://codereview.chromium.org/7003007/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.cc:138: if (path == "strings")
This string appears twice.  Maybe make it a named constant at the top of the
file?

http://codereview.chromium.org/7003007/diff/1/chrome/common/jstemplate_builde...
File chrome/common/jstemplate_builder.cc (right):

http://codereview.chromium.org/7003007/diff/1/chrome/common/jstemplate_builde...
chrome/common/jstemplate_builder.cc:64: ReplaceSubstringsAfterOffset(&jstext, 0,
"</", "<\\/");
This escaping actually isn't allowed in JSON.  This isn't really JSON, it's
JavaScript (especially with the var declaration).

http://codereview.chromium.org/7003007/diff/1/chrome/common/jstemplate_builder.h
File chrome/common/jstemplate_builder.h (right):

http://codereview.chromium.org/7003007/diff/1/chrome/common/jstemplate_builde...
chrome/common/jstemplate_builder.h:53: 
Looks like there's an extra space here.

[Tom Sepez, 2011-05-10 21:35:39]

>ReplaceeSubstringsAfterOffset(&jstext, 0, "</", "<\\/");
> This escaping actually isn't allowed in JSON.  This isn't really JSON, it's
JavaScript (especially with the var declaration).
> 

Yup, this is just copied from the existing code.  You're right that we probably
not need this; doing script src="" will not get confused in the same way when it
sees a string literal "</script>" or the like.

If you want to get really cute, one thing you can do (and I've done this in a
previous life in a (server-side) JS string routine) is to convert < to \u003c
and > to \u003e, since the \u - syntax is part of JSON, and you create an
equivalent string without having the literal punctuation present in the output
for someone to get hung upon when they try to document.write it (ugh).

[abarth-chromium, 2011-05-10 21:39:17]

Using the \u syntax would work, I'm not sure it's that important for this to
actually be JSON.  It might be easier to just change the function name to
something that doesn't promise JSON.

[arv (Not doing code reviews), 2011-05-11 17:44:17]

Please do not put things in shared/ that is not supposed to be shared across
different WebUI. shared/ is only for reusable things. Things that are only used
once should preferably be inlined. I understand that you are un inlining things
for the CSP but can we at least put this in a non generic place?

[Tom Sepez, 2011-05-11 17:52:31]

On 2011/05/11 17:44:17, arv wrote:
> Please do not put things in shared/ that is not supposed to be shared across
> different WebUI. shared/ is only for reusable things. Things that are only
used
> once should preferably be inlined. I understand that you are un inlining
things
> for the CSP but can we at least put this in a non generic place?
sure.

Sure.  I can map these all under //settings, with just a small duplication of
the same logic that serves files out from under shared.
I can either do this by copying, but it seems that these could share a common
implementation.  Is there a reasonable place to move such logic?

[arv (Not doing code reviews), 2011-05-11 17:56:03]

Can we at least combine all these resources into a single resource so we don't
have to do like ~20 requests?

[Evan Stade, 2011-05-11 18:01:39]

yea, please don't move the files or add them to shared_resources.grd. Instead
you need to add a new data source similar to
chrome/browser/ui/webui/shared_resources_data_source.h or
chrome/browser/ui/webui/ntp/thumbnail_source.h

http://codereview.chromium.org/7003007/diff/5004/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7003007/diff/5004/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:73: <script
src="chrome://resources/js/chrome://resources/js/options/about_page.js"></script>
what

[Evan Stade, 2011-05-11 18:06:58]

On 2011/05/11 17:52:31, Tom Sepez wrote:
> On 2011/05/11 17:44:17, arv wrote:
> > Please do not put things in shared/ that is not supposed to be shared across
> > different WebUI. shared/ is only for reusable things. Things that are only
> used
> > once should preferably be inlined. I understand that you are un inlining
> things
> > for the CSP but can we at least put this in a non generic place?
> sure.
> 
> Sure.  I can map these all under //settings, with just a small duplication of
> the same logic that serves files out from under shared.
> I can either do this by copying, but it seems that these could share a common
> implementation.  Is there a reasonable place to move such logic?

I don't think it's unreasonable to copy the logic, that file is pretty  short,
as are most data source files

[Tom Sepez, 2011-05-11 18:09:12]

Sure.  Seems like there should be a new options_resources.grd file (which
parallels what is going on with several of the other sub-directories under
/resources.   Is this OK?

[Evan Stade, 2011-05-11 18:15:39]

On Wed, May 11, 2011 at 11:09 AM,  <tsepez@chromium.org> wrote:
> Sure.  Seems like there should be a new options_resources.grd file (which
> parallels what is going on with several of the other sub-directories under
> /resources.   Is this OK?

sgtm. You could follow arv's recommendation by making it return a
clump of all the js resources given a request for
chrome://whatever/options_resources.js.

>
> http://codereview.chromium.org/7003007/
>

[Tom Sepez, 2011-05-11 18:18:20]

Sure. But I'll try it first the other way to see if we notice a degradation.

[Evan Stade, 2011-05-11 18:22:44]

On Wed, May 11, 2011 at 11:18 AM,  <tsepez@chromium.org> wrote:
> Sure. But I'll try it first the other way to see if we notice a degradation.
>
> http://codereview.chromium.org/7003007/
>

well, the other advantage is that options.html would get a lot more
concise, instead of needing to list all the files twice you'd only do
it once (in the grd)

[abarth-chromium, 2011-05-11 18:24:26]

On 2011/05/11 18:18:20, Tom Sepez wrote:
> Sure. But I'll try it first the other way to see if we notice a degradation.

IMHO, bundling up all the scripts into one request is probably a good idea. 
We'll need that for other HTML UI anyway.

[Tom Sepez, 2011-05-11 18:30:53]

Maybe we need a flattenjs option much like for flattenhtml?  That way folks
could work on the individual files without stepping on each other.

Alternatively, one could do the "flattening" at run-time, extracting multiple
resources and returning them as the result of single URL.  So is the performance
bottleneck in the fetching of resources or in the processing of URL requests?

[tony, 2011-05-11 18:34:45]

On 2011/05/11 18:30:53, Tom Sepez wrote:
> Maybe we need a flattenjs option much like for flattenhtml?  That way folks
> could work on the individual files without stepping on each other.

Adding this to grit seems fine to me.  Basically, it would be a simple
preprocessor that knows how to handle #include or something.

[Evan Stade, 2011-05-11 18:45:38]

On Wed, May 11, 2011 at 11:30 AM,  <tsepez@chromium.org> wrote:
> Maybe we need a flattenjs option much like for flattenhtml?  That way folks
> could work on the individual files without stepping on each other.
>
> Alternatively, one could do the "flattening" at run-time, extracting
> multiple
> resources and returning them as the result of single URL.  So is the
> performance
> bottleneck in the fetching of resources or in the processing of URL
> requests?

either one of these approaches seems fine to me and probably wouldn't
be too difficult

>
> http://codereview.chromium.org/7003007/
>

[Tom Sepez, 2011-05-11 19:39:37]

Heh.  The flattenhtml preprocessor seems to do the right things wrt. <include>
and <if> tags.  A quick grep of all the .grd files shows that there are places
where a .js file is included with this option, though none appear to use the
preprocesor features.  Those are probably bugs.

[Tom Sepez, 2011-05-11 23:20:31]

Evan, arv, I think patch set 8 is ready for a review cycle.  Thanks heaps.

[Evan Stade, 2011-05-12 01:12:00]

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/options_ui.cc (right):

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/options_ui.cc:130:
ResourceBundle::GetSharedInstance().GetRawDataResource(
I'm surprised you have to make a copy. Doesn't resource bundle already cache,
and can't you copy by reference?

[Tom Sepez, 2011-05-12 18:30:24]

On 2011/05/12 01:12:00, Evan Stade wrote:

> Doesn't resource bundle already cache,

The pattern of using a static variable is used in dozens of places throughout
the code.  If it is caching, then there is significant cleanup that ought to
take place.  I didn't dig deep enough to prove to myself whether the caching
happens at the ResourceBundle level.  Do you know who might be an expert on this
and could answer?

> and can't you copy by reference?

Seems like a worthy goal, esp. if it is being cached locally.  But the copy
pattern again is used in dozens of places, and an inspection of a smattering of
them didn't disclose a place where a clever person had avoided the copy.

Now it seems like SendResponse wants a RefCountedMemory class, not necessarily a
RefCountedBytes class, and there appears to be a RefCountedStaticMemory class
that ignores refcounts.  

So ... I'm going to try to make this work via keeping a copy in the static
variable, and then avoiding the copy on each call (since I don't want to trust
ResourceBundle to keep it unchanged underneath during the whole time the
response is being processed).

If this works, then there's a task for someone to fix a bunch of other places
where we're needlessly copying large chunks.

[Evan Stade, 2011-05-12 18:39:26]

yea, ok. I think it's Tony who would be the best person to answer these
questions. This change LGTM.

[tony, 2011-05-12 19:10:17]

LGTM

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/resources/op...
File chrome/browser/resources/options/options_bundle.js (right):

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/resources/op...
chrome/browser/resources/options/options_bundle.js:1: <include
src="preferences.js"></include>
Nit: You might want to add a comment to the top of this file explaining what it
is for.

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/i18n_process.js (right):

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/i18n_process.js:1:
i18nTemplate.process(document, templateData);
Nit: This file probably warrants a comment too (saying how it should be used
with i18nTemplate).

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/options_ui.cc (right):

http://codereview.chromium.org/7003007/diff/11004/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/options_ui.cc:130:
ResourceBundle::GetSharedInstance().GetRawDataResource(
On 2011/05/12 01:12:01, Evan Stade wrote:
> I'm surprised you have to make a copy. Doesn't resource bundle already cache,
> and can't you copy by reference?

ResourceBundle only caches decoded images.  For other calls, the StringPiece is
basically just a pointer directly to the data in the mmapped file.

Since it's a StringPiece, it doesn't take much extra memory to keep a persistent
reference to it.

http://codereview.chromium.org/7003007/diff/11004/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7003007/diff/11004/chrome/chrome.gyp#newcode1208
chrome/chrome.gyp:1208: '<(grit_out_dir)/options_resources.pak',
I think you need to update chrome_dll.gypi too for mac.

[Tom Sepez, 2011-05-12 20:41:59]

Thanks, Tony.  

It appears to work when avoiding the copy from the string piece to the
RefCountedMemory by using RefCountedStaticMemory, but I'm not going to check
that in, since this ought to be part of a larger cleanup and I'm not sure about
memory ownership rules that might be violated.

Something along the lines of:

void OptionsUIHTMLSource::StartDataRequest(const std::string& path,
                                           bool is_incognito,
                                           int request_id) {
  scoped_refptr<RefCountedMemory> response;
  SetFontAndTextDirection(localized_strings_.get());

  if (path == kLocalizedStringsFile) {
    // Return dynamically-generated strings.
    std::string template_data;
    jstemplate_builder::AppendJsonJS(localized_strings_.get(), &template_data);
    RefCountedBytes *response_bytes = new RefCountedBytes;
    response_bytes->data.resize(template_data.size());
    std::copy(template_data.begin(),
              template_data.end(),
              response_bytes->data.begin());
    response = response_bytes;
  } else if (path == kOptionsBundleJsFile) {
    // Return (and cache) static javascript from the resource bundle.
    static const base::StringPiece options_javascript(
        ResourceBundle::GetSharedInstance().GetRawDataResource(
            IDR_OPTIONS_BUNDLE_JS));
    response = new RefCountedStaticMemory(
        reinterpret_cast<const unsigned char*>(options_javascript.begin()),
        options_javascript.size());
  } else {
    // Return (and cache) the main options html page as the default.
    static const base::StringPiece options_html(
        ResourceBundle::GetSharedInstance().GetRawDataResource(
            IDR_OPTIONS_HTML));
    response = new RefCountedStaticMemory(
        reinterpret_cast<const unsigned char *>(options_html.begin()),
        options_html.size());
  }

  SendResponse(request_id, response);
}

If this is legit, then you can avoid std::copy in the dozen or so cases where
the author has declared a static string piece.

[tony, 2011-05-12 21:04:54]

On 2011/05/12 20:41:59, Tom Sepez wrote:
> If this is legit, then you can avoid std::copy in the dozen or so cases where
> the author has declared a static string piece.

Yes, I think this is legit.  The memory should be valid until we unload the pak
files which doesn't happen until shutdown.

Doing this as a separate cleanup patch sounds great.

[Tom Sepez, 2011-05-12 21:25:19]

Ran into issue with [27799:27799:0512/133747:5137520053:INFO:CONSOLE(143)]
"Failed: undefined with exception: Code generation from strings disallowed for
this context", source:  (143)
On two regression tests as well as from the developer tools console.

We'll land the fix without actually enabling the csp in the options file while
we hunt down who might be calling "eval()".

[arv (Not doing code reviews), 2011-05-12 22:37:08]

LGTM

[Nikita (slow), 2011-05-16 12:18:48]

Seems that this CL broke settings on Chrome OS.
In a result only "Basics" tab is shown.

1. warning: chrome.commandLineString is not present. Not initializing
cr.commandLine
command_line.js:85
2. Cannot read property 'options' of null (cr.commandLine.options)
options_bundle.js:7319

[Tom Sepez, 2011-05-16 16:05:11]

Looking at it now.  Is there a bug number for this regression?

[Nikita (slow), 2011-05-16 16:34:11]

On Mon, May 16, 2011 at 8:05 PM, <tsepez@chromium.org> wrote:

> Looking at it now.  Is there a bug number for this regression?


I've just created one
http://code.google.com/p/chromium-os/issues/detail?id=15348

-- 
Nikita

[Tom Sepez, 2011-05-16 17:55:39]

Just did a clean build of "chrome for chromeos" on linux (e.g. export
GYP_DEFINES="chromeos=1") and did not reproduce. I see the following headings on
the left hand side of the chrome://settings page, all of which loaded:
basics/personal/system/internet/under the hood/users.

Can you try doing a clean rebuild if you haven't already?

[Nikita (slow), 2011-05-16 18:40:21]

Yes, it might be the cause but I'll be able to verify only tomorrow.
Hopefully someone in MTV would verify it too.
We've seen it on 2 machines but hadn't done a clean rebuild.
16.05.2011 21:55 пользователь <tsepez@chromium.org> написал:
> Just did a clean build of "chrome for chromeos" on linux (e.g. export
> GYP_DEFINES="chromeos=1") and did not reproduce. I see the following
> headings on
> the left hand side of the chrome://settings page, all of which loaded:
> basics/personal/system/internet/under the hood/users.
>
> Can you try doing a clean rebuild if you haven't already?
>
> http://codereview.chromium.org/7003007/

[Tom Sepez, 2011-05-16 19:40:16]

Another data point might be to note whether versions prior to r85322 produced
the same two warnings but continued to render the settings page properly.  One
of the consequences of combining all of the JS into a single file is that it is
more sensitive to errors occurring anywhere, whereas before each file got its
own <script> block.  I'd like to rule out this case if nothing else.

[Tom Sepez, 2011-05-16 20:13:29]

Previous comment is not likely.  Deliberately breaking
OptionsUI::RenderViewCreated() by commenting out the setting of:

// render_view_host->SetWebUIProperty("commandLineString", command_line_string);
 

produces the reported behaviour (basic only item on left side) *both* before and
after r85532 where I changed settings.  Would like to see a chromeos run under
the debugger showing what's going on at OptionsUI::RenderViewCreated() on your
two boxes.

[Tom Sepez, 2011-05-16 20:35:44]

s/85532/85322/.  No matter, I verified files and code being executed in the
"before" case.

[Tom Sepez, 2011-05-17 15:39:30]

See: http://code.google.com/p/chromium-os/issues/detail?id=15348

> Root cause is CommandLine refactoring and the way we reinitialize
> it in login_utils.cc

Ok. Glad you found it and that it is outside this CL