"UninstallString" for natively installed web apps should be quoted properly

RESOLVED FIXED in Firefox 26

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: jesper, Unassigned)

Tracking

Trunk
Firefox 26
All
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

WebApps cannot be uninstalled if the path to webapp-uninstaller.exe contains spaces such as:
C:\Users\Jesper Staun Hansen\AppData\Roaming\http;m.lanyrd.com\uninstall\webapp-uninstaller.exe


Easily confirmed as navigating to HKEY_USERS\S-1-5-21-3528198288-4018989589-2019204228-1000\Software\Microsoft\Windows\CurrentVersion\Uninstall\http://m.lanyrd.com
then changing UninstallString to place quotes around the string fixes the issue.

There could possible be issues with the other path strings as well, but I do not know how these strings are used.
This is probably a one-line fix.  We'll want to change the line [1] to read:
  subKey.writeStringValue("UninstallString",  "\"" + this.uninstallerFile.path + "\"");

I'm not sure, but it probably makes sense to do the same thing for the `InstallLocation` key.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/WebappsInstaller.jsm?rev=7e21e4ace8b7#375
Tim: is this something you can take on?
Posted patch Patch (obsolete) — Splinter Review
I haven't built or tested this patch, but this should fix the issue.

I'll post back when I've tested.
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #656591 - Flags: review?(felipc)
Comment on attachment 656591 [details] [diff] [review]
Patch

Thanks for looking into this Tim
Attachment #656591 - Flags: review?(felipc) → review+
Comment on attachment 656591 [details] [diff] [review]
Patch

Actually I'm not able to reproduce this bug.

I tested our uninstaller by moving an installation of Favimon to "C:\path with spaces".  I modified the registry entries to point to the new location, but did _not_ add quotes.  I ran the uninstaller and it worked fine.

Additionally, tons of other apps have their uninstallers at locations with spaces in them (especially the "C:\Program Files[...]" variety) and none of them put quotes in their `UninstallString` values.

Jesper, can you provide more precise steps to reproduce?  Or can someone else verify that this is indeed an issue?

Thanks!
Attachment #656591 - Flags: review+
1. https://dl.dropbox.com/u/72157/Webapp.html#details7
2. Install it
3. Windows control panel (Kontrolpanel)
4. Remove programs (Fjern eller rediger et program) - http://i.imgur.com/yFPJh.png
5. Find QR Code Generator in list
6. Remove (Fjern)
7. Dialog appear with text that removing failed - http://i.imgur.com/wka4s.png

Additional steps to confirm missing quotes:
8. open regedit
9. navigate to HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Uninstall\http://classictutorials.com - http://i.imgur.com/hico7.png
10. Edit UninstallString to have quotes around the entire string - http://i.imgur.com/Ly2TD.png
11. Try to remove again from step 6
12. Removal dialog now appears - http://i.imgur.com/RCMkl.png

If it's not the spaces then I have no idea what it is :-)
I created an account on my machine called "Jesper Staun Hansen," installed the "QR Code Generator" app from the dropbox link provided, ran it, then uninstalled it.  I didn't see this issue reproduce so I have to close it as "Works for me."  Please reopen if more information becomes available.  Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
A similar issue was recently discussed in bug 868746.

In case users ever copy+paste the UninstallString into a terminal as part of a social engineering attack, we should quote the UninstallString.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
See Also: → 868746
Summary: WebApps cannot be uninstalled if path to uninstaller contains spaces such as a username → "UninstallString" for natively installed web apps should be quoted properly
Assignee: tabraldes → nobody
Could we land your patch (after unbitrotting)?
Flags: needinfo?(tabraldes)
Sure, that sounds good
Flags: needinfo?(tabraldes)
Priority: -- → P2
I've just updated Tim's patch and it was already reviewed by Felipe. Re-requesting review just to be safe.
Attachment #656591 - Attachment is obsolete: true
Attachment #785823 - Flags: review?(felipc)
Attachment #785823 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/5f0e6a6812e7
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
No longer blocks: 904938
Depends on: 904938
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.