Skip to content

Commit

Permalink
Fix Actor XSS, introduce subscribe (#3329)
Browse files Browse the repository at this point in the history
* Fix XSS, intorduce subscribe

* Update changelog
  • Loading branch information
HarelM authored Nov 10, 2023
1 parent c63e731 commit 5b2b2bd
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
### 🐞 Bug fixes

- Fix null feature properties in resolve_tokens ([#3272](https://github.com/maplibre/maplibre-gl-js/pull/3272))
- Fixes a security issue in `Actor` against XSS attacks in postMessage / onmessage ([#3239](https://github.com/maplibre/maplibre-gl-js/pull/3239))
- _...Add new stuff here..._

## 3.5.2
Expand Down
14 changes: 14 additions & 0 deletions src/util/actor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,18 @@ describe('Actor', () => {

expect(spy).not.toHaveBeenCalled();
});

test('should not process a message with the wrong origin', async () => {
const worker = workerFactory() as any as WorkerGlobalScopeInterface & ActorTarget;
const actor = new Actor(worker, '1');

const spy = jest.fn().mockReturnValue(Promise.resolve({}));
worker.worker.actor.registerMessageHandler('getClusterExpansionZoom', spy);

actor.target.postMessage({type: 'getClusterExpansionZoom', data: {} as any, origin: 'https://example.com'});

await sleep(100);

expect(spy).not.toHaveBeenCalled();
});
});
25 changes: 16 additions & 9 deletions src/util/actor.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {isWorker} from './util';
import {Subscription, isWorker, subscribe} from './util';
import {serialize, deserialize, Serialized} from './web_worker_transfer';
import {ThrottledInvoker} from './throttled_invoker';

Expand All @@ -21,6 +21,7 @@ export interface ActorTarget {
type MessageData = {
id: string;
type: MessageType | '<cancel>' | '<response>';
origin: string;
data?: Serialized;
targetMapId?: string | number | null;
mustQueue?: boolean;
Expand Down Expand Up @@ -59,6 +60,7 @@ export class Actor implements IActor {
invoker: ThrottledInvoker;
globalScope: ActorTarget;
messageHandlers: { [x in MessageType]?: MessageHandler<MessageType>};
subscription: Subscription;

/**
* @param target - The target
Expand All @@ -73,8 +75,8 @@ export class Actor implements IActor {
this.taskQueue = [];
this.abortControllers = {};
this.messageHandlers = {};
this.invoker = new ThrottledInvoker(this.process);
this.target.addEventListener('message', this.receive, false);
this.invoker = new ThrottledInvoker(() => this.process());
this.subscription = subscribe(this.target, 'message', (message) => this.receive(message), false);
this.globalScope = isWorker(self) ? target : window;
}

Expand Down Expand Up @@ -106,6 +108,7 @@ export class Actor implements IActor {
const cancelMessage: MessageData = {
id,
type: '<cancel>',
origin: location.origin,
targetMapId: message.targetMapId,
sourceMapId: this.mapId
};
Expand All @@ -118,16 +121,19 @@ export class Actor implements IActor {
...message,
id,
sourceMapId: this.mapId,
origin: location.origin,
data: serialize(message.data, buffers)
};
this.target.postMessage(messageToPost, {transfer: buffers});
});
}

receive = (message: {data: MessageData}) => {
receive(message: {data: MessageData}) {
const data = message.data;
const id = data.id;

if (data.origin !== location.origin) {
return;
}
if (data.targetMapId && this.mapId !== data.targetMapId) {
return;
}
Expand Down Expand Up @@ -158,9 +164,9 @@ export class Actor implements IActor {
// In the main thread, process messages immediately so that other work does not slip in
// between getting partial data back from workers.
this.processTask(id, data);
};
}

process = () => {
process() {
if (this.taskQueue.length === 0) {
return;
}
Expand All @@ -179,7 +185,7 @@ export class Actor implements IActor {
}

this.processTask(id, task);
};
}

async processTask(id: string, task: MessageData) {
if (task.type === '<response>') {
Expand Down Expand Up @@ -220,6 +226,7 @@ export class Actor implements IActor {
id,
type: '<response>',
sourceMapId: this.mapId,
origin: location.origin,
error: err ? serialize(err) : null,
data: serialize(data, buffers)
};
Expand All @@ -228,6 +235,6 @@ export class Actor implements IActor {

remove() {
this.invoker.remove();
this.target.removeEventListener('message', this.receive, false);
this.subscription.unsubscribe();
}
}
27 changes: 27 additions & 0 deletions src/util/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -642,3 +642,30 @@ export async function getImageData(
}
return readImageDataUsingOffscreenCanvas(image, x, y, width, height);
}

export interface Subscription {
unsubscribe(): void;
}

export interface Subscriber {
addEventListener: typeof window.addEventListener;
removeEventListener: typeof window.removeEventListener;
}

/**
* This method is used in order to register an event listener using a lambda function.
* The return value will allow unsubscribing from the event, without the need to store the method reference.
* @param target - The target
* @param message - The message
* @param listener - The listener
* @param options - The options
* @returns a subscription object that can be used to unsubscribe from the event
*/
export function subscribe(target: Subscriber, message: keyof WindowEventMap, listener: (...args: any) => void, options: boolean | AddEventListenerOptions): Subscription {
target.addEventListener(message, listener, options);
return {
unsubscribe: () => {
target.removeEventListener(message, listener, options);
}
};
}

0 comments on commit 5b2b2bd

Please sign in to comment.