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

feat: make desktopCapturer main-process-only #30720

Merged
merged 7 commits into from
Oct 4, 2021
Merged

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Aug 26, 2021

Description of Change

This makes desktopCapturer.getSources a main-process-only method.

This is a security precaution; it prohibits renderers from being able to
request images from the desktop without explicit permission from the main
process.

Checklist

Release Notes

Notes: desktopCapturer.getSources is now only available in the main process.

@nornagon nornagon requested a review from a team as a code owner August 26, 2021 22:33
@nornagon nornagon added no-backport semver/major incompatible API changes labels Aug 26, 2021
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Aug 26, 2021
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking changes document needs to be updated.
And this needs to be deprecated first rught?

@nornagon
Copy link
Member Author

@MarshallOfSound yep, i thought i'd open this first instead of an issue, to gauge a response before going through the longer process of deprecation/etc.

are you on board with removing this, ultimately?

@MarshallOfSound
Copy link
Member

are you on board with removing this, ultimately?

Yes 👍 It's just a convenience IPC wrapper at the moment that is trivial to implement but is currently non trivial to intercept and protect against abuse. removing makes sense.

@miniak
Copy link
Contributor

miniak commented Aug 27, 2021

we should merge lib/browser/api/desktop-capturer.ts with lib/browser/desktop-capturer.ts

@miniak
Copy link
Contributor

miniak commented Aug 27, 2021

@MarshallOfSound

It's just a convenience IPC wrapper at the moment that is trivial to implement

that's not entirely true, it depends on

function serializeNativeImage (image: Electron.NativeImage) {
const representations = [];
const scaleFactors = image.getScaleFactors();
// Use Buffer when there's only one representation for better perf.
// This avoids compressing to/from PNG where it's not necessary to
// ensure uniqueness of dataURLs (since there's only one).
if (scaleFactors.length === 1) {
const scaleFactor = scaleFactors[0];
const size = image.getSize(scaleFactor);
const buffer = image.toBitmap({ scaleFactor });
representations.push({ scaleFactor, size, buffer });
} else {
// Construct from dataURLs to ensure that they are not lost in creation.
for (const scaleFactor of scaleFactors) {
const size = image.getSize(scaleFactor);
const dataURL = image.toDataURL({ scaleFactor });
representations.push({ scaleFactor, size, dataURL });
}
}
return { __ELECTRON_SERIALIZED_NativeImage__: true, representations };
}
function deserializeNativeImage (value: any, createNativeImage: typeof Electron.nativeImage['createEmpty']) {
const image = createNativeImage();
// Use Buffer when there's only one representation for better perf.
// This avoids compressing to/from PNG where it's not necessary to
// ensure uniqueness of dataURLs (since there's only one).
if (value.representations.length === 1) {
const { buffer, size, scaleFactor } = value.representations[0];
const { width, height } = size;
image.addRepresentation({ buffer, scaleFactor, width, height });
} else {
// Construct from dataURLs to ensure that they are not lost in creation.
for (const rep of value.representations) {
const { dataURL, size, scaleFactor } = rep;
const { width, height } = size;
image.addRepresentation({ dataURL, scaleFactor, width, height });
}
}
return image;
}
export function serialize (value: any): any {
if (value && value.constructor && value.constructor.name === 'NativeImage') {
return serializeNativeImage(value);
} if (Array.isArray(value)) {
return value.map(serialize);
} else if (isSerializableObject(value)) {
return value;
} else if (value instanceof Object) {
return objectMap(value, serialize);
} else {
return value;
}
}
export function deserialize (value: any, createNativeImage: typeof Electron.nativeImage['createEmpty'] = getCreateNativeImage()): any {
if (value && value.__ELECTRON_SERIALIZED_NativeImage__) {
return deserializeNativeImage(value, createNativeImage);
} else if (Array.isArray(value)) {
return value.map(value => deserialize(value, createNativeImage));
} else if (isSerializableObject(value)) {
return value;
} else if (value instanceof Object) {
return objectMap(value, value => deserialize(value, createNativeImage));
} else {
return value;
}
}

but is currently non trivial to intercept

https://github.com/electron/electron/blob/main/docs/api/web-contents.md#event-desktop-capturer-get-sources
https://github.com/electron/electron/blob/main/docs/api/app.md#event-desktop-capturer-get-sources

@nornagon
Copy link
Member Author

@miniak see #30729

@nornagon nornagon marked this pull request as draft September 2, 2021 18:46
@nornagon nornagon removed the wip ⚒ label Sep 2, 2021
@nornagon
Copy link
Member Author

nornagon commented Sep 2, 2021

draft until 16.x forks

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 2, 2021
@miniak
Copy link
Contributor

miniak commented Sep 3, 2021

@nornagon also remove --enable-api-filtering-logging from docs/api/command-line-switches.md
and kEnableApiFilteringLogging from shell/common/options_switches.h, etc.

@zcbenz zcbenz added the wip ⚒ label Sep 21, 2021
@miniak
Copy link
Contributor

miniak commented Sep 22, 2021

@nornagon 16-x-y was branched off, we can land this now in main

@zcbenz
Copy link
Member

zcbenz commented Sep 23, 2021

nornagon is off until next year, so we should either pick this up, or wait for some time.

@miniak miniak removed the wip ⚒ label Sep 24, 2021
@miniak miniak marked this pull request as ready for review September 24, 2021 15:57
@miniak
Copy link
Contributor

miniak commented Sep 24, 2021

@zcbenz taking it over. rebased and conflicts resolved

@miniak miniak self-assigned this Sep 24, 2021
@zcbenz zcbenz merged commit 4fd7c2a into main Oct 4, 2021
@zcbenz zcbenz deleted the desktop-capturer-main-only branch October 4, 2021 03:16
@release-clerk
Copy link

release-clerk bot commented Oct 4, 2021

Release Notes Persisted

desktopCapturer.getSources is now only available in the main process.

t57ser pushed a commit to t57ser/electron that referenced this pull request Oct 27, 2021
* feat: make desktopCapturer main-process-only

* remove --enable-api-filtering-logging

* remove test

* merge lib/browser/api/desktop-capturer.ts with lib/browser/desktop-capturer.ts

* remove desktop-capturer-get-sources event

* fix specs

* getSources needs to be async

Co-authored-by: Milan Burda <milan.burda@gmail.com>
t57ser pushed a commit to t57ser/electron that referenced this pull request Oct 27, 2021
* feat: make desktopCapturer main-process-only

* remove --enable-api-filtering-logging

* remove test

* merge lib/browser/api/desktop-capturer.ts with lib/browser/desktop-capturer.ts

* remove desktop-capturer-get-sources event

* fix specs

* getSources needs to be async

Co-authored-by: Milan Burda <milan.burda@gmail.com>
t57ser pushed a commit to t57ser/electron that referenced this pull request Oct 29, 2021
* feat: make desktopCapturer main-process-only

* remove --enable-api-filtering-logging

* remove test

* merge lib/browser/api/desktop-capturer.ts with lib/browser/desktop-capturer.ts

* remove desktop-capturer-get-sources event

* fix specs

* getSources needs to be async

Co-authored-by: Milan Burda <milan.burda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants