Whitelisting `127.0.0.1` and `localhost` for HTTP in extensions' CSP.

Whitelisting `127.0.0.1` and `localhost` for HTTP in extensions' CSP.

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

[Mike West, 2012-08-13 10:52:22]

Hi Adam and Mihai!

Would you mind taking a look at this CL? It whitelists HTTP access to 127.0.0.1
and 'localhost' for ease of local development.

I'd considered tying this whitelisting to the state of the "Developer Mode"
checkbox, but I don't think the effort would be worth it. The validator doesn't
know anything about a profile at the moment, and adding support would be
nontrivial.

Thanks!

-mike

[levin, 2012-08-13 11:07:23]

http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
chrome/common/extensions/csp_validator.cc:50: StartsWithASCII(source,
"http://localhost", true) ||
Does this allow http://localhost.mydomain.com/.... ?
If so, then this should be restricted further or not allowed.

[Mike West, 2012-08-13 11:13:33]

On 2012/08/13 11:07:23, levin wrote:
>
http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
> File chrome/common/extensions/csp_validator.cc (right):
> 
>
http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
> chrome/common/extensions/csp_validator.cc:50: StartsWithASCII(source,
> "http://localhost", true) ||
> Does this allow http://localhost.mydomain.com/.... ?
> If so, then this should be restricted further or not allowed.

You're entirely correct. Not sure what I was thinking there. New patch in a few
minutes.

Thanks!

[Mike West, 2012-08-13 11:14:04]

On 2012/08/13 11:13:33, Mike West (chromium) wrote:
> On 2012/08/13 11:07:23, levin wrote:
> >
>
http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
> > File chrome/common/extensions/csp_validator.cc (right):
> > 
> >
>
http://codereview.chromium.org/10855122/diff/1/chrome/common/extensions/csp_v...
> > chrome/common/extensions/csp_validator.cc:50: StartsWithASCII(source,
> > "http://localhost", true) ||
> > Does this allow http://localhost.mydomain.com/.... ?
> > If so, then this should be restricted further or not allowed.
> 
> You're entirely correct. Not sure what I was thinking there. New patch in a
few
> minutes.
> 
> Thanks!

I need to fix both, really, as it would also allow `127.0.0.1.evil.com` in the
current state.

[Mike West, 2012-08-13 11:18:46]

Less stupid patch attached. Thanks!

[abarth-chromium, 2012-08-13 15:50:01]

LGTM (but I'm not an owner)

http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:50: source == "http://localhost" ||
This one should probably be case insensitive.

[abarth-chromium, 2012-08-13 15:50:41]

http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:50: source == "http://localhost" ||
On 2012/08/13 15:50:01, abarth wrote:
> This one should probably be case insensitive.

Do we want to allow non-default ports?  You might want:

StartsWithASCII("http://localhost:" as well.

[levin, 2012-08-13 16:12:21]

On 2012/08/13 15:50:41, abarth wrote:
>
http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
> File chrome/common/extensions/csp_validator.cc (right):
> 
>
http://codereview.chromium.org/10855122/diff/2002/chrome/common/extensions/cs...
> chrome/common/extensions/csp_validator.cc:50: source == "http://localhost" ||
> On 2012/08/13 15:50:01, abarth wrote:
> > This one should probably be case insensitive.
> 
> Do we want to allow non-default ports?  You might want:

Please do :).
> 
> StartsWithASCII("http://localhost:" as well.

[Aaron Boodman, 2012-08-13 18:32:10]

Please don't forget to put this in the docs.

LGTM

[Mike West, 2012-08-14 11:27:09]

On 2012/08/13 18:32:10, Aaron Boodman wrote:
> Please don't forget to put this in the docs.
> 
> LGTM

Good call.

Added to the docs, fixed Adam's nits. Throwing into the CQ.

[commit-bot: I haz the power, 2012-08-14 11:27:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10855122/4003

[commit-bot: I haz the power, 2012-08-14 13:48:28]

Change committed as 151470

[Mihai Parparita -not on Chrome, 2012-10-01 03:23:21]

Somehow the documentation changes for this made into the M22 branch, but
the code is only in the M23 branch (see
http://stackoverflow.com/questions/12645683). Might be worth merging this
into M22?

Mihai

On Tue, Aug 14, 2012 at 6:48 AM, <commit-bot@chromium.org> wrote:

> Change committed as 151470
>
>
https://chromiumcodereview.**appspot.com/10855122/<https://chromiumcodereview...
>

[Mike West, 2012-10-01 15:05:26]

On 2012/10/01 03:23:21, Mihai Parparita wrote:
> Somehow the documentation changes for this made into the M22 branch, but
> the code is only in the M23 branch (see
> http://stackoverflow.com/questions/12645683). Might be worth merging this
> into M22?

Ugh. That's no good.

The new warning string landed in http://crrev.com/156781; I think we wanted to
merge the docs drive-by back to M22, but merging the code change was premature
(that was http://crrev.com/156957).

I think either way is pretty low-risk, so... *shrug* I'll ask. :)

-mike