String sanitation for forms and serializers #1395

oliverroick · 2017-04-10T10:54:30Z

Proposed changes in this pull request

Fixes Input fields should be sanitized to exclude code #1157: Reject all strings that either contain HTML or start with +, -, + or @ to prevent injection of potentially malicious macros.
Fixes Critical: Emojis should not be valid Org or Project names #1224: Reject all strings that contain emojis.

Detailed discussion of added features

Adds function sanitize_string to core.validators which is used throughout all forms and serializers to verify if a string will be accepted or not. The function uses the library beautifulsoup to identify HTML code, and two regular expressions to identify emojis and starting +, -, + or @. The function returns False if the provided string matches any of the regular expressions or contains HTML.
Adds sanitize.js to cadasta/core/static/js, which introduces a new Parsley validator to sanitise fields on the client-side.
Adds the template filter set_parsley_sanitize to core.templatetags.filters, which adds the attribute data-parsley-sanitize to all form fields where the filter is applied. The additional attribute indicates to Parsley that this field should be validated using the validator added through sanitize.js.
Adds a new form mixing SanitizeFieldsForm to core.form_mixins that checks the values or all CharField instances using core.validators.santize_string. The mixin is added to all forms where we allow input of strings.
Adds a new serializer mixin SanitizeFieldSerializer to core.serializers that checks the values of all CharField instances and all items in all JSONField instances using core.validators.santize_string. The mixin is added to all serializers where we allow input of strings.
Adds a new serializer mixin JSONAttrsSerializer to core.serializers that validates contents of the field attributes using jsonattrs. jsonattrs validation wasn't implemented as of yet which allowed the injection of invalid values to be added via the API, which caused the platform to break in some instances.

When should this PR be merged

This is a critical fix, so it should be merged as soon as possible.

Risks

The PR itself is low risk; we should test the changes thoroughly on staging because it affects most parts of the platform.

Follow-up actions

@oliverroick to document form and serializer mixins in the dev wiki. (Draft done; documentation for forms and serializers)

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly? It should have the migration label if a new migration is added.
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

seav · 2017-04-10T11:22:12Z

cadasta/core/static/js/sanitize.js

+    var macros = /^[-=+@]/;
+    return !value.match(emojis) && !value.match(macros);
+  }, 2)
+  .addMessage('sanitize', gettext('Input can not contain HTML tags, emojis or start with any of + - = and @.'));


The message here indicates that HTML tags are not allowed, but the validator does not contain any logic to check for HTML tags.

@oliverroick: Is this feedback invalid?

It's not. I totally missed replying to this, thanks for reminding me again.

I've added a check for HTML to sanitize.js as well.

linzjax · 2017-04-12T16:20:04Z

I know there was some conversation about this in the issue, but did we decide emoji's in the project name was ok?

I also got a "Please provide a name" error when I added an emoji to the contact fields, rather than an error explaining that "there's no way my name does not contain an emoji so stop messing around".

oliverroick · 2017-04-12T16:58:16Z

@linzjax Crap it looks like the regex I wrote doesn't cover unicorns, I'll look into that tomorrow. Also need to look at the names thing.

linzjax · 2017-04-12T17:51:46Z

I know apple added a ridiculous new set of emojis and fancied up old ones that aren't included in the standard emoji set.

Also,

Crap it looks like the regex I wrote doesn't cover unicorns

is a hilarious sentence.

oliverroick · 2017-04-19T08:44:07Z

@linzjax I added new regex definitions that should also cover unicorns.

linzjax

Don't mind me just coming through trying to break things 🙅 :

Two things weird about this:

