Add more detail to validation FAQ; unsafe JavaScript warnings need more explanation

VERIFIED FIXED in 5.5

Status

addons.mozilla.org Graveyard
Developer Pages
P4
normal
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: Dave Garrett, Assigned: jorgev)

Tracking

unspecified

Details

(Whiteboard: [ReviewTeam], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Follow up to bug 508984.

If the validator is going to have warnings for unsafe JavaScript then it needs to actually say why it's unsafe somewhere. Warnings are not helpful unless they tell you what you're doing wrong and why.

A table should be added to the "Unsafe JavaScript Tests" section of the validator FAQ with a row with an explanation for each set of unsafe JS being tested for, similar to what was added for unsafe settings.
(Assignee)

Comment 1

9 years ago
Taking this one.
Assignee: nobody → jorge
Priority: -- → P4
Target Milestone: --- → 5.5
(Assignee)

Updated

8 years ago
Whiteboard: [required amo-editors]
(Assignee)

Comment 2

8 years ago
Created attachment 420147 [details] [diff] [review]
Patch V1. JS warnings explained in a similar table.

This explains the same warnings as the previous version in the desired table format, with a couple of additions. I also removed the mention to mozIJSSubScriptLoader, which I believe is not being validated. I haven't run into any cases of this being flagged, despite it being used on several add-ons.

I don't have the environment set to test this, but the fix is simple enough that I don't expect problems.
Attachment #420147 - Flags: review?(clouserw)
(Reporter)

Comment 3

8 years ago
The validator FAQ page links to the source, where you can see what is actually checked for down in all_security_filterUnsafeJS().

http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/controllers/components/validation.php?view=markup

$unsafePatterns = array('/nsIProcess/',
'/\.launch\s*\(/',
'/\beval\s*\(/',
'/\bsetInterval\s*\(/',
'/\bsetTimeout\s*\(/',
'/<browser\s*(?![^<>]*type=["\'])[^<>]*>/i',
'/<iframe\s*(?![^<>]*type=["\'])[^<>]*>/i',
'/xpcnativewrappers=/',
'/evalInSandbox/',
'/mozIJSSubscriptLoader/',
'/\bFunction\((?:.*?[\w\d_]|.*?,.*?(?:[\'"]\s*?[+]\s*?[\w_]|[\w\d_]\s*?[+]\s*?[\'"]).*?)\s*?\)/sm',
'/Object\.prototype/',
'/String\.prototype/',
'/Array\.prototype/',
'/wrappedJSObject/',
'/ctypes\.jsm/',
'/@mozilla\.org\/jsctypes/');

So, mozIJSSubscriptLoader should be flagged, theoretically. Either it's just not common or it's missing it for some reason (i.e. in source but not used on production).
(Assignee)

Comment 4

8 years ago
Comment on attachment 420147 [details] [diff] [review]
Patch V1. JS warnings explained in a similar table.

OK, I'm working on a new version that includes all current validations.
Attachment #420147 - Attachment is obsolete: true
Attachment #420147 - Flags: review?(clouserw)
(Assignee)

Comment 5

8 years ago
Created attachment 420164 [details] [diff] [review]
Patch V2. Added all current validations.

This includes all validations in the script, assuming fixes for bug 538005 and bug 538010. Even if those bugs aren't fixed, I think it's better that the text reflects what we should be checking for.
Attachment #420164 - Flags: review?(clouserw)
(Reporter)

Comment 6

8 years ago
Comment on attachment 420164 [details] [diff] [review]
Patch V2. Added all current validations.

This is missing wrappedJSObject and evalInSandbox.
Comment on attachment 420164 [details] [diff] [review]
Patch V2. Added all current validations.

looking great
Attachment #420164 - Flags: review?(clouserw) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 420192 [details] [diff] [review]
Patch V3. Added missing lines.

Added missing lines as per comment #6.
Attachment #420164 - Attachment is obsolete: true
Attachment #420192 - Flags: review?(clouserw)
r59056
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Attachment #420192 - Flags: review?(clouserw) → review+
(Reporter)

Comment 10

8 years ago
Thanks Jorge, this section is wonderfully helpful now. :)

https://preview.addons.mozilla.org/en-US/firefox/pages/validation#help-21
Status: RESOLVED → VERIFIED
(Assignee)

Comment 11

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.