Replace __MSG_some_name__ template within extension css/html files with local...

Replace __MSG_some_name__ template within extension css files with localized messages.

We avoid replacing messages within html and js extension files for security reasons. Also, developers can already localize messages in html/js using chrome.i18n.getMessage calls.

TEST=Localize extension, try body{direction: __MSG_@@bidi_reversed_dir__;} in popup.css, while using non-rtl locale. Text should be alligned to the right (as if we were using rtl locale).

BUG=26144
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=38717

[Nebojša Ćirić, 2010-02-03 19:45:40]

CL is not ready for review (missing tests), but I wanted to get early opinion
from you.

This change is related to extension i18n, where we would like to replace
template __MSG_some_name__ with actual localized message in css/html files for
extensions only.

I need the file to be fully loaded to do message replacement, and this looked
like the best way to do it.

I've added new FilterType, and applied it to extension only files in
ResourceDispetcherHost (I check the URL scheme which is chrome-extension).

ResourceDispetcher then creates new Peer Filter for extensions where I do the
replacement.

I've separated my code from security filters.

One thing that bugs me is the GetURLForDebugging call. I actually need the URL
to get extension ID from it. Is it ok to use GetURLForDebugging or not for that?
Is there a better way to go from request_id to URL?

wtc, jcampan if you could answer my question/look at the design would be great
(or point to somebody else who can).
You are probably going to be interested in:
chrome/browser/renderer_host/resource_dispatcher_host.cc
chrome/common/extensions/extension_l10n_util.cc
chrome/common/extensions/extension_message_filter_peer.{cc,h}
chrome/common/filter_policy.h
chrome/common/resource_dispatcher.cc

mpcomplete:
- I've centralized loaded messages so we keep only one copy in the Renderer
- Extensions that are not localized will call
ViewHostMsg_GetExtensionMessageBundle but won't touch disk, so it shouldn't be
bad.

Thanks for the review

[Matt Perry, 2010-02-03 21:34:02]

http://codereview.chromium.org/570007/diff/1/11
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/570007/diff/1/11#newcode336
chrome/common/resource_dispatcher.cc:336: new_peer =
ExtensionMessageFilterPeer::CreateExtensionMessageFilterPeer(
I think the request_info has the URL, and/or maybe the response_head. If so, you
can pass the URL into your filter peer to avoid the GetURLForDebugging call.

[Nebojša Ćirić, 2010-02-03 22:01:43]

ResourceLoaderBridge::RequestInfo struct has url member, but I have access
only to the ResourceDispatcher::PendingRequestInfo which hasn't (confusing
names).

ResourceResponseHead inherits from ResourceLoaderBridge::ResponseInfo, and
neither of them has the url.

I could probably:

- Change name of GetURLForDebugging to be less scary to use, at least in my
case where I need extension_id only not the whole url.

or

- Change IPCResourceLoaderBridge::Start to pass url_ to
AddPendingRequest(...)...

Cira

On Wed, Feb 3, 2010 at 1:34 PM, <mpcomplete@chromium.org> wrote:

