685155 - Add-on Manager should treat symbolic links the same as proxy files

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kris Maglione [:kmag]]

Symbolic links should be treated exactly as proxy files by the Add-on Manager. They should not have upgrade permissions, and the links should be deleted when the add-on is uninstalled rather than the target directories.

The attached patch implements both of these, and adds tests to that effect for both symlinked and proxy file linked add-ons. The tests unfortunately rely on nsIProcess and ln(1) to create the symlinks, due to a lack of support in GRE. Let me know if you think there is a cleaner way to do this.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Proposed fix

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 558805 [details] [diff] [review]
Proposed fix

Review of attachment 558805 [details] [diff] [review]:
-----------------------------------------------------------------

One thing that'd be super nice, but not required, would be to test that uninstalling an add-on where there is a symlink inside the add-on's files itself works. I think your patch fixes it where previously we'd be recursing through the add-on incorrectly.

Otherwise this looks good, only a - for the couple of test changes.

::: toolkit/mozapps/extensions/test/xpcshell/test_proxies.js
@@ +138,5 @@
> +        // Ensure that valid proxied add-ons were installed properly on
> +        // platforms that support the installation method.
> +        do_check_true(addon != null
> +                        || ADDONS[i].dirId != null
> +                        || ADDONS[i].type == "symlink" && !gHaveSymlinks);

This will incorrectly pass if the add-on is installed when dirId is non-null, maybe something like:

do_check_eq(addon == null, ADDONS[i].dirId != null || ADDONS[i].type == "symlink" && !gHaveSymlinks);

