Support whitelisting to handle insecure origins as trustworthy origins (chromium)

Support whitelisting to handle insecure origins as trustworthy origins

The whitelist is given and initialized via
--unsafety-treat-insecure-origin-as-secure command-line flag. This needs to be specified with --user-data-dir
to take effect.

Specifying this flag shows a security banner:
"You are using unsupported command-line flag: ....
Stability and security will suffer."

Depends on blink-side patch:
https://codereview.chromium.org/1082173003/

BUG=441605
TEST=manual (to see SW on http works with command-line flag & the security banner is shown)
TEST=unit_tests:SecureOriginWhiteList.UnsafetyTreatInsecureOriginAsSecure
TEST=content_unittests:URLSchemesTest.IsOriginSecure
TEST=content_unittests:ServiceWorkerDispatcherHostTest.*

Committed: https://crrev.com/aaff1f6f8942f714a85a097228dd1e0fb13a36b8
Cr-Commit-Position: refs/heads/master@{#327875}

[kinuko, 2015-04-20 06:22:34]

Patchset #1 (id:1) has been deleted

[kinuko, 2015-04-20 08:15:58]

kinuko@chromium.org changed reviewers:
+ jochen@chromium.org, palmer@chromium.org

[kinuko, 2015-04-20 08:17:10]

jochen@, palmer@: could you take a look?

(I'm slightly unsure how content and chrome wiring should be done for
IsOriginSecure())

[palmer, 2015-04-20 22:16:18]

https://codereview.chromium.org/1072933006/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/bad_flags_prompt.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/bad_flags_prompt.cc:83: // This flag adds whitelisting
to disable the Secure Origin handling.
Nit: to whitelist an exception or 2 is not quite the same as fully disabling the
check. Perhaps just say,

  // This flag allows people to whitelist certain origins as secure, even if
they are not.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:556: bool
ChromeContentClient::IsOriginTrustWorthy(const GURL& url) const {
"Trustworthy" is 1 word, so call this |IsOriginTrustworthy| (lowercase w).

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
File chrome/common/origin_util.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:30: if (content::IsOriginSecure(url))
No, please don't add a |content::IsOriginSecure|. When developing this stuff the
past 2 weeks, we had a big discussion about whether there should be an
|IsOriginSecure| in both chrome:: and content::, and the consensus was no. See
https://codereview.chromium.org/1049533002/

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:48: const OriginSet& GetWhiteListedSecureOrigins()
{
|GetWhiteListedSecureOrigins| and |ClearWhiteListedSecureOrigins| could be in
the anonymous namespace? Do they have any callers other than |IsOriginSecure|?
(I haven't read this whole CL yet.)

For that matter, does |ClearWhiteListedSecureOrigins| have or need to have any
callers? As I imagine it, this feature should be very simple: Additional origins
are added on the command line, parsed and inserted into |g_origin_whitelist|,
and then do not change for the lifetime of the Chrome process. Right?

I think for safety reasons it should stay that way, and there should be no
temptation to make this feature dynamic in any way.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:65: for (const std::string& origin : origins)
Nit: Maybe you can use auto here?

https://codereview.chromium.org/1072933006/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:515: for (const GURL& origin :
origins)
Nit: auto here too?

https://codereview.chromium.org/1072933006/diff/40001/content/public/common/c...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/content/public/common/c...
content/public/common/content_client.cc:116: return IsOriginSecure(url);
So, this calls the 1 in content::, right? This ambiguity is 1 of the things we
wanted to avoid.

https://codereview.chromium.org/1072933006/diff/40001/content/public/common/o...
File content/public/common/origin_util.h (right):

https://codereview.chromium.org/1072933006/diff/40001/content/public/common/o...
content/public/common/origin_util.h:20: // by embedders.
If we keep this, and I'm fairly sure we shouldn't?, we should document that this
function is basically an implementation detail of |chrome::IsOriginSecure|, and
that nobody should call it directly.

[jochen (gone - plz use gerrit), 2015-04-21 09:45:48]

it would be nice if this was a content-only features, so other embedders can use
it as well

[kinuko, 2015-04-21 12:26:07]

Thanks.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
File chrome/common/origin_util.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:30: if (content::IsOriginSecure(url))
On 2015/04/20 22:16:18, palmer wrote:
> No, please don't add a |content::IsOriginSecure|. When developing this stuff
the
> past 2 weeks, we had a big discussion about whether there should be an
> |IsOriginSecure| in both chrome:: and content::, and the consensus was no. See
> https://codereview.chromium.org/1049533002/

Hmm, the discussion there seems very relevant here too.  As SW code needs
IsOriginSecure() in the content level I'm thinking about moving IsOriginSecure()
(or IsOriginTrustworthy()) to //content and adding RegisterSchemeAsSecure() /
RegisterOriginAsSecure() methods exposed by content (chrome will call them to
register chrome-level schemes and whitelists), something similar to what David
suggested in the thread.  Please let me know if this plan doesn't sound good.

[kinuko, 2015-04-21 16:15:48]

Updated, PTAL

https://codereview.chromium.org/1072933006/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/bad_flags_prompt.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/bad_flags_prompt.cc:83: // This flag adds whitelisting
to disable the Secure Origin handling.
On 2015/04/20 22:16:17, palmer wrote:
> Nit: to whitelist an exception or 2 is not quite the same as fully disabling
the
> check. Perhaps just say,
> 
>   // This flag allows people to whitelist certain origins as secure, even if
> they are not.

Done.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:556: bool
ChromeContentClient::IsOriginTrustWorthy(const GURL& url) const {
On 2015/04/20 22:16:18, palmer wrote:
> "Trustworthy" is 1 word, so call this |IsOriginTrustworthy| (lowercase w).

Done.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
File chrome/common/origin_util.cc (right):

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:30: if (content::IsOriginSecure(url))
On 2015/04/21 12:26:07, kinuko wrote:
> On 2015/04/20 22:16:18, palmer wrote:
> > No, please don't add a |content::IsOriginSecure|. When developing this stuff
> the
> > past 2 weeks, we had a big discussion about whether there should be an
> > |IsOriginSecure| in both chrome:: and content::, and the consensus was no.
See
> > https://codereview.chromium.org/1049533002/
> 
> Hmm, the discussion there seems very relevant here too.  As SW code needs
> IsOriginSecure() in the content level I'm thinking about moving
IsOriginSecure()
> (or IsOriginTrustworthy()) to //content and adding RegisterSchemeAsSecure() /
> RegisterOriginAsSecure() methods exposed by content (chrome will call them to
> register chrome-level schemes and whitelists), something similar to what David
> suggested in the thread.  Please let me know if this plan doesn't sound good.

Moved this completely to //content (so the name confusion won't happen,
embedders can add additional schemes/origins and can just call the content's
IsOriginSecure)

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:48: const OriginSet& GetWhiteListedSecureOrigins()
{
On 2015/04/20 22:16:18, palmer wrote:
> |GetWhiteListedSecureOrigins| and |ClearWhiteListedSecureOrigins| could be in
> the anonymous namespace? Do they have any callers other than |IsOriginSecure|?
> (I haven't read this whole CL yet.)
> 
> For that matter, does |ClearWhiteListedSecureOrigins| have or need to have any
> callers? As I imagine it, this feature should be very simple: Additional
origins
> are added on the command line, parsed and inserted into |g_origin_whitelist|,
> and then do not change for the lifetime of the Chrome process. Right?
> 
> I think for safety reasons it should stay that way, and there should be no
> temptation to make this feature dynamic in any way.

This is added only for testing. I renamed this so that it becomes clear (and
presubmit warns) that it's only for testing.

https://codereview.chromium.org/1072933006/diff/40001/chrome/common/origin_ut...
chrome/common/origin_util.cc:65: for (const std::string& origin : origins)
On 2015/04/20 22:16:18, palmer wrote:
> Nit: Maybe you can use auto here?

Done.

[kinuko, 2015-04-22 12:42:41]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 12:43:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072933006/100001

[kinuko, 2015-04-22 13:09:34]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2015-04-23 01:19:39]

Rebased on ToT. Could you PTAL? We want this to land. (If it helps I can split
the patch into two: one for origin_util refactoring/moving and the other for
command-line handling)

[kinuko, 2015-04-23 01:59:06]

On 2015/04/23 01:19:39, kinuko wrote:
> Rebased on ToT. Could you PTAL? We want this to land. (If it helps I can split
> the patch into two: one for origin_util refactoring/moving and the other for
> command-line handling)

Split this change into two.
* https://codereview.chromium.org/1101033003/ for moving IsOriginSecure into
content, and
* this patch for introducing the command-line option (depends on the former
patch)

[palmer, 2015-04-23 19:21:16]

LGTM, thanks!

[kinuko, 2015-04-24 04:46:59]

jochen@ could you do owner review? Thanks,

[jochen (gone - plz use gerrit), 2015-04-24 14:04:44]

can we move the command line option to content as well, so we can also test with
content shell?

[kinuko, 2015-04-24 22:01:20]

On 2015/04/24 14:04:44, jochen wrote:
> can we move the command line option to content as well, so we can also test
with
> content shell?

I wanted so, but we need to check if it's given with kUserDayaDir, which is in
chrome.

[jochen (gone - plz use gerrit), 2015-04-27 19:30:16]

On 2015/04/24 at 22:01:20, kinuko wrote:
> On 2015/04/24 14:04:44, jochen wrote:
> > can we move the command line option to content as well, so we can also test
with
> > content shell?
> 
> I wanted so, but we need to check if it's given with kUserDayaDir, which is in
chrome.

can you explain why we require this?

[kinuko, 2015-04-28 03:11:56]

On 2015/04/27 19:30:16, jochen wrote:
> On 2015/04/24 at 22:01:20, kinuko wrote:
> > On 2015/04/24 14:04:44, jochen wrote:
> > > can we move the command line option to content as well, so we can also
test
> with
> > > content shell?
> > 
> > I wanted so, but we need to check if it's given with kUserDayaDir, which is
in
> chrome.
> 
> can you explain why we require this?

The bug crbug.com/441605 has some context (I just cc'ed), but in short: with
this option one could make persisting changes that survive browser restarts
(e.g. registering a ServiceWorker for insecure origin etc), but such state
should not be left after browser restarts without the option.  We have thought
about requiring incognito mode but it defeats the usefulness of this option for
testing (as incognito mode also changes various behavior).  Requiring
--user-data-dir (so that it won't at least leave persist state in the user's
default profile) was the practical solution we roughly agreed with.

[kinuko, 2015-04-28 10:51:34]

Patchset #9 (id:180001) has been deleted

[jochen (gone - plz use gerrit), 2015-04-28 12:01:57]

On 2015/04/28 at 03:11:56, kinuko wrote:
> On 2015/04/27 19:30:16, jochen wrote:
> > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > On 2015/04/24 14:04:44, jochen wrote:
> > > > can we move the command line option to content as well, so we can also
test
> > with
> > > > content shell?
> > > 
> > > I wanted so, but we need to check if it's given with kUserDayaDir, which
is in
> > chrome.
> > 
> > can you explain why we require this?
> 
> The bug crbug.com/441605 has some context (I just cc'ed), but in short: with
this option one could make persisting changes that survive browser restarts
(e.g. registering a ServiceWorker for insecure origin etc), but such state
should not be left after browser restarts without the option.  We have thought
about requiring incognito mode but it defeats the usefulness of this option for
testing (as incognito mode also changes various behavior).  Requiring
--user-data-dir (so that it won't at least leave persist state in the user's
default profile) was the practical solution we roughly agreed with.

why don't we just not run service workers from insecure origins?

[kinuko, 2015-04-28 13:05:58]

On 2015/04/28 12:01:57, jochen wrote:
> On 2015/04/28 at 03:11:56, kinuko wrote:
> > On 2015/04/27 19:30:16, jochen wrote:
> > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > can we move the command line option to content as well, so we can also
> test
> > > with
> > > > > content shell?
> > > > 
> > > > I wanted so, but we need to check if it's given with kUserDayaDir, which
> is in
> > > chrome.
> > > 
> > > can you explain why we require this?
> > 
> > The bug crbug.com/441605 has some context (I just cc'ed), but in short: with
> this option one could make persisting changes that survive browser restarts
> (e.g. registering a ServiceWorker for insecure origin etc), but such state
> should not be left after browser restarts without the option.  We have thought
> about requiring incognito mode but it defeats the usefulness of this option
for
> testing (as incognito mode also changes various behavior).  Requiring
> --user-data-dir (so that it won't at least leave persist state in the user's
> default profile) was the practical solution we roughly agreed with.
> 
> why don't we just not run service workers from insecure origins?

Are you asking why we don't add a simple option that just allows service workers
to run from secure origins?  Does the following comment by jww@
(https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?

"Whitelists discourage absent-minded mistakes, and given how dangerous it is to
have this enabled, it's important that developers be explicit about where they
want it turned on."

If this doesn't make sense or you have other ideas could we discuss this on the
bug: https://crbug.com/441605

[jochen (gone - plz use gerrit), 2015-04-28 13:40:35]

On 2015/04/28 at 13:05:58, kinuko wrote:
> On 2015/04/28 12:01:57, jochen wrote:
> > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > On 2015/04/27 19:30:16, jochen wrote:
> > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > can we move the command line option to content as well, so we can
also
> > test
> > > > with
> > > > > > content shell?
> > > > > 
> > > > > I wanted so, but we need to check if it's given with kUserDayaDir,
which
> > is in
> > > > chrome.
> > > > 
> > > > can you explain why we require this?
> > > 
> > > The bug crbug.com/441605 has some context (I just cc'ed), but in short:
with
> > this option one could make persisting changes that survive browser restarts
> > (e.g. registering a ServiceWorker for insecure origin etc), but such state
> > should not be left after browser restarts without the option.  We have
thought
> > about requiring incognito mode but it defeats the usefulness of this option
for
> > testing (as incognito mode also changes various behavior).  Requiring
> > --user-data-dir (so that it won't at least leave persist state in the user's
> > default profile) was the practical solution we roughly agreed with.
> > 
> > why don't we just not run service workers from insecure origins?
> 
> Are you asking why we don't add a simple option that just allows service
workers to run from secure origins?  Does the following comment by jww@
(https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?
> 
> "Whitelists discourage absent-minded mistakes, and given how dangerous it is
to have this enabled, it's important that developers be explicit about where
they want it turned on."
> 
> If this doesn't make sense or you have other ideas could we discuss this on
the bug: https://crbug.com/441605

no, that's not what I meant.

My question is why can you make persistent changes? E.g. if you register a
service worker on an insecure origin, and then restart without the command line
flag, why don't we just discard the service worker?

[kinuko, 2015-04-28 14:09:38]

On 2015/04/28 13:40:35, jochen wrote:
> On 2015/04/28 at 13:05:58, kinuko wrote:
> > On 2015/04/28 12:01:57, jochen wrote:
> > > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > > On 2015/04/27 19:30:16, jochen wrote:
> > > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > > can we move the command line option to content as well, so we can
> also
> > > test
> > > > > with
> > > > > > > content shell?
> > > > > > 
> > > > > > I wanted so, but we need to check if it's given with kUserDayaDir,
> which
> > > is in
> > > > > chrome.
> > > > > 
> > > > > can you explain why we require this?
> > > > 
> > > > The bug crbug.com/441605 has some context (I just cc'ed), but in short:
> with
> > > this option one could make persisting changes that survive browser
restarts
> > > (e.g. registering a ServiceWorker for insecure origin etc), but such state
> > > should not be left after browser restarts without the option.  We have
> thought
> > > about requiring incognito mode but it defeats the usefulness of this
option
> for
> > > testing (as incognito mode also changes various behavior).  Requiring
> > > --user-data-dir (so that it won't at least leave persist state in the
user's
> > > default profile) was the practical solution we roughly agreed with.
> > > 
> > > why don't we just not run service workers from insecure origins?
> > 
> > Are you asking why we don't add a simple option that just allows service
> workers to run from secure origins?  Does the following comment by jww@
> (https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?
> > 
> > "Whitelists discourage absent-minded mistakes, and given how dangerous it is
> to have this enabled, it's important that developers be explicit about where
> they want it turned on."
> > 
> > If this doesn't make sense or you have other ideas could we discuss this on
> the bug: https://crbug.com/441605
> 
> no, that's not what I meant.
> 
> My question is why can you make persistent changes? E.g. if you register a
> service worker on an insecure origin, and then restart without the command
line
> flag, why don't we just discard the service worker?

If we want to reset when the browser's restarted without the command line we
should also discard any changes that are made on persistent storage (e.g. cache
storage, idb or any other) for consistency, which is tough to implement and will
complicate the code. If just asking developers to also supply --user-data-dir
works fine that's definitely simpler-- that was our take.

[jochen (gone - plz use gerrit), 2015-04-28 14:12:30]

On 2015/04/28 at 14:09:38, kinuko wrote:
> On 2015/04/28 13:40:35, jochen wrote:
> > On 2015/04/28 at 13:05:58, kinuko wrote:
> > > On 2015/04/28 12:01:57, jochen wrote:
> > > > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > > > On 2015/04/27 19:30:16, jochen wrote:
> > > > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > > > can we move the command line option to content as well, so we
can
> > also
> > > > test
> > > > > > with
> > > > > > > > content shell?
> > > > > > > 
> > > > > > > I wanted so, but we need to check if it's given with kUserDayaDir,
> > which
> > > > is in
> > > > > > chrome.
> > > > > > 
> > > > > > can you explain why we require this?
> > > > > 
> > > > > The bug crbug.com/441605 has some context (I just cc'ed), but in
short:
> > with
> > > > this option one could make persisting changes that survive browser
restarts
> > > > (e.g. registering a ServiceWorker for insecure origin etc), but such
state
> > > > should not be left after browser restarts without the option.  We have
> > thought
> > > > about requiring incognito mode but it defeats the usefulness of this
option
> > for
> > > > testing (as incognito mode also changes various behavior).  Requiring
> > > > --user-data-dir (so that it won't at least leave persist state in the
user's
> > > > default profile) was the practical solution we roughly agreed with.
> > > > 
> > > > why don't we just not run service workers from insecure origins?
> > > 
> > > Are you asking why we don't add a simple option that just allows service
> > workers to run from secure origins?  Does the following comment by jww@
> > (https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?
> > > 
> > > "Whitelists discourage absent-minded mistakes, and given how dangerous it
is
> > to have this enabled, it's important that developers be explicit about where
> > they want it turned on."
> > > 
> > > If this doesn't make sense or you have other ideas could we discuss this
on
> > the bug: https://crbug.com/441605
> > 
> > no, that's not what I meant.
> > 
> > My question is why can you make persistent changes? E.g. if you register a
> > service worker on an insecure origin, and then restart without the command
line
> > flag, why don't we just discard the service worker?
> 
> If we want to reset when the browser's restarted without the command line we
should also discard any changes that are made on persistent storage (e.g. cache
storage, idb or any other) for consistency, which is tough to implement and will
complicate the code. If just asking developers to also supply --user-data-dir
works fine that's definitely simpler-- that was our take.

i'm not sure why we'd need to do that. We also don't do that for other scary
options such as --disable-web-security or --allow-file-access-from-files

[kinuko, 2015-04-28 14:13:48]

kinuko@chromium.org changed reviewers:
+ jww@chromium.org

[kinuko, 2015-04-28 14:32:08]

On 2015/04/28 14:12:30, jochen wrote:
> On 2015/04/28 at 14:09:38, kinuko wrote:
> > On 2015/04/28 13:40:35, jochen wrote:
> > > On 2015/04/28 at 13:05:58, kinuko wrote:
> > > > On 2015/04/28 12:01:57, jochen wrote:
> > > > > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > > > > On 2015/04/27 19:30:16, jochen wrote:
> > > > > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > > > > can we move the command line option to content as well, so we
> can
> > > also
> > > > > test
> > > > > > > with
> > > > > > > > > content shell?
> > > > > > > > 
> > > > > > > > I wanted so, but we need to check if it's given with
kUserDayaDir,
> > > which
> > > > > is in
> > > > > > > chrome.
> > > > > > > 
> > > > > > > can you explain why we require this?
> > > > > > 
> > > > > > The bug crbug.com/441605 has some context (I just cc'ed), but in
> short:
> > > with
> > > > > this option one could make persisting changes that survive browser
> restarts
> > > > > (e.g. registering a ServiceWorker for insecure origin etc), but such
> state
> > > > > should not be left after browser restarts without the option.  We have
> > > thought
> > > > > about requiring incognito mode but it defeats the usefulness of this
> option
> > > for
> > > > > testing (as incognito mode also changes various behavior).  Requiring
> > > > > --user-data-dir (so that it won't at least leave persist state in the
> user's
> > > > > default profile) was the practical solution we roughly agreed with.
> > > > > 
> > > > > why don't we just not run service workers from insecure origins?
> > > > 
> > > > Are you asking why we don't add a simple option that just allows service
> > > workers to run from secure origins?  Does the following comment by jww@
> > > (https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?
> > > > 
> > > > "Whitelists discourage absent-minded mistakes, and given how dangerous
it
> is
> > > to have this enabled, it's important that developers be explicit about
where
> > > they want it turned on."
> > > > 
> > > > If this doesn't make sense or you have other ideas could we discuss this
> on
> > > the bug: https://crbug.com/441605
> > > 
> > > no, that's not what I meant.
> > > 
> > > My question is why can you make persistent changes? E.g. if you register a
> > > service worker on an insecure origin, and then restart without the command
> line
> > > flag, why don't we just discard the service worker?
> > 
> > If we want to reset when the browser's restarted without the command line we
> should also discard any changes that are made on persistent storage (e.g.
cache
> storage, idb or any other) for consistency, which is tough to implement and
will
> complicate the code. If just asking developers to also supply --user-data-dir
> works fine that's definitely simpler-- that was our take.
> 
> i'm not sure why we'd need to do that. We also don't do that for other scary
> options such as --disable-web-security or --allow-file-access-from-files

Let me call jww@ for his thoughts.

Because this option enables insecure ServiceWorker which is scarier compared
with what we had before, namely it could persist and run even without opening
the page?  And I assume that's why you suggest that we just discard
ServiceWorkers that were registered with the option (but don't care about other
persistent states).  I don't really feel adding a logic to discard
ServiceWorkers for insecure origins at browser restart makes much sense if
that's only for testing, as it adds extra overhead at startup and complexity, as
well as we could leave the origin's persistent storage in an inconsistent state.

[jochen (gone - plz use gerrit), 2015-04-29 09:43:50]

On 2015/04/28 at 14:32:08, kinuko wrote:
> On 2015/04/28 14:12:30, jochen wrote:
> > On 2015/04/28 at 14:09:38, kinuko wrote:
> > > On 2015/04/28 13:40:35, jochen wrote:
> > > > On 2015/04/28 at 13:05:58, kinuko wrote:
> > > > > On 2015/04/28 12:01:57, jochen wrote:
> > > > > > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > > > > > On 2015/04/27 19:30:16, jochen wrote:
> > > > > > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > > > > > can we move the command line option to content as well, so
we
> > can
> > > > also
> > > > > > test
> > > > > > > > with
> > > > > > > > > > content shell?
> > > > > > > > > 
> > > > > > > > > I wanted so, but we need to check if it's given with
kUserDayaDir,
> > > > which
> > > > > > is in
> > > > > > > > chrome.
> > > > > > > > 
> > > > > > > > can you explain why we require this?
> > > > > > > 
> > > > > > > The bug crbug.com/441605 has some context (I just cc'ed), but in
> > short:
> > > > with
> > > > > > this option one could make persisting changes that survive browser
> > restarts
> > > > > > (e.g. registering a ServiceWorker for insecure origin etc), but such
> > state
> > > > > > should not be left after browser restarts without the option.  We
have
> > > > thought
> > > > > > about requiring incognito mode but it defeats the usefulness of this
> > option
> > > > for
> > > > > > testing (as incognito mode also changes various behavior). 
Requiring
> > > > > > --user-data-dir (so that it won't at least leave persist state in
the
> > user's
> > > > > > default profile) was the practical solution we roughly agreed with.
> > > > > > 
> > > > > > why don't we just not run service workers from insecure origins?
> > > > > 
> > > > > Are you asking why we don't add a simple option that just allows
service
> > > > workers to run from secure origins?  Does the following comment by jww@
> > > > (https://code.google.com/p/chromium/issues/detail?id=441605#c10) answer?
> > > > > 
> > > > > "Whitelists discourage absent-minded mistakes, and given how dangerous
it
> > is
> > > > to have this enabled, it's important that developers be explicit about
where
> > > > they want it turned on."
> > > > > 
> > > > > If this doesn't make sense or you have other ideas could we discuss
this
> > on
> > > > the bug: https://crbug.com/441605
> > > > 
> > > > no, that's not what I meant.
> > > > 
> > > > My question is why can you make persistent changes? E.g. if you register
a
> > > > service worker on an insecure origin, and then restart without the
command
> > line
> > > > flag, why don't we just discard the service worker?
> > > 
> > > If we want to reset when the browser's restarted without the command line
we
> > should also discard any changes that are made on persistent storage (e.g.
cache
> > storage, idb or any other) for consistency, which is tough to implement and
will
> > complicate the code. If just asking developers to also supply
--user-data-dir
> > works fine that's definitely simpler-- that was our take.
> > 
> > i'm not sure why we'd need to do that. We also don't do that for other scary
> > options such as --disable-web-security or --allow-file-access-from-files
> 
> Let me call jww@ for his thoughts.
> 
> Because this option enables insecure ServiceWorker which is scarier compared
with what we had before, namely it could persist and run even without opening
the page?  And I assume that's why you suggest that we just discard
ServiceWorkers that were registered with the option (but don't care about other
persistent states).  I don't really feel adding a logic to discard
ServiceWorkers for insecure origins at browser restart makes much sense if
that's only for testing, as it adds extra overhead at startup and complexity, as
well as we could leave the origin's persistent storage in an inconsistent state.

I would assume that before we fire up a service worker, we'd check that this is
actually allowed? I wouldn't go through the list of all workers and delete them,
but I'd hope that we just ignore requests to start insecure ones.

[kinuko, 2015-04-29 10:29:32]

On 2015/04/29 09:43:50, jochen wrote:
> On 2015/04/28 at 14:32:08, kinuko wrote:
> > On 2015/04/28 14:12:30, jochen wrote:
> > > On 2015/04/28 at 14:09:38, kinuko wrote:
> > > > On 2015/04/28 13:40:35, jochen wrote:
> > > > > On 2015/04/28 at 13:05:58, kinuko wrote:
> > > > > > On 2015/04/28 12:01:57, jochen wrote:
> > > > > > > On 2015/04/28 at 03:11:56, kinuko wrote:
> > > > > > > > On 2015/04/27 19:30:16, jochen wrote:
> > > > > > > > > On 2015/04/24 at 22:01:20, kinuko wrote:
> > > > > > > > > > On 2015/04/24 14:04:44, jochen wrote:
> > > > > > > > > > > can we move the command line option to content as well, so
> we
> > > can
> > > > > also
> > > > > > > test
> > > > > > > > > with
> > > > > > > > > > > content shell?
> > > > > > > > > > 
> > > > > > > > > > I wanted so, but we need to check if it's given with
> kUserDayaDir,
> > > > > which
> > > > > > > is in
> > > > > > > > > chrome.
> > > > > > > > > 
> > > > > > > > > can you explain why we require this?
> > > > > > > > 
> > > > > > > > The bug crbug.com/441605 has some context (I just cc'ed), but in
> > > short:
> > > > > with
> > > > > > > this option one could make persisting changes that survive browser
> > > restarts
> > > > > > > (e.g. registering a ServiceWorker for insecure origin etc), but
such
> > > state
> > > > > > > should not be left after browser restarts without the option.  We
> have
> > > > > thought
> > > > > > > about requiring incognito mode but it defeats the usefulness of
this
> > > option
> > > > > for
> > > > > > > testing (as incognito mode also changes various behavior). 
> Requiring
> > > > > > > --user-data-dir (so that it won't at least leave persist state in
> the
> > > user's
> > > > > > > default profile) was the practical solution we roughly agreed
with.
> > > > > > > 
> > > > > > > why don't we just not run service workers from insecure origins?
> > > > > > 
> > > > > > Are you asking why we don't add a simple option that just allows
> service
> > > > > workers to run from secure origins?  Does the following comment by
jww@
> > > > > (https://code.google.com/p/chromium/issues/detail?id=441605#c10)
answer?
> > > > > > 
> > > > > > "Whitelists discourage absent-minded mistakes, and given how
dangerous
> it
> > > is
> > > > > to have this enabled, it's important that developers be explicit about
> where
> > > > > they want it turned on."
> > > > > > 
> > > > > > If this doesn't make sense or you have other ideas could we discuss
> this
> > > on
> > > > > the bug: https://crbug.com/441605
> > > > > 
> > > > > no, that's not what I meant.
> > > > > 
> > > > > My question is why can you make persistent changes? E.g. if you
register
> a
> > > > > service worker on an insecure origin, and then restart without the
> command
> > > line
> > > > > flag, why don't we just discard the service worker?
> > > > 
> > > > If we want to reset when the browser's restarted without the command
line
> we
> > > should also discard any changes that are made on persistent storage (e.g.
> cache
> > > storage, idb or any other) for consistency, which is tough to implement
and
> will
> > > complicate the code. If just asking developers to also supply
> --user-data-dir
> > > works fine that's definitely simpler-- that was our take.
> > > 
> > > i'm not sure why we'd need to do that. We also don't do that for other
scary
> > > options such as --disable-web-security or --allow-file-access-from-files
> > 
> > Let me call jww@ for his thoughts.
> > 
> > Because this option enables insecure ServiceWorker which is scarier compared
> with what we had before, namely it could persist and run even without opening
> the page?  And I assume that's why you suggest that we just discard
> ServiceWorkers that were registered with the option (but don't care about
other
> persistent states).  I don't really feel adding a logic to discard
> ServiceWorkers for insecure origins at browser restart makes much sense if
> that's only for testing, as it adds extra overhead at startup and complexity,
as
> well as we could leave the origin's persistent storage in an inconsistent
state.
> 
> I would assume that before we fire up a service worker, we'd check that this
is
> actually allowed? I wouldn't go through the list of all workers and delete
them,
> but I'd hope that we just ignore requests to start insecure ones.

Let's discuss this in another thread. I'll cc you.

[jochen (gone - plz use gerrit), 2015-04-30 07:15:32]

https://codereview.chromium.org/1072933006/diff/220001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1072933006/diff/220001/content/renderer/rende...
content/renderer/render_thread_impl.cc:391: void
RegisterSecureOriginsWhitelist() {
this can be moved to chrome now, no? somewhere in chrome content renderer client

[kinuko, 2015-04-30 08:16:25]

https://codereview.chromium.org/1072933006/diff/220001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1072933006/diff/220001/content/renderer/rende...
content/renderer/render_thread_impl.cc:391: void
RegisterSecureOriginsWhitelist() {
On 2015/04/30 07:15:32, jochen wrote:
> this can be moved to chrome now, no? somewhere in chrome content renderer
client

Oops, yes actually I already had the code in chrome_content_renderer_client.cc,
should have just reverted the changes in this file. Done.

[jochen (gone - plz use gerrit), 2015-04-30 08:17:00]

lgtm

[jww, 2015-04-30 18:26:42]

lgtm, with minor test suggestion.

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist_unittest.cc (right):

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist_unittest.cc:5: #include
"base/basictypes.h"
remove?

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist_unittest.cc:33:
EXPECT_TRUE(content::IsOriginSecure(GURL("http://127.example.com/fun.html")));
Would you mind adding a few other sanity checks, such as verifing that
http://128.example.com is *not* considered secure, or similarly that
http://foobar.127.example.com is also not secure?

[kinuko, 2015-05-01 03:10:57]

Thanks, updated, will be landing.

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
File chrome/common/secure_origin_whitelist_unittest.cc (right):

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist_unittest.cc:5: #include
"base/basictypes.h"
On 2015/04/30 18:26:42, jww wrote:
> remove?

Done.

https://codereview.chromium.org/1072933006/diff/240001/chrome/common/secure_o...
chrome/common/secure_origin_whitelist_unittest.cc:33:
EXPECT_TRUE(content::IsOriginSecure(GURL("http://127.example.com/fun.html")));
On 2015/04/30 18:26:42, jww wrote:
> Would you mind adding a few other sanity checks, such as verifing that
> http://128.example.com is *not* considered secure, or similarly that
> http://foobar.127.example.com is also not secure?

Done.

[kinuko, 2015-05-01 03:11:02]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-05-01 03:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
jww@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1072933006/#ps260001
(title: "added more tests")

[commit-bot: I haz the power, 2015-05-01 03:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072933006/260001

[commit-bot: I haz the power, 2015-05-01 04:04:31]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2015-05-01 04:05:33]

Patchset 12 (id:??) landed as
https://crrev.com/aaff1f6f8942f714a85a097228dd1e0fb13a36b8
Cr-Commit-Position: refs/heads/master@{#327875}