>
> http://codereview.chromium.org/570007/diff/1/11
> File chrome/common/resource_dispatcher.cc (right):
>
> http://codereview.chromium.org/570007/diff/1/11#newcode336
> chrome/common/resource_dispatcher.cc:336: new_peer =
> ExtensionMessageFilterPeer::CreateExtensionMessageFilterPeer(
> I think the request_info has the URL, and/or maybe the response_head. If
> so, you can pass the URL into your filter peer to avoid the
> GetURLForDebugging call.
>
>
> http://codereview.chromium.org/570007
>

[wtc, 2010-02-04 20:29:48]

I looked at the files you asked me to review.  I am
not familiar with that part of the code, so I can only
give superficial review comments (mostly nits) below.

jcampan and abarth might be better persons to review those
files.

http://codereview.chromium.org/570007/diff/1/5
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/570007/diff/1/5#newcode301
chrome/common/extensions/extension_l10n_util.cc:301: // Apply filter only to
chrome html and css extension files, that don't have
Nit: Same here, remove the comma (,) before "that".

http://codereview.chromium.org/570007/diff/1/6
File chrome/common/extensions/extension_l10n_util.h (right):

http://codereview.chromium.org/570007/diff/1/6#newcode13
chrome/common/extensions/extension_l10n_util.h:13: #include
"webkit/glue/resource_type.h"
Nit: add a blank line to separate C++ system headers
from other headers.

http://codereview.chromium.org/570007/diff/1/6#newcode97
chrome/common/extensions/extension_l10n_util.h:97: // Applies
FilterPolicy::FILTER_EXTENSION_MESSAGES to all html/css requests,
Nit: remove the comma (,) at the end of this line.

On the next line, should "schema" be "scheme"?

http://codereview.chromium.org/570007/diff/1/10
File chrome/common/filter_policy.h (right):

http://codereview.chromium.org/570007/diff/1/10#newcode31
chrome/common/filter_policy.h:31: FILTER_EXTENSION_MESSAGES,
It's not clear what you meant by "override".  Can there
be multiple filter policies in use at the same time?

Nit: since any policy except DONT_FILTER will override this,
perhaps this should be listed right after DONT_FILTER?

[jcampan, 2010-02-04 21:25:19]

http://codereview.chromium.org/570007/diff/1/7
File chrome/common/extensions/extension_message_bundle.cc (right):

http://codereview.chromium.org/570007/diff/1/7#newcode310
chrome/common/extensions/extension_message_bundle.cc:310: } else {
Nit: else is unecessary.

http://codereview.chromium.org/570007/diff/1/8
File chrome/common/extensions/extension_message_bundle.h (right):

http://codereview.chromium.org/570007/diff/1/8#newcode75
chrome/common/extensions/extension_message_bundle.h:75: static bool
ReplaceMessages(
Use a different name for the static method.

http://codereview.chromium.org/570007/diff/1/8#newcode169
chrome/common/extensions/extension_message_bundle.h:169: // Return the
extension_id to messages map.
Nit: Returns.

http://codereview.chromium.org/570007/diff/1/8#newcode172
chrome/common/extensions/extension_message_bundle.h:172: // Return message map
that matches given extension_id, or NULL.
Nit: Returns.

http://codereview.chromium.org/570007/diff/1/9
File chrome/common/extensions/extension_message_filter_peer.h (right):

http://codereview.chromium.org/570007/diff/1/9#newcode6
chrome/common/extensions/extension_message_filter_peer.h:6: #ifndef
CHROME_COMMON_EXTENSIONS_EXTENSION_MESSAGE_FILTER_PEER_H__
Nit: only one '_' at the end

http://codereview.chromium.org/570007/diff/1/9#newcode54
chrome/common/extensions/extension_message_filter_peer.h:54:
webkit_glue::ResourceLoaderBridge::Peer* original_peer_;
Would be nice to comment those members.

[Nebojša Ćirić, 2010-02-05 00:53:54]

All nits done.

I'll add tests and ping you again when it's ready.

wtc - I found your name in couple of CLs related to this code, sorry I didn't
check the extent of the changes you made. I've removed you from R line.

On 2010/02/04 21:25:19, jcampan wrote:
> http://codereview.chromium.org/570007/diff/1/7
> File chrome/common/extensions/extension_message_bundle.cc (right):
> 
> http://codereview.chromium.org/570007/diff/1/7#newcode310
> chrome/common/extensions/extension_message_bundle.cc:310: } else {
> Nit: else is unecessary.
> 
> http://codereview.chromium.org/570007/diff/1/8
> File chrome/common/extensions/extension_message_bundle.h (right):
> 
> http://codereview.chromium.org/570007/diff/1/8#newcode75
> chrome/common/extensions/extension_message_bundle.h:75: static bool
> ReplaceMessages(
> Use a different name for the static method.
> 
> http://codereview.chromium.org/570007/diff/1/8#newcode169
> chrome/common/extensions/extension_message_bundle.h:169: // Return the
> extension_id to messages map.
> Nit: Returns.
> 
> http://codereview.chromium.org/570007/diff/1/8#newcode172
> chrome/common/extensions/extension_message_bundle.h:172: // Return message map
> that matches given extension_id, or NULL.
> Nit: Returns.
> 
> http://codereview.chromium.org/570007/diff/1/9
> File chrome/common/extensions/extension_message_filter_peer.h (right):
> 
> http://codereview.chromium.org/570007/diff/1/9#newcode6
> chrome/common/extensions/extension_message_filter_peer.h:6: #ifndef
> CHROME_COMMON_EXTENSIONS_EXTENSION_MESSAGE_FILTER_PEER_H__
> Nit: only one '_' at the end
> 
> http://codereview.chromium.org/570007/diff/1/9#newcode54
> chrome/common/extensions/extension_message_filter_peer.h:54:
> webkit_glue::ResourceLoaderBridge::Peer* original_peer_;
> Would be nice to comment those members.

[Nebojša Ćirić, 2010-02-08 22:31:49]

I've added tests, it's ready for review.

[Nebojša Ćirić, 2010-02-08 23:44:11]

Both wtc and jshin recommended abarth as a good reviewer for this CL, so I've
added him.

On 2010/02/08 22:31:49, Nebojša Ćirić wrote:
> I've added tests, it's ready for review.

[abarth-chromium, 2010-02-08 23:56:57]

I'll take a look at this CL this evening.

[Matt Perry, 2010-02-09 01:15:56]

> - Change IPCResourceLoaderBridge::Start to pass url_ to
> AddPendingRequest(...)...

I would prefer this approach. Otherwise LGTM.

[abarth-chromium, 2010-02-09 02:10:35]

Woah, this is crazy.  Are we sure we want to do this?  I can't think of a better
design off the top of my head, but this is pretty magical.

[abarth-chromium, 2010-02-09 16:07:14]

http://codereview.chromium.org/570007/diff/7001/7004
File chrome/common/extensions/extension_message_filter_peer.cc (right):

http://codereview.chromium.org/570007/diff/7001/7004#newcode38
chrome/common/extensions/extension_message_filter_peer.cc:38:
StartsWithASCII(mime_type, "text/html", false))
What about XML?  application/html+xml?  What about SVG?

[Nebojša Ćirić, 2010-02-09 18:33:26]

Abarth and I chatted offline about this CL, and decided to restrict string
replacements only to text/css files.

Our concern is that replacements happens after security filtering is done, and
we are introducing user generated content to html without further checks (XSS is
possible in that case).

Our main use case is to localize external css files, where we keep bidi text
direction, margins, alignment, and it's hard to do that with JavaScript, while
one can do:
<script>document.write(chrome.i18n.getMessage("name"))</script>
in html.

On 2010/02/09 16:07:14, abarth wrote:
> http://codereview.chromium.org/570007/diff/7001/7004
> File chrome/common/extensions/extension_message_filter_peer.cc (right):
> 
> http://codereview.chromium.org/570007/diff/7001/7004#newcode38
> chrome/common/extensions/extension_message_filter_peer.cc:38:
> StartsWithASCII(mime_type, "text/html", false))
> What about XML?  application/html+xml?  What about SVG?

[Nebojša Ćirić, 2010-02-09 20:28:38]

mpcomplete:
- got rid of GetURLForDebugging call. I am passing request.url down to the
filter.

abarth:
- We accept only local (that came with the extension) extension css files for
localization.

On 2010/02/09 18:33:26, Nebojša Ćirić wrote:
> Abarth and I chatted offline about this CL, and decided to restrict string
> replacements only to text/css files.
> 
> Our concern is that replacements happens after security filtering is done, and
> we are introducing user generated content to html without further checks (XSS
is
> possible in that case).
> 
> Our main use case is to localize external css files, where we keep bidi text
> direction, margins, alignment, and it's hard to do that with JavaScript, while
> one can do:
> <script>document.write(chrome.i18n.getMessage("name"))</script>
> in html.
> 
> On 2010/02/09 16:07:14, abarth wrote:
> > http://codereview.chromium.org/570007/diff/7001/7004
> > File chrome/common/extensions/extension_message_filter_peer.cc (right):
> > 
> > http://codereview.chromium.org/570007/diff/7001/7004#newcode38
> > chrome/common/extensions/extension_message_filter_peer.cc:38:
> > StartsWithASCII(mime_type, "text/html", false))
> > What about XML?  application/html+xml?  What about SVG?

