[D7] Country Access

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

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.

Hi there,
I went through your code, and this surely would be a handy module.

I'm setting this to "Needs work" because of the security issue below.

Access to the admin form. Of this module requires a permission that is defined by the module itself. Since it does not warn about potential security issues either, module must sanitize the user-provided error message. There is no such thing in the provided default template.

If you enter some malicious code in the error message in admin settings, it gets executed without a problem, who chairs, should not happen.

also, the function you use to get the country code is amend "set" where it's actually returning the country code and nothing else.

As a feature request, I would also like to see if module can support Cloudflare-provided country code. Cloudflare provides the country code in an HTTP header in the request, so you can access it without any module support.

Also, I would use the Drupal core's country_get_list() function to provide a select list for better UX (as opposed to asking user to enter country codes). Select list will provide you with an array, with no duplicates and further validation necessary.

Well, your solution fixes the security issue, but since the value is saved in the encoded form, it gets encoded again and again when you save the form. Try entering a denied message that has "&" character in it.

Attached patch fixes the same issue but it does not sanitize the user-entered value before saving it to the database (you shouldn't). User input is sanitized when outputting it.

Otherwise looks good to me.

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

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

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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 / No: Does not follow] 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.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*)variable_set('country_access_active', 0); line no 14 in ".install" file, this variable not used in .module. Please remove this variable if not required
2. (*)"access_country_active" line number 92, Why you seting this variable here, you can set this variable in .install file and remove "country_access_active" variable
3. (*) Always create admin configuration page changes in new file such as country_access.admin.inc file so that it will be easy to handle
4. (*) line no 56, used l() functon of drupal.As below
   t('Choose the module for IP to country detection') . '
   ' .
   t('supported modules') . l(t('Smart IP'), 'https://www.drupal.org/project/smart_ip') . t(' or ') . l(t("IP-based Determination of a Visitor's Country"), "https://www.drupal.org/project/ip2country"),
5. (*)Used above approach on line 69
6. (*) Why there is need of form_alter line 110, you can used $form['#validate'][] = 'country_access_admin_form_validate'; before return system_settings_form($form); and remove the form alter code.

Assigning to myself for next review that will be tonight.

Review of the 7.x-1.x branch (commit 53a2360):
Already used the pareview template above.
Manual Review :

• Readme is fine. Add a hook_help in module.

Not found any blocker.Assigning @klausi to give it a final look

manual review:

1. country_access_manage_access(): doc block short description is not useful, it only repeats the function name? Please provide a useful description, see https://www.drupal.org/coding-standards/docs#functions . Also elsewhere, please check all your doc blocks.
2. country_access_user_has_role(): I would not hard-code this to user 1. I would create a permission "bypass country access" and if the user has that permission then I would not block the request, ever.
3. country_access_page(): do not call "print" in a page callback. Always return a render array instead. See https://api.drupal.org/api/examples/page_example!page_example.module/fun... for example.
4. "'#title' => 'Select the action to perform on the countries listed above',": all user facing text must run through t() for translation.
5. country_access_admin_form_validate(): doc block is wrong, this si not a submit callback and for validation callbacks should be documented as described in https://www.drupal.org/coding-standards/docs#forms
6. country_access_preprocess_page(): Since this has nothing to do with theming I would use hook_page_build() instead.
7. This module will not work for sites that have page caching enabled. Scenario: anonymous user from an allowed country requests a page => page gets cached. Anonymous user from a disallowed country requests the same page => Drupal happily serves the previously cached page bypassing all menu callbacks and the country access check. I think you should add a big fat warning to the project page that this will not work with page caching. You could also look into implementing hook_boot() which will always be called, but that has its own problems since you have to manually load the IP modules.
8. country_access_set_country_code(): $_SESSION might not always be set for all users (anonymous users for example), so you will get PHP warnings here that the array key does not exist? Shouldn't you call smart_ip here and ask for location info if there is not session?

I'm not sure if the page caching access bypass qualifies as security issue, since users can work around the country access check with proxies anyway. The rest is also not critically blocking, so ...

Thanks for your contribution, Vincenzo Gambino!

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.