Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108853002:
Restrict use of two app-launching command line flags (Closed)
|
Created:
4 years, 5 months ago by proberge Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestrict use of two app-launching command line flags
Mitigate abuse of the load-and-launch-app and load-apps flags by
disallowing loading of non-app extensions from these flags.
For the official build, the flags no-op instead of showing an error popup
to avoid giving users with hijacked shortcuts a bad time (the error
message is shown then Chrome shuts down)
BUG=624098
Committed: https://crrev.com/93c60cd3ef5908984e173112dc09345bb5714036
Cr-Commit-Position: refs/heads/master@{#407824}
Patch Set 1 #Patch Set 2 : Prevent loading of extensions from the app flags instead #Patch Set 3 : revert change to documentation file #
Total comments: 7
Patch Set 4 : Add a browsertest #
Total comments: 2
Patch Set 5 : Add /* explanation */ for the bool parameter #
Total comments: 20
Patch Set 6 : Apply mws' comments #
Total comments: 10
Patch Set 7 : Fix last nits #Patch Set 8 : Use ASCIIToUTF16 to make mac tests happy #
Messages
Total messages: 42 (17 generated)
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by making them into no-ops on Official Chrome builds if the user's profile is not in extension developer mode. The flags no-op instead of showing an error message similar to kUnpackedExtensionsBlacklistedError to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG= ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by making them into no-ops on Official Chrome builds if the user's profile is not in extension developer mode. The flags no-op instead of showing an error message similar to kUnpackedExtensionsBlacklistedError to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG= ==========
proberge@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
I'm still a little wary of doing this with no real data around how frequently this is used. Would it be reasonable to add some kind of UMA around this first to find out how many calls this would block? (Also, we should have an associated BUG= for this.)
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by making them into no-ops on Official Chrome builds if the user's profile is not in extension developer mode. The flags no-op instead of showing an error message similar to kUnpackedExtensionsBlacklistedError to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG= ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by making them into no-ops on Official Chrome builds if the user's profile is not in extension developer mode. The flags no-op instead of showing an error message similar to kUnpackedExtensionsBlacklistedError to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
On 2016/06/28 20:08:40, Devlin wrote: > I'm still a little wary of doing this with no real data around how frequently > this is used. Would it be reasonable to add some kind of UMA around this first > to find out how many calls this would block? > > (Also, we should have an associated BUG= for this.) Sent you an UMA link and added a bug number.
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by making them into no-ops on Official Chrome builds if the user's profile is not in extension developer mode. The flags no-op instead of showing an error message similar to kUnpackedExtensionsBlacklistedError to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from thees flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from thees flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
PTAL
Also, can we add a test? https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:160: if (only_allow_apps && !extension()->is_app()) { If we don't opt for the std::set<Type> allowed_types approach, we should restrict this to is_platform_app(), rather than is_app(). We don't want to allow loading of legacy packaged apps here. Otherwise, this can just be allowed_types.count(extension()->type()). https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:162: // Avoid crashing for users with hijacked shortcuts. I can understand the motivation to avoid crashing, but if this fails, we just pop up a dialog saying the extension can't be loaded. Since that doesn't brick the user's chrome, it seems like it would also serve as an indicator that something was wrong, and allow them to fix it. If a user has one bad commandline flag, it makes sense they might have more issues, so I'd rather we let them know so they can take action to get back into a good state, rather than just try to continue gracefully and give no indication. https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.h (right): https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); nit: I think this would be more flexible if we passed in a std::set<Manifest::Type> allowed_types (perhaps making it a ptr so that null implied all types).
proberge@chromium.org changed reviewers: + mek@chromium.org, pkasting@chromium.org
https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:160: if (only_allow_apps && !extension()->is_app()) { On 2016/06/29 14:59:54, Devlin wrote: > If we don't opt for the std::set<Type> allowed_types approach, we should > restrict this to is_platform_app(), rather than is_app(). We don't want to > allow loading of legacy packaged apps here. > > Otherwise, this can just be allowed_types.count(extension()->type()). Done. https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.cc:162: // Avoid crashing for users with hijacked shortcuts. On 2016/06/29 14:59:54, Devlin wrote: > I can understand the motivation to avoid crashing, but if this fails, we just > pop up a dialog saying the extension can't be loaded. Since that doesn't brick > the user's chrome, it seems like it would also serve as an indicator that > something was wrong, and allow them to fix it. If a user has one bad > commandline flag, it makes sense they might have more issues, so I'd rather we > let them know so they can take action to get back into a good state, rather than > just try to continue gracefully and give no indication. The comment is to explain the "return true;". If we "return false", Chrome stops trying to load (and actually crashes during shutdown), effectively bricking the user's Chrome unless they know how to edit command line flags. I'm open to having ReportExtensionLoadError in both cases, but I'm worried that for most users this will just be an annoying popup on every Chrome launch that they don't know how to (and will never) fix. https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.h (right): https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); On 2016/06/29 14:59:54, Devlin wrote: > nit: I think this would be more flexible if we passed in a > std::set<Manifest::Type> allowed_types (perhaps making it a ptr so that null > implied all types). With only three call sites, this seems a bit overkill. I'll keep the bool unless the other reviewers insist.
On 2016/06/29 14:59:54, Devlin wrote: > Also, can we add a test? > > https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/unpacked_installer.cc (right): > > https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/unpacked_installer.cc:160: if (only_allow_apps && > !extension()->is_app()) { > If we don't opt for the std::set<Type> allowed_types approach, we should > restrict this to is_platform_app(), rather than is_app(). We don't want to > allow loading of legacy packaged apps here. > > Otherwise, this can just be allowed_types.count(extension()->type()). > > https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/unpacked_installer.cc:162: // Avoid crashing for users > with hijacked shortcuts. > I can understand the motivation to avoid crashing, but if this fails, we just > pop up a dialog saying the extension can't be loaded. Since that doesn't brick > the user's chrome, it seems like it would also serve as an indicator that > something was wrong, and allow them to fix it. If a user has one bad > commandline flag, it makes sense they might have more issues, so I'd rather we > let them know so they can take action to get back into a good state, rather than > just try to continue gracefully and give no indication. > > https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/unpacked_installer.h (right): > > https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); > nit: I think this would be more flexible if we passed in a > std::set<Manifest::Type> allowed_types (perhaps making it a ptr so that null > implied all types). Added a test
lgtm. I still think that the allowed_types change would be worthwhile, but I won't block on it. Also, please wrap your CL description to 72 chars to make it git-friendly. https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.h (right): https://codereview.chromium.org/2108853002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); On 2016/06/29 20:56:49, proberge wrote: > On 2016/06/29 14:59:54, Devlin wrote: > > nit: I think this would be more flexible if we passed in a > > std::set<Manifest::Type> allowed_types (perhaps making it a ptr so that null > > implied all types). > > With only three call sites, this seems a bit overkill. I'll keep the bool unless > the other reviewers insist. I imagine we'll want to make a similar change to the load-extension switch. We can wait until that comes about, but it does seem like now would be a better time.
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
proberge@chromium.org changed reviewers: + asargent@chromium.org - mek@chromium.org, pkasting@chromium.org
Trying a different reviewer
On 2016/07/20 18:14:27, proberge wrote: > Trying a different reviewer Looks like Devlin already gave this the ok, were there files you needed me to look at that he isn't in OWNERS for?
On 2016/07/20 19:08:21, Antony Sargent wrote: > On 2016/07/20 18:14:27, proberge wrote: > > Trying a different reviewer > > Looks like Devlin already gave this the ok, were there files you needed me to > look at that he isn't in OWNERS for? It doesn't look like Devlin is an owners in apps/. Could you review these files?
lgtm https://codereview.chromium.org/2108853002/diff/60001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/2108853002/diff/60001/apps/app_load_service.c... apps/app_load_service.cc:87: true); optional: sometimes it can aid readability to either use an enum for paramters like this (eg ONLY_ALLOW_APPS, ALLOW_ALL_TYPES), or at least have an inline comment about the meaning of the boolean, so that the reader doesn't have to go look at the declaration of the method in some other file to understand what the meaning is - e.g. LoadFromCommandLine(base::FilePath(extension_path), &extension_id, true /* only_allow_apps */);
proberge@chromium.org changed reviewers: + msw@chromium.org
+msw for dependency on simple_message_box_internal.h:
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
'+chrome/browser/ui/simple_message_box_internal.h',
https://codereview.chromium.org/2108853002/diff/60001/apps/app_load_service.cc
File apps/app_load_service.cc (right):
https://codereview.chromium.org/2108853002/diff/60001/apps/app_load_service.c...
apps/app_load_service.cc:87: true);
On 2016/07/20 21:33:32, Antony Sargent wrote:
> optional: sometimes it can aid readability to either use an enum for paramters
> like this (eg ONLY_ALLOW_APPS, ALLOW_ALL_TYPES), or at least have an inline
> comment about the meaning of the boolean, so that the reader doesn't have to
go
> look at the declaration of the method in some other file to understand what
the
> meaning is - e.g.
>
> LoadFromCommandLine(base::FilePath(extension_path), &extension_id,
> true /* only_allow_apps */);
Done.
I have some tangential comments and questions; hope that's alright. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... File apps/load_and_launch_browsertest.cc (right): https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:120: class PlatformAppLoadAndLaunchBrowserTest : public PlatformAppBrowserTest { optional nit: LoadAndLaunchPlatformAppBrowserTest https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:126: app_path_ = test_data_dir_ optional nit: not used elsewhere; make this function-local. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:156: app_path_ = test_data_dir_.AppendASCII("good") nit: not used elsewhere; make this function-local. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:158: .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj") nit: cache extension id string (file-local static?) for use in test fixture. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:167: // Skip showing the error message box in order to avoid freezing the main optional nit: -'in order' for a one-liner https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:181: // TODO(jackhou): Make this test not flaky on Vista or Linux Aura. See Would you mind trying to re-enable this test in a followup? We don't support vista anymore and there aren't bots for it; and this likely meant to disable on linux desktop, but it also disables cros; all linux is aura now. Pinged the bug. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:194: LoadAndLaunchApp(); optional nit: check that the app actually loaded? https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:202: // Expect kUnpackedExtensionInsteadOfAppError. nit: check for this value? https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:203: EXPECT_EQ(1u, errors->size()); Does this ever run on official builds? If so, it'll fail, right? https://codereview.chromium.org/2108853002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.h (right): https://codereview.chromium.org/2108853002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); nit: comment above: "used to avoid side-loading non-app extensions" or similar.
Thanks for the great comments @msw! I appreciate that you spent extra time reviewing this CL. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... File apps/load_and_launch_browsertest.cc (right): https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:120: class PlatformAppLoadAndLaunchBrowserTest : public PlatformAppBrowserTest { On 2016/07/21 17:13:49, msw wrote: > optional nit: LoadAndLaunchPlatformAppBrowserTest Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:126: app_path_ = test_data_dir_ On 2016/07/21 17:13:50, msw wrote: > optional nit: not used elsewhere; make this function-local. Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:156: app_path_ = test_data_dir_.AppendASCII("good") On 2016/07/21 17:13:50, msw wrote: > nit: not used elsewhere; make this function-local. Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:158: .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj") On 2016/07/21 17:13:50, msw wrote: > nit: cache extension id string (file-local static?) for use in test fixture. Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:167: // Skip showing the error message box in order to avoid freezing the main On 2016/07/21 17:13:50, msw wrote: > optional nit: -'in order' for a one-liner Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:181: // TODO(jackhou): Make this test not flaky on Vista or Linux Aura. See On 2016/07/21 17:13:50, msw wrote: > Would you mind trying to re-enable this test in a followup? We don't support > vista anymore and there aren't bots for it; and this likely meant to disable on > linux desktop, but it also disables cros; all linux is aura now. Pinged the bug. Sure, I'll try in a follow-up CL. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:194: LoadAndLaunchApp(); On 2016/07/21 17:13:50, msw wrote: > optional nit: check that the app actually loaded? Will do once the test is re-enabled. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:202: // Expect kUnpackedExtensionInsteadOfAppError. On 2016/07/21 17:13:49, msw wrote: > nit: check for this value? Done. https://codereview.chromium.org/2108853002/diff/80001/apps/load_and_launch_br... apps/load_and_launch_browsertest.cc:203: EXPECT_EQ(1u, errors->size()); On 2016/07/21 17:13:50, msw wrote: > Does this ever run on official builds? If so, it'll fail, right? Good catch! Thanks https://codereview.chromium.org/2108853002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/unpacked_installer.h (right): https://codereview.chromium.org/2108853002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/unpacked_installer.h:55: bool only_allow_apps); On 2016/07/21 17:13:50, msw wrote: > nit: comment above: "used to avoid side-loading non-app extensions" or similar. Done.
lgtm with more minor nits. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... File apps/load_and_launch_browsertest.cc (right): https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:23: #include "testing/gmock/include/gmock/gmock.h" nit: remove https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:24: #include "testing/gtest/include/gtest/gtest.h" optional nit: remove; included via extension_browsertest.h, etc. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:37: static const std::string kTestExtensionId_("behllobkkfkfnphdnhnkndlbkcpglgmj"); nit: no trailing underscore. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:200: EXPECT_EQ(0u, errors->size()); optional nit: EXPECT_TRUE(errors->empty()); https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:204: EXPECT_THAT(base::UTF16ToUTF8(errors->at(0)), nit: consider doing this instead: EXPECT_NE(base::string16::npos, errors[0].find(base::ASCIIToUTF16("...")));
https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... File apps/load_and_launch_browsertest.cc (right): https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:23: #include "testing/gmock/include/gmock/gmock.h" On 2016/07/25 23:44:46, msw wrote: > nit: remove Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:24: #include "testing/gtest/include/gtest/gtest.h" On 2016/07/25 23:44:46, msw wrote: > optional nit: remove; included via extension_browsertest.h, etc. Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:37: static const std::string kTestExtensionId_("behllobkkfkfnphdnhnkndlbkcpglgmj"); On 2016/07/25 23:44:46, msw wrote: > nit: no trailing underscore. Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:200: EXPECT_EQ(0u, errors->size()); On 2016/07/25 23:44:46, msw wrote: > optional nit: EXPECT_TRUE(errors->empty()); Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:204: EXPECT_THAT(base::UTF16ToUTF8(errors->at(0)), On 2016/07/25 23:44:46, msw wrote: > nit: consider doing this instead: > EXPECT_NE(base::string16::npos, errors[0].find(base::ASCIIToUTF16("..."))); Done without the ASCIIToUTF16 and with ->at(0) instead of [0]. qq: Why is EXPECT_NE(npos, ...) preferred over testing::HasSubstr?
https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... File apps/load_and_launch_browsertest.cc (right): https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:23: #include "testing/gmock/include/gmock/gmock.h" On 2016/07/25 23:44:46, msw wrote: > nit: remove Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:24: #include "testing/gtest/include/gtest/gtest.h" On 2016/07/25 23:44:46, msw wrote: > optional nit: remove; included via extension_browsertest.h, etc. Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:37: static const std::string kTestExtensionId_("behllobkkfkfnphdnhnkndlbkcpglgmj"); On 2016/07/25 23:44:46, msw wrote: > nit: no trailing underscore. Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:200: EXPECT_EQ(0u, errors->size()); On 2016/07/25 23:44:46, msw wrote: > optional nit: EXPECT_TRUE(errors->empty()); Done. https://codereview.chromium.org/2108853002/diff/100001/apps/load_and_launch_b... apps/load_and_launch_browsertest.cc:204: EXPECT_THAT(base::UTF16ToUTF8(errors->at(0)), On 2016/07/25 23:44:46, msw wrote: > nit: consider doing this instead: > EXPECT_NE(base::string16::npos, errors[0].find(base::ASCIIToUTF16("..."))); Done without the ASCIIToUTF16 and with ->at(0) instead of [0]. qq: Why is EXPECT_NE(npos, ...) preferred over testing::HasSubstr?
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, asargent@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2108853002/#ps120001 (title: "Fix last nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, rdevlin.cronin@chromium.org, asargent@chromium.org Link to the patchset: https://codereview.chromium.org/2108853002/#ps140001 (title: "Use ASCIIToUTF16 to make mac tests happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 ========== to ========== Restrict use of two app-launching command line flags Mitigate abuse of the load-and-launch-app and load-apps flags by disallowing loading of non-app extensions from these flags. For the official build, the flags no-op instead of showing an error popup to avoid giving users with hijacked shortcuts a bad time (the error message is shown then Chrome shuts down) BUG=624098 Committed: https://crrev.com/93c60cd3ef5908984e173112dc09345bb5714036 Cr-Commit-Position: refs/heads/master@{#407824} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/93c60cd3ef5908984e173112dc09345bb5714036 Cr-Commit-Position: refs/heads/master@{#407824}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2180853004/ by proberge@chromium.org. The reason for reverting is: Breaks official builds. chrome/browser/extensions/unpacked_installer.cc:43:12: error: unused variable 'kUnpackedExtensionInsteadOfAppError' .
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2181393002/ by robliao@chromium.org. The reason for reverting is: Compile Error chrome/browser/extensions/unpacked_installer.cc:43:12: error: unused variable 'kUnpackedExtensionInsteadOfAppError' [-Werror,-Wunused-const-variable] On Official Builds. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
