Skip to content
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

[Firefox] Can't modify page properties on sites which use CSP #997

Closed
chocolateboy opened this issue Jun 2, 2020 · 17 comments
Closed

[Firefox] Can't modify page properties on sites which use CSP #997

chocolateboy opened this issue Jun 2, 2020 · 17 comments

Comments

@chocolateboy
Copy link

This is my abiding issue with Violentmonkey (which I ❤️ - thank you!), but I can't see an open issue for it. There are related closed issues, and this issue may point the way to a fix, but I thought it'd be better to track the bug explicitly rather than inferring it from the documentation[1] and scattered comments.

What is the problem?

It's not possible to modify/mutate direct or nested properties of a page's window object with the following combination:

  • Violentmonkey for Firefox
  • sites which use CSP (e.g. GitHub, Google, Twitter)

Sites which use CSP don't run in Violentmonkey for Firefox unless @inject-into content is enabled, but @inject-into content is not compatible with @grant none, which is needed to modify page objects.

Userscript engines this works in:

  • Tampermonkey (tested on Firefox)

Userscript engines this doesn't work in:

How to reproduce it?

// ==UserScript==
// @name          Hook XHR#open
// @version       0.0.1
// @include       https://twitter.com/*
// @include       https://github.com/*
// @include       https://*.google.tld/*
// @grant         none
// @inject-into   content
// ==/UserScript==

function hookXHROpen (oldOpen) {
    return function open (...args) {
        console.warn('inside XHR#open')
        return oldOpen.apply(this, args)
    }
}

// or unsafeWindow...
window.XMLHttpRequest.prototype.open = hookXHROpen(
    window.XMLHttpRequest.prototype.open
)

What is the expected result?

XHR#open should be hooked and the message should be logged on those sites.

What is the actual result?

XHR#open isn't hooked and the message isn't logged.

Related issues

Environment

  • Browser: Firefox v76.0.1
  • Violentmonkey: v2.12.7
  • OS: Linux (Arch)

Footnotes

  1. "Scripts requiring access to JavaScript objects in the web page will not work in [@inject-into content] mode."
  2. "GM4 does not yet support @grant none."
  3. @grant none isn't supported. unsafeWindow is but I couldn't get the XHR#open hook to work.
@chocolateboy
Copy link
Author

chocolateboy commented Jun 2, 2020

