Skip to content

Commit 7ab8ec1

Browse files
fix(taint): instrument object listeners in postMessage analysis (#25)
This commit updates `instrumentPostMessage` in `taint_shim.js` to correctly instrument event listeners that are objects with a `handleEvent` method, ensuring security checks within these methods are analyzed. It also addresses JSDOM environment quirks discovered during testing by ensuring `window.addEventListener` is explicitly patched if it differs from the prototype, and adds proper cache cleanup to support robust testing. Fixes bug where object-based event listeners were ignored. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 parent 77835c4 commit 7ab8ec1

2 files changed

Lines changed: 100 additions & 4 deletions

File tree

internal/analysis/active/taint/taint_shim.js

Lines changed: 77 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@
9898
// 5. Reset instrumentation flag - MUST BE LAST
9999
// This flag is used by async callbacks (setTimeout, Promises, Observers) to check if the context is still valid.
100100
scope.__SCALPEL_TAINT_INSTRUMENTED__ = false;
101+
102+
// Clear shared caches to allow re-instrumentation in tests
103+
if (scope.instrumentedCache) scope.instrumentedCache.clear();
104+
if (scope.listenerWrapperMap) scope.listenerWrapperMap = new WeakMap();
101105
}
102106

103107
// Cleanup previous shim instances if re-initializing (e.g. in tests)
@@ -731,8 +735,11 @@ function instrumentPostMessage() {
731735

732736
// --- 3. Add Event Listener Wrapper ---
733737
const addWrapper = function(type, listener, options) {
734-
// Filter: Must be 'message' event, must be a function, must be on the Window
735-
if (type !== 'message' || typeof listener !== 'function' || !isGlobalWindow(this)) {
738+
const isFunction = typeof listener === 'function';
739+
const isObjectHandler = typeof listener === 'object' && listener !== null && typeof listener.handleEvent === 'function';
740+
741+
// Filter: Must be 'message' event, must be a function or object handler, must be on the Window
742+
if (type !== 'message' || (!isFunction && !isObjectHandler) || !isGlobalWindow(this)) {
736743
return originalAddEventListener.call(this, type, listener, options);
737744
}
738745

@@ -755,7 +762,11 @@ function instrumentPostMessage() {
755762

756763
try {
757764
// Execute the original listener with the proxied event
758-
listener.call(this, eventProxy);
765+
if (isFunction) {
766+
listener.call(this, eventProxy);
767+
} else {
768+
listener.handleEvent(eventProxy);
769+
}
759770
} catch (e) {
760771
throw e;
761772
} finally {
@@ -780,7 +791,10 @@ function instrumentPostMessage() {
780791

781792
// --- 4. Remove Event Listener Wrapper ---
782793
const removeWrapper = function(type, listener, options) {
783-
if (type === 'message' && typeof listener === 'function' && isGlobalWindow(this)) {
794+
const isFunction = typeof listener === 'function';
795+
const isObjectHandler = typeof listener === 'object' && listener !== null && typeof listener.handleEvent === 'function';
796+
797+
if (type === 'message' && (isFunction || isObjectHandler) && isGlobalWindow(this)) {
784798
if (scope.listenerWrapperMap.has(listener)) {
785799
const wrappedListener = scope.listenerWrapperMap.get(listener);
786800
return originalRemoveEventListener.call(this, type, wrappedListener, options);
@@ -794,6 +808,65 @@ function instrumentPostMessage() {
794808
scope.EventTarget.prototype.addEventListener = addWrapper;
795809
scope.EventTarget.prototype.removeEventListener = removeWrapper;
796810

811+
// Register cleanup for EventTarget.prototype modifications
812+
cleanupFunctions.push(() => {
813+
try {
814+
if (scope.EventTarget.prototype.addEventListener === addWrapper) {
815+
scope.EventTarget.prototype.addEventListener = originalAddEventListener;
816+
}
817+
if (scope.EventTarget.prototype.removeEventListener === removeWrapper) {
818+
scope.EventTarget.prototype.removeEventListener = originalRemoveEventListener;
819+
}
820+
} catch(e) {
821+
logger.error("Error restoring EventTarget.prototype listeners:", e);
822+
}
823+
});
824+
825+
// JSDOM/Environment FIX: Ensure window.addEventListener is also updated if it did not inherit the change.
826+
// This check detects if window has its own property that masks the prototype, OR if inheritance failed.
827+
if (scope.addEventListener !== addWrapper) {
828+
const originalWindowAddEvent = scope.addEventListener;
829+
const originalWindowRemoveEvent = scope.removeEventListener;
830+
831+
try {
832+
Object.defineProperty(scope, 'addEventListener', {
833+
value: addWrapper,
834+
writable: true,
835+
configurable: true
836+
});
837+
Object.defineProperty(scope, 'removeEventListener', {
838+
value: removeWrapper,
839+
writable: true,
840+
configurable: true
841+
});
842+
843+
// Register cleanup to restore window-specific methods
844+
cleanupFunctions.push(() => {
845+
try {
846+
if (scope.addEventListener === addWrapper && originalWindowAddEvent) {
847+
Object.defineProperty(scope, 'addEventListener', {
848+
value: originalWindowAddEvent,
849+
writable: true,
850+
configurable: true
851+
});
852+
}
853+
if (scope.removeEventListener === removeWrapper && originalWindowRemoveEvent) {
854+
Object.defineProperty(scope, 'removeEventListener', {
855+
value: originalWindowRemoveEvent,
856+
writable: true,
857+
configurable: true
858+
});
859+
}
860+
} catch (e) {
861+
logger.error("Error restoring window.addEventListener during cleanup:", e);
862+
}
863+
});
864+
865+
} catch (e) {
866+
// Ignore if we can't redefine (e.g. non-configurable)
867+
}
868+
}
869+
797870
// Mark as instrumented
798871
scope.instrumentedCache.add(originalAddEventListener);
799872
scope.instrumentedCache.add(originalRemoveEventListener);

internal/analysis/active/taint/taint_shim.test.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,29 @@ describe('Scalpel Taint Shim V2 (Unabridged Compliance Suite)', () => {
847847
// Handler should be called exactly once.
848848
expect(handler).toHaveBeenCalledTimes(1);
849849
});
850+
851+
it('Object Listener: Should report vulnerability when listener is an object with handleEvent', async () => {
852+
let handlerExecuted = false;
853+
const unsafeHandler = {
854+
handleEvent: function(e) {
855+
handlerExecuted = true;
856+
const val = e.data; // Taint sink access
857+
}
858+
};
859+
window.addEventListener('message', unsafeHandler);
860+
861+
window.dispatchEvent(new MessageEvent('message', {
862+
data: TAINTED_STRING,
863+
origin: "http://evil.com"
864+
}));
865+
866+
await wait();
867+
868+
expect(handlerExecuted).toBe(true);
869+
expect(mockSinkCallback).toHaveBeenCalled();
870+
const report = mockSinkCallback.mock.calls[0][0];
871+
expect(report.type).toBe("POSTMESSAGE_MISSING_ORIGIN_CHECK");
872+
});
850873
});
851874

852875
describe('2. Storage Inspector (Local & Session)', () => {

0 commit comments

Comments
 (0)