[D7] Data Manipulate UI

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

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've started to give this a bit of a review but first I recommend that you run it through Pareview and work through the warnings and errors that it has detected. You can have a look at the results here: http://pareview.sh/pareview/httpgitdrupalorgsandboxofflein2491739git

Once you've worked through the errors and committed them to your sandbox, you can run it again as often as necessary by clicking the repeat review tab.

EDIT:

I will be delving deeper into this once you've followed up on the PAReview errors but this is my review according to the reviewing template: https://groups.drupal.org/node/427683

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 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
No: See comment re cross site request forgery (CSRF) vulnerability by Ayesh: https://www.drupal.org/node/2492195#comment-9955719

Coding style & Drupal API usage
No: Fails PAReview

Cheers,
Andrew

Oh, you'll need to adjust your info file to tell it add a dependency for jquery_update.

Hi there,
Thanks for submitting the application. I don't use the Data module often (never came across a need), but this project had me wonder why there isn't such a module to do this! Well, here we are with a new project that provides a neat UI to do that.

Unfortunately, this module does not protect itself against cross site request forgery (CSRF). It's a straight forward exploit, and requires to trick someone with data_manipulate_ui_access permission passing. Note that the attacker can trick him to follow certain action that he didn't intend. This can be done with Javascript too (with same-origin policies bypassed).

<form action="http://dev/work/applications/ajax/data_manipulate_ui/test" method="post"> 
<input type='hidden' name='{"row":{"test":69},"id":null, "escape": "' value='escaped"}' />
<input type="submit" value="This is completely harmless. Trust me" />
</form>

Let me take you through this:
First, we create a fake form with method=post, so we are essentially trying to add a row to the test table. Notice the action URL.

Now, we need to pass a valid json string, to be decoded server side. Module expects the row to be there, and we feed all the data we need into the test. Since forms follow the normal HTTP query parameters, we leave an open quote (notice the "escape": "), so the equal sign added by the browser gets ignored in the json_decode operation.

At the server side, we end up with an array that has 3 keys: row (= array(test => 69)), id (= null), and escape (= '=escaped').

Since this input field is a hidden one, the administrator we are tricking into doesn't notice this without checking the actual HTML. You can change the appearance of the Submit button to look completely innocent. It's also possible to submit the form with Javascript; on document.ready even.

In cases like this, we definitely need to validate the form with an anti-CSRF token. It is possible to administrator to visit some page, and load the form. But browsers forbid that with the same-origin policy. Notice how the link to enable a theme works. It has a session-dependent token that can't be guessed by outsiders.

In your module, you can add a CSRF token to be passed along with the request, and validate it at the server side. drupal_get_token() and drupal_valid_token() are great candidates.

Thanks Craig. Yep, the CSRF handling looks perfect now!

The pareview complains about the module name is valid for some extent though.

views module has views, views_ui. Rules module has rules, rules_admin, etc. Usually the base module name is prefixed by sub modules.
This is purely a question: Is there any reason why you didn't use data_manipulate_ui_access for the module name? That is what pareview is expecting, and makes more sense to me too, because this module is only restricting access to the UI (of data_manipulate_ui module). However, I don't think this is a major issue at all. VBO for example, provides two modules with completely different names.

If your module is ready to be reviewed again, feel free to set its status to "Needs review". As always, Review Bonus can help speed things up.

Hello

Mannual review
1. Please fix pareview.sh issues http://pareview.sh/pareview/httpgitdrupalorgsandboxofflein2491739git
2. All functions should be prefixed with your module name.

Please fix following issues:

js/data_manipulate_ui.js: line 6, col 2, Error - Use the function form of "use strict". (strict)
js/data_manipulate_ui.js: line 14, col 36, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 17, col 36, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 29, col 74, Error - Properties shouldn't be quoted as all quotes are redundant. (quote-props)
js/data_manipulate_ui.js: line 33, col 36, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 36, col 34, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 38, col 13, Error - "id" is not defined. (no-undef)
js/data_manipulate_ui.js: line 38, col 18, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 39, col 13, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 39, col 43, Error - "id" is not defined. (no-undef)
js/data_manipulate_ui.js: line 44, col 9, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 45, col 9, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 52, col 45, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 58, col 9, Error - Move function declaration to function body root. (no-inner-declarations)
js/data_manipulate_ui.js: line 59, col 22, Error - Properties shouldn't be quoted as all quotes are redundant. (quote-props)
js/data_manipulate_ui.js: line 64, col 15, Error - data is defined but never used (no-unused-vars)
js/data_manipulate_ui.js: line 72, col 31, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 73, col 17, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 75, col 31, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 84, col 9, Error - Move function declaration to function body root. (no-inner-declarations)
js/data_manipulate_ui.js: line 87, col 15, Error - rowUniqueId is defined but never used (no-unused-vars)
js/data_manipulate_ui.js: line 90, col 21, Error - Expected '===' and instead saw '=='. (eqeqeq)
js/data_manipulate_ui.js: line 95, col 22, Error - Properties shouldn't be quoted as all quotes are redundant. (quote-props)
js/data_manipulate_ui.js: line 101, col 15, Error - data is defined but never used (no-unused-vars)
js/data_manipulate_ui.js: line 109, col 31, Error - Missing space before function parentheses. (space-before-function-paren)
js/data_manipulate_ui.js: line 110, col 17, Error - "$jqxGrid" is not defined. (no-undef)
js/data_manipulate_ui.js: line 112, col 31, Error - Missing space before function parentheses. (space-before-function-paren)

Looks interesting, and I need to look at this in more depth.

I'll say now though that I think that having access control in a separate and optional module is not a good idea.

That sounds like a design decision at the discretion of the module author and should not be an application blocker unless it results in a security vulnerability.

> That sounds like a design decision at the discretion of the module author and should not be an application blocker unless it results in a security vulnerability.

True, I was a bit too hasty there in assuming that without the submodule, there was no access control at all.

However, that submodule does have design flaws. It appears to store a table of user UIDs and table names, as a record of who can edit which table. That seems to me to just be reinventing the role system. It would be equivalent to define a permission for each table, and then let admins assign roles with those permissions to users.

  else {
    return t('Unable to load jQWidgets library!');
  }

If the jQWidgets library is a requirement, use hook_requirements() rather than checking for it here.

data_manipulate_ui_get_data() loads all the data in a table in an AJAX response. That's not going to scale.

> function data_manipulate_ui_access($table_name, $account = NULL) {

Docs are missing a parameter here.