Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

security: allow to block desktopCapturer.getSources() calls #15964

Merged
merged 3 commits into from Dec 20, 2018

Conversation

@miniak
Copy link
Contributor

commented Dec 5, 2018

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.

@miniak miniak requested review from as code owners Dec 5, 2018

@miniak miniak force-pushed the miniak/desktop-capturer-filtering branch from 1cd375f to fe1397e Dec 5, 2018

@miniak miniak self-assigned this Dec 5, 2018

@zcbenz

zcbenz approved these changes Dec 7, 2018

@zcbenz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

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.

* `webContents` [WebContents](web-contents.md)

Emitted when `desktopCapturer.getSources()` is called in the renderer process of `webContents`.
Calling `event.preventDefault()` will make it throw an error.

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Dec 7, 2018

Member

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

This comment has been minimized.

Copy link
@miniak

miniak Dec 8, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@miniak

miniak Dec 8, 2018

Author Contributor

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.

@miniak miniak force-pushed the miniak/desktop-capturer-filtering branch 2 times, most recently from d07a7e2 to ce9740f Dec 8, 2018

@nornagon
Copy link
Contributor

left a comment

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

sam is on holiday now!

@miniak miniak force-pushed the miniak/desktop-capturer-filtering branch from ce9740f to d947d98 Dec 10, 2018

@miniak miniak force-pushed the miniak/desktop-capturer-filtering branch from d947d98 to b25e4cc Dec 11, 2018

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

# 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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

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

@zcbenz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

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

@miniak miniak force-pushed the miniak/desktop-capturer-filtering branch from b25e4cc to be02125 Dec 18, 2018

@miniak

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

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

@zcbenz

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

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

@zcbenz zcbenz merged commit 547097b into master Dec 20, 2018

25 of 27 checks passed

electron-lint Build #20181220.1 has failed
Details
Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be merged or rebased
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181220.2 succeeded
Details
electron-arm64-testing Build #20181220.2 succeeded
Details
electron-mas-testing Build #20181220.1 succeeded
Details
electron-osx-testing Build #20181220.2 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Dec 20, 2018

Release Notes Persisted

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

@zcbenz zcbenz deleted the miniak/desktop-capturer-filtering branch Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.