Blajax

Coder with the settings as Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards found a few errors.

blajax.module

Line 105: else statements should begin on a new line
} else {

Line 137: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see blajax_action()

Line 237: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see _blajax_form_validate_block()

Line 238: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see _blajax_form_save_block()

Line 316: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$block_extra = _blajax_block_data($_POST['module'], $_POST['delta']);

Line 323: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$_POST['js']

Line 331: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$user->blajax_token[$_POST['module']][$_POST['delta']] >= time()-15

Line 349: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$_POST['module'],

Line 350: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$_POST['delta'],

Line 351: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$_POST['bid'],

Line 352: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
$_POST['scope']

Line 400: else statements should begin on a new line
} elseif ($blockExtra->title) {

Line 492: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see http://drupal.org/project/js

Line 579: else statements should begin on a new line
} else {

Line 598: else statements should begin on a new line
} else {

Line 923: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see block.module

Line 924: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see block_list()

Line 949: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see _blajax_form_validate_block()

Line 962: else statements should begin on a new line
} else {

Line 972: @see references should be separated by "," followed by a single space and with no trailing punctuation
* @see _blajax_form_save_block()

Line 985: else statements should begin on a new line
} elseif (

blajax.admin.inc

Line 164: in most cases, replace the string function with the drupal_ equivalent string functions
array('!linktype' => t(ucfirst($type)))

Line 170: in most cases, replace the string function with the drupal_ equivalent string functions
array("%linktype" => t(ucfirst($type)))

Line 177: in most cases, replace the string function with the drupal_ equivalent string functions
t(ucfirst($type))

With regards to your comments;

Gracefully don't mind the 4 instead of 2 spaces indent; everything else has been checked with coder and already fixed.*

*Edit: else/elseif style as well, I would prefer to keep it as is and assumed this not being a hard issue since I have seen it in various other "real" projects as well!?

I really dont think its down to you, everyone should adhere to the coding standards and unless you follow those rules then you may really struggle to get this module through to its final release.

I am also using Coder 6.x-2.0-rc1, did you have the settings on 'minor (most)'?

I understand what you mean with regards to the $_POST data never actually being manipulated by an external source but I am sure you may struggle to get your module to a full release unless you patch those vulnerabilities.

I had a mess with your code locally and it is easily patched, but as you said it may be an extra overhead, how much of an overhead I am not really sure.

With regards to Line 316 where you have
$block_extra = _blajax_block_data($_POST['module'], $_POST['data']);
When this is changed to
$block_extra = _blajax_block_data(filter_xss($_POST['module']), filter_xss($_POST['data']));
Then Coder no longer flags that line as a CSRF vulnerability.

Similarily, if you were to use
$block_extra = _blajax_block_data(check_plain($_POST['module']), check_plain($_POST['data']));
That also satisfies the Coder modules checking algorithm.

It may be worth doing some research into whether check_plain() or filter_xss() actually has a noticeable effect on the processing overhead and possibly use the 'lighter' solution.

Maybe someone that is more knowledgable than me on this subject will chime in and answer your question a little more eloquently :)

Since there remain some outstanding items, changing status.

Have you traced all your $_POST variables all the way through? Do you know that at least one of them can eventually become part of the argument to an include_once() call, unsanitized?

I'm not saying that will happen, without either a mistake on your part or malicious intent on a site-administrator's part. But it could. I hope you can understand why I thought you might still be working on those items. I mean, I would.

I saw "I will test both of your proposals meanwhile" and I took that to mean you were working on it. I'm sorry.

It's also not at all clear that the seven bullet-points you listed are things which you don't intend to work on any further. I saw "here's what remains after having fixed the rest". Again, I'm sorry.

argument to an include_once() call, unsanitized?

Is that given?

No. It would require either a function-naming mistake on your part, in which case the module wouldn't actually even work, or, malice on some site administrator's part - and those folks already have such power as to not need to exploit something this obscure.

