528130 - Add password in preferences check to Code Validation Tool

Status: RESOLVED FIXED

Description

[Jorge Villalobos [:jorgev]]

We need a validation for extensions that store passwords in the preferences. It's not that big of a deal, but a warning would be nice.
We could match the string "password", but only on preference files. So this should only be run on
defaults/preferences/*.js
The Help text should be something like "Extensions should use the Password Manager instead of storing passwords in the preferences. Users should be able to manage their passwords in the Password Manager and protect all of them with a Master Password if they choose to."

Comment 1

[Nick Nguyen [:osunick]]

Jorge,

Wil can show you how to do this.  Probably good to learn since you'll be the person who knows best regarding what the validator should do.

Comment 2

[Wil Clouser [:clouserw]]

RJ - do you have time for this?  Otherwise, I'll do it.  Let me know when you know. :)

Comment 3

[RJ Walsh [:rjwalsh]]

Created attachment 414625 [details] [diff] [review]
Fix

Comment 4

[RJ Walsh [:rjwalsh]]

Created attachment 414626 [details] [diff] [review]
Migration and sql changes

Comment 5

[Wil Clouser [:clouserw]]

Comment on attachment 414626 [details] [diff] [review]
Migration and sql changes

25 already exists.  Make it 26 and I'll r+.  And I already ran the 26 version on preview.amo so it should just start working when the next patch goes in.

Comment 6

[Wil Clouser [:clouserw]]

Comment on attachment 414625 [details] [diff] [review]
Fix

This is great stuff, but I think Jorge's explanation is way too long for that spot.  In the actual test we should return "A preferences file references a password."

The longer explanation Jorge gave should go on https://addons.mozilla.org/en-US/firefox/pages/validation

Also, use ___() for the string in validation.php

Thanks RJ!

Comment 7

[Jorge Villalobos [:jorgev]]

How about "Possible insecure password storage in preferences".

Comment 8

[RJ Walsh [:rjwalsh]]

Created attachment 414805 [details] [diff] [review]
v2

It would probably be a good idea to get test #25 into remora.sql

Comment 9

[Wil Clouser [:clouserw]]

Comment on attachment 414805 [details] [diff] [review]
v2

> How about "Possible insecure password storage in preferences".

That does sound better than mine.  RJ, mind changing it to that before you commit?

> It would probably be a good idea to get test #25 into remora.sql

That's a good idea too.  I'll do it.  Thanks for the patch!

Comment 10

[RJ Walsh [:rjwalsh]]

Fixed in r56920 and migration in r56921.  Hope everyone had a happy Thanksgiving!