I can search for emojis and it shows up in the url
It returned a document (which I didn't know was possible on my local machine) for a document that I tried to name "👩‍🎓" and it wouldn't let me.

linzjax · 2017-04-19T21:26:13Z

I can also import emojis and super useless html.

oliverroick · 2017-04-21T08:54:44Z

@linzjax Sanitizing imports now too

linzjax · 2017-04-21T14:28:11Z

cadasta/organization/importers/base.py

@@ -273,6 +275,7 @@ def _map_attrs_to_content_types(self, headers, row, content_types,
                    continue
                if selector in ['DEFAULT', content_type.get('type', '')]:
                    for attr in attrs:
+                        print(attrs)


wayward print sighting

linzjax

Ok besides the print statement, one more question: have you run this against ODK submissions? My Test phone is dead and apparently I didn't bring an android charger with me, but I don't see anything is xforms/mixins/model_helper.py

oliverroick · 2017-04-24T09:05:30Z

@linzjax Of course GeoODK submissions were not sanitized, but they should be now.

linzjax

Al➡️, that covers everything I can 🤔 of, which means I'm done 🐝ing a pain in your 🐴 now.

wonderchook · 2017-05-05T22:26:24Z

@seav can you finish reviewing this please?

seav

Did some tests and this looks good to me.

amplifi · 2017-05-25T14:51:30Z

Organization form validates tag inputs:

amplifi · 2017-05-25T15:09:26Z

User registration form validates emoji and tags, including saving for full name (same also allowed on editing user profile):

amplifi · 2017-05-25T15:31:08Z

When adding a new project or editing project details, phone number field doesn't display error when invalid string is submitted. Just refreshes form page without visual cues or error message:

amplifi · 2017-05-25T15:41:02Z

Add location form, edit location relationship form, add resource form validate with tags:

seav · 2017-05-25T15:57:27Z

cadasta/core/static/js/sanitize.js

+  .addValidator('sanitize', function (value, requirement) {
+    function isHTML(str) {
+        const doc = new DOMParser().parseFromString(str, "text/html");
+        return Array.from(doc.body.childNodes).some(function(node) {return node.nodeType === 1});


This JavaScript code apparently does not properly check for <script> and <style> tags. We should also do the conditional checks below. If any of them is true, then the string is not sanitized.

doc.styleSheets.length > 0

doc.scripts.length > 0

amplifi · 2017-05-25T16:00:52Z

On submission, Edit resource form shows green validation for fields containing tags (Name & Description), then after form reloads it shows the red error cues.

bjohare · 2017-05-25T16:55:20Z

We should probably be sanitizing questionnaires as well..

amplifi · 2017-05-25T18:37:12Z

We should consolidate any additional fixes/sanitization into a new GitHub Issue (or Issues). As long as there are no breaking changes, and I haven't found any yet, this PR has to merge for the upcoming release. Better to have most inputs validated than none.

oliverroick · 2017-05-26T09:22:32Z

I added client-side code to catch script and style elements. Also added code to sanitize user registration and profile updates.

@bjohare Questionnaires should be sanitized, could you send me the form you used?

bjohare · 2017-05-26T09:35:01Z

@oliverroick I just used the standard questionnaire with an emoji added to the party_type label

linzjax · 2017-06-01T15:40:32Z

When signing in, I can submit fields with emojis. I can't sign in, but they are submitted.

If I go through the "password reset" process, I can reset my password to contain an emoji.

Similar to the sign in form, the "add member" form submits with emojis.

Organization and Project URL fields accept emojis and script tags

I can't add resource with a filename that contains an emoji, but I also don't get an error

dpalomino · 2017-06-01T17:22:43Z

Some feedback, uploading questionnaires when creating projects:

Not sure if this is an issue, but I can upload a questionnaire containing an emoji, although at the end of the project creation process (step 3, user permissions), project is rejected. Tried including emojis in labels, names, types, choices, settings
You can include an emoji in a column name of a questionnaire (I tested with "tittle" column in settings tab). Interestingly:
- If included in the "tittle" column of the settings tab, it is accepted. Example here
- If included in the "name" column of the survey tab, it is not accepted (but you are able to upload)
I tried to change the tab name to include an emoji, and was rejected but not because we were filtering emojis, but because there were missing information:

For scripts:

Same behaviour. You can generally upload a questionnaire containing script chars (I tried with <script>), although at the end of the project creation process (step 3, user permissions), project is rejected. Tried including script code in labels, names, types, choices, settings
Interestingly, including script in the "hint" column (hint<script>asdf</script>)is accepted and you can actually create a project, see here.
Same behaviour for title column in settings tab, was able to include "title<script>ha!</script>" and project was created (check here)

dpalomino · 2017-06-01T18:01:49Z

Regarding search:

I can include emojis in the search:
And the same for the script code:

amplifi · 2017-06-01T19:01:48Z

Search is fine; input isn't stored and Elasticsearch has its own query validation. Same for login.

The main concerns are strings that are stored in the database.

dpalomino · 2017-06-05T14:19:45Z

I've created individual tickets #1558, #1559, and #1560 for tracking these issues.

oliverroick added PR: bugfix PR: needs review PR: needs testing priority: high labels Apr 10, 2017

oliverroick requested review from seav and linzjax April 10, 2017 10:54

seav reviewed Apr 10, 2017

View reviewed changes

linzjax suggested changes Apr 19, 2017

View reviewed changes

linzjax reviewed Apr 21, 2017

View reviewed changes

linzjax approved these changes Apr 26, 2017

View reviewed changes

seav approved these changes May 8, 2017

View reviewed changes

oliverroick force-pushed the bugfix/#1157 branch from 64760ba to 16403b7 Compare May 8, 2017 13:55

oliverroick added PR: approved and removed PR: needs review PR: needs testing labels May 8, 2017

amplifi added the PR: needs testing label May 17, 2017

seav reviewed May 25, 2017

View reviewed changes

oliverroick added 3 commits May 26, 2017 11:28

String sanitation for forms and serializers

6722ff5

Catch script and style elements

a22dc2a

Add sanitation to user signup and profile update

e1cce78

oliverroick force-pushed the bugfix/#1157 branch from 34ca1cb to e1cce78 Compare May 26, 2017 09:29

oliverroick added 3 commits May 26, 2017 12:15

Fix tests after rebase

5a50da5

Sanitize multi-language labels

467bfbf

Fix client-side validation for imports

cbd8a6d

amplifi merged commit f2e6917 into master May 30, 2017

amplifi deleted the bugfix/#1157 branch May 30, 2017 09:15

This was referenced Jun 5, 2017

String Sanitation - Password reset #1558

Closed

String Sanitation - Organization and Project URLs accepts emojis #1559

Closed

String Sanitation - Uploading questionnaires with emojis or script code #1560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

String sanitation for forms and serializers #1395

String sanitation for forms and serializers #1395

oliverroick commented Apr 10, 2017 •

edited

seav Apr 10, 2017

seav Apr 28, 2017

oliverroick May 2, 2017

linzjax commented Apr 12, 2017

oliverroick commented Apr 12, 2017

linzjax commented Apr 12, 2017 •

edited

oliverroick commented Apr 19, 2017

linzjax left a comment •

edited

linzjax commented Apr 19, 2017 •

edited

oliverroick commented Apr 21, 2017

linzjax Apr 21, 2017

linzjax left a comment

oliverroick commented Apr 24, 2017

linzjax left a comment

wonderchook commented May 5, 2017

seav left a comment

amplifi commented May 25, 2017

amplifi commented May 25, 2017 •

edited

amplifi commented May 25, 2017 •

edited

amplifi commented May 25, 2017 •

edited

seav May 25, 2017

amplifi commented May 25, 2017

bjohare commented May 25, 2017

amplifi commented May 25, 2017

oliverroick commented May 26, 2017

bjohare commented May 26, 2017

linzjax commented Jun 1, 2017

dpalomino commented Jun 1, 2017

dpalomino commented Jun 1, 2017

amplifi commented Jun 1, 2017

dpalomino commented Jun 5, 2017

String sanitation for forms and serializers #1395

String sanitation for forms and serializers #1395

Conversation

oliverroick commented Apr 10, 2017 • edited

Proposed changes in this pull request

Detailed discussion of added features

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

seav Apr 10, 2017

Choose a reason for hiding this comment

seav Apr 28, 2017

Choose a reason for hiding this comment

oliverroick May 2, 2017

Choose a reason for hiding this comment

linzjax commented Apr 12, 2017

oliverroick commented Apr 12, 2017

linzjax commented Apr 12, 2017 • edited

oliverroick commented Apr 19, 2017

linzjax left a comment • edited

Choose a reason for hiding this comment

linzjax commented Apr 19, 2017 • edited

oliverroick commented Apr 21, 2017

linzjax Apr 21, 2017

Choose a reason for hiding this comment

linzjax left a comment

Choose a reason for hiding this comment

oliverroick commented Apr 24, 2017

linzjax left a comment

Choose a reason for hiding this comment

wonderchook commented May 5, 2017

seav left a comment

Choose a reason for hiding this comment

amplifi commented May 25, 2017

amplifi commented May 25, 2017 • edited

amplifi commented May 25, 2017 • edited

amplifi commented May 25, 2017 • edited

seav May 25, 2017

Choose a reason for hiding this comment

amplifi commented May 25, 2017

bjohare commented May 25, 2017

amplifi commented May 25, 2017

oliverroick commented May 26, 2017

bjohare commented May 26, 2017

linzjax commented Jun 1, 2017

dpalomino commented Jun 1, 2017

dpalomino commented Jun 1, 2017

amplifi commented Jun 1, 2017

dpalomino commented Jun 5, 2017

oliverroick commented Apr 10, 2017 •

edited

linzjax commented Apr 12, 2017 •

edited

linzjax left a comment •

edited

linzjax commented Apr 19, 2017 •

edited

amplifi commented May 25, 2017 •

edited

amplifi commented May 25, 2017 •

edited

amplifi commented May 25, 2017 •

edited