New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Rules Severities for Marketplace Technical Review #14

Open
lenaorobei opened this Issue Jan 22, 2019 · 6 comments

Comments

Projects
None yet
5 participants
@lenaorobei
Copy link
Collaborator

lenaorobei commented Jan 22, 2019

Marketplace Technical Review

PHP CodeSniffer is one of the tools Magento Marketplace uses for extensions verification. Find more details about current approach here.
Severities is the way to determine how strict the rule is. The higher is the severity the stricter the rule is.

⚠️ Note: only severity 10 issues lead to extension rejection.

Proposed Severities Definition

Type Severity Description
error 10 Critical code issue.
warning 9 Security issue (possible).
warning 8 Magento specific code issue.
warning 7 General code issue.
warning 6 Code style issue (PSR2).
warning 5 PHPDoc formatting or commenting issue.

Proposed Rules Severities

The Magento standard contains 96 sniffs.

Fixable column identifies whether issue can be fixed by phpcbf tool

  • ✔️ yes
  • ✖️ no
  • ✔️ ✖️ partially
Sniff Description Fixable
1️⃣0️⃣ ERRORS - Critical issues
Generic.Functions.CallTimePassByReference Ensures that variables are not passed by reference when calling a function. ✖️
Generic.PHP.CharacterBeforePHPOpeningTag Checks that the opening PHP tag is the first content in a file. ✖️
Generic.PHP.DeprecatedFunctions Discourages the use of deprecated PHP functions. ✖️
Generic.PHP.NoSilencedErrors Throws an error or warning when any code prefixed with an asperand is encountered. ✖️
Generic.PHP.Syntax Ensures PHP believes the syntax is clean. ✖️
Magento.Legacy.MageEntity Detects typical Magento 1.x (Mage::) classes constructions. ✖️
Magento.NamingConvention.ReservedWords Validates that class name is not reserved word. ✖️
Magento.PHP.FinalImplementation Magento is a highly extensible and customizable platform. The use of final classes and methods is prohibited. ✖️
Magento.PHP.Goto Detects use of goto. ✖️
Magento.PHP.ReturnValueCheck Detects misusing of === and !===operators when checking srtpos, stripos, array_search ✖️
Magento.Security.LanguageConstruct Detects possible usage of discouraged language constructs (exit, echo, backquotes). ✖️
Magento.Security.Superglobal Detects possible usage of super global variables. ✖️
Magento.Strings.ExecutableRegEx Detects executable regular expressions. ✖️
PSR1.Classes.ClassDeclaration Checks the declaration of the class is correct. ✖️
PSR2.Files.ClosingTag Checks that the file does not end with a closing tag. ✔️
Squiz.PHP.Eval The use of eval is discouraged. ✖️
9️⃣WARNINGS - Possible security issues
Magento.Security.IncludeFile Detects possible improper usage of include functions. ✖️
Magento.Security.XssTemplate Detects not escaped output in phtml templates. ✖️
8️⃣WARNINGS - Magento specific code issues
Magento.Classes.ObjectInstantiation Detects direct object instantiation via new keyword. ✖️
Magento.Classes.ObjectManager Class ObjectManagerSniff detects direct ObjectManager usage. ✖️
Magento.Exceptions.DirectThrow Detects possible direct throws of Exception. ✖️
Magento.Files.LineLength Line length sniff which ignores long lines in case they contain strings intended for translation. ✖️
Magento.NamingConvention.InterfaceName Detects possible interface declaration without 'Interface' suffix. ✖️
Magento.PHP.DateTime Detects overcomplicated Date/Time handling. ✖️
Magento.PHP.ShortEchoSyntax Validate short echo syntax is used. ✖️
Magento.Templates.ThisInTemplate Detects possible usage of $this variable files. ✖️
Magento.Translation.ConstantUsage Make sure that constants are not used as the first argument of translation function. ✖️
7️⃣WARNINGS - General code issues
Generic.Arrays.DisallowLongArraySyntax Bans the use of the PHP long array syntax. ✔️ ✖️
Generic.Classes.DuplicateClassName Reports errors if the same class or interface name is used in multiple files. ✖️
Generic.CodeAnalysis.ForLoopShouldBeWhileLoop Detects for-loops that can be simplified to a while-loop. ✖️
Generic.CodeAnalysis.ForLoopWithTestFunctionCall Detects for-loops that use a function call in the test expression. ✖️
Generic.CodeAnalysis.JumbledIncrementer Detects incrementer jumbling in for loops. ✖️
Generic.CodeAnalysis.UnconditionalIfStatement Detects unconditional if- and elseif-statements. ✖️
Generic.CodeAnalysis.UnusedFunctionParameter Checks the for unused function parameters. ✖️
Generic.CodeAnalysis.UselessOverridingMethod Detects unnecessary overridden methods that simply call their parent. ✖️
Generic.Metrics.CyclomaticComplexity Checks the cyclomatic complexity (McCabe) for functions. ✖️
Generic.Metrics.NestingLevel Checks the nesting level for methods. ✖️
Generic.PHP.DisallowShortOpenTag Makes sure that shorthand PHP open tags are not used. ✔️ ✖️
Magento.CodeAnalysis.EmptyBlock Detects empty statements and functions. ✖️
Magento.Exceptions.Namespace Detects possible usage of exceptions without namespace declaration. ✖️
Magento.PHP.DiscouragedFunction Detects possible usage of discouraged functions. ✖️
Magento.PHP.LiteralNamespaces Detects the use of literal class and interface names. ✖️
Magento.PHP.Var Detects possible usage of var language construction. ✖️
Magento.Performance.EmptyCheck Allows easily detect wrong approach for checking empty variables in conditions. ✖️
Magento.Strings.StringConcat Detects string concatenation via + operator. ✖️
PEAR.Functions.ValidDefaultValue Ensures function params with default values are at the end of the declaration. ✖️
Squiz.Functions.GlobalFunction Tests for functions outside of classes. ✖️
Squiz.Operators.IncrementDecrementUsage Ensures that the ++ operators are used when possible. ✖️
Squiz.PHP.GlobalKeyword Stops the usage of the global keyword. ✖️
Squiz.PHP.NonExecutableCode Warns about code that can never been executed. ✖️
Squiz.Scope.MemberVarScope Verifies that class members have scope modifiers. ✖️
6️⃣WARNINGS - Code style issues
Generic.ControlStructures.InlineControlStructure Verifies that inline control statements are not present. ✔️
Generic.Files.ByteOrderMark A simple sniff for detecting BOMs that may corrupt application work. ✖️
Generic.Files.LineEndings Checks that end of line characters are correct. ✔️
Generic.Formatting.DisallowMultipleStatements Ensures each statement is on a line by itself. ✔️
Generic.Functions.FunctionCallArgumentSpacing Checks that calls to methods and functions are spaced correctly. ✔️
Generic.NamingConventions.UpperCaseConstantName Ensures that constant names are all uppercase. ✖️
Generic.PHP.LowerCaseConstant Checks that all uses of true, false and null are lowercase. ✔️
Generic.PHP.LowerCaseKeyword Checks that all PHP keywords are lowercase. ✔️
Generic.WhiteSpace.DisallowTabIndent Throws errors if tabs are used for indentation. ✔️
Generic.WhiteSpace.ScopeIndent Checks that control structures are defined and indented correctly. ✔️
Magento.Whitespace.EmptyLineMissed Class EmptyLineMissedSniff ✖️
Magento.Whitespace.MultipleEmptyLines Detects possible usage of multiple blank lines in a row. ✖️
PEAR.ControlStructures.ControlSignature Verifies that control statements conform to their coding standards. ✖️
PSR1.Files.SideEffects Ensures a file declares new symbols and causes no other side effects, or executes logic with side effects, but not both. ✖️
PSR1.Methods.CamelCapsMethodName Ensures method names are defined using camel case. ✖️
PSR2.Classes.ClassDeclaration Checks the declaration of the class and its inheritance is correct. ✔️ ✖️
PSR2.Classes.PropertyDeclaration Verifies that properties are declared correctly. ✖️
PSR2.ControlStructures.ControlStructureSpacing Checks that control structures have the correct spacing around brackets. ✔️
PSR2.ControlStructures.ElseIfDeclaration Verifies that there are no else if statements (elseif should be used instead). ✔️
PSR2.ControlStructures.SwitchDeclaration Ensures all switch statements are defined correctly. ✔️ ✖️
PSR2.Files.EndFileNewline Ensures the file ends with a newline character. ✔️
PSR2.Methods.FunctionCallSignature Checks that the function call format is correct. ✔️
PSR2.Methods.FunctionClosingBrace Checks that the closing brace of a function goes directly after the body. ✔️
PSR2.Methods.MethodDeclaration Checks that the method declaration is correct. ✔️ ✖️
PSR2.Namespaces.NamespaceDeclaration Ensures namespaces are declared correctly. ✔️
PSR2.Namespaces.UseDeclaration Ensures USE blocks are declared correctly. ✔️ ✖️
Squiz.Classes.ValidClassName Ensures classes are in camel caps, and the first letter is capitalized. ✖️
Squiz.ControlStructures.ControlSignature Verifies that control statements conform to their coding standards. ✔️ ✖️
Squiz.ControlStructures.ForEachLoopDeclaration Verifies that there is a space between each condition of foreach loops. ✔️ ✖️
Squiz.ControlStructures.ForLoopDeclaration Verifies that there is a space between each condition of for loops. ✔️ ✖️
Squiz.ControlStructures.LowercaseDeclaration Ensures all control structure keywords are lowercase. ✔️
Squiz.Functions.FunctionDeclaration Checks the function declaration is correct. ✖️
Squiz.Functions.FunctionDeclarationArgumentSpacing Checks that arguments in function declarations are spaced correctly. ✔️
Squiz.Functions.LowercaseFunctionKeywords Ensures all function keywords are lowercase. ✖️
Squiz.Functions.MultiLineFunctionDeclaration Ensure single and multi-line function declarations are defined correctly. ✔️
Squiz.Scope.MethodScope Verifies that class methods have scope modifiers. ✖️
Squiz.WhiteSpace.LogicalOperatorSpacing Verifies that operators have valid spacing surrounding them. ✔️
Squiz.WhiteSpace.ScopeClosingBrace Checks that the closing braces of scopes are aligned correctly. ✔️
Squiz.WhiteSpace.ScopeKeywordSpacing Ensure there is a single space after scope keywords. ✔️
Squiz.WhiteSpace.SuperfluousWhitespace Checks for unneeded whitespace. ✔️
5️⃣ WARNINGS - Annotations and commenting
Magento.Annotation.ClassAnnotationStructure Sniff to validate structure of class, interface annotations. ✖️
Magento.Annotation.MethodAnnotationStructure Sniff to validate structure of public, private, protected method annotations. ✖️
Magento.Annotation.MethodArguments Sniff to validate method arguments annotations. ✖️
Squiz.Commenting.DocCommentAlignment Tests that the stars in a doc comment align correctly. ✔️
Squiz.PHP.CommentedOutCode Warn about commented out code. ✖️

