[D7] Angular js example

See https://www.drupal.org/node/1011698 - this issue should be in the project applications queue.

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

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.

The code seems good, I've installed it, and it works well.
But even though this is just an example module, some good practices are not respected.

angular_js_example_help :

• - You should add t() function to translate the help text :
  $output = t("This is simple example of angular js. How we can add records in db and can show through angular js.");

angular_js_example_menu :

• Add t() function to description and title menu items (in order to translate)

angularjs-form.tpl.php :

• Add t() function to your labels
• The code indentation and the spacings are very strange. You should do a break line and indent after a TR tag.
• The value of the "colspan" attribute in TD should be wrapped with double quotes

Regards.

angular_js_example_menu :

• Add t() function to description and title menu items (in order to translate)
• Sorry, this is a mistake

The Drupal 6 and 7 menu system stores menu item titles and descriptions in English. This allows the system to cache the data, but display to users using various languages on demand. For this to work, you should not use t() on the title or description of menu items in your hook_menu() implementation.

angular_js_example_menu :

• Add t() function to description and title menu items (in order to translate)
• Sorry, this is a mistake

The Drupal 6 and 7 menu system stores menu item titles and descriptions in English. This allows the system to cache the data, but display to users using various languages on demand. For this to work, you should not use t() on the title or description of menu items in your hook_menu() implementation.

Manual Review:

1. You should consider adding a description to the module page.
2. You have provided the access callback as "TRUE". So it can be accessible by an anonymous user too. Is that what you feel right?
3. You can use url() instead of $base_url
4. You have not filtered the $_POST data before inserting in DB. This might be a security issue
5. You can also feel free to create an issue in Examples Module to add this as a feature.

You have not filtered the $_POST data before inserting in DB. This might be a security issue.

Saving $_POST data is not a problem by itself, since Drupal stores user provided data unaltered. "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

But we have 2 other related security problems here, can you name those? As part of our git admin training I'm assigning this to joachim so he can take a look. If he does not find anything within a week I'm going to post the vulnerability details.

Hello,

Thanks for your module, I was looking for an example too to try Angular JS.

My question is, why not using the module 'angularjs' as dependency ? It could load the angularjs lib properly instead of hardcoding the path to the js in the block content array ?

Anyway, I made a small patch to fix some stuff, I don't really know if it fixes the two security issues, I haven't seen them.

> This means that conversions are performed when content is output, not when saved to the database

AFAICT, that's not happening. There are two places where data retrieved from the database is output with print json_encode() in page callbacks. I'm not a JS expert (and the messy indentation in the JS file makes it harder to read), but I don't think the JS that retrieves that data is doing any sanitizing of its own.

So while it's ok to put whatever the user gives you into the database verbatim, you need to sanitize it before you output it anywhere. All the fields from the query should be run through check_plain().

A few other things:

- $schema['books'] = array(

Prefix table names with your module name to avoid clashes.

- The indentation in the TPL file needs fixing as well as in the JS file.

Hello,

Once again, why not using the module angularjs as dependency ?

Thanks for replying.

Ok thanks!

If you have made all the essential changes suggested. then change the status to "Needs Review" and also try to get some "Review Bonus" to speed up the process.

You should do a minimum of three manual reviews of other projects inorder to add a review bonus tag. But you have mentioned only two reviews in the issue summary.

Please add one more review and add the review bonus tag.

@himmatbhatia: Sorry. But the latest reviews you did were just pasting the automated reviews. Actually in order to get the review bonus, you have to do a manual review of other projects. Because PA robot already mentioned about the automated review result in all the applications. If you spent some time to review other projects manually others would do it back to you and your code would meet the standards.

Here is a template that you can use to make sure you cover all of the necessary points in your review:
https://groups.drupal.org/node/427683

Anyone with the "access content" permission can save book data. I think this is a very bad example security-wise and not a realistic scenario, so you should use a proper content creator permission for that.

angular_js_example_form_data_save() bypasses the form API and directly saves data from $_POST. Security quiz: what is the name of that security vulnerability and why is it a problem?

@klausi: Form API will sanitize the text using check_plain() by default. If form API is not used, we have to apply check_plain() manually before inserting into db. Is that right?

Nope, that is not right, XSS is not the problem here. As I said: "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 check_plain()s in angular_js_example_form_data_save() are wrong and should be removed.

There is a different vulnerability here, keep on searching.

Cool, and there is still a security vulnerability present. Leaving this assigned to joachim so that he can take a look if he has time. Otherwise I will post the vulnerability details in one week.

@klausi: Instead of filtering using check_plain() before insetring into DB, should we use that in template (angularjs-form.tpl.php) before listing all the books created?

@himmatbhatia: Please don't do any changes until kalusi approves it.

angular_js_example_form_data_save(): this menu callback is vulnerable to CSRF exploits. Data from POST requests that is written to the database needs to be CSRF protected with the form API or CSRF tokens, see http://epiqo.com/en/all-your-pants-are-danger-csrf-explained or https://docs.acquia.com/articles/protecting-your-drupal-module-against-c...

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

Multiple Applications

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

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

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

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.

Leaving this application open instead of the other one.

angular_js_example_form_data_save() does not check the CSRF token? So the CSRF vulnerability is still there.

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

Hi,

Please fix below errors:

1. angular_js_example.module : line number -96 : Array closing indentation error, expected 6 spaces but found 4.
2. Try to use only hooks in module file and any other function should be kept in separate file, it is just my recommendation not an error.
3. It is very specific to a previous version of angular js, please mention that in ReadME.txt.

Thanks,

@manish.upadhyay: those are good tips for improvements, but surely not application blockers. Anything else that you found or should this be RTBC instead?

The module looks good, it works well.

Some observations/suggestion:
1. You should have to check on db_insert query fails or success. You can use below try catch code to do that:
try {
db_insert('...')->execute();
// No exception thrown; PDO thinks the record was inserted correctly.
}
catch (PDOException $e) {
// Query failed; redirect or show user custom message by using $e->getMessage()
}

2. bookprice and authorid fields should be integer instead of varchar
3. Check for blank fields while inserting to database (Validation on form)

Then it is ready for approval, I can't see any other (blocking) issue here.

@himmatbhatia - You can now promote this to a full project yourself.

@himmatbhatia - in order to make your project available for download, you need to tag a release. I'm not sure how to do that, but there is documentation on drupal.org that will guide you how.

As for security coverage, you need to complete the review process here, and request security coverage.

It has been marked RTBC, so that's a good sign.

You should conduct at least 3 manual reviews of other projects in order to get the review bonus, that means your project will get reviewed by one of the Git admins.

Thank you for your contribution!

I updated your account so you can opt into security advisory coverage now.

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.

Thank the dedicated reviewer(s) as well.

Assigning credits.