Download: drupal.org/project/honeytrap

Summary

The Honeytrap module allows site owners and system administrators to monitor web crawlers that do not follow the rules set out in the robot.txt file or via the RobotsText module or similar method and as a result put an unnecessarily high load on servers.

This is especially important for large, very high traffic and/or high profile sites where the activity of these non-compliant crawlers can bring servers to their knees. If these crawlers are not blocked or slowed down quickly enough then these crawlers can result in servers being knocked completely offline.

Please note that the Honeytrap module does not directly block or slow down offending IP addresses itself; it only logs and reports them, leaving you in full control of how you want to deal with them.

WARNING: When set up correctly with a firewall or similar this module will slow down and block traffic to your website. This potentially could include yourself, website owners, maintainers and legitimate visitors to your website. If you use this module then you do so at your own risk.

Requirements

  • Views 2 module
  • A configurable firewall (ideally)
  • A running Drupal cron task (ideally)
  • RobotsTxt module or access to your robots.txt file
  • This module will not currently work on a Windows environment because it assumes that the '/' character separates path components.

More information

For much more detailed help and information see the README.html file in the module's docs folder or visit http://www.moofreechocolates.com/mikey-bunny/honeytrap.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mikey Bunny’s picture

Status: Active » Needs review

Please review and provide me with any feedback. Thanks.

mermentau’s picture

You forgot your sandbox link.

Mikey Bunny’s picture

Thanks, I knew that I'd miss something. The Honeytrap sandbox link is:

http://drupal.org/sandbox/MikeyBunny/1087828

Mikey Bunny’s picture

Assigned: Unassigned » Mikey Bunny
Status: Needs review » Needs work

After talking to the local OPs guys and another developer I am making some minor changes to the module in order to make its use slightly simpler.

mermentau’s picture

Assigned: Mikey Bunny » Unassigned

From what I'm seeing the Assigned too: would be the name of the one taking on the review of your project. Changed it for you.

Jackinloadup’s picture

subscribing. Looks like an interesting module.

Mikey Bunny’s picture

Status: Needs work » Needs review

Ready for review. I'll be releasing a firewall module which will also work with Honeytrap to provide a simple firewall solutions after this.

clemens.tolboom’s picture

The forms directory is not really drupal.

You can user either honeytrap.pages.inc or honeytrap.admin.inc for this depending on the purpose of the pages / forms.

See ie http://api.drupal.org/api/drupal/modules--node--node.pages.inc/6 and http://api.drupal.org/api/drupal/modules--node--node.admin.inc/6

Mikey Bunny’s picture

I have now moved all of the form functions in to honeytrap.admin.inc and tested that they still work. If you already have the Honeytrap module installed you will need to clear your menu's cache once you get the update.

Mikey Bunny’s picture

I will be releasing another module shortly which Honeytrap will optionally integrate with to provide firewall functionality for those that do not have access to their firewall settings.

Berdir’s picture

Status: Needs review » Needs work

Some hints...

- You should not add a LICENSE.txt file, that will be added automatically
- Never seen an @ingroup tag directly on the @file docblock, not sure if that is wrong but I doubt it gets parsed. Also, it's not really necessary to do that for your install hooks, this is already quite obvious as they are in the .install file :)
- When adding @see tags, also add (), e.g. "@see honeytrap_uninstall()".
- You forgot to commit honeytags.admin.inc
- "$form_path = drupal_get_path('module', 'honeytrap') . '/forms';" that line seems unecessary now
- Haven't seen your admin settings callback as it is missing, but when you save all settings in a single variable, then you can't use system_settings_form() I guess and will have to deal with saving them yourself. Not really wrong, just uncommon...
- No reason to make the hook_cron() implementation directly accessible through the menu system?
- It's not necessary to prefix your page callbacks with _
- The text after @param $variable should be on the next line, intended by two spaces.
- You should use l() in your footer hook,you can pass in additional attributes, see http://api.drupal.org/api/drupal/includes--common.inc/function/l/7

Mikey Bunny’s picture

I've been battling with git for the past week to try and recover the contents of the missing file. I'll add and commit the updates just as soon as I can work out how to recover everything.

Mikey Bunny’s picture

Status: Needs work » Needs review

Thank's Berdir. I have updated the code with your recommendations and added the missing file. I have also fixed an issue with the Options field in the view and have made some minor documentation updates regarding the cron.

pillarsdotnet’s picture

Component: new project application » module
FileSize
829.68 KB

Please:

  1. Download and install the Coder module.
  2. Visit admin/modules to enable both "Coder" and "Coder Review" modules
  3. Visit admin/config/development/coder/review.
  4. Under apply the checked coding reviews select minor (most).
  5. Under WHAT TO REVIEW and SELECT SPECIFIC MODULES check the box next to honeytrap.
  6. Click the Run reviews button at the bottom.
  7. Correct the problems noted.

I have done this for you once. The results are attached.

Additional problems not covered by Coder module:

README.txt
Installation instructions should reference http://drupal.org/documentation/install/modules-themes/modules-7 rather than http://drupal.org/node/70151.

I'm impressed. There was very little I could pick at.

Leaving this at "needs review" because someone may consider it RTBC-worthy despite the minor errors found.

Mikey Bunny’s picture

Thank you pillarsdotnet. I'm not sure what to do as http://drupal.org/documentation/install/modules-themes/modules-7 refers to Drupal 7 installations and this is currently only a Drupal 6 module. I will spend time making any necessary changes to make a D7 version of this module one it is approved. I'd appreciate any feedback that you may have regarding this.

Thanks

Mike

pillarsdotnet’s picture

Sorry; the proper link for d6 installation instructions is http://drupal.org/documentation/install/modules-themes/modules-5-6. I was reviewing several d7 modules and I must have gotten yours mixed in with them.

(looking...)

Well, most of the coder review notices apply whether you're running D6 or D7. Do your best to clean things up and I'll take another look.

Here are corrected instructions for d6 coder review:

  1. Download and install the Coder module.
  2. Visit admin/build/modules to enable the "Coder" module.
  3. Visit coder to run Code review.
  4. Under apply the checked coding reviews select minor (most).
  5. Under What to review, uncheck active modules and themes and check include files (inc | php | install | test).
  6. Under Select specific modules, check the box next to your module.
  7. Click the Submit button at the bottom.
  8. Correct the problems noted.
Mikey Bunny’s picture

Thanks again pillarsdotnet. I've ran coder and made the corrections. There is still one outstanding issue on several lines I get the following:

@see references should be separated by "," followed by a single space and with no trailing punctuation

I've tried everything that I can think of but can't get this to pass. I've looked at the Drupal doxygen standards for @see at http://drupal.org/node/1354 but still can't see that I've done anything different from the examples there.

Any advice on how to fix the @see problem would be greatly appreciated.

Thanks

MB

mermentau’s picture

Any advice on how to fix the @see problem would be greatly appreciated.

I had the same problem and think it is a bug. If you comply with the coding standards page I wouldn't worry.

pillarsdotnet’s picture

Your honeytrap.admin.inc file has DOS line-endings. Please convert to Unix line-endings.

In honeytrap.module, I think @sa should be changed to @see, unless @sa means something I'm not familiar with.

In honeytrap.views.default.inc on line 25, you have a block comment that should be changed to an inline comment. Additionally, it should probably be moved to its own line to avoid running over 80 characters line length.

