Moved user-script detection to background scripts #2719

Sxderp · 2017-11-21T16:27:41Z

I made these changes in preparation for some other things I was testings some time ago, but I recently found out that moving script detection to the background can help bypass some of the current CSP issues Greasemonkey is having with installing scripts.

Per #2631, script detection fails for CSP hosts. With this change scripts are properly detect.

Some things to note.

This makes use of filterResponseData which is FF 57+ only. So no FF 52 ESR support nor Chrome support (I think Chrome support was dropped/no long a priority per Switch back to browser API with promises #2560?)
Performing the check in the background allows for safely caching a script in the background (in some variable or a database) so when ready for install the extra XHR to retrieve it is unnecessary.

arantius · 2017-11-21T18:29:58Z

This is missing mime type detection, necessary for things like not breaking browsing to the UI view of a script that is stored in e.g. GitHub.

Also, what happens if the server is slow to respond? What if it's a big script, and it takes much longer to load the whole thing than just the metadata which we strictly need?

Sxderp · 2017-11-22T04:21:51Z

This is missing mime type detection, necessary for things like not breaking browsing to the UI view of a script that is stored in e.g. GitHub.

I'll have to look at what can be done about extracting the mime type.

Also, what happens if the server is slow to respond? What if it's a big script, and it takes much longer to load the whole thing than just the metadata which we strictly need?

I'm not sure I understand the concern. With the current method the script is fetched and downloaded, the content script runs, and then we get the install dialog. The whole script has to be loaded anyway.

arantius · 2017-11-22T16:14:47Z

With the current method the script is fetched and downloaded, the content script runs, and then we get the install dialog.

Exactly. There could be a very long delay between clicking the link and seeing anything happen.

The whole script has to be loaded anyway.

But it doesn't have to be loaded now, while the user waits with no feedback. Look at how 3.x acted. It stops as soon as it has the metadata, pops the install dialog, then starts the actual download of the script and all resources, while the user can read the dialog and decide what to do.

Sxderp · 2017-11-22T17:38:18Z

From what I understand of the 3.x code flow, this is what happens.

If URI matches a user script profile then initiate download of the text content of the script.
The entire text content is fetched[1].
The dialog box is opened while the dependencies are downloaded in the background.
Based on what the user does.
1. If the user does not install everything is cleaned up.
2. If the user installs then everything is saved.

At the moment I feel doing significant changes to the install workflow (from the current WebExtension version) is out of the scope of this particular pull request, and should be addressed in followup commits.

[1] There're stubs for what looks like downloading just enough for the metablock but it doesn't seem to be implemented.

arantius · 2017-11-22T18:14:47Z

I used to have a public demo of this but it's broken so launch this: https://gist.github.com/arantius/3b493cb7ae6aad92d6c93881c14f1c4f with GM 3.x installed. Browse to http://localhost:8000/ and you'll see it take ~10 seconds to load. Browse to http://localhost:8000/slow.user.js and you'll see that it takes about 2 seconds to pop the install dialog (which then continues for another ~8 to complete the download).

GM 4.x definitely does not do this today (Firefox does some funny buffering of the entire document before it displays any of it, and we don't run until that point anyway). But that's because 4.x was the simplest hack I could throw together that would work at all. We should be heading in this direction (UI feedback as soon as possible), not away from it.

Sxderp · 2017-11-22T21:23:27Z

This is missing mime type detection

Added in recent commit.

GM 4.x definitely does not do this today

Added in recent commit. However, still using the full tab for the install message. But, it can be easily switched over when the dialog windows are working. Which, they seem to be working[1] fine in FF 59. Not sure about 57-58.

[1] Title prefix is still semi-broken.

Sxderp · 2017-11-22T21:26:08Z

Oh, I failed to mention that this doesn't affect the current installation flow. That should probably be done in a followup.

Sxderp · 2017-12-04T01:41:38Z

The changes made in this branch partially put us toward the 3.x functionality. If this gets merged I have some additional commits that'll bring the download and install much closer to 3.x. Key points on what I have.

Pops install as soon as possible (only item on this list that is in this pull request).
Uses a install dialog / window (works-ish on FF59, as mentioned before, needs testing for previous versions).
Continues downloading the main script without interrupting the page request.
As the main script is downloading it also asynchronously starts downloading the dependencies.
Displays the main script in the original tab, allows for viewing of the source should someone desire.
If the 'Install' button is pressed the dialog window closes. Resources continue downloading in the background and on completion and database transaction notifies the user (browser.notification) that the script has installed.
If the 'Cancel' button is pressed the dialog window closes. All the network requests are aborted, save for the main script in the original tab.
Further, how scripts are updated (from editor save or to be implemented auto-update, the code path should be future proof..ish) was modified too. Should help alleviate Editing user script causes @require to fail loading #2683.

I could push these changes to this branch, but it's kind of a lot (all having to deal with the same thing though).

Note, there are no progress indicators / bars. I used fetch because of their inherent promise-ness and using them makes it very difficult to display some sort of status updates.

Sxderp · 2018-01-13T20:13:33Z

Rebased to deal with #2641. Also includes a small fix to cause the page to close after clicking install.

arantius · 2018-01-19T16:55:03Z

manifest.json

@@ -9,7 +9,7 @@
  "applications": {
    "gecko": {
      "id": "{e4a8a97b-f2ed-450b-b12d-ee082ba24781}",
-      "strict_min_version": "52.0"
+      "strict_min_version": "57.0"


filterResponseData is FF 57+
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/filterResponseData

arantius · 2018-01-19T16:55:10Z

src/bg/on-message.js

-  if (message.name == 'OpenInstallDialog') {
-    // No-op; content can legitimately ask for the dialog to be opened.
-  } else if (
+  if (


Should be one line now.

arantius · 2018-01-19T16:55:45Z

src/bg/user-script-detect.js

@@ -0,0 +1,103 @@
+/*
+Add listeners to detecting user scripts and opening the installation


This form:

/* Add listeners to detect user scripts and open the installation dialog. */

Is better grammar and fits on one line.

arantius · 2018-01-19T16:55:47Z

src/bg/user-script-detect.js

+dialog.
+*/
+
+


Why two blank lines here?

arantius · 2018-01-19T16:56:18Z

src/bg/user-script-detect.js

+// Examine headers before determining if script checking is needed
+function checkHeaders(responseHeaders) {
+  for (header of responseHeaders) {
+    if ('Content-Type' === header.name && userScriptTypes.has(header.value)) {


These should be case-insensitive matches, as the server is not guaranteed to provide any particular case for these values. (Just tested, when I send all-lower values, the header name is exactly "content-type" here.)

arantius · 2018-01-19T16:57:56Z

src/content/install-dialog.js

-  chrome.windows.getCurrent(null, win => {
-    chrome.windows.remove(win.id);
+  chrome.tabs.getCurrent(curTab => {
+    chrome.tabs.remove(curTab.id);


Why?

Desktop vs. Android?

Using tabs works on both devices. Android needs to use tabs since it doesn't have windows. On desktop since the installer is opened using a dialog the window is considered a single tab window. When that tab is removed so is the window.

arantius · 2018-01-19T17:00:19Z

My test server delivers:

$ curl -vI 'http://localhost/user-scripts/updateable/script5.user.js' 2>/dev/null | grep -i content-type
Content-Type: text/plain; charset=utf-8

Which this fails to match, the install dialog does not pop.

arantius · 2018-01-19T17:04:45Z

Ideally this would act like 3.x, if the load is detected as a script, abort and never show the navigation. If necessary, always abort (pause?), and when detected not a script, re-start (unpause?).

Right now after patching, I'm seeing both the install dialog pop and the browser navigate to the script.

Sxderp · 2018-01-19T17:21:15Z

Right now after patching, I'm seeing both the install dialog pop and the browser navigate to the script.

It was so that users could see the source before installing. As mentioned in another issue you talked about the possibility for a malicious server to send different content on the second request that GM currently uses to fetch and save a script. While that concern is valid, it's also somewhat moot, at this moment, due to GM still not having a way to install scripts in a disabled state.

Furthermore, a followup PR that I'd like to submit would resolve the above issue anyway, as it reworks the script download / installation so that a secondary request isn't required, among other things (background / async).

I can look into modifying this PR so that it reflects the abort / navigation redirect that 3.x does. But I don't think, in the longer term, it should remain like that.

Sxderp · 2018-01-19T17:41:54Z

Ah, I don't believe I address #2643 in this PR. Should be an easy fix to add a check at the top of the main function call.

Sxderp · 2018-01-20T01:53:31Z

I can look into modifying this PR so that it reflects the abort / navigation redirect that 3.x does. But I don't think, in the longer term, it should remain like that.

Attempting to implement the 3.x behavior and I've hit a problem. With a StreamFilter the request content is given to us as Firefox receives it. Now, after we have 'enough' content to open an installation dialog I can tell Firefox to stop processing the request.

However, we now have a problem. The user is then presented with a blank page, or a page with a partially downloaded script if filter.write() is used before closing. I'm having trouble trying to figure out a 'nice' way to deal with this result.

I can get the tab that the script request belongs to but unfortunately, as far as I can see, there's no history.back() equivalent in the background. And I can't just close the tab as it may have a non-zero history. Redirecting using details.originUrl is also a poor choice. If the script was opened using middle-click (open in new tab) then details.originUrl will be that of the previous tab. After redirect you're stuck with two tabs at the same url.

I also attempted to do a tabs.executeScript() with a history.back(), but that doesn't seem to be working with a middle-click either.

If this is absolutely necessary, then I'm stumped. Maybe someone else has some ideas?

greasemonkey#2643

arantius · 2018-01-26T14:53:53Z

I'd still prefer that clicking a link on a script pops the install dialog and otherwise leaves the browser unchanged, like 3.x. But this fixes installs from sites with CSP like GitHub and also #2643 both of which are good. I'm merging it, we can figure out more improvements in the future.

Note, there are no progress indicators / bars. I used fetch because of their inherent promise-ness and using them makes it very difficult to display some sort of status updates.

I'd also like to see progress bars come back.

arantius · 2018-01-26T14:57:18Z

(I think Chrome support was dropped/no long a priority per #2560?)

No, the idea was to switch our code to Promise-y browser APIs, and use the linked polyfill to get Chrome compatibility. (Firefox first, not Firefox only.) But also supporting Chrome adds a lot of testing/verification burden so who knows if/when it will happen.

arantius added this to the 4.x milestone Nov 23, 2017

Sxderp force-pushed the detect-scripts-in-background branch from 1c3be73 to 44925a8 Compare November 28, 2017 15:33

Sxderp mentioned this pull request Nov 28, 2017

Add "install disabled" feature to fill the "view script source" feature #2567

Closed

arantius modified the milestones: 4.x, 4.2 Dec 4, 2017

Sxderp mentioned this pull request Dec 7, 2017

Safely handle bad userscript save transactions #2750

Merged

Sxderp force-pushed the detect-scripts-in-background branch 2 times, most recently from 909b0b8 to 2c47caf Compare December 11, 2017 23:26

arantius modified the milestones: 4.2, 4.3 Jan 5, 2018

Sxderp mentioned this pull request Jan 6, 2018

Install via pop up window, not tab #2641

Closed

Sxderp force-pushed the detect-scripts-in-background branch 6 times, most recently from ca43699 to 382b5a4 Compare January 13, 2018 20:10

Sxderp force-pushed the detect-scripts-in-background branch from 382b5a4 to 3c00f70 Compare January 18, 2018 19:34

arantius requested changes Jan 19, 2018

View reviewed changes

Sxderp added 2 commits January 19, 2018 21:00

Moved user-script detection to background scripts

9109b62

Adjust formatting per greasemonkey#2719

89724a0

Sxderp force-pushed the detect-scripts-in-background branch 2 times, most recently from d5beb92 to 8143de5 Compare January 22, 2018 20:08

Sxderp added 2 commits January 22, 2018 15:11

Match .user.js files on content types with parameters

aef5bdd

Dont open install dialog if GM is disabled

7790d90

greasemonkey#2643

Sxderp force-pushed the detect-scripts-in-background branch from 8143de5 to 7790d90 Compare January 22, 2018 20:12

arantius merged commit 7790d90 into greasemonkey:master Jan 26, 2018

Sxderp deleted the detect-scripts-in-background branch January 27, 2018 01:55

Sxderp mentioned this pull request Jan 31, 2018

GM 4.0 does not install scripts from GitHub (CSP/sandbox issue) #2631

Closed

arantius mentioned this pull request Feb 2, 2018

Test install in Firefox for Android #2827

Closed

Sxderp mentioned this pull request Feb 15, 2018

Don't hide scripts when prompting to install. #2851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved user-script detection to background scripts #2719

Moved user-script detection to background scripts #2719

Sxderp commented Nov 21, 2017

arantius commented Nov 21, 2017

Sxderp commented Nov 22, 2017

arantius commented Nov 22, 2017

Sxderp commented Nov 22, 2017

arantius commented Nov 22, 2017

Sxderp commented Nov 22, 2017

Sxderp commented Nov 22, 2017

Sxderp commented Dec 4, 2017

Sxderp commented Jan 13, 2018

arantius Jan 19, 2018

Sxderp Jan 19, 2018

arantius Jan 19, 2018

arantius Jan 19, 2018

arantius Jan 19, 2018

arantius Jan 19, 2018

arantius Jan 19, 2018

Sxderp Jan 19, 2018

arantius commented Jan 19, 2018

arantius commented Jan 19, 2018

Sxderp commented Jan 19, 2018

Sxderp commented Jan 19, 2018

Sxderp commented Jan 20, 2018

arantius commented Jan 26, 2018

arantius commented Jan 26, 2018

		@@ -0,0 +1,103 @@
		/*
		Add listeners to detecting user scripts and opening the installation

Moved user-script detection to background scripts #2719

Moved user-script detection to background scripts #2719

Conversation

Sxderp commented Nov 21, 2017

arantius commented Nov 21, 2017

Sxderp commented Nov 22, 2017

arantius commented Nov 22, 2017

Sxderp commented Nov 22, 2017

arantius commented Nov 22, 2017

Sxderp commented Nov 22, 2017

Sxderp commented Nov 22, 2017

Sxderp commented Dec 4, 2017

Sxderp commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arantius commented Jan 19, 2018

arantius commented Jan 19, 2018

Sxderp commented Jan 19, 2018

Sxderp commented Jan 19, 2018

Sxderp commented Jan 20, 2018

arantius commented Jan 26, 2018

arantius commented Jan 26, 2018