Security Testing

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxudaksh1611500git

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

please get a review bonus first. Then try to fix issues raised by automated review tools and set this back to "needs review".

Really well written, readable code. Kudos :)
Need to devote more time for in-depth review, yet the first look of the module is clean and nicely done.

However, I would suggest to go through the naming of form elements again. http://drupal.org/coding-standards#naming.
Its always good to prefix them with your module name, esp. the global variables.
e.g.
$form['location'] (#90 .module) is quite general while $form['mybutton1'] (#95, .module file) is quite vague.
Also better rename menu.inc to security_testing_menu.inc.

Leaving the application in NR status. Hope to see more reviewers chip in.

Wow, this looks very promising. This review is just a code review and a cursory one at that. This module need several reviewers because there's a lot to take in.

I've included corrections for some spelling mistakes and comments on readability of variable names. They are not that critical, but I include them as a help if you want it (and my list is probably not complete). Also, I'm new to reviewing so I might go way overboard in some cases. They are not hard recommendations, and I would welcome some comments on my comments as I would learn from it as well.

security_testing.info:

dependencies should be listed (http://drupal.org/node/542202) like this:
dependencies[] = panels

You could also consider adding a link to the config screen in the info files as well like this:
configure = admin/config/content/example

security_testing.install:

Very sparsely commented, but the code is easy to follow so I guess it's ok.

security_testing.module:

Perhaps you should consider making your own set of permissions?

line 104: one to many "from"

line 113: consider adding warning type. I think 'warning' would be appropiate in this case.

line 145-147: you're making some assumptions here that I'm not sure will always work. You're combining all the source files into one big file, remove all instances of '< ?php' and adds a new '< ?php' at the top. Perhaps add a str_replace for '? >' as well? They shouldn't be there... but they might, and that would ruin the php flow.

line 218 and elsewhere: you might want to consider using drupal_basename instead of basename, check this one: http://api.drupal.org/api/drupal/includes!file.inc/function/drupal_basen...

line 273: these two listS (plural), nitpicking, I know ;)

line 323: you're using count($x) inside a for loop. For big $x this can be a performance bottleneck since count($x) is calculated each iteration. Consider adding a $counter before the loop and iterate using that. (Like you're doing on line 335). There are more of these, search for "< count(".

line 355: been -> being

line 450: function security_testing_get_all_callbacks() does a lot of stuff and would benefit from a bit more commenting. Optionally you could consider renaming the variables in lines 487-489 to $check1.. $check3

line 539: Not 100% sure but I think functions should not use camelCase: security_testing_functionCalls() (http://drupal.org/node/1801276)

lines 574->: consider nameng variables more ... describably? $ite, $itemm and $nnode doesn't immediately tell me what they contain making it hard to read since I new to double back once in a while to check "what did that variable containt"?

line 643: I'd really like some more commenting here. Lots of stuff happening, especially in the late 700s/early 800 lines :)

line 656 and others: warning message "[..] is looking to be vulnerable." -> "[..] seems to be vulnerable.""

line 693: this may be way to much subjectivity, but I'd split the if check to something like this:

if ($count > 1 && $func_call_name == 't') {
  $string2 = $params[1];
}
if ($count > 2 && $func_call_name == 'watchdog') {
  $string2 = $params[2];
}

and I don't think line 702 (if ($string2->isType(T_ARRAY)) {) must be inside the if in 693 so I think this would be more readable and stil work as designed.

line 848: why $varr = $variable? $variable isn't modified somewhere so I think you could just use $variable instead of $varr.

line 949: instead of

$blah = $val->trim();
$tempp = array();
$tempp[] = $blah;

I think you could just do

$tempp = array();
$tempp[] = $val->trim();

or even just

$tempp = array($val->trim());

line 1228: I believe this is redundant:

$ret = $ins;
return $ret;

Not all functions specify what they @return in the doc block. Variable naming is sometimes hard to follow, atleast for me.

I will surely use this module in my own projects! Thanks for your work :)

The git link you provide in your original post is the maintainer version, not the non-maintainer version. Please replace with
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/udaksh/1611500.git security_testing

Automated review:
http://ventral.org/pareview/httpgitdrupalorgsandboxudaksh1611500git

Manual Review:
Wow, this is a rather complex module. I just scratched the surface, but I think I've found enough issues to be worked on. Let me preface this by saying that you shouldn't take the number of issues I have listed as an indication that there are serious issues with your code - on the contrary, your code is rather well written. It is simply a very large module and with more code comes more bugs more often than not. I think the module has great potential and I would encourage you to continue to hone its security detection capabilities.

