Skip to content

Commit 64e7e5c

Browse files
damianhxydamianhxy
authored
Improve extensions + submissions autocomplete (#1532)
* Apply cg2v/Autolab@057493b * Apply cg2v/Autolab@752a44f * Prevent XSS * Remove html_safe * Handle differences in base 64 encoding * Add semicolon * Remove extraneous help-blocks * Update form style * Standardize submissions autocomplete * Fix gradebook xss * Extract backend autocomplete logic * Extract frontend autocomplete logic * Prevent email XSS * Refactor autocomplete partial * Escape HTML in flash * Fix roster csv * urlsafe_encode64 -> strict_encode64 * Remove sanitize * Add informative comments * Rename common to components * Shift method to Course model
1 parent f48d1a8 commit 64e7e5c

File tree

11 files changed

+71
-75
lines changed

11 files changed

+71
-75
lines changed

‎app/controllers/courses_controller.rb‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -279,9 +279,9 @@ def download_roster
279279
output = ""
280280
@cuds.each do |cud|
281281
user = cud.user
282-
output += "#{@course.semester},#{cud.user.email},#{user.last_name},#{user.first_name}," \
283-
"#{cud.school},#{cud.major},#{cud.year},#{cud.grade_policy}," \
284-
"#{cud.lecture},#{cud.section}\n"
282+
# to_csv avoids issues with commas
283+
output += [@course.semester, cud.user.email, user.last_name, user.first_name, cud.school,
284+
cud.major, cud.year, cud.grade_policy, cud.lecture, cud.section].to_csv
285285
end
286286
send_data output, filename: "roster.csv", type: "text/csv", disposition: "inline"
287287
end

‎app/controllers/extensions_controller.rb‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,7 @@ class ExtensionsController < ApplicationController
1414
action_auth_level :index, :instructor
1515
def index
1616
@extensions = @assessment.extensions.includes(:course_user_datum)
17-
@users = {}
18-
@usersEncoded = {}
19-
@course.course_user_data.each do |cud|
20-
@users[cud.full_name_with_email] = cud.id
21-
@usersEncoded[Base64.encode64(cud.full_name_with_email.strip).strip] = cud.id
22-
end
17+
@users, @usersEncoded = @course.get_autocomplete_data
2318
@new_extension = @assessment.extensions.new
2419
end
2520

‎app/controllers/submissions_controller.rb‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,7 @@ def new
3535
render([@course, @assessment, :submissions]) && return
3636
end
3737
else
38-
@cuds = {}
39-
# TODO: change order
40-
@course.course_user_data.joins(:user).order("email ASC").each do |cud|
41-
@cuds[cud.full_name_with_email] = cud.id
42-
end
38+
@users, @usersEncoded = @course.get_autocomplete_data
4339
end
4440
end
4541

‎app/form_builders/form_builder_with_date_time_input.rb‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,19 @@ def date_helper(name, options, strftime, date_format, alt_format)
126126
wrap_field name, field, options[:help_text]
127127
end
128128

129-
def wrap_field(name, field, help_text, display_name = nil)
130-
129+
def wrap_field(name, field, help_text = nil, display_name = nil)
131130
@template.content_tag :div, class: "input-field" do
132131
label(name, display_name, class: "control-label") +
133132
field + help_text(name, help_text)
134133
end
135134
end
136135

137136
def help_text(_name, help_text)
138-
@template.content_tag :p, help_text, class: "help-block"
137+
if help_text.nil?
138+
""
139+
else
140+
@template.content_tag :p, help_text, class: "help-block"
141+
end
139142
end
140143

141144
def objectify_options(options)

‎app/helpers/gradebook_helper.rb‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,12 @@ def gradebook_rows(matrix, course, section = nil, lecture = nil)
7171
grace_days = 0
7272
late_days = 0
7373

74+
# CGI.escapeHTML to avoid XSS
7475
row["id"] = cud.id
75-
row["email"] = cud.user.email
76+
row["email"] = CGI.escapeHTML cud.user.email
7677
row["student_gradebook_link"] = sgb_link
77-
row["first_name"] = cud.user.first_name
78-
row["last_name"] = cud.user.last_name
78+
row["first_name"] = CGI.escapeHTML cud.user.first_name
79+
row["last_name"] = CGI.escapeHTML cud.user.last_name
7980
row["section"] = cud.section
8081

8182
# TODO: formalize score render stack, consolidate with computed score

‎app/models/course.rb‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,20 @@ def asmts_before_date(date)
267267
asmts.where("due_at < ?", date)
268268
end
269269

270+
# Used by manage extensions and create submission
271+
def get_autocomplete_data
272+
users = {}
273+
usersEncoded = {}
274+
course_user_data.each do |cud|
275+
# Escape once here, and another time in _autocomplete.html.erb (see comments)
276+
users[CGI.escapeHTML cud.full_name_with_email] = cud.id
277+
# base64 to avoid issues where leading / trailing whitespaces are stripped (see #931)
278+
# strict_encode64 to avoid line feeds
279+
usersEncoded[Base64.strict_encode64(cud.full_name_with_email.strip).strip] = cud.id
280+
end
281+
[users, usersEncoded]
282+
end
283+
270284
private
271285

272286
def saved_change_to_grade_related_fields?

‎app/views/components/_autocomplete.html.erb‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<script type="application/javascript">
2+
/* requires @usersEncoded and @users to be set, i.e. via retrieve_autocompletion_data! */
3+
jQuery(function() {
4+
/* match user name/email with cud_id */
5+
/* escape_javascript prevents issues with backslashes in names, etc. */
6+
userData = {
7+
<% @usersEncoded.each do |k,v| %>
8+
"<%= j k %>": "<%= v %>",
9+
<% end %>
10+
};
11+
12+
/* user autocomplete */
13+
$studentAutocompleteField = $('#student_autocomplete');
14+
$hiddenCUDField = $('<%= hiddenCUDField %>');
15+
/* note that the names were already escaped once in retrieve_autocompletion_data! */
16+
/* and now, each name (k) is implicitly escaped a second time */
17+
/* this is desired behavior since autocomplete unescapes once */
18+
$studentAutocompleteField.autocomplete({
19+
data: {
20+
<% @users.each do |k,v| %>
21+
"<%= j k %>": null,
22+
<% end %>
23+
}
24+
});
25+
26+
/* track changes in student autocomplete field */
27+
$studentAutocompleteField.on('change', function() {
28+
/* $studentAutocompleteField.val() unescapes a second time */
29+
encoded = window.btoa($studentAutocompleteField.val())
30+
$hiddenCUDField.val(userData[encoded]);
31+
});
32+
});
33+
</script>

‎app/views/extensions/index.html.erb‎

Lines changed: 3 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,9 @@
11
<% @title = "Manage Extensions" %>
22

33
<% content_for :javascripts do %>
4-
4+
<%= render partial: "components/autocomplete", locals: { hiddenCUDField: "#extension_course_user_datum_id" } %>
55
<script type="application/javascript">
66
jQuery(function() {
7-
/* match user name/email with cud_id */
8-
userData = {
9-
<% @usersEncoded.each do |k,v| %>
10-
"<%= k %>": "<%= v %>",
11-
<% end %>
12-
};
13-
14-
/* user autocomplete */
15-
$studentAutocompleteField = $('#student_autocomplete');
16-
$hiddenCUDField = $('#extension_course_user_datum_id');
17-
$studentAutocompleteField.autocomplete({
18-
data: {
19-
<% @users.each do |k,v| %>
20-
"<%= k %>": null,
21-
<% end %>
22-
}
23-
});
24-
25-
/* track changes in student autocomplete field */
26-
$studentAutocompleteField.on('change', function() {
27-
$hiddenCUDField.val(userData[window.btoa($studentAutocompleteField.val())]);
28-
})
29-
307
/* set up dates */
318
$dueDate = moment("<%= @assessment.due_at.to_s %>", "YYYY-MM-DD hh:mm:ss ZZ").startOf('day');
329

@@ -102,9 +79,8 @@
10279
<p><b>Create New Extension</b></p>
10380
<%= form_for @new_extension, :as=>"extension", :url=>{:action=>"create"}, builder: FormBuilderWithDateTimeInput do |f| %>
10481
<div class="input-field">
105-
<label class="control-label active" for="student_autocomplete">Student Name/Email</label>
106-
<input type="text" id="student_autocomplete" placeholder="Start typing student name or email" class="autocomplete" autocomplete="off">
107-
<p class="help-block"></p>
82+
<input type="text" size="3" id="student_autocomplete" class="autocomplete" autocomplete="off"/>
83+
<label for="student_autocomplete">Start typing student name or email</label>
10884
</div>
10985
<p>Select a new due date (Currently due at: <span class="moment-date-time"><%= @assessment.due_at.to_s %></span>)</p>
11086
<%= f.date_select :due_at, greater_than: @assessment.due_at, id: "extension_due_at" %><br>

‎app/views/layouts/application.html.erb‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,15 @@
6868

6969
<!-- Flashes -->
7070
<div id="flashes">
71+
<!-- Ruby escapes msg by default, no need for sanitization -->
7172
<% flash.each do |name, msg| %>
7273
<% if name == "roster_error" %>
7374
<div class="error" id="flash_error">
7475
<%= render :partial=>'courses/uploadError',
7576
locals: {roster_errors: msg} %>
7677
</div>
7778
<% else %>
78-
<%= content_tag :div, sanitize(msg), id: "flash_#{name}",
79+
<%= content_tag :div, msg, id: "flash_#{name}",
7980
class: "#{name}" %>
8081
<% end %>
8182
<% end %>

‎app/views/layouts/home.html.erb‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@
4545

4646
<!-- Flashes -->
4747
<div id="flashes">
48+
<!-- Ruby escapes msg by default, no need for sanitization -->
4849
<% flash.each do |name, msg| %>
49-
<%= content_tag :div, sanitize(msg), id: "flash_#{name}",
50-
class: "#{name}" %>
50+
<%= content_tag :div, msg, id: "flash_#{name}",
51+
class: "#{name}" %>
5152
<% end %>
5253

5354
<noscript>

0 commit comments

Comments
 (0)