[D7] Custom Sitename

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

Fixed the git clone URL in the issue summary for non-maintainer users.

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

There is some coding standard errors to fix:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxsahilkhambra2774499git

Individual user account

Yes

Master Branch

Yes: Follows

3rd party assets/code

Yes: Follows

README.txt/README.md

No: Please add a README.txt or README.md file.

Code long/complex enough for review

Yes. Well documented.

Secure code

Yes: None security issue found. Looks good.

At least the function names should be fixed from the automated review, can you do that @Nitesh Pawar?

@Sahil Khambra

Add readme file for your module.

https://www.drupal.org/node/2181737

Hi @Sahil,

In custom_sitename.module :

• Variables need to be sanitized, use check_plain/filter_xss for the variables used inside t() function.
  Example : In line 307 drupal_set_message(t($sitename_data['sitename'] . ' has been deleted.'));

Please use t() functions for text arguments in order to make it translatable.

Thanks,
Nisith

Hi Sahil Khambra,

I did a manual review of your code and installed this module in local. Please see my comments below:

1. Sanitize user entered values before displaying in front-end. If enter javascript inside sitename field, it is getting executed while displaying. To replicate the issue give this value "<script>alert('XSS');</script>" inside sitename field. It seems like XSS vulnerability and an application blocker. Use check_plain()/filter_xss() function to sanitize data before displaying. In admin pages you can use filter_xss_admin() to sanitize values.
2. Add configuration url in custom_sitename.info.
3. Validation error while editing existing sitenames. While editing content type site names, getting error "URL does not exist". Please fix this.
4. Fix typo in error messages. eg: URL already exit with alias !value.

Thanks,
ARUN AK

Error changed to:- Sitename for the given URL already exit with alias !value

You mean the error message

Sitename for the given URL already exist with alias !value

?

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

I was manually reviewed your module, as said in comment #13, still the code is not sanitized,
I tried setting site name as <script>alert('XSS');</script> and saved, immediately alert is triggered. When i navigated to page also, i got the alert.

But in the comment #14, you have mentioned that you did sanitization.
Whatever you mentioned in comment #14 is not in the git repository.
when i check the git log your last commit message is " sahil: change in menu callback"

I think you need to check your git, whether you pushed the latest changes to repository.

git log shows your last commit

commit 8feb1523fd54da3ce55b01ca3b273a67646fd886
Author: Sahil khambra <khambra.sahil@gmail.com>
Date:   Tue Sep 20 13:17:58 2016 +0530

    sahil: pareview fixes

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi,

File custom_sitename.module
1. Line 148, 150, etc
We can use single qoute instead of double

2. Line 149, 151
We can remove double qoute from $value->id

3. Line 347
We can directly return $query->execute()->$fetch_type(), no need of $result variable.

@rajat.dkte: Double quotes vs. single quotes and variable usage alone is surely not an application blocker. Anything else that you found or should this be RTBC instead?

Hi klausi ,

It should be in RTBC.

Hi @sahil-khambra
This module works fine for me. But one suggestion, I think list of all the sites name on edit page (/admin/config/system/custom_sitename/edit/[ID]) is bit confusing.

Thanks,
Khurram

Hi Sahil Khambra,

Module looks good , my findings are :

1: do not use filter_xss() on values that are inserted into the database. "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

function custom_sitename_form_submit(&$form, &$form_state) {
  $form_values = $form_state['values'];
  $values['type'] = $form_state['type'];
  $values['sitename'] = filter_xss($form_values['sitename']);

2: If i am saving &gt;<a href="http://www.google.com">fishing&lt;</a> as site name and then on click of fishing its redirecting to google .com

So You need to sanitize data before printing. For more information about sanitizing, please read https://www.drupal.org/node/28984.

Hi ,
It works all fine. Changing status to RTBC.

Please use % or @ instead of ! placeholder.
The placeholders (e.g. '%value' or '@value') are passed as plain-text and will be escaped when inserted into the translatable string. You can disable this escaping by using placeholders of the form '!value' but only if you are sure that the string is safe.

(https://www.drupal.org/node/28984)

    if ($check_url_exist) {
      $result = custom_sitename_fetch_record($path_to_check);
      if ($result) {
        form_set_error('url', t('Sitename for the given URL already exit with alias !value',
          array('!value' => $path_to_check['value'])));
      }
    }
    if ($check_url_alias_exist) {
      $path_to_check['value'] = $check_url_alias_exist;
      $result = custom_sitename_fetch_record($path_to_check);
      if ($result) {
        form_set_error('url', t('Sitename for the given URL already exit with basepath !value',
          array('!value' => $path_to_check['value'])));
      }
    }

Hi Sahil,

Your module provides an interesting functionality, but I think it can be achieve by using other existent components. Site name can be enclosed as a block, actually that's the default in D8. blockify module provides a block for a lot of common display components like the site name, site slogan or the page title. Then you can control the main site name visibility block with visibility settings, and add some other custom block as an alternative where needed.

Never the less, the site name is an important variable for a website that should remain constant for a same domain name. You may consider the powerfull domain module. With it, you can create several subdomains for one drupal installation, with custom settings each one, like the site name.

Review from site-builder point of view

• module name is entirely typed using uppercase in the .info file. Only capitalized words should be used
• custom administrative permission not needed, general "administer site configuration" permission is just valid
• URL config type doesn't support willcards?, that's a basic feature
• consider other entity types, like taxonomy terms

HI Sahil Khambra,
Please fix space warning from your module file.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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