Allow NaCl for non web store apps if their top level URL is whitelisted.

Allow NaCl for non web store apps if their top level URL is whitelisted.
Refactored NaCl restriction logic and added unit tests.
BUG=113668
TEST=unit_tests

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

[bbudge, 2012-02-09 23:26:07]

jam for OWNERS
abarth for security scan

[jam, 2012-02-10 17:54:17]

On 2012/02/09 23:26:07, bbudge1 wrote:
> jam for OWNERS
> abarth for security scan

is there a bug with more background? i'm always a bit apprehensive when we
hardcode google urls to whitelist specific features for.

[abarth-chromium, 2012-02-10 18:32:52]

Why are we hard-coding extra privileges for google.com?  That doesn't seem like
a good thing for the open web platform.

[Erik does not do reviews, 2012-02-10 18:37:26]

On 2012/02/10 18:32:52, abarth wrote:
> Why are we hard-coding extra privileges for google.com?  That doesn't seem
like
> a good thing for the open web platform.

I filed crbug.com/113668 to track the issue.  Happy to chat with folks in person
if you'd like more background than the bug provides.

[abarth-chromium, 2012-02-10 18:42:22]

> I filed crbug.com/113668 to track the issue.  Happy to chat with folks in
person
> if you'd like more background than the bug provides.

Thanks.  I wouldn't be reviewing this CL, but that shouldn't stop other folks
from reviewing it.

[jam, 2012-02-10 20:13:44]

On 2012/02/10 18:42:22, abarth wrote:
> > I filed crbug.com/113668 to track the issue.  Happy to chat with folks in
> person
> > if you'd like more background than the bug provides.
> 
> Thanks.  I wouldn't be reviewing this CL, but that shouldn't stop other folks
> from reviewing it.

btw there's no OWNERS in chrome\renderer.. so anyone can lgtm this. perhaps send
this to someone working on nacl?

[bbudge, 2012-02-10 20:44:08]

Change reviewers for folks familiar with NaCl and apps.

[Mihai Parparita -not on Chrome, 2012-02-10 23:03:33]

http://codereview.chromium.org/9368046/diff/3003/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9368046/diff/3003/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:488: bool
ChromeContentRendererClient::IsNaClAllowed(
This is complex enough that a unit tests would be good to have (you might need
to change some of the parameters to make this more testable, e.g. pass in the
top frame URL directly).

http://codereview.chromium.org/9368046/diff/3003/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:533: if
(top_url.SchemeIs("http") || top_url.SchemeIs("https")) {
Nit: why not merge this into the whilelisted_url boolean expression (i.e.
whitelisted_url = (top_url.SchemeIs("http") || top_url.SchemeIs("https")) && ...

[bbudge, 2012-02-17 00:54:06]

I refactored the NaCl restriction logic into two static methods, and added unit
tests for them.

[bbudge, 2012-02-17 00:57:30]

Fix lint errors.

[Mihai Parparita -not on Chrome, 2012-02-17 01:19:13]

LGTM

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:500: GURL
ChromeContentRendererClient::GetNaClContentHandlerURL(
Nit: static functions usually have a "// static" comment (applies to
IsNaClAllowed too).

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client_unittest.cc (right):

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:62:
EXPECT_EQ(ChromeContentRendererClient::GetNaClContentHandlerURL(
Nit: the param ordering of EXPECT_EQ is expected, then actual.

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:76: GURL(), GURL(),
true, false, false, params));
All the boolean parameters make the tests harder to follow. Perhaps you can add
some constants at the start of the test (e.g. nacl_unrestricted and
nacl_restricted) to make it easier to read.

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:125: GURL(),
GURL("http://plus.google.com/games"),
Mind also adding a full games URL (like
https://plus.google.com/games/209089085730?).

[bbudge, 2012-02-17 19:25:04]

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:500: GURL
ChromeContentRendererClient::GetNaClContentHandlerURL(
On 2012/02/17 01:19:13, Mihai Parparita wrote:
> Nit: static functions usually have a "// static" comment (applies to
> IsNaClAllowed too).

Done.

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client_unittest.cc (right):

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:62:
EXPECT_EQ(ChromeContentRendererClient::GetNaClContentHandlerURL(
On 2012/02/17 01:19:13, Mihai Parparita wrote:
> Nit: the param ordering of EXPECT_EQ is expected, then actual.

Done.

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:76: GURL(), GURL(),
true, false, false, params));
Done. Good idea.
On 2012/02/17 01:19:13, Mihai Parparita wrote:
> All the boolean parameters make the tests harder to follow. Perhaps you can
add
> some constants at the start of the test (e.g. nacl_unrestricted and
> nacl_restricted) to make it easier to read.

http://codereview.chromium.org/9368046/diff/17002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client_unittest.cc:125: GURL(),
GURL("http://plus.google.com/games"),
On 2012/02/17 01:19:13, Mihai Parparita wrote:
> Mind also adding a full games URL (like
> https://plus.google.com/games/209089085730?).

Done.

[Mihai Parparita -not on Chrome, 2012-02-17 20:29:46]

Still LGTM

[bbudge, 2012-02-18 00:07:39]

Patch got out of date.

[commit-bot: I haz the power, 2012-02-18 00:07:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/9368046/25003

[bbudge, 2012-02-18 11:17:35]

Whoa, almost broke plugins. Reversed the sense of the NaCl plugin test and
create plugin in non-NaCl cases.

[commit-bot: I haz the power, 2012-02-18 13:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/9368046/23007

[commit-bot: I haz the power, 2012-02-19 00:11:11]

Change committed as 122709

[Chris Evans, 2013-01-30 02:26:26]

On 2012/02/19 00:11:11, I haz the power (commit-bot) wrote:
> Change committed as 122709

John / Adam / Erik -- feel free to bring in the Chrome Security Team in the
future if you ever see anything that seems to be granting a special privilege to
a web page.

Please r- anything if it's ever giving a special privilege to an http URL.

[Erik does not do reviews, 2013-10-04 18:50:43]

I removed the restrict access to the bug (crbug.com/113688) and added some more
context there.

[bbudge-google, 2013-10-04 18:53:54]

Bug link is off:
https://code.google.com/p/chromium/issues/detail?id=113668

-Bill


On Fri, Oct 4, 2013 at 11:50 AM, <erikkay@chromium.org> wrote:

> I removed the restrict access to the bug (crbug.com/113688) and added
> some more
> context there.
>
>
>
https://codereview.chromium.**org/9368046/<https://codereview.chromium.org/93...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.