GNOME Bugzilla – Bug 658070
Move sweettooth-plugin into gnome-shell tree, rebrand
Last modified: 2011-09-12 18:37:45 UTC
Import source from http://github.com/magcius/sweettooth-plugin, as suggested on ML.
Created attachment 195497 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one.
Review of attachment 195497 [details] [review]: I'd like a configure option to turn it off entirely. Did anyone review the code to this extension? Someone with experience with NPAPI? ::: src/browser-plugin/browser-plugin.c @@ +128,3 @@ + hostname = get_string_property (instance, + NPVARIANT_TO_OBJECT (location), + "hostname"); I wonder if this is susceptible to scripts manipulating document.hostname; depends when the browser instantiates the plugin I guess. ::: src/browser-plugin/npapi/npapi.h @@ +449,3 @@ + NPNVGtk12 = 1, + NPNVGtk2 +} NPNToolkitType; What is this? Just cargo-culted from some other plugin? Actually I guess there's a huge amount of crap here, like Cocoa. Can you describe where you got this code and what was modified (if anything)? A README file would be best.
Also I should put in here again my objections to doing this through a browser plugin - we're extending the attack surface available to the entire unfiltered internet. It should be trivial to make a GtkWebKit app.
(In reply to comment #2) > Review of attachment 195497 [details] [review]: > > I'd like a configure option to turn it off entirely. > > Did anyone review the code to this extension? Someone with experience with > NPAPI? Owen posted a big review on gnome-shell-list, and the full history is at http://github.com/magcius/sweettooth-plugin/ http://mail.gnome.org/archives/gnome-shell-list/2011-September/msg00014.html > ::: src/browser-plugin/browser-plugin.c > @@ +128,3 @@ > + hostname = get_string_property (instance, > + NPVARIANT_TO_OBJECT (location), > + "hostname"); > > I wonder if this is susceptible to scripts manipulating document.hostname; > depends when the browser instantiates the plugin I guess. No. window.location.hostname is set by the browser and cannot be manipulated by scripts. > ::: src/browser-plugin/npapi/npapi.h > @@ +449,3 @@ > + NPNVGtk12 = 1, > + NPNVGtk2 > +} NPNToolkitType; > > What is this? Just cargo-culted from some other plugin? Actually I guess > there's a huge amount of crap here, like Cocoa. https://wiki.mozilla.org/NPAPI lists http://code.google.com/p/npapi-sdk/ which is where I got the headers from. I can put in a README if necessary. (In reply to comment #3) > Also I should put in here again my objections to doing this through a browser > plugin - we're extending the attack surface available to the entire unfiltered > internet. It should be trivial to make a GtkWebKit app. It's quite easy if you want to do this, but I am not going to. Just export an object, window.SweetTooth, which contains: * shellVersion * apiVersion * listExtensions() * getExtensionInfo(uuid) * setExtensionEnabled(uuid, enabled) * getExtensionErrors(uuid) * installExtension(manfiestURL) It's pretty straightforward. Right now the API version is '1', but the idea is that the website can work with older Shell versions if we need to break the API for any reason.
Review of attachment 195497 [details] [review]: I think I'd prefer to see this all in a separate toplevel directory to make clear the separation from the rest of the code and allow clearly associating a README with the code - probably in browser-plugin/. Code is looking good to me now; still a few bugs and style points. ::: configure.ac @@ +213,3 @@ +AC_ARG_ENABLE(browser-plugin-foreign-origin, + AS_HELP_STRING([--enable-browser-plugin-foreign-origin=yes],[Allow origins other than "extensions.gnome.org" in the browser plugin]),,allow_foreign_origin=no) +AM_CONDITIONAL(ALLOW_FOREIGN_ORIGIN, test "x$allow_foreign_origin" = xyes) AM_CONDITIONAL is for makefile conditionals. You probably wanted AC_DEFINE() I think this configuration doesn't make sense. At least with the installation as you had it previously, this would be "make any site on the internet be able to run arbitrary code on my machine"! If someone is hacking on the site, they can hack their /etc/hosts to set extensions.gnome.org appropriately. @@ +217,3 @@ +AC_ARG_ENABLE(browser-plugin-insecure-http, + AS_HELP_STRING([--enable-browser-plugin-insecure-http=yes],[Allow raw HTTP, don't force HTTPS]),,allow_insecure_http=no) +AM_CONDITIONAL(ALLOW_INSECURE_HTTP, test "x$allow_insecure_http" = xyes) This one might make more sense, because setting up a HTTPS server for a test install is a pain, and being in the position to hijack the DNS for a extensions.gnome.org hacker is harder then getting them to go to some web page. Still I'm wondering if it might be better as an envvar you need to set before running firefox so it doesn't stick. It *is* possible that I might guess you have this config set, and at the next GUADEC trojan the wireless to redirect extensions.gnome.org on you. (Another option that comes to mind is that gethostbyname("extensions.gnome.org") is 127.0.0.1, then skip the https check, but I think that's too magical and complex for a development hack.) ::: src/Makefile-browser-plugin.am @@ +9,3 @@ +libgnome_shell_browser_plugin_la_LIBADD = \ + $(GNOME_SHELL_LIBS) \ + $(JSON_GLIB_LIBS) JSON_GLIB_LIBS doesn't exist; I also don't think you want to link the browser plugin against everything in GNOME_SHELL_LIBS - having the separate BROWSER_PLUGIN_LIBS which has what it needs (json-glib, gio, perhaps) should be sufficient. ::: src/browser-plugin/browser-plugin.c @@ +35,3 @@ +#include <json-glib/json-glib.h> + +#include "config.h" config.h should always be the first include @@ +98,3 @@ +check_origin_and_protocol (NPP instance) +{ + gboolean ret = TRUE; this function returns TRUE if an error is encountered reading document.location.hostname - that's definitely not the safe way to fail. If something goes wrong, then assume it's not OK. @@ +114,3 @@ + &document); + if (error != NPERR_NO_ERROR) + goto doc_error; Are you sure that the NPVariant has been initialized in the error case at this point? I think it probably would be best to do: NPVariant document = { NPVariantType_Void }; (the initializer also will 0-initialize the unsupplied fields), so you can just the variants them all unconditionally, and also set NPObject *window to NULL, and do if (window != NULL) for that and the same for the stirngs, and then you can have only a single error point, rather than a whole set of them. Much easier to get right. @@ +118,3 @@ + goto doc_error; + + funcs.getproperty (instance, NPVARIANT_TO_OBJECT (document), You don't assign error here, but you use it below @@ +250,3 @@ + NULL, /* GCancellable */ + &error); + if (!data->proxy) { misindentation @@ +381,3 @@ +{ + gsize i, length; + length = strlen (uuid); Doesn't matter, but generally not sthe stylish way to iterate over a C string, since you traverse over the string once just to find the length for (i = 0; uuid[i]; i++) { ... } @@ +425,3 @@ + json = json_generator_to_data (generator, &json_length); + + buffer = funcs.memalloc (json_length); this is likely allowed to fail, so result should be checked (add a boolean return this method then) @@ +627,3 @@ + } + + buffer = funcs.memalloc (strlen (version)); this is likely allowed to fail, so the return should be checked @@ +715,3 @@ + return plugin_get_api_version (obj, result); + + if (name == shell_version_id) would be clearer to write this as an else if chain
Created attachment 195540 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. (In reply to comment #5) > Review of attachment 195497 [details] [review]: > > I think I'd prefer to see this all in a separate toplevel directory to make > clear the separation from the rest of the code and allow clearly associating a > README with the code - probably in browser-plugin/. Code is looking good to me > now; still a few bugs and style points. It's a top-level directory now, and I added a simple README. Good enough? > ::: configure.ac > @@ +213,3 @@ > +AC_ARG_ENABLE(browser-plugin-foreign-origin, > + AS_HELP_STRING([--enable-browser-plugin-foreign-origin=yes],[Allow origins > other than "extensions.gnome.org" in the browser > plugin]),,allow_foreign_origin=no) > +AM_CONDITIONAL(ALLOW_FOREIGN_ORIGIN, test "x$allow_foreign_origin" = xyes) > > AM_CONDITIONAL is for makefile conditionals. You probably wanted AC_DEFINE() > > I think this configuration doesn't make sense. At least with the installation > as you had it previously, this would be "make any site on the internet be able > to run arbitrary code on my machine"! If someone is hacking on the site, they > can hack their /etc/hosts to set extensions.gnome.org appropriately. OK, I removed this one. > @@ +217,3 @@ > +AC_ARG_ENABLE(browser-plugin-insecure-http, > + AS_HELP_STRING([--enable-browser-plugin-insecure-http=yes],[Allow raw HTTP, > don't force HTTPS]),,allow_insecure_http=no) > +AM_CONDITIONAL(ALLOW_INSECURE_HTTP, test "x$allow_insecure_http" = xyes) > > This one might make more sense, because setting up a HTTPS server for a test > install is a pain, and being in the position to hijack the DNS for a > extensions.gnome.org hacker is harder then getting them to go to some web page. > Still I'm wondering if it might be better as an envvar you need to set before > running firefox so it doesn't stick. It *is* possible that I might guess you > have this config set, and at the next GUADEC trojan the wireless to redirect > extensions.gnome.org on you. > > (Another option that comes to mind is that > gethostbyname("extensions.gnome.org") is 127.0.0.1, then skip the https check, > but I think that's too magical and complex for a development hack.) I kept the configuration parameter. If you want it to be an envvar, tell me. :) > ::: src/Makefile-browser-plugin.am > @@ +9,3 @@ > +libgnome_shell_browser_plugin_la_LIBADD = \ > + $(GNOME_SHELL_LIBS) \ > + $(JSON_GLIB_LIBS) > > JSON_GLIB_LIBS doesn't exist; I also don't think you want to link the browser > plugin against everything in GNOME_SHELL_LIBS - having the separate > BROWSER_PLUGIN_LIBS which has what it needs (json-glib, gio, perhaps) should be > sufficient. Whoops, yeah, that's a hold-over from the integration. Fixed. > @@ +114,3 @@ > + &document); > + if (error != NPERR_NO_ERROR) > + goto doc_error; > > Are you sure that the NPVariant has been initialized in the error case at this > point? I think it probably would be best to do: > > NPVariant document = { NPVariantType_Void }; > > (the initializer also will 0-initialize the unsupplied fields), so you can just > the variants them all unconditionally, and also set NPObject *window to NULL, > and do if (window != NULL) for that and the same for the stirngs, and then you > can have only a single error point, rather than a whole set of them. Much > easier to get right. NPN_ReleaseVariantValue is provided by the browser, but I think it's fairly safe to make the assumption that NPVariantType_Void won't do anything major. See: http://hg.mozilla.org/mozilla-central/file/e5815c156b6c/dom/plugins/base/nsNPAPIPlugin.cpp#l1907 http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/npruntime.cpp#L247
Created attachment 195814 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. https://github.com/magcius/sweettooth-plugin/commit/234a73279b4f3e3536af1bc1aaddc621b5c62b40
Review of attachment 195814 [details] [review]: Mostly good, few style problems and bugs ::: browser-plugin/Makefile.am @@ +1,2 @@ + +# this should really be retrieved from pkg-config Is this true? If it was retrieved from pkg-config, we might a) install out of the install tree b) not be found byr Chrome, epiphany, etc. It seems that this is a "standard directory" (also, stray blank line above) ::: browser-plugin/browser-plugin.c @@ +68,3 @@ +{ + NPError error; + NPVariant result; you need to initialize this or you might release junk on error @@ +109,3 @@ + if (error != NPERR_NO_ERROR) + { + ret = FALSE; it's simpler if you just set ret to TRUE when everything has succeeded. @@ +148,3 @@ + "protocol"); + + if (g_strcmp0 (protocol, "https:")) write != 0 in this case, to make it clear that you are doing a numeric comparison, not a backwards boolean comparison. @@ +383,3 @@ + gsize i; + + for (i = 0; (uuid[i]); i ++) parens here to me are extraneous @@ +428,3 @@ + json = json_generator_to_data (generator, &json_length); + + buffer = funcs.memalloc (json_length); Almost certainly need to add 1 here for the trailing NUL byte @@ +611,3 @@ + } + + buffer = funcs.memalloc (strlen (version)); certainly have to add 1 here for the trailing NUL! @@ +615,3 @@ + { + goto out; + ret = FALSE; Backwards!
Created attachment 195936 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. Fixed review comments and other bugs.
Review of attachment 195936 [details] [review]: This looks good to me, but before we commit it, can you describe what you are planning to do based on my recommendations in: https://mail.gnome.org/archives/desktop-devel-list/2011-August/msg00242.html Thanks!
Created attachment 196149 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. Minor, minor adjustments: clean up unused ALLOW_FOREIGN_ORIGIN #define, and rename manifest to version_tag.
Created attachment 196197 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. Added Uninstall method
Review of attachment 196197 [details] [review]: Looks good to me
Created attachment 196256 [details] [review] Move sweettooth-plugin into gnome-shell tree, rebrand This is a direct import from http://github.com/magcius/sweettooth-plugin . All the source code is the same, as it had no branding. Everything else is just renaming and modifying the "build system" to work with gnome-shell's existing one. Removed ALLOW_INSECURE_HTTP as per review in bug #658612
Review of attachment 196256 [details] [review]: Still good
Attachment 196256 [details] pushed as 52273b6 - Move sweettooth-plugin into gnome-shell tree, rebrand