Cackle

Git repo: git clone --recursive --branch 7.x-1.x cackle@git.drupal.org:sandbox/cackle/1822808.git cackle

I'm not able to clone your project with these command, maybe you must remove "crackle"(username) from that command.

I just checked your project with PAReview service. Seems like there is no issues. I will try to check it manually.

Hey,

I found no problems on automatic review using ventral.

On my manual review:
function cackle_closewindow in cackle.admin.inc does not seem to be ever called.

In cackle.module:42 instead of reading arg(1) and do a node_load, you can access the loaded node using menu_get_object()

Function cackle_output_footer_comment_js is weird, I would recommend to just output a nomal PHP string, instead of doing what you are currently doing.

Function cackle_node_view you probably just need to add the js if you are showing the node in the teaser probably.

Function cackle_admin_settings() the description of the form fields are wrong and repeated.

In cackle.php:
Line 21 is never used:
21 $sql = "select common_value from common where `common_name` = 'last_time'";

In this file there are several functions that don't use correctly the DB Api, no correct definition of tables, and not using drupal_write_record to write in the comments table.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I recommend you include installation/configuration instructions in the module itself, not just in the project page.

The project page has a typo: "Cofiguration".

Don't use db_query('ALTER TABLE .... Use db_add_field(), instead.

Don't use db_query('SELECT'... Use db_select() instead. See the tutorial on Dynamic queries.

Any time you add something to an existing setup, I'd recommend pre-pending its name with 'cackle_' to avoid conflicts and make it clear what it's for. So, in this case, call the new field 'cackle_user_agent'.

cackleComment() uses a lot of raw HTML and PHP tags to do its thing. It looks like it's for direct output generation. Use Drupal's theme mechanism, instead.

Many of your functions have no comments describing their parameters, for example dbConnect($sql, $insert=FALSE, $table='comment', $array=array()). See http://drupal.org/coding-standards/docs.

Add your class definition/interface files using a 'files[]' line in your .info file. See http://drupal.org/node/542202.

The '#title' field of a form element shows up on the form. So, for your admin form, I'd recommend something more descriptive than 'mc_site', 'site_api_key', and 'account_api_key'. Use normal English. Also, the description for all three is 'The maximum number of links to display in the block,' which appears to be incorrect.

It's good to have the configuration directions in the help. I'd recommend turning 'cackle.me' into a live link.

You should use a more specific administration permission than 'access administration pages'. 'Administer comments and comment settings' is one obvious choice, or add your own permission.

changed git link

Hi Cackle,

Manual Review :
Try to use template.php for the working inside the function cackleComment() line 183 cackle.php. Also try not to include javascript code inside your php code, instead make a seperate .js file and add that javascript using drupal_add_js().

Your module also seems to be cross site scripting vulnerable. Your module will output the results from database query without sanitization. Results from db_query at line 233 cackle.php are displayed inside the cackleComment() line 183 cackle.php. You should use check_plain($comment->homepage), check_plain($comment->name) line 187 cackle.php and all other similar places.

Thanks
Udaksh

Hi,cackle

Manual Review:

cackle.module

• There should be no trailing spaces after mc.type = 'text/javascript';
• Same as above after (document.getElementsByTagName('head')[0] ||

document.getElementsByTagName('body')[0]).appendChild(mc);
cackle.php

• There should be no trailing spaces after PHP tag
• Same as above after "$this->listComments($nodeid)"

Thanks,
Mayank

Hey cackle,

I was saying about the Cross Site Scripting Vulnerability not SQL Injection. You are correct your module is secure from SQL Injection but it is vulnerable to Cross Site Scripting. For more info on XSS go to https://www.owasp.org/index.php/Cross-site_Scripting_(XSS).

Consider function cackle_block_view() at line 37 in file cackle.module. At line 50 it is calling another function cacke_in(), In this function you are making an object class cackle and calling a member function cackleDisplayComments(). Inside function cackleDisplayComments() at line 211 you are calling another member function listComments(). In this function you are calling getLocalComments() and get the results from database and then you are displaying these results inside function cackleComment(). So the value from the database does not sanitized and if this value contains some XSS like <script>alert("XSS")</script> then it will result into Cross Site Scripting.

I have created a new entry in comment table with name as <script>alert("XSS")</script>. When i enable your module it is resulting into a pop-up. Have a look at the screenshot. You can secure from these vulnerability by using filters like check_plain() at the time of displaying your content.

Hope I make you clear.

Thanks,
Udaksh

Manual review

** all files

