[#8779] AccountAttributes: Remove HTML sanitization before saving for googleId, name, institute #9368

ChooJeremy · 2019-01-25T08:04:49Z

Outline of Solution

Basically just removed the sanitization in AccountAttributes and updated tests. I have tested with single quotes in the email (as per #8774) and it works.

I'm not sure if I need to write a migration script, let me know if I need to. Based on my checks, if there was something to be migrated, it wouldn't have made it to the database anyway because the application would have just rejected it.

Ready for review.

xpdavid

Yes, we might not need a script.

xpdavid · 2019-01-26T06:22:33Z

src/main/java/teammates/common/datatransfer/attributes/AccountAttributes.java

@@ -181,9 +181,9 @@ public String getJsonString() {

    @Override
    public void sanitizeForSaving() {
-        this.googleId = SanitizationHelper.sanitizeForHtml(googleId);


Should use SanitizationHelper.sanitizeGoogleId?

xpdavid · 2019-01-26T06:22:40Z

src/main/java/teammates/common/datatransfer/attributes/AccountAttributes.java

@@ -181,9 +181,9 @@ public String getJsonString() {

    @Override
    public void sanitizeForSaving() {
-        this.googleId = SanitizationHelper.sanitizeForHtml(googleId);
-        this.name = SanitizationHelper.sanitizeForHtml(name);


SanitizationHelper.sanitizeName?

xpdavid · 2019-01-26T06:23:21Z

src/main/java/teammates/common/datatransfer/attributes/AccountAttributes.java

@@ -181,9 +181,9 @@ public String getJsonString() {

    @Override
    public void sanitizeForSaving() {
-        this.googleId = SanitizationHelper.sanitizeForHtml(googleId);
-        this.name = SanitizationHelper.sanitizeForHtml(name);
-        this.institute = SanitizationHelper.sanitizeForHtml(institute);


SanitizationHelper.sanitizeName maybe?

ChooJeremy · 2019-01-26T11:11:42Z

Done, ready for review.

I used SanitizationHelper.sanitizeTitle for institute sanitization (based on AccountAttributes's Builder build() method.)

xpdavid · 2019-01-26T13:28:17Z

package.json

@@ -44,7 +44,7 @@
  },
  "devDependencies": {
    "@angular-builders/jest": "~7.1.2",
-    "@angular-devkit/build-angular": "~0.11.4",


Not relevant change?

xpdavid · 2019-01-26T13:31:08Z

src/test/java/teammates/test/cases/datatransfer/AccountAttributesTest.java

@@ -95,9 +94,9 @@ public void testSanitizeForSaving() {
        AccountAttributes expectedAccount = createAccountAttributesToSanitize();
        actualAccount.sanitizeForSaving();

-        assertEquals(SanitizationHelper.sanitizeForHtml(expectedAccount.googleId), actualAccount.googleId);


So your tests should also reflect the changes?

e.g. Having some values that need to be sanitised and test whether the actualAccount.sanitizeForSaving(); work or not?

ChooJeremy · 2019-01-26T17:48:30Z

Sorry, forgot about the tests. Interestingly, this means that the tests would have passed no matter what, because the Builder in AccountAttributes sanitized the results when building the object.

I've changed the email test to add the single quote and added whitespace around all the values since the sanitization code is effectively just trimming the values.

Ready for review.

xpdavid · 2019-01-27T09:16:15Z

src/main/java/teammates/common/datatransfer/attributes/AccountAttributes.java

@@ -92,6 +92,10 @@ public AccountAttributes build() {
            return accountAttributes;
        }

+        public AccountAttributes buildWithoutSanitizing() {


I don't think it is a good practice to introduce this method in production code just for testing.

xpdavid · 2019-01-27T09:19:06Z

src/test/java/teammates/test/cases/datatransfer/AccountAttributesTest.java

-        assertEquals(SanitizationHelper.sanitizeForHtml(expectedAccount.name), actualAccount.name);
-        assertEquals(SanitizationHelper.sanitizeForHtml(expectedAccount.institute), actualAccount.institute);
+        assertEquals(SanitizationHelper.sanitizeGoogleId(expectedAccount.googleId), actualAccount.googleId);
+        assertEquals(SanitizationHelper.sanitizeName(expectedAccount.name), actualAccount.name);


Interestingly, this means that the tests would have passed no matter what, because the Builder in AccountAttributes sanitized the results when building the object.

If that is the case. Maybe we can just do assertEquals(expectedAccount.name, actualAccount.name)

I'm not quite sure I understand how that solves the problem. Since build sanitizes all the fields, both expectedAccount and actualAccount will have the same values. Checking equality would only verify that running sanitizeForSaving on sanitized values doesn't change the values at all.

Perhaps the method createAccountAttributesToSanitize could just modify the values directly? So something like:

private AccountAttributes createAccountAttributesToSanitize() { AccountAttributes unsanitizedAttributes = AccountAttributes.builder().build(); unsanitizedAttributes.googleId = " google'Id@gmail.com\t"; unsanitizedAttributes.name = "'name'\n\n"; unsanitizedAttributes.institute = "\\\t\n/"; unsanitizedAttributes.email = "&<email>&\n"; unsanitizedAttributes.isInstructor = true; return unsanitizedAttributes; }

Cool. We can do that.

…tly instead

ChooJeremy · 2019-01-27T14:57:28Z

Ok, ready for review.

xpdavid

Some comment on test data selection.

xpdavid · 2019-01-28T14:52:39Z

src/test/java/teammates/test/cases/datatransfer/AccountAttributesTest.java

+        unsanitizedAttributes.googleId = "    google'Id@gmail.com\t";
+        unsanitizedAttributes.name = "'name'\n\n";
+        unsanitizedAttributes.institute = "\\\t\n/";
+        unsanitizedAttributes.email = "&<email>&\n";


I think we should change this to a valid email address.

xpdavid · 2019-01-28T14:54:13Z

src/test/java/teammates/test/cases/datatransfer/AccountAttributesTest.java

-
+        AccountAttributes unsanitizedAttributes = AccountAttributes.builder().build();
+        unsanitizedAttributes.googleId = "    google'Id@gmail.com\t";
+        unsanitizedAttributes.name = "'name'\n\n";


There should be some extra space inside name in order to make sure sanitizeName is doing the job? 'name'\n\n?

xpdavid · 2019-01-28T14:54:33Z

src/test/java/teammates/test/cases/datatransfer/AccountAttributesTest.java

+        AccountAttributes unsanitizedAttributes = AccountAttributes.builder().build();
+        unsanitizedAttributes.googleId = "    google'Id@gmail.com\t";
+        unsanitizedAttributes.name = "'name'\n\n";
+        unsanitizedAttributes.institute = "\\\t\n/";


Same for institute.

ChooJeremy · 2019-01-28T16:05:01Z

Ok, test data modified, whitespace added to the middle of the strings.

Ready for review.

xpdavid

LGTM!

ChooJeremy added 5 commits January 25, 2019 13:59

Removed sanitization and updated tests

49d241e

Merge branch 'master' into account-sanitization

f33aba2

Optimized imports

bc10e2d

Fixed static analysis

093693a

Reverted changes to AccountAttributes Builder's build

94ee02d

xpdavid reviewed Jan 26, 2019

View reviewed changes

xpdavid self-assigned this Jan 26, 2019

xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Jan 26, 2019

Updated sanitization methods

34152b7

xpdavid reviewed Jan 26, 2019

View reviewed changes

ChooJeremy added 4 commits January 26, 2019 23:23

Updated tests

63f35e5

Revert package.json change

b1453e3

Revert change from ~ to ^

d060a7c

Added email sanitization

3c98a9b

xpdavid reviewed Jan 27, 2019

View reviewed changes

ChooJeremy added 4 commits January 27, 2019 20:05

Changed Account Attribute's tests sanitization to modify values direc…

63a80e6

…tly instead

Revert removal of newline

3128a36

Merge branch 'master' into account-sanitization

c4dc60b

Removed trailing whitespace

6405ab1

xpdavid reviewed Jan 28, 2019

View reviewed changes

ChooJeremy added 2 commits January 28, 2019 23:36

Updated test data selection

ff4462c

Merge branch 'master' into account-sanitization

f919b09

xpdavid approved these changes Jan 29, 2019

View reviewed changes

xpdavid requested a review from damithc January 29, 2019 06:23

xpdavid added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Jan 29, 2019

damithc approved these changes Jan 29, 2019

View reviewed changes

Merge branch 'master' into account-sanitization

60be1fe

xpdavid merged commit 1ba05cc into TEAMMATES:master Jan 29, 2019

xpdavid added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests and removed s.FinalReview The PR is ready for final review labels Jan 29, 2019

xpdavid mentioned this pull request Jan 29, 2019

[#8779] AccountAttributes: remove HTML sanitization before saving for googleId #8786

Closed

wkurniawan07 added this to the V7.0.0-beta.0 milestone Feb 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#8779] AccountAttributes: Remove HTML sanitization before saving for googleId, name, institute #9368

[#8779] AccountAttributes: Remove HTML sanitization before saving for googleId, name, institute #9368

ChooJeremy commented Jan 25, 2019

xpdavid left a comment

xpdavid Jan 26, 2019

xpdavid Jan 26, 2019

xpdavid Jan 26, 2019

ChooJeremy commented Jan 26, 2019

xpdavid Jan 26, 2019

xpdavid Jan 26, 2019

ChooJeremy commented Jan 26, 2019

xpdavid Jan 27, 2019

xpdavid Jan 27, 2019

ChooJeremy Jan 27, 2019 •

edited

xpdavid Jan 27, 2019

ChooJeremy commented Jan 27, 2019

xpdavid left a comment

xpdavid Jan 28, 2019

xpdavid Jan 28, 2019

xpdavid Jan 28, 2019

ChooJeremy commented Jan 28, 2019

xpdavid left a comment

[#8779] AccountAttributes: Remove HTML sanitization before saving for googleId, name, institute #9368

[#8779] AccountAttributes: Remove HTML sanitization before saving for googleId, name, institute #9368

Conversation

ChooJeremy commented Jan 25, 2019

xpdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Jan 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Jan 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy Jan 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Jan 27, 2019

xpdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChooJeremy commented Jan 28, 2019

xpdavid left a comment

Choose a reason for hiding this comment

ChooJeremy Jan 27, 2019 •

edited