[D7] Custom Required Marker

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.

I read through the code and it looks fine. I haven't run it through coder or actually enabled it yet, but plan to do so.

Always a bit of a challenge with names as long as "custom_required_marker" & the 80 character lines. Not a big deal though.

Thanks for contributing this Dave. I know that there are folks who worry about this as an accessibility issue and will want to enable it for their site. I don't see it as a barrier (having discussed this in D7 & D8 Core) but it certainly makes it more explicit.

Somewhat related Core issues.
#1162802: Asterisk * used for required or changed marker should be in abbr not span
#2121775: Make the markup associated with the required star on field items silent
#72197: Forms should show explanation of required (*) fields

Ran the module. Behaved as expected. Coder gave it a great review too. This looks good to go for me.

Posting the review below , needs one security fix

Automated Review :
Coder found no warnings, issues with the project.

Manual Review:

Individual user account
Yes, Follows
No duplication
Yes,Follows
Master Branch
Yes: Follows
Licensing
Yes: Follows.
3rd party assets/code :
Yes, follows
Security Code:
No.
1.(*) The configuration form input fields need to be sanitized , If we enter

into the input, then it will create a javascript alert .This vulnerability is mitigated by the fact that an attacker must have permissions to administer the markers, but this should still be fixed.
2. Change the title and description of the second field (Custom Required Marker Marker to a more helpful one) to make it more clear , maybe Custom Required Marker Text ? Not a blocker, just a recommendation as it got me a little confused until I saw what it did.

After the security fix , it’s good to go

The comment stripped the script tags around the alert, posting it below
<script>alert('XSS');</script>

1. custom_required_marker_settings_form_submit(): "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984 . So the filter_xss_admin() is wrong here and should be done when the content is printed to HTML. Looks like it is done in custom_required_marker_form_required_maker().
2. custom_required_marker_form_required_maker(): this is a theme function so it should not do any database queries. All variables a theme function needs should be passed to it. See https://api.drupal.org/api/drupal/includes!theme.inc/function/theme/7 . Sanitizing those variables shiuld either be done before calling theme() or in a preprocess function.

Oh and since there was an XSS vulnerability I'm adding the security tag. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

drupal_add_js in the form alter should be replaced with #attached on the $form variable. Make sure everything works as expected. I let Dave know already.

Looks good to me.

Hi
The modules works fine just 2 issues that i noted
1> When we enter any text inside < > the text is not displayed as it is stripped
For eg if i enter in Textfield nothing is dipalyed

2>Instead of stripping for XSS which will change text it would be better if a form validation can be added to throw error when unwanted
character are entered in Textfields

I noticed you are are using "if (function_exists('i18n_variable_set'))" to handle variable translation which is not very conventional, there is a better way to implement this
please check https://www.drupal.org/node/1113374
Thank you for contributing

Agreed that it's had a pretty through review at this stage.

Adding to my queue.

To address some specific comments in this issue:

#14, you never know who will have admin privileges on a site (think sites with multiple admins). It is even more important to protect admin pages from accidental input, as you may compromise a site via an admin user.

#23, the Drupal way is to store data exactly as entered and sanitize on output: https://www.drupal.org/node/28984

#24, that is something very minor and not a reason to push an application back to NW. The current main reasons are: security issues, licensing issues, and third-party code issues.

Automated Review

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

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

  
  FILE: /Users/matt/PAR/pareview_temp/custom_required_marker.module
  ---------------------------------------------------------------------------
  FOUND 3 ERRORS AFFECTING 3 LINES
  ---------------------------------------------------------------------------
   41 | ERROR | [x] Multi-line function declaration not indented correctly;
      |       |     expected 4 spaces but found 2
   42 | ERROR | [x] Multi-line function declaration not indented correctly;
      |       |     expected 4 spaces but found 2
   43 | ERROR | [x] Multi-line function declaration not indented correctly;
      |       |     expected 4 spaces but found 2
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  
  Time: 233ms; Memory: 7Mb
  

• ESLint has found some issues with your code (please check the JavaScript coding standards).
  

  /Users/matt/PAR/pareview_temp/custom_required_marker.js: line 6, col 1, Error - Definition for rule 'keyword-spacing' was not found (keyword-spacing)
  /Users/matt/PAR/pareview_temp/custom_required_marker.js: line 8, col 3, Error - Strings must use singlequote. (quotes)
  
  2 problems
  

• 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.

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

Secure code

Security issues found by @klausi are addressed by using filter_xss_admin() in a preprocess for the theme function. Ok with me.
Also thought about this a bit, and do think filter_xss_admin() admin here is appropriate to allow an admin to build HTML for this for a11y reasons, instead of a plain text message.

Coding style & Drupal API usage

You should add a hook_help() at some point.

custom_required_marker_settings_form() has a @see to a non-existant function.

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.

Two admins have looked at this; I feel comfortable here with promotion now.

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.

Thanks for your contribution, Dave Bagler!

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.