This is not specifically mentioned in the coding standards, but there must be a blank line between function and method declarations.

function func1() {
  /* ... */
}

function func2() {
  /* ... */
}

** cackle.module, line 28

There is no need to manually load cackle.php file. Use Drupal's class autoloading.

** cackle.php

The whole class, in my opinion, validates one of the Drupal's main ideas: presentation must be separated from functionality.

All HTML should refactored out of the class body into theming-hooks. Preferably, JavaScript snippets should be moved to separate files too.

** cackle.php, line 233

Directly querying field tables is not save because there is no guarantee that the comment_body field will exists. You must properly use API of the comment and field modules, otherwise your module may cause a crash.

** cackle.info

Your module has an implicit dependency on the comment module, but it is not declared in the .info file.

** cackle.php, line 141

Do not insert records into a table that belongs to another module. Use comment_save() instead.

** cackle.php, line 42

Your module has an implicit dependency on the curl PHP extension, but it is not mentioned in neither .info file nor in module's documentation. Curl is not required by Drupal, so it may be missing on some hosts. If your module is enabled on such host, it will produce a fatal error.

You must implement hook_requirements() and prevent installation of the module, in the curl extension is not enabled.

Take a look at the simpletest_requirements() function. It has this check.

** cackle.php, line 44

It is not an error per se, but I really do not like how your module declares itself as Firefox in a curl request. This is not a fair behavior, and it can be detected anyway. You should honestly use something like:

    $drupal_version = VERSION;
    $cackle_version = CACKLE_VERSION; // You will have to declare this constant;
    $php_version = phpversion();
    $useragent = "Drupal $drupal_version; cackle module $cackle_version; PHP $php_version";
    curl_setopt($ch, CURLOPT_USERAGENT, $useragent);

Update: In fact, most of my comments have already been mentioned by previous reviewers. Please fix them.

I liked the module in its simpleness. Quick to install and just as quick to get up and running.

Manual review:

• Consider using other names for the fields in the admin page of the module. On cackle.me they refer to the values you have to get as 'Site ID', 'Site API Key', and 'Account API Key', while you use 'mcSite', 'siteApiKey', and 'accountApiKey'. They should reflect each other.
• A minor tip about the variable $a in cacle.module => cackle_in(). It would be better named $cackle for code readability.
• The same goes for the function-name itself; cackle_in(). Are you refering to the word "init", if so consider naming it that; cackle_init() or cackle_initialize().
• If I'm not mistaken, the comment for the function cackle_node_view() should include "Implements hook_node_view().".
• In the custompage.tpl.php page, in the last two lines you set @debug to ob_get_clean() and then echo it out on the next line. Why not just echo out ob_get_clean() ?

Hi,

manual review:
#1: cackle.module:
put your Javascript code in a dedicated JS file and pass down settings from PHP with Drupal.settings, see http://drupal.org/node/756722

#2: cackle.module, line 76ff:
You don't set a subject for the block.
If I understand correctly you allow this module to work only with the 3 standard content types forum, article and page. If this is intended you should be adding a comment to explain this. But please understand that in bigger projects these 3 CT are only rarely used but extended by custom CT. Dont limit yourself.

#3: cackle.php:
Do you really need to transfer the various versions for Drupal, PHP and the module on every request?

#4: custompage.tpl.php: using ob_start should not be necessary with templates

@hhhc

#3: cackle.php:
Do you really need to transfer the various versions for Drupal, PHP and the module on every request?

It was my suggestion to include versions of everything in the UserAgent string. The reasons why I have advised to do so are:

1. If I implemented this module, this would be the kind of UserAgent string that I would send;
2. Take a look at a typical browser's UserAgent string. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.89 Safari/532.5 — it clearly specifies its versions.

Of course, the maintainer of this module is free to decide whether or not to send all this information. If you have some reasons to believe that this is a bad idea, perhaps, the UserAgent string can be made less verbose.

Please note: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/cackle.php
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   62 | ERROR | Incorrect spacing between argument "$cackle_last_comment" and
      |       | equals sign; expected 1 but found 0
   62 | ERROR | Incorrect spacing between default value and equals sign for
      |       | argument "$cackle_last_comment"; expected 1 but found 0
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:

