Changes

Jump to: navigation, search

Add-ons/Reviewers/Guide/Reviewing

419 bytes removed, 23:59, 19 January 2015
Code review
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 ===
Every line of All add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, so which is why we must always be on the lookout for bad code. For versions in the Preliminary Review queue, the code have a review should just ensure the add-on's safetyteam.
All review pages Add-on History entries have a View Contents link that take you to the code browser page. Pending updates Updates also have a Compare with Public Version link next to it, 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.
For updates== Libraries, the compare link should be used. However, it can sometimes fail to work for very large add-ons. In that case, you can either review the full source code, or download the new and public files to your system frameworks and compare them using tools like [http://other unreadable code.google.com/p/amo-editor-tools/source/browse/trunk/sh/xpidiff.sh xpidiff.sh] or [https://wiki.mozilla.org/AMO:Editors/WinMerge WinMerge]. ==
Validation results are integrated into the code viewerIt's very common for add-ons to use libraries or frameworks such as jQuery or Bootstrap. Some add-ons use complex frameworks like Kango, so you can see validation warnings in contextusually to achieve cross-browser compatibility. For Finally, the Add-ons SDK extensionsgenerates various files around the actual add-on code. Our code validator will try to detect them. If detected, known library files are shaded code won't generate other validator messages, and it is greyed out in greythe code viewer. If a library or framework isn't detected, it can mean that it was either modified by the developer (and their origin is stated 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 popup mailing list. Library or frameork code that appears when hovering over 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 ====
{| width="70080%" cellspacing="0" cellpadding="1" border="0"
|-
! style="border-bottom: 2px solid black" scope="col" | Policy Issue
! style="border-bottom: 2px solid black" scope="col" | Action
! style="border-bottom: 2px solid black" scope="col" | Notes
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Remote code download or execution, custom update code.
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-ons should not download As explained in the validation section, no remote code in any way. They are free to interact with web APIs as long as all that execution is being transmitted is data, not code. Add-ons are allowed to insert local scripts into webpages (within reason), but are not allowed to insert remote scripts.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Modified on unverified AddUsing plain HTTP for security-ons SDK library codesensitive operations.
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Library files that are unrecognized will not appear Sending passwords over unprotected HTTP if there's a secure alternative. Sending passwords in grey colorthe URL or other headers than POSTDATA. Performing any security-sensitive operations over HTTP when there's a secure alternative.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using non-release version of Add-ons SDK library| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Reviewreview| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | 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 or beta versions of the SDK. Others should only get preliminary review.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using outdated Add-ons SDK library
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Reviewreview| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Except under special circumstances, we 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.|- style="vertical-align: top;"| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | HTTP security violations | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Reject | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | 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.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Bad or no namespacing
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review review
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | 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.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preference names without "extensions." or "services.sync.extensions." prefix | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review review (nominations only)| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | 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.|- style="vertical-align: top;"| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Extension preferences names in Sync without "services.sync.extensions." prefix | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-on preferences using the Sync service of Firefox should use the "services.sync.extensions." prefix, and should also have a reasonable namespace.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Privacy issues
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review review | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | 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).
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Performance problems
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review review | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Synchronous (non-local) HTTP requests, synchronous SQL queries, noticeably inefficient code, random UI freezes, loading large amounts of JS code (directly in overlays.|- style="vertical-align: top;"| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Writing files outside of the profile folder.| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Some add-ons need helper files like SQL DBs or logs. Those files must be written in the order profile folder, not the extension installation folder or other locations.|- style="vertical-align: top;"| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Bootstrapped or SDK add-on doesn't clean up after itself. | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Some things to look for: the add-on must not require a restart for any of tens of thousands of lines of code) directly in overlaysits 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. [[http://maglione-k.users.sourceforge.net/bootstrapped.xhtml More details].|- style="vertical-align: top;"| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | New version doesn't follow previous review requests.| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary review | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | If the author keeps submitting the same version, contact the mailing list.
|- style="vertical-align: top;"
| style="padding: 0.5ex 1ex 1ex 0pt; border-bottom: 1px solid black;" | Not using prefwindow for Using non-standard preferences UI | style="padding: 0.5ex 1ex 1ex 0pt; border-bottom: 1px solid black;" | Add Note note| style="padding: 0.5ex 1ex 1ex 0pt; border-bottom: 1px solid black;" | If an add-on has a preferences windowUI, we recommend that authors do 2 things: 1) it should use one of the ''prefwindow'' element, 2) add the line in install[https://developer.mozilla.rdf that enables this window to be opened from the org/en-US/Add-ons Manager window/Install_Manifests#optionsURL supported methods].
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Duplicate / hidden files or folders
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add Notenote
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | See canned response.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Unpacked add-on writes files in own installation directory
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Writing in the installation directory can generate problems with the Add-ons Manager, where the add-on can be stuck in an update loop. Add-ons should write all files in the profile dir.
|- style="vertical-align: top;"
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Bootstrapped Add-on doesn't clean up after itself
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Preliminary Review
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | See section on [[AMO:Editors/EditorGuide/SpecialAddonTypes#Bootstrapped_.28Restartless.29_Add-ons|Bootstrapped Add-on policies]].
|}
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.
See If the [[AMO:Editors/EditorGuide/AddonReviews/Security|security code review]] page add-on version is not nominated for more detail on the above security rejection reasons. 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.  By the end of the code full review, or if by this point you'll may have a series of notes that will require sufficient information to deny the add-on to be significantly rewritten. If you think that's the case, it's OK to resolve the full review without proceeding to the next step. If the add-on needs work but is safe to use, it you can be given a preliminary review approval without performing any testing. If the add-on needs work stop reviewing and submit what you are not sure if it is safe to use, then leave the add-on for someone else to review, request a admin review, or ask for help in the mailing listhave.
=== Step 4: Feature Review ===
Canmove, confirm
1,448
edits

Navigation menu