Re-add support for Chromecast

[bsclifton]

Fixes brave/brave-browser#209
Related brave/brave-browser#4631

---

Revert "Merge pull request #2119 from brave/revert_enabling_chromecast"
This reverts commit 29ea1df, reversing
changes made to c08710a.

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Windows
  • macOS
  • Linux
• Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
  • Windows
  • macOS
  • Linux
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection

Test Plan:

First Launch

1. Launch Brave with a new profile
2. Verify that Cast option is not available in the hamburger menu
3. Open wireshark, filter traffic for udp.port == 1900 and verify that no traffic is generated by Brave.

Settings Page

1. Open brave://settings and verify that Extensions > Media Router is disabled
2. Toggle the switch, relaunch.
3. Navigate to the hamburger menu and check Cast is available
4. Click Cast, and verify if you're able to cast your screen to a Chrome-cast device (Note: that signature verification takes some time on first try, so wait for ~10-20 secs before trying this)
5. Toggle the switch back off, relaunch and verify Cast is not available in the hamburger menu.

Upgrade

1. Open Brave-Browser without this change
2. Navigate to brave://flags and enable media router
3. Relaunch and verify that ChromeCast doesn't work
4. Open Brave-Browser (use the same profile) with the change
5. Navigate to brave://settings and confirm Media Router preference is enabled
6. Verify Cast works as expected.

Settings UI

1. Navigate to brave://settings
2. Toggle the Media Router Setting
3. Verify that a fixed bottom bar requesting a relaunch.
4. Toggle the Media Router Setting and verify that the bar disappears
5. Toggle it back on, and navigate to a different page. The bar should disappear
6. Navigate to brave://settings and the bar should re-appear.

Sync with brave://flags

1. Navigate to brave flags, set Load Media Router Component Extension to enabled
2. Verify that Media Router setting in, brave://settings is also enabled
3. Navigate to brave flags, set Load Media Router Component Extension to disabled
4. Verify that Media Router setting in, brave://settings is also disabled

---

1. Navigate to brave flags, set Media Router Setting to enabled
2. Verify that the Media Router Flag, in brave://flags is enabled
3. Navigate to brave flags, set Media Router Setting to disabled
4. Verify that the Media Router Flag, in brave://flags is disabled

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[bsclifton]

Errors from prior merge attempt:

12:53:51  NETWORK AUDIT FAIL: https://clients2.google.com/service/update2/crx?os=linux&arch=x64&os_arch=x86_64&nacl_arch=x86-64&prod=chromiumcrx&prodchannel=unknown&prodversion=73.0.64.26&lang=en-US&acceptformat=crx2,crx3&x=id%3Dpkedcjkdefgpdelpbcmbmeomcjbeemfm%26v%3D0.0.0.0%26installedby%3Dother%26uc%26ping%3Dr%253D-1%2526e%253D1
12:53:51  NETWORK AUDIT FAIL: https://clients2.google.com/service/update2/crx?os=linux&arch=x64&os_arch=x86_64&nacl_arch=x86-64&prod=chromiumcrx&prodchannel=unknown&prodversion=73.0.64.26&lang=en-US&acceptformat=crx2,crx3&x=id%3Dpkedcjkdefgpdelpbcmbmeomcjbeemfm%26v%3D0.0.0.0%26installedby%3Dother%26uc%26ping%3Dr%253D-1%2526e%253D1
12:53:51  NETWORK AUDIT FAIL: https://extensionupdater.brave.com/service/update2/crx?os=linux&arch=x64&os_arch=x86_64&nacl_arch=x86-64&prod=chromiumcrx&prodchannel=unknown&prodversion=73.0.64.26&lang=en-US&acceptformat=crx2,crx3&x=id%3Dpkedcjkdefgpdelpbcmbmeomcjbeemfm%26v%3D0.0.0.0%26installedby%3Dother%26uc%26ping%3Dr%253D-1%2526e%253D1
12:53:51  NETWORK AUDIT FAIL: http://redirector.gvt1.com/edgedl/chromewebstore/L2Nocm9tZV9leHRlbnNpb24vYmxvYnMvMjJlQUFXRC12Ny1ldUFnMXF3SDlXZDlFZw/7319.128.0.1_pkedcjkdefgpdelpbcmbmeomcjbeemfm.crx
12:53:51  NETWORK AUDIT FAIL: http://redirector.gvt1.com/edgedl/chromewebstore/L2Nocm9tZV9leHRlbnNpb24vYmxvYnMvMjJlQUFXRC12Ny1ldUFnMXF3SDlXZDlFZw/7319.128.0.1_pkedcjkdefgpdelpbcmbmeomcjbeemfm.crx
12:53:51  NETWORK AUDIT FAIL: http://r6---sn-nx5e6ne6.gvt1.com/edgedl/chromewebstore/L2Nocm9tZV9leHRlbnNpb24vYmxvYnMvMjJlQUFXRC12Ny1ldUFnMXF3SDlXZDlFZw/7319.128.0.1_pkedcjkdefgpdelpbcmbmeomcjbeemfm.crx?cms_redirect=yes&mip=34.219.185.75&mm=28&mn=sn-nx5e6ne6&ms=nvh&mt=1553863827&mv=m&pl=12&shardbypass=yes

