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

VERIFIED FIXED in 6.0.6

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P2
normal
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: jorgev, Assigned: basta)

Tracking

unspecified
6.0.6

Details

(Whiteboard: [ReviewTeam], URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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".
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.
(Reporter)

Comment 2

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

Comment 3

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

/defaults/preferences/defaults.js

Any others?
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.
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.
(Reporter)

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
There's a pull for this:

https://github.com/mozilla/amo-validator/pull/16
Target Milestone: 6.0.5 → 6.0.6
(Assignee)

Comment 8

7 years ago
Pushed to mozilla/amo-validator
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 9

7 years ago
Jorgev, is there a test add-on I can verify this with? Thanks!
(Reporter)

Comment 10

7 years ago
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

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

Jorgev, thanks for providing the test add-on.
Status: RESOLVED → VERIFIED

Comment 12

7 years ago
Created attachment 527350 [details]
post-fix screenshot
(Reporter)

Comment 13

6 years ago
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.