[Matt Perry, 2010-02-09 20:34:47]

LGTM

[Erik does not do reviews, 2010-02-09 22:58:26]

On Tue, Feb 9, 2010 at 10:33 AM, <cira@chromium.org> wrote:

> Abarth and I chatted offline about this CL, and decided to restrict string
> replacements only to text/css files.
>
> Our concern is that replacements happens after security filtering is done,
> and
> we are introducing user generated content to html without further checks
> (XSS is
> possible in that case).
>

Wait, I don't understand this.  How would this be "user generated"?  These
substitutions all come from static extension data files.  Also, how is this
worse than doing document.write?



> Our main use case is to localize external css files, where we keep bidi
> text
> direction, margins, alignment, and it's hard to do that with JavaScript,
> while
> one can do:
> <script>document.write(chrome.i18n.getMessage("name"))</script>
> in html.


Incidentally, we don't really want to encourage document.write.  We should
be guiding them towards innerText mostly.


>
>
> On 2010/02/09 16:07:14, abarth wrote:
>
>> http://codereview.chromium.org/570007/diff/7001/7004
>> File chrome/common/extensions/extension_message_filter_peer.cc (right):
>>
>
>  http://codereview.chromium.org/570007/diff/7001/7004#newcode38
>> chrome/common/extensions/extension_message_filter_peer.cc:38:
>> StartsWithASCII(mime_type, "text/html", false))
>> What about XML?  application/html+xml?  What about SVG?
>>
>
>
>
> http://codereview.chromium.org/570007
>

[Nebojša Ćirić, 2010-02-09 23:16:02]

On Tue, Feb 9, 2010 at 2:58 PM, Erik Kay <erikkay@chromium.org> wrote:

> On Tue, Feb 9, 2010 at 10:33 AM, <cira@chromium.org> wrote:
>
>> Abarth and I chatted offline about this CL, and decided to restrict string
>> replacements only to text/css files.
>>
>> Our concern is that replacements happens after security filtering is done,
>> and
>> we are introducing user generated content to html without further checks
>> (XSS is
>> possible in that case).
>>
>
> Wait, I don't understand this.  How would this be "user generated"?  These
> substitutions all come from static extension data files.  Also, how is this
> worse than doing document.write?
>
> As far as I understand the security concern, at the point I am making
string replacement we were already done with security checks (like XSS), so
extension developer could inject anything using message system and user
won't get the warning. Abarth can probably explain this better.


>
>
>> Our main use case is to localize external css files, where we keep bidi
>> text
>> direction, margins, alignment, and it's hard to do that with JavaScript,
>> while
>> one can do:
>> <script>document.write(chrome.i18n.getMessage("name"))</script>
>> in html.
>
>
> Incidentally, we don't really want to encourage document.write.  We should
> be guiding them towards innerText mostly.
>

I've used document.write as the simplest example on how it's possible to do
message replacement in html without our support - I think dev-rel knows not
to encourage that. Possibly better way would be using this trick
http://www.sitepoint.com/blogs/2007/07/11/insert-in-place-without-documentwri...,
or some kind of templating system.