they are all related to the same issue (and will never ever be displayed to a user

Being displayed to a user (or injected into a SQL query) aren't the only things to worry about, only the most obvious.

With that in mind, look closely at where module_invoke($module, 'block', 'view', $delta) sends $module and $delta (unsanitized from $_POST). If the proper function name is not found, module_hook() will search for include files based on the arbitrary string. It's true that if the function name isn't found, the module won't work, but this illustrates the risk, I think.

I haven't made the time to look at what happens with theme('block', $block); but, your $block object presents unsanitized $_POST stuff to theme(). That looks pretty scary, although I don't think the current implementation of theme() will actually do anything with your $block->module or $block->delta properties. Will that always be true?

OK. Neither of those points appear to me to be actual risks in your module, currently. They just illustrate the attention to detail necessary, if you're not going to sanitize. And why I thought you were still working on it.

Here you go

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/blajax.module:
   +234: [minor] Comment should be read "Implements hook_foo()."
   +473: [minor] Comment should be read "Implements hook_foo()."
   +503: [minor] Comment should be read "Implements hook_foo()."
   +528: [minor] Comment should be read "Implements hook_foo()."
   +553: [minor] Comment should be read "Implements hook_foo()."
   +583: [minor] Comment should be read "Implements hook_foo()."
   +775: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 7 minor warnings, 0 warnings were flagged to be ignored
  

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove the translations folder, translations are done on http://localize.drupal.org
• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./blajax.install:14:} // end function blajax_install
  ./blajax.install:66:} // end function blajax_schema
  ./blajax.install:97:} // end function blajax_uninstall
  ./blajax.module:56:      break; // hide
  ./blajax.module:74:      break; // minimize
  ./blajax.module:91:      break; // maximize
  ./blajax.module:92:    } // switch ($action)
  ./blajax.module:116:} // End function blajax_action
  ./blajax.module:154:    break; // hide
  ./blajax.module:165:    break; // edit
  ./blajax.module:177:    break; // minimize
  ./blajax.module:189:    break; // maximize
  ./blajax.module:201:    break; // refresh
  ./blajax.module:202:  } // switch $type
  ./blajax.module:229:} // End function blajax_create_link
  ./blajax.module:302:} // End function blajax_form_block_admin_configure_alter
  ./blajax.module:366:} // End function blajax_get_block
  ./blajax.module:421:        break; // all
  ./blajax.module:426:        break; // subject
  ./blajax.module:430:        break; //default
  ./blajax.module:434:} // End function blajax_get_block_data
  ./blajax.module:468:} // End function blajax_get_caption
  ./blajax.module:523:} // End function blajax_init
  ./blajax.module:548:} // End function blajax_js
  ./blajax.module:578:} // End function blajax_menu
  ./blajax.module:590:} // End function blajax_perm
  ./blajax.module:770:} // End function blajax_preprocess_block.
  ./blajax.module:806:} // End function blajax_theme
  ./blajax.module:861:} // End function theme_blajax_link
  ./blajax.module:896:} // End function theme_blajax_toolbar
  ./blajax.module:922:} // End function theme_blajax_captionbar
  ./blajax.module:979:} // End function _blajax_check_path
  ./blajax.module:1003:} // End function _blajax_form_save_block
  ./blajax.module:1039:} // End function _blajax_form_validate_block
  ./js/blajax.js:152:					case "all": // entire block
  ./blajax.admin.inc:199:} // end function blajax_admin_settings_form
  

