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

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla12
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [backed out])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 558805 [details] [diff] [review]
Proposed fix
Assignee: nobody → maglione.k
Attachment #558805 - Flags: review?(dtownsend)
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.
Attachment #558805 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 3

7 years ago
(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.
(Assignee)

Comment 4

7 years ago
(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.
(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.
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
Created attachment 569529 [details] [diff] [review]
Updated patch

Updated with the requested tests.
Attachment #558805 - Attachment is obsolete: true
Attachment #569529 - Flags: review?(dtownsend)
Comment on attachment 569529 [details] [diff] [review]
Updated patch

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

Awesome
Attachment #569529 - Flags: review?(dtownsend) → review+
Seems this fell through the cracks - sorry!

https://hg.mozilla.org/integration/fx-team/rev/ea0539bb2118
Status: NEW → ASSIGNED
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [fixed-in-fx-team]
Version: unspecified → 12 Branch
Target Milestone: --- → mozilla12
Version: 12 Branch → unspecified
Bumping this bug because it still needs fixing. *bump!*
(Assignee)

Comment 12

6 years ago
Hm. Any chance those test results are still around anywhere? I never saw the bugmail for Blair's comments.
(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?
(Assignee)

Comment 14

6 years ago
Created attachment 718794 [details] [diff] [review]
De-bitrotted patch

No, I never get around to requesting access. I de-bitrotted the patch, if you can push it to try.
Attachment #569529 - Attachment is obsolete: true
(In reply to Geoff Lankow (:darktrojan) from comment #11)
> Bumping this bug because it still needs fixing. *bump!*

And again!
Duplicate of this bug: 930090

Comment 17

5 years ago
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 test@bad.extension.com

[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.
(Assignee)

Comment 18

5 years ago
(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

5 years ago
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 hostile@cat.pictures.com

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.
(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 hostile@cat.pictures.com
> 
> 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.
(Assignee)

Comment 21

2 years ago
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
(Assignee)

Comment 22

2 years ago
Required surprisingly little de-bitrotting.

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce194b7ba385
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED

Comment 24

2 years ago
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.
(Assignee)

Comment 25

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.