[D7] Report Field

Manual code check result

1.1 Ensure your application contains a repository and project page link.
OK
1.2 Ensure your project is not a duplication.
It is not.
2.1 Ensure the repository actually contains code.
OK
3.1 Ensure the project does not contain any security issues.

hmm I am not a security expert but you could consider two things
- use db_select instead of db_query line 72 $query = 'SELECT COUNT(*) FROM {report_field_invalid_fields}';
- use checkPlain() for $_POST vars esp line 104 and 105 where user can submit content

4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.
OK
4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.
OK
5 and 6 are ok to me
7 API check
line 144 you should use drupal_goto instead of header("location:index.php");
line 150 function report_field_form($form, &$form_state) { you should not build table manuallly use theme table function
line 393 you should use t() for text strings

You should not add html tags to variables. You should use apropirate theme function instead.

Hi dkolarevic,

the constant REPORT_FIELD_SECRET_HASH is not secret anymore, when you define it in your (open source) module. You shoud generate secret tokens by using drupal_get_token(). (This is an application blocker for me).

The wrapper in your report_fiel.js is missing one parameter. You define the anonymous function with three parameters, but you are calling it with four arguments. So the parameter document gets the value of this (the window object).

(function ($, Drupal, document) {
.
.
.
}(jQuery, Drupal, this, this.document));

It works but it may be confusing.

You should at least print the content of your README in hook_help().

You should consider to move your admin setting forms / callbacks to a separate report_field.admin.inc file.

In report_field_submit_form():468 you are checking for the not existence of $form['#attributes']['class'] and then you set $form['#attributes'] = array('class' => 'field-flag-submit-form');. In the case wehn only the 'class' is not set but other attributes, you would remove all the other attributes. Since you do it in your own form, its OK but if you would do it in e.g. hook_form_alter() I would have counted it as an application blocker. (The same in report_field_error_checkbox_form():422)

The menu item $items['callback'] that you define in your hook_menu() is very general. You should make it more specific to your module to avoid conflicts with other modules. The same for $items['mail_confirm'].

Just a note: Why are you using nested if's instead combining their conditions in one if in report_field_preprocess_field()?

Best regards

@dkolarevic just a casual check which could be helpful in UI perspective.

Spelling -> Typo Error
README.txt
Typo: In word 'Simular' (at line 11)

report_field.module

Typo: In word 'Confim' (at line 58)
Typo: In word 'registred' (at line 74)
Typo: In word 'registred' (at line 90)
Typo: In word 'unregistred' (at line 122)
Typo: In word 'Unregisterd' (at line 304)

Unused Variable
report_field.module
Unused parameter 'logged'. The value of the parameter is not used anywhere. (at line 433)

Don't you have hook_permission inside your module. ?
Don't we need it.

@dkolarevic

Well done :). Nothing to add from my side.

Hello

Here are few notes about the module

report_field.module
missing t() on form labels in report_field_submit_form()
cache_clear_all() might cause performance issues in report_field_preprocess_field() maybe another way?

report_field.js
$(document).ready() please consider drupal behaviors , here is a useful link https://www.lullabot.com/articles/understanding-javascript-behaviors-in-...

Automated Review

The automated review by pareview.sh looks good. No automated test cases were found, but this is not a requirement.

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.

README and project page might be improved further by more closely following the README template (linked above) and the project page template, but the basic information needed is there. You also might want to check out the tips for a great project page.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Doesn't meet the security requirements. See below for more information on the issue(s) identified.

Coding style & Drupal API usage

1. (*) XSS: You're working with the raw values entered by the user without filtering them for XSS in some cases. This is a security issue! You need to sanitize all insecure values using either check_plain(), filter_xss() or filter_xss_admin() before output. To test, try the following:
   • XSS for Reported setting:
     1. On your module's settings page, enable the reporting functionality for a field and add <IMG SRC=/ onerror="alert(String.fromCharCode(88,83,83))"></img> as value of the Reported field.
     2. As a logged in user, report the field for a node by submitting the form.
     3. Once you reload the page, you should see a popup displaying XSS.
2. (*) module file, line 38: The permission administer site configuration is already defined by the system module of Drupal core. Since there is only one permission name space, as a best practice put your module name into every permission name you define. A common name for a permission like this would be administer report fields. Currently, your implementation of hook_permission() results in the permission being listed under your module name instead of the system module name, which might confuse users. It might make them inadvertently think that the permission only applies to your module. So this is a security issue. Either change the permission name (recommended) or, if you really want to do that, just remove your hook_permission() implementation and reuse the permission defined by the core system module (discouraged).
3. (*) I can basically add arbitrary values to the table shown on your reported fields page, since you do not check any of the fields, before you write them to the database. The only requirement is, that the node id is numeric, because otherwise I'll get a PDO exception. This can't be used for XSS, since the values are sanitized by theme_table(), but you might want to do more validation. E.g. field name and node title are passed on to the email as is and I can set any email address I want. I leave the rest to your imagination. To try it yourself, you could e.g. use curl like this: curl --data "passedEmail=Hi there, how are you?&passedMessage=Okay, this is just some text.&passedFieldName=The field name is not validated.&passedContentType=I can add anything I like here.&passedNodeId=0&passedNodeTitle=Reviewing the field_report module" http://example.org/report_field_callback. To fix this, perform more validation on the fields and implement Drupal AJAX API for your form (see below). You could then get rid of the hidden fields and rely on form state to pass these values around (recommended). Or calculate an hmac on the hidden fields and validate that (see below for a Drupal API function to do that, but discouraged).
4. (*) report_field_page_callback_function(): You're using openssl_encrypt() to generate a hash value to send as part of a one-time url. Depending on what you intended to do here, this might be a security issue or not. If you intended to securely encrypt the data, it will not work. E.g. your initialization vector should be random (and unique), but you are using the same string every time. Also, strings returned by openssl_encrypt() are not neccessarily safe to use in URLs without proper treatment. Encryption can be difficult to get right. So why not instead rely on the API of Drupal core, which is supported by the Drupal security team? E.g. one option is to use drupal_hmac_base64() to get a url safe hmac value.
5. (*) module file, line 14f: As a rule of thumb, never place arbitrary code outside of a function (the defines are fine, though). If you really want code to run on every page request, there are a number of hooks available, that you could implement. In this case, the best option you have is to either add your JS and CSS only where it is needed (as a best practice, do not use drupal_add_js() or drupal_add_css(), but use #attached instead) or specify the stylesheets and scripts options in your .info file (see Writing module .info files (Drupal 7.x)).
6. (+) info file, line 3: Please do not add a separate package just for your module, but choose a fitting package name from the ones already used by other modules, e.g. Fields or User interface.
7. (+) While fields may be attached to any entity (e.g. user or taxonomy term), your module only seems to work with fields attached to nodes. Please document this on your project page and in your README file, since it might not be expected by users of your module.
8. (+) It seems that your module will currently only work for people with JS enabled. Please document that in your README file and on your project page, since it might not be expected by potential users of your module.
9. (+) report_field_form(): Please prefix all variable names (I refer to variable names used with variable_get() and variable_set() here, not to regular variables) with your module name to avoid clashes with variable names used by other modules. Also note, that it is common practice to use underscores instead of dashes as a separator in variable names.
10. (+) report_field_form_reading_db(): Why are you using system_settings_form() here? No need to save all the form values using variable_set() for this form, since you already store them in your own table. For performance reasons, please don't do that. In Drupal 7, every variable present in the system will be loaded on each page request, so the less variables you set, the better.
11. (+) report_field_preprocess_field(): You're including the admin.inc file here and load your settings form. Then you loop through the form to get the settings. Please don't do that for performance reasons. Instead, first check, if $vars['element']['#entity_type'] equals node, since you only support fields attached to nodes currently. Next, get the content type from $vars['element']['#bundle'] and the field name from $vars['element']['#field_name'] and prepare the name of your variable. Then use variable_get() to get the settings for that field and act on them accordingly.
12. As a best practice, please use single quotes instead of double quotes unless in a few very specific cases (see Drupal Coding Standards). There are several places in your module, where you use double quotes instead of single quotes.
13. report_field_page_callback_function(): Since this page callback is unrelated to your admin pages, consider moving it into your module file for clarity.
14. report_field_form(): If a system has a lot of fields attached to nodes, a lot of variables might be added by your module. Since Drupal 7 will load every variable for each page request, fewer variables are better. Consider using a separate table for your settings for performance reasons.
15. report_field_form(): Since the settings are stored by node type, consider removing unused variables/settings upon deletion of a node type (e.g. by implementing hook_node_type_delete()).
16. report_field_form(): Consider adding some explanation text to the form that describes, what the different fields (Label, Reported and Message field?) do or add it to your documentation (README/project page). It might not be obvious for users not familiar with your module.
17. _report_field_form_validate(): A better approach would be to name this function report_field_form_reading_db_validate, so that Form API will call it automatically for your form.
18. hook_menu(): To give admins more fine grained control over permissions, consider adding a permission to use your module and use that instead of access content.
19. module file, line 85: Why are you including admin.inc here? The callback is in your module file and nothing in it seems to rely on admin.inc being loaded.
20. module file, line 113: To make your form work with other modules, that might extend your module in the future, don't pass a string here. The class attribute is an array containing one value per class. Note that there are more places in your module, where this should be changed.
21. report_field_render_form(): Instead of calling drupal_render() twice, you might want to create a render array containing both forms and call drupal_render() only once on that render array.
22. report_field_render_form(): Is there a specific reason why you are using two different forms? Why not just combine the two forms into one? If you did so, you could use the #states option of Form API to hide a field initially and only show it, if the checkbox has been checked by the user instead of writing your own JS for that.
23. report_field_submit_form(): Instead of using your own JS file to submit the form via Ajax using jQuery, why don't you use the Drupal AJAX API? See Ajax Forms in Drupal 7 and the AJAX example module for more information on how to implement it for your form. If implemented properly, you would automatically gain support for people having JS disabled to mention just one benefit of using Drupal Core APIs over custom solutions.
24. report_field_preprocess_field(): The DocBlock should read Implements hook_preprocess_HOOK() for field.tpl.php.
25. report_field_preprocess_field(): Instead of adding the email address of the user as a JS setting here, why don't you check in the form building function, if the user is logged in and prefill the email field there by adding it as a default value? From your JS, it looks like you don't want the user to change the email address, since you set it to readonly. Note, that a malicious user could still change her email address, since you rely on the data in the form to be accurate. For a more secure approach, why not hide the email field for logged in users entirely by setting #access => FALSE for the email form element in the form building function and using $user->mail for logged in users when you insert the values into the database?
26. report_field_preprocess_field(): If you want to keep the JS setting, as a best practice, add your setting in a bucket containing your module name to avoid clashes with other modules and use #attached like this:
    

          $vars['items'][$el_no - 1]['#attached']['js'][] = array(
             'data' => array(
               'report_field' => array(
                 'currentUserEmail' => $user->mail,
               ),
             ),
             'type' => 'setting',
           );
          

    To get your setting in your JS file, use Drupal.settings.report_field.currentUserEmail.

27. report_field_preprocess_field(): There is a comment regarding support for field collection fields. Why don't you add a setting for fields referecing field collections, so that admins can enable this in your settings form?
28. report_field_mail_confirm_function(): A user might pass an invalid hash, which is not present in the database (e.g. if another user already confirmed her report for the same field or the admin deleted a pending report). You might want to check for that case.
29. report_field_mail_confirm_function(): Consider passing the hash as a parameter to the menu callback using the page arguments option of hook_menu() instead of relying on $_GET.
30. report_field_mail_confirm_function(): If you want to redirect to the front page, use drupal_goto('<front>');.
31. report_field_mail_confirm_function(): A confirmation message to the user might be a good idea, so that the user knows, that the field was successfully reported.
32. report_field_mail(): Instead of using $base_url to construct the URL, consider using url() with the absolute and query options.
33. JS file: If you follow my advice above, you could get rid of most of your custom JS code, so I wont go into as much detail here. However, to make your module compatible with other modules using the Drupal AJAX API, add and use the context parameter to your anonymous function (assigned to the attach property of your behaviour). You need to pass that as the second parameter to some of your jQuery calls to make sure, that you only act on stuff present in the context parameter. Also, you should make sure to process every element you target only once, e.g. by using jQuery Once (bundled with Drupal Core), since a behaviour might be called multiple times for the same page.
34. Improve DocBlocks of your non-hook functions so that they more closely follow the API documentation and comment standards (e.g. add @param and @return where missing).
35. If a lot of anonymous users report fields, but they never confirm these reports, the database might get full of those pending reports. Consider to add an option to delete pending reports after a specified time using hook_cron().
36. You delete all pending reports for a field, once a user confirms a report for the field. If the message field is enabled for a field, messages in pending reports might provide additional insight to admins into why a field was flagged, so you might not want to delete all pending reports for a field upon confirmation of a report for a field.

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.

Revision reviewed: 74378d3

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.

Hi dkolarevic,

thank you for your work on this, very good job! The code looks a lot better now.
I did not do a full review this time, but instead looked at the commits you made
since my last review.

As far as I can see, the following items have been resolved:

1., 2., 3., 4., 5., 6., 7., 9., 10., 11., 12., 13., 17., 20., 21., 22., 23., 24., 25., 26., 30., 33.

For the record, the following items are still open. While not required to pass
the review, I really appreciate your intention to work on these as well:

8., 14., 15., 16., 18., 19., 27., 28., 29., 31., 32., 34., 35., 36.

Regarding item 19., it is now on line 66 of the module file.

Also, there are a few new observations, which you might want to consider working on:

37. (+) report_field_form(): You're attaching a CSS file to the form, but you have since removed the file from the repository. It's nice that you used #attached instead of drupal_add_css(), though.
38. (+) report_field_error_form(): Please use t() for the value of your submit button.
39. report_field_form(): No need to explicitly specify the validation handler, since it should be called automatically by Form API based on the function name. Doesn't hurt, though ;).

None of the new observations are blockers, though the first two items should be
fixed before a stable project release.

Bottom line: I verify that all blocker issues and especially the security issues
identified in my original review have been fixed. In my view,
this module is RTBC! :)

Revision reviewed: 7fc0b61

Thanks for your contribution, Dragan!

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.