After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 658612 - Last minute tweaks to the Extension System, for supporting more SweetTooth features.
Last minute tweaks to the Extension System, for supporting more SweetTooth fe...
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-08 22:41 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2011-09-16 17:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
extensionSystem: Replace manifest system with a more direct approach (4.76 KB, patch)
2011-09-08 22:41 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
extensionSystem: Use a closure for all things (28.06 KB, patch)
2011-09-08 22:41 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
extensionSystem: Make final adjustments towards SweetTooth (7.71 KB, patch)
2011-09-09 22:13 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
extensionSystem: Add "UninstallExtension" DBus method (3.62 KB, patch)
2011-09-10 21:11 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
extensionSystem: Add "UninstallExtension" DBus method (3.83 KB, patch)
2011-09-11 02:38 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
extensionSystem: Always enable an extension for a user (1.24 KB, patch)
2011-09-11 02:38 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
extensionSystem: Make final adjustments towards SweetTooth (7.97 KB, patch)
2011-09-12 13:56 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_after_freeze Details | Review
extensionSystem: Add "UninstallExtension" DBus method (3.89 KB, patch)
2011-09-12 13:58 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
extensionSystem: Always enable an extension for a user (1.26 KB, patch)
2011-09-12 13:59 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
extensionSystem: Add "UninstallExtension" DBus method (3.99 KB, patch)
2011-09-12 14:36 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
extensionSystem: Add an explicit approval dialog (5.24 KB, patch)
2011-09-13 02:22 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2011-09-08 22:41:15 UTC
Owen had a bit of issue with the manifest system, so here's a more direct approach.
The closure is something that should have been done earlier: without this, a bad
extension can disable or change the source. When updating and the blacklist logic are
done, the benefits of this will be helpful.
Comment 1 Jasper St. Pierre (not reading bugmail) 2011-09-08 22:41:17 UTC
Created attachment 196049 [details] [review]
extensionSystem: Replace manifest system with a more direct approach

For security reasons, we shouldn't allow the Shell to download and install
any extension URL.
Comment 2 Jasper St. Pierre (not reading bugmail) 2011-09-08 22:41:19 UTC
Created attachment 196050 [details] [review]
extensionSystem: Use a closure for all things

If we're going to be doing more direct things in the extension system, like
blacklisting and updating, extensions cannot be able to modify or inject the
extension system.
Comment 3 Dan Winship 2011-09-09 11:08:54 UTC
Comment on attachment 196050 [details] [review]
extensionSystem: Use a closure for all things

>If we're going to be doing more direct things in the extension system, like
>blacklisting and updating, extensions cannot be able to modify or inject the
>extension system.

Can you explain in more detail precisely what this patch is supposed to be doing/preventing? (Like, say, an example of something bad that could be done against the existing code, which is supposed to not be possible with the new code.)

eg, nothing here would stop an extension from saying "ExtensionSystem.enableExtension = function () { evil; }"

(also, you forgot to export installExtensionFromUUID)
Comment 4 Owen Taylor 2011-09-09 20:52:40 UTC
Review of attachment 196049 [details] [review]:

::: js/ui/shellDBus.js
@@ +175,3 @@
     },
 
+    InstallRemoteExtension: function(uuid, server_uuid) {

what's the distinction between uuid and server_uuid?
Comment 5 Jasper St. Pierre (not reading bugmail) 2011-09-09 22:13:27 UTC
Created attachment 196148 [details] [review]
extensionSystem: Make final adjustments towards SweetTooth

For security reasons, we shouldn't allow the Shell to download and install any
extension URL. Instead, use a direct URL for the Shell to query, and add a
popup dialog to protect the user from unintentional installation of an
extension.



Added a dialog, renamed to "version_tag". Otherwise, same as before.
Comment 6 Jasper St. Pierre (not reading bugmail) 2011-09-10 21:11:26 UTC
Created attachment 196190 [details] [review]
extensionSystem: Add "UninstallExtension" DBus method

For those who like their system pure, this provides the ability to purge a
pesky extension and its precious place on your disk space, and in your
"Local Extension" list.
Comment 7 Jasper St. Pierre (not reading bugmail) 2011-09-11 02:38:35 UTC
Created attachment 196195 [details] [review]
extensionSystem: Add "UninstallExtension" DBus method

For those who like their system pure, this provides the ability to purge a
pesky extension and its precious place on your disk space, and in your
"Local Extension" list.



Fix an error -- importers are marked as PERMANENT, so we can't delete them, so
set them to undefined and hope that nobody uses Object.keys() on the list.
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-09-11 02:38:49 UTC
Created attachment 196196 [details] [review]
extensionSystem: Always enable an extension for a user

When the user installs an extension, we always enable it. Change the
'enabled-extensions' key, if necessary, to reflect this.



--

I have no idea how I got this far without this.
Comment 9 Owen Taylor 2011-09-11 20:37:23 UTC
Review of attachment 196148 [details] [review]:

Needs freeze-break approval and a few other things

::: js/ui/extensionSystem.js
@@ +33,3 @@
 };
 
