User-Agent Cache Bypass

Hello. I spent a Review of your module.
variable_get('ua_cache_bypass_crawler_names'... this is user provided text, I think you need to sanitize it to avoid XSS attacks.

switch ($path) {
case 'admin/help#ua_cache_bypass':
return filter_filter('process', 1, NULL, file_get_contents(dirname(__FILE__) . "/README.txt"));
}

I think IF operator is better for this.

Okay.
Sorry I missed string

I think IF operator is better for this.

in my previous comment.

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

• Drupal Code Sniffer has found some code style issues (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.

</code></p><code>

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/ua_cache_bypass.module<br>
--------------------------------------------------------------------------------<br>
FOUND 2 ERROR(S) AND 5 WARNING(S) AFFECTING 7 LINE(S)<br>
--------------------------------------------------------------------------------<br>
  9 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br>
    |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br>
 12 | WARNING | Line exceeds 80 characters; contains 87 characters<br>
 19 | ERROR   | Comments may not appear after statements.<br>
 53 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br>
    |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br>
 64 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br>
    |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br>
 72 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br>
    |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br>
 93 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
    |         | question marks<br>
--------------------------------------------------------------------------------<br>

Source: http://ventral.org/pareview - PAReview.sh online service

No manual review given, and the minor coding style issues do not justify the "needs work" status.

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

As your module represents new code it should go with the most recent version of coding standards where possible. From http://drupal.org/coding-standards :

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version.

In ua_cache_bypass.module:102
your code:watchdog('cache bypass', 'Page cache was bypassed via user-agent string: !ua', array('!ua' => $_SERVER['HTTP_USER_AGENT']), WATCHDOG_NOTICE);

Superglobal variables, like $_SERVER['HTTP_USER_AGENT'] can be spoofed and used for XSS attack. It needs to be filtered before being inserted into log by watchdog(). Therefore you should use %ua rather than !ua.

In hook_menu() you can remove the ua_cache_bypass_config_page() menu callback and use drupal_get_form().

Instead of

/**
 * Implements hook_menu().
 */
function ua_cache_bypass_menu() {
  $items = array();
  $items['admin/settings/ua_cache_bypass'] = array(
    'title' => 'User-Agent Cache Bypass',
    'description' => 'Configuration for Tableau Crawl Bypass.',
    'access callback' => 'user_access',
    'access arguments' => array('administer ua cache bypass'),
    'page callback' => 'ua_cache_bypass_config_page',
    'file' => 'ua_cache_bypass.admin.inc',
  );
  return $items;
}

You can just use 'drupal_get_form' as the 'page callback' and array('ua_cache_bypass_settings') as the 'page arguments'.

/**
 * Implements hook_menu().
 */
function ua_cache_bypass_menu() {
  $items = array();
  $items['admin/settings/ua_cache_bypass'] = array(
    ...
    'page callback' => 'drupal_get_form',
    'page arguments' => array('ua_cache_bypass_settings'),
    ...
  );
  return $items;
}

BTW, this is a very minor tweak. I have also tested that the code is working as expected using Firefox's User Agent Switcher.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. ua_cache_bypass_install(): no need to set variables here as you can use default values in variable_get() anyway.
2. ua_cache_bypass_boot(): Are you sure that you need to call "exit" directly and cannot use drupal_exit()?
3. ua_cache_bypass_log_bypass(): the return statement at the end is not needed, remove it.
4. ua_cache_bypass_perm(): hook_perm() does not exist in drupal 7 (not a security issues, it just means that no one but user 1 can access the admin page right now).

But that are just minor issues, otherwise this looks RTBC to me. Assigning to tim.plunkett as he might have time to finally approve this.

I granted access based on klausi's RTBC. I think that's all that needs to be done? If not, let me know.

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

Michelle has 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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.