[D7] Agegate

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxnazarioa2600074git

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.

Please note that you are not supposed to change the status to "Fixed". It can only be "Fixed" when you are approved to grant the module page permission.

I didn't check your code, but I found the README.md is not in markdown format. It is better to change it.

@nazarioa

No worry. I am new in this process and still waiting for someone granting me the module page permission too. I have waited for 2 weeks but there are many issues posted for months. So you need to be patient :)

I would like to know if there is an official validation tool for README.md in Drupal community too. You still can use some online markdown editor to check the correctness of syntax, such as https://markable.in

There are some other suggestions.

* please remove .gitignore, it seems you have copied the .gitignore from drupal project.

* add comment for the constant you defined in .module file.

* consider to use drupal_match_path() rather the below snippet from line 71 to 82 in .module file.

  // Is the current page one of the pages where the Agegate not be dispalyed?
  $current_path = trim(request_path());
  $hide_from = array_map('trim', explode("\n", variable_get('agegate_hidefrom')));

  foreach ($hide_from as $needle) {
    $test = strripos($current_path, $needle);
    if ($test > -1) {
      $display = AGEGATE_NO;
      // Current page is in the "do not display agegate on this page"
      // list so lets set dispaly to no.
    }
  }

* I found that $_SERVER['PANTHEON_ENVIRONMENT'] is specific for Pantheon platform. I don't know if it is a good practise to include platform specific code in contrib module. You need to remove it or write down some comment for this part.

* change agegate__verification_box_html() into agegate_verification_box_html(). It is rare to have double underscore in function name.

* implement hook_uninstall().

I change the status to "Needs work". You can change it back to "Needs review" after fixing the problems.

@nazarioa

Great job, mate.

I just read through your code and did find something I missed last time.

/**
 * Implements hook_theme().
 */
function agegate_theme($existing, $type, $theme, $path) {
  $result = array(
    'preprocess_method' => array(
      'render element' => 'element',
      'template' => 'agegate--popup',
    ),
  );
  return $result;
}

Your theme function code is valid but it cannot follow the best practise. 'preprocess_method' should not be the proper name of your theme key. In your case, I suggest you to change it as 'agegate_popup', then you will have your own theme function as theme_agegate_popup().

Also, refer to this post, 'render element' is only used for renderable array element. You need to make use of 'variables' rather then 'render element'. So that you can directly use the variables in template. You don't need to put them into $agegate as array.

Everything worked as I expected without any problems. The only default configuration option I had to change to activate the popup was "Cookie's Domain".

I confirmed that the issue brought up in #12 has been fixed.

Secure code

You probably want to use check_plain() or filter_xss_admin() on the text strings you are outputting here. It is currently possible to include and execute javascript (eg, <script>alert("Hello");</script>) in agegate_name, agegate_message, agegate_html_btn_cancel, and agegate_html_btn_verify

  $settings['popuphtml'] = theme('agegate_popup', array(
    'agegate_html_validatebox'  => agegate_verification_box_html($settings['verificationtype'], array('age' => $settings['legalage'])),
    'agegate_name'          => variable_get('agegate_sitename'),
    'agegate_message'       => variable_get('agegate_sitemessage'),
    'agegate_html_btn_cancel' => '<button id="agegate_cancel" class="button cancel-verify">' . variable_get('agegate_cancelbtntext') . '</button>',
    'agegate_html_btn_verify' => '<button id="agegate_verify" class="button age-verify">' . variable_get('agegate_verifybtntext') . '</button>',
  ));

