There isn't much to review yet as we are just starting implementation but eventually I would like a review of the new password manager UI architecture in relationship to fission and use of untrusted user content. I'll update this comment if there are more things to review and I'll email pi-request@ when the review can begin.

Thanks jaws. No need for pi-request, just need-info me here when this is ready for me to take a look at the code. I'm looking through the results of the sec-review now to make sure im up to speed, and i see there is some JSM code already in https://searchfox.org/mozilla-central/source/browser/components/aboutlogins so I have a quick skim.

Hi Paul, this is ready for you to take a look at the code.

Notes from the May 21 Security Review meeting: https://docs.google.com/document/d/1V0cwDSRCWOn7v6RohBvAY4topXZJPCPqN7cVMr_2RR8/edit

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)

Hi Paul, this is ready for you to take a look at the code.

Thanks Jared, I'll take a look asap. Just returned from PTO so I'm un-burying myself, but first thing next week if not this week.

Hi Jared, I've spent some time today looking at this. All looks good to me. I still need to write up my testing notes, but I didnt find any issues. Mostly I focused on the threats of:

• a malicious website trying to provide spurious data to trick user, or subvert about:logins behavior
• a compromised web content process trying to abuse the IPC attack surface introduced by this API

The only minor issue i saw was around around input validation - you can enter a string like "javascript://" in the host the UI changes, but it fails to commit the change in the background since that's not a valid origin. Not a security issue though.

I'll write up testing notes first thing next week, just wanted to advise that everything looked good.

Thanks, Paul. Appreciate you letting us know!

Ok i've written up my notes [1] from testing. There are only two items for follow-up:

1. Checking the origin of a message doesn’t actually ensure anything (until Fission)
   So according to gijs, checks like this are currently not effective in general, as the child can load whatver it wants. HOWEVER in this case I think because we have special behavior in e10sUtils.shouldLoadURI for about:logins, that a regular content process should not actually be able to load about:logins (at least not as a top level URI). This item is just for me to follow-up with Gijs and better understand exactly what we can and can NOT trust about the message.target object, inside a ReceiveMessage function.

2. Validation of input can lead to confusing state for user
   Editing a credential in about:logins to have an origin like: “javascript://” bypasses the validation in the page, but then fails the nsURI creation. The error is NOT handled though, and the UI is updated as if the edit was succesful, even though the login item has NOT been modified. Not a security issue on its own, but maybe a confusing state which could be combined with another issue in the future.

[1] https://docs.google.com/document/d/1B7dOnpKWEMlnoQZ2rEcJOEpwRegNLyDsCLqcUHnkxO8/edit#