"Reset Camino" does not remove Camino-inserted password entries from Keychain

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
OS Integration
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.3, relnote})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.3, relnote

Details

(Whiteboard: l10n)

Attachments

(1 attachment)

fix
4.89 KB, patch
Josh Aas
: review+
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Mark Mentovai
: superreview+
Details | Diff | Splinter Review
We removed that feature because it ate the entire Keychain; this is the placeholder for restoring it when it is able to delete only Camino-added entries that no other app is also using.  (This depends on the Keychain Services API bug, right?)

In light of the "Firefox broke up my engagement" issue, we should also delete the "Keychain Deny List" file from the profile when doing this (that file is not human-editable, so there's no way to change your mind about one site without trashing the entire list; should there be a way?)

Comment 1

12 years ago
(In reply to comment #0)

> (that file is
> not human-editable, so there's no way to change your mind about one site
> without trashing the entire list; should there be a way?)

It should juset be saved in XML format, not binary.

WRT the second pgh of comment 0, see also bug 331652.  However, I think the solution of making the Keychain Deny List be cleared by "Reset Camino" (and not "stored permanently" in the future Private Browsing mode) while making it XML is better.  The file *does* need to be reasonably-easily editable (we fielded a query this week on fixing an accidentally-pressed deny) without requiring us to implement any UI to do so.

Also targeting this for 1.1 atm, since in theory big chunks of both comment 0 pgh 1 and pgh 2 are a Keychain-related bug, and in theory the Keychain-related bugs are getting fixed for 1.1 ;)
Component: General → OS Integration
QA Contact: os.integration
Target Milestone: --- → Camino1.1
Keychain bugs -> 1.2.
Target Milestone: Camino1.1 → Camino1.2
(Assignee)

Comment 4

12 years ago
Taking. We should start setting the kSecCreatorItemAttr so that we can definitively identify passwords we create.  We'll either have to fix this in two stages--one where we start tagging items as we find them, and then another where we re-enable deletion--or else we'll need to do a sweep of all keychain items. Not doing a sweep has the disadvantage that any password you haven't used since stage one lands wouldn't be deleted.

In theory, neither of those things is safe, but in practice it seems unlikely that anyone else uses our somewhat archaic auth/protocol pair. (It would be good to know if bug 320208 ever actually happened to anyone.)
Assignee: mikepinkerton → stuart.morgan
(Assignee)

Comment 5

12 years ago
Apparently bug 320208 was seen in testing, but given that the deletion code was finding items to delete essentially the same way it finds items to fill with, and the old keys we use mean we don't find Safari's keychain items, I don't see how that could have happened. We'll need to understand that before we can do any kind of upgrading of old items, since we don't want to mark existing items as Camino-owned if they aren't.
(Assignee)

Comment 6

12 years ago
(In reply to comment #4)
> In theory, neither of those things is safe, but in practice it seems unlikely
> that anyone else uses our somewhat archaic auth/protocol pair.

At least iDisk uses this pair, which is what prompted bug 320208. They have both a path and crtr though, so that's filterable.

Still, probably best to do incremental upgrades and live with a partial delete, rather than risk catching things we don't own in a big one-pass sweep.
(Assignee)

Comment 7

11 years ago
*** Bug 362690 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

11 years ago
Bug 202337 added creator markers for all new entries we create, so this will be easy to fix. We're not going to be able to clean up any entries made before that change went in though, so we'll have to relnote that or something.
Plus it will be a bit wonky for people moving from Safari permanently, but I don't see any way to be safe for other Safari-sharing cases and please these people, who are probably a very small edge case ;)
Keywords: relnote
(Assignee)

Comment 10

11 years ago
Created attachment 254767 [details] [diff] [review]
fix

I just discovered that I had this in my tree and never posted it. Causes 'Reset Camino' to delete Camnio-stored entries, as well as clearing the deny list. (It also removes some needless complexity from the deny list storage object.)

There's probably no reason we couldn't take this for 1.1
Attachment #254767 - Flags: review?(joshmoz)
(Assignee)

Comment 11

11 years ago
Comment on attachment 254767 [details] [diff] [review]
fix

Smokey, could you put the reset part of this through its paces in your test account?

The only other thing this will need is a strings change like:
"Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies and site cookie permissions, and clear any passwords stored in the keychain by Camino.\n\nYou cannot undo this action.\n";
Attachment #254767 - Flags: review?(alqahira)
This'll need a string change along these lines:

- "Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies, and clear all site cookie permissions.\n\nPasswords saved in your Keychain are not removed.\n\nYou cannot undo this action.\n";

+ "Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies, clear all site cookie permissions, and remove any passwords Camino saved in your Keychain.\n\nYou cannot undo this action.\n";
Flags: camino1.1b1?
Whiteboard: [string changes in comment 11] l10n
Er, ignore that last comment; I hit the wrong button on the conflict page :(
Comment on attachment 254767 [details] [diff] [review]
fix

r=ardissone; this only deleted the 7(!) entries I added or changed since the rewrite.

We'll need to relnote this well, like the rewrite.
Attachment #254767 - Flags: review?(alqahira) → review+

Updated

11 years ago
Attachment #254767 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

11 years ago
Attachment #254767 - Flags: superreview?(mark)

Updated

11 years ago
Attachment #254767 - Flags: superreview?(mark) → superreview+
(Assignee)

Comment 15

11 years ago
(In reply to comment #13)
> Er, ignore that last comment; I hit the wrong button on the conflict page :(

I'm glad you did, since your text is better :)
(Assignee)

Comment 16

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH with text slightly between mine and Smokey's
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
Whiteboard: [string changes in comment 11] l10n → l10n
Flags: camino1.1b1? → camino1.1b1+
Keywords: fixed1.8.1.2 → fixed1.8.1.3
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.