1. security_testing.info - dependencies (Grammar Parser, Simpletest) need to be added or when someone attempts to run the security scan, they'll get a fatal error about missing classes.
2. security_testing.module line 16 - title should omit underscores and capitalize first word for consistency.
3. Some comments regarding a module I scanned http://drupal.org/node/1784544
   1. Function call system_settings_form at line 44 in file user_requestname.admin.inc is looking to be vulnerable - return system_settings_form($form); is valid and doesn't constitute any security risk that I can detect.
   2. Function call drupal_mail at line 205 in file user_requestname.module is looking to be vulnerable - is valid and doesn't constitute any security risk that I can detect.
   3. Function call user_preferred_language at line 205 in file user_requestname.module is looking to be vulnerable - is perfectly valid and doesn't constitute any security risk that I can detect.
   4. Function call token_replace at line 257 in file user_requestname.module is looking to be vulnerable - is valid and doesn't constitute any security risk that I can detect.
4. Security Test does not appear in the Simpletest list. See attached screenshot.
5. security_testing.module line 88 - security_testing_form has a function comment that suggests it is an implementation of hook_form(), however it is used as a form implementation used with drupal_get_form (see your hook_menu() implementation).
6. security_testing.module line 248, 254, 261 - change db_query() to db_select()
7. security_testing.module line 539 - function name should not have camelCase component
8. security_testing.module line 1471 - $values is undefined
9. security_testing_menu.inc line 27, 181, 239 - change db_query() to db_select().

Added more application reviews

Hi Udaksh, let me illustrate what I mean with an example:

my-module.module:

// code

my-module.inc:

// code

When you strip out, combine them and add a php-tag you end up with this:

// code
? >
// code... outside of php!

Now closing files with ?> isn't what you should do, but then, it might happen and is easy to correct by stripping out ?>'s as well.

Did that make sense?

Deleting my comment through edit. I misunderstood the above comment. Apologies!

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. @param : $body: data type missing, remove ":" and insert string, int, bool or whatever $body should be. See http://drupal.org/node/1354#functions . Also elsewhere.
2. security_testing.info: why is simpletest not a dependency?
3. security_testing_shutdown(): doc block is wrong: this is not a hook.
4. This module has some heavy global variables and invasive hooks into the database on every single page request. Therefore the README.txt should contain a note that this module should never be used on a production site, only for testing purposes on development sites.
5. security_testing_functions(): I really hate global variables. They make it very hard to understand the flow of your module. Can't suggest any improvements though, as I'm not familiar with the control/data flow.
6. "@return ret": the @return directive should contain the datatype, not the variable name. Example: string, int, bool, array ...
7. ERROR: substr() expects parameter 1 to be string, object given on line 478 in security_testing.module (Warning)
8. When I run this with drush PHP crashes with a segmentation fault (tested on php5_5.4.6-1ubuntu1). What version of PHP did you use for testing?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Changed git clone command

manual review:

1. ERROR: Object of class PGPExpression could not be converted to string on line 478 in security_testing.module (Recoverable error)
2. I already said that I'm unhappy with all the global variables, and also with the amount of undocumented code.
3. SecurityTestingTestCase does not make sense at all, this does not look like a Simpletest? There is not a single assert() method call, so what are you testing exactly? Do you know how automated unit/integration tests should work?
4. security_testing_form(): you are accidentally implementing hook_form() here, better rename that function.
5. Do not silence errors with "@". Rework your code with the appropriate isset() checks etc. so that it does not throw PHP warnings/notices.
6. SecurityTestingTestCase: "$this->module = check_plain(file_get_contents($this->path . '/module_name.txt'));": why do you need check_plain() here? No user provided input is involved and you are not printing anything to the user?

Regarding the segmentation fault: please check if this works for you:

git clone --recursive --branch 7.x-2.x http://git.drupal.org/project/restws.git
drush sec-test /path/to/restws

That results in a segmentation fault on my computer, also in the web interface.

So while I personally have some severe issues with the code I see no real application blockers, therefore I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Ah, I should have read the @file comment in the test file.

Review of the 7.x-1.x branch:

• Remove all backup files from your repository:
  

  ./security_testing_menu.inc~
  ./security_testing.module~
  ./security_testing.install~
  ./security_testing.test~
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. Remove .goutputstream-07DVOW from the repository.
2. "$aa = module_invoke_all('permission');": that variable name does not make sense. Better use $permissions instead?

Back to RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, udaksh!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

It looks like a nice module, but the command in the description:
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/udaksh/1611500.git security_testing

Is giving me the following output in the terminal:
Cloning into 'security_testing'...
fatal: http://git.drupal.org/sandbox/udaksh/1611500.git/info/refs not found: did you run git update-server-info on the server?

Help in solving the issue will be appreciated.

Best regards,
Nikolay D.

@ndobromirov - This isn't sandbox project now. you can try it as

git clone --recursive --branch 7.x-1.x http://git.drupal.org/project/securitytesting.git

added additional reviews