[D7] Variable Editor

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

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.

You have some inconsistencies in quotes.

Line 83 of your .module file has single and double quotes. You should stick with single quotes in this case:
from
$form['#attached']["js"][] = drupal_get_path("module", "variables") . "/variable_save.js";
to
$form['#attached']['js'][] = drupal_get_path('module', 'variables') . '/variable_save.js';

You have a number of other style related errors reported by the review tool linked by PA robot. You should address these issues. Also, coder is a very useful module to install and check your code locally.

Manual Review

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

I reviewed you code and found following things in code.

1) Still there are some coding standards errors.
Please refer below url for errors:
http://pareview.sh/pareview/httpgitdrupalorgsandboxfabianferns302561535git

2) Variables name should be more meaning full and readable.
For example :
$v_name should be $variable_name
$v_value should be $variable_value
$v_update should be $variable_update

3) As suggested by @shelane, there is inconsistencies in quotes.

4) When I have tried to enable this module, I got this fatal error.

Fatal error: Can't use function return value in write context in D:\xampp\htdocs\drupal7\sites\all\modules\variable_editor\variables.module on line 173

Hi Fabian,

Your module is working fine (Using Drupal 7.38).
Everything if working fine, if you add appropriate comments to your code then it will be easy to understand.

And still found some issues in Parview.sh.

Unable to update Array Variables. After updating it is showing blank array.

Hi Fabian,

Now it is working for Array Variables also.
and no issues found in http://git.drupal.org/sandbox/fabianferns30/2561535.git also.
Changing the status to RTBC.

Hi Fabian,
Thank you for your contribution. I'm sure this one and many other potential contributions will become much appreciated ones.

However, I found a few issues, including security so I had to set status back to Beeds Work.

- This module allows permitted people to "create" new variables. But when you display them, Their names are not sanitized, which makes it possible for users with permission to administer these variables can steal session cookie of someone else, potentially the super admin too.

- Most importantly, please take a look at the variable_editor_form_editor_update/delete functions.

Note that using POST method does not make anything anymore secure. The attacker can still forge a form to send what he needs to send in a form and trick an admin to submit the form, which can be automated with JS. I can see you have used check_plain for security, but what you actually need is a full confirmation form with drupal_get_form (which protects against CSRFs), and do the actual variable changes in the submit handles of each form.

