anupom.gogoi [anupomgogoi]

1.     return '<p>'. t('Here you can create a new InnerFade block. Once you have created this block you can configure and enable it on the <a href="@overview">blocks</a> page.', array('@overview' => url('admin/build/block'))) .'</p>';
   

   The link really should be embedded thus:-

       return '<p>'. t('Here you can create a new InnerFade block. Once you have created this block you can configure and enable it on the !overview page.', array('!overview' => l(t('overview'), 'admin/build/block'))) .'</p>';
   

2.         $blocks[$row->infid]['info'] = $row->title;
   

   Can you confirm that $row->title should not be run through check_plain()? In fact, there are other places where the "title" field from the database are not escaped.

3. Please use drupal_substr() in place of PHP's native substr() function.

4.         if (!isset($block->subject)) {
             $block->subject = FALSE;
           }
           else {
             $block->subject = $inf_block['title'];
           }
           $output .= theme('block', $block);
   

   The theme template doesn't provide any output filtration, as a module writer that's your responsibility and I don't see any in your module.

If the output filtration isn't an issue and your confident there's no XSS problems them some comments in the code would be a good idea so reviewers have a better understanding of your intent.

Alas, I installed your module and then entered :-

<script>alert('XSS!')</script>

as block titles and bingo, it ran.

If you create a new block and use the above as the title you'll find it's escaped.

So I'm not convinced at all about your assurance that filtering is not needed. I would suggest spending some time reading http://drupal.org/writing-secure-code.

As a reviewer it always concerns me that after pointing out the documentation regarding security issues someone can digest it all in 38minutes.

For example, in http://drupal.org/node/149117#comment-1605360 you show the following example:-

  $sql = db_query("Select * From {node} Where type = '%s'", 'page');
  while($o = db_fetch_array($sql)) {
    $options[] = $o['title'];
  }  
  $form["drop"] = array(
    '#type' => 'select',
    '#title' => t('My Select Box'),
    '#options' => $options,
  );

With your new found knowledge could you tell me what's wrong with that advice?

Every input coming from the user must be checked, every times it is used in HTML output, or SQL queries.

OK, here's my problem.

1. The addition of check_plain() to the title isn't needed here as it's passed as options to FormAPI. It's FormAPI responsiblity to handle that side of things for you, that's what it's there for.

2. From my point of view, there is not any wrong on the above code.

   This is the biggest issue. You have failed to spot the two related security issues with:-

       $sql = db_query("Select * From {node} Where type = '%s'", 'page');
   

   And for those wondering what's up, here's what it ought to look like:-

       $sql = db_query(db_rewrite_sql("SELECT title FROM {node} WHERE status = 1 AND type = 'page'"));
   

   This corrects the two access by-pass violations, the first being the "status = 1", you shouldn't list unpublished nodes as that would leak the title and secondly the use of db_rewrite_sql() to ensure the selection of nodes follows Drupal's access system. Also, since "page" is a fixed string and not a variable there's little point using the %s placeholder here.

   There are other API related issues also. Such as what if another module dynamically altered the node title in some way? If you select it directly from the database table {node} you've bypassed that entirely. That's why we have an API and node_load().

   Additional information can be found on my blog page: http://stellartech.co.uk/node/9

For a CVS account to be approved it's pretty much mandatory to have a good understanding of writing secure code and Drupal's API. I suggest you take some time out to find out more about this and re-apply at a future date.