@lenaorobei lenaorobei changed the title [Proposal] Rules severities [Proposal] Rules Severities for Marketplace Technical Review Jan 22, 2019

@larsroettig

This comment has been minimized.

Copy link
Member

larsroettig commented Jan 22, 2019

Hi @lenaorobei,
if we have 9 (Security issue) it should also to be checked or rejected.

Also, I recommend using the Checks to build a score and publish on Marketplace in these 3 categories.

Categories:

Value Description
60 % Critical and Security issue
30 % Code issue
10 % Code style issue or PHPDoc

Socre and Bagde:

Score Description
90 - 100 Very Good Extension
80 - 90 Good Extension
60 -80 Normal Extension
< 60 Bad Extension - No Listed (Rejection)

The Badge and Score can be used to find nice Extension also, it can be used for Marketplace Ranking and Trust. I think most extension provider will update their Normal Extension to get a better ranking in the Marketplace.

@lenaorobei

This comment has been minimized.

Copy link
Collaborator Author

lenaorobei commented Jan 22, 2019

@larsroettig thanks for the feedback!

Answering to your concerns about security - they are possible issues, so I didn't assign severity 10 to them. For example Magento.Security.XssTemplate sniff is built based on Templates XSS security.

/* @noEscape */ before output. Output doesn’t require escaping. Test is green.
/* @escapeNotVerified */ before output. Output escaping is not checked and should be verified. Test is green.

