security: allow to block desktopCapturer.getSources() calls

[miniak]

Description of Change

Allows blocking of desktopCapturer.getSources() calls by handling the desktop-capturer-get-sources event.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR title follows semantic commit guidelines

Release Notes

Notes: Allowed blocking of desktopCapturer.getSources() calls by handling the desktop-capturer-get-sources event.

[zcbenz]

The failing Windows tests do not seem to be caused by the changes in this PR, but I'm not sure if they are revealed because of the new tests added by this PR. If it is I would like to fix the failing tests before merging this.

[MarshallOfSound]

Instead of making this throw an error, can we make it transparent to the caller and return an empty array.

[miniak]

That's what I did initially before pushing the PR and then I though an error is more explicit.
What do others think?
cc @nornagon, @deepak1556, @alexeykuzmin

[miniak]

It's similar to remote.require() blocking, the purpose is to prevent malicious use. In which case I think it's better if it blows up on the caller side. Also makes it easier to debug, when you are trying to get rid of these calls to allow the blocking to be enabled.

[nornagon]

I prefer the empty array approach; it's less likely to break things.

sam is on holiday now!

[miniak]

@zcbenz, tests are not failing, but electron crashes on this DCHECK

MessageWindow::WindowClass::~WindowClass() {
  if (atom_ != 0) {
    BOOL result = UnregisterClass(MAKEINTATOM(atom_), instance_);
    // Hitting this DCHECK usually means that some MessageWindow objects were
    // leaked. For example not calling
    // ui::Clipboard::DestroyClipboardForCurrentThread() results in a leaked
    // MessageWindow.
    DCHECK(result);
  }
}

[miniak]

# tests 1115
# pass 1115
# fail 0
[5128:1212/170752.867:FATAL:message_window.cc(67)] Check failed: result. 
Backtrace:
	logging::LogMessage::~LogMessage [0x00007FF7F8757D1F+111]
	base::LazyInstance<base::win::MessageWindow::WindowClass,base::internal::DestructorAtExitLazyInstanceTraits<base::win::MessageWindow::WindowClass> >::OnExit [0x00007FF7F888D390+112]
	base::RepeatingCallback<void __cdecl(void)>::Run [0x00007FF7F87590E7+423]
	base::AtExitManager::ProcessCallbacksNow [0x00007FF7F875885B+235]
	base::AtExitManager::~AtExitManager [0x00007FF7F87583DF+143]
	std::unique_ptr<base::AtExitManager,std::default_delete<base::AtExitManager> >::reset [0x00007FF7F872CE68+24]
	content::ContentMainRunnerImpl::Shutdown [0x00007FF7F872D351+273]
	service_manager::Main [0x00007FF7F8EC48DC+2140]
	content::ContentMain [0x00007FF7F872C7D1+65]
	wWinMain [0x00007FF7F63613B8+664]
	__scrt_common_main_seh [0x00007FF7FBD919C2+262] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
	BaseThreadInitThunk [0x00007FFDA80213D2+34]
	RtlUserThreadStart [0x00007FFDAA9A54F4+52]
An error occurred inside the spec runner: Error: Electron tests failed with code 2147483651.

[miniak]

@deepak1556, @felixrieseberg, @nornagon any ideas what could be wrong?

[zcbenz]

The assertion means there is a message window still open when quitting, the code probably need to do some cleanup job for desktopCapturer.

[miniak]

@zcbenz when desktopCapturer.getSources() is blocked, the IPC message is immediately replied to with an empty list and no further processing is done.

[zcbenz]

The crash does not seem to be this PR's fault.

After looking into the crash, I found that running any desktopCapturer test would result in the same crash. And the desktopCapturer tests have actually been disabled in CI on Windows so CI does not break for this.

I'll fix the root cause of the crash.

[zcbenz]

The lint error is not related to this PR, it comes from master branch.

[release-clerk]

Release Notes Persisted

Allowed blocking of desktopCapturer.getSources() calls by handling the desktop-capturer-get-sources event.