feat(stealth): undetectable JS instrumentation via Proxy/exportFunction#1154
Open
feat(stealth): undetectable JS instrumentation via Proxy/exportFunction#1154
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “stealth” JavaScript instrumentation mode for OpenWPM that aims to be less detectable by websites, adds a detection test page + test suite, and extends schema/types/build plumbing to support new instrumentation settings.
Changes:
- Add a stealth instrumentation entry point (
/stealth.js) using FirefoxexportFunction+Proxy, plus associated settings/error-handling modules. - Add end-to-end stealth/legacy detection tests (
test/test_stealth.py) and a detection harness page (stealth_detection.html), plus config validation for mutual exclusion. - Extend JS instrumentation settings schema and generate TypeScript typings from JSON Schema during extension build.
Reviewed changes
Copilot reviewed 16 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
test/test_stealth.py |
Adds pytest coverage for stealth undetectability and data capture, plus a legacy-detectable control. |
test/test_pages/stealth_detection.html |
Adds a local detection-vectors page that reports results via DOM attributes for Selenium. |
test/test_dataclass_validations.py |
Adds validation test ensuring stealth_js_instrument and js_instrument are mutually exclusive. |
schemas/js_instrument_settings.schema.json |
Extends the schema with depth and overwrittenProperties/structured propertiesToInstrument. |
openwpm/config.py |
Adds stealth_js_instrument flag and enforces mutual exclusion with legacy JS instrumentation. |
Extension/webpack.config.js |
Adds a new webpack entry for the stealth bundle. |
Extension/src/types/js_instrument_settings.d.ts |
Generated typings for the updated instrumentation schema. |
Extension/src/types/javascript-instrument.d.ts |
Declares the privileged exportFunction global for TypeScript. |
Extension/src/stealth/stealth.ts |
New stealth content script entry that intercepts frames and wraps functions via Proxy. |
Extension/src/stealth/settings.ts |
Defines stealth instrumentation targets/settings (e.g., Navigator.webdriver override). |
Extension/src/stealth/instrument.ts |
Core stealth instrumentation logic: property discovery, wrapper injection, logging, and overwrite support. |
Extension/src/stealth/error.ts |
Error cloning/stack sanitization helpers for stealth behavior. |
Extension/src/feature.ts |
Wires stealth_js_instrument into extension startup/config dispatch. |
Extension/src/background/javascript-instrument.ts |
Adds legacy mode toggle and registers /stealth.js vs /content.js accordingly. |
Extension/package.json |
Adds json-schema-to-typescript and a schema→types generation build step. |
Extension/package-lock.json |
Locks new build-time dependencies for schema→types generation. |
Extension/eslint.config.mjs |
Ignores the new stealth bundle output. |
Extension/.gitignore |
Ignores the new stealth bundle output. |
.gitignore |
Adds ignore rules for Crosslink-managed local state. |
.dockerignore |
Ignores compiled JS under Extension/src/stealth/ for Docker builds. |
Files not reviewed (1)
- Extension/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+348
to
+360
| declare global { | ||
| interface Object { | ||
| getPrototypeByDepth(subject: string | undefined, depth: number): any; | ||
| getPropertyNamesPerDepth(subject: any, maxDepth: number): any; | ||
| findPropertyInChain(subject, propertyName); | ||
| } | ||
| interface ObjectConstructor { | ||
| getPropertyDescriptor( | ||
| subject: any, | ||
| name: string, | ||
| ): PropertyDescriptor | undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
The helper methods are added via Object.defineProperty(Object.prototype, ...) but the code calls them as static functions (e.g., Object.getPrototypeByDepth / Object.getPropertyNamesPerDepth / Object.findPropertyInChain). These are not declared on ObjectConstructor, so this won’t typecheck in TypeScript and is easy to break at runtime. Prefer plain module-scope helper functions, or explicitly extend ObjectConstructor and define the properties on Object (not Object.prototype) if you truly need static calls.
Comment on lines
+388
to
+435
| Object.defineProperty(Object.prototype, "getPropertyNamesPerDepth", { | ||
| enumerable: false, | ||
| configurable: false, | ||
| writable: false, | ||
| value: function (subject, maxDepth = 0) { | ||
| if (subject === undefined) { | ||
| throw new Error("Can't get property names for undefined"); | ||
| } | ||
| const res = []; | ||
| let depth = 0; | ||
| let properties = Object.getOwnPropertyNames(subject); | ||
| res.push({ depth, propertyNames: properties, object: subject }); | ||
| let proto = Object.getPrototypeOf(subject); | ||
|
|
||
| while (proto !== null && depth < maxDepth) { | ||
| depth++; | ||
| properties = Object.getOwnPropertyNames(proto); | ||
| res.push({ depth, propertyNames: properties, object: proto }); | ||
| proto = Object.getPrototypeOf(proto); | ||
| } | ||
| return res; | ||
| }, | ||
| }); | ||
|
|
||
| /** | ||
| * Finds a property along the prototype chain | ||
| */ | ||
| Object.defineProperty(Object.prototype, "findPropertyInChain", { | ||
| enumerable: false, | ||
| configurable: false, | ||
| writable: false, | ||
| value: function (subject, propertyName) { | ||
| if (subject === undefined || propertyName === undefined) { | ||
| throw new Error("Object and property name must be defined"); | ||
| } | ||
| let properties = []; | ||
| let depth = 0; | ||
| while (subject !== null) { | ||
| properties = Object.getOwnPropertyNames(subject); | ||
| if (properties.includes(propertyName)) { | ||
| return { depth, propertyName }; | ||
| } | ||
| depth++; | ||
| subject = Object.getPrototypeOf(subject); | ||
| } | ||
| throw Error("Property not found. Check whether configuration is correct!"); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This file introduces new properties on Object.prototype (getPrototypeByDepth/getPropertyNamesPerDepth/findPropertyInChain). Even though they’re non-enumerable, this is still prototype pollution and is detectable via Object.getOwnPropertyNames(Object.prototype) (and it’s non-configurable so it can’t be cleaned up). For a “stealth” mode, consider keeping these as local helper functions instead of modifying Object.prototype.
Suggested change
| Object.defineProperty(Object.prototype, "getPropertyNamesPerDepth", { | |
| enumerable: false, | |
| configurable: false, | |
| writable: false, | |
| value: function (subject, maxDepth = 0) { | |
| if (subject === undefined) { | |
| throw new Error("Can't get property names for undefined"); | |
| } | |
| const res = []; | |
| let depth = 0; | |
| let properties = Object.getOwnPropertyNames(subject); | |
| res.push({ depth, propertyNames: properties, object: subject }); | |
| let proto = Object.getPrototypeOf(subject); | |
| while (proto !== null && depth < maxDepth) { | |
| depth++; | |
| properties = Object.getOwnPropertyNames(proto); | |
| res.push({ depth, propertyNames: properties, object: proto }); | |
| proto = Object.getPrototypeOf(proto); | |
| } | |
| return res; | |
| }, | |
| }); | |
| /** | |
| * Finds a property along the prototype chain | |
| */ | |
| Object.defineProperty(Object.prototype, "findPropertyInChain", { | |
| enumerable: false, | |
| configurable: false, | |
| writable: false, | |
| value: function (subject, propertyName) { | |
| if (subject === undefined || propertyName === undefined) { | |
| throw new Error("Object and property name must be defined"); | |
| } | |
| let properties = []; | |
| let depth = 0; | |
| while (subject !== null) { | |
| properties = Object.getOwnPropertyNames(subject); | |
| if (properties.includes(propertyName)) { | |
| return { depth, propertyName }; | |
| } | |
| depth++; | |
| subject = Object.getPrototypeOf(subject); | |
| } | |
| throw Error("Property not found. Check whether configuration is correct!"); | |
| }, | |
| }); | |
| function getPropertyNamesPerDepth(subject, maxDepth = 0) { | |
| if (subject === undefined) { | |
| throw new Error("Can't get property names for undefined"); | |
| } | |
| const res = []; | |
| let depth = 0; | |
| let properties = Object.getOwnPropertyNames(subject); | |
| res.push({ depth, propertyNames: properties, object: subject }); | |
| let proto = Object.getPrototypeOf(subject); | |
| while (proto !== null && depth < maxDepth) { | |
| depth++; | |
| properties = Object.getOwnPropertyNames(proto); | |
| res.push({ depth, propertyNames: properties, object: proto }); | |
| proto = Object.getPrototypeOf(proto); | |
| } | |
| return res; | |
| } | |
| /** | |
| * Finds a property along the prototype chain | |
| */ | |
| function findPropertyInChain(subject, propertyName) { | |
| if (subject === undefined || propertyName === undefined) { | |
| throw new Error("Object and property name must be defined"); | |
| } | |
| let properties = []; | |
| let depth = 0; | |
| while (subject !== null) { | |
| properties = Object.getOwnPropertyNames(subject); | |
| if (properties.includes(propertyName)) { | |
| return { depth, propertyName }; | |
| } | |
| depth++; | |
| subject = Object.getPrototypeOf(subject); | |
| } | |
| throw Error("Property not found. Check whether configuration is correct!"); | |
| } |
Comment on lines
+480
to
+485
| // include the object to each item | ||
| propertiesToInstrument.forEach((propertyList) => { | ||
| propertyList.object = Object.getPrototypeByDepth( | ||
| proto, | ||
| propertyList.depth, | ||
| ); |
There was a problem hiding this comment.
getObjectProperties assumes every entry in logSettings.propertiesToInstrument is an object with .depth and that it’s safe to assign .object onto it. However, the schema/types allow string entries as well, so this will throw or silently misbehave if a settings file uses the string form. Handle the union explicitly (e.g., filter for object entries before adding .object, or normalize strings into {depth, propertyNames} objects).
Suggested change
| // include the object to each item | |
| propertiesToInstrument.forEach((propertyList) => { | |
| propertyList.object = Object.getPrototypeByDepth( | |
| proto, | |
| propertyList.depth, | |
| ); | |
| // include the object on entries that use the object form | |
| propertiesToInstrument = propertiesToInstrument.map((propertyList) => { | |
| if ( | |
| propertyList !== null && | |
| typeof propertyList === "object" && | |
| "depth" in propertyList | |
| ) { | |
| return { | |
| ...propertyList, | |
| object: Object.getPrototypeByDepth(proto, propertyList.depth), | |
| }; | |
| } | |
| return propertyList; |
Comment on lines
+846
to
+865
| function startInstrument(context) { | ||
| for (const item of jsInstrumentationSettings) { | ||
| // retrieve Object properties alont the chain | ||
| let propertyCollection; | ||
| try { | ||
| propertyCollection = getObjectProperties(context, item); | ||
| } catch (err) { | ||
| console.error(err); | ||
| continue; | ||
| } | ||
| // Instrument each Property per object/prototype | ||
| if (propertyCollection[0] !== "") { | ||
| propertyCollection.forEach(({ depth, propertyNames, object }) => { | ||
| if (needsWrapper(object)) { | ||
| propertyNames.forEach((propertyName) => | ||
| instrument(context, item, depth, propertyName), | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This guard is ineffective: propertyCollection[0] is an object (e.g., {depth, propertyNames, object}), so comparing it to the empty string is always true. If you intended to skip empty collections, check propertyCollection.length > 0 (or validate propertyNames.length) instead.
Comment on lines
+4
to
+60
| function generateErrorObject( | ||
| err: { stack: any; name: string | number; message: any }, | ||
| context = undefined, | ||
| ) { | ||
| context = context !== undefined ? context : window; | ||
| const cleaned = cleanErrorStack(err.stack); | ||
| const stack = splitStack(cleaned); | ||
| const lineInfo = getLineInfo(stack); | ||
| const fileName = getFileName(stack); | ||
| let fakeError: { lineNumber: any; columnNumber: any }; | ||
| try { | ||
| const wrappedJS = context.wrappedJSObject; | ||
| const ErrorConstructor = | ||
| typeof err.name === "string" && wrappedJS[err.name] | ||
| ? wrappedJS[err.name] | ||
| : null; | ||
|
|
||
| if ( | ||
| err instanceof DOMException || | ||
| (typeof err.name === "string" && err.name === "DOMException") | ||
| ) { | ||
| // DOMException constructor takes (message, name) not (message, fileName) | ||
| fakeError = new wrappedJS.DOMException(err.message, err.name); | ||
| } else if ( | ||
| ErrorConstructor && | ||
| typeof ErrorConstructor === "function" && | ||
| (ErrorConstructor === wrappedJS.Error || | ||
| ErrorConstructor.prototype instanceof wrappedJS.Error) | ||
| ) { | ||
| fakeError = new ErrorConstructor(err.message, fileName); | ||
| } else { | ||
| // Fallback for custom errors or unknown error types | ||
| fakeError = new wrappedJS.Error(err.message, fileName); | ||
| } | ||
|
|
||
| if (lineInfo) { | ||
| fakeError.lineNumber = lineInfo.lineNumber; | ||
| fakeError.columnNumber = lineInfo.columnNumber; | ||
| } | ||
| } catch (error) { | ||
| console.log("ERROR creation failed. Error was:" + error); | ||
| } | ||
| return fakeError || err; | ||
| } | ||
|
|
||
| /* | ||
| * Trims traces from the stack, which contain the extionsion ID | ||
| */ | ||
| function cleanErrorStack(stack) { | ||
| const extensionID = browser.runtime.getURL(""); | ||
| const lines = typeof stack !== "string" ? stack : splitStack(stack); | ||
| lines.forEach((line) => { | ||
| if (line.includes(extensionID)) { | ||
| stack = stack.replace(line + "\n", ""); | ||
| } | ||
| }); | ||
| return stack; |
There was a problem hiding this comment.
generateErrorObject calls cleanErrorStack(err.stack) and splitStack(cleaned) before the try/catch. If err.stack is undefined/null (or any non-string), cleanErrorStack/splitStack will throw (e.g., lines.forEach / stack.split), reintroducing the crash path this PR aims to fix. Make cleanErrorStack/getBeginOfScriptCalls/splitStack resilient to missing/non-string stacks (e.g., treat falsy as "" and only use .replace/.split when stack is a string).
|
|
||
| export const jsInstrumentationSettings: JSInstrumentSettings = [ | ||
| { | ||
| object: "ScriptProcessorNode", // Depcrecated. Replaced by AudioWorkletNode |
There was a problem hiding this comment.
Spelling: “Depcrecated” → “Deprecated”.
Suggested change
| object: "ScriptProcessorNode", // Depcrecated. Replaced by AudioWorkletNode | |
| object: "ScriptProcessorNode", // Deprecated. Replaced by AudioWorkletNode |
Comment on lines
+149
to
+152
| // Add shared prototype by AudioContenxt/OfflineAudioContext | ||
| { | ||
| object: "AudioContext", | ||
| instrumentedName: "[AudioContenxt|OfflineAudioContext]", |
There was a problem hiding this comment.
Spelling: “AudioContenxt” → “AudioContext” (appears in both the comment and instrumentedName string).
Suggested change
| // Add shared prototype by AudioContenxt/OfflineAudioContext | |
| { | |
| object: "AudioContext", | |
| instrumentedName: "[AudioContenxt|OfflineAudioContext]", | |
| // Add shared prototype by AudioContext/OfflineAudioContext | |
| { | |
| object: "AudioContext", | |
| instrumentedName: "[AudioContext|OfflineAudioContext]", |
| constructor.prototype, | ||
| "contentWindow", | ||
| ); | ||
| // TODO: Continue here!!!! |
There was a problem hiding this comment.
There’s a leftover “TODO: Continue here!!!!” in the frame-property protection logic. Since this runs at document_start on all frames, TODOs here make it hard to assess completeness and can hide missing cases. Either remove the TODO or replace it with a concrete, actionable comment describing what remains and why it’s safe to ship as-is.
Suggested change
| // TODO: Continue here!!!! | |
| // Wrap the native getter so every accessed frame window is passed through | |
| // `singleCallback` before being returned. This hook intentionally covers | |
| // `contentWindow` and `contentDocument`, which are the frame accessors used | |
| // by the surrounding protection logic; broader frame-surface hardening is | |
| // handled elsewhere and is not required for this interception point. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1154 +/- ##
==========================================
- Coverage 62.18% 60.62% -1.57%
==========================================
Files 40 41 +1
Lines 3898 4015 +117
==========================================
+ Hits 2424 2434 +10
- Misses 1474 1581 +107 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Use Object.defineProperty with enumerable:false for Object.prototype additions (getPrototypeByDepth, getPropertyNamesPerDepth, findPropertyInChain) to prevent prototype pollution detection - Fix innerHTML/outerHTML setter protection: use computed property setter syntax `set [property](value)` instead of broken Proxy-trap-style `set(obj, _prop, value)` - Replace wrappedObjects array with WeakSet to prevent memory leaks - Add instanceof/existence checks in generateErrorObject for DOMException and custom error types, with fallback to generic Error - Add null/empty stack guards in getLineInfo and getFileName; switch matchAll string args to regex literals for correctness
…onal output sanitization [CL-2] - generateErrorObject: return original error as fallback instead of undefined - DOMException: preserve err.name (e.g. SecurityError) via constructor second arg - Object.getPropertyDescriptor: use Object.defineProperty with enumerable:false - Enhance test_stealth_records_js_calls with data quality assertions - Add test_legacy_records_with_call_stacks baseline test
- Guard err.stack in generateErrorObject before cleanErrorStack/splitStack - Fix propertyCollection guard to check .length instead of comparing object to string - Fix typos: Depcrecated→Deprecated, AudioContenxt→AudioContext - Remove stale TODO comment in iframe contentWindow instrumentation
vringar
force-pushed
the
feat/stealth-js-instrument-v2
branch
from
3e82973 to
eecc4f6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
exportFunctionAPI andProxyobjectstoString(), error stacks are cleaned of extension URLs, no globals leakedgenerateErrorObjectcrash path (could returnundefined, caller throws it)SecurityErrorwas silently downgraded toError)Object.getPropertyDescriptornon-enumerable to prevent fingerprinting viaObject.keys(Object)Based on Krumnow et al.. Supersedes #1148. Incorporates fixes from adversarial review (VDD methodology).
VDD Review History
undefinedthrow path, DOMException name erasureTest plan
test_stealth_passes_all_detection_checks— defeats 10 detection vectorstest_stealth_records_js_calls— JS API calls recorded to databasetest_legacy_instrument_is_detectable— control test confirms detection page workspre-commit run --all-filespasses