• ./blajax.install: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function blajax_update_6000() {
  

• ./blajax.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function _blajax_block_data($module, $delta) {
  --
  
  function _blajax_check_path($configOption) {
  

• ./blajax.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  19- *  Which action to perform. Possible options are:
  26- *  The module name as used in any block related function
  29- *  The block delta as used in any block related function
  32- *  The block id as used in any block related function
  127- *  Equivalent to the related action of the link.
  130- *  The module name as used in any block related function
  133- *  The block delta as used in any block related function
  136- *  The block id as used in any block related function
  374- *  The module name as used in any block related function
  377- *  The block delta as used in any block related function
  380- *  The block id as used in any block related function
  383- *  Which data to return (default: 'all')
  386- *  The new block content or parts of it
  442- *  The module name as used in any block related function
  445- *  The block delta as used in any block related function
  448- *  The block id as used in any block related function
  451- *  Fallback value if no different caption has been set
  454- *  The effective caption
  812- *
  814- *
  816- *
  818- *
  820- *
  822- *
  824- *
  826- *
  828- *
  830- *
  832- *
  867- *  The pre-themed link elements
  870- *  The pre-themed caption bar
  873- *
  875- *
  877- *  The module name as used in any block related function
  880- *  The block delta as used in any block related function
  883- *  The block id as used in any block related function
  902- *
  904- *
  906- *
  908- *  The module name as used in any block related function
  911- *  The block delta as used in any block related function
  914- *  The block id as used in any block related function
  930- *  The module name as used in any block related function
  933- *  The block delta as used in any block related function
  955- *  For which option we are to check the path
  958- *  Whether the requested option is active in this path.
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./blajax.module:836:  $type, $path, $query, $class, $id, $label, $tooltip, $module, $delta, $bid, $use_icons=FALSE
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Why do some coder executions - with exactly the same settings! - obviously produce different results as others?

I used to experience the same thing. When I moved from Coder X.x-X.x to Coder X.x-X.x-dev versions, I found that there are differences. I believe that most reviewers are using the -dev versions of Coder.

[minor] Comment should be read "Implements hook_foo()."

This particular one was a code style change between Drupal 6 and Drupal 7. It may not appear if using the Drupal 6 version of coder, but will be flagged by the Drupal 7 version (which is being leveraged by some reviewer's automated review scripts).

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

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./js/blajax.js:152:                                     case "all": // entire block
  

• ./blajax.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  823- *
  925- */
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• remove the 6.x-0.x branch, I think you don't need it anymore? Also clean out the master branch, as described here in step 5: http://drupal.org/node/1127732
• blajax.js: indentation errors, use the same 2 space style as in PHP files
• ""SELECT caption FROM {blajax} WHERE module='%s' AND delta='%s'",": there should be a space before and after the "="
• "$success = $action == "refresh" || is_object($test=user_save($user, $user_ext));": same here.
• "$items["js/blajax/action/%/%/%/%"] = array(": I think you should add a permission who is allowed to use that path.
• "t('You should be aware that refreshing every !value ...": why are you using the "!" placeholder here? The value should be a integer anyway, so use "@".

Regarding your 'EDIT' comment in #17 ... unless an anonymous user is expected to be allowed to use the 'auto-refresh/refresh blocks with AJAX' permisisons, then why not a permission callback of 'user_is_logged_in()'; which could help simplify the code?

Cool module -- I'm only seeing the Refresh and Edit options while authenticated as an admin, though; should I be seeing Hide, Minimize, Maximize as well? Am I missing a config option somewhere?

Got it, thanks -- that last pass was just general UAT. Just did a quick review of the actual code:

• Missing a t() on '#description' on 'Blajax additions' field in blajax_form_block_admin_configure_alter
• PAReview is still reporting "All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting" -- I got around this by adding 2 lines to the end of files; I suspect this is more of a Git trimming issue than a problem with PAReview, though. Otherwise, the mod came up clean.
• [super picky] Personally, I would lose the "// End function" comments and "//-------" visual delimiters...any decent IDE will apply code collapse to delineate functions.

Aside from those minor tweaks, this looks good to me...solid code base, works as described and is generally super-useful on 6.x sites.

Have you checked out the latest version of PAReview.sh as it has a bug fix related to the newline detection.

Looks good to me...I'm putting the RTBC stamp on it; I see the newlines in browser source (though still not in bash), but don't consider the issue a stopper; more experienced reviewers are welcome to chime in, of course.

Some things to consider going forward -- it will be interesting to see how/if this mod plays with D7 contextual links. I also found this backport of the same: http://drupal.org/project/contextual ...I see that your mod's scope is a bit more focused, but it may be worth considering a merge at some point.

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.
• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./css/blajax.css ./blajax.install ./README.txt ./blajax.info ./blajax.module ./js/blajax.js ./blajax.admin.inc
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• SOAP? Where exactly does the SOAP support in your module live?
• $label = theme('blajax_link_label', check_plain($label), ...: do not use check_plain() here, move that to your theme function. Sanitization should always happen as closely as possible to the actual output.
• ""SELECT * FROM {blajax} WHERE module='%s' AND delta='%s'",": there should be a space before and after "="
• "$postdata['module'] = preg_replace(": why do you sanitize the $_POST data here? You are not outputting it anywhere to the user and you use the DB API properly which will escape anything dangerous for you. Maybe I miss something, but you should move the sanitization to the places where it actually could be dangerous.
• blajax_get_caption(): same here, do not check_plain() so early.
• I'm very happy with your documentation, good work on the function doc blocks!
• blajax_get_block(): you have a very loooooooong if statement in there, which is very hard to grasp. Try to split it up as described here: http://drupal.org/node/318#linelength
• You are using your own style of breaking lines (you do it early sometimes). I don't think it violates our coding standards, but it looks quite unusual to me and might be harder to read for some people. However, I don't insist that you change it, just wanted to let you know.

Formatting

RE: new lines
Sorry to say it Dave but Klausi's script is spot on, there are no new lines at the end of the files. What it does have is new lines at the end of the previous line but no newline on the very bottom line.

I know you are using TextPad for coding, with the files open, hit 'CTRL+Q' (the cursor should change to a finger pressing a key) then click 'I'. It will show you the indentations and line breaks as dots and small symbols, newlines are like a backwards 'b'. You should have one of these on the very last line to pass the new line test ran by PAReview.sh.

When I first downloaded your module it said...

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./blajax.admin.inc ./blajax.info ./blajax.install ./blajax.module ./css/blajax.css ./js/blajax.js ./README.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Then once I inserted the newline characters it reported back...

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

I was going to zip them up and send them to you but I will leave that as an exercise for you :)

note* and all this on a Windows box.

changed just so you notice :)

• There are many examples where sanitization is done in theme functions, see theme_image() for example. However, you removed the check_plain() from the return statement of an API function, so I'm satisfied.
• blajax_create_link(): for the l() at the end: no need to check_plain($tooltip) here, this will be done by drupal_attributes() in l() for you (and double escaping is bad).
• Regarding the post data: you want to save CPU cycles on the long if statement, but you keep the preg_place() for every request (which is much more expensive)? You argument is not consistent here :-P _blajax_block_data() is safe for any data provided. blajax_get_block_data() looks more vulnerable: $delta is never sanitized and you pass it as is to the theme function. I'm not sure how an attacker could exploit that (would have to do a POST request on your behalf with script code in it that is then passed through Drupal and rendered in you browser to get the XSS effect). Anyway, "case "all"" in blajax_get_block_data() looks like the perfect place to do sanitization.
• ""SELECT custom,title FROM {blocks} WHERE bid=%d",": there should be a space before and after "=". And after ",".

• I think you changed the wrong line in blajax_create_link()? I was refering to check_plain($tooltip) on line 250. $label on line 219 must be sanitized as it is user provided data (or you do it in your theme function).
• blajax_get_block_data(): $bid and $delta must be numbers, right? So I suggest to just cast them to integers instead of using check_plain().

pareview.sh and drupalcs are under heavy development right now, I suggest to git pull them every time before you start working. I'm not aware of any git notification system that tells you about new commits (maybe there are some RSS feeds somewhere).

drupalcs complains about your artificial function separators (// -----------), see attachment. You could use /* ... */ style comments, if you don't want drupalcs to complain, as they are not checked.

Yes, hook_form_FORM_ID_alter() is a false positive :-(

Otherwise I think this is ready.

and a big doitDave++ for looking into so many other project application issues. Thanks!

Forgot attachment.

Discussion group: http://groups.drupal.org/code-review

All your files end with a blank line. This is wrong. Use Drupal core as an example and you will see that no core files have trailing blank lines.

Huh? What are you talking about?

Some interpret 'end with a single newline' as there should be no more than one at the end of the file, and others interpret it as 'the end of a file (last line) should contain a single newline'. If we're talking core patches, then strictly the former is the correct interpretation - but I know that I personally always assumed the second.

Let's try not to hold up applications for basic coding standards violations ... this will get sorted out through automation as we turn it up (since a bot will only enforce one interpretation or the other) ... but for now, I don't really think it's worth holding up applications for this particular coding standards debate, which doesn't really affect readability of the code in any way.

That said, I'd still block any files which don't have 'at least one' newline (due to the 'no newline at end of file' error that future patches would trigger), and no file should ever have 'more than two'.

Setting back to RTBC as per 41, and considering my comments above.

Thanks for your contribution, David! 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. You have been a great help in the review process by reviewing other projects, and I hope you continue that involvement. I have found this part of the d.o community to be one of the best places for picking up on a huge amount of knowledge about the different ways to get things done and continue to learn things every day.

Please also consider the following:

Remove block as dependency

There is no need to include core required modules as there is no way to not have them installed.

Consistency of quotes

You have single and double quotes mixed without any real pattern. Even within the same concatenated string. There's no guideline on which you should be using, but using single quotes as much as possible is faster for PHP to handle. There are two exceptions - when variable expansion is desired, and to avoid escaping apostrophes.

git commit messages

Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages.