[D7] JSON Filter

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

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

No pareview errors found

Manual Review

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.

[No: Does not follow] the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements.

This review uses the Project Application Review Template.

Hi Yaron,
I went through your code and found some issues.

---

You've done a great job addressing every single best practice, style and documentation issues. The automated reviews are not, however, testing your module for unit tests, etc.

My first test case is with the following JSON string.

$data = array(
  array('Ayesh'),
  array(23),
);
$str = json_encode($data);

This triggers a PHP error with foreach, because the module's recurring logic makes it call itself for integer and other scalar data types, which foreach() cannot be called on.

I don't think gettype is not suitable for this in first place. is_scalar() would be more appropriate and straight forward. Even so, you will need extra processing for Boolean and object types. Boolean values cannot be printed (needs to be replaced with "false" or "true" to make them visible to end users). Objects cannot be iterated with foreach, so you will need to cast it to an array.

---

Most importantly, your code unfortunately raises a security issue.

Following is the test case:

{"oh no":"<script>alert(\"Oh no!\");<\/script>"}

Note that I have configured the "Text format" of my text area field to "Plain text", and even after that, JSON filter module did not sanitize the script tag above, and browser executed the alert code.

If you want to allow HTML, the solution would be to run the specified text formatter on each element. (See check_markup() function). This can be expensive so do some tests first.
A better solution would be call check_plain() on each text that you are printing. This will sanitize everything to safe plain text.
For your reference, I'm attaching a patch with this changed in your current git HEAD.

Since this module has a security issue, I'm adding the security tag. Please do not remove it even if you fix the security issue. It will help future applicants to learn.

---

Also, json_filter_obj_to_table() function calls json_filter_obj_to_ulli() function when recurring. I'm not sure it's intended. It's perfectly valid HTML even if you have nested HTML tables. Usability will be degraded, but so are deeply nested lists.

---

It's not a field formatter's job to validate text, but I would go the extra mile to add a validation to validate incoming data to be valid JSON (Easily possible with hook_field_attach_validate())

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.