-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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: service worker preload scripts #44411
base: main
Are you sure you want to change the base?
Conversation
d13f9c5
to
044dccc
Compare
// Use of this source code is governed by the MIT license that can be | ||
// found in the LICENSE file. | ||
|
||
#include "shell/renderer/preload_utils.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's talk more about evaluateInMainWorld
. Making new functions that allow evaluation of strings directly contravenes web security standards and the original purpose of the context bridge. It will expose a giant security footgun and I'm a strong -1 on exposing such a feature.
I'd love to understand the motivation / usecase and discuss with the Security WG (cc @electron/wg-security) how to solve those use-cases with an API that isn't such a footgun.
@MarshallOfSound the goal with For my particular use case, I'd like to overwrite extension APIs such as const { ipcRenderer, contextBridge } = require('electron');
// Expose setBadgeText API
contextBridge.exposeInMainWorld('electron', {
setBadgeText: (text) => ipcRenderer.send('action.setBadgeText', text)
});
// Overwrite extension API to provide custom functionality
contextBridge.evaluateInMainWorld(`(function () {
chrome.action.setBadgeText = (text) => {
electron.setBadgeText(text);
};
}());`); A potential alternative might be to accept functions. This is similar to what's offered by chrome.scripting APIs. function overrideActionApi () {
chrome.action.setBadgeText = (text) => {
electron.setBadgeText(text);
};
}
contextBridge.evaluateInMainWorld({
func: overrideActionApi,
args: []
}); If this method existing on |
webFrame.executeJavaScript is also a foot gun, it's an API that wouldn't land nowadays and if we could, we'd remove it. I wouldn't use it as an example It sounds like what you want is support for overriding existing APIs from contextBridge which is a thing it supports internally but isn't exposed via API |
@MarshallOfSound The example I provided is limited. However, I do have use cases which require additional logic where providing a complete function would be necessary. For example, some extension APIs require serializing arguments such as action.setIcon. Additionally, the v8::Context provided in this implementation is based on ShadowRealms. These lack most DOM APIs, but could be partially restored by evaluating a method. // Polyfill setTimeout in ShadowRealmGlobalScope
function setTimeoutAsync (delay) {
return contextBridge.evaluateInMainWorld({
func: function mainWorldFunc (delay) {
return new Promise((resolve) => setTimeout(resolve, delay));
},
args: [delay]
});
} |
In this case adding support for zero-copy context bridge transfer of
I don't think this is a good enough usecase to justify a security footgun, the web knows it, chrome knows it, passing strings around to be evalled is just a nightmare. Someones gonna do something silly like |
@MarshallOfSound If I refactor this API to accept a |
Ideally we find a way to avoid this APi surface entirely, does the |
My current constraints:
There's also the issue of future unknowns. JS execution will allow the flexibility to solve varied problems with a small API surface. I'm not sure I fully understand the footgun argument against JS execution (outside of eval strings). Electron provides application developers with full control over a chromium browser environment through JS APIs, and this seems to take away from that level of control. This is a fairly common API provided in projects such as Chrome DevTools Protocol, Chrome Extensions, Puppeteer/Playwright, Selenium, and node's vm module. |
Let me clarify my stance
To give a path forward given the constraints noted above (thanks for those, gives a clear picture of what is needed)
Docs for the function thing could be fun, but at least technically that's the way forward IMO |
4791bda
to
c6164aa
Compare
I've refactored Tests to guarantee return values go through the context bridge reuse the logic from our webFrame.executeJavaScript world safe test. The internals of chrome.scripting.executeScript internals
contextBridge.evaluateInMainWorld(script) typesThe types are currently using interface EvaluationScript {
/**
* A JavaScript function to evaluate. This function will be serialized which means
* that any bound parameters and execution context will be lost.
*/
func: (...args: any[]) => any;
/**
* The arguments to pass to the provided function. These arguments must be
* JSON-serializable.
*/
args?: any[];
}
interface ContextBridge {
evaluateInMainWorld(evaluationScript: EvaluationScript): any;
} |
c6164aa
to
5710a61
Compare
5710a61
to
b29afcf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still waiting on agreement about how preload IDs should be handled, or is the current API what we've settled on?
FWIW I'm fine with the APIs operating based on IDs, but I do think it would be nice if registerPreloadScript
could take an optional ID and generate a UUID if none is provided.
|
||
* `preloads` string[] - An array of absolute path to preload scripts | ||
|
||
Adds scripts that will be executed on ALL web contents that are associated with | ||
this session just before normal `preload` scripts run. | ||
|
||
#### `ses.getPreloads()` | ||
#### `ses.getPreloads()` _Deprecated_ | ||
|
||
Returns `string[]` an array of paths to preload scripts that have been | ||
registered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should clarify that this will only return webContents preload scripts, and not service worker preloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added deprecation notes in 4bf498d, good call out!
There was discussion on potentially changing this during the API WG meeting. However, that never made it into feedback here. My thoughts are that using IDs will reduce complexity of the API by limiting the information needed to later update preloads. Only an ID needs to be stored rather than the entire registration object. This is a familiar pattern to If folks have objections to using IDs, I'd encourage sharing thoughts in this PR. Ideally this would have been discussed during the RFC stage 😅 I've refactored |
Arguments passed into const { contextBridge } = require('electron');
const start = Date.now();
const onCallback = () => {
const elapsed = Date.now() - start;
console.log(`invoked callback after ${elapsed}ms`);
};
contextBridge.executeInMainWorld({
func: (callback) => {
setTimeout(callback, 1000);
},
args: [onCallback]
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API LGTM
I'm very excited about contextBridge.executeInMainWorld()
🎉
} | ||
args_array = args_value.As<v8::Array>(); | ||
} else { | ||
args_array = v8::Array::New(isolate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a new array here only to do nothing with it is not required, we should optimize for the empty case and use MaybeLocal
here, that's what most of the ctx bridge does (allows for "empty" values)
<!-- TODO(samuelmaddock): add generics to map the `args` types to the `func` params --> | ||
|
||
* `executionScript` Object | ||
* `func` (...args: any[]) => any - A JavaScript function to execute. This function will be serialized which means |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the func
naming here consistent with something else? We can't use function
because reserved but I think this could be a case where matching spawn(thing, args)
might be nice. E.g. exec(fn, args, { ...opts })
Returns `string` - The ID of the registered preload script. | ||
#### `ses.unregisterPreloadScript(id)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did we land on the alternative API here:
registerPreloadScript(path, opts);
unregisterPreloadSCript(path[, opts]);
I don't love this ID system (as discussed in API WG), this id serves no purpose other than a "pointer" which we don't need if we treat the path as a key to multiple scripts. Record<Path, Opts[]>
} | ||
// Clone certain DOM APIs only within Window contexts. | ||
blink::ExecutionContext* execution_context = | ||
blink::ExecutionContext::From(destination_context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to be checking the source_context
here?
v8::MaybeLocal<v8::Context> maybe_target_context; | ||
|
||
blink::ExecutionContext* execution_context = | ||
blink::ExecutionContext::From(source_context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know how expensive this is, it might be worth passing this down through arg binds instead of recomputing in the function wrapper each time. i.e. calc once and then all transitive "bridged" things retain that state. A context can't transition from "window" to "worker" so we should be good to cache like that
if (!script_result->IsFunction()) { | ||
isolate->ThrowException(v8::Exception::Error( | ||
gin::StringToV8(isolate, "Unknown error retrieving function"))); | ||
return v8::Local<v8::Value>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why MaybeLocal exists, we shouldn't be returning empty locals.
} | ||
} | ||
v8::Local<v8::Value> script_result; | ||
if (!maybe_script_result.ToLocal(&script_result)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, need to be in destination context, let's keep this clean and obvious
std::vector<v8::Local<v8::Value>> proxied_args; | ||
{ | ||
bool support_dynamic_properties = false; | ||
context_bridge::ObjectCache object_cache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird calling PassValueToOtherContext
multiple times with the same object cache, it works but it's not intended. Is there a DX reason we didn't just call PassValueToOtherContext
on the entire array?
} | ||
|
||
// Clone the result into the callee/source context | ||
v8::Context::Scope target_scope(target_context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're cloning into the source context why is the scope for the target context?
@@ -1290,6 +1290,115 @@ describe('contextBridge', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('executeInMainWorld', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update generateTests
so the same "it doesn't leak X" tests run in a service worker environment too. They current run for sandbox + unsandbox. They should run for sandbox + unsandbox + service workers.
Description of Change
Implements RFC #8
Tip
I recommend reviewing by commit. I've split up the work into logical commits to make reading it more manageable.
Todo
contextBridge.evaluateInMainWorld
to acceptFunction
andArgs[]
rather than astring
feat: service worker preload scripts #44411 (comment)
feat: e pr download-dist <number> build-tools#679
evaluateInMainWorld
should use contextBridge value passing rather than JSON-serialization if possible.registerPreloadScript
to allowid
as an optional property.fromVersionID
could be better named to something likegetWorkerFromVersionID
Documentation
ServiceWorkerVersion
andServiceWorkerHost
are relevant for understandingServiceWorkerMain
in this PR.Overview
v8::Context
in renderer worker threads to allow secure interaction with service worker contexts.setPreloads
andgetPreloads
onSession
to support additional targets and more accessible registration from third-party librariesServiceWorkerMain
to main process to enable IPC with renderer worker threads.ServiceWorkerMain
Tracks lifetime of
content::ServiceWorkerVersion
. This class lives as long as a service worker registration is live. A new instance is required for each new 'version' of a service worker script installed.Only
scope
,versionId
, andipc
are currently exposed.Service worker IPCs
ServiceWorkerMain.ipc
matches the implementation ofIpcMain
to enable IPC with the renderer process service worker thread. IPCs sent from the render worker threads are dispatched onSession
; currently only handling service worker IPCs.ipcMainInternal
now handles IPCs from both web frames and service workers. To differentiate the two, an IPC event has atype
of either 'frame' or 'service-worker'.Preload script changes
Preload scripts now require options beyond only their script location. 'frame' and 'service-worker' are now supported via the
type
property. The appropriate scripts will be fetched when our JS bundles are executed in the renderer process.Given the requirements change, I took this opportunity to make our preload APIs better support third-party libraries based on our best practices.
Architecture Flow
As a starting point for reviewing, consider some of the flows this feature implements.
Checklist
npm test
passesRelease Notes
Notes:
registerPreloadScript
,unregisterPreloadScript
,getPreloadScripts
onSession
.getPreloads
andsetPreloads
onSession
.ServiceWorkerMain
class to interact with service workers in the main process.fromVersionID
onServiceWorkers
to get an instance ofServiceWorkerMain
.running-status-changed
event onServiceWorkers
to indicate when a service worker's running status has changed.contextBridge.evaluateInMainWorld
to safely evaluate code across world boundaries.