Improve extensions + submissions autocomplete

[damianhxy]

Improve correctness of extensions + submissions autocomplete, and extract common logic.
Also fix an XSS issue in gradebook, and various visual display issues.

Description

• CGI.escapeHTML on backend
• escape_javascript on frontend
• remove sanitize from flash
• replace encode64 with strict_encode64 to avoid line feeds
• extracted common backend logic to ApplicationController
• extracted common frontend logic to components/_autocomplete.html.erb
• use to_csv for download_roster

Motivation and Context

Currently, there are several issues with autocomplete, including

• XSS in name / email
• Disappearing letters when backslashes exist in name
• base64 breaking on long names
• download roster breaking on names with commas

This PR serves to improve the robustness of extensions + submissions autocomplete

How Has This Been Tested?

First created the following users:
(invalid emails can be saved by using inspect element and deleting the type from the input field)

First Name	Last Name	Email	Comments
\back	\slash	[email protected]	Test js escaping
<img src=a onerror=alert(document.cookie)>	alert	[email protected]	Test XSS, also tests long base64 encoding
<strong>first</strong>	<strong>last</strong>	[email protected]	Test HTML escaping
qn?	mark	[email protected]	Tests base64 encodings with /
Space (with a leading space)	Name	[email protected]	Tests that btoa is serving its purpose of handling names with leading / trailing spaces
italics	email	<em>name</em>@italicsemail.com	Tests HTML escaping
bad	email	<strong>name</strong>@bademail.com	Tests HTML escaping
script	email	<script>alert("hello")</script>@scriptemail.com	Tests HTML escaping

• Ensured that flash displays correctly when creating users (i.e. any tags are escaped)
• Successfully give all users an infinite extension, with no autocomplete display issues
• Successfully create submissions for all users (via Manage Submissions > Create New Submission), with no autocomplete display issues
• Ensured that gradebook displays correctly
• Ensured that download roster produces a csv file that displays correctly

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
• My change requires a change to the documentation, which is located at Autolab Docs
• I have updated the documentation accordingly, included in this PR

[cg2v]

Have you tested with names containing characters that would be entity encoded (I used ' (') to make sure they're not double encoded?

Did you verify that the flash xss occurs without these changes? the flash rendering seems safe already:

              <%= content_tag :div, sanitize(msg), id: "flash_#{name}",
                  class: "#{name}" %>

[damianhxy]

Re: flash, I recall that the success flash did not escape <em>name</em>@italicsemail.com before the changes. However, I'll do more testing later and with the error flashes too.

I will also test for double encoding later.

[damianhxy]

Examples of flashes with email: <em>italics</em>@email.com

[damianhxy]

It appears sanitize isn't sufficient for our purposes as it strips tags, rather than escapes them.

Will remedy this by replacing sanitize(msg) with sanitize(h msg)

[damianhxy]

OK - Admittedly sanitize does prevent XSS by stripping unsafe tags and attributes. But nonetheless, I believe we should still HTML escape in order to display the text as-is.

[damianhxy]

Re: double encoding, there do not seem to be any issues. Text displays correctly, and creating extensions, creating submissions, sudo, etc. work.

FWIW: it seems that escape_html does not cause double escapes, but CGI.escapeHTML has the potential to do so, thus care must be taken

[victorhuangwq]

Is this still WIP or is it ready for review?

[cg2v]

it seems pointless to call sanitize() after escape_html(). sanitize won't find anything to filter because escape_html has already encoded all the tag metacharacters.

It appears that if you don't call sanitize, you don't need escape_html either, because rails will automatically escape the inner text of the content_tag.(https://makandracards.com/makandra/2579-how-to-use-html_safe-correctly)

[cg2v]

Instead of this, it might make sense to have the controller use strict_encode64 instead of urlsafe_encode64 (my bad there)

[cg2v]

I suppose I'll have to test this at some point, but I don't see how this can avoid breaking the autocomplete in some cases. the keys in usersEncoded have to match the keys in users. They differ only in whether or not they're encoded. If the keys don't match, the user can't be autocompleted.

[damianhxy]

Re: sanitize / escape_html, if Ruby indeed escapes everything correctly in the absence of sanitize, removing it seems to be the best solution. The only downside I can think of is that we wouldn't be able to style the flashes -- but I don't think we do that anyway (other than roster_error, which is handled specially in application.html.erb).

Re: strict_encode64, I agree that that's a better solution given that we don't use the base64 in URLs, in which case I'll also remove the additional .replaces

Re: autocomplete, I'm not sure which line of code you're specifically referring to. However, I don't see any immediate issues with the current approach. Could you clarify this?

[cg2v]

Say my name is

First Name	Last Name	Email
Chask'l	Grundman	[email protected]

Then @users[CGI.escapeHTML cud.full_name_with_email] = cud.id will use the value
Chask'l Grundman ([email protected])
and @usersEncoded[Base64.urlsafe_encode64(cud.full_name_with_email.strip).strip] = cud.id will use
Q2hhc2snbCBHcnVuZG1hbiAoY2cydkBhdXRvbGFiLmlvKQ==

When the javascript calls btoa on Chask'l Grundman ([email protected]), it gets Q2hhc2smYXBvcztsIEdydW5kbWFuIChjZzJ2QGF1dG9sYWIuaW8p which does not match.

However, the existing code is broken if there are tags in the name (materialize strips the tags - there's no xss). So perhaps the correct thing to do is to escapeHTML before encoding. Things are getting very hacky at this point and it leads me to wonder how other software deals with this problem.

[damianhxy]

Hmm, this doesn't seem to be an issue (at least on my browser, FF100 Mac) since $studentAutocompleteField.val() has the correct unescaped string before the call to btoa.

Perhaps there is no need to change the code then?

[damianhxy]

Also note that I am doing j k instead of j k.html_safe as we appear to need an additional level of escaping to avoid XSS attacks in the autocomplete dropdown.

Essentially, it seems that the autocomplete code unescapes once when displaying text (such that .val() is twice unescaped). Hence, we need exactly two levels of escaping, in order to avoid an XSS after the first level of unescaping, and to obtain the correct string from $studentAutocompleteField.val().

An Example
Original string: <strong>name</strong>@bademail.com -- not escaped
String passed to autocomplete: <strong>name</strong>@bademail.com -- twice escaped
HTML code of dropdown option: <strong>name</strong>@bademail.com -- once escaped
What user sees of dropdown option: <strong>name</strong>@bademail.com -- not escaped
.val(): <strong>name</strong>@bademail.com -- not escaped

[damianhxy]

Alternatively, CGI.escapeHTML (CGI.escapeHTML cud.full_name_with_email) together with j k.html_safe also seems to work, but I'm not sure which is preferable.

[cg2v]

If that's what autocomplete is doing, I will withdraw my objections

[damianhxy]

@victorhuangwq This PR is ready for review!

[victorhuangwq]

Tested the functionality as given by the test case, works as expected.
Good that you abstracted repeated functionality!

I approved, but I had a nit. Thanks for addressing this issue.