Repository navigation
feat(bson-parsing): restrict web worker scope COMPASS-11128 #892
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
Changes from all commits
1d6f733
fa50d8b
d71ba44
fa3c639
1b917a2
5a5cddb
c6d9db7
8120d01
59f0c1a
9c092b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,106 @@ import { trackBSON, untrackBSON } from './structured-clone-bson.js'; | |
|
|
||
| import type { WorkerRequest, WorkerResponse } from './worker-types.js'; | ||
|
|
||
| // Exported for test | ||
| const { self, postMessage } = globalThis; | ||
|
|
||
| // Exported for tests | ||
| export const ALLOWED_GLOBALS = new Set([ | ||
| 'Object', | ||
| 'Array', | ||
| 'Function', | ||
| 'String', | ||
| 'Number', | ||
| 'Boolean', | ||
| 'Symbol', | ||
| 'BigInt', | ||
| 'Math', | ||
| 'Date', | ||
| 'RegExp', | ||
| 'JSON', | ||
| 'Map', | ||
| 'Set', | ||
| 'WeakMap', | ||
| 'WeakSet', | ||
| 'Promise', | ||
| 'Proxy', | ||
| 'Reflect', | ||
| 'Error', | ||
| 'TypeError', | ||
| 'RangeError', | ||
| 'SyntaxError', | ||
| 'ReferenceError', | ||
| 'EvalError', | ||
| 'URIError', | ||
| 'ArrayBuffer', | ||
| 'SharedArrayBuffer', | ||
| 'DataView', | ||
| 'Uint8Array', | ||
| 'Int8Array', | ||
| 'Uint8ClampedArray', | ||
| 'Uint16Array', | ||
| 'Int16Array', | ||
| 'Uint32Array', | ||
| 'Int32Array', | ||
| 'Float32Array', | ||
| 'Float64Array', | ||
| 'BigInt64Array', | ||
| 'BigUint64Array', | ||
| 'TextEncoder', | ||
| 'TextDecoder', | ||
| 'undefined', | ||
| 'NaN', | ||
| 'Infinity', | ||
| 'isNaN', | ||
| 'isFinite', | ||
| 'parseFloat', | ||
| 'parseInt', | ||
| 'encodeURIComponent', | ||
| 'decodeURIComponent', | ||
| 'Buffer', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was added to support the usage around LegacyUUID in scope.ts. I switched to Also, now that we added support for node as well, I've to keep it as web-worker package relies on it
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @mabaasit, the ticket says to remove Node globals, but the PR’s allowlist still includes both process and Buffer. The comment says process is needed by the web-worker package. You mentioned the code switched to bson.ByteUtils instead of Buffer (I can't open 4623dfa link - if it's addressed there). Confirming whether Buffer is still needed and whether keeping process is an acceptable exception to the task’s security goal.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| // bson's byteutils use these internally | ||
| 'atob', | ||
| 'btoa', | ||
| 'crypto', | ||
|
|
||
| // web-worker module relies on process as it supports both node and browser. | ||
| 'process', | ||
| 'console', | ||
| ]); | ||
|
|
||
| // Exported for tests | ||
| export function restrictGlobalScope(scope: object): void { | ||
| for (const key of Object.getOwnPropertyNames(scope)) { | ||
| if (ALLOWED_GLOBALS.has(key)) continue; | ||
| try { | ||
| delete (scope as any)[key]; | ||
| } catch { | ||
| // Non-configurable in this environment | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Exported for tests | ||
| export const DISALLOWED_PROTOTYPE_PROPS = [ | ||
| '__proto__', | ||
| '__defineGetter__', | ||
| '__defineSetter__', | ||
| '__lookupGetter__', | ||
| '__lookupSetter__', | ||
| ] as const; | ||
|
|
||
| // Exported for tests | ||
| export function restrictObjectPrototype(): void { | ||
| for (const key of DISALLOWED_PROTOTYPE_PROPS) { | ||
| try { | ||
| delete (Object.prototype as any)[key]; | ||
| } catch { | ||
| // Non-configurable in this environment | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Exported for tests | ||
| export function handleRequest(request: WorkerRequest): WorkerResponse { | ||
| const { id, args } = request; | ||
| try { | ||
|
|
@@ -16,7 +115,9 @@ export function handleRequest(request: WorkerRequest): WorkerResponse { | |
| } | ||
|
|
||
| if (typeof self !== 'undefined') { | ||
| restrictObjectPrototype(); | ||
| restrictGlobalScope(globalThis); | ||
| self.onmessage = (event: MessageEvent<WorkerRequest>) => { | ||
| (self as unknown as Worker).postMessage(handleRequest(event.data)); | ||
| postMessage(handleRequest(event.data)); | ||
| }; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Based on what data is the full allow list constructed?
I could see "console" isn't in allow list. Could be the case web-worker's Node shim calls console.error when a handler throws, which would turn errors into confusing ReferenceErrors.
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 list is based on the current tests, integration with compass, globals needed for parsing etc.
Nice catch with
console. I'll add that to this list