AMO review (partly accepted but needs changes)

[minj]

Original issue 892 created by convincedd on 2012-01-22T16:38:30.000Z:

Your add-on, FoxTrick 0.8.2, has been reviewed by an editor and did not meet the criteria for being hosted in our gallery.

Reviewer: Andreas Wagner

Comments: Your version was rejected because of the following problems:

1. The file hash of the jquery library included in your add-on does not match the original jquery file hash of that version. Please include an unmodified copy of jquery from jquery.com so our validator can recognize it.

This version didn't pass full review because of the following issues:

1. Your add-on makes remote, synchronous XMLHttpRequests which have the ability to lock-up the browser UI and are not allowed in public add-ons. Please use asynchronous requests instead.

2. Extending the prototype of native objects like Object, Array and String is not allowed because it can cause compatibility problems with other add-ons or the browser itself.

3. To prevent vulnerabilities, event handlers (like 'onclick' and 'onhover') should always be defined using addEventListener.

Please replace your innerHTML calls by more secure DOM creation methods like dom.createTextNode or node.textContent. We have to check every innerHTML call for remote injection (which is extremly dangerous), this costs us a lot of time and increases your waiting time.

Please fix them and submit again. Thank you.

[minj]

Comment #1 originally posted by CatzHoek on 2012-02-21T21:40:51.000Z:

1. Convincedd updated in be87b42c2f5dfa95c0311cb456be681c569271d8 r8448

[minj]

Comment #2 originally posted by CatzHoek on 2012-02-21T21:57:59.000Z:

1. Convincedd updated in be87b42c2f5dfa95c0311cb456be681c569271d8 r8448 (The JQuery one)

[minj]

Comment #3 originally posted by convincedd on 2012-03-09T01:51:31.000Z:

Your add-on, FoxTrick, 0.9.0.1, has been preliminarily reviewed by an editor and is now available for download in our gallery at https://addons.mozilla.org/addon/foxtrick/

Reviewer: Kris Maglione

Comments: Your preliminary review request has been approved. Due to caching and mirroring on our site, it can take a few hours before your add-on appears public, so please be patient.

However, I have the following issues which should be addressed before your next update:

1. Your add-on creates DOM nodes from HTML strings containing unsanitized data, by assigning to innerHTML or through similar means. Aside from being inefficient, this is a major security risk. For more information, see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion

2. Generating script fragments such as event listeners from unsanitized string data is error prone and poses a major risk of security vulnerabilities. For more information, please see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion#listeners

Thank you.

Your add-on will now appear in search results and categories with some limitations. After 10 days you may request full review to remove these limitations and enable additional features. To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

Mozilla Add-ons https://addons.mozilla.org

Labels: -Priority-Medium, Priority-Critical, Milestone-0.9.1

[minj]

Comment #4 originally posted by CatzHoek on 2012-03-21T21:26:18.000Z:

No comment on the 0.9.0.5 review? Did it go though without feedback?

[minj]

Comment #5 originally posted by convincedd on 2012-03-21T21:31:53.000Z:

the automatic test still shows lots of innerhtml complains. but that's not strictly forbidden, just annoys them to check each of those manually. so, main thing is external html, which i started already:

---

However, I have the following issues which should be addressed before your next update:

1. Generating script fragments such as event listeners from unsanitized string data is error prone and poses a major risk of security vulnerabilities. For more information, please see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion#listeners

Thank you.

[minj]

Comment #6 originally posted by convincedd on 2012-05-17T22:47:56.000Z:

Your add-on, FoxTrick 0.9.2, has been reviewed by an editor and did not meet the criteria for being hosted in our gallery.

Reviewer: Kris Maglione

Comments: Your version was rejected because of the following problems:

1. Generating script fragments such as event listeners and javascript: URLs from unsanitized string data is error prone and poses a major risk of security vulnerabilities. For more information, please see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion#listeners

Firefox has builtin zip support, both through jar: URIs and the nsIZipReader and nsIZipWriter classes. Please use those rather than including third party libraries.

Please fix them and submit again. Thank you.

This version of your add-on has been disabled. You may re-request review by addressing the editor's comments and uploading a new version. To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

Mozilla Add-ons https://addons.mozilla.org

