Warn on `//@key` without space

[tophf]

Although the GM specification doesn't say explicitly the space is required, but greasyfork.org, openuserjs.org, ViolentMonkey, GreaseMonkey, and Chromium require it. Firemonkey and Tampermonkey don't.

Expected Behavior

Script A is not installed/saved. Script B doesn't have jquery external loaded.

Script

A

// ==UserScript==
//@name MyName
// ==/UserScript==

B

// ==UserScript==
// @name MyName
//@require https://ajax.googleapis.com/ajax/libs/jquery/2.1.3/jquery.min.js
// ==/UserScript==

[derjanb]

This would potentially break quite some scripts and I would like to avoid this as long as it is not really necessary.

Although the GM specification doesn't say explicitly the space is required, but greasyfork.org, openuserjs.org, ViolentMonkey, GreaseMonkey, and Chromium require it.

Examples: https://greasyfork.org/en/scripts/425068-sort-number-for-greasy-fork-scripts/code https://openuserjs.org/scripts/EvilSpark/LazyScroll/source https://github.com/froksen/aUserscripts/blob/main/calender-exportToOutlook/calendar-exportToOutlook.user.js

[tophf]

Out of ~100,000 scripts that exist on the internet I see ~50 on greasyfork that have //@require without a space so there's no real compatibility problem in that direction. The real problem is that while the majority of userscript-related services and engines require this space, Tampermonkey allows the authors to produce an invalid script.

[tophf]

What about showing a warning in the editor, for starters? It will help gradually reduce the near 0% of such scripts to real 0%.

[derjanb]

The real problem is that while the majority of userscript-related services and engines require this space, Tampermonkey allows the authors to produce an invalid script.

This script at Greasyfork was updated a month ago, Github and Github Gist by nature doesn't require it neither. I've found scripts at openuserjs.org without the space as well. In addition, what exactly the majority is can be determined using various parameters. 🤓

What about showing a warning in the editor, for starters?

I guess this could be implemented at https://github.com/Yash-Singh1/eslint-plugin-userscripts. which is used by Tampermonkey's editor to lint the header.

[tophf]

Indeed, the exact numbers are hard or impossible to get but the order of magnitude seems to be reliably deducible from the fact that only ~50 scripts don't have a space on greasyfork, i.e. practically 0%.

[tophf]

To clarify, greasyfork and OUJS simply ignore such keys, they aren't recognized as proper metadata. So by "requiring a space" I meant they require a space in order for a key to be recognized.

[JasonBarnabe]

Considering this syntax is not widely supported, I'm going to be adding validation on Greasy Fork to prevent it. Not sure if that affects the decision for Tampermonkey.

[Martii]

This discussion again ay... they say history repeats itself... I'm beginning to agree with that sentiment.

@tophf and everyone else

Although the GM specification doesn't say explicitly the space is required...

Yes __it does__ (snippet of what Aaron, @arantius , and I wrote quoted):

The metadata block must follow the format:

// ==UserScript==
// @key value
// ==/UserScript==

... there is a space there and a "must". This is clear information. I also swear this conversation has been had at some point in the past but it evades me from linking atm. Dejavu isn't around any more and I wish it had been more backed up before its demise.

but greasyfork.org, openuserjs.org, ViolentMonkey, GreaseMonkey, and Chromium require it.

So does GM Port from last check.

@derjanb

This would potentially break quite some scripts and I would like to avoid this as long as it is not really necessary.

If someone is using a flaw in TMs implementation of GM then those scripts are already broken for not following a standard specification. I'll be retesting TM in a little bit and if it is executing/loading resources in plain comments, which what //@key is on a single line, then that add-on may be submitted for removal from their hosting at best.

Github and Github Gist by nature doesn't require it neither.

GH and gists aren't the defining nature of the UserScript metadata block... GM proper will always be with some enhancements from other forks. Why would you even type this? This is also a reason why OUJS has its own block.

@tophf

What about showing a warning in the editor, for starters?

OUJS may add something like that. However as I mentioned //@key should be treated as a plain comment by all .user.js engines.

To clarify, greasyfork and OUJS simply ignore such keys, they aren't recognized as proper metadata.

That is correct because we aren't a .user.js engine... however we can definitely prevent upload... However as I mentioned //@key should be treated as a plain comment by all .user.js engines.

@JasonBarnabe

Considering this syntax is not widely supported, I'm going to be adding validation on Greasy Fork to prevent it.

