[D7] XSSecurity

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

You also remove the translations folder, translations are done on http://localize.drupal.org as well as remove "version" from the info file, it will be added by drupal.org packaging automatically.

Remove all old CVS $Id tags, they are not needed anymore.

There are other stuffs to clean up. For details: http://ventral.org/pareview/httpgitdrupalorgsandboxbvanmeurs1506718git

Manual review:
* code fully documented (great)
* missing README.txt in the current branch?
* included files not placed inside a folder

Edit: Typing to slow, most has been said above. :)

Your git clone url is not usable by onyone except you. Better use this one:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/bvanmeurs/1506718.git xssecurity

Setting the status accordingly, switch back to "needs review" when you are ready.

First of all, great idea, beautiful looking code. And it seems to be doing its job. However:

Automated review:
See http://ventral.org/pareview/httpgitdrupalorgsandboxbvanmeurs1506718git

Manual review:
Well, small stuff.
xssecurity.module, line 63 and 72.
I was told in my own project application that page callbacks should be public functions, so you should remove '_'.

xssecurity.module, line 80 and many others:
* Returns TRUE iff forms with the specified form id should be considered risky; returns NULL if the form id is not specified in the settings or known.
First of all, this is a long line. But unless you mean another word "iff", then "if" is written with one f. This is done several times in the same file.

I also find it confusing that you can't disable all forms temporarily on the admin page, where you only can disable the actual form you are on right now. Should it not be an option to disable all protected forms for an hour (yes i know i can turn off the module, but still?)

And the default text you have on that checkbox (which also applies to i.e the modules form) says "If you enable this checkbox, all forms with form id %form_id will be unlocked for the next hour.". Is it not an ID because it only is _one_ form with that ID? Should this not be reformulated?

In conclusion
Coming from a fellow project applicant, this a hard thing to do, but I think I have to change the status back to "needs work".

EDIT: Put a link to the automated review instead.

@eiriksm thanks for your participation, but please do not paste these ugly automated reports in here, just provide a link or an attachment containing it. as you can still edit your comment, please short it.

Hi bvanmeurs,

I have just conducted a review of your module and all I could find were minor coding standards issues (mainly comment line lengths, see: http://ventral.org/pareview/httpgitdrupalorgsandboxbvanmeurs1543016git-7...).

The module installed and uninstalled cleanly and worked very well while installed. All issues already highlighted above appear to have been fixed.

For me this module is RTBC, so I am changing the status accordingly.

Review of the 7.x-dev branch:

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code 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. Get a review bonus and we will come back to your application sooner.

manual review:

1. wrong branch name, 7.x-dev should be 7.x-1.x, see http://drupal.org/node/1015226
2. As inidicated by the automated review please fix the length of your comment lines, right now it is hard to read.
3. "variable_set('xssecurity_risky_form_ids', $settings);": if you are setting your own custom variables you should remove them on hook_uninstall().
4. "drupal_alter('xssecurity_form_is_risky', $risky_value, $form);": if you are calling your own hooks you should document them in a *.api.php file. See http://drupal.org/node/161085#api_php
5. xssecurity_form_alter(): wrong doc block.
6. xssecurity_form_alter(): do not create image markup yourself, use theme('image', ...) instead.
7. xssecurity_check.js: you should use Drupal.behaviors, see http://drupal.org/node/756722
8. "'#type' => 'hidden',": If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as #value. From http://drupal.org/node/751826
9. "array_key_exists('xssecurity_unlock', $form_state['values']": why don't you use the shorter and more common isset($form_state['values']['xssecurity_unlock']) ?
10. "array_key_exists('xssecurity_form_is_safe', $form_state['values']) && $form_state['values']['xssecurity_form_is_safe']": that can be shortened to "!empty($form_state['values']['xssecurity_form_is_safe'])". Makes the code much easier to read.
11. Your module file is quite large. Have you considered to put your page callbacks into dedicated *.pages.inc or *.admin.inc files?
12. _xssecurity_is_known_risky_form_id(): you could just use "return isset($known_form_ids[$form_id]);" ?

Protecting against XSS is a commendable goal.

The module contains a few flaws, one critical.

A sophisticated XSS payload targeting a user with the permission "administer site configuration" could submit any desired form via the following approach:

- Submit the desired form
- Fetch admin/config/system/xssecurity/xss-attempts and read the correct answer
- Submit the desired form with the answer retrieved

To remedy, generate a new captcha when it is answered. Do not output the correct answer.

In addition:
- Captchas that are answered should be removed from the session to prevent re-use.
- The captcha is too easy IMO (though not tested), it could probably decoded by a JS captcha breaker in a short time.

Nice and good work. :)

This seems like a bit of a weird situation given that you don't plan to contribute this project but do wish to use it for approval.

@bvanmeurs - maybe you would go for the review bonus one more time to finish this off? http://drupal.org/node/1410826

Changing the status to see if @bvanmeurs is still interested in getting this project approved. I think the idea is still a good one - XSS security holes show up in Drupal from time to time and it would be nice to have a module that would help prevent them.

Please clean up your git branches:

• Remove the master branch (see http://drupal.org/node/1659588)
• Looks like the commits in 7.x-dev are older than the commits in 7.x-1.x. 7.x-1.x is the preferred nomenclature, so please remove the 7.x-dev branch as well.

Even if you don't intend to submit this module, the git work will help you become a better Drupal contributor. I know it helped me. :-)

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1842466
Project 2: http://drupal.org/node/1512124

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

Change the branch according to the review comment.