I'm trying to install and test now.

pillarsdotnet’s picture

FileSize
3.77 KB

I got some undefined variable warnings for $cron_url.

Also, you might want to note either in the module documentation or in a hook_requirements() implementation that this module will not work on Windows, because it assumes that the '/' character separates path components.

Here are some additional suggested changes.

pillarsdotnet’s picture

Changing the DOS line endings to Unix line endings resolves the bogus @see warnings from coder.

pillarsdotnet’s picture

(looking...) Actually, my suggested change to honeytrap.admin.inc is wrong. It should make the same check as _honeytrap_get_list_filename() before checking with is_writable(). But I see you've already begun to make some changes in the repository, so I'll wait for you to finish.

Mikey Bunny’s picture

Thanks again pillarsdotnet. I have updated the code as you suggested. I'm so glad that you spotted the DOS line endings; I usually work on Linux but I edited some on the code on a windows machine and the coder error that it caused was driving me mad.

Re you comment: "In honeytrap.views.default.inc on line 25, you have a block comment that should be changed to an inline comment. Additionally, it should probably be moved to its own line to avoid running over 80 characters line length."

The entry that you refer to is generated by a views 2 export. Because this code is automatically generated anyway I will remove the comment.

I've also added a hook for the robotstxt module which will automatically add the required "Disallow" entries if the robotstxt module is enabled. Finally I ran coder again after all of my code changes.

I look forwards to receiving further feedback.

pillarsdotnet’s picture

Status: Needs review » Needs work
FileSize
63.86 KB

The entry that you refer to is generated by a views 2 export. Because this code is automatically generated anyway I will remove the comment.

If I were you, I'd open a bug report against views 2.

Reviewing now... I must say that I like the idea of your module very much. Once it's approved, I'll probably submit a d7-upgrade patch.

Turned on Internationalization checks and found more coder issues. See attached. Also:

honeytrap.admin.inc line 55
  $form['settings']['url']['disallowed_paths'] = array(
    '#type' => 'textarea',
    '#title' => t('Disallowed paths'),
    '#description' => t('One entry per line.'),
    '#default_value' => $disallowed_paths,
    '#cols' => 60,
    '#rows' => 5,
    '#required' => TRUE,
  );

You need some more description here. I'm testing http://example.com/d6/index.php?q=admin/settings/honeytrap and trying to get my own IP address added to the blacklist. I've tried inserting the following into the Disallowed paths block, to no success:

  • admin
  • /admin
  • index.php?q=admin
  • /index.php?q=admin
  • d6/index.php?q=admin
  • /d6/index.php?q=admin

Your users shouldn't have to resort to trial-and-error to figure out what values are appropriate.

honeytrap.admin.inc line 97
  $form['settings']['list_files']['list_files_path'] = array(
    '#type' => 'textfield',
    '#title' => t('List file path'),
    '#description' =>
      '',
    '#default_value' => $honeytrap_settings['list_files']['path'],
    '#size' => 60,
    '#maxlength' => 254,
    '#required' => FALSE,
  );

You need some more description here. At a minimum, you should explain:

  • What happens when the value is left blank
  • What happens when the value starts with a slash "/" character.
  • What happens when the value does not start with a slash character.

As an aside, you might want to consider asking for three complete paths (black-list path, white-list path, naughty-list path) rather than three path parts (file path, prefix, suffix). This would provide additional flexibility at no additional cost.

honeytrap.admin.inc line 136
  $form['settings']['cron'] = array(
    '#type' => 'fieldset',
    '#title' => t('Honeytrap cron'),
    '#description' => t(
      "The Honeytrap cron task can be ran either by using the standard Drupal cron. "
    ),
    '#collapsible' => TRUE,
    '#collapsed' => FALSE,
  );

This description is confusing because you state one of the "either/or" options but not the other. Also, if there is a separate URL for invoking honeytrap cron, you should state it here.

honeytrap.admin.inc line 733
      $description .=
        '<li>' .
          $honeytrap_settings['list_files']['path'] .
          $honeytrap_settings['list_files']['prefix'] .
          $list_name .
          $honeytrap_settings['list_files']['suffix'];
        '</li>';

There should be a '/' between $honeytrap_settings['list_files']['path'] and $honeytrap_settings['list_files']['prefix'].

Mikey Bunny’s picture

Thank you again pillarsdotnet for your very concise and helpful instructions and advice.

I have now enabled internationalization on coder and made the required corrections. I have simplified the settings page and added more descriptions and help text. I also changed the list files to be three separate fields as you suggested as I agree that the interface is much simpler that way. Finally after testing everything I ran coder again to ensure that the code is still ship-shape and bristol fashion.

When you get time, please let me know what you think this time around. Cheers.

Mikey Bunny’s picture

Status: Needs work » Needs review
pillarsdotnet’s picture