See template_preprocess_page (https://api.drupal.org/api/drupal/includes!theme.inc/function/template_p...) for an example of how it's handled in core:

$variables['site_name'] = (theme_get_setting('toggle_name') ? filter_xss_admin(variable_get('site_name', 'Drupal')) : '');

Some nitpicks to consider for more polish:

1. On the admin config screen:

   What pages would you like the Agegate to AGEGATE_NOT dispaly on?

   Why is the word "NOT" replaced with "AGEGATE_NOT" here? It makes the sentence confusing. Also "display" is misspelled.

2. Why is the module called "Agegate" (one capitalized G) instead of "AgeGate" (two capitalized Gs)?

   I bring this up because with only one upper G, the name of the module looks like a misspelling of or a play on the word "aggregate," which is not what you want. Maybe you have a specific reason for the current spelling.

3. On the config screen, "Cookie's Domain" should be changed to "Cookie Domain".

Good work!

Tried to install module on new installed drupal 7, got that error. Error is because of jqeury_update.

Automated Review

No automated tests, but this is not a requirement.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

There is the sandbox module mentioned in comment #22, but as far as I can see the author never applied for full project status. Also, there is https://www.drupal.org/node/2013591, where the author has been granted permission to promote his sandbox project to full project status, but it seems, he has never done so.

There is https://www.drupal.org/project/le_gate, which sounds similar in functionality. As far as I read the guidelines, unfortunately, this is a blocker for now, but you might follow up on this and explain, how your module is different.

For now, I have to review this as No: Causes 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

In the README.md, date is listed as a dependency, but it is missing on the project page.

Similar projects is missing an actual listing of similar projects.

You might want to link to the configuration instructions in the REAMDE file from the project page.

Otherwise good job.

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

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

Coding style & Drupal API usage

1. (*) You use user_role_grant_permissions() with a specific role id in hook_enable(). Since the role id of the administrator role may be configured at Configuraton > People > Account settings > Administrator role, this is a major security issue! Also, while I imagine there are people who appreciate the functionality, I like modules, which do not silently assign permissions, but that's just me ;). So either get the correct role id from configuration or remove the functionality, please.
2. (*) XSS: Your website name, legal message, verify button text, cancel button text and Cancel URL settings are vulnerable to XSS. Use filter_xss() or filter_xss_admin() before you pass these to JS or output them in a template/function. This is a major security issue! Check it yourself by adding e.g. <IMG SRC=/ onerror="alert(String.fromCharCode(88,83,83))"></img> into any of the settings.
3. (+) List date as a dependecy on project page.
4. (+) List date_popup as a dependecy on project page and README file.
5. (+) Use t() for the strings on line 397 of module file.
6. You have an option to configure the legal drinking age, but in several places in module descriptions and help text you refer to a specific age 21. Potential users might think, that they can't configure the age. Also, there might be other use cases for age validation than verifying drinking age so maybe rename the option to something more general like Minimum age or so.
7. Typos in line 4 and 11 of install file.
8. You might want to remove either line 10 or 11 in module file.
9. Typos in line 39 of module file.
10. Since you already added the js to your info file, you don't need to add it again in hook_preprocess_page().
11. You might want to generate and add the js settings in hook_preprocess_page() only, if age gate should be shown for this path.
12. A more common name for the permission would be Administer Agegate.
13. You might want to use single quotes for the cookie name in line 321 of your module file to avoid the escape slashes and make it easier for the l10n community.
14. Line 337 of module file: There is no hook_form_validate(). Instead, this is a validation callback for your form.
15. The doc block of agegate_verification_box_html() is missing documentation for the parameters and return values.
16. In line 78 of your module file, you check for the administrator right and do not show the box, if set. A better approach would be to provide a permission "Bypass age gate verification" in hook_permissions() and check for that permission. That way, users can also assign this permission to other roles, that may bypass age gate verification, e.g. site editors or users, who are known to be over the required age.
17. Typo in line 7 of template file.
18. Wrong name of the preprocess function in line 15 of template file. Since you don't provide it anyway, just remove the line.
19. In the doc block of your template file, you specify other variables than in the implementation of the file. Also the variable names should be listed in the variables parameter in hook_theme().
20. If title and message are not required, why don't you check in the template file, if they are set, before outputting the wrapper divs?
21. You might want to pass verification type and age directly to theme() and use a preprocess function to generate the validation box, instead of using agegate_verification_box_html(). That way, people could modify the html of the validation box in their theme or custom module, if required.
22. In your JS, use the context variable like so $('body', context) to make it work with other modules using Drupal AJAX API.
23. When you click on Configure > Agegate, you don't see the options page. You have to click on Configure > Agegate > Settings.
24. You might want to improve the description of Do Not Display Agegate On These Pages by telling users, that they should list one entry per line. Also, I was able to use an asterisc (*) as a wildcard to exclude e.g. all admin pages. That's a nice feature that should be documented in the description so that users know about it.
25. You might want to set the default cookie domain to the domain of the current Drupal instance to make it work out of the box.
26. Cookie format encoded option in advanced settings is unchecked by default. From the description, it looks like you intended to have this checked by default?
27. If you do not specify any pages, where you do not want to display agegate, the age gate will not be shown on the front page.

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 the security tag for tracking and to show examples of security problems. Thanks a lot for finding those @FeyP!

Hi nazarioa, sorry, that it took me so long to get back to you.

I didn't do a full review this time, but instead reviewed the git commits made since my last review. I verify, that the blocker/security issues I identfied have been fixed. I installed the module again and was no longer able to reproduce the XSS issue. I also checked your project again using preview.sh and it found a few issues, but this doesn't block the application. It would be nice, if you would fix those errors, though.

I also learned since my last review, that module duplication is, though discouraged as we prefer collaboration over over competition, not really a blocker for an application. So...

... in my view, this module is RTBC :).

That said, I found a few issues in your commits:

28. (+) 3fdc01a: One check_plain() is enough for agegate_name
29. 880c589: Please use single quotes instead of double quotes since double quotes are no longer needed after you changed the wording.

Regarding your questions:

2) So I tried adding "<IMG SRC=/ onerror="alert(String.fromCharCode(88,83,83))"></img>" as you suggested to see what would happen. I really wanted to see it break but it did not. All I see is the string. See attached screen shot. Someone else pointed this out as a possible vulnerability (see: #19 - #21) and I added check_plain() to handle it. I would like to hear how one is better than the other in your opinion.
See image.

I reviewed the code in your public repository on drupal.org and at the time, those check_plain()s were not there. Maybe you forgot to push the lastest revision to drupal.org after you made the updates? Anyway, it is fine now and I was not able to reproduce the XSS.

10) I prefer to remove it from the .info file so that is what I did. Thoughts?

That's fine. It just shouldn't be included in both places.

13) Interesting. I used to have it like this 'Some text "with some quoted text" and then some more text'. I was told this was bad form in the Drupal world -- normally in PHP I only use single quotes for strings. Are you suggesting that I change the inner text to be single quote? According to American English grammar that is incorrect. Please advise.

It's important to remember that localizers are not neccessarily proficient in PHP or coding in general, so we can't expect them to know how to escape single our double quotes properly. So we should avoid strings, that require escaping to make it easier for the localiziers. It's true, that single quotes should be the default. There are a few clearly defined exceptions to this, however. One of them is escaping quotes. So instead of t('Let\'s mark this module as RTBC'); you better use t("Let's mark this module as RTBC");. In the example in question, part of your string is You probobly don't need to change this, so you correctly use double quotes. But now you need to escape the double quotes around the cookie name \"ISLEGAL\", so we gained nothing here. That's why I suggested you use single quotes around the cookie name. I'm not a native speaker of English, so I don't know that much about grammer ;). If it's bad style, consider using HTML to highlight the cookie name instead: t('Ewinery looks for a cookie named <em>ISLEGAL</em>');.

Read more about the topic in the Drupal Coding Standards and Localization API documentation.

Thanks for your contribution, Nazario!

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.