>
>>
>>
>> On 2010/02/09 16:07:14, abarth wrote:
>>
>>> http://codereview.chromium.org/570007/diff/7001/7004
>>> File chrome/common/extensions/extension_message_filter_peer.cc (right):
>>>
>>
>>  http://codereview.chromium.org/570007/diff/7001/7004#newcode38
>>> chrome/common/extensions/extension_message_filter_peer.cc:38:
>>> StartsWithASCII(mime_type, "text/html", false))
>>> What about XML?  application/html+xml?  What about SVG?
>>>
>>
>>
>>
>> http://codereview.chromium.org/570007
>>
>
>

[abarth-chromium, 2010-02-09 23:28:28]

On Tue, Feb 9, 2010 at 3:15 PM, Nebojša Ćirić <cira@chromium.org> wrote:
> On Tue, Feb 9, 2010 at 2:58 PM, Erik Kay <erikkay@chromium.org> wrote:
>> On Tue, Feb 9, 2010 at 10:33 AM, <cira@chromium.org> wrote:
>>> Abarth and I chatted offline about this CL, and decided to restrict
>>> string
>>> replacements only to text/css files.
>>>
>>> Our concern is that replacements happens after security filtering is
>>> done, and
>>> we are introducing user generated content to html without further checks
>>> (XSS is
>>> possible in that case).
>>
>> Wait, I don't understand this.  How would this be "user generated"?  These
>> substitutions all come from static extension data files.  Also, how is this
>> worse than doing document.write?
>
> As far as I understand the security concern, at the point I am making string
> replacement we were already done with security checks (like XSS), so
> extension developer could inject anything using message system and user
> won't get the warning. Abarth can probably explain this better.

This isn't a problem currently, but its adding complexity that would
limit our ability to do things in the future.  For example, imagine if
we decided to do something like mount_points in the future.  Now, if
the web server processes user-generated content, that content can
interact with the i18n service using these __MSG_foo__ magic strings,
which could well surprise XSS filters and other security mechanisms.

Here's a concrete example.  Suppose __MSG_foo__ expands to "script is
delicious".  Now, the use supplies this blog comment:

<__MSG_foo__>alert(document.cookie)</__MSG_foo__>

You can easily imagine that getting through an XSS filter, but after
the auto-magic substitution, disaster ensues.

The problem is this mechanism is very unusual and magical in the web
platform, where as doing the substitutions in the DOM is much more
predictable, even if because of the simple fact that it happens after
parsing.  It's also easy to forget because it doesn't affect the
common case.

Nebojša told me that the CSS use case was more compelling than the
HTML use case.  The CSS use case is also a lot less risky because:

1) There's not a whole lot of user-generated content inlined into CSS.
2) Even if an attacker can take control of your CSS, it's not the end
of the world.  (Don't get me wrong, it's not great, but it's not game
over.)

For these reasons, it seemed like the best thing to do in the short
term was to limit the filter to CSS.  If having auto-magic i18n in
HTML is important, I think we should look at DOM-level solutions.  For
example, a geam-like design that replaces text nodes after parsing.

Adam

[Erik does not do reviews, 2010-02-10 00:02:15]

On Tue, Feb 9, 2010 at 3:27 PM, Adam Barth <abarth@chromium.org> wrote:

> On Tue, Feb 9, 2010 at 3:15 PM, Nebojša Ćirić <cira@chromium.org> wrote:
> > On Tue, Feb 9, 2010 at 2:58 PM, Erik Kay <erikkay@chromium.org> wrote:
> >> On Tue, Feb 9, 2010 at 10:33 AM, <cira@chromium.org> wrote:
> >>> Abarth and I chatted offline about this CL, and decided to restrict
> >>> string
> >>> replacements only to text/css files.
> >>>
> >>> Our concern is that replacements happens after security filtering is
> >>> done, and
> >>> we are introducing user generated content to html without further
> checks
> >>> (XSS is
> >>> possible in that case).
> >>
> >> Wait, I don't understand this.  How would this be "user generated"?
>  These
> >> substitutions all come from static extension data files.  Also, how is
> this
> >> worse than doing document.write?
> >
> > As far as I understand the security concern, at the point I am making
> string
> > replacement we were already done with security checks (like XSS), so
> > extension developer could inject anything using message system and user
> > won't get the warning. Abarth can probably explain this better.
>
> This isn't a problem currently, but its adding complexity that would
> limit our ability to do things in the future.  For example, imagine if
> we decided to do something like mount_points in the future.  Now, if
> the web server processes user-generated content, that content can
> interact with the i18n service using these __MSG_foo__ magic strings,
> which could well surprise XSS filters and other security mechanisms.
>
> Here's a concrete example.  Suppose __MSG_foo__ expands to "script is
> delicious".  Now, the use supplies this blog comment:
>
> <__MSG_foo__>alert(document.cookie)</__MSG_foo__>
>
> You can easily imagine that getting through an XSS filter, but after
> the auto-magic substitution, disaster ensues.
>
> The problem is this mechanism is very unusual and magical in the web
> platform, where as doing the substitutions in the DOM is much more
> predictable, even if because of the simple fact that it happens after
> parsing.  It's also easy to forget because it doesn't affect the
> common case.
>
> Nebojša told me that the CSS use case was more compelling than the
> HTML use case.  The CSS use case is also a lot less risky because:
>
> 1) There's not a whole lot of user-generated content inlined into CSS.
> 2) Even if an attacker can take control of your CSS, it's not the end
> of the world.  (Don't get me wrong, it's not great, but it's not game
> over.)
>

