-
Notifications
You must be signed in to change notification settings - Fork 315
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
Arbitrary WebAPI JS instrumentation #642
Conversation
Getting errors like OpenWPM: Error name: TypeError post_request_ajax.html:237:17 OpenWPM: Error message: can't redefine non-configurable property "UNSENT" post_request_ajax.html:238:17
@englehardt - how would you feel if "fingerprinting" instrumented a few more things e.g. whole of navigator and CanvasRenderingContext2D? |
If it seems like it can be used for fingerprinting I think it's fair game. My only warning is that some APIs are incredibly noisy. |
Yeah, I think I need to leave a more flexible solution open. |
@englehardt - how do you feel about this as a starting point API for this PR. It will meet all my needs and the needs of the current fingerprinting instrumentation. We can follow-up with more implementation as needs arise (see "follow-on prs" above). js_instrument:true,
js_instrument_modules: [
// Shortcut
"fingerprinting",
// APIs
"Storage",
{"XMLHttpRequest": ["send"]},
// Specific instances on window
{"window.document": ["cookie", "referrer"]},
{"window": ["name", "localStorage", "sessionStorage"]}
],
http_instrument:true,
callstack_instrument:true, |
This looks like a great starting point! Thanks! |
* We build and mandate LogSettings. * We have a new JSInstrumentatinRequest that everything runs through * Preset, fingerprinting, will be specified in JSON
Enum for Operation
Changing my mind - all validation and construction to be done python side. This will reduce JS overhead at runtime.
This reverts commit 8eb4edb.
@englehardt This PR is finished. I've incorporated all your suggestions / questions / concerns and the things we discussed in person the other day. The only thing I had to not do was "JSON everywhere". Half way through changing, I remembered why I don't do that. Here's why: (copied from front matter on this PR)
Whilst it may be possible to do it via JSON being passed to the webext, I don't think it should hold up this PR. Users still write a python list or JSON, it's just the transport between python and webext that's finecky and I don't think this PR needs to guarantee that that interface will be stable. |
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.
r+ with a few comments and nits. Thanks for taking this all the way and adding some thorough tests for the changes!
Whilst it may be possible to do it via JSON being passed to the webext, I don't think it should hold up this PR. Users still write a python list or JSON, it's just the transport between python and webext that's finecky and I don't think this PR needs to guarantee that that interface will be stable.
Okay that makes sense. I guess we could eval
those strings, but that seems messy. Either way, no need to hold up this PR.
@@ -45,11 +45,13 @@ export interface WebNavigationOnCommittedEventDetails | |||
transitionQualifiers?: TransitionQualifier[]; | |||
} | |||
|
|||
/* |
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.
Why comment these out?
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.
Whoops. Was planning to delete them. They're not used anywhere so was just cleaning up. That can be separate PR.
@@ -5,7 +5,7 @@ | |||
|
|||
curdir = os.path.dirname(os.path.realpath(__file__)) | |||
schema_path = os.path.join( | |||
curdir, 'js_instrumentation', 'js_instrument_modules.schema' | |||
curdir, os.pardir, 'schemas', 'js_instrument_settings.schema.json' |
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 .schema.json
the accepted naming pattern, or should this be _schema.json
?
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 the naming pattern that the jsonschema2md uses by default. i'm not wed to it, I don't think it's a big change to jsonschema2md call to fix it.
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 it's the default that's fine. No need to change, I just wanted to make sure this wasn't a bug.
Co-authored-by: Steven Englehardt <englehardt@gmail.com>
* Add title * Fix typo in mac-osx hyperlink
We're not using the js in two htmls now, so unify like other test files
* pyside test must instrument browser apis * add more to readme to clarify instrumenting
@englehardt this is done. The bad test needed a bigger tweak (#642 (comment)) but nothing wild. |
Fixes #641
To do:
Can't redefine non-configurable property "XXX"
. Need to handle this property type. There may be other cases that come up. wontfix - these errors don't prevent instrumentation occurring.For follow-on issues:
Add an "all" option(nope - crashes everything)Do you want to be able to specify just a property? e.g.XMLHttpReqest.send
vsXMLHttpRequest
(this could be a follow-on PR)Questions:
'{object: window.CanvasRenderingContext2D.prototype, instrumentedName: "CanvasRenderingContext2D",...}'
and{object: "window.CanvasRenderingContext2D.prototype", instrumentedName: "CanvasRenderingContext2D",...}
. In the latter case where we're passing around JSON, on the JS side we then have to find a way to turn the string"window.CanvasRenderingContext2D.prototype"
into the objectwindow.CanvasRenderingContext2D.prototype
.