1. cackle.info: dependency to the comment module is missing.
2. cackle_install(): use db_add_field() instead.
3. cackle_uninstall(): use db_drop_field() instead.
4. cackle_help(): Heading should also run through t() for translation.
5. cackle_in(): why do you check for cURL here? Please add a comment.
6. cackle_theme(): Do not use the "$" for variable names here.
7. "'template' => 'custompage',": the template file should use the proper module name prefix for clarity.
8. cackle_block_view(): node types are hard coded; better use some sort of configuration for which node types cackle should be enabled?
9. cackle_block_view(): why this complicated $node_type computation? Just use $node->type?
10. cackle_block_view(): doc block is wrong, you are not initializing the module here, you are preparing the block contents.
11. "'access arguments' => array('access administration pages'),": the administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
12. custompage.tpl.php: why do you have to create a script element? Just use drupal_add_js('http://example.com/example.js', 'external'); ? See http://drupal.org/node/756722 . Same for cackle_output_footer_comment_js() used in cackle_node_view().
13. custompage.tpl.php: what is $debug for? and why do you need ob_start()?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Reviews of other projects:

You have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

manual review:

1. cackle.info: you are adding your JS to every single page request. Shouldn't it only be added if there actually comments on the page?
2. "echo check_plain($comment->homepage)": use check_url() instead.
3. "'access arguments' => array('access administration pages'),": the administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
4. custompage.tpl.php: did you forget to remove that file from the repository and is cackle_template.tpl.php the replacement?

The permission problem is a minor security blocker, otherwise looks nearly ready to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

And what is the problem?

The problem is that the permission system has much more depth than that. The other problem is that English is a confusing language.

"access administration pages" means that a user can see at least one administration page, probably with the administration theme overlay. Look at admin/people/permissions. There are many, many possible permissions.

Let's imagine that you put an internal financial report on an administration page. It's not for every visitor to your site. It's just for your accountant. So, you give your accountant access to the administration pages and read access on the report. However, that's all you give him permission for. You don't give him permission to write a blog. You don't give him permission to change the settings of trigger events in your Rules module. You don't give him permission to change the picture on your home page. You don't give him permission to change your Cackle settings. You don't give him permission to delete other people's Cackle posts. You don't want to give him permission for anything but to view that report. When he goes to the administration menu on the administration page, you want him to see only one link to click on there: The financial report.

If you make the Cackle permission "access administration pages", then the accountant who can see your financial report will also be able to administer Cackle. That is the problem. Do not use that permission.

Since this is not dependent on the Comment module, I think it would be best to to create one or more new permissions like "administer Cackle settings" and "administer Cackle posts," then use those.

You're welcome.

Can I just change it for "administer site configuration" permission for only administrator access?

It would be better to add new permissions, I think. It's easy. See hook_permission(). You might want something like this:

/**
 * Implements hook_permission().
 */
function cackle_permission() {
  return array(
    'administer cackle settings' => array(
      'title' => t('Administer Cackle settings'), 
      'description' => t('Change the settings for the Cackle module.'),
      'restrict access' => TRUE,
    ),
    'administer cackle posts' => array(
      'title' => t('Administer Cackle posts'), 
      'description' => t('Delete and edit all Cackle posts.'),
      'restrict access' => TRUE,
    ),
    'administer own cackle posts' => array(
      'title' => t('Administer own Cackle posts'), 
      'description' => t('Delete and edit Cackle posts created by the same user.'),
    ),
  );
}

Put in this hook implementation, and you'll be able to use 'administer cackle settings', 'administer cackle posts', and 'administer own cackle posts' as permissions for your module. You'll be able to give users these permissions without being required to give them permissions to do anything else.

review links added.

Hi,

I have reviewed your cackle module. I just found minor issues.

Got error for the first time when I enable cackle module.

Fatal error: Call to undefined function curl_init() in C:\xampp\htdocs\projects\drupal714\sites\all\modules\cackle\cackle.php on line 43

Please be mentioned in your documentation saying that enable curl in your server side (php.ini etc).
---------
File : cackle.admin.inc
Line 14: It will be great if you use placeholders for link/string

Please follow example Placeholders

Line 23: Expected ',' for the last form attribute and ')' can move it to next line.
Line 30: Expected ',' for the last form attribute and ')' can move it to next line.
Line 37: Expected ',' for the last form attribute and ')' can move it to next line.
------------
File: cackle.install
Line 19: Expected one blank line.
------------
File: cackle.js
File: counter.js
You may use Drupal.behaviors (http://drupal.org/node/114774#javascript-behaviors)
------------
File: cackle.php
Line 59: Expected one blank line.
Line 70: Expected one blank line.

Other than that I never found any issues while doing manual review.

Regards,
D. Veera Prasad.
www.drup-all.com

Looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to tim.plunkett as he might have time to take a final look at this.

Now actually assigning.

Thanks for your contribution, cackle!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.