[simonhong]

As @jumde mentioned, case is still not working due to Disabling extension pkedcjkdefgpdelpbcmbmeomcjbeemfm ('Chrome Media Router') due to install verification failure.

and @jumde tested it is enabled with below patch.

index 32b499da4..9672b8987 100644
--- a/browser/extensions/brave_extension_management.cc
+++ b/browser/extensions/brave_extension_management.cc
@@ -16,6 +16,7 @@
 #include "brave/browser/extensions/brave_tor_client_updater.h"
 #include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/extensions/install_verifier.h"
 #include "components/prefs/pref_service.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
@@ -41,6 +42,9 @@ void BraveExtensionManagement::RegisterBraveExtensions() {
       *base::CommandLine::ForCurrentProcess();
   if (!command_line.HasSwitch(switches::kDisableTorClientUpdaterExtension))
     g_brave_browser_process->tor_client_updater()->Register();
+  std::set<std::string> ids;
+  ids.insert("pkedcjkdefgpdelpbcmbmeomcjbeemfm");
+  InstallVerifier::Get(profile_)->AddProvisional(ids);
 }

With above patch, Disabling extension pkedcjkdefgpdelpbcmbmeomcjbeemfm ('Chrome Media Router') due to install verification failure. is not visible but still can't get available device list. Works with clean profile.

[jumde]

As @jumde mentioned, case is still not working due to Disabling extension pkedcjkdefgpdelpbcmbmeomcjbeemfm ('Chrome Media Router') due to install verification failure.

and @jumde tested it is enabled with below patch.

index 32b499da4..9672b8987 100644
--- a/browser/extensions/brave_extension_management.cc
+++ b/browser/extensions/brave_extension_management.cc
@@ -16,6 +16,7 @@
 #include "brave/browser/extensions/brave_tor_client_updater.h"
 #include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/extensions/install_verifier.h"
 #include "components/prefs/pref_service.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
@@ -41,6 +42,9 @@ void BraveExtensionManagement::RegisterBraveExtensions() {
       *base::CommandLine::ForCurrentProcess();
   if (!command_line.HasSwitch(switches::kDisableTorClientUpdaterExtension))
     g_brave_browser_process->tor_client_updater()->Register();
+  std::set<std::string> ids;
+  ids.insert("pkedcjkdefgpdelpbcmbmeomcjbeemfm");
+  InstallVerifier::Get(profile_)->AddProvisional(ids);
 }

With above patch, Disabling extension pkedcjkdefgpdelpbcmbmeomcjbeemfm ('Chrome Media Router') due to install verification failure. is not visible but still can't get available device list.

@simonhong - With this patch I checked that the signature check fails in https://cs.chromium.org/chromium/src/chrome/browser/extensions/install_verifier.cc?l=631. I'm not sure if we're okay with that.

[simonhong]

@jumde I edited right before your last comment :)
Device list is fetched properly with clean profile.

[diracdeltas]

out of curiosity, why https instead of * here?

[jumde]

it was not connecting over http but I agree, using * would help if the code fallsback to the non-secure url

[simonhong]

@bridiver added cause this PR adds new patch file.

[petemill]

Thanks for making the restart notification persistent. Minor changes requested to the JS api

[petemill]

👍 on the pattern but please can we call the command getRestartNeeded so that:

• it's about the data, not the UI implementation (toast)
• it's specific about getting data, and not performing an action

[petemill]

