1129306 - Passwords

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Wesley Johnston (:wesj)]

We need a password storage system

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Pull request

From the PR: 

This implements a passwords store using sqlite. It also includes the connection between this and content. That's mostly copied from Firefox's LoginManagerContent.js script. I'll upload a diff to the bug.

My goal for now is to just be careful that 1.) We're never trusting any domains sent from content and 2.) We only fill logins in the top frame for the page. Happy to get lots of feedback on security implications.

Also, this patch lands disabled (the actual saving of form data is commented out in PasswordsManager.swift. Since we don't have any sort of "Do you want to save..." prompt, I decided to just leave this off for now.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Diff with LoginManagerContent.js

Comment 3

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

Comment on attachment 8558941 [details] [review]
Pull request

I see two major issues:

1) we're not being careful escaping and encoding JSON.  You could convince me it's not a concern here, but I'd want a good explanation in the code comments of why not, and it's a huge red flag.

2) I don't see abbreviating password to anything, pretty much ever.  Let's be descriptive!

The database munging looks fine.  I didn't really look at the password JS dump -- I'm not the right person to try to apprehend 600 lines of JS right now.  If it still needs eyes, re-request and I'll set aside time for it.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8558941 [details] [review]
Pull request

Updated the PR

Comment 5

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

Comment on attachment 8558941 [details] [review]
Pull request

Looks good to me.  Rebase, fold down, include bug number in commit message (please), ship it!