Thanks for the examples.  I guess I was assuming we wouldn't allow this in
live pages.  Live pages already have plenty of great ways to do localization
in their own frameworks.  That said, the line will certainly get blurry and
I could imagine bugs slipping through here.

Incidentally, I haven't looked at the code in detail, but does the
substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
 I'd prefer it didn't (they can use getMessage() for that).



> For these reasons, it seemed like the best thing to do in the short
> term was to limit the filter to CSS.  If having auto-magic i18n in
> HTML is important, I think we should look at DOM-level solutions.  For
> example, a geam-like design that replaces text nodes after parsing.
>

I agree that this seems more reasonable.  This sounds familiar to the
solution that Maciej proposed for how we handle indic font transformations
using the CSS text-transform pipeline.  We could hook into that pretty
easily I think.

Erik

[Nebojša Ćirić, 2010-02-10 00:13:55]

>
> 1) There's not a whole lot of user-generated content inlined into CSS.
>> 2) Even if an attacker can take control of your CSS, it's not the end
>> of the world.  (Don't get me wrong, it's not great, but it's not game
>> over.)
>>
>
> Thanks for the examples.  I guess I was assuming we wouldn't allow this in
> live pages.  Live pages already have plenty of great ways to do localization
> in their own frameworks.  That said, the line will certainly get blurry and
> I could imagine bugs slipping through here.
>
> Incidentally, I haven't looked at the code in detail, but does the
> substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
>  I'd prefer it didn't (they can use getMessage() for that).
>
>
Without this CL we can only substitute __MSG_ in the manifest. With this CL
we would substitute only within external CSS (and manifest). They would
still need to use getMessage for dynamic HTML.

So this works:

popup.css (external css within extension package):

body {
  direction: __MSG_@@bidi_dir__;
  color: black;
  width: 300px;
}

This doesn't:

popup.html

<html><script>...document.write("__MSG_name__")</script></html>

[Erik does not do reviews, 2010-02-10 00:21:06]

On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org> wrote:

> 1) There's not a whole lot of user-generated content inlined into CSS.
>>> 2) Even if an attacker can take control of your CSS, it's not the end
>>> of the world.  (Don't get me wrong, it's not great, but it's not game
>>> over.)
>>>
>>
>> Thanks for the examples.  I guess I was assuming we wouldn't allow this in
>> live pages.  Live pages already have plenty of great ways to do localization
>> in their own frameworks.  That said, the line will certainly get blurry and
>> I could imagine bugs slipping through here.
>>
>> Incidentally, I haven't looked at the code in detail, but does the
>> substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
>>  I'd prefer it didn't (they can use getMessage() for that).
>>
>>
> Without this CL we can only substitute __MSG_ in the manifest. With this CL
> we would substitute only within external CSS (and manifest). They would
> still need to use getMessage for dynamic HTML.
>

Yes, I understand.  I was talking about prior to the last change you made.
 Under what circumstances did that substitution work?



>
> So this works:
>
> popup.css (external css within extension package):
>
> body {
>   direction: __MSG_@@bidi_dir__;
>   color: black;
>   width: 300px;
> }
>
> This doesn't:
>
> popup.html
>
> <html><script>...document.write("__MSG_name__")</script></html>
>
>
>

[Nebojša Ćirić, 2010-02-10 00:29:55]

Ah, sorry. It would replace all __MSG_whatever__ within loaded html file.

So all of this would work:

<html><body>__MSG_some_text__ <script>document.write("__MSG_other_text__");
var ups=chrome.i18n.getMessage("__MSG_var_name__");</script></body></html>

It was a blind search/replace on a in-memory string.

On Tue, Feb 9, 2010 at 4:20 PM, Erik Kay <erikkay@chromium.org> wrote:

> On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org> wrote:
>
>>  1) There's not a whole lot of user-generated content inlined into CSS.
>>>> 2) Even if an attacker can take control of your CSS, it's not the end
>>>> of the world.  (Don't get me wrong, it's not great, but it's not game
>>>> over.)
>>>>
>>>
>>> Thanks for the examples.  I guess I was assuming we wouldn't allow this
>>> in live pages.  Live pages already have plenty of great ways to do
>>> localization in their own frameworks.  That said, the line will certainly
>>> get blurry and I could imagine bugs slipping through here.
>>>
>>> Incidentally, I haven't looked at the code in detail, but does the
>>> substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
>>>  I'd prefer it didn't (they can use getMessage() for that).
>>>
>>>
>> Without this CL we can only substitute __MSG_ in the manifest. With this
>> CL we would substitute only within external CSS (and manifest). They would
>> still need to use getMessage for dynamic HTML.
>>
>
> Yes, I understand.  I was talking about prior to the last change you made.
>  Under what circumstances did that substitution work?
>
>
>
>>
>> So this works:
>>
>> popup.css (external css within extension package):
>>
>> body {
>>   direction: __MSG_@@bidi_dir__;
>>   color: black;
>>   width: 300px;
>> }
>>
>> This doesn't:
>>
>> popup.html
>>
>> <html><script>...document.write("__MSG_name__")</script></html>
>>
>>
>>
>

