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

Main world User Script shared params #284

Open
jonathanKingston opened this issue Sep 29, 2022 · 14 comments
Open

Main world User Script shared params #284

jonathanKingston opened this issue Sep 29, 2022 · 14 comments

Comments

@jonathanKingston
Copy link

jonathanKingston commented Sep 29, 2022

Passing state/parameters to content scripts that are injected into the main world currently requires message passing that is visible to the page and also delays the functionality that the script can provide.

A use case DuckDuckGo need this for is:

  • Overriding/removing page prototypes (for privacy reasons) but allowing breakage to be remotely / user configured to disable such protections.

chrome.scripting.globalParams was added to solve similar use-cases however it's not ever exposed to the main world scripts. I'd instead propose a way to allow these variables to be passed into the script (perhaps cloned so that modification isn't possible) some avenues to inject these would be:

  • document.currentScript.params
  • import.meta.params
  • Inject in a script local variable prior to execution of the script. (I believe all JS engines have the ability to easily register script local properties programatically)

These shouldn't be exposed on globalThis/window as the page should have no visibility of these parameters; most importantly the page should have no ability to read the contents.

Moved out from: #279 (comment)

  • This would be much more appealing than asking for another permission / user consent etc.
  • Dynamic scripts pose a large security risk; if this is implemented correctly it has limited security issues.

cc @dotproto see also: https://bugs.chromium.org/p/chromium/issues/detail?id=1054624#c47

@ghostwords
Copy link

ghostwords commented Sep 29, 2022

Could this also be used to facilitate secure document_start main-world-to-content-script communication (#78 (comment), https://bugs.chromium.org/p/chromium/issues/detail?id=1054624#c61)?

cc @hackademix

@ghostwords
Copy link

Previously: #103

@jonathanKingston
Copy link
Author

Could this also be used to facilitate secure document_start main-world-to-content-script communication (#78 (comment), https://bugs.chromium.org/p/chromium/issues/detail?id=1054624#c61)?

Yes 👍🏻 kzar is actually on my team so was documenting our workaround.

@jonathanKingston
Copy link
Author

jonathanKingston commented Sep 29, 2022

In the latest meeting I think there was some confusion with this being for executeScript this would be additionally required for chrome.scripting.registerContentScripts executed prior to the page load.

One suggestion was to use func; perhaps this could be resolved with allowing func to registerContentScripts such that the following is possible:

let someSharedGlobal = 0
browser.alarms.onAlarm.addListener(() => {
  someSharedGlobal++
});

chrome.scripting.registerContentScripts([{
  js: ['script.js'],
  matches: ['<all_urls>'],
  runAt: 'document_start',
  world: 'MAIN',
  func: () => {
    let myVar = someSharedGlobal
  }
}])
// script.js
console.log(`alarm hit ${myVar} times`)

@ghostwords
Copy link

ghostwords commented Sep 29, 2022

Thank you for clarifying we need this ability for declaratively declared (registerContentScripts) content scripts.

Rather than func, we could provide a map of JSON-serializable variables, to avoid raising dynamic code concerns. Then the run at document_start, MAIN world script.js would ingest and delete those variables from page global scope, preventing the page from knowing the secret communication key or whatever. Would that work?

@jonathanKingston
Copy link
Author

Then the run at document_start, MAIN world script.js would ingest and delete those variables from page global scope, preventing the page from knowing the secret communication key or whatever. Would that work?

This would solve some of it; the other problem being the state is dynamic and so currently the ISOLATED script still has to message pass to the background/worker context which creates a race condition.

Essentially what we currently have is:

// background.js
let someSharedGlobal = 0
browser.alarms.onAlarm.addListener(() => {
    someSharedGlobal++
})

chrome.runtime.onMessage.addListener((message) => {
    return someSharedGlobal
})

chrome.scripting.registerContentScripts([
    {
        id: '1-isolated',
        js: ['isolated.js'],
        allFrames: true,
        matches: ['<all_urls>'],
        runAt: 'document_start',
        world: 'ISOLATED'
    },
    {
        id: '2-main',
        js: ['script.js'],
        allFrames: true,
        matches: ['<all_urls>'],
        runAt: 'document_start',
        world: 'MAIN'
    }
])
// isolated.js
function getSecret () {
    return new Promise(resolve => {
        window.addEventListener('secret', event => {
            event.stopImmediatePropagation()
            resolve(event.detail)
        }, { once: true })
    })
}

const secret = await getSecret()

chrome.runtime.sendMessage({
    messageType: 'registeredContentScript',
}, someSharedGlobal => {
    // Init script.js with the someSharedGlobal.
    window.dispatchEvent(new CustomEvent(secret, {
        detail: {
            type: 'register',
            someSharedGlobal
        }
    }))
})
// script.js
const secret = window.crypto.randomUUID()

window.addEventListener(secret, ({ detail: message }) => {
    if (!message) return

    if (message.type === 'register') {
        console.log(`alarm hit ${message.someSharedGlobal} times`)
    }
})

window.dispatchEvent(new CustomEvent('secret', {
    detail: secret
}))

In the example code we're using DOM events; they perhaps could be static functions and the proposed changes of serializing globals would simplify; however there would still be a delay in getting the data from the message handler in the background.js

It would be fine if your suggestion worked like:

let someSharedGlobal = 0
browser.alarms.onAlarm.addListener(() => {
  someSharedGlobal++
});

chrome.scripting.registerContentScripts([{
  js: ['script.js'],
  matches: ['<all_urls>'],
  runAt: 'document_start',
  world: 'MAIN',
  args: [
    'someSharedGlobal'
  ]
}])
const myVar = globalThis.someSharedGlobal
delete globalThis.someSharedGlobal
// script.js
console.log(`alarm hit ${myVar} times`)

However I'd prefer these vars not to be global at all to reduce the need for any clear up mistakes:

  • JS engines work within execution contexts and registerContentScripts could declare it script locally as if the code author had written: const someSharedGlobal = .... into the script.js
  • Both document.currentScript.params and import.meta.params illustrate scripts aren't just thrown into window with no differing state.

@theseanl
Copy link

theseanl commented Sep 29, 2022

Any API design that depends on an ephemeral MV3 service worker's top-level lexical scope is unlikely to be implemented.
Requiring API users to provide an immutable argument at registration time would still allow a dynamism as described in the previous comment.

browser.alarms.onAlarm.addListener(async () => {
  let someSharedGlobal = await browser.storage.local.get('someSharedGlobal') | 0;
  someSharedGlobal++;
  await browser.scripting.registerContentScriptArgument({
    id: 1,
    args: { someSharedGlobal }
  });
  await browser.storage.local.set({ someSharedGlobal })
});

chrome.scripting.registerContentScripts([{
  id: 1, 
  js: ['script.js'],
  /* ... */
}])

https://bugs.chromium.org/p/chromium/issues/detail?id=1054624#c56 is a proposal that is in line with requiring static arguments.

@jonathanKingston
Copy link
Author

Any API design that depends on an ephemeral MV3 service worker's top-level lexical scope is unlikely to be implemented.

Right I was omitting state storage for brevity (the code sample was already big enough 😆 ); we're not relying on that instead we're using session and local browser.storage.

IMO, the above example would be better implemented via message passing, for declarative content scripts' internal structure may not be supposed to be updated as often as a user clicks on a toolbar icon.

  • This creates races we're trying to avoid.
  • Additionally I'm not expecting the data to change once the script has executed; doing this could be achieved with a structure like SharedArrayBuffer but it's currently not a requirement of ours; I think message passing could be used here if a requirement anyway.
  • State updating out of the content scripts through assignment also isn't a need as we can message pass back.

@theseanl
Copy link

It's not just about toy example in your comment. registerContentScripts is different from executeScript, and having func in the former, as you've proposed in #284 (comment) would make it depend on certain lexical scope of an ephemeral worker, which makes it very unlikely to be implemented.

I've already removed that part of the comment, because it seemed tangential to the main issue.

@jonathanKingston
Copy link
Author

jonathanKingston commented Sep 29, 2022

Ah I understand now, on worker startup there's likely a race between storage fetch and the func argument execution; I can see why this was omitted from the API now.

  • This func toy proposal came off the back of the meeting call from another persons suggestion

I agree the arguments API proposal resolves this, optionally just populating 'arguments' with chrome.scripting.globalParams or similar would work similar to comment 58

@tophf
Copy link

tophf commented Oct 11, 2022

Then the run at document_start, MAIN world script.js would ingest and delete those variables from page global scope, preventing the page from knowing the secret communication key or whatever. Would that work?

https://bugs.chromium.org/p/chromium/issues/detail?id=1261964 in Chrome (same for Firefox and maybe Safari as well) shows that a page can spoof the environment in a same-origin iframe/page prior to document_start injection point e.g. it can install setters on window to intercept a known global variable.

I wonder if there's an internal JS engine trick that can limit the visibility of a global variable to just the injected code? Or somehow set the global without triggering setters , just overwrite the descriptor regardless of its configurable:false mode.

A simpler solution might be for the browser to auto-wrap the injected code in an IIFE like (function(...args) { /* script code here */ })() when args are configured via chrome.scripting.setGlobalParams.

@xeenon
Copy link
Collaborator

xeenon commented Oct 14, 2022

Currently, without a separate isolated world, all globals are visible to any main world scripts. That's one of the big things isolated world gives you.

@jonathanKingston
Copy link
Author

To be clear we explicitly don't want it as a global at all.

Alternatively the ability to reach into the window like you can in Firefox xrays would equally work for our use cases.

@jonathanKingston
Copy link
Author

Has anyone else from the browser teams looked at this some more?

  • There's a few valid approaches here that would work from: script defined local variables or allowing a func to be passed in script setup.

The registerContentScriptArgument proposal seems the simplest and cleanest.

Service worker:

const pies = true
await browser.scripting.registerContentScriptArgument({
  id: 1,
  args: { pies }
});
chrome.scripting.registerContentScripts([{
  id: 1, 
  js: ['script.js'],
}])

Script:

if (args.pies) {
  console.log('hey I like pies')
}

Executes as:

((args) => {
  if (args.pies) {
    console.log('hey I like pies')
  }
})({pies: true})

After looking at @EmiliaPaz's #279 (comment) proposal, it's clear that isn't the direction of this request. There's no desire for arbitrary script execution here, we're just wanting to pass variables into the main world script faster than the page can execute.

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

No branches or pull requests

7 participants