I'm not sure we are ready now to reject extensions because of wrong annotation. Once we'll bring this message to developers and spread the idea about Magento coding standard, we can make rules stricter.

I like the idea you've described very much! Scores and badges is definitely what we're aiming to. To make it happen we need to define severities and start using this coding standard. Then we will be able to move forward with more complex calculations.

@sprankhub

This comment has been minimized.

Copy link

sprankhub commented Jan 23, 2019

That possible security issues are not rejected got my attention at first sight as well. I am not 100% sure what the current security-related sniffs check and how many false positives they produce. However, if rejecting extensions "because of wrong annotation" is your only concern, I would not care. Even if this is quite strict, it is probably easy to fix and hence, a rejection is not a big issue as long as it leads to better security in Magento extensions in general.

@lenaorobei

This comment has been minimized.

Copy link
Collaborator Author

lenaorobei commented Jan 23, 2019

@sprankhub I see your point and the rejection of extensions with XSS issues is what we are striving to, but I think we should start with explanation and popularization the idea of how to write good code and warn that every 9, 8 or 7 severity issue might became 10 in future.

@convenient

This comment has been minimized.

Copy link

convenient commented Jan 23, 2019

A warning for possible security issues is not enough IMO. I participate in the bug bounty program and I actively look for escapeNotVerified for things to target.

Things that have been added to the code and marked as "secure enough" are often broken and used in attack chains. If it is manually reviewed by a human who thinks "this is secure and can't be broken" there is still the chance they are mistaken.

I know some popular modules still use the php unserialize function directly when they should be steering well clear of it. I'd like to see the marketplace become more strict.

@ravmenon

This comment has been minimized.

Copy link

ravmenon commented Feb 2, 2019

@larsroettig Thanks for the scoring suggestion - it aligns well with a plan we had to encourage extension developers to fix the warnings too. The scoring mechanism will be incorporated along with this consolidation work as part of the enhancement of phpcs tests in EQP 2 with a plan for a beta release initially not affecting any submissions, with plans to give developers enough time to be ready when it is fully activated.

lenaorobei added a commit that referenced this issue Feb 11, 2019

@lenaorobei lenaorobei added accepted and removed need to discuss labels Feb 11, 2019

@lenaorobei lenaorobei self-assigned this Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment