[D7] Anonymous Suggestion Box

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

No issue reported.
http://pareview.sh/pareview/httpgitdrupalorgsandboxbenjaminarthurt231263...

Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a

lot, then post a link to the automated review and mention that problems should be addressed.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the

href="https://www.drupal.org/node/2181737">README Template

.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) anonymous_suggestion_box_admin: This is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> in the admin

   settings form, tested for anonymous_suggestion_box_form_intro element, every time If I visit this Suggestion form, get a

   nasty javascript popup. You need to sanitize this before rendering, make sure to read

   href="https://www.drupal.org/node/28984">https://www.drupal.org/node/28984

   again.

   In given code anonymous_suggestion_box_form_intro variable coming form user provided input and you are directly rendering without sanitize that's why these popup coming.

   

   
   Also check the same for other places too in your code.
   

2. (*) Email need to be validated irrespective of usage.
   

   /**
    * Implements hook_validate().
    */
   function anonymous_suggestion_box_admin_validate($form, &$form_state) {
     if ($form_state['values']['anonymous_suggestion_box_method_email'] == 1) {
       if (!valid_email_address($form_state['values']['anonymous_suggestion_box_to_email'])) {
         form_set_error('anonymous_suggestion_box_to_email', t('The To e-mail address is not valid.'));
       }
       if (!valid_email_address($form_state['values']['anonymous_suggestion_box_from_email'])) {
         form_set_error('anonymous_suggestion_box_from_email', t('The From e-mail address is not valid.'));
       }
     }
   }

   

3. (*)
   Any specific reason that you limited teatarea content to 21845?

    if (drupal_strlen($form_state['values']['suggested']) < 2) {
       form_set_error('suggested', t('You must enter more text for the suggested improvements.'));
     }
     if (drupal_strlen($form_state['values']['location']) > 21845) {
       form_set_error('suggested', t('You must enter less text for the location.'));
     }

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are

important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are

recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Adding PAReview: security tag to this thread. please don't remove the security tag, we keep that for statistics and to show examples of security problems.

@benjaminarthurt
Thanks for your contribution.
After reviewing the code & functionality it seems to me that this form can be created by the webform module and does not provide some additional functionality.if it provide additional features, would you please add the differences of this module with the webform module in the summary above.
I am changing its status to postponed. Please change it to 'Needs Review' so that we can review the differences after update.

Thanks again!

@benjaminarthurt Thanks for your quick feedback.

The anonymity is the core purpose of this module. I'll update the project page to better explain this.

Regarding the anonymity there is also a contibuted module Webform Anonymous that does the same job and it adds a simple Yes/No question in the webform form settings to "Anonymize results." It also adds a checkbox to permanently lock the results in an anonymous state, so that it would be impossible for anyone to see who used a webform.
For more confirmation I have attached the webform in which I have used the webform anonymous module and submitted 4 applications.


I have changed the status to 'Needs work'. Please change its status to 'Needs review' with the differences of the your module with the above modules I mentioned so that your module Does not cause module duplication and fragmentation

Thanks Again!

Even with the Webform Anonymous Module it is possible to tell that a logged in user submitted the webform. This issue is exactly what prompted the creation of this module.

This looks like the cool point that makes me to agree at a point regarding it's uniqueness you mentioned but I am not confirmed about it.Will see what other reviewers will think on this point ? Please add it to your issue summary with the difference between this module VS webform + webform anonymous module.Also add other differences if have. I will comeup with my reviews & comments soon.
Thanks Again !

Hi @benjaminarthurt,
Thanks for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. if (isset($_REQUEST['asb_view_start'])) {
       $record_start = $_REQUEST['asb_view_start'];
     }

   uses 'asb_view_start' directly in a URL. This is user input, so it is possible for a user with the 'view anonymous suggestion box' to inject something
   naughty on the page. You need to check_plain() this.

Coding style & Drupal API usage

1. if (drupal_strlen($form_state['values']['location']) < 2) {
       form_set_error('location', t('You must enter more text for the location.'));
     }

   Please keep the minimum characters in a constant and used at other places too where you have used.So that it can be easily manageable.Similar is for the maximum characters limit check.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Thanks Again!

This review uses the Project Application Review Template.

Hi benjaminarthurt,
Thanks for the updates. All looking good.Few More changes below.

Automated Review

All best practices identified by the pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxbenjaminarthurt231263...

Few more code changes

1. No need to set variable value on hook_enable.Use the default value while use it as default value
   

   function anonymous_suggestion_box_enable(){  variable_set("anonymous_suggestion_box_min_textarea", "2");
   
     variable_set("anonymous_suggestion_box_max_textarea", "21845");}

   Please remove hook_enable completely including the configuration link message because there is already configure set in the module info file.

2. Use variable_get with default value in function anonymous_suggestion_box_form()Change this if (drupal_strlen($form_state['values']['location']) < variable_get("anonymous_suggestion_box_min_textarea")) { to if (drupal_strlen($form_state['values']['location']) < variable_get("anonymous_suggestion_box_min_textarea" , "2")) {

Thanks Again !

Hi benjaminarthurt,

I have gone through you module, and found few things:

Automated Review

No issue reported.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

function anonymous_suggestion_box_entry_load($entry = array()) {
  if (isset($_REQUEST['asb_view_start'])) {
    $record_start = $_REQUEST['asb_view_start'];
  }
  else {
    $record_start = 0;
  }
  $record_end = $record_start + 10;
  $select = db_select('anonymous_suggestion_box', 's');
  $select->fields('s');
  $select->orderBy('sid', 'DESC');
  $select->range($record_start, $record_end);
  foreach ($entry as $field => $value) {
    $select->condition($field, $value);
  }
  return $select->execute()->fetchAll();
}

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

I think either you have not committed your changes or have committed to new tag. Please update!!

Commit the changes again :)

Thanks Again!

Duplication

This sounds like a feature that should live in the existing Webform Anonymous project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Webform Anonymous issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process. Based on your descriptions, total anonymity could be a feature that could be added to Webform Anonymous as a patch.

If that fails for whatever reason, or if you think a patch is totally impossible, please get back to us and set this back to "needs review", and we will continue this application

Thanks for the details. I don't have any objections, and think this can proceed.

Assigning to myself for next review.

Automated Review

Review of the 7.x-1.x branch (commit f9a64ec):

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation; addressed sufficiently.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

You may want to eventually move the page callbacks to include files. This can help reduce memory footprint at runtime.

(+) A hook_help() would be nice.

(+) The mail body is anonymous_suggestion_box_mail() is untranslated. It is also best to make the body an array of lines, instead of adding the newlines yourself. See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

(*) Your usage of check_plain on the email addresses in anonymous_suggestion_box_mail_send() is incorrect. These are not going to output. See https://api.drupal.org/api/drupal/modules%21contact%21contact.pages.inc/... for similar handling of email addresses from user input.

In anonymous_suggestion_box_db_insert(), when you write a full record to your own schema, the normal convention is to use drupal_write_record()/

anonymous_suggestion_box_db_insert() has some weird indentations / spacings.

You have a fail number of strings that are using double quotes. Drupal preference is to use single quoted strings whenever possible. See https://www.drupal.org/coding-standards#quotes

anonymous_suggestion_box_admin_validate() isn't a hook_validate. It is a validation handler for the anonymous_suggestion_box_admin_form form.

(*) In anonymous_suggestion_box_view(), you have a few problems. The all boil down to now using the Drupal pager. Your db_select() should use the PagerDefault extension (https://www.drupal.org/node/508796) and then the output should include the pager. See pager_example in the Example module for a concrete example.

(+) anonymous_suggestion_box_view() should return a render array and not themed output directly.

Technically, you also don't need to plain your Submission ID since the database layer is generating this, and it isn't user input.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I also want to note that the security problem in #22 is incorrect as noted in the comment (I am keeping the security tag as I did not got back to to look at this in detail). You are outputting $_REQUEST['asb_view_start'] so it needs to be plained, but user input going to db_select() uses placeholder queries which prevent SQL injection. See the D7 database examples in https://www.drupal.org/writing-secure-code for more details

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Forgot to unassign myself.

Major issues:

• Default values of variable_get()s in anonymous_suggestion_box_form_submit() do not match defaults in anonymous_suggestion_box_admin() form. Therefore, until the admin form gets saved the first time, the default actions (send email but do not save to DB) do not match the admin form (http://bit.ly/1wszkGX). Instead of having to maintain parity between default values on the admin settings form and default values in the form submit, set default values for the Drupal variables in your hook_install().
• hook_form_submit() is not real. Change the docblock comment to something like "Form submit callback for anonymous_suggestion_box_form".
• It's possible for the "site_mail" Drupal variable to be empty, in which case function anonymous_suggestion_box_mail_send() would pass empty $to and $from values to drupal_mail(). If both "site_mail" and "anonymous_suggestion_box_xx_email" are empty, then display an appropriate error message instead of attempting to send.
• anonymous_suggestion_box_db_insert(): $caught_error is always FALSE.
• Comments needed. The only function with inline comments is anonymous_suggestion_box_mail_send(). And even the function docblock comments are pretty short. If the docblock comment only says "Implements hook_x()", then the docblock additional description usually needs details about the implementation and purpose.

Minor issues:

• Fix the capitalization error "Here" to "here": http://bit.ly/1sViU9r
• README.txt says "See below for captcha configuration" but no captcha configuration instructions provided.
• Move $options definition inside the switch statement. No need to call it on every hook_mail() invocation: http://bit.ly/1whsCXw Also, why only pass langcode to the first t() call and not the others?
• On configuration page, change "Do you want to records saved to the Database when a suggestion is submitted?" to "Do you want to save records to the Database when a suggestion is submitted?"
• In the email, add spaces between field name and submitted content: http://bit.ly/1zn209L

Suggestion:

Consider using a custom entity to store the the submissions. In this way you can customize the entity properties and be sure not to store any sensitive data, but still have the power to leverage FieldAPI. The main benefit here is that you could then allow users to customize the form questions.

Removing review bonus. Need one more review (6 total) to get it back after failing a review.

Those two aren't what we consider blocking issues (they are small bugs). Is there anything major that your found?

Assigning to myself for next review, which will hopefully be tonight.

Automated Review

Review of the 7.x-1.x branch (commit d93ac8a):

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. (*) anonymous_suggestion_box_view(): This is vulnerable to XSS exploits. If I enter <script>alert('XSS Form');</script> in the Anonymous Suggestion Box form, tested for Location, Observation and Suggested Improvement elements, every time If I visit View All Submissions page, get a nasty javascript popup. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again.
   In given code, Location, Observation and Suggested Improvement variables coming from user provided input and you are directly rendering without sanitize that's why these popup coming.

   foreach ($results as $row) {
       $rows[$row->sid] = array($row->location, $row->observation, $row->suggested);
     }
     $output['content'] = array(
       '#theme' => 'table',
       '#header' => array('location','observation','suggestion'),
       '#rows' => $rows,
       '#sticky' => 'false',
       '#empty' => t('No results found.'),
     );

   
   
   
   

2. (+) IMHO, you should move your setting form to a separate admin.inc file that is useful to keep all administration stuff like menu callbacks and forms etc. It's also useful for performance as well because all the functions from .module files get loaded at every initialization.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Hi benjaminarthurt

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

This review uses the Project Application Review Template.

I'm not really sure, if this counts as a review, because I was not able to find anything . . .

Tobi

@guelzow,

I'm not really sure, if this counts as a review, because I was not able to find anything . . .

If you do manual code review then it would be consider as a review, if not found any application blockers simply move to RTBC. See the workflow: https://www.drupal.org/node/532400.

Anyways I am going to further review this.

Automated Review

Review of the 7.x-1.x branch (commit 5e8eb49):

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

anonymous_suggestion_box_mail(): the switch() statement does not make sense here since you only have one case? Use if() instead?

Blocking issues from #39 have been addressed and above point is just a suggestion. Been sitting at RTBC for a while now, so...

Thanks for your contribution, benjaminarthurt!

I updated your account so you can 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 stay 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.