[[ talked to him in the chat. the list of coplaints there was adressed in an 0921 upload (to be reviewed). the zip thing we can do later ]]

[minj]

Comment #7 originally posted by convincedd on 2012-05-18T21:32:54.000Z:

had a another chat. nsizipreader isn't realy of much help for us since it only uses local files (we could save first and unzip later, but that wouldn't be realy elegant either). so, we'll leave the zip part as it is

[minj]

Comment #8 originally posted by minj on 2012-07-22T20:57:30.000Z:

Our status is 'preliminary-reviewed'. What are the odds to upgrade it? We ditched ZIP AFAIK?

[minj]

Comment #9 originally posted by convincedd on 2012-07-23T11:24:15.000Z:

dunno. but maybe for fun we should try with on particular nice version

[minj]

Comment #10 originally posted by minj on 2012-11-25T14:50:01.000Z:

#812 has been merged into this issue.

[minj]

Comment #11 originally posted by minj on 2012-12-19T10:10:03.000Z:

For future reference

Illegal or deprecated access to the scriptloader global is a non-issue: https://groups.google.com/d/topic/mozilla.dev.extensions/TMFWetkVEDQ/discussion

All of these have unprefixed counter-parts as of 0.12.0 -moz-linear-gradient -moz-transition -moz-perspective -moz-transform -moz-backface-visibility

Labels: -Platform-All, -Milestone-0.9.1, Platform-Firefox, Milestone-1.0

[minj]

Comment #12 originally posted by minj on 2012-12-19T10:22:10.000Z:

Toolbaritem.js makes us use E4X which is deprecated (contacted the original dev)

[minj]

Comment #13 originally posted by minj on 2012-12-19T10:32:35.000Z:

jester.js triggers some nonsensical warnings due to using 'fns[eventExt].constructor === Function'

seems stupid to me, why not use typeof(foo) == 'function'?

contacted the original dev as well

[minj]

Comment #14 originally posted by minj on 2012-12-28T10:23:06.000Z:

Your add-on, FoxTrick, 0.11.0.2, has been preliminarily reviewed by an editor and is now available for download in our gallery at https://addons.mozilla.org/addon/foxtrick/

Reviewer: Nils Maier

Comments: Your preliminary review request has been approved.

However, here are some important issues that you should address in the future (prelim. reviews):

• Naming your inner jar with a version number isn't really compatible with our tools and hence requires a lot more work to review your add-on. We would appreciate if you stuck to a single file name. Better yet, do not use em:unpack + inner jar at all. Firefox < 10 is long dead ;)
• E4X aka. inline XML is dead and will be entirely removed with Firefox 20. https://developer.mozilla.org/en-US/docs/E4X
• There are multiple places where your code uses parseInt(expr) without a radix.
• When including new third-party libraries (like yaml.js), please preferably use an unminified version if the library is less popular than jquery and friends, if available, and at least indicate where the original code can be retrieved from, so that we do not have to track this down ourselves. Right now our tools only detect a limited set of third-party libraries, and all other undetected libraries need to be reviewed manually. https://github.com/mozilla/amo-validator/blob/master/extras/jslibfetcher.py

Thank you.

[minj]

Comment #15 originally posted by minj on 2012-12-28T10:23:40.000Z:

• Naming your inner jar with a version number isn't really compatible with our tools and hence requires a lot more work to review your add-on. We would appreciate if you stuck to a single file name. Better yet, do not use em:unpack + inner jar at all. Firefox < 10 is long dead ;) We are using this as a work-around (see https://bugzilla.mozilla.org/show_bug.cgi?id=822386) We don't plan to support FF<10 for long but jar has a significant impact on the compression rate.
• E4X aka. inline XML is dead and will be entirely removed with Firefox 20. https://developer.mozilla.org/en-US/docs/E4X This is needed by the toolbaritem.js lib. For now we've informed the original dev about the problem
• When including new third-party libraries (like yaml.js), please preferably use an unminified version if the library is less popular than jquery and friends, Is the minified version of yaml.js OK in the future now?

[minj]

Comment #16 originally posted by minj on 2012-12-28T10:55:25.000Z:

Replied to that bug.

We don't plan to support FF<10 for long but jar has a significant impact on the compression rate.

