[D7] block_proxies

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxisantolin2600020git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

I have checked the code and have some findings as below:

1. There is no doc block for each file header to describe the usage of the files.

/**
 * @file
 * Description you have to input.
 */

2. Missing doc block for each function, especially for the hook functions. such as.

/**
 * Implements hook_boot().
 */

3. Indentation inside block_proxies_boot() is in a mess. More details from Coding standards - Indenting and Whitespace

Please update the issue summary according to https://www.drupal.org/node/1011698 and project name too.

Please fix the following errors.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.md
-----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------
3 | WARNING | [ ] Line exceeds 80 characters; contains 130 characters
3 | ERROR | [x] Expected 1 newline at end of file; 0 found
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/block_proxies.admin.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
184 | WARNING | Empty return statement not required here
----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/block_proxies.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
33 | WARNING | Unused variable $current_ip.
----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/block_proxies.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
29 | WARNING | Messages are user facing text and must run through t() for
| | translation
---------------------------------------------------------------------------

Please see http://pareview.sh/pareview/httpgitdrupalorgsandboxisantolin2600020git

Manual Review

1. 'description' => 'Block Unwanted Connections.',
   should be in t()
2. Please consider to add @param and @retung doc to all call back fucntions.
3. drupal_set_message(t('<a href="https://pear.php.net/package/Net_DNSBL/" target="_blank">Net_DNSBL</a> Library not installed'), 'error');
   should consider to use l()

I think the review bonus tag was added by accident here, no reviews of other projects listed in the issue summary.

@isantolin: some code reviews:
In block_proxies_form(), all the #default_value variables are not properly initialized and so would cause unnecessary E_NOTICE messages in some sites and in some configurations if $field_values is not set.
It is always a good idea to initialize when the assignment is done in a conditional.

Also in block_proxies.module line 75

function block_proxies_deny_access($message) {
  header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
  print $message;
  exit();
}

The $message should be check_plain()'ed before printing because the input is obtained from the textfield in the admin form block_proxies_form(). All user input should be sanitized before printing to browser.

Hi isantolin,

Have you try to do the same just with this modules?

https://www.drupal.org/project/troll
https://www.drupal.org/project/badbehavior

In case your module is different please explain the differences on the project description.

Automated Review

have some issue in automated review - http://pareview.sh/pareview/httpgitdrupalorgsandboxisantolin2600020git

Manual Review

I have checked code. You have committed some additional files which is created by Netbeans 'nbproject'. Please remove from commit. I think it's not required.

Need more description about module configuration etc.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Update the issue's summary only.

The task of this issue queue is not publishing projects, but giving users the vetted role that allows them to opt into security coverage.
The code is now reviewed by volunteers, who will verify what you understand about writing secure code that correctly uses the Drupal API and follows the Drupal coding standards.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

Remove the @license part in the first file comments. The link given (http://www.gnu.org/copyleft/gpl.html) takes to https://www.gnu.org/licenses/gpl-3.0.html, which describes the GPLv3 license. When you consent to the Git agreement, you agree to commit code that is under GPLv2 license, not GPLv3.

I will only commit code licensed as GPL-2.0+ and non-code assets licensed with GPL friendly licenses to Drupal project repositories.

Drupal Git Contributor Agreement & Repository Usage Policy has the following note.

The "and later" clause in the Drupal GPLv2+ license allows anyone to distribute a derivative work (i.e. a version of Drupal core, modules, themes) linked to a library under a license compatible with a later version of GPL, such as GPLv3. Distribution of such derivative works is not allowed from Drupal.org.

/**
 * Implements hook_uninstall().
 */
function block_proxies_uninstall() {
  drupal_uninstall_schema('block_proxies');
}

Modules don't need to uninstall the database tables defined in hook_schema(). Drupal will automatically removed them.

/**
 * Implements hook_from_submit().
 */
function block_proxies_form_submit($form, &$form_state) {
  $form_state['values']['dnsbl'] = str_replace(' ', '', $form_state['values']['dnsbl']);

  $data = serialize($form_state['values']);

  $result = db_select('block_proxies', 'bp')->fields('bp')->execute();
  $num_of_results = $result->rowCount();

  if ($num_of_results == 0) {
    db_insert('block_proxies')->fields(array('site' => 'all', 'configuration' => $data))->execute();
  }
  else {
    db_update('block_proxies')->fields(array('configuration' => $data))->condition('site', 'all')->execute();
  }

  drupal_set_message(t('Configuration Updated'));
  $form_state['redirect'] = 'admin/config/people/block-proxies';

  // return;.
}

That function is not an hook implementation (and surely not a hook_from_submit() implementation). It's not clear why the function is setting a site value if then users aren't allowed to select for which site the configuration is.

REQUIREMENTS
------------

This module requires the following modules:

 * Net_DNSBL (https://pear.php.net/package/Net_DNSBL/)
 * Drupal 7 (https://www.drupal.org/)

Drupal isn't a module, nor is Net_DNSBL.

 * You may want to disable Toolbar module, since its output clashes with
   Administration Menu.

This seems a comment taken from the documentation of another module. I don't see how it is relevant for this module that the Administration Menu module has problems with the Toolbar module.

Thanks for your contribution!

1. info file: Remove the line "; Information added by drupal.org packaging script on 2012-02-14"
2. block_proxies_menu(): the permission "ban IP address ranges" looks undefined. Did you forget to implement hook_permission()?
3. block_proxies_boot(): no need to call unserialize() on a variable. Drupal will serialize variables automatically when writing to the DB in variable_set().
4. block_proxies_form_submit(): doc block is wrong, this is not a hook but a form submit callback. See https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
5. block_proxies_schema(): Looks like you are not using the database table. Should this hook be removed?

Otherwise I don't see any security issues/blockers.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

I add my current employer.