- also, the variable name autocomplete field needs a permission wrapper because presence of a particular variable is useful for an attacker to prep the attack (which modules are in use, etc). As of now, you have "access arguments" set to TRUE, which is wrong (access callback must be set to TRUE if you want to allow everyone to access the router item. But it's not the case in this router).

Adding the security tag for others to find and learn, and for statistical purposes. This wouldn't block your application ones they are fixed. I'm looking forward to review it again!

Hi Fabian,
No issues till now and even security issues also.
Changing status to RTBC.

Are you aware of Variable module?

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Removing review bonus tag, the third review link is the same as the first one?

When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

Hello Fabian,
While reviewing the code, get some points:-

1. In variable_editor_form_editor function, there is function call 'variable_editor_get_variables' without any argument, but in function definition it implemented with one argument.
2. Use checkplain in variable_editor_new_variable_create_form_submit function.
3. In db_query second argument should be an array.

Hello Fabian

Thank you for the module, i did a quick review on the source code
here are few notes

variable_editor.module

variable_editor_permission()
variable permission can be changed to something more comprehensive (like administer all variables) or something like that.

variable_editor_form_editor()
I see dependency on ctools to implement the modal, have you considered a different way to avoid this dependency?

Use of prefix and suffix to add css classes, this can be accomplish through #attributes, please check https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

missing t() use in multiple places https://www.drupal.org/node/1844980

_variable_editor_autosearch()
hardcoded role administrator, consider using user_access() instead

Ill do some more review later and post if i find any more issues .

Hi
It is really a nice light module for variable editor. After install your module I have few things noticed and also did some manual review of your code.

- If someone does not have javascript enabled in browsers then your adding variable does not work. It gives error.(Most of people will have enabled i know but it is always nice to have fallback)

- In function variable_editor_get_variables() you used a lot $_GET variable. You can use drupal_get_query_parameters() function to get GET parameters.

-I saw you create permission string but you created one. i would really like to have several like delete, create, update because you might not allow all user role to add or delete or update variables. Because if you delete any core variable then it is really bad.

- You do not show any confirmation page when u delete or update. It is very important as you do not do any mistake.
- Just wondering what is big difference between devel variable feature where you can edit delete variables and also search for it.

Hopefully it is helpful for you.

Hi Fabian,
I could take a really good look at your modules latest code, but I'm sorry but in my opinion, there are some improvements left. This review process is never supposed to be something to discourage anyone, or to ensure everyone who pass can write perfect code. While I see a lot of use of this module, and believe that this could be really helpful for majority of people, specially now with the Drupal 8's configuration management system.

Below are some of the blocking issues, and followed by some suggestions.

- In Drupal 7, unless the PHP filter module enabled, Drupal core does not make it possible someone to enter arbitrary PHP code and run it. Views, Rules and such other modules make it clear that the user is entering PHP code, so the site administrator can decide who can access those areas (argument validator PHP code, Rules PHP eval, etc). In your module, the add new variable page calls PHP eval(), and anyone with variable permission permission can execute arbitrary PHP code (in a similar fashion that you make an SQL injection). This permission is not marked as a risky one (see hook_permission for details), nor attempted to safeguard it at least. I think the best way would be to allow the user to select the type, or force them to enter a serialized/json string. There is not really much you can do prevent vulnerabilities eval() can introduce.

- When you write some data to the database, it is not necessary to check_plain() them. In fact, it can introduce bugs when you resave the variable. check_plain() is only useful when you want to sanitize something for the browsers. Database layer can take care to sanitize variables. Your database layer usage is otherwise very good (we do not see db_merge, and making great use of db_select often, but you do).

- There is a reference to a missing JS file. Also, please try to not use drupal_add_js/css functions. I can see you have used #attached properties. Ctools modal popups has done an excellent job making sure everything in the form array gets passed to the browser. Simply attach your assets to the #attached property, and it will be present when the form is being shown in JS and non-JS.

Here are some minor suggestions.
- The add new variable permission could be a sub path of the main variable editor UI. That way, the add new variable page will have the administration theme automatically.
- In a minimal installation (most people install the "standard" installation profile), there is no "administrator" role. I can see you have used that to determine some permissions in the autocomplete callback. I would rather use some extra permissions as @takim suggested.

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.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2701619

Project 2: https://www.drupal.org/node/2561547

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

I moved the other issue to your own modules issue queue because it was for an improvement in that module itself. Marking "Active" again.

it's very interesting that you have a 'create variable' function. but i do have to say, its probably not a good idea to allow (even someone with permission) the ability to create one.

Generally it's best practice for a module should to any Vars that is creates (when that module is disabled/uninstalled). therefore, when your module is uninstalled you should be deleted the ones that it created. hook_uninstall()

its also a bit worrisome that you are using an php eval() -- for many reasons.. as your module is letting overrides to all other var this could easily jeopardize whole other security/privacy issues.
in the absolute least you should be you should be buffering it. something like:

ob_start();
eval("echo "This is some really fine output."");
$this_string = ob_get_contents();
ob_end_clean();

i'm not actually sure shy your doing this at all.. but i haven't look very closely. i would general recommend finding a better way of handling this.

• it is throwing an 'Use of undefined constant' error as well..i think because of the eval, but now sure, for sure. (:
• your autosearch is also throwing error
  PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'Array' in 'where clause': SELECT variable_name FROM {variable_user} WHERE variable_name LIKE :string AND user = Array LIMIT 10; Array ( [:string] => %goo% ) in _variable_editor_autosearch() (line 343 of /sites/all/modules/variable_editor/variable_editor.module).
• Notice: Array to string conversion in _variable_editor_autosearch() (line 343 of /sites/all/modules/variable_editor/variable_editor.module).
• you repo is missing the variable_save.js file outright currently.

just a quick example link (sample may not work): stackexchange | create tabs with hook_menu() for custom module

 function variable_editor_editor_form($form, &$form_state){
  $form['actions'] = array('#type' => 'actions');
  $form['actions']['save'] = array(
    '#type' => 'submit',
    '#value' => 'save',
  );
 $form['actions']['delete'] = array(
    '#type' => 'submit',
    '#value' => 'delete',
    '#submit' => 'variable_editor_form_editor_delete_submit',
  );
return form;
}

function variable_editor_form_editor_submit($form, &$form_state){
  // does normal save stuff
}
function variable_editor_form_editor_delete_sumbit($form, &$form_state){
  // does delete stuff
}

sorry, actually you could 'reduce down' the namespace for both sets of forms overall -- it is wordy.. (:
variable_editor_form()
variable_editor_form_submit()
variable_editor_delete_form_submit()
variable_editor_create_form()
variable_editor_create_validate_form()
variable_editor_create_submit_form()

certainly, your project page and read me should clearly mention 'the risk' of this kind of Dev too... as it really is for a 'User 1' kind of dev not an admin or content editor.

ps: the _variable_editor_autosearch() 'admin' user seek seems strange to do. ( i am just visually skimming over it)

--> it's a cool tool my friend. i don't mean to be down on it.

adding security tag.

a follow up thought:
if you were to check for all module implementations of hook_permission and restrict any Var by that module namespace to that permission, it would help reduce some of the greater security access concerns (except for user 1, as would be allow under normal acl standard).

Based on #43 to #46, it looks like this one could do with some more work.

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.