Re: Perl::Critic Tests

Hi Nick,

Thanks for gathering this data.

As we are a webapp and as we pride ourselves in delivering secure code, I think we should live up to the promise by requiring the CERT secure coding standards to be applied -- at least as far as our new code goes. There may be a few points in the new code where we are currently violating the policies because we're "calling out" to old code. This definitely can't be the case for all of the listed violations. So, I think that *if* we need to allow a violation (and need to add a Critic suppression), we should *only* do that under the condition that there's a well documented explanation of why this is required.

In short: I think we should adopt the coding guidelines for new code.

For old code, I'm definitely interested to see the violations as well. However, we may not be able to fix all of them. When we can't, I think we should be open and upfront about it. If the number of violations in old code is simply too big to fix (definitely a possibility), the stats provide a new angle as to where we should start rewriting that code :-)

Regards,

Erik.

On Fri, Apr 14, 2017 at 8:02 PM, Nick Prater <..hidden..> wrote:

As discussed in chat, I've been following up on ehuelsmann's suggestion
to test our code against the Perl::Critic policies recommended by CERT.
See:

https://gist.github.com/briandfoy/4525877
https://www.securecoding.cert.org/confluence/display/perl/SEI+CERT+Perl+Coding+Standard
https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$14917685211662024izFqM:matrix.org
https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$1491849623220407yWMci:matrix.org

Enabling the CERT recommended policies on our 'new' code results in 1050
policy violations:

'Perl::Critic::Policy::Subroutines::RequireFinalReturn' => 293,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMagicNumbers' =>
172,
'Perl::Critic::Policy::Modules::RequireVersionVar' => 170,
'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122,
'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' =>
34,
'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31,
'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' =>
26,
'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMixedBooleanOperators'
=> 23,
'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21,
'Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest'
=> 14,
'Perl::Critic::Policy::InputOutput::RequireCheckedOpen' => 11,
'Perl::Critic::Policy::Objects::ProhibitIndirectSyntax' => 11,
'Perl::Critic::Policy::Variables::ProhibitUnusedVariables' => 10,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoWarnings' => 10,
'Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars' =>
10,
'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMismatchedOperators'
=> 9,
'Perl::Critic::Policy::Subroutines::ProtectPrivateSubs' => 8,
'Perl::Critic::Policy::InputOutput::RequireCheckedClose' => 8,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitBooleanGrep' => 5,
'Perl::Critic::Policy::Variables::RequireLexicalLoopIterators' => 5,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitCommaSeparatedStatements'
=> 4,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringySplit' => 4,
'Perl::Critic::Policy::ControlStructures::ProhibitUnreachableCode' => 3,
'Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations' => 3,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoStrict' => 3,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitUniversalIsa' => 3,
'Perl::Critic::Policy::InputOutput::RequireEncodingWithUTF8Layer' => 2,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval' => 1,

I've not tested 'old' code, as I view 'new' code as the first priority.

Our current coding guidelines are set out here:
https://ledgersmb.org/community-guide/community-guide/development/coding-guidelines/perl-coding-guidelines

Moving forward, Do we want to make these Perl::Critic policies part of
our coding standard?

If yes, I'm happy to start preparing patches to fix these violations and
add policies to the tests in xt/01.1-critic.t.

Happy Easter!

Nick

--
Nick Prater - NP Broadcast Limited
100 Pitfold Road Lee London SE12 9HY
T: 020 3627 3815 M: 07887 916 458

NP Broadcast Limited is registered in England and Wales number 0794374
VAT Registration Number: GB 129 0388 11

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ledger-smb-devel mailing list
Ledger-smb-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.