While that works as a safety measure it also prevents "twiddling" on a published script. Typically in the past decade or so I've personally used //@key when I do NOT want a .user.js engine to load a resource (usoCheckup is a prime example from back in the days with USO). If OUJS has to I'll add a restriction as well but that will be unfortunate for those who have comments describing their keys/other use case as well.

... In the meantime I have to test current TM for this VULNERABILITY and if exists on my end TM might be kicked out of places until it follows the the written standard.

Please remember that I am one of the original documenters and I've had extensive communication/history with GM. If there is a flaw in TM it needs to be fixed.

OUJS Admin/OUJS CoOwner/GM Port Maintainer and overall geek too. :smile_cat:

[Martii]

Confirmed is a vulnerability in at least:

• TM 4.13.6136 for Fx 92.0

@derjanb So when/do you plan on fixing this to the standard please?

[tophf]

Yes it does

Implicitly. It'd be much better to add an explicit clause about that space just as there is an explicit clause about the space between the key and value. I'm probably "spoilt" by WHATWG/W3C specifications :-)

[Martii]

@tophf

Implicitly

The documentation format for the GM Wiki, at least initially on my part, was modeled after the organizational structure of many authors, including myself, such as the JavaScript Bible 5th Edition ca.2004 which is a well known standard of written publications.

It'd be much better to add an explicit clause about that space just as there is an explicit clause about the space between the key and value.

I'm sure if @arantius were willing he could accommodate that for those that aren't familiar with publications.

Don't get me wrong... I'm not disagreeing with your issue but I swear this has been talked about before and I thought it was resolved. The linkage evades memory but I'll report back if I find it when I have time.

[tophf]

I'd like to clarify that wiki per se is fine as its primary purpose is to help userscript authors, but it could have a section for engine implementers with more explicit phrasing or even the precise parsing syntax used in specifications/RFC.

[Martii]

Refs:

• Here is GM's current peg implementation for them in descriptive engineering terms.
• We model similarly at OUJS.
• GM Port does it here.

[derjanb]

... In the meantime I have to test current TM for this VULNERABILITY and if exists on my end TM might be kicked out of places until it follows the the written standard.

Really? Wow.

So when/do you plan on fixing this to the standard please?

The next beta release will show a linter warning at the editor and once this was released as stable version for some time, then its probably safe to change the parser to ignore such keys.

[Martii]

@derjanb

Really?

Yes (and sarcasm detected I think).

Wow.

Definitely. Such as violating https://developer.chrome.com/docs/webstore/terms/ and other web browsers TOS/TOUs. Both GF and OUJS have that option as well both individually and in tandem with the stores. So does Microsoft. Don't take it personal. It's advice to let you know the severity. You already know that I've supported multiple forks of GM so don't get any unusual ideas of my intent.

The next beta release will show a linter warning at the editor and once this was released as stable version for some time, then its probably safe to change the parser to ignore such keys.

I'm not sure I agree with the "some time" part. You really need to do this ASAP. If enough reports come in about your extension you will at minimum be blocked in the stores. Please reconsider doing this soon as the parser just needs a space added which is very simple to do and get reviewed. Milestones are good too here on GH and labels as you already know.

If you value your users and your professional reputation its best to fix these kind of bug reports as quickly as possible.

[7nik]

VULNERABILITY

Can you provide an example of how it can be used as a vulnerability? Otherwise, it's just an issue of parsing the metablock.

I've personally used //@key when I do NOT want a .user.js engine to load a resource

It's a non-obvious way to disable something. Here is a more obvious way I use: // @ key. And IMHO you shouldn't leave it in a public version.

that will be unfortunate for those who have comments describing their keys/other use case as well.

IMHO, anyway, the metablock should contain only valid (enabled) keys. All describing comments should go before or after the metablock.

TM might be kicked out of places until it follows the the written standard.

If enough reports come in about your extension you will at minimum be blocked in the stores.

sarcasm detected I think

Well, I did not. And, maybe due to it, for me personally, it sounds threatening, and more hilarious is that it's threatening over nothing.

Don't get me wrong, I agree that it should be fixed but I still don't see any reason why it should be fixed ASAP.

BTW, after reading it, I think that it would be better if, at saving and updating a script, a script manager warns users about unknown/invalid properties and GM.functions in the metablock. Thus a user will know whether a script suits a script manager, and can report it to the script developer or the SM developer.

[derjanb]

@Martii

VULNERABILITY Otherwise, it's just an issue of parsing the metablock.

That's what I see in it.

Yes (and sarcasm detected I think).

No sarcasm but real amazement at a very strong reaction to an unimportant problem. But it is your website and of course you can do what you want there.

you will at minimum be blocked in the stores