This may be a micro-optimization, so consider it a nit: I'd feel more comfortable if this didn't require a DOM change on 99.9% of all Settings page loads (i.e. page loads which don't have the toast showing).
This can be achieved by either:

• Preferred: Wrap the element in an <if which tests hideRestartToast (though positive variable name is better, e.g. showRestartToast
• Similar pattern to what you have: Create a css class called something like pageNeedsRestart, and only add this class if the variable comes back true.

[simonhong]

nit: restart_needed_ is the chromium style.

[simonhong]

It would be good to add some comments why this is needed.

[jumde]

done!

[jumde]

This happens because for Media Router the signature fetch and check happens out of order. This is likely a chromium bug, but not seen because chromium uses ENFORCE instead of ENFORCE_STRICT by default.(https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_verifier_delegate.cc?l=110)

But, ENFORCE_STRICT (https://cs.chromium.org/chromium/src/extensions/browser/content_verifier_delegate.h?g=0&l=42) is better for security.

We have multiple options here:

1. Use ENFORCE over ENFORCE_STRICT similar to chromium.
2. SkipSignatureVerification for MediaRouter as seen in this PR.
3. RedoSignature check after the fetch is complete.

Thoughts? @bridiver @diracdeltas @tomlowenthal

cc: @bsclifton @simonhong

[bsclifton]

Option 2 or 3 sound great to me- I'd vote for option 2 as my choice 😄

[diracdeltas]

i prefer 3 since Google Chrome uses ENFORCE_STRICT (https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_verifier_delegate.cc?l=68); so IIUC, 3 is the only option that gives us parity with Google Chrome security-wise.

[jumde]

@diracdeltas - ENFORCE_STRICT is only used when field trials are enabled, I don't think Chrome has field trials enabled for release builds. Let me know if I'm missing something.

[bridiver]

@diracdeltas I thought it was the other way around? Chromium uses ENFORCE so my preference is to follow that and not patch here

[jumde]

@diracdeltas @bridiver - ENFORCE_STRICT works well for the extensions I have tested, but I understand the concern that we might run into web-compat issues with an experimental feature.

I have a patch for 3 but, I'm okay with 1 to avoid any web-compat risk we might run into.

[bridiver]

I'm confused, I thought we were doing a lazy check to sync with the pref?

[bridiver]

if we need notification at the time this changes then we should add a pref observer for kEnabledLabsExperiments

[jumde]

I think I could have named it better, the intention of this call is to update the temporary pref kBraveEnabledMediaRouter that we use in brave://settings when flags are toggled. So toggling the flag correctly toggles the Media Router switch in brave://settings.

[bridiver]

this can be done without a patch using a chromium_src override for SetFeatureEntryEnabled which I thought is what we had before and maybe this is why I'm confused here

[bridiver]

lint 4 spaces

[bridiver]

I believe the correct way to handle this is to just link to kChromeUIRestartURL

[jumde]

Navigation to kChromeUIRestartURL internally calls chrome::AttemptRestart: https://cs.chromium.org/chromium/src/chrome/browser/browser_about_handler.cc?l=98.

We can link to kChromeUIRestartURL in javascript directly but I was just trying to avoid chances of regression if kChromeUIRestartURL changes in future.

[bridiver]

I think it is better to use kChromeUIRestartURL than to duplicate the logic here. Not sure why kChromeUIRestartURL would change in the future

[petemill]

@bridiver just noting that the same functionality in chrome://flags does not link to kChromeUIRestartUI but sends a message to a message handler via chrome.send too and calls AttemptRestart. I wonder what would happen to session restore if the tab was navigated away from the settings page to kChromeUIRestartURL. The settings page may not restore, which would be unfortunate. Although I did spot comments around this being a non-navigation url, so maybe it would restore... Still, I wonder why the Flags page doesn't use it.

[bridiver]

@petemill because a lot of other things happen in FlagsDOMHandler::HandleRestartBrowser before the restart. The only thing we're doing here is calling AttemptRestart so it's functionally equivalent

[bridiver]

  DCHECK(flags_storage_);
#if defined(OS_CHROMEOS)
  // On ChromeOS be less intrusive and restart inside the user session after
  // we apply the newly selected flags.
  base::CommandLine user_flags(base::CommandLine::NO_PROGRAM);
  about_flags::ConvertFlagsToSwitches(flags_storage_.get(),
                                      &user_flags,
                                      flags_ui::kAddSentinels);

  // Apply additional switches from policy that should not be dropped when
  // applying flags..
  chromeos::UserSessionManager::MaybeAppendPolicySwitches(
      Profile::FromWebUI(web_ui())->GetPrefs(), &user_flags);

  base::CommandLine::StringVector flags;
  // argv[0] is the program name |base::CommandLine::NO_PROGRAM|.
  flags.assign(user_flags.argv().begin() + 1, user_flags.argv().end());
  VLOG(1) << "Restarting to apply per-session flags...";
  AccountId account_id =
      user_manager::UserManager::Get()->GetActiveUser()->GetAccountId();
  chromeos::UserSessionManager::GetInstance()->SetSwitchesForUser(
      account_id,
      chromeos::UserSessionManager::CommandLineSwitchesType::
          kPolicyAndFlagsAndKioskControl,
      flags);
#endif
  chrome::AttemptRestart();
}

[bridiver]

they need all of that code to run on chromeos before attempting the restart

[bridiver]

why are you cloning this? Also args->GetList()[0] is not the correct or safe way to get this value.

std::string callback_id;
args->GetString(0, &callback_id);

and then pass in base::Value(callback_id)

[bridiver]

we shouldn't be directly calling ExtensionPrefs::SetExtensionEnabled. You should enable/disable using the ExtensionRegistry

[bridiver]

how do we know this still works if the extension version changes?

[jumde]

changed to random_hash and random_version, the pattern https://github.com/brave/brave-core/pull/2493/files#diff-6337bc2627a3b8782b369e66dc193cb9R47 should catch all changes to hash and version

[jumde]

Needed because SetFeatureEntryEnabled in about_flags.cc calls, SetFeatureEnabled in flags_state.cc

[petemill]

Found a minor issue with the restart prompt. Since I've already provided feedback about that which may have led to this issue, I've implemented a proposed change in #2974. Apart from that it all looks good.

[petemill]

Cast off! ⚓️