userscripts icon indicating copy to clipboard operation
userscripts copied to clipboard

Only change `@inject-into` if the `@grant` value is valid?

Open quoid opened this issue 3 years ago • 5 comments

Take the following userscript for example:

// ==UserScript==
// @name        NewScript-fg00n069
// @description This is your new file, start writing code
// @match          *://*/*
// @grant            foo
// @inject-into   auto
// ==/UserScript==

console.log("bar")

The @grant value is invalid, but because it has an @grant, the @inject-into will be set to content. It might make sense to check *if the @grant value is validbefore changing the@inject-into`.

related #265

quoid avatar Aug 27 '22 02:08 quoid

Also only use grant in GM.info.script object - currently the keys grant and grants both appear.

quoid avatar Aug 27 '22 03:08 quoid

Maybe restricting to content script context only when granting supported high-level APIs, it sounds like that would work.

After all, except for the supported APIs, It's just an undefined function name, it doesn't have any effect, we don't have to deal with it.

ACTCD avatar Aug 27 '22 04:08 ACTCD

Personally, I think a much better solution would be to change the context and display warning message that the requested @grant was not provided. From my experience, this type of logic where "A changes to B, but only if C is not D" usually leads to cryptic errors that are hard to debug.

Also semantically, there's a difference between @grant unsafeWindow (valid but not possible in Userscripts) and @grant foo (does not exist anywhere). Another case -- what would happen if valid and invalid @grants are requested?

scholtzm avatar Aug 27 '22 21:08 scholtzm

@scholtzm

Personally, I think a much better solution would be to change the context and display warning message that the requested @grant was not provided.

Are you suggesting we expand the current warning to include which @grants caused the context change?

Currently, the warning is a bit ambiguous, example:

Somefile.js @inject-into value changed due to @grant values

From my experience, this type of logic where "A changes to B, but only if C is not D" usually leads to cryptic errors that are hard to debug.

This is a valid point, but I'd want to reduce the number of compatibility issues, as noted in #305 - I think it could be a very simple change (more on this below).

Another case -- what would happen if valid and invalid @grants are requested?

This is somewhat related to the quote above as well (ie. reducing the complexity), but the implementation I was thinking of was quite simple.

The current implementation in the beta build is:

const grants = userscript.scriptObject.grants; // <---------
const injectInto = userscript.scriptObject["inject-into"];
// prepare the api string
let api = `const US_uid = "${uid}";\nconst US_filename = "${filename}";`;
// all scripts get access to US_info / GM./GM_info, prepare that object
const scriptData = {
    "script": userscript.scriptObject,
    "scriptHandler": scriptHandler,
    "scriptHandlerVersion": scriptHandlerVersion,
    "scriptMetaStr": userscript.scriptMetaStr
};
api += `\nconst US_info = ${JSON.stringify(scriptData)}`;
api += "\nconst GM_info = US_info;";
gmMethods.push("info: US_info");
// if @grant explicitly set to none, empty grants array
if (grants.includes("none")) {
    grants.length = 0;
}
// @grant exist for page scoped userscript
if (grants.length && injectInto === "page") {
    // remove grants
    grants.length = 0;
    // provide warning for content script
    userscript.warning = `${filename} @grant values changed due to @inject-into value`;
}
// @grant exist for auto scoped userscript
if (grants.length && injectInto === "auto") {
    // change scope
    userscript.scriptObject["inject-into"] = "content";
    // provide warning for content script
    userscript.warning = `${filename} @inject-into value changed due to @grant values`;
}
// loop through each @grant for the userscript, add methods as needed
for (let j = 0; j < grants.length; j++) {
    // add methods here
}

As you can see above, it's a really basic implementation. We get the @grant values and check if any are present. If so, @grant or @inject-into is manipulated.

To make the change suggested in this issue, we could simply filter out invalid grant values from const grants = userscript.scriptObject.grants; from a set/array of valid grant values, either in this background.js file or on the swift side before the grants are passed to background.js.

That way, when the check is run to determine whether or not to change @grant or @inject-into only valid grant values with be present.

quoid avatar Aug 27 '22 22:08 quoid

This is a valid point, but I'd want to reduce the number of compatibility issues, as noted in https://github.com/quoid/userscripts/issues/305 - I think it could be a very simple change (more on this below).

Basically, what I was hinting at is a scenario like this:

  • script author uses unsupported @grant value or just makes a typo
  • scope is changed
  • completely different error occurs in the script, related to switched context
  • no warning about unsupported @grant is shown in the console

Needless to say, if compatibility is the main concern, the proposed solution makes sense.

scholtzm avatar Aug 28 '22 06:08 scholtzm

done in https://github.com/quoid/userscripts/commit/9c7066f70482cb432247b883a10a7b68d8dad3c6

quoid avatar Oct 11 '22 14:10 quoid