Incidentally, I'd prefer to use unsafeWindow rather than @grant none, but that doesn't work either (i.e. doesn't allow the underlying window object to be modified) if @inject-into content is used.

Ditto window.wrappedJSObject.

@tophf
Copy link
Member

tophf commented Jun 2, 2020

AFAIK Tampermonkey rewrites the CSP response header to allow its script elements. We could do it as well but it seems quite radical, I'm not even sure Mozilla will allow it in the future, they dislike addons rewriting CSP last time I heard.

unsafeWindow.wrappedJSObject works for me in Violentmonkey:

// ==UserScript==
// @name        test wrappedJSObject
// @match       https://www.google.com/*
// @grant       GM_foo
// @run-at      document-start
// @inject-into content
// ==/UserScript==

const pageWnd = unsafeWindow.wrappedJSObject;
const p = pageWnd.XMLHttpRequest.prototype;
const {open} = p;
const xhrOpen = function (method, url) {
  console.warn('intercepted', url);
  return open.apply(this, arguments);
}
p.open = exportFunction(xhrOpen, pageWnd);

@tophf
Copy link
Member

tophf commented Jun 2, 2020

Don't forget to use exportFunction or cloneInto with cloneFunctions:true option.

@tophf tophf closed this as completed Jun 2, 2020
@chocolateboy
Copy link
Author

This ended up being quite fiddly to get working. While I appreciate the fact that a workaround exists, I still think this is a bug and that unsafeWindow should work out of the box without the need for engine-specific workarounds.

For anyone else who needs a working example of this in action, see here.

In particular, note that getting Object.defineProperty to work (e.g. to modify a XHR response) may be non-obvious:

This doesn't work

const descriptor = cloneInto({ value: json }, unsafeWindow.wrappedJSObject)
Object.defineProperty(xhr, 'responseText', descriptor)

This works

const descriptor = cloneInto({ value: json }, unsafeWindow.wrappedJSObject)
unsafeWindow.wrappedJSObject.Object.defineProperty(xhr, 'responseText', descriptor)

@tophf
Copy link
Member

tophf commented Jun 3, 2020

We couldn't make unsafeWindow work like this by default because we need to support legacy scripts which is the entire point of using Violentmonkey in Firefox instead of Greasemonkey/Firemonkey. We tried it already but failed because the userscripts need to knowingly and correctly use exportFunction/cloneInto. When they don't their code breaks in various unexpected ways if they expose stuff into unsafeWindow like e.g. unsafeWindow.foo = () => console.log('123'); What's worse if such code overrides a standard window property it will completely break a lot of pages which actually happened last time we tried.

This works

Yep. Every global builtin such as Object belongs to window of a respective execution context, this is how JS does things.

@chocolateboy
Copy link
Author

chocolateboy commented Jun 3, 2020

We couldn't make unsafeWindow work like this by default because we need to support legacy scripts which is the entire point of using Violentmonkey in Firefox instead of Greasemonkey/Firemonkey.

Legacy scripts expect unsafeWindow to just work, the way it works in Greasemonkey 3 and Tampermonkey — i.e. direct mutation of the page's objects. That's not currently how it works in Violentmonkey on sites which use CSP. Because of its non-conforming behavior, I don't think it should even be called unsafeWindow in this context.

We tried it already but failed

That's why I think this bug should remain open until it's either fixed in Firefox, or Violentmonkey for Firefox migrates to the new userscript API.

@tophf
Copy link
Member

tophf commented Jun 3, 2020

The problem was that if such code overrides a standard window property via unsafeWindow it will completely break a lot of pages which actually happened last time we tried.

@chocolateboy
Copy link
Author

chocolateboy commented Jun 3, 2020

Then call it safeWindow? The point is, it differs from the standard/expected behavior.

Tampermonkey

The unsafeWindow object provides full access to the pages javascript functions and variables.

Greasemonkey 3

This API object allows a User script to access "custom" properties--variable and functions defined in the page--set by the web page. The unsafeWindow object is shorthand for window.wrappedJSObject. It is the raw window object inside the XPCNativeWrapper provided by the Greasemonkey sandbox.

@tophf
Copy link
Member

tophf commented Jun 3, 2020

Tampermonkey modifies CSP of the sites which is rather intrusive. I'm not convinced we should do the same as I explained above.

Greasemonkey 3 doesn't run anymore in modern Firefox 57+.

Greasemonkey 4 doesn't care about compatibility with the older scripts so we can't follow suit.

Anyway AFAIK there aren't that many scripts that are affected by this predicament as people don't normally use the content mode. As for me, I don't use Firefox, the same applies to gera2ld, so there's no one to spearhead development of such browser-specific things as wrappedJSObject and the extensive reasearch/testing of the existing userscripts it entails.

@sn260591
Copy link

sn260591 commented Jun 4, 2020

This ended up being quite fiddly to get working. While I appreciate the fact that a workaround exists, I still think this is a bug and that unsafeWindow should work out of the box without the need for engine-specific workarounds.

For anyone else who needs a working example of this in action, see here.

In particular, note that getting Object.defineProperty to work (e.g. to modify a XHR response) may be non-obvious:

This doesn't work

const descriptor = cloneInto({ value: json }, unsafeWindow.wrappedJSObject)
Object.defineProperty(xhr, 'responseText', descriptor)

This works

const descriptor = cloneInto({ value: json }, unsafeWindow.wrappedJSObject)
unsafeWindow.wrappedJSObject.Object.defineProperty(xhr, 'responseText', descriptor)

In this case, there is no need to clone the descriptor.
cloneInto and exportFunction are needed only for objects and functions that the page code accesses directly. I recommend that you read the official documentation.

chocolateboy added a commit to chocolateboy/userscripts that referenced this issue Jun 4, 2020
@chocolateboy
Copy link
Author

chocolateboy commented Jun 4, 2020

In this case, there is no need to clone the descriptor.

Thanks!

Unfortunately, that doesn't appear to work for me in this case (install), but I can't see an error message (the script just hangs), so it's possible I've overlooked something. Do you have an example of it working without the clone?

cloneInto and exportFunction are needed only for objects and functions that the page code accesses directly.

Where does it say that?

I recommend that you read the official documentation.

I recommend that this is provided by the userscript engine as unsafeWindow so that userscript developers don't need to be versed in low-level details of Firefox addon development to reimplement a standard userscript API.

@sn260591
Copy link

sn260591 commented Jun 4, 2020

Unfortunately, that doesn't appear to work for me in this case (install), but I can't see an error message (the script just hangs), so it's possible I've overlooked something. Do you have an example of it working without the clone?

Use Object.defineProperty without Compat.unsafeWindow.

Where does it say that?

This conclusion is drawn from the documentation and my observations.

@chocolateboy
Copy link
Author

Use Object.defineProperty without Compat.unsafeWindow.

Yeah, that doesn't work either (install). It doesn't modify the XHR object in the page (why would it?).

This conclusion is drawn from the documentation and my observations.

I look forward to seeing a working example to confirm this.

@sn260591
Copy link

sn260591 commented Jun 4, 2020

Yeah, that doesn't work either (install). It doesn't modify the XHR object in the page (why would it?).

You must export this function, because it is called by xhr, which is in the page scope.

I look forward to seeing a working example to confirm this.

Something like that?

// ==UserScript==
// @name test
// @match *://*/*
// @inject-into content
// @run-at document-start
// ==/UserScript==

const originalLog = console.log;
const func = exportFunction(() => originalLog("fake message"), unsafeWindow.wrappedJSObject);
Object.defineProperty(unsafeWindow.wrappedJSObject.console, "log", { value: func });

@chocolateboy
Copy link
Author

You must export this function, because it is called by xhr, which is in the page scope.

We should take this to my repo rather than further derailing this issue (if you have a particular fix or feature in mind), but, to be clear, that function (and the userscript) works fine without that, and doesn't work with your suggested changes.

As for the example you've given, I don't see any difference between that and assigning directly to console.log, e.g.:

unsafeWindow.wrappedJSObject.console.log = func

- which is what the script already does, e.g.:

unsafeWindow.wrappedJSObject.XMLHttpRequest.prototype.send = func

@sn260591
Copy link

sn260591 commented Jun 5, 2020

@chocolateboy How to determine that your script is working properly?

As for the example you've given, I don't see any difference between that and assigning directly to console.log, e.g.:

unsafeWindow.wrappedJSObject.console.log = func

Try execute console.log("test") in the web console. Without exporting the function, an error will occur.

@chocolateboy chocolateboy changed the title [Firefox] Can't modify window properties on sites which use CSP [Firefox] Can't modify page properties on sites which use CSP Jun 5, 2020
@sn260591
Copy link

sn260591 commented Jun 7, 2020

@chocolateboy I apologize for misleading you. It seems that your method is the only working one, although it has a strange implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants