Skip to content
This repository

"@grant none" (or auto-detected none) combined with JQuery == Bad News #1614

Closed
arantius opened this Issue August 28, 2012 · 24 comments

8 participants

arantius Johan Sundström Brock tomchen dindog Jay R. Ventero DavidJCobb
arantius
Collaborator

https://groups.google.com/d/topic/greasemonkey-users/ne90b8Lea_Q/discussion

I saw a lot of complaints on the addons page about how scripts that @require
jquery weren't working. I started encountering the same issue when I got
upgraded last night. At first, I thought that it was RES (Reddit Enhancement
Suite) that was causing the problems, but it turns out that it was a small
script that I had written that was also running. This is the problem in short:

When using @grant none, or auto-detect with no GM_* functions, the @required
version of JQuery is stomping on the one that's loaded in the "unsafeWindow"
namespace (because they're now the same). By adding a simple @grant GM_log,
all of the problems disappear because JQuery is being loaded in the sandboxed
window.

So, either all scripts need to add a @grant line to continue working, or
the new auto-detection needs to be reconsidered. IMO, continuing with the old
behavior and only using the unsandboxed behavior when @grant none is
specified seems to be the best idea for backwards compatibility.

Which disagrees with my findings in #1557 but needs to be checked.

arantius
Collaborator

Script: https://gist.github.com/3497870
Page: http://jsfiddle.net/Gc8HN/4/

Produces:

Page1: I expect jquery 1.8.0, I have: 1.6
Page2: I expect jquery 1.8.0, I have: 1.6
Script: I expect jquery 1.6.0, I have: 1.6

Confirmed. This is the opposite direction as the test in #1557; jQuery effectlive does "window.$ = ..." -- which now directly assigns to the content scope. Which has great potential to break the page.

Johan Sundström
Collaborator

I think the best direction to go on this is requiring scriptwrights that @require jQuery to change to use jQuery.noConflict() rather than forcing the use of @grant none to opt out of backwards-compat baggage.

I think the proper change needed for a jQuery-using script is to wrap itself in

`jQuery.noConflict(true)(function() {
  // code here, possibly with a leading "var $ = jQuery;" too
});

http://stackoverflow.com/questions/1566595/can-i-use-multiple-versions-of-jquery-on-the-same-page seems to have better docs than http://api.jquery.com/jQuery.noConflict/ on the matter. I haven't done any testing, and admittedly it may be a little messier to achieve this wrapping for multi-file scripts, where multiple parts are referencing $ or jQuery.

A heuristic approach at helping people auto-invoke backwards compat for jQuery usage might be to recognize many of the popular jQuery CDN urls and append this hack right after the jQuery script body, and at the end of all files.

Another part of the reason I'm weary about requiring @grant none for a non-sandboxed user script on behalf of jQuery is that the XPCNativeWrapped jQuery you get is fundamentally broken for all sorts of advanced usage, on behalf of it not being written defensively against such a harsh environment. Greasemonkey users have just been limited to the quite arbitrary (and completely undocumented) subset of jQuery functionality which still mostly works.

arantius
Collaborator

Indeed. If I change the test script

before: https://gist.github.com/3497870/5a305d3f9ef17f7ca042bc0cdff89a3d7745b685
now: https://gist.github.com/3497870/dbf3a27cb46eb80bb8bfbcfc16b67281943e8eb8

Then the test page outputs:

Page1: I expect jquery 1.8.0, I have: 1.8.0
Page2: I expect jquery 1.8.0, I have: 1.8.0
Script: I expect jquery 1.6.0, I have: 1.6

As desired.

A heuristic approach at helping people auto-invoke backwards compat for jQuery
usage might be to recognize many of the popular jQuery CDN urls and append
this hack right after the jQuery script body, and at the end of all files.

If you want scripts to otherwise continue to work unmodified, you probably need to do something like:

var my_jquery = jQuery;
jQuery.noConflict(true);
var $ = my_jquery, jQuery = my_jquery;

So the $ and jQuery references get set to the script's version (var = at the top level puts it in the script scope, not window scope).

Any comments as to whether documenting this is "good enough"? And we expect all authors to fix jquery using scripts?

Another part of the reason I'm weary about requiring @grant none for a non-sandboxed user script on behalf of jQuery is

I can't quite parse this. Did you mean that you don't want to require granting something (and thus running in the legacy security sandbox), to use jQuery without breaking the page?

arantius
Collaborator

dmittedly it may be a little messier to achieve this wrapping for multi-file scripts, where multiple parts are referencing $ or jQuery.

Ah, indeed. If a @require depends on jquery in line, things could get dicey.

Brock

The correct thing to do is to restore the default sandbox behavior. Remove the sandbox ONLY if @grant none is specified.
Better yet, the sandbox should be controlled by its own flag, not by crazy side effects of another meta setting!

Even if I don't use the Greasemonkey API, the scope separation is still an excellent thing, its true power is not just to guard against a slight privilege hijacking, but to avoid a whole plethora of side-effects and dependencies.

Fixing the @grant problem, breaks no existing scripts, as all existing scripts were developed with the sandbox in play.

However, this new willy-nilly removal of the sandbox, on existing scripts!!! has broken several of my scripts and those of many others.
It also busts scripts that some of us have running on pages with javascript disabled.

This new behavior, no doubt, busts thousands of legacy scripts and, worse, it does it in a way that is not always obvious. Whereas, changing the sandbox to be on unless explicitly shut off, breaks zero scripts, and keeps developers aware of the possible ramifications.

Please release an emergency update that restores the sandbox UNLESS a meta directive explicitly shuts it off.

arantius
Collaborator

Continued research, with slightly tricky syntax:

Script: https://gist.github.com/3497870/bc17d3acbc8f72fb2dad66b33a6c8a0e97f048ae
Page: http://jsfiddle.net/Gc8HN/10/

Produces, in the console:

page, at load time: function
page, at load time: 1.8.0
script, pre: function
script, pre: 1.6.4
script, post: function
script, post: 1.6.4
page, later: function
page, later: 1.8.0

I.E. there's your workaround. Keeping the right names. Would work as a @require between jQuery and anything else, if necessary. And it's the standard supported way to use (another verison of) jQuery in a page where it already exists!

So, specifically, the script does this.$ = this.jQuery = jQuery.noConflict(true);. Because the script is @grant none, this is the global scope for the script (not the page). And then this line saves the script-jquery reference in the name you're likely to want it without breaking; more obvious syntax (var jQuery = ...) fails in mysterious ways.

@BrockA: Yes, this is breaking backwards compatibility. I probably wouldn't have released if I knew this was happening. I need to think a bit about how to proceed, now that it's out there. It is an (admittedly painful) path towards the exact goal I was aiming for with the changes in 1.0. And ironically has better jQuery support -- given that one line workaround to avoid page conflicts. Maybe a release to heuristically inject this, near term, until script authors can do so themselves.

Brock

This issue is not just about jQuery, I'm not sure why it's getting sidetracked in that direction.

This is about good code practices and also about not busting legacy scripts for zero gain!

The jQuery argument seems like a red herring to me. I use jQuery extensively -- including .data() -- and have had no major issues (at least for the last many GM releases) until now. Furthermore, for existing scripts, whatever issues there may be have been worked around.

Sure, switching off the sandbox can simplify some tasks (and make GM scripts even less compatible with all other browsers), but it should be a conscious, deliberate, choice by the script writer.

Doing it right should also be much simpler to code and test.

Is @sandbox off in the meta-data? Then don't use the sandbox (and throw errors if the API is attempted). Otherwise keep it in place!
Simple, no side effects, Zero busted scripts, zero people wondering what broke in Greasemonkey.

arantius
Collaborator

So I've downloaded (with permission from the site owner) the active version of every script on userscripts.org. I hope this is a representative sample of all scripts out there, so we can use it to understand script author's behaviors at large. For a very rough first approximation:

$ ls | wc -l
82084
$ grep -irl jquery . | wc -l
10620
$ grep -irl '@require.*jquery' . | wc -l
4529
$ grep -l GM_ `grep -irl '@require.*jquery' .` | wc -l
1985

So about one in eight scripts mentions jquery, but only about one in twenty @require their own version. Of those, about half do (probably) use some sort of GM_* API. Leaving the other about half (2544 scripts, 3.1% of total) to probably hit this bug.

I was curious about those ~6k of scripts that reference jQuery but don't require their own version do, it seems:

$ grep -irl 'unsafewindow.*jquery' . | wc -l
2508

Many but not all of them are just using the content scope's copy.

Johan Sundström
Collaborator

I can't quite parse this. Did you mean that you don't want to require granting something (and thus running in the legacy security sandbox), to use jQuery without breaking the page?

I meant I'd prefer a solution where the sandbox is opt-in ("please let me run sandboxed from the page and suffer all the XPCNativeWrapper pitfalls; I know what I am asking for!") rather than opt-out. My data about the specifics of jQuery degradation under the sandbox is not fresh; this may have improved in recent years.

arantius
Collaborator

I meant I'd prefer a solution where the sandbox is opt-in ...

Ok. Me too. That's the point of the changes that have happened.

This, of course, is not how things were pre-1.0. @johan: How do you feel about the backwards-incompatibility-break as it stands now? I'm starting to lean towards it being OK IMO.

P.S. Above I said "Leaving the other about half (2544 scripts, 3.1% of total) to probably hit this bug.". Where it's actually some subset of those scripts that also run on a page that has its own jQuery, and where the version the script requires breaks some part of the page's scripts because it's a different version than the page expects to have. Super hard to measure, but definitely some amount smaller.

Brock

Crucial points:

  • This sandbox change busts much more than just jQuery. ANY library is vulnerable. As are random conflicts with the page's javascript.
  • The failures may not be immediately obvious. So this should yield support calls for months to come (I've already had several).

"Opt-in":

  • Busts at least 2500 scripts (probably 10 to 20 times that).
  • Requires hundreds of script devs to scramble to work around this bug. I've had to mod 79 scripts so far.
  • Sets up side effects and cryptic behavior which will bite asses for years to come.

"Opt-out":

  • Busts no existing scripts.
  • Requires no script rewrites or hacks.
  • Can be simple, explicit, and obvious -- relating meta settings to behavior.
  • Is no-doubt more straightforward to implement in Greasemonkey's source code.

Why is this choice not obvious?

Johan Sundström
Collaborator

This, of course, is not how things were pre-1.0. @johan: How do you feel about the backwards-incompatibility-break as it stands now? I'm starting to lean towards it being OK IMO.

Yep, same here, by this reasoning:

As I understand the present state, old-style scripts that @grant themselves any right at all (say @grant GM_log) work as they used to: the sandbox gets invoked, and things remain as they always did. This solution should work for scripts that don't need GM_log too, but do need the sandbox. (We could add some @sandbox on directive, but it feels like unnecessary baggage.)

Scripts that don't have a @grant but that happen to use any GM_* APIs should work, too, on behalf of auto-detect code kicking in.

So the breakage in the wild is limited to sandbox-dependent scripts in the wild that nobody cares to update with a // @grant GM_log line (or equivalent, if we want to prettify it). We should probably note as much in some easy-to-find place to help bring the knowledge out, and maybe do a little more reach-out to explain why we do this, as @BrockA might not be alone in believing that this 1.0-motivating change is of benefit to nobody and adds more incompatibility with other browsers than it removes.

Johan Sundström
Collaborator

@BrockA Willingly, intentionally or not, you are omitting the reason we are moving away from the sandbox: it breaks all sorts of web development fundamentals that forces web developers to learn the craft of avoiding sandbox pitfalls in an XPCNativeWrappers world you can't even do live testing with in Firebug which shows up as, breakages that no other web browser suffers.

Knowledgeable people like yourself that want that sandbox won't be prevented from manually enabling it and keep user scripting like it's 1999 – and if you feel it important enough that the keyword to do so be // @sandbox on (or something along those lines) rather than // @grant something we're certainly willing to consider that.

The difficulties of becoming aware of and learning to abide by the limitations of the sandbox (which most javascript libraries in general don't – and thus partially or completely break) are what prompted this breaking 1.0 change of not making it the default environment.

The pains of this change should be somewhat mitigated by the nowadays-existing script update feature, so the extra line needed to kick broken scripts back into the sandbox at least shouldn't be necessary for users to make unaidedly.

Brock

Okay, I guess I can understand (but not agree with) some of the motivation to switch to "Opt in". It will be (and is) a headache for the existing user-base, but might make it easier for newbies.

But consider: we can avoid ALL the compatibility problems, and still help the newbies by:

  1. Use "Opt out" behavior.
  2. Change the default "New User Script" boilerplate to include @sandbox off -- just like it currently includes @version 1, etc.
  3. Edit the newbie example code to show @sandbox off.

And please use the @sandbox switch. It will make scripts a little more self-documenting.

tomchen

https://groups.google.com/forum/#!topic/greasemonkey-users/mow6fpWmIvE
Similar discussion on Google Group

I'll second the @sandbox switch idea.

By the way, Scriptish, Tampermonkey and native Google Chrome, they all use a sandbox. But Opera and IE7Pro do otherwise.

For my scripts I wrote a function that is compatible with these browsers and extensions to get the real window:

function getUnsafeWindow() {
    if (typeof(this.unsafeWindow) != "undefined") {//Greasemonkey, Scriptish, Tampermonkey, etc.
        return this.unsafeWindow;
    } else if (typeof(unsafeWindow) != "undefined" && this === window && unsafeWindow === window) {//Google Chrome natively
        var node = document.createElement("div");
        node.setAttribute("onclick", "return window;");
        return node.onclick();
    } else {//Opera, IE7Pro, etc.
        return window;
    }
}
var myUnsafeWindow = getUnsafeWindow();

It works fine with all these extensions, incl. Greasemonkey <1.0 or 1.0.

It does a little hack with native Google Chrome, because for the native Google Chrome, unsafeWindow exists but is still something like a sandboxed window rather than the 'real' one. So, I'm wondering why Google Chrome insists on doing such sandbox things, while Greasemonkey 1.0 somehow cancels the sandbox for most scripts.

dindog

Would someone tell me what exactly users or script's author gain with these @grant things? It looks intelligence at first glance, but when you come to think of quite a portion of script is no longer writing exclusive for GM ( Opera, Webkit-alike, Scriptish, Chrome ... ), if that is no much border, author tend to make them script works in universal.

I doubt there will be much script's writer will explicitly declare @grant none, if any. Because a "standard" grant none script will break support of Tampermonkey and Scriptish, we are talking hundred of thousand users here. According to official stats, Tampermonkey has over 200k user, Scriptish has over 100k, and there are more using Chrome or Webkit native script-loaders...

And the @grant things to me, seems a default unwrap + GM_api limitation. If the limitation is made for user tell what is the potential security problem can raise by a given script, again, I doubt that. They pick script by reputation, then luck, not by telling which GM_api the script use.

Jay R.

I still agree with @BrockA - this problem isn't jQuery specific, it's just one of the easiest pitfalls to hit. Getting caught up in "jQuery vs. other" is a red herring. Removing the sandbox breaks backwards compatibility across the board and introduces potential conflicts in every script, regardless of jQuery usage, unless the script author is intimately familiar with every variable name used in every script on the original site.

arantius
Collaborator

Because a "standard" grant none script will break support of Tampermonkey and Scriptish ...

In what way? If it's a problem, it would be a really small change for Scriptish to port this feature in (and the only problem I can imagine is the same sort of breakage to standard browser features that we're explicitly looking to put an end to), and I don't know of anything that would make specifying @grant none make a script stop working in Tampermonkey.

... and there are more using Chrome or Webkit native script-loaders.

That's the whole point! Use the @grant none mode in Greasemonkey, and the native implementations in those browsers, and everything just works. Use custom APIs and they only work where they're implemented, and (in order for GM to support them) lots of standard browser features break.

Finally: More testing.
Two test scripts: https://gist.github.com/3644903
Test page: http://jsbin.com/abemus/2

At the test page, I get:

Grant-less with GM 1.0:

In-line time:
bare_val: undefined
var_val: undefined
window_val: undefined
Script sees content_val: content_val
Script sees window.content_val: content_val
Script sees unsafeWindow.content_val: content_val
Load time:
bare_val: undefined
var_val: undefined
window_val: grant-less window

Grant-ed with GM 1.0:

In-line time:
bare_val: undefined
var_val: undefined
window_val: undefined
Script sees content_val: undefined
Script sees window.content_val: undefined
Script sees unsafeWindow.content_val: content_val
Load time:
bare_val: undefined
var_val: undefined
window_val: undefined

Either with GM 0.9: same as grant-ed with 1.0.

Either with Chrome:

In-line time:
bare_val: undefined
var_val: undefined
window_val: undefined
Script sees content_val: undefined
Script sees window.content_val: undefined
Script sees unsafeWindow.content_val: undefined
Load time:
bare_val: undefined
var_val: undefined
window_val: undefined

In other words, in grant-less 1.0 you can read content scope values (something people want to do and is very hard in 0.9) and you can write to the content scope: but only with an explicit window.foo reference*; regular variables do not leak to the content scope. You won't get this in other user script engines, but you (script authors) don't have to use these features, they're just available in GM now.

* And nobody does this; there's no reason to. Over all scripts on userscripts.org (see my recent wiki post about API usage; I grabbed every script for purposes like that and this) grep -rl 'window\.[^;]+=' . | wc -l gives zero results. This only happens in a case like requiring jQuery. For which there is an easy/standard workaround.

This is explicitly the design goal of 1.0. If you'd still like to discuss the design, please do so at the greasemonkey-dev list where everyone subscribed can participate, not just the ~half dozen people that happen to know this ticket exists. Issue closed.

arantius arantius closed this September 05, 2012
Ventero

I know this is a bit late, but I just wanted to point out a small mistake in the way you checked window.foo usage in scripts: grep -rl 'window\.[^;]+=' greps for something like window.a+=, since, as the man page states:

In basic regular expressions the meta-characters ?, +, {, |, (, and ) lose their special meaning; instead use the backslashed versions \?, \+, \{, \|, \(, and \).

So the correct way to check would've been grep -rl 'window\.[^;]\+='.

arantius
Collaborator

I'm pretty sure it's correct. What is being searched for is the string "window" followed by a literal dot, followed by one or more characters that are not a semicolon, followed by an equal sign. I.E. an assignment to "window.something". I do not want to search for a literal plus sign, it is being used as a repetition operator.

Ventero

I do not want to search for a literal plus sign, it is being used as a repetition operator.

As the man page says, in basic regular expressions (grep without -E), the + character (among others) loses the special meaning of being a repetition operator, and you have to use \+ instead. This can easily be verified:
echo "window.foo = 1;" | grep -l 'window\.[^;]+=' has no output, whereas echo "window.foo = 1;" | grep -l 'window\.[^;]\+=' shows (standard input).

arantius
Collaborator

Whoops sorry! I didn't read what you said properly. Indeed. Good catch!

Grr Unix commands and their all having separate incompatible implementations of regular expressions! In that case the results are very different. (And I should have been suspicious of the zero, in the first place.) First, updated to \b=\b to find single-equals assignment, not double-equals testing. Then I get:

$ grep -rl 'window.[^;]+[^!]\b=\b' . | wc -l
5940
$ grep -rl 'window.location[^;]*\b=\b' . | wc -l
2845

About 6k scripts assign to some property of window. About 3k do an assignment to window.location (so somewhere between 3k and 6k actually might get caught, depending on how many do both). If they assign to a name that the content scope's scripts depend on then they have the potential to break something.

DavidJCobb

Probably too late to change anything about this, but why not just modify @grant so that it doesn't auto-detect as none and strip out the sandbox if there are any @require scripts present? Requiring entire external script libraries is a bizarre and unlikely thing to do in a script that's supposed to run in the content scope of a page that likely already has its own scripting.

I can understand the motives behind making the sandbox opt-in. Making @require trigger a sandbox unless @grant none is explicitly specified seems like the ideal middle-ground between unboxing new scripts and maintaining backward compatibility with old ones, though. :\

Johan Sundström
Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.