[Erik does not do reviews, 2010-02-10 00:54:47]

On Tue, Feb 9, 2010 at 4:29 PM, Nebojša Ćirić <cira@chromium.org> wrote:

> Ah, sorry. It would replace all __MSG_whatever__ within loaded html file.
>
> So all of this would work:
>
> <html><body>__MSG_some_text__ <script>document.write("__MSG_other_text__");
> var ups=chrome.i18n.getMessage("__MSG_var_name__");</script></body></html>
>
> It was a blind search/replace on a in-memory string.
>

Sorry to belabor the point, but if I said:

document.getElementById("foo").innerHTML = "__MS" + "G_foo__";

Would that have done anything?  Or what if I loaded CSS as a data: URL?  I'm
just trying to get an idea of what level this hook is being run at.

Erik



>
> On Tue, Feb 9, 2010 at 4:20 PM, Erik Kay <erikkay@chromium.org> wrote:
>
>> On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org> wrote:
>>
>>>  1) There's not a whole lot of user-generated content inlined into CSS.
>>>>> 2) Even if an attacker can take control of your CSS, it's not the end
>>>>> of the world.  (Don't get me wrong, it's not great, but it's not game
>>>>> over.)
>>>>>
>>>>
>>>> Thanks for the examples.  I guess I was assuming we wouldn't allow this
>>>> in live pages.  Live pages already have plenty of great ways to do
>>>> localization in their own frameworks.  That said, the line will certainly
>>>> get blurry and I could imagine bugs slipping through here.
>>>>
>>>> Incidentally, I haven't looked at the code in detail, but does the
>>>> substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
>>>>  I'd prefer it didn't (they can use getMessage() for that).
>>>>
>>>>
>>> Without this CL we can only substitute __MSG_ in the manifest. With this
>>> CL we would substitute only within external CSS (and manifest). They would
>>> still need to use getMessage for dynamic HTML.
>>>
>>
>> Yes, I understand.  I was talking about prior to the last change you made.
>>  Under what circumstances did that substitution work?
>>
>>
>>
>>>
>>> So this works:
>>>
>>> popup.css (external css within extension package):
>>>
>>> body {
>>>   direction: __MSG_@@bidi_dir__;
>>>   color: black;
>>>   width: 300px;
>>> }
>>>
>>> This doesn't:
>>>
>>> popup.html
>>>
>>> <html><script>...document.write("__MSG_name__")</script></html>
>>>
>>>
>>>
>>
>

[Nebojša Ćirić, 2010-02-10 01:03:19]

This hook replaces text just before it gets handled to WebKit. So I think:

document.getElementById("foo").innerHTML = "__MS" + "G_foo__";

wouldn't trigger the replacement.

As for CSS question - I first detect if the resource type is STYLESHEET, and
if it is and it has chrome-extension scheme it gets flagged for
substitution. Then before we do substitution I check if the mime is
text/css, if not we pass it without changes. I am not sure what would happen
for data: URL. Do you have short snippet I could try?

On Tue, Feb 9, 2010 at 4:54 PM, Erik Kay <erikkay@chromium.org> wrote:

> On Tue, Feb 9, 2010 at 4:29 PM, Nebojša Ćirić <cira@chromium.org> wrote:
>
>> Ah, sorry. It would replace all __MSG_whatever__ within loaded html file.
>>
>> So all of this would work:
>>
>> <html><body>__MSG_some_text__
>> <script>document.write("__MSG_other_text__"); var
>> ups=chrome.i18n.getMessage("__MSG_var_name__");</script></body></html>
>>
>> It was a blind search/replace on a in-memory string.
>>
>
> Sorry to belabor the point, but if I said:
>
> document.getElementById("foo").innerHTML = "__MS" + "G_foo__";
>
> Would that have done anything?  Or what if I loaded CSS as a data: URL?
>  I'm just trying to get an idea of what level this hook is being run at.
>
> Erik
>
>
>
>>
>> On Tue, Feb 9, 2010 at 4:20 PM, Erik Kay <erikkay@chromium.org> wrote:
>>
>>> On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org> wrote:
>>>
>>>>  1) There's not a whole lot of user-generated content inlined into CSS.
>>>>>> 2) Even if an attacker can take control of your CSS, it's not the end
>>>>>> of the world.  (Don't get me wrong, it's not great, but it's not game
>>>>>> over.)
>>>>>>
>>>>>
>>>>> Thanks for the examples.  I guess I was assuming we wouldn't allow this
>>>>> in live pages.  Live pages already have plenty of great ways to do
>>>>> localization in their own frameworks.  That said, the line will certainly
>>>>> get blurry and I could imagine bugs slipping through here.
>>>>>
>>>>> Incidentally, I haven't looked at the code in detail, but does the
>>>>> substitution work even in dynamic HTML?  (innerHTML, document.write, etc.)
>>>>>  I'd prefer it didn't (they can use getMessage() for that).
>>>>>
>>>>>
>>>> Without this CL we can only substitute __MSG_ in the manifest. With this
>>>> CL we would substitute only within external CSS (and manifest). They would
>>>> still need to use getMessage for dynamic HTML.
>>>>
>>>
>>> Yes, I understand.  I was talking about prior to the last change you
>>> made.  Under what circumstances did that substitution work?
>>>
>>>
>>>
>>>>
>>>> So this works:
>>>>
>>>> popup.css (external css within extension package):
>>>>
>>>> body {
>>>>   direction: __MSG_@@bidi_dir__;
>>>>   color: black;
>>>>   width: 300px;
>>>> }
>>>>
>>>> This doesn't:
>>>>
>>>> popup.html
>>>>
>>>> <html><script>...document.write("__MSG_name__")</script></html>
>>>>
>>>>
>>>>
>>>
>>
>