@@ +140,5 @@
> +        do_check_true(addon != null
> +                        || ADDONS[i].dirId != null
> +                        || ADDONS[i].type == "symlink" && !gHaveSymlinks);
> +
> +        if (addon != null) {

Can you add a check that getResourceURI returns paths that point at the real location of the files.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Dave Townsend (:Mossop) from comment #2)
> One thing that'd be super nice, but not required, would be to test that
> uninstalling an add-on where there is a symlink inside the add-on's files
> itself works. I think your patch fixes it where previously we'd be recursing
> through the add-on incorrectly.

Ok, but I'll add a separate test for that. It's irrelevant in the
case of proxied extensions, so I don't think it would fit with
those tests.

> Otherwise this looks good, only a - for the couple of test changes.
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/test_proxies.js
> @@ +138,5 @@
> > +        // Ensure that valid proxied add-ons were installed properly on
> > +        // platforms that support the installation method.
> > +        do_check_true(addon != null
> > +                        || ADDONS[i].dirId != null
> > +                        || ADDONS[i].type == "symlink" && !gHaveSymlinks);
> 
> This will incorrectly pass if the add-on is installed when dirId is
> non-null, maybe something like:
> 
> do_check_eq(addon == null, ADDONS[i].dirId != null || ADDONS[i].type ==
> "symlink" && !gHaveSymlinks);

I was thinking that was best left to another test, but you're right that
it makes sense to have it here too.

> Can you add a check that getResourceURI returns paths that point at the real
> location of the files.

Done.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #3)
> (In reply to Dave Townsend (:Mossop) from comment #2)
> > One thing that'd be super nice, but not required, would be to test that
> > uninstalling an add-on where there is a symlink inside the add-on's files
> > itself works. I think your patch fixes it where previously we'd be recursing
> > through the add-on incorrectly.
> 
> Ok, but I'll add a separate test for that. It's irrelevant in the
> case of proxied extensions, so I don't think it would fit with
> those tests.

Hm. This is easier said than done. As far as I can tell, the only
case where this would be an issue is when trying to remove an
unpacked add-on where a subdirectory lacks write permissions. In
these cases, though, the uninstall fails because the move operations
fail when attempting to move the add-on to a staging directory,
after which we never see a recursive remove.

If you want, I should be able to deal with that by fixing the
permissions when necessary during the move to the staging directory.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Kris Maglione [:kmag] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #3)
> > (In reply to Dave Townsend (:Mossop) from comment #2)
> > > One thing that'd be super nice, but not required, would be to test that
> > > uninstalling an add-on where there is a symlink inside the add-on's files
> > > itself works. I think your patch fixes it where previously we'd be recursing
> > > through the add-on incorrectly.
> > 
> > Ok, but I'll add a separate test for that. It's irrelevant in the
> > case of proxied extensions, so I don't think it would fit with
> > those tests.
> 
> Hm. This is easier said than done. As far as I can tell, the only
> case where this would be an issue is when trying to remove an
> unpacked add-on where a subdirectory lacks write permissions. In
> these cases, though, the uninstall fails because the move operations
> fail when attempting to move the add-on to a staging directory,
> after which we never see a recursive remove.

Sorry for the long delay while I was on vacation. Sounds like it's probably not worth this at this point. Lets just get an updated patch landed.

Comment 6

[Kris Maglione [:kmag]]

Ok. I'll add a simplified version of the test anyway, as it turns out that symlinks were being followed when the directory was being moved to the staging directory prior to deletion and then (correctly) deleted by dir.remove(true).

I'll file another bug about the move to the staging directory failing if the permissions are broken, since it makes the recursiveRemove function somewhat useless.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Updated patch

Updated with the requested tests.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 569529 [details] [diff] [review]
Updated patch

Review of attachment 569529 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Seems this fell through the cracks - sorry!

https://hg.mozilla.org/integration/fx-team/rev/ea0539bb2118

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Backed out - test_proxies.js is failing on OSX:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8194232&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=8192927&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=8194173&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=8193316&tree=Fx-Team


https://hg.mozilla.org/integration/fx-team/rev/ceef67c17cfa

Comment 11

[Geoff Lankow (:darktrojan)]

Bumping this bug because it still needs fixing. *bump!*

Comment 12

[Kris Maglione [:kmag]]

Hm. Any chance those test results are still around anywhere? I never saw the bugmail for Blair's comments.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Kris Maglione [:kmag] from comment #12)
> Hm. Any chance those test results are still around anywhere? I never saw the
> bugmail for Blair's comments.

AFAIK, no :\ Can you push it to Try?

Comment 14

[Kris Maglione [:kmag]]

Attachment: De-bitrotted patch

No, I never get around to requesting access. I de-bitrotted the patch, if you can push it to try.

Comment 15

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #11)
> Bumping this bug because it still needs fixing. *bump!*

And again!

Comment 17

[jgotts]

Snipped from my bug report:

Steps to reproduce:

[jgotts@linux directory-containing-important-files]$ pwd
/home/jgotts/directory-containing-important-files
[jgotts@linux directory-containing-important-files]$ ls
bar  baz  foo

[jgotts@linux extensions]$ pwd
/home/jgotts/.mozilla/firefox/tskjodnk.default/extensions
[jgotts@linux extensions]$ ln -s /home/jgotts/directory-containing-important-files [e-mail]

[Stop and start Firefox (all versions from 10ESR-24.0 and possibly newer versions).]

[jgotts@linux directory-containing-important-files]$ pwd
/home/jgotts/directory-containing-important-files
[jgotts@linux directory-containing-important-files]$ ls
[jgotts@linux directory-containing-important-files]$


Actual results:

Any extension I believe has write access to the extensions directory. I could be wrong on this, and if I'm wrong the bug is less severe but still needs to be fixed.

Any carefully crafted symlink pointing to let's say /tmp will cause Firefox to wipe what the symlink is pointing to.


Expected results:

Firefox should just remove the symlink and never dereference it. This has caused me quite a bit of lost productivity when I didn't check work into our repo and our automatic extension update logic kicked in.

Comment 18

[Kris Maglione [:kmag]]

(In reply to jgotts from comment #17)
> Any carefully crafted symlink pointing to let's say /tmp will cause Firefox
> to wipe what the symlink is pointing to.

Firefox runs with the same privileges, in general, as the owner of its profile directory. I don't think carefully crafted symlinks are an issue.

> Firefox should just remove the symlink and never dereference it. This has
> caused me quite a bit of lost productivity when I didn't check work into our
> repo and our automatic extension update logic kicked in.

This is already the logic used for proxy files, which the above patch extends to symlinks.

Comment 19

[jgotts]

Let's say that I installed a Firefox extension called FunnyCatPictures. FunnyCatPictures is hostile, appears to not work, and the user thinks that it wasn't compatible with his browser. It actually does this:

cd /home/jgotts/.mozilla/firefox/tskjodnk.default/extensions
ln -s /home/jgotts [e-mail]

Upon the next Firefox restart /home/jgotts is erased.

This bug has wiped out entire git trees. It even wipes .[a-zA-Z]*. Firefox extentions being able to write into <profile directory name>/extensions is really iffy to me.

Comment 20

[Dave Townsend [:mossop]]

(In reply to jgotts from comment #19)
> Let's say that I installed a Firefox extension called FunnyCatPictures.
> FunnyCatPictures is hostile, appears to not work, and the user thinks that
> it wasn't compatible with his browser. It actually does this:
> 
> cd /home/jgotts/.mozilla/firefox/tskjodnk.default/extensions
> ln -s /home/jgotts [e-mail]
> 
> Upon the next Firefox restart /home/jgotts is erased.
> 
> This bug has wiped out entire git trees. It even wipes .[a-zA-Z]*. Firefox
> extentions being able to write into <profile directory name>/extensions is
> really iffy to me.

Why would the add-on waste its time by creating that symlink when it can just go ahead and delete those files directly any way?

This bug doesn't give add-ons any more abilities than they already have by more direct means.

Comment 21

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/ce194b7ba385b6603933d3773ef4c7566fac7b7e
Bug 685155: Treat symbolic links the same as proxy files in the Add-on Manager. r=Mossop

Comment 22

[Kris Maglione [:kmag]]

Required surprisingly little de-bitrotting.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce194b7ba385

Comment 24

[Carlos [nohamelin]]

With this change, using a symbolic link to a proxy file itself is not supported anymore: they are always wiped out of the extensions directory. I don't know if it was intended, though it's not a big deal losing this possibility.

Comment 25

[Kris Maglione [:kmag]]

(In reply to Carlos [nohamelin] from comment #24)
> With this change, using a symbolic link to a proxy file itself is not
> supported anymore: they are always wiped out of the extensions directory. I
> don't know if it was intended, though it's not a big deal losing this
> possibility.

It wasn't intended, but that behavior was also never intended to be supported.