+const REPOSITORY_URL_BASE = 'https://extensions.gnome.org';

hmm, so doesn't this make the ALLOW_INSECURE_HTTP thing sort of pointless if we then go and fetch over https? Maybe just put complete instructions in the README for sweettooth for doing a devel setup with a self-signed cert on 127.0.0.1 and get rid of the configure flag? (Which I'm not very fond of still.)

@@ +433,3 @@
+                         }]);
+
+        let message = _("Do you want to install %s?".format(name));

needs a mail to release-team and gnome-l10n to ask for this freeze break, I'm happy to send it. I think I'd slightly prefer as a string something like:

 Download and install '%s' from extensions.gnome.org?

                               [ Cancel] [ Install ]

@@ +449,3 @@
+                     error: '' };
+
+        _signals.emit('extension-state-changed', meta);

This is odd... it was already UNINSTALLED, right? (well, maybe - the logic for not installing a currently installed extension through this mechanism seems to be relying on the web site?) I think it's better to use state-changed to only notify of actual changes to the current extension state.

@@ +466,3 @@
+                       api_version: API_VERSION.toString() };
+
+        let url = REPOSITORY_URL_DOWNLOAD.format(this._uuid);

uuid should be in the URL or the params, not both
Comment 10 Owen Taylor 2011-09-11 20:45:19 UTC
Review of attachment 196195 [details] [review]:

Looks close

::: js/misc/fileUtils.js
@@ +38,3 @@
+            deleteGFile(child);
+        else if (type == Gio.TypeType.DIRECTORY)
+            recursivelyDelete(child);

this function isn't called that. Please test with subdirs! :-)

::: js/ui/extensionSystem.js
@@ +121,3 @@
+    let meta = extensionMeta[uuid];
+    if (!meta || meta.state != ExtensionState.DISABLED)
+        return false;

why not just disable the extension ourselves and make this more robust rather than requiring this?
Comment 11 Owen Taylor 2011-09-11 20:47:51 UTC
Review of attachment 196196 [details] [review]:

Looks good

::: js/ui/extensionSystem.js
@@ +181,3 @@
+        if (enabledExtensions.indexOf(uuid) == -1)
+            enabledExtensions.push(uuid);
+        global.settings.set_strv(ENABLED_EXTENSIONS_KEY, enabledExtensions);

I think it's just vaguely cleaner to put this with the previous line in {} - not that the efficiency matters, but to keep the person reading this from having to think "is there another codepath that changes this? hmm, no. does the efficiency matter? No. OK, move on"
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-09-11 22:08:26 UTC
Review of attachment 196148 [details] [review]:

::: js/ui/extensionSystem.js
@@ +449,3 @@
+                     error: '' };
+
+        _signals.emit('extension-state-changed', meta);

This makes the extension website put the switch back to "Off" when someone hits "Cancel". I make HTTP requests through async, and I really don't want to make the plugin more complicated by calling DBus async-ly, so I'm doing this.
Comment 13 Owen Taylor 2011-09-12 13:27:42 UTC
(In reply to comment #12)
> Review of attachment 196148 [details] [review]:
> 
> ::: js/ui/extensionSystem.js
> @@ +449,3 @@
> +                     error: '' };
> +
> +        _signals.emit('extension-state-changed', meta);
> 
> This makes the extension website put the switch back to "Off" when someone hits
> "Cancel". I make HTTP requests through async, and I really don't want to make
> the plugin more complicated by calling DBus async-ly, so I'm doing this.

OK, well, document it then in a comment.
Comment 14 Jasper St. Pierre (not reading bugmail) 2011-09-12 13:56:14 UTC
Created attachment 196259 [details] [review]
extensionSystem: Make final adjustments towards SweetTooth

For security reasons, we shouldn't allow the Shell to download and install any
extension URL. Instead, use a direct URL for the Shell to query, and add a
popup dialog to protect the user from unintentional installation of an
extension.



(In reply to comment #9)
> Review of attachment 196148 [details] [review]:
> 
> Needs freeze-break approval and a few other things
> 
> ::: js/ui/extensionSystem.js
> @@ +33,3 @@
>  };
> 
> +const REPOSITORY_URL_BASE = 'https://extensions.gnome.org';
> 
> hmm, so doesn't this make the ALLOW_INSECURE_HTTP thing sort of pointless if we
> then go and fetch over https? Maybe just put complete instructions in the
> README for sweettooth for doing a devel setup with a self-signed cert on
> 127.0.0.1 and get rid of the configure flag? (Which I'm not very fond of
> still.)

