canmove, Confirmed users
1,448
edits
AMO:Editors/EditorGuide/AddonReviews: Difference between revisions
AMO:Editors/EditorGuide/AddonReviews (view source)
Revision as of 00:05, 29 November 2011
, 29 November 2011→Step 3: Code Review
| Line 246: | Line 246: | ||
Every line of add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, so we must always be on the lookout for bad code. For versions in the Preliminary Review queue, the code review should just ensure the add-on's safety. | Every line of add-on code must be reviewed. The code validator can't detect all possible security or code quality issues, so we must always be on the lookout for bad code. For versions in the Preliminary Review queue, the code review should just ensure the add-on's safety. | ||
All review pages have a View Contents link that take you to the code browser page. Pending updates also have a Compare with Public Version link next to it, which will show you the code with the changed sections highlighted | All review pages have a View Contents link that take you to the code browser page. Pending 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. 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 and compare them using tools like [http://code.google.com/p/amo-editor-tools/source/browse/trunk/sh/xpidiff.sh xpidiff.sh] or [https://wiki.mozilla.org/AMO:Editors/WinMerge WinMerge]. | For updates, 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 and compare them using tools like [http://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 viewer, so you can see validation warnings in context. For Add-ons SDK extensions, known library files are shaded in grey, and their origin is stated in the popup that appears when hovering over them. | |||
==== Policies and Actions ==== | ==== Policies and Actions ==== | ||
{| width=" | {| width="700" cellspacing="0" cellpadding="1" border="0" | ||
|- | |- | ||
! style="border-bottom: 2px solid black" scope="col" | Policy | ! style="border-bottom: 2px solid black" scope="col" | Policy | ||
| Line 264: | Line 260: | ||
! 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;" | Remote code download or execution, custom | | 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;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-ons should not download remote code in any way. They are free to interact with web APIs as long as all that 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="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add-ons should not download remote code in any way. They are free to interact with web APIs as long as all that 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="vertical-align: top;" | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Modified on unverified Add-ons SDK library code | ||
| 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 in grey color. | |||
|- style="vertical-align: top;" | |||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Using non-release Add-ons SDK 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;" | 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;" | 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;" | Reject | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | 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. | ||
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 | |||
|- style="vertical-align: top;" | |- 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;" | Bad or no namespacing | ||
| Line 296: | Line 298: | ||
| 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;" | Duplicate / hidden files or folders | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add Note | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | Add Note | ||
| style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | | | style="padding: .5ex 1ex 1ex 0; border-bottom: 1px solid black;" | See canned response. | ||
|- style="vertical-align: top;" | |- 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;" | Bootstrapped Add-on doesn't clean up after itself | ||
| Line 306: | Line 308: | ||
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. | 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 review, you may have a series of notes that will require the add-on to be significantly rewritten. If you think that's the case, it's OK to resolve the review without proceeding to the next step. If the add-on needs work but is safe to use, it can be given a preliminary review approval without performing any testing. | By the end of the code review, you'll may have a series of notes that will require the add-on to be significantly rewritten. If you think that's the case, it's OK to resolve the review without proceeding to the next step. If the add-on needs work but is safe to use, it can be given a preliminary review approval without performing any testing. | ||
=== Step 4: Feature Review === | === Step 4: Feature Review === | ||