GNOME Bugzilla – Bug 658612
Last minute tweaks to the Extension System, for supporting more SweetTooth features.
Last modified: 2011-09-16 17:13:08 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.
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.
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 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)
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?
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.
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.
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.
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.
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
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?
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"
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.
(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.
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.
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.
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)?
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
Review of attachment 196259 [details] [review]: Looks good, still waiting for freeze break approval.
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!
Review of attachment 196267 [details] [review]: Looks good
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.
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 on attachment 196325 [details] [review] extensionSystem: Add an explicit approval dialog Attachment 196325 [details] pushed as 7db92ad - extensionSystem: Add an explicit approval dialog