Add-ons/Reviewers/Guide/Reviewing: Difference between revisions
(Proofreading) |
(Copy edits) |
||
| Line 2: | Line 2: | ||
'''Reviewer Intro Tour:''' ask your guide to select an add-on for you to review. Don't submit your first review without their pre-approval! | '''Reviewer Intro Tour:''' ask your guide to select an add-on for you to review. Don't submit your first review without their pre-approval! | ||
Add-on reviewers have a big responsibility. We need to ensure add-ons are safe to use, good quality, and clearly presented to our users. We also need to make sure developers get quick, clear, and actionable reviews. | Add-on reviewers have a big responsibility. We need to ensure add-ons are safe to use, of good quality, and clearly presented to our users. We also need to make sure developers get quick, clear, and actionable reviews. | ||
AMO is designed so that popular | AMO is designed so that popular add-ons bubble to the top search rankings. The full and preliminary review levels also create two layers that allow us to list add-ons that are not quite ready for general consumption. Since we have good filtering mechanisms on AMO, our general policy is to '''only reject when strictly necessary'''. Rejection is necessary when an add-on has security issues, doesn't meet our content policies, or fits one of the special cases spelled out later in this guide. If an add-on doesn't meet the criteria for rejection, it should at the least be granted preliminary approval. | ||
== Policies and actions == | == Policies and actions == | ||
The rest of this page explains | The rest of this page explains policies and recommended actions for common add-on issues. Add-ons must be reviewed '''completely''', and all issues written down as they are found. Once the review is complete, the sum of all noted issues is used to determine what the review resolution should be. Regardless of the result, all issues should be included in the notes sent to developers. | ||
= Step 1: Review Add-on Metadata = | = Step 1: Review Add-on Metadata = | ||
Before getting started, here's a very important legal note: reviews '''must not''' involve checking for copyright or trademark violations. You should not take '''any''' action on an add-on because you suspect it copies code from others without permission or may otherwise infringe somebody else's copyright or trademark. | Before getting started, here's a very important legal note: reviews '''must not''' involve checking for copyright or trademark violations. You should not take '''any''' action on an add-on because you suspect it copies code from others without permission, or may otherwise infringe somebody else's copyright or trademark. We are granted special legal protection against liability for copyright infringement by the [http://en.wikipedia.org/wiki/Digital_Millennium_Copyright_Act DMCA], but only under strict conditions, which include a requirment that we not screen the content that we host for copyright issues. | ||
If you have any concerns about the legality or legitimacy of an add-on, please email amo-editors AT mozilla DOT org. | If you have any concerns about the legality or legitimacy of an add-on, please email amo-editors AT mozilla DOT org. | ||
| Line 26: | Line 26: | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on doesn't meet AMO content policies. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on doesn't meet AMO content policies. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These are add-ons | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These are add-ons which: | ||
* Embed known spyware or malware. | |||
* Engage in illegal activities, such as click fraud. | |||
* Have the primary purpose of facilitating illegal activities, such as access online gambling or pirated materials, including warez and unlicensed music/video downloads. | |||
* Have the direct purpose of facilitating access to pornography. This includes, for instance add-ons which specifically interact with, or direct users to, porn sites, but does not include, for instance, image downloaders why may, but need not, be used for pornographic purposes. | |||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on uses Mozilla trademarks in its name. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on uses Mozilla trademarks in its name. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | We allow | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | We allow the use of certain trademarks when appended to the name of an add-on (e.g., "Video downloader for Firefox", "Inspector for Mozilla") in such a way as not to cause confusion as to the origin of the add-on. We do not allow uses which may suggest that the add-on is a Mozilla product (e.g., "Firefox downloader", "My Mozilla downloader", "Firefox++"). When in doubt, ask on the list or request super-review. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on version number is identical to previous version number. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on version number is identical to previous version number. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Due to | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Due to the inner workings of our content delivery network, uploads which re-use a version number will not reliably replace the previous version. Every new submission needs a new version number. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on doesn't provide enough information in its descriptions for users to figure out what it does. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on doesn't provide enough information in its descriptions for users to figure out what it does. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request more information | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request more information | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Explain clearly why the provided description is insufficient, and what needs to be done to improve it. Once the developer has corrected the deficiencies and contacted us, the review may be completed without a further upload. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on name and/or code appear copied or very similar to a popular add-on (like AdBlock Plus). | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The add-on name and/or code appear copied or very similar to a popular add-on (like AdBlock Plus). | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These add-ons | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These add-ons often include malicious code, and may trick users into mistaking them for the original. We accept add-on forks, and add-ons with similar features to other add-ons, but we need to be very careful to ensure that they do not mislead users. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Other copyright suspicions. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Other copyright suspicions. | ||
| Line 60: | Line 63: | ||
In some cases, however, reviewers should deny full review to add-ons due to their usefulness. Add-ons that commonly fall into this category are: | In some cases, however, reviewers should deny full review to add-ons due to their usefulness. Add-ons that commonly fall into this category are: | ||
* Add-ons that help access very specific webpages. For example, an add-on that makes it easier to use an internal business site or a university library. | * Add-ons that help access very specific webpages. For example, an add-on that makes it easier to use an internal business site or a university library. | ||
* Add-ons targeted to a limited geographical area. For example, an add-on for a local | * Add-ons targeted to a limited geographical area. For example, an add-on for a small, local newspaper or radio station. | ||
* Add-ons that don't do more than link to websites through buttons or menu items. | * Add-ons that don't do more than link to websites through buttons or menu items. | ||
* YouTube (or other) video downloaders. We already have too many add-ons of this kind, with little to no distinction between them. Unless the add-on provides | * YouTube (or other) video downloaders. We already have too many add-ons of this kind, with little to no distinction between them. Unless the add-on provides especially innovative features, it should not be granted full approval. | ||
When in doubt, don't apply this policy. | When in doubt, don't apply this policy. | ||
| Line 84: | Line 87: | ||
! style="border-bottom: 2px solid black" scope="col" | Notes | ! style="border-bottom: 2px solid black" scope="col" | Notes | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using eval, Function(), setTimeout, setInterval to evaluate JS code. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using <code>eval()</code>, <code>Function()</code>, <code>setTimeout()</code>, or <code>setInterval()</code> to evaluate JS code. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | eval | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | <code>eval</code> may sometimes be allowed when it is used carefully to patch Firefox functions with local code. <code>setTimeout</code> and <code>setInterval</code> may be used with hardcoded JS strings, but using closures is preferred. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Remote script injection. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Remote script injection. | ||
| Line 92: | Line 95: | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-ons can use data-only APIs, but should never download and execute remote code, not even in the scope of a webpage. Any use of the <script> tag (like createElement("script")) needs to be carefully analyzed. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-ons can use data-only APIs, but should never download and execute remote code, not even in the scope of a webpage. Any use of the <script> tag (like createElement("script")) needs to be carefully analyzed. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | <browser> or <iframe> elements with no type. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | <code><browser></code> or <code><iframe></code> elements with no <code>type</code> attribute, used in privileged documents. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | See the [https://developer.mozilla.org/en/XUL/iframe#a-browser.type iframe documentation]. The type | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | See the [https://developer.mozilla.org/en/XUL/iframe#a-browser.type iframe documentation]. The type must be one of <code>"content"</code>, <code>"content-targetable"</code>, or <code>"content-primary"</code>. This must be done ''before'' anything is loaded on that iframe. If the iframe or browser is used to load only chrome content, and it is clear from the code that it will never load anything else, <code>type="chrome"</code> may be used when necessary. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Inserting remote content with innerHTML. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Inserting remote content with <code>innerHTML</code>. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The canned response points to the [https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/XUL_School/DOM_Building_and_HTML_Insertion | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | The canned response points to the [https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/XUL_School/DOM_Building_and_HTML_Insertion preferred documentation] about this topic. Assignments to <code>innerHTML</code> will result in the execution of any JavaScript code present in the injected string, so it needs to be very clear that any such strings are safe and sane. The documentation offers various methods of ensuring this. Since this issue can often be confusing to developers, make sure to include a reference to a code file and line where this occurs. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using DOM Mutation events. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using DOM Mutation events. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | [https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver Mutation Observers] are the recommended alternative. For instances of this issue which were approved in past reviews, this should only result in a warning that it must be fixed. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Native object prototype extension / Using Prototype library. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Native object prototype extension / Using the Prototype library. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | This only applies in XUL overlays, where the prototype extension affects the prototypes used by Firefox code and other overlays. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | This only applies in XUL overlays, where the prototype extension affects the prototypes used by Firefox code and other overlays. | ||
| Line 110: | Line 113: | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Storing passwords or other sensitive user data in the preferences. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Storing passwords or other sensitive user data in the preferences. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Passwords and other sensitive data should be stored in the login service rather than in preferences. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Changing Firefox preferences without user consent. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Changing Firefox preferences without user consent. | ||
| Line 116: | Line 119: | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These include: network preferences, update system preferences, homepage, User Agent string. They also must be restored to their previous values when the add-on is uninstalled. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | These include: network preferences, update system preferences, homepage, User Agent string. They also must be restored to their previous values when the add-on is uninstalled. | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Changing security preferences, permissions, certificates (nsIX509CertDB). | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Changing security preferences, permissions, certificates (<code>nsIX509CertDB</code>). | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | ||
|- style="vertical-align: top;" | |- style="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using nsIProcess. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using <code>nsIProcess</code>. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Request super-review | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | ||
| Line 130: | Line 133: | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Localization errors. | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Localization errors. | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Ignore | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Ignore | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Errors which result in breakage of user interfaces should result in rejection when built-in Firefox UIs are rejected, and preliminary review when only add-on interfaces are affected. Otherwise, they should be ignored. | ||
|} | |} | ||
Revision as of 23:02, 26 January 2015
Performing a Review
Reviewer Intro Tour: ask your guide to select an add-on for you to review. Don't submit your first review without their pre-approval!
Add-on reviewers have a big responsibility. We need to ensure add-ons are safe to use, of good quality, and clearly presented to our users. We also need to make sure developers get quick, clear, and actionable reviews.
AMO is designed so that popular add-ons bubble to the top search rankings. The full and preliminary review levels also create two layers that allow us to list add-ons that are not quite ready for general consumption. Since we have good filtering mechanisms on AMO, our general policy is to only reject when strictly necessary. Rejection is necessary when an add-on has security issues, doesn't meet our content policies, or fits one of the special cases spelled out later in this guide. If an add-on doesn't meet the criteria for rejection, it should at the least be granted preliminary approval.
Policies and actions
The rest of this page explains policies and recommended actions for common add-on issues. Add-ons must be reviewed completely, and all issues written down as they are found. Once the review is complete, the sum of all noted issues is used to determine what the review resolution should be. Regardless of the result, all issues should be included in the notes sent to developers.
Step 1: Review Add-on Metadata
Before getting started, here's a very important legal note: reviews must not involve checking for copyright or trademark violations. You should not take any action on an add-on because you suspect it copies code from others without permission, or may otherwise infringe somebody else's copyright or trademark. We are granted special legal protection against liability for copyright infringement by the DMCA, but only under strict conditions, which include a requirment that we not screen the content that we host for copyright issues.
If you have any concerns about the legality or legitimacy of an add-on, please email amo-editors AT mozilla DOT org.
Policies and Actions
| Issue | Action | Notes |
|---|---|---|
| The add-on doesn't meet AMO content policies. | Reject | These are add-ons which:
|
| The add-on uses Mozilla trademarks in its name. | Reject | We allow the use of certain trademarks when appended to the name of an add-on (e.g., "Video downloader for Firefox", "Inspector for Mozilla") in such a way as not to cause confusion as to the origin of the add-on. We do not allow uses which may suggest that the add-on is a Mozilla product (e.g., "Firefox downloader", "My Mozilla downloader", "Firefox++"). When in doubt, ask on the list or request super-review. |
| The add-on version number is identical to previous version number. | Reject | Due to the inner workings of our content delivery network, uploads which re-use a version number will not reliably replace the previous version. Every new submission needs a new version number. |
| The add-on doesn't provide enough information in its descriptions for users to figure out what it does. | Request more information | Explain clearly why the provided description is insufficient, and what needs to be done to improve it. Once the developer has corrected the deficiencies and contacted us, the review may be completed without a further upload. |
| The add-on name and/or code appear copied or very similar to a popular add-on (like AdBlock Plus). | Request super-review | These add-ons often include malicious code, and may trick users into mistaking them for the original. We accept add-on forks, and add-ons with similar features to other add-ons, but we need to be very careful to ensure that they do not mislead users. |
| Other copyright suspicions. | No action | We have a legal obligation not to take action. See note at the beginning of this section. |
| Missing Privacy Policy when necessary. | Preliminary review | A Privacy Policy is required if an add-on sends any user information to a remote server, even if it is not personally-identifying. Even stats pings require a Privacy Policy. |
Add-on Relevance
Add-ons must not be rejected because a reviewer doesn't find them useful. We let AMO users make that call, and add-ons that aren't very useful won't gain much usage and have low search rankings, so they'll stay mostly out of the way.
In some cases, however, reviewers should deny full review to add-ons due to their usefulness. Add-ons that commonly fall into this category are:
- Add-ons that help access very specific webpages. For example, an add-on that makes it easier to use an internal business site or a university library.
- Add-ons targeted to a limited geographical area. For example, an add-on for a small, local newspaper or radio station.
- Add-ons that don't do more than link to websites through buttons or menu items.
- YouTube (or other) video downloaders. We already have too many add-ons of this kind, with little to no distinction between them. Unless the add-on provides especially innovative features, it should not be granted full approval.
When in doubt, don't apply this policy.
Step 2: Automatic validation
We have a extensive set of tests that identify common bad practices and possible security problems with add-on code. Reviewers must run the code validator and inspect the results when performing a review. Each Add-on History entry has a validation link, and you'll want to validate the latest one.
Clicking on the link will take you to the validation page, where the automatic code validator will run for that version of the add-on and then the results will be displayed. We recommend opening this link in a new tab.
If the validator shows a validation error and no results, it's possible that reloading the validation page will fix the problem. If the problem persists contact the review team on the #amo-editors IRC channel or pick a different add-on to review.
Policies and actions
| Issue | Action | Notes |
|---|---|---|
Using eval(), Function(), setTimeout(), or setInterval() to evaluate JS code.
|
Reject | eval may sometimes be allowed when it is used carefully to patch Firefox functions with local code. setTimeout and setInterval may be used with hardcoded JS strings, but using closures is preferred.
|
| Remote script injection. | Reject | Add-ons can use data-only APIs, but should never download and execute remote code, not even in the scope of a webpage. Any use of the <script> tag (like createElement("script")) needs to be carefully analyzed. |
<browser> or <iframe> elements with no type attribute, used in privileged documents.
|
Reject | See the iframe documentation. The type must be one of "content", "content-targetable", or "content-primary". This must be done before anything is loaded on that iframe. If the iframe or browser is used to load only chrome content, and it is clear from the code that it will never load anything else, type="chrome" may be used when necessary.
|
Inserting remote content with innerHTML.
|
Reject | The canned response points to the preferred documentation about this topic. Assignments to innerHTML will result in the execution of any JavaScript code present in the injected string, so it needs to be very clear that any such strings are safe and sane. The documentation offers various methods of ensuring this. Since this issue can often be confusing to developers, make sure to include a reference to a code file and line where this occurs.
|
| Using DOM Mutation events. | Preliminary review | Mutation Observers are the recommended alternative. For instances of this issue which were approved in past reviews, this should only result in a warning that it must be fixed. |
| Native object prototype extension / Using the Prototype library. | Preliminary review | This only applies in XUL overlays, where the prototype extension affects the prototypes used by Firefox code and other overlays. |
| Storing passwords or other sensitive user data in the preferences. | Preliminary review | Passwords and other sensitive data should be stored in the login service rather than in preferences. |
| Changing Firefox preferences without user consent. | Preliminary review | These include: network preferences, update system preferences, homepage, User Agent string. They also must be restored to their previous values when the add-on is uninstalled. |
Changing security preferences, permissions, certificates (nsIX509CertDB).
|
Request super-review | |
Using nsIProcess.
|
Request super-review | |
| Using JS c-types. | Request super-review | |
| Localization errors. | Ignore | Errors which result in breakage of user interfaces should result in rejection when built-in Firefox UIs are rejected, and preliminary review when only add-on interfaces are affected. Otherwise, they should be ignored. |
There are many other validation flags of varying importance. If you're unsure about which action to take, please ask on the mailing list.
Step 3: Code Review
All add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, which is why we have a review team.
All Add-on History entries have a View Contents link that take you to the code browser page. Updates also have a Compare link, which will show you the code with the changed sections highlighted. For updates, the compare link should be used. Validation results are integrated into the code viewer, so you can see validation warnings in context. It's okay if you prefer to use other tools for code analysis and diffing.
Libraries, frameworks and other unreadable code
It's very common for add-ons to use libraries or frameworks such as jQuery or Bootstrap. Some add-ons use complex frameworks like Kango, usually to achieve cross-browser compatibility. Finally, the Add-ons SDK generates various files around the actual add-on code. Our code validator will try to detect them. If detected, this code won't generate validator warnings and it will be greyed out in the code viewer.
If a library or framework isn't detected, it can mean that it was either modified by the developer (and the add-on should be rejected), or there's a problem in our detection code, in which case it should be brought up on the mailing list. Library or framework code that has been detected as valid doesn't need to be reviewed.
Aside from libraries, many add-ons can include minified, obfuscated or compiled code. Since this code can't be easily reviewed without the original sources, only admin reviewers can review them. The Add-on History entry should indicate if the source code has been provided by the developer. If that's not the case, you can use the canned info request.
Policies and Actions
| Issue | Action | Notes |
|---|---|---|
| Remote code download or execution, custom update code. | Reject | As explained in the validation section, no remote code execution is allowed. |
| Using plain HTTP for security-sensitive operations. | Reject | Sending passwords over unprotected HTTP if there's a secure alternative. Sending passwords in the URL or other headers than POSTDATA. Performing any security-sensitive operations over HTTP when there's a secure alternative. |
| Using non-release version of Add-ons SDK. | Preliminary review | The validator can detect many experimental versions of the SDK, but that doesn't make it OK to use in public add-ons. Public add-ons should use release versions of the SDK. Others should only get preliminary review. |
| Using outdated Add-ons SDK library. | Preliminary review | We accept the two most recent minor releases of the Add-on SDK. If the validator warns that the add-on is using an outdated SDK version, it is acceptable for full review if it is one release older than the suggested release. Otherwise, it is only eligible for preliminary review. |
| Bad or no namespacing. | Preliminary review | All scripts that are included in the main window overlay should have proper namespacing to avoid name conflicts with other add-ons. The name should normally correspond to the add-on name in order to guarantee its uniqueness. |
| Preference names without "extensions." or "services.sync.extensions." prefix. | Preliminary review | Add-on preferences should use the "extensions." prefix, and should also have a reasonable namespace (like "extensions.myVideoDownloader."). If the add-on is already public, this rule should be ignored. |
| Privacy issues. | Preliminary review | An add-on can claim to work with a popular website like Twitter, but then send the user data through some other site, most likely owned by the developer. There needs to be a justified reason to handle user data in this manner, and the privacy policy and add-on descriptions need to be very clear about this. Passwords should never be handled in this way, and they should only be transmitted directly to the original API provider (Reject otherwise). |
| Performance problems. | Preliminary review | Synchronous (non-local) HTTP requests, synchronous SQL queries, noticeably inefficient code, random UI freezes, loading large amounts of JS code directly in overlays. |
| Writing files outside of the profile folder. | Preliminary review | Some add-ons need helper files like SQL DBs or logs. Those files must be written in the profile folder, not the extension installation folder or other locations. |
| Bootstrapped or SDK add-on doesn't clean up after itself. | Preliminary review | Some things to look for: the add-on must not require a restart for any of its features to fully work, must not require a restart after being disabled or uninstalled, must unregister all observers and remove all UI when disabled or uninstalled. More details. |
| New version doesn't follow previous review requests. | Preliminary review | If the author keeps submitting the same version, contact the mailing list. |
| Using non-standard preferences UI. | Add note | If an add-on has a preferences UI, it should use one of the supported methods. |
| Duplicate / hidden files or folders. | Add note | See canned response. |
In general you should apply your judgement and try to identify code that may appear suspicious or out of place. Try to understand what everything does and how it all fits together.
If the add-on version is not nominated for full review, or if by this point you have sufficient information to deny the add-on full review, you can stop reviewing and submit what you have.
Step 4: Feature Review
The last step in a review is to install and test the add-on. This step is only necessary for an add-ons that are requesting Full Review.
Add-ons are normally cross-platform, in which case there will only be a single file to review. If the add-on is offered for a limited number of platforms or has different files for different platforms, there will be individual links for each one in the Add-on History entry. In this case all supported platforms should be tested.
Regarding application support, you don't need to test the add-on for all of them. If the add-on supports Firefox and others, it's OK to only test on Firefox.
Testing setup
- Always use a separate profile for testing, never your main profile. See Setting up an extension development environment.
- Ideally you should perform your tests in a virtual machine. It is always useful in case you need to test in multiple platforms. VirtualBox is free and works well.
- The Extension Test add-on helps detect loose variables and DOM IDs, prototype extension, dangerous category registration, and other difficult to spot problems.
- Test with the Browser Console always open, and look for errors or log messages generated by the add-on.
Policies and actions
| Issue | Action | Notes |
|---|---|---|
| Security violations. | Reject | Adding HTTP content to HTTPS pages. If the add-on injects content like iframes or images, make sure to visit HTTPS sites the add-on supports and look for any security warnings in the URL bar. |
| No Surprises violation | Preliminary review | Changing homepage, default search provider, including unexpected ads or content changes without explicit user opt-in. |
| Privacy violations. | Preliminary review | Incorrect or insufficient privacy policies, not respecting Private Browsing Mode. |
| Showing a modal dialog at startup. | Preliminary review | Opening modal (blocking) dialogs at startup is not allowed. Non-modal dialogs, separate windows or new tabs are allowed. |
| Errors in the Browser Console. | Preliminary review | Make sure the errors only occur with the add-on installed and are generated from add-on code and not Firefox code. In the latter case, it should only be noted. |
| Add-on is very hard to use even with instructions. | Preliminary review | If the testing instructions are missing, use Request more information. If even then it is too hard to use, give Preliminary review. |
| Toolbar buttons are not customizable. | Preliminary review | It must be possible to remove add-on buttons from the toolbar and move them to the menu panel. |
| Affiliate linking. | Preliminary review | Some add-ons add affiliate codes to Amazon links (or similar) in order to make money. At the moment we allow this as long as (1) the add-on follows the No Surprises policy, (2) the feature doesn't replace or remove any existing affiliate codes, (3) the affiliate codes aren't inserted in the merchant website's links (inserting Amazon affiliate codes in Amazon.com pages). |
| Requires third party software or paid registration. | Request super-review | |
| Inserts ads into content. | Request super-review | The rules in these cases are complex. They need to be clearly labeled as coming from the add-on (otherwise Prelim). They can't remove or replace existing ads (otherwise Reject). They need to follow No Surprises (otherwise Prelim). And there are also security concerns (Reject) and privacy concerns (Prelim). |
Other tests to perform:
- Test all add-on features, within reason. If there too many, focus on the main features.
- Remove all added toolbar buttons, disable all added toolbars, and restart the browser. Make sure that buttons and toolbars are all removable and do not reappear on restart. Make sure that missing toolbar buttons to not cause errors to appear in the Error Console.
- Open the Customize Toolbar dialog and make sure that all buttons have appropriate icons and label text.
- Open the add-on's preferences window, from the Add-ons Manager and elsewhere, and verify that preference changes apply properly. Make sure the window fits all of its contents (a common problem in Mac OS).
Step 5: Resolution
Choose the appropriate resolution and include all of your notes. Make sure you use a corteous and professional tone, and be as helpful as you can when pointing out problems or areas for improvement. The canned responses are very useful in formatting a response. Once you're ready, click the Save button.
Reviewer Intro Tour: remember to ask the guide to give your response a look before sending it. After submitting your first review, make sure you spend some time reading the rest of the guide.
