Disable environment variable override of API keys on official builds.

Disable environment variable override of API keys on official builds.

BUG=710575
Review-Url: https://codereview.chromium.org/2939403002
Cr-Commit-Position: refs/heads/master@{#480962}
Committed: https://chromium.googlesource.com/chromium/src/+/156c7095f41e676fbdd780ead1450be6aaff6d53

[robertshield, 2017-06-16 18:16:37]

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

[commit-bot: I haz the power, 2017-06-16 18:17:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robertshield, 2017-06-16 18:56:56]

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

[commit-bot: I haz the power, 2017-06-16 18:57:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robertshield, 2017-06-16 18:57:55]

Patchset #1 (id:1) has been deleted

[robertshield, 2017-06-16 19:06:27]

robertshield@chromium.org changed reviewers:
+ rogerta@chromium.org

[robertshield, 2017-06-16 19:06:28]

Hi Roger,

  I'm taking a stab at fixing crbug.com/710575 which seems to be caused by users
with funky environment variables that override the google api keys used in
Google Chrome.

  I'm not really sure why we allow overriding API keys via the environment in
official Google Chrome builds, so this disables it. This said, there's a decent
chance this breaks something I've never heard of. 

  Do you know where I should ask to see who's using this functionality? Worst
case I can ask on chromium-dev@

Thanks!
Robert

[commit-bot: I haz the power, 2017-06-16 20:06:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 20:06:06]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-06-19 13:31:13]

chrome-eng-review@google.com may be a good place to broadcast this change.
Thanks for taking a stab at this!

On Fri, Jun 16, 2017, 9:06 PM <robertshield@chromium.org> wrote:

> Reviewers: Roger Tawa
> CL: https://codereview.chromium.org/2939403002/
>
> Message:
> Hi Roger,
>
> I'm taking a stab at fixing crbug.com/710575 which seems to be caused by
> users
> with funky environment variables that override the google api keys used in
> Google Chrome.
>
> I'm not really sure why we allow overriding API keys via the environment in
> official Google Chrome builds, so this disables it. This said, there's a
> decent
> chance this breaks something I've never heard of.
>
> Do you know where I should ask to see who's using this functionality? Worst
> case I can ask on chromium-dev@
>
> Thanks!
> Robert
>
>
> Description:
> Disable environment variable override of API keys on official builds.
>
> BUG=710575
>
> Affected files (+12, -1 lines):
> M google_apis/google_api_keys.cc
> M google_apis/google_api_keys_unittest.cc
>
>
> Index: google_apis/google_api_keys.cc
> diff --git a/google_apis/google_api_keys.cc
> b/google_apis/google_api_keys.cc
> index
>
ab92d1e4dccf7420e8c0b72b49b8f8343c995016..015b254b5494615b540d798ee3be0791c09c0db4
> 100644
> --- a/google_apis/google_api_keys.cc
> +++ b/google_apis/google_api_keys.cc
> @@ -246,7 +246,8 @@ class APIKeyCache {
> // Gets a value for a key. In priority order, this will be the value
> // provided via a command-line switch, the value provided via an
> // environment variable, or finally a value baked into the build.
> - // |command_line_switch| may be NULL.
> + // |command_line_switch| may be NULL. Official Google Chrome builds will
> not
> + // use the value provided by an environment variable.
> static std::string CalculateKeyValue(const char* baked_in_value,
> const char* environment_variable_name,
> const char* command_line_switch,
> @@ -265,11 +266,17 @@ class APIKeyCache {
> << " with value " << key_value << " from Info.plist.";
> }
> #endif
> +
> +#if !defined(GOOGLE_CHROME_BUILD)
> + // Don't allow using the environment to override API keys for official
> + // Google Chrome builds. There have been reports of mangled environments
> + // affecting users (crbug.com/710575).
> if (environment->GetVar(environment_variable_name, &temp)) {
> key_value = temp;
> VLOG(1) << "Overriding API key " << environment_variable_name
> << " with value " << key_value << " from environment variable.";
> }
> +#endif
>
> if (command_line_switch && command_line->HasSwitch(command_line_switch)) {
> key_value = command_line->GetSwitchValueASCII(command_line_switch);
> Index: google_apis/google_api_keys_unittest.cc
> diff --git a/google_apis/google_api_keys_unittest.cc
> b/google_apis/google_api_keys_unittest.cc
> index
>
74765ecaefe2747c79eed074233c5a8b0403d93c..1b0515ffa6949bf6f3c50d1962c1eeeb447246b7
> 100644
> --- a/google_apis/google_api_keys_unittest.cc
> +++ b/google_apis/google_api_keys_unittest.cc
> @@ -394,6 +394,8 @@ TEST_F(GoogleAPIKeysTest, OverrideAllKeys) {
> EXPECT_EQ("SECRET_REMOTING_HOST", secret_remoting_host);
> }
>
> +#if !defined(GOOGLE_CHROME_BUILD)
> +
> // Override all keys using both preprocessor defines and environment
> // variables. The environment variables should win.
> namespace override_all_keys_env {
> @@ -481,6 +483,8 @@ TEST_F(GoogleAPIKeysTest,
> OverrideAllKeysUsingEnvironment) {
> EXPECT_EQ("env-SECRET_REMOTING_HOST", secret_remoting_host);
> }
>
> +#endif // !defined(GOOGLE_CHROME_BUILD)
> +
> #if defined(OS_IOS)
> // Override all keys using both preprocessor defines and setters.
> // Setters should win.
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roger Tawa OOO till Jul 10th, 2017-06-19 14:11:16]

Hi Robert, this may break apps that use chrome code as a webview.  More
specifically, I think the iOS folks may be depending on this.  Please reach out
to ios-web-view-eng@

[Roger Tawa OOO till Jul 10th, 2017-06-20 18:10:28]

lgtm

Thanks for checking Robert.

[robertshield, 2017-06-20 18:35:40]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2017-06-20 18:35:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 21:26:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497983740421020,
"parent_rev": "676e8ccff70ea1c30674e8f97e38296bdd1af8fc", "commit_rev":
"156c7095f41e676fbdd780ead1450be6aaff6d53"}

[commit-bot: I haz the power, 2017-06-20 21:27:04]

Description was changed from

==========
Disable environment variable override of API keys on official builds.

BUG=710575
==========

to

==========
Disable environment variable override of API keys on official builds.

BUG=710575

Review-Url: https://codereview.chromium.org/2939403002
Cr-Commit-Position: refs/heads/master@{#480962}
Committed:
https://chromium.googlesource.com/chromium/src/+/156c7095f41e676fbdd780ead145...
==========

[commit-bot: I haz the power, 2017-06-20 21:27:05]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/156c7095f41e676fbdd780ead145...