[Erik does not do reviews, 2010-02-10 01:22:36]

On Tue, Feb 9, 2010 at 5:03 PM, Nebojša Ćirić <cira@chromium.org> wrote:

> This hook replaces text just before it gets handled to WebKit. So I think:
>
>
> document.getElementById("foo").innerHTML = "__MS" + "G_foo__";
>
> wouldn't trigger the replacement.
>
> As for CSS question - I first detect if the resource type is STYLESHEET,
> and if it is and it has chrome-extension scheme it gets flagged for
> substitution. Then before we do substitution I check if the mime is
> text/css, if not we pass it without changes. I am not sure what would happen
> for data: URL. Do you have short snippet I could try?
>

Thanks for chatting it over in person.

The quick snippet would look something like this: (hopefully good, I didn't
test it)

<link rel="stylesheet" href="data:text/css,body{direction: __MSG_@
@bidi_dir__;}">



>
> On Tue, Feb 9, 2010 at 4:54 PM, Erik Kay <erikkay@chromium.org> wrote:
>
>> On Tue, Feb 9, 2010 at 4:29 PM, Nebojša Ćirić <cira@chromium.org> wrote:
>>
>>> Ah, sorry. It would replace all __MSG_whatever__ within loaded html file.
>>>
>>> So all of this would work:
>>>
>>> <html><body>__MSG_some_text__
>>> <script>document.write("__MSG_other_text__"); var
>>> ups=chrome.i18n.getMessage("__MSG_var_name__");</script></body></html>
>>>
>>> It was a blind search/replace on a in-memory string.
>>>
>>
>> Sorry to belabor the point, but if I said:
>>
>> document.getElementById("foo").innerHTML = "__MS" + "G_foo__";
>>
>> Would that have done anything?  Or what if I loaded CSS as a data: URL?
>>  I'm just trying to get an idea of what level this hook is being run at.
>>
>> Erik
>>
>>
>>
>>>
>>> On Tue, Feb 9, 2010 at 4:20 PM, Erik Kay <erikkay@chromium.org> wrote:
>>>
>>>> On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org>wrote:
>>>>
>>>>>  1) There's not a whole lot of user-generated content inlined into CSS.
>>>>>>> 2) Even if an attacker can take control of your CSS, it's not the end
>>>>>>> of the world.  (Don't get me wrong, it's not great, but it's not game
>>>>>>> over.)
>>>>>>>
>>>>>>
>>>>>> Thanks for the examples.  I guess I was assuming we wouldn't allow
>>>>>> this in live pages.  Live pages already have plenty of great ways to do
>>>>>> localization in their own frameworks.  That said, the line will certainly
>>>>>> get blurry and I could imagine bugs slipping through here.
>>>>>>
>>>>>> Incidentally, I haven't looked at the code in detail, but does the
>>>>>> substitution work even in dynamic HTML?  (innerHTML, document.write,
etc.)
>>>>>>  I'd prefer it didn't (they can use getMessage() for that).
>>>>>>
>>>>>>
>>>>> Without this CL we can only substitute __MSG_ in the manifest. With
>>>>> this CL we would substitute only within external CSS (and manifest). They
>>>>> would still need to use getMessage for dynamic HTML.
>>>>>
>>>>
>>>> Yes, I understand.  I was talking about prior to the last change you
>>>> made.  Under what circumstances did that substitution work?
>>>>
>>>>
>>>>
>>>>>
>>>>> So this works:
>>>>>
>>>>> popup.css (external css within extension package):
>>>>>
>>>>> body {
>>>>>   direction: __MSG_@@bidi_dir__;
>>>>>   color: black;
>>>>>   width: 300px;
>>>>> }
>>>>>
>>>>> This doesn't:
>>>>>
>>>>> popup.html
>>>>>
>>>>> <html><script>...document.write("__MSG_name__")</script></html>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[Nebojša Ćirić, 2010-02-10 01:41:27]

The CSS data loading doesn't work (message doesn't get replaced). It has to be
external CSS file.

