647109 - Validator should flag the term 'password' in defaults/preference file

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

Description

[Jorge Villalobos [:jorgev] (he/him)]

In the default preferences file that is under the preferences directory, we need to check for the term 'password' and show a warning: "Storing passwords in the preferences is insecure and the Login Manager should be used instead".

Comment 1

[Kris Maglione [:kmag]]

It might be best to just migrate the regexp tests from the old validator. There's no especially good reliable way that I can think of to test for this, but the old tests seemed to quite a few instances, as far as I can recall.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

For this particular test, it only checked for "password" in the old validator. We could widen the search to just "pass", though.

Comment 3

[Matt Basta [:basta]]

What file(s) are we running this on? My preliminary Googleage turned up:

/defaults/preferences/defaults.js

Any others?

Comment 4

[Kris Maglione [:kmag]]

Jorge: Hm. I could have sworn it caught things like setCharPref("password", ... in the past, but I checked the source and I guess it doesn't.

Matt: defaults/preferences/*.js should do.

Comment 5

[Axel Grude [:realRaven]]

There is a little stumbling block for add-on that are specifically written for password management functions such as my own QuickPasswords, then the Saved Password Editor and the Master Passwords+ Extensions. These all work hand in hand with the built in Password Manager and actually do not store passwords themselves - they just make accessing and modification easier.

When I release a new version of QP I usually ignore the warnings on storing "passwords" in the registry (as all my settings start with extensions.quickpasswords.*, as this is the name of the extension)

As long as it doesn't block the submission process completely I am all for it.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #5)
> As long as it doesn't block the submission process completely I am all for it.

It's intended to be a warning, like it was before.

Comment 7

[Matt Basta [:basta]]

There's a pull for this:

https://github.com/mozilla/amo-validator/pull/16

Comment 8

[Matt Basta [:basta]]

Pushed to mozilla/amo-validator

Comment 9

[krupa raj[:krupa]]

Jorgev, is there a test add-on I can verify this with? Thanks!

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

Yes, you can test with this one:
https://addons.allizom.org/en-US/developers/addon/scylighter/file/114906/validation

At least in preview there's no flag yet.

Comment 11

[krupa raj[:krupa]]

verified fixed @ https://addons-next.allizom.org/en-US/developers/upload/6618e3cd722046cda9344bb54f8749de

Jorgev, thanks for providing the test add-on.

Comment 12

[krupa raj[:krupa]]

Attachment: post-fix screenshot

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...