Fine with me. For testing, because it's a bit obnoxious to set up a proper
self-signed cert and all that, I just hack this out in the source anyway. See:

https://github.com/magcius/gnome-shell/commit/1bf29d57b7859ac57c6b27408902d7bc3898ce54

> @@ +433,3 @@
> +                         }]);
> +
> +        let message = _("Do you want to install %s?".format(name));
> 
> needs a mail to release-team and gnome-l10n to ask for this freeze break, I'm
> happy to send it. I think I'd slightly prefer as a string something like:
> 
>  Download and install '%s' from extensions.gnome.org?
> 
>                                [ Cancel] [ Install ]

Fine with me. (Also fixed an embarrassing localization bug where I localized the
formatted string)

> @@ +466,3 @@
> +                       api_version: API_VERSION.toString() };
> +
> +        let url = REPOSITORY_URL_DOWNLOAD.format(this._uuid);
> 
> uuid should be in the URL or the params, not both

I use this URL scheme so that anyone downloading the zipfile manually (which
was still a thing back in the mimetype handler days) would get a useful
filename. I removed it from the params.
Comment 15 Jasper St. Pierre (not reading bugmail) 2011-09-12 13:58:35 UTC
Created attachment 196261 [details] [review]
extensionSystem: Add "UninstallExtension" DBus method

For those who like their system pure, this provides the ability to purge a
pesky extension and its precious place on your disk space, and in your
"Local Extension" list.



(In reply to comment #10)
> Review of attachment 196195 [details] [review]:
> 
> Looks close
> 
> ::: js/misc/fileUtils.js
> @@ +38,3 @@
> +            deleteGFile(child);
> +        else if (type == Gio.TypeType.DIRECTORY)
> +            recursivelyDelete(child);
> 
> this function isn't called that. Please test with subdirs! :-)
> 

Whoops.

> ::: js/ui/extensionSystem.js
> @@ +121,3 @@
> +    let meta = extensionMeta[uuid];
> +    if (!meta || meta.state != ExtensionState.DISABLED)
> +        return false;
> 
> why not just disable the extension ourselves and make this more robust rather
> than requiring this?

I did that, but I guess there's still a problem if the extension is ERROR'd.

The original thought process was "in the chance that it's not DISABLED,
tool using our APIs would know how to clean up better than us." But I guess
that's not very likely.
Comment 16 Jasper St. Pierre (not reading bugmail) 2011-09-12 13:59:14 UTC
Created attachment 196262 [details] [review]
extensionSystem: Always enable an extension for a user

When the user installs an extension, we always enable it. Change the
'enabled-extensions' key, if necessary, to reflect this.



"Optimization" done -- should I apply the same ones in ShellDBus (from which
this code was copy/pasted)?
Comment 17 Jasper St. Pierre (not reading bugmail) 2011-09-12 14:36:55 UTC
Created attachment 196267 [details] [review]
extensionSystem: Add "UninstallExtension" DBus method

For those who like their system pure, this provides the ability to purge a
pesky extension and its precious place on your disk space, and in your
"Local Extension" list.



Replace faulty if() with a comment
Comment 18 Owen Taylor 2011-09-12 16:13:33 UTC
Review of attachment 196259 [details] [review]:

Looks good, still waiting for freeze break approval.
Comment 19 Owen Taylor 2011-09-12 16:15:46 UTC
Review of attachment 196262 [details] [review]:

Looks good, if we cared if shellDBus.js matched, there would be a method in extensionSystem rather than duplicated code!
Comment 20 Owen Taylor 2011-09-12 16:17:56 UTC
Review of attachment 196267 [details] [review]:

Looks good
Comment 21 Jasper St. Pierre (not reading bugmail) 2011-09-12 18:38:32 UTC
Attachment 196262 [details] pushed as 02b8804 - extensionSystem: Always enable an extension for a user
Attachment 196267 [details] pushed as 7928f90 - extensionSystem: Add "UninstallExtension" DBus method


I split "final adjustments" into two commits -- the one that adds the dialog will be commited after the freeze, if it gets accepted.
Comment 22 Jasper St. Pierre (not reading bugmail) 2011-09-13 02:22:44 UTC
Created attachment 196325 [details] [review]
extensionSystem: Add an explicit approval dialog

Pop up a dialog when trying to install an extension so that users are aware
they are installing one. This is a security precaution in the case that an XSS
exploit has been found on the website, which could cause someone to inject a
<script> tag and silently install an extension.



And for good measure, attach my last patch here, so it's tracked *somewhere*.
Comment 23 Jasper St. Pierre (not reading bugmail) 2011-09-13 21:38:02 UTC
Comment on attachment 196325 [details] [review]
extensionSystem: Add an explicit approval dialog

Attachment 196325 [details] pushed as 7db92ad - extensionSystem: Add an explicit approval dialog