Yeah, due to the massive number of files and non-solid nature of ZIP the increase in download size is a valid concern. I just checked and the XPI size would go up to 4MB+, which wouldn't be that great.

• E4X aka. inline XML is dead and will be entirely removed with Firefox 20. https://developer.mozilla.org/en-US/docs/E4X This is needed by the toolbaritem.js lib. For now we've informed the original dev about the problem

Just saying... Should the upstream not react in time you'll need to fix that yourself. ;)

• When including new third-party libraries (like yaml.js), please preferably use an unminified version if the library is less popular than jquery and friends, Is the minified version of yaml.js OK in the future now?

Yes and no. It should OK for now since I took the time to track down the unminified sources and review them. Other editors should pick up on that unless the file is changed. But once you change the file use an unminified copy please. Then again yaml.js doesn't look like it changes a lot.

[minj]

Comment #17 originally posted by minj on 2012-12-28T11:50:41.000Z:

<empty>

Blockedon: #1033, #1034

[minj]

Comment #18 originally posted by minj on 2013-03-07T21:50:06.000Z:

killed the e4x (issue 1033)

[minj]

Comment #19 originally posted by foxtrickdev on 2013-03-14T18:35:27.000Z:

JavaScript Compile-Time Error Warning: A compile-time error in the JavaScript halted validation of that file. Message: syntax error chrome/foxtrick-0.12.0.jar/content/overlay.xul

nsISound.play should not be used. Warning: The nsISound.play function is synchronous, and thus freezes the interface while the sound is playing. It be avoided in favor of the HTML5 audio APIs. chrome/foxtrick-0.12.0.jar/content/util/misc.js

complaints about mouseover for htev and BlobBuilder

Status: Started

[minj]

Comment #20 originally posted by minj on 2013-06-02T17:17:08.000Z:

<empty>

Labels: Maintainability

[minj]

Comment #21 originally posted by minj on 2013-11-01T22:09:18.000Z:

OK, so I've found a workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=822386

We're back in business...

[minj]

Comment #22 originally posted by minj on 2013-11-01T22:09:43.000Z:

5e79ea99ca64d7f91fde80c09a237722c4eae011 r12394

[minj]

Comment #23 originally posted by minj on 2013-11-03T02:15:47.000Z:

https://wiki.mozilla.org/AMO:Editors/EditorGuide/AddonReviews

[minj]

Comment #24 originally posted by minj on 2013-11-03T03:31:11.000Z:

Things to consider:

• private mode
• privacy policy and HY
• remote innerHTML
• MutationEvents
• local innerHTML
• setTimeout (possibly n/a as we don't use strings)?
• storing oauth token in prefs?
• synchronous requests in chrome?

No Opera12, FF13, Safari 5

[minj]

Comment #25 originally posted by minj on 2013-11-03T03:33:54.000Z:

How could I forget

• test account

[minj]

Comment #26 originally posted by minj on 2013-11-03T05:26:38.000Z:

<empty>

Blockedon: #1158

[minj]

Comment #27 originally posted by minj on 2013-11-03T05:30:27.000Z:

<empty>

Blockedon: #950

[minj]

Comment #28 originally posted by minj on 2013-11-03T22:04:51.000Z:

other performance stuff:

• ditch onreadystatechange
• Remove Event Listeners when done
• chromeWindow.setTimeout leaks?
• optimize css: use as much id-only and class-only selectors as possible use ? lazy loading (lazyGetters) async startup JSMs https://developer.mozilla.org/en-US/docs/Extensions/Common_causes_of_memory_leaks_in_extensions popupshowing nsICryptoHash

[minj]

Comment #29 originally posted by minj on 2014-02-15T19:30:12.000Z:

Apparently it's OK to store oAuth tokens in prefs.

What is highly desirable is loading chrome resources in async mode. Should not be a huge issue in preferences page but initing prefs, locales, world data and css asynchronously for content would be a huge PITA. Not to forget contentscriptinit runs every time on chrome :/

[minj]

Comment #30 originally posted by minj on 2014-02-16T01:56:39.000Z:

So...

• MOs
• Strip innerhtml
• improve css
• privacy
• clean listeners: dom, self-rm
• onreadystatechange
• async init