Status: Needs review » Needs work
  • The coder internationalization checks missed this, but the first argument to the t() function should be a single literal text string with no concatenation, variables, or operators. In some places, you have concatenated several strings together, probably in an effort to keep your line length down. This is incompatible with the way the Drupal localization server will process your files. For instance, in honeytrap.admin.inc lines 44-48, you have:
        '#description' => t(
          'Please ensure that the paths specified here are also added to your robot.txt file or equivalent.' .
          ' These entries will be automatically added to the robots.txt file if you have the RobotsTxt module installed.' .
          ' DO NOT use paths that a regular user to the site may visit.'
        ),
    

    One way to simultaneously keep line length down and satisfy multilingual requirements is as follows:

      '#description' => t(
        'Please ensure that the paths specified here are also added to your %robots.txt file or equivalent.
    These entries will be automatically added to the %robots.txt file if you have the %robotstxt module installed.
    DO NOT use paths that a regular user to the site may visit.',
        array('%robots.txt' => 'robots.txt', '%robotstxt' => 'RobotsTxt')
      ),
    

    See the Localization API and Dynamic strings with placeholders pages for details.

  • When submitting the admin/settings/honeytrap page, I received the following notices:
    • Notice: Undefined index: enabled in honeytrap_settings_form_submit() (line 286 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: enabled in honeytrap_settings_form_submit() (line 286 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: path in honeytrap_settings_form_submit() (line 288 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: path in honeytrap_settings_form_submit() (line 288 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: prefix in honeytrap_settings_form_submit() (line 289 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: prefix in honeytrap_settings_form_submit() (line 289 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: suffix in honeytrap_settings_form_submit() (line 290 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
    • Notice: Undefined index: suffix in honeytrap_settings_form_submit() (line 290 of d6/sites/all/modules/honeytrap/honeytrap.admin.inc).
  • When clicking on the View lists tab, I received the following notices:
    • Notice: Undefined variable: type in views_get_handler() (line 803 of d6/sites/all/modules/views/views.module).
    • Warning: Creating default object from empty value in views_many_to_one_helper->ensure_my_table() (line 859 of d6/sites/all/modules/views/includes/handlers.inc).
Mikey Bunny’s picture

Status: Needs work » Needs review

Thanks again. I have changed the t() functions and switched on PHP notices. I then fixed the code that was causing the first notice. However, I'm not getting the notices on the view list page. Please can you let me know the steps to get this? Is it as simple as visiting the list?

Thanks

pillarsdotnet’s picture

Status: Needs review » Needs work

Okay, here's what I did:

  1. Installed fresh Drupal-6 on localhost as http://example.com/d6/
  2. Visited admin/settings/honeytrap
  3. Set Disallowed paths to /admin
  4. Set Black list filename to /tmp/black
  5. Left White list filename blank
  6. Set Naughty list filename to /tmp/naughty
  7. Clicked on the Save changes button
  8. Clicked on the View lists tab

(Investigating the views notice...)

Ah. This is a bug in Views 3 that has since been fixed. See #1062506: Misnamed parameter for creation of broken handlers.

(Pulling latest views from git repository and trying again...)

Still getting:

Warning: Creating default object from empty value in views_many_to_one_helper->ensure_my_table() (line 900 of d6/sites/all/modules/views/includes/handlers.inc).

(Investigating...)

Another views bug. Filed patch at #1174130-7: Strict warning: Creating default object from empty value in views_many_to_one_helper->ensure_my_table()

Okay, no PHP notices now. Let's see if I triggered the blacklist...

(running cron manually...)

Got another notice:

Notice: Undefined variable: retval in honeytrap_cron() (line 171 of d6/sites/all/modules/honeytrap/honeytrap.module).

Yup. That return $retval should be return TRUE or removed altogether.

Let's see if my IP showed up in a list...

Nope. Why not?

(Looking at code some more...)

Since your list names are exposed in user-visible admin forms, you ought to make them translatable. Right now they're stored in HONEY_TRAP_LISTS which is not translatable. If you instead defined them in a hook_init() implementation, you could wrap the static text in t().

(looking some more...)

Now I understand! The Disallowed paths block must be filled with paths that do not otherwise exist in the menu system. So instead of /admin I should have supplied and then visitied /admin/foo.

(testing...)

That worked, but even a rogue robot isn't going to visit /admin/foo unless I supply a link that points there.

Okay, so changes needed for final review:

  • In honeytrap_cron(), remove the return $retval line as hook_cron() doesn't need to return anything.
  • In the Disallowed paths description, note that these paths should not already exist in the menu system.

Some nice-to-have items on my personal wishlist, not at all required for project approval:

  • Provides a validation callback that returns an error message if any of the Disallowed paths exist elsewhere in the menu system.
  • Provide a hook_block() implementation that contains invisible links with randomly-generated link text that lead to the Disallowed paths. This should then be added to the front page header region.
  • Provide a theme wrapper for the information written to the white/black/naughty lists so that a motivated sysadmin can make them directly readable by whatever firewall program is being used.
  • Make the list name titles (black/white/naughty) translatable by passing them as literal strings through the t() function.
  • Provide a D7 version of the module.

NOTE TO SELF: ping mlncn on IRC when this is RTBC

sun’s picture

  // Check if the Honeytrap variable exists from a previous installation
  if (!variable_get('honeytrap_settings', FALSE)) {
    // Set the default values in the honeytrap_settings variable
    $honeytrap_settings = array(
      'cron' => array(
        'expired_entry_time' => 86400, // 24 hours
        'white_list' => array(),
      ),
      'url' => array(
        'disallowed_paths' => array(
          '/example1',
          '/example2'
        ),
        'rand_min' => 10,
        'rand_max' => 10000,
      ),
/*      'list_files' => array(
        'enabled' => 0,
        'path' => '',
        'prefix' => 'honeytrap-',
        'suffix' => '.list',
      ),*/
      'list_files' => array(
        'black' => '',
        'white' => '',
        'naughty' => '',
      ),
    );
    variable_set('honeytrap_settings', $honeytrap_settings);
  }

These should be separate variables normally.

      // Ensure that any whitespace and trailing or leading slashes are removed
      $naughty_url = trim($naughty_url, '/');
      $items[$naughty_url] = array(

This needs to happen during form validation.

In general, these dynamically defined callbacks are risky. The form validation also has to verify that the entered path does not clash with an existing system path (which likely involves checking the router directly).

      $items[$naughty_url . '/%'] = array(
        'access callback' => TRUE,
        'page callback' => 'honeytrap_naughty_link',
        'type' => MENU_CALLBACK,
      );

This second router item can be removed. The callback of the first definition already gets any additional arguments.

watchdog('Honeytrap', 
watchdog('Honeytrap log'

Watchdog category names are normally all-lowercase.

Also, a module should normally use only one category name, if possible.

function honeytrap_view_list($list) {
  // Surface the text for the items on the specified list (for firewall etc)
  $sql = "SELECT * FROM {honeytrap_list} WHERE list = '%s'";
  $result = db_query($sql, $list);
  while ($item = db_fetch_array($result)) {
    print theme('honeytrap_list_item', $item);
  }
  return NULL;
}

A page callback is not supposed to print/output anything directly. The output has to be returned.

define("HONEY_TRAP_LISTS", "black,white,naughty");

Any reason for why this is not a variable?

  // Force the page to a 404 so that the webcrawler is not alerted that it just
  // triggered the honeytrap
  header("HTTP/1.0 404 Not Found");

  return NULL;

The page callback should simply return MENU_NOT_FOUND instead.

function honeytrap_views_data() {
function honeytrap_views_handlers() {

I'd recommend to move the Views hook implementations into a separate honeytrap.views.inc file, so all of that code is not loaded on every single request (improves performance).

  if ($enabled) {
    $form['lists']['purge_lists'] = array(
      '#type' => 'submit',
      '#value' => t('Purge list files'),
      '#name' => 'purge_lists',
    );
  }
...
function honeytrap_maintenance_form_submit($form, &$form_state) {
  switch ($form_state['clicked_button']['#name']) {
    case 'purge_lists':
      _honeytrap_purge_lists(TRUE);
      drupal_set_message(t('Any expired entries were removed from the list files.'));
      break;
  }
}

Usage of switch statements in form submit handlers is discouraged since Drupal 6. Instead, each form button is able to specify a dedicated form submit handler, if necessary:

  $form['lists']['purge_lists'] = array(
    '#type' => 'submit',
    '#value' => t('Purge list files'),
    '#access' => $enabled,
    '#submit' => array('honeytrap_maintenance_form_submit_purge'),
  );
pillarsdotnet’s picture

I agree with most of @sun's review, but:

These should be separate variables normally.

For the record, that's a matter of personal preference and not part of coding standards.

Mikey Bunny’s picture

Thank you very much for all you help and feedback pillarsdotnet and sun. I have addressed most of these issues but have a few questions about the few remaining ones below:

[honeytrap_settings variable array] These should be separate variables normally.

both: The variable array is now built in to the structure of the module and the way that it uses it. As changing it will be very disruptive to pretty much all functionality in the module and require extensive testing in order to ensure that everything still works as intended. Can we live with this as it is?

Provide a theme wrapper for the information written to the white/black/naughty lists so that a motivated sysadmin can make them directly readable by whatever firewall program is being used.

pillarsdotnet: This can already be done by defining your own theme hook for "honeytrap_list_item". Did you have something else in mind?

A page callback is not supposed to print/output anything directly. The output has to be returned.

sun: I've removed these callbacks now as they are no longer required. However, please could you let me know what is the approved way of rendering the output returned from a callback function while ensuring that the content is not marked-up in any way with headers/footer etc?

I'd recommend to move the Views hook implementations into a separate honeytrap.views.inc file, so all of that code is not loaded on every single request (improves performance).

sun: I already tried this. If I move them in to the .inc files the view will stop working with the following errors:

Notice: Undefined index: table in view->init_query() (line 536 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/view.inc).
Notice: Undefined property: views_query::$fields in views_query->query() (line 909 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/query.inc).
Warning: Invalid argument supplied for foreach() in views_query->query() (line 936 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/query.inc).
Notice: Undefined property: views_query::$fields in views_query->query() (line 909 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/query.inc).
Warning: Invalid argument supplied for foreach() in views_query->query() (line 911 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/query.inc).
Warning: Invalid argument supplied for foreach() in views_query->query() (line 936 of /mnt/hgfs/www/html/cartoonito/trunk/sites/all/modules/contrib/views/includes/query.inc).
User warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near &#039;FROM honeytrap_list honeytrap_list ) count_alias&#039; at line 2 query: SELECT COUNT(*) FROM (SELECT FROM honeytrap_list honeytrap_list ) count_alias in _db_query() (line 148 of /mnt/hgfs/www/html/cartoonito/trunk/includes/database.mysqli.inc).
User warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near &#039;FROM honeytrap_list honeytrap_list LIMIT 0, 25&#039; at line 2 query: SELECT FROM honeytrap_list honeytrap_list LIMIT 0, 25 in _db_query() (line 148 of /mnt/hgfs/www/html/cartoonito/trunk/includes/database.mysqli.inc).

Any suggestions on how to resolve this other than keeping the functions in the .module file?

pillarsdotnet’s picture

pillarsdotnet: This can already be done by defining your own theme hook for "honeytrap_list_item". Did you have something else in mind?

That works. Docs/example would be nice. Will review the rest later today.

Mikey Bunny’s picture

Thanks pillarsdotnet. Please could you elaborate on exactly what you would like to see in Docs/example so that I get it right? Do you mean add an example of how to define a theme for this in the existing documentation? It does already mention that you will need to define this theme to change it I'm not sure if this is enought?

pillarsdotnet’s picture

The point is that without any links to the honey-trapped pages, a web crawler that ignores robots.txt would not find them anyway.

So your docs should explain this, and explain how to create links that are visible to a web crawler but invisible (or at least obviously undesirable) to a real human.

Ideally, the links should be inserted automatically near the top of the front (home) page.

Mikey Bunny’s picture

Priority: Major » Normal
Status: Needs review » Needs work

The links are already automatically inserted in to every page - see honeytrap_footer(). I will look at making this clearer in the documentation. This is already stated at the top of the settings page as follows:

IMPORTANT: These paths must not already exist or be likely to exist on your site as you do not want a normal user to accidentally visit them. These paths will automatically be added to your site as hidden links.

I hope that this helps?

pillarsdotnet’s picture

Yup. Will do a more thorough review later today.

Mikey Bunny’s picture

Status: Needs work » Needs review
Mikey Bunny’s picture

Priority: Normal » Major

Application priority changes as per http://drupal.org/node/894256 guidelines.

Mikey Bunny’s picture

Issue summary: View changes

Updated description and sanbox url.

Mikey Bunny’s picture

Priority: Normal » Critical
Status: Needs work » Needs review

Application priority changes as per http://drupal.org/node/894256 guidelines.

Mikey Bunny’s picture

...and still the tortuous wait for someone to review modules goes on. It has got to the point now after waiting for over 5 month where I am helping to review 3 other modules now myself in order to try and help get the queue down.

I have several other very useful modules that I would also like to contribute. Does anyone now if I submit more modules to the review queue and then finally one of them gets reviewed will all the other modules have to wait for reviewing after that as well?

Unfortunately I fear that I may just be talking to myself. I know, how about an infuriatingly slow review process that drives the applicant insane? It could be an impossible challenge a bit like trying to get in to the special forces. Now there's an idea...

MB

pillarsdotnet’s picture

...and still the tortuous wait for someone to review modules goes on.

Hey; we're all volunteers here. I didn't even know the module review queue existed until just recently.

It has got to the point now after waiting for over 5 month where I am helping to review 3 other modules now myself in order to try and help get the queue down.

Hooray! Thanks for helping!

Suggested changes recommended by @sun, coder review, and myself attached and annotated as follows:

EDIT: Attachment is wrong. See #45.

  • Spelling correction:
    diff --git a/honeytrap.admin.inc b/honeytrap.admin.inc
    index a4c17f6..5964de0 100644
    --- a/honeytrap.admin.inc
    +++ b/honeytrap.admin.inc
    @@ -1,7 +1,7 @@
     <?php
     /**
      * @file
    - * The Honeytrap admin form function defininitions.
    + * The Honeytrap admin form function definitions.
      *
      * @ingroup forms
      */
    
  • It would be helpful, at least for testing purposes, if the example URL were clickable. Also, I think that line breaks in translatable text should follow periods or commas:
    @@ -23,10 +23,9 @@ function honeytrap_settings_form($form_state) {
       $trap = honeytrap_get_trap_array();
       if ($trap) {
         drupal_set_message(
    -      t(
    -        'An example trap URL based on your current settings is <em>%url</em>. Visit this page to
    -get added to the naughty list.',
    -        array('%url' => $trap['url'])
    +      t('An example trap URL based on your current settings is <em>!url</em>.
    +Visit this page to get added to the naughty list.',
    +        array('!url' => l($trap['url'], $trap['url']))
           )
         );
       }
    
  • My suggestions for better descriptive text; feel free to ignore if you disagree:
    @@ -42,12 +41,14 @@ get added to the naughty list.',
         '#type' => 'fieldset',
         '#title' => t('Honeytrap url'),
         '#description' => t(
    -      '<b>IMPORTANT:</b> These paths must not already exist or be likely to exist on your site as
    -you do not want a normal user to accidentally visit them. These paths will
    -automatically be added to your site as hidden links.<br><br>
    -These entries will automatically be added to your robots.txt file if you have the
    -RobotsTxt module enabled. If you do not have this module enabled then please ensure that the
    -paths specified here are also added to your robot.txt file.'
    +      '<b>IMPORTANT:</b>
    +These paths must not already exist on your site.
    +Honeytrap will add hidden links to these paths at the bottom of each page.<br />
    +<br />
    +The <a href="!robotstxt">RobotsTxt</a> module, if enabled,
    +will warn well-behaved web-crawlers to ignore these links.
    +Otherwise, the paths must be added by manually editing your robots.txt file.',
    +      array('!robotstxt' => 'http://drupal.org/module/robotstxt')
         ),
         '#collapsible' => TRUE,
         '#collapsed' => FALSE,
    @@ -58,11 +59,14 @@ paths specified here are also added to your robot.txt file.'
         '#type' => 'textarea',
         '#title' => t('Disallowed paths'),
         '#description' => t(
    -      'Use one entry per line.<br /><br /><b>Example:</b> If you enter "/admin" on a line then the
    -IP address of anyone who vists the URL "/admin" will be added to the naughty list. If you have the
    -RobotsTxt module installed then the entries "Disallow: /admin" (clean URLs) and
    -"Disallow: /?q=admin" (no clean URLs) will be added to your robots.txt file. You should manually
    -add the correct entries if you do not have the RobotsTxt module enabled.'
    +      'Use one entry per line.<br />
    +<br />
    +<b>Example:</b> If you add a line "example",
    +then the IP address of anyone who visits the URL "/example" will be added to the naughty list.
    +If you have the RobotsTxt module installed,
    +then the entries "Disallow: /example" (clean URLs),
    +and "Disallow: /?q=example" (no clean URLs) will be added to your robots.txt file.
    +You should manually add the correct entries if you do not have the RobotsTxt module enabled.'
         ),
         '#default_value' => $disallowed_paths,
         '#cols' => 60,
    @@ -78,9 +82,15 @@ add the correct entries if you do not have the RobotsTxt module enabled.'
         '#type' => 'fieldset',
         '#title' => t('List files (optional)'),
         '#description' => t(
    -      'These files will be required by some firewalls. If used, the folders for these files must
    -exist and be writable. Remember, these are file names, so if you start their paths with a slash
    -then it will indicate the root of your filesystem and not your website.'
    +      'These files will be required by some firewalls.
    +If used, the folders for these files must exist and be writable.
    +File names will be evaluated from the current working directory.<br />
    +For example, %relative is in the default Drupal files directory,
    +but %absolute is in the Unix filesystem temporary directory.',
    +      array(
    +        '%relative' => 'sites/default/files/list.txt',
    +        '%absolute' => '/tmp/list.txt',
    +      )
         ),
         '#collapsible' => TRUE,
         '#collapsed' => FALSE,
    
  • Coder review says use <br /> instead of <br>:
    diff --git a/honeytrap.admin.inc b/honeytrap.admin.inc
    index a4c17f6..41d2db6 100644
    --- a/honeytrap.admin.inc
    +++ b/honeytrap.admin.inc
    @@ -44,7 +44,7 @@ get added to the naughty list.',
         '#description' => t(
           '<b>IMPORTANT:</b> These paths must not already exist or be likely to exist on your site as
     you do not want a normal user to accidentally visit them. These paths will
    -automatically be added to your site as hidden links.<br><br>
    +automatically be added to your site as hidden links.<br /><br />
     These entries will automatically be added to your robots.txt file if you have the
     RobotsTxt module enabled. If you do not have this module enabled then please ensure that the
     paths specified here are also added to your robot.txt file.'
    
  • My suggestion for clearer description text for the "List files" fieldset:
    @@ -78,9 +78,15 @@ add the correct entries if you do not have the RobotsTxt module enabled.'
         '#type' => 'fieldset',
         '#title' => t('List files (optional)'),
         '#description' => t(
    -      'These files will be required by some firewalls. If used, the folders for these files must
    -exist and be writable. Remember, these are file names, so if you start their paths with a slash
    -then it will indicate the root of your filesystem and not your website.'
    +      'These files will be required by some firewalls.
    +If used, the folders for these files must exist and be writable.
    +File names will be evaluated from the current working directory.<br />
    +For example, %relative is in the default Drupal files directory,
    +but %absolute is in the root filesystem temporary directory.',
    +      array(
    +        '%relative' => 'sites/default/files/list.txt',
    +        '%absolute' => '/tmp/list.txt',
    +      )
         ),
         '#collapsible' => TRUE,
         '#collapsed' => FALSE,
    
  • If you insert line breaks into translatable text, I think they should be at the end of sentences or phrases:
    @@ -90,8 +96,8 @@ then it will indicate the root of your filesystem and not your website.'
         '#type' => 'textfield',
         '#title' => t('Black list filename'),
         '#description' => t(
    -      'Leave blank if your firewall does not need this file.<br /> The IP addresses added by the
    -Honeytrap to this file should be blocked by firewalls.'
    +      'Leave blank if your firewall does not need this file.<br />
    +The IP addresses Honeytrap adds to this file should be blocked by firewalls.'
         ),
         '#default_value' => $honeytrap_settings['list_files']['black'],
         '#size' => 80,
    @@ -103,8 +109,8 @@ Honeytrap to this file should be blocked by firewalls.'
         '#type' => 'textfield',
         '#title' => t('White list filename'),
         '#description' => t(
    -      'Leave blank if your firewall does not need this file.<br /> IP addresses added by the
    -Honeytrap to this file should always be allowed by firewalls.'
    +      'Leave blank if your firewall does not need this file.<br />
    +The IP addresses Honeytrap adds to this file should be allowed by firewalls.'
         ),
         '#default_value' => $honeytrap_settings['list_files']['white'],
         '#size' => 80,
    @@ -116,8 +122,8 @@ Honeytrap to this file should always be allowed by firewalls.'
         '#type' => 'textfield',
         '#title' => t('Naughty list filename'),
         '#description' => t(
    -      'Leave blank if your firewall does not need this file.<br /> IP addresses added by the
    -Honeytrap to this file should nomally be throttled by firewalls.'
    +      'Leave blank if your firewall does not need this file.<br />
    +The IP addresses Honeytrap adds to this file should be throttled by firewalls.'
         ),
         '#default_value' => $honeytrap_settings['list_files']['naughty'],
         '#size' => 80,
    @@ -138,7 +148,7 @@ Honeytrap to this file should nomally be throttled by firewalls.'
         '#type' => 'textfield',
         '#title' => t('Auto expiry time in seconds (0 for none)'),
         '#description' => t(
    -      'The minimum amount of time in seconds that an automatically added entry will remain on the lists.'
    +      'The minimum number of seconds that an automatically added entry will remain on the lists.'
         ),
         '#default_value' => $honeytrap_settings['cron']['expired_entry_time'],
         '#size' => 8,
    
  • Spelling correction:
    @@ -168,7 +178,7 @@ Honeytrap to this file should nomally be throttled by firewalls.'
      * @ingroup forms
      */
     function honeytrap_settings_form_validate($form, &$form_state) {
    -  // Check that the minumum random value is less than or equal to the maximum
    +  // Check that the minimum random value is less than or equal to the maximum
       // random value
       if ($form_state['values']['rand_min'] > $form_state['values']['rand_max']) {
         form_set_error(
    
  • More line-break adjustments:
    @@ -185,7 +195,8 @@ function honeytrap_settings_form_validate($form, &$form_state) {
         form_set_error(
           'list_filename_black',
           t(
    -        'The black list filename is not valid. Please check that the folder exists and is writeable.',
    +        'The black list filename is not valid.
    +Please check that the folder exists and is writable.',
             array('%filename' => $form_state['values']['list_filename_black'])
           )
         );
    @@ -199,7 +210,8 @@ function honeytrap_settings_form_validate($form, &$form_state) {
         form_set_error(
           'list_filename_white',
           t(
    -        'The white list filename is not valid. Please check that the folder exists and is writeable.',
    +        'The white list filename is not valid.
    +Please check that the folder exists and is writable.',
             array('%filename' => $form_state['values']['list_filename_white'])
           )
         );
    @@ -213,7 +225,8 @@ function honeytrap_settings_form_validate($form, &$form_state) {
         form_set_error(
           'list_filename_naughty',
           t(
    -        "The naughty list filename is not valid. Please check that the folder exists and is writeable.",
    +        "The naughty list filename is not valid.
    +Please check that the folder exists and is writable.",
             array('%filename' => $form_state['values']['list_filename_naughty'])
           )
         );
    
  • This is what @sun meant by moving the trim() changes to the validation function. Also fixes a style error; Drupal if blocks should always use curly-braces:
    @@ -221,22 +234,28 @@ function honeytrap_settings_form_validate($form, &$form_state) {
    
       // Get the disallowed paths
       $disallowed_paths = explode("\n", $form_state['values']['disallowed_paths']);
    +  $corrected_paths = array();
       foreach ($disallowed_paths as $index => $path) {
    -    // Remove any whitespace and leading/traling slashes for menu checks
    +    // Remove any whitespace and leading/trailing slashes for menu checks
         $path = check_plain(trim($path, " /\r\n"));
    
         // Check if the path already exist
         $router_item = menu_get_item($path);
    -    if (($router_item !== FALSE) && ($router_item['page_callback'] != 'honeytrap_naughty_link'))
    +    if (($router_item !== FALSE) && ($router_item['page_callback'] != 'honeytrap_naughty_link')) {
           form_set_error(
             'disallowed_paths',
             t(
    -          "The menu path %path already exists. Please choose something more obscure.",
    +          'The menu path %path already exists.
    +Please choose something more obscure.',
               array('%path' => $path)
    -      )
    -    );
    +        )
    +      );
    +    }
    +    else {
    +      $corrected_paths[] = $path;
    +    }
       }
    -
    +  $form_state['values']['disallowed_paths'] = implode("\n", $corrected_paths);
     }
    
     /**
    
  • Use single-quotes where you can; they're marginally faster:
    @@ -440,7 +459,7 @@ function honeytrap_add_ip_form_submit($form, &$form_state) {
       )) {
         drupal_set_message(
           t(
    -        "The IP address <em>%ip</em> was added to the <em>%list</em> list.",
    +        'The IP address <em>%ip</em> was added to the <em>%list</em> list.',
             array('%ip' => $form_state['values']['new-ip'], '%list' => $form_state['values']['list'])
           )
         );
    @@ -450,7 +469,7 @@ function honeytrap_add_ip_form_submit($form, &$form_state) {
       else {
         drupal_set_message(
           t(
    -        "The IP address <em>%ip</em> could not be added to the <em>%list</em> list.",
    +        'The IP address <em>%ip</em> could not be added to the <em>%list</em> list.',
             array(
               '%ip' => $form_state['values']['new-ip'],
               '%list' => $form_state['values']['list'],
    
  • Spelling corrections:
    @@ -716,7 +735,7 @@ function honeytrap_edit_ip_form_submit($form, &$form_state) {
    
     /**
      * @file
    - * Form builder for the maintainance form.
    + * Form builder for the maintenance form.
      *
      * @param $form_state
      *   Standard hook_form() parameter
    @@ -745,7 +764,7 @@ function honeytrap_maintenance_form(&$node) {
       $form = array();
    
       if ($enabled) {
    -    $description = t('Remove any old entries from the list files immedaitely rather than waiting for the cron.');
    +    $description = t('Remove any old entries from the list files immediately rather than waiting for the cron.');
       }
       else {
         $description = t('You must specify at least one list filename on the <em>Settings</em> page first.');
    
  • Whitespace (indent) correction from coder review:
    @@ -759,7 +778,7 @@ function honeytrap_maintenance_form(&$node) {
         '#collapsed' => FALSE,
       );
    
    - $form['lists']['purge_lists'] = array(
    +  $form['lists']['purge_lists'] = array(
         '#type' => 'submit',
         '#value' => t('Purge list files'),
         '#access' => $enabled,
    
  • Typo fix:
    @@ -777,7 +796,7 @@ function honeytrap_maintenance_form(&$node) {
      * @param $form_state
      *   Standard hook_submit() parameter
      *
    - * @see honeytrap_maintenance_form_submit_form()
    + * @see honeytrap_maintenance_form()
      * @see _honeytrap_purge_lists()
      *
      * @ingroup forms
    
  • Leading slashes are invalid in drupal paths:
    diff --git a/honeytrap.install b/honeytrap.install
    index 4b4cb34..f0b027a 100644
    --- a/honeytrap.install
    +++ b/honeytrap.install
    @@ -26,8 +26,8 @@ function honeytrap_install() {
         ),
         'url' => array(
           'disallowed_paths' => array(
    -        '/example1',
    -        '/example2'
    +        'example1',
    +        'example2'
           ),
           'rand_min' => 10,
           'rand_max' => 10000,
    
  • Debatable spelling correction; feel free to ignore:
    --- a/honeytrap.module
    +++ b/honeytrap.module
    @@ -2,7 +2,7 @@
     /**
      * @file
      * The Honeytrap module file.
    - * The Honeytrap module tracks and logs non compliant spiders and webcrawlers
    + * The Honeytrap module tracks and logs non compliant spiders and web-crawlers
      * for use by systems such as firewalls. This allows web site owners and
      * administrators to trap, throttle and/or block IP addresses and user agents
      * that are putting unnecessary heavy load on a server(s).
    
  • Slashes should be trimmed in the validation form:
    @@ -91,8 +91,8 @@ function honeytrap_menu() {
       $honeytrap_settings = variable_get('honeytrap_settings', array());
       if (is_array($honeytrap_settings['url']['disallowed_paths'])) {
         foreach ($honeytrap_settings['url']['disallowed_paths'] as $naughty_url) {
    -      // Ensure that any whitespace and trailing or leading slashes are removed
    -      $naughty_url = trim($naughty_url, '/');
    +      // Slashes were trimmed in honeytrap_settings_form_validate().
    +      // $naughty_url = trim($naughty_url, '/');
           $items[$naughty_url] = array(
             'access callback' => TRUE,
             'page callback' => 'honeytrap_naughty_link',
    
  • Lowercase the first watchdog() argument:
    @@ -148,7 +148,7 @@ function honeytrap_cron() {
       _honeytrap_purge_lists();
       _honeytrap_flush_buffers();
    
    -  watchdog('Honeytrap', 'The Honeytrap cron ran successfully.', array(), WATCHDOG_INFO);
    +  watchdog('honeytrap', 'The Honeytrap cron ran successfully.', array(), WATCHDOG_INFO);
     }
    
     /**
    @@ -170,7 +170,7 @@ function _honeytrap_purge_lists($force_regenerate = FALSE) {
         $sql = "DELETE FROM {honeytrap_list} WHERE auto = 1 AND modified < %d";
         db_query($sql, time() - $honeytrap_settings['cron']['expired_entry_time']);
    
    -    watchdog('Honeytrap', 'The Honeytrap lists were purged.', array(), WATCHDOG_INFO);
    +    watchdog('honeytrap', 'The Honeytrap lists were purged.', array(), WATCHDOG_INFO);
       }
    
       if ($force_regenerate) {
    
  • Spelling corrections / adjustment from UK English to American English:
    @@ -180,7 +180,7 @@ function _honeytrap_purge_lists($force_regenerate = FALSE) {
     }
    
     /**
    - * A maintainance task to regenerate all of the list files.
    + * A maintenance task to regenerate all of the list files.
      *
      * @see _honeytrap_list_file_recreate()
      * @see _honeytrap_purge_lists()
    @@ -246,10 +246,10 @@ function honeytrap_get_trap_array() {
    
       // Get a random disallowed path
       $rand_index = array_rand($honeytrap_settings['url']['disallowed_paths']);
    -  $dissallowed_path = rtrim($honeytrap_settings['url']['disallowed_paths'][$rand_index], '/');
    +  $disallowed_path = rtrim($honeytrap_settings['url']['disallowed_paths'][$rand_index], '/');
    
       // Check if a random number should be added to the url
    -  $url = $dissallowed_path;
    +  $url = $disallowed_path;
       if ($honeytrap_settings['url']['rand_max']) {
         // Get a random number
         $rand_number = rand($honeytrap_settings['url']['rand_min'], $honeytrap_settings['url']['rand_max']);
    @@ -257,7 +257,7 @@ function honeytrap_get_trap_array() {
       }
    
       $trap = array(
    -    'disallowed_path' => $dissallowed_path,
    +    'disallowed_path' => $disallowed_path,
         'url' => $url,
       );
    
    @@ -273,8 +273,8 @@ function honeytrap_get_trap_array() {
     function honeytrap_naughty_link() {
       $honeytrap_settings = variable_get('honeytrap_settings', array());
    
    -  // Clear out any old entries so that we do not penalise anyone that committed
    -  // an offence a long time ago
    +  // Clear out any old entries so that we do not penalize anyone that committed
    +  // an offense a long time ago
       _honeytrap_purge_lists();
    
       // Get the IP address of the current user
    
  • Set a temporary variable to avoid repetition later:
    @@ -341,9 +341,9 @@ function _honeytrap_flush_buffers() {
      */
     function _honeytrap_list_file_add_ip($ip, $list) {
       $honeytrap_settings = variable_get('honeytrap_settings', array());
    -
    +  $list_filename = $honeytrap_settings['list_files'][$list];
       // Check that this list file is required
    -  if ($honeytrap_settings['list_files'][$list]) {
    +  if ($list_filename) {
         // Get the output text
         $sql = "SELECT * FROM {honeytrap_list} WHERE ip = '%s'";
         if ($result = db_query($sql, $ip)) {
    @@ -351,12 +351,12 @@ function _honeytrap_list_file_add_ip($ip, $list) {
           $output = theme('honeytrap_list_item', $item);
    
           // Add the text to the list file
    -      if (!error_log($output, 3, $honeytrap_settings['list_files'][$list])) {
    +      if (!error_log($output, 3, $list_filename)) {
             // Error adding entry to the list file
    
  • More lowercasing:
             watchdog(
    -          'Honeytrap',
    +          'honeytrap',
               "Unable to write output for IP '%ip' to list file '%filename'.",
    -          array('%ip' => $ip, '%filename#' => $honeytrap_settings['list_files'][$list]),
    +          array('%ip' => $ip, '%filename#' => $list_filename),
               WATCHDOG_ERROR
             );
           }
    @@ -364,7 +364,7 @@ function _honeytrap_list_file_add_ip($ip, $list) {
         else {
           // Could not find the entry to add to the list file in the db
           watchdog(
    -        'Honeytrap',
    +        'honeytrap',
             "Unable to find the database entry for '%ip' to write of to the list file '%filename'.",
             array('%ip' => $ip, '%filename#' => $list_filename),
             WATCHDOG_ERROR
    
  • Spelling correction:
    @@ -374,7 +374,7 @@ function _honeytrap_list_file_add_ip($ip, $list) {
     }
    
     /**
    - * Remove the spcified IP address from a list.
    + * Remove the specified IP address from a list.
      *
      * @param $ip
      *   The name of the IP address to remove.
    
  • More temporary variable usage:
    @@ -394,9 +394,9 @@ function _honeytrap_list_file_remove_ip($ip, $list) {
      */
     function _honeytrap_list_file_recreate($list) {
       $honeytrap_settings = variable_get('honeytrap_settings', array());
    -
    +  $list_filename = $honeytrap_settings['list_files'][$list];
       // Check if a file is required for this list
    -  if ($honeytrap_settings['list_files'][$list]) {
    +  if ($list_filename) {
         // Get all of the entries for the list from the db
         $sql = "SELECT * FROM {honeytrap_list} WHERE list = '%s'";
         $result = db_query($sql, $list);
    @@ -406,12 +406,12 @@ function _honeytrap_list_file_recreate($list) {
         }
    
         // Replace the contents of the file with the updated contents
    -    if (file_put_contents($honeytrap_settings['list_files'][$list], $output, LOCK_EX) === FALSE) {
    +    if (file_put_contents($list_filename, $output, LOCK_EX) === FALSE) {
           // Error adding entry to the list file
           watchdog(
    -        'Honeytrap',
    +        'honeytrap',
             t("Error recreating the file '%filename'."),
    -        array('%filename' => $honeytrap_settings['list_files'][$list]),
    +        array('%filename' => $list_filename),
             WATCHDOG_ERROR
           );
         }
    
  • Spelling correction:
    @@ -447,7 +447,7 @@ function _honeytrap_list_file_update($old_ip, $old_list, $new_ip, $new_list) {
      * @param $list
      *   The list that the IP address was added to.
      * @param $agent
    - *   Optional string spefifying the user agent.
    + *   Optional string specifying the user agent.
      *
      */
     function _honeytrap_log($ip, $list, $agent = '') {
    
  • More lowercasing:
    @@ -456,7 +456,7 @@ function _honeytrap_log($ip, $list, $agent = '') {
         array('%ip' => $ip, '%list' => $list, '%agent' => $agent)
       );
    
    -  watchdog('Honeytrap', $message, array(), WATCHDOG_NOTICE);
    +  watchdog('honeytrap', $message, array(), WATCHDOG_NOTICE);
     }
    
     /**
    
  • Comments should end with a period:
    @@ -470,18 +470,18 @@ function _honeytrap_log($ip, $list, $agent = '') {
     function honeytrap_footer() {
       $trap = honeytrap_get_trap_array();
    
    -  // Construct an automated link name from the disallowed path name
    +  // Construct an automated link name from a randomly-selected disallowed path name.
       $link_text = str_replace('/', ' ', trim($trap['disallowed_path'], '/'));
       $link_text = str_replace('-', ' ', $link_text);
    
  • I suggest adding "display:none;" as well as "visibility:hidden;" to avoid trapping screen-readers:
       $link_text = str_replace('_', ' ', $link_text);
    
    -  return l($link_text, trim($trap['url'], '/ '), array('attributes' => array('style' => 'visibility:hidden;')));
    +  return l($link_text, trim($trap['url'], '/ '), array('attributes' => array('style' => 'display:none;visibility:hidden;')));
     }
    
    
  • Comments should end in a period, and corrects a capitalization inconsistency:
     /**
      * Implementation of hook_views_data().
      *
    - * Adds the Honeytrap list view type to allow a view of the honeytrab table(s)
    + * Adds the Honeytrap list view type to allow a view of the Honeytrap table(s).
      * to be created.
      *
      * @see honeytrap_handler_filter_many_to_one()
    
  • The 'notafield' option is a D5/Views1 artifact. It appears to be unsupported/unneeded in Views 2:
    @@ -621,7 +621,6 @@ function honeytrap_views_data() {
         'field' => array(
           'handler' => 'honeytrap_handler_field_markup',
           'click sortable' => FALSE,
    -      'notafield' => TRUE,
         ),
       );
    
    
  • Spelling correction:
    @@ -662,7 +661,7 @@ function honeytrap_views_handlers() {
     function honeytrap_robotstxt() {
       $ret = array();
    
    -  // Construct the array of disallowed paths for the RobotdTxt module
    +  // Construct the array of disallowed paths for the RobotsTxt module
       $honeytrap_settings = variable_get('honeytrap_settings', array());
       foreach ($honeytrap_settings['url']['disallowed_paths'] as $trap_url) {
         // For the "no clean url" disallow entry ensure that the path has a
    
  • Correct inconsistency in capitalization:
    diff --git a/views/honeytrap.views_default.inc b/views/honeytrap.views_default.inc
    index 5d026a4..7e20f34 100755
    --- a/views/honeytrap.views_default.inc
    +++ b/views/honeytrap.views_default.inc
    @@ -5,7 +5,7 @@
      */
    
     /**
    - * The default view definition for the honeytrap lists.
    + * The default view definition for the Honeytrap lists.
      *
      * @see honeytrap_handler_filter_many_to_one()
      * @see honeytrap_views_api()
    
  • Use the l() function so that your links work when Drupal is in a subdirectory or when clean urls are turned off:
    @@ -237,7 +237,7 @@ function honeytrap_views_default_views() {
           'label' => 'Options',
           'alter' => array(
             'alter_text' => 1,
    -        'text' => '<span class="honeytrap-options"><a href="/admin/settings/honeytrap/edit/[ip]">edit</a> <a href="/admin/settings/honeytrap/delete/[ip]">delete</a></span>',
    +        'text' => '<span class="honeytrap-options">' . l(t('edit'), 'admin/settings/honeytrap/edit/[ip]') . ' ' . l(t('delete'), 'admin/settings/honeytrap/delete/[ip]') . '</span>',
             'make_link' => 0,
             'path' => '',
             'link_class' => '',
    
  • I got an error every time I added an IP via admin/settings/honeytrap/list-add.
    Other modules that had 'operator' => 'or', also had 'exposed' => FALSE,
    I'm not a views expert, but this change makes the error go away:
    @@ -274,7 +274,7 @@ function honeytrap_views_default_views() {
             'naughty' => 'naughty',
           ),
           'group' => '0',
    -      'exposed' => TRUE,
    +      'exposed' => FALSE,
           'expose' => array(
             'use_operator' => 0,
             'operator' => 'list_op',
    
pillarsdotnet’s picture

Please note that most of my suggested changes are just that; suggestions.

The problems that are blocking RTBC are:

  • Coder still complains a bit.
  • Settings validation should happen in the form validation function.
  • The defaults added by honeytrap.install are not valid.
  • The comments suggest adding /admin as a disallowed path which is also not valid.
  • Adding an IP address results in a PHP warning.

Fix those bits, and once your module is approved I'll move my other suggestions to a patch in your module issue queue.

pillarsdotnet’s picture

Status: Needs review » Needs work
FileSize
21.52 KB

Bah; some of my suggestions didn't make it to the attachment. Re-attaching.

Mikey Bunny’s picture

Hi pillars, thanks again for you exhaustive feedback. I have applied the changes and will commit them later on today once I have an environment to test the code and ensure no additional issues have crept in.

Unfortunately I am on a windows machine for the rest of today with a CentOS VM running by web services. My VM killed itself this morning due to lack of disk space on the host drive and I'm struggling to rescue it at present. It is insisting on more disk space for the repair but as the VM automatically gobbled up all the disk space in the first place so it's the chicken and egg scenario. I may just end up throwing in the towel and creating a new VM but that will take a few hours - which I have already wasted trying to recover it.

Oh, one comment on your comment about my comment:

- * Adds the Honeytrap list view type to allow a view of the honeytrab table(s)
+ * Adds the Honeytrap list view type to allow a view of the Honeytrap table(s).
* to be created.

The above sentence already ends with a period on the second line "...a view of the Honeytrap table(s) to be created.". It's probably a little confusing as it spans 2 lines to stay within the width restriction for comments.

You definitely deserve a mention if this module ever makes it live.

Thanks again

MB

pillarsdotnet’s picture

The above sentence already ends with a period on the second line

Sorry; that's why it's always good to have a second pair of eyes look at changes before committing them.

You definitely deserve a mention if this module ever makes it live.

I'm just looking forward to porting it to D7 so I can actually use it.

Mikey Bunny’s picture

The D7 help is great news. The latest D7s won't install for me at present - there's an outstanding issue about it somewhere.

If your going to find this module useful I think that you may love my next one. I know that is it something driving sys admins and developers on large scale Drupal projects mad everywhere that I have worked at so far.

MB

Mikey Bunny’s picture

Status: Needs work » Needs review

Apologies for the extended delay, my machine trashed itself when it ran out of disk space and then I got very ill for a few weeks. With the exception of the update below I have made the suggested changes and updates with a few minor modification.

@@ -237,7 +237,7 @@ function honeytrap_views_default_views() {
'label' => 'Options',
'alter' => array(
'alter_text' => 1,
- 'text' => 'edit delete',
+ 'text' => '' . l(t('edit'), 'admin/settings/honeytrap/edit/[ip]') . ' ' . l(t('delete'), 'admin/settings/honeytrap/delete/[ip]') . '',
'make_link' => 0,
'path' => '',
'link_class' => '',

This code is generated by a views export. If it is modified to use the l() function then the view's "[ip]" token replacement will not work as the l() function will replace the square brackets with their html encoded entities.

pillarsdotnet’s picture

This code is generated by a views export. If it is modified to use the l() function then the view's "[ip]" token replacement will not work as the l() function will replace the square brackets with their html encoded entities.

File a bug against views; the export is wrong. At the very least, you should prefix the path with base_path(). If you don't, and someone has Drupal installed in a subdirectory of the web root, all your links will be broken.

diff --git a/views/honeytrap.views_default.inc b/views/honeytrap.views_default.inc
index 5d026a4..1a2be84 100755
--- a/views/honeytrap.views_default.inc
+++ b/views/honeytrap.views_default.inc
@@ -237,7 +237,7 @@ function honeytrap_views_default_views() {
       'label' => 'Options',
       'alter' => array(
         'alter_text' => 1,
-        'text' => '<span class="honeytrap-options"><a href="/admin/settings/honeytrap/edit/[ip]">edit</a> <a href="/admin/settings/honeytrap/delete/[ip]">delete</a></span>',
+        'text' => '<span class="honeytrap-options"><a href="' . base_path() . 'admin/settings/honeytrap/edit/[ip]">edit</a> <a href="' . base_path() . 'admin/settings/honeytrap/delete/[ip]">delete</a></span>',
         'make_link' => 0,
         'path' => '',
         'link_class' => '',

I'll test the rest soon.

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

According to the guidelines on Process redesign, we probably should have approved this long ago.

greggles’s picture

Status: Reviewed & tested by the community » Fixed

The installation instructions could use some more text about going to admin > setttings > honeytrap.

Module looks great to me as well. I was concerned about XSS via the browser user agent or IP or comment, but your views handlers are filtering the data. Well done!

Thanks for your contribution, Mikey Bunny! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

Mikey Bunny’s picture

Thanks greggles that's excellent news and thank you everyone that helped, especially you pillarsdotnet. I will cut an alpha release just as soon as I work out how to do it.

Mikey Bunny’s picture

Issue summary: View changes

Corrected description formatting issues.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated download location with full project url.