On 2010/02/10 01:22:36, Erik Kay wrote:
> On Tue, Feb 9, 2010 at 5:03 PM, Nebojša Ćirić <mailto:cira@chromium.org>
wrote:
> 
> > This hook replaces text just before it gets handled to WebKit. So I think:
> >
> >
> > document.getElementById("foo").innerHTML = "__MS" + "G_foo__";
> >
> > wouldn't trigger the replacement.
> >
> > As for CSS question - I first detect if the resource type is STYLESHEET,
> > and if it is and it has chrome-extension scheme it gets flagged for
> > substitution. Then before we do substitution I check if the mime is
> > text/css, if not we pass it without changes. I am not sure what would happen
> > for data: URL. Do you have short snippet I could try?
> >
> 
> Thanks for chatting it over in person.
> 
> The quick snippet would look something like this: (hopefully good, I didn't
> test it)
> 
> <link rel="stylesheet" href="data:text/css,body{direction: __MSG_@
> @bidi_dir__;}">
> 
> 
> 
> >
> > On Tue, Feb 9, 2010 at 4:54 PM, Erik Kay <mailto:erikkay@chromium.org>
wrote:
> >
> >> On Tue, Feb 9, 2010 at 4:29 PM, Nebojša Ćirić <mailto:cira@chromium.org>
wrote:
> >>
> >>> Ah, sorry. It would replace all __MSG_whatever__ within loaded html file.
> >>>
> >>> So all of this would work:
> >>>
> >>> <html><body>__MSG_some_text__
> >>> <script>document.write("__MSG_other_text__"); var
> >>> ups=chrome.i18n.getMessage("__MSG_var_name__");</script></body></html>
> >>>
> >>> It was a blind search/replace on a in-memory string.
> >>>
> >>
> >> Sorry to belabor the point, but if I said:
> >>
> >> document.getElementById("foo").innerHTML = "__MS" + "G_foo__";
> >>
> >> Would that have done anything?  Or what if I loaded CSS as a data: URL?
> >>  I'm just trying to get an idea of what level this hook is being run at.
> >>
> >> Erik
> >>
> >>
> >>
> >>>
> >>> On Tue, Feb 9, 2010 at 4:20 PM, Erik Kay <mailto:erikkay@chromium.org>
wrote:
> >>>
> >>>> On Tue, Feb 9, 2010 at 4:13 PM, Nebojša Ćirić <cira@chromium.org>wrote:
> >>>>
> >>>>>  1) There's not a whole lot of user-generated content inlined into CSS.
> >>>>>>> 2) Even if an attacker can take control of your CSS, it's not the end
> >>>>>>> of the world.  (Don't get me wrong, it's not great, but it's not game
> >>>>>>> over.)
> >>>>>>>
> >>>>>>
> >>>>>> Thanks for the examples.  I guess I was assuming we wouldn't allow
> >>>>>> this in live pages.  Live pages already have plenty of great ways to do
> >>>>>> localization in their own frameworks.  That said, the line will
certainly
> >>>>>> get blurry and I could imagine bugs slipping through here.
> >>>>>>
> >>>>>> Incidentally, I haven't looked at the code in detail, but does the
> >>>>>> substitution work even in dynamic HTML?  (innerHTML, document.write,
> etc.)
> >>>>>>  I'd prefer it didn't (they can use getMessage() for that).
> >>>>>>
> >>>>>>
> >>>>> Without this CL we can only substitute __MSG_ in the manifest. With
> >>>>> this CL we would substitute only within external CSS (and manifest).
They
> >>>>> would still need to use getMessage for dynamic HTML.
> >>>>>
> >>>>
> >>>> Yes, I understand.  I was talking about prior to the last change you
> >>>> made.  Under what circumstances did that substitution work?
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>> So this works:
> >>>>>
> >>>>> popup.css (external css within extension package):
> >>>>>
> >>>>> body {
> >>>>>   direction: __MSG_@@bidi_dir__;
> >>>>>   color: black;
> >>>>>   width: 300px;
> >>>>> }
> >>>>>
> >>>>> This doesn't:
> >>>>>
> >>>>> popup.html
> >>>>>
> >>>>> <html><script>...document.write("__MSG_name__")</script></html>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>

[Nebojša Ćirić, 2010-02-10 18:39:53]

Erik, are you ok with current implementation?

[Erik does not do reviews, 2010-02-10 18:53:35]

Yes.  I didn't look at the code though.  I'll defer to Adam, Matt and Jay
here.

On Wed, Feb 10, 2010 at 10:39 AM, <cira@chromium.org> wrote:

> Erik, are you ok with current implementation?
>
>
> http://codereview.chromium.org/570007
>

[Nebojša Ćirić, 2010-02-10 22:22:47]

Thanks, Matt already gave LGTM, waiting on Adam and Jay (no rush).

On 2010/02/10 18:53:35, Erik Kay wrote:
> Yes.  I didn't look at the code though.  I'll defer to Adam, Matt and Jay
> here.
> 
> On Wed, Feb 10, 2010 at 10:39 AM, <mailto:cira@chromium.org> wrote:
> 
> > Erik, are you ok with current implementation?
> >
> >
> > http://codereview.chromium.org/570007
> >
>

[jcampan, 2010-02-10 22:24:54]

LGTM

[abarth-chromium, 2010-02-11 00:14:29]

Looks fine to me.

Adam


On Wed, Feb 10, 2010 at 2:24 PM,  <jcampan@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/570007
>