Not sure what the stores have to do with it, but different ways of parsing a userscript header is, in my opinion, outside their area of interest.

If someone is using a flaw in TMs implementation of GM then those scripts are already broken for not following a standard specification.

I think I need to clarify something here. I don't see Tampermonkey as an "implementation of GM". They shared some common APIs some time, but since the beginning Tampermonkey also included additionals APIs. Don't get me wrong, I see the benefits of some specifications especially between the userscript engines and hosters. Therefore it is fine to have the GM wiki to see how things are handled there.

However, the GM wiki is not the authoritative source for Tampermonkey's implementation for multiple reasons:

• first, I have neither control nor the right to have a say. Besides, you may know who has write access to it, but I don't.
• second, Tampermonkey's goal is to be backward compatible to Greasemonkey up to version 3. Which I interpret as: a userscript written for GM 3 or below should work in Tampermonkey, but not necessarily the other way around. Especially GM 4 started to divert from the GM 3 API while Tampermonkey still provides this useful synchronous API (which is not part of GM's specification anymore). However, I've also added the GM. APIs for compatibility reasons.
• finally, Tampermonkey provides more features and enhances existing ones and therefore it needs its own documentation

I try to document all stable features, some experimental features are only roughly explained at their Github issue though.

I say again: I changed my mind, for the sake of compatibility, I'll add a warning to TM's editor now and I'll change the parser to ignore such keys later, but I see no need to hurry and want to create smooth transition to allow userscripts authors to fix the issue before the syntax becomes invalid.

For example there are companies that use Tampermonkey with their own internal scripts. I do not know their scripts neither do you. To warn on issues that might make a script not work in a future version is how I try to maintain "professional reputation". 😉

@JasonBarnabe

Considering this syntax is not widely supported, I'm going to be adding validation on Greasy Fork to prevent it. Not sure if that affects the decision for Tampermonkey.

Yes, thanks.

[arantius]

As far as Greasemonkey is concerned, the syntax is:

https://github.com/greasemonkey/greasemonkey/blob/master/peg.txt

Which absolutely requires whitespace between the key and value, for keys that take values. (The link in that file is broken, apparently the parser generator tool is now at https://pegjs.org/ .)

[tophf]

@arantius, you just wrote "between the key and value" but this issue is about the space between the // and @key so you're implicitly confirming it as a source of confusion and my suggestion was to add an explicit note about this space in the wiki, that it must be exactly one space, in addition to the existing note about the space between the key and the value.

[arantius]

Honestly I really don't like Martii's communication style and didn't read most of the above text, but the space after slashes is also explicitly required by that grammar.

[JasonBarnabe]

To make it clear, as the owner of Greasy Fork, I consider this just to be a compatibility issue and not a security issue, and have no intention of blocking TM or reporting it for removal.

[tophf]

@arantius, well, the grammar isn't linked in the wiki :-) so my reasoning was that if wiki mentions a kinda expected space between the key and the value then it should also mention the space before the key because spaces are typically not limited to an exact number thereof.

[Martii]

@7nik https://openuserjs.org/scripts/Marti/RFC_2606%C2%A73_-_Hello%2C_World!_(test_for_TM_vulnerability) then go to http://www.example.com/ . Should only see one alert() popup. Consider if it was malicious injected code that someone expected to be treated as a comment.

@derjanb

No sarcasm but real amazement at a very strong reaction to an unimportant problem

Seems important and relevant to at least 3 others which is why I was summoned in private to this issue... otherwise I would have missed it as I'm super busy in real life atm. I am here hoping that this doesn't snowball like TM's updating issue a few years back that caused DDoS attacks against GF and OUJS. That was just as important imho.

[7nik]

@Martii and? For some scripts, it's required behavior to work properly, for others it isn't. And this is why it's better to fix in a few steps to allow developers to correct their scripts. The code, that is run in the "disabled" requirement, still is included by the author but not a third party. So this is no reason to fix it ASAP.

Unlike the problem of @include, which allows running poorly written scripts on others pages, which may exploit badly written code. This, in my opinion, is more real vulnerability. E.g., the following script runs on https://httpbin.org/get?target=www.example.com/ though it isn't "included":

// ==UserScript==
// @name         @include vulnerability
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  try to take over the world!
// @author       7nik
// @include      https://*.example.com/*
// @icon         https://www.google.com/s2/favicons?domain=example.com
// @grant        none
// ==/UserScript==

(function() {
    'use strict';

    alert(42);
})();

[derjanb]

Warning implemented at 4.14.6144 (in review|crx)