sanitize quiz answer comments on the way in

I am not aware of any XSS vulnerabilities, because these were
sanitized on the way out, but defense in depth seems like a
good idea here

closes ADMIN-2494

Change-Id: I5d782b23e494a3877302d19aa4cd1c1b29a22b02
Reviewed-on: https://gerrit.instructure.com/180631
Tested-by: Jenkins
Reviewed-by: Jon Willesen <jonw+gerrit@instructure.com>
QA-Review: Anju Reddy <areddy@instructure.com>
QA-Review: Mysti Lilla <mysti@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>

app/models/quizzes/quiz_question/answer_parsers/essay.rb

@@ -24,7 +24,7 @@ module Quizzes::QuizQuestion::AnswerParsers
 
       answer = Quizzes::QuizQuestion::RawFields.new({comments: comment, comments_html: comments_html})
       question[:comments] = answer.fetch_with_enforced_length(:comments, max_size: 5.kilobyte)
-      question[:comments_html] = answer.fetch_with_enforced_length(:comments_html, max_size: 5.kilobyte)
+      question[:comments_html] = answer.sanitize(answer.fetch_with_enforced_length(:comments_html, max_size: 5.kilobyte))
 
       question.answers = @answers
       question

app/models/quizzes/quiz_question/answer_parsers/fill_in_multiple_blanks.rb

@@ -26,7 +26,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           id: fields.fetch_any(:id, nil),
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: fields.fetch_any([:answer_weight, :weight]).to_f,
           blank_id: fields.fetch_with_enforced_length(:blank_id)
         })

app/models/quizzes/quiz_question/answer_parsers/matching.rb

@@ -30,7 +30,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           left: fields.fetch_with_enforced_length(:answer_match_left),
           right: fields.fetch_with_enforced_length(:answer_match_right),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]))
         }
 
         a[:left_html] = a[:html] = fields.sanitize(fields.fetch_any(:answer_match_left_html)) if answer[:answer_match_left_html].present?

app/models/quizzes/quiz_question/answer_parsers/missing_word.rb

@@ -24,7 +24,7 @@ module Quizzes::QuizQuestion::AnswerParsers
         a = {
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: fields.fetch_any([:answer_weight, :weight]).to_f
         }
 

app/models/quizzes/quiz_question/answer_parsers/multiple_answers.rb

@@ -26,7 +26,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           id: fields.fetch_any(:id, nil),
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: fields.fetch_any([:answer_weight, :weight]).to_f
         }
 

app/models/quizzes/quiz_question/answer_parsers/multiple_choice.rb

@@ -26,7 +26,7 @@ module Quizzes::QuizQuestion::AnswerParsers
         id = id.to_i if id
         text = fields.fetch_with_enforced_length([:answer_text, :text])
         comments = fields.fetch_with_enforced_length([:answer_comment, :comments])
-        comments_html = fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])
+        comments_html = fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]))
         weight = fields.fetch_any([:answer_weight, :weight]).to_f
         html = fields.sanitize(fields.fetch_any([:answer_html, :html]))
 

app/models/quizzes/quiz_question/answer_parsers/multiple_dropdowns.rb

@@ -28,7 +28,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           id: fields.fetch_any([:id, :answer_id], nil).try(:to_i),
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: fields.fetch_any(:answer_weight, 0).to_f,
           blank_id: fields.fetch_with_enforced_length(:blank_id)
         }

app/models/quizzes/quiz_question/answer_parsers/numerical.rb

@@ -25,7 +25,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           id: fields.fetch_any(:id, nil),
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: 100
         }
 

app/models/quizzes/quiz_question/answer_parsers/short_answer.rb

@@ -25,7 +25,7 @@ module Quizzes::QuizQuestion::AnswerParsers
           id: fields.fetch_any([:id, :answer_id], nil),
           text: fields.fetch_with_enforced_length([:answer_text, :text]),
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           weight: 100
         }
 

app/models/quizzes/quiz_question/answer_parsers/true_false.rb

@@ -23,7 +23,7 @@ module Quizzes::QuizQuestion::AnswerParsers
         fields = Quizzes::QuizQuestion::RawFields.new(answer)
         a = {
           comments: fields.fetch_with_enforced_length([:answer_comment, :comments]),
-          comments_html: fields.fetch_with_enforced_length([:answer_comment_html, :comments_html]),
+          comments_html: fields.sanitize(fields.fetch_with_enforced_length([:answer_comment_html, :comments_html])),
           text: fields.fetch_any([:answer_text, :text]),
           weight: fields.fetch_any([:answer_weight, :weight]).to_i
         }

app/models/quizzes/quiz_question/question_data.rb

@@ -69,9 +69,9 @@ class Quizzes::QuizQuestion::QuestionData
     question[:incorrect_comments] = fields.fetch_with_enforced_length(:incorrect_comments, max_size: 5.kilobyte)
     question[:neutral_comments] = fields.fetch_with_enforced_length(:neutral_comments, max_size: 5.kilobyte)
 
-    question[:correct_comments_html] = fields.fetch_with_enforced_length(:correct_comments_html, max_size: 5.kilobyte)
-    question[:incorrect_comments_html] = fields.fetch_with_enforced_length(:incorrect_comments_html, max_size: 5.kilobyte)
-    question[:neutral_comments_html] = fields.fetch_with_enforced_length(:neutral_comments_html, max_size: 5.kilobyte)
+    question[:correct_comments_html] = fields.sanitize(fields.fetch_with_enforced_length(:correct_comments_html, max_size: 5.kilobyte))
+    question[:incorrect_comments_html] = fields.sanitize(fields.fetch_with_enforced_length(:incorrect_comments_html, max_size: 5.kilobyte))
+    question[:neutral_comments_html] = fields.sanitize(fields.fetch_with_enforced_length(:neutral_comments_html, max_size: 5.kilobyte))
 
     question[:question_type] = fields.fetch_any(:question_type, "text_only_question")
     question[:question_name] = fields.fetch_any(:question_name, I18n.t(:default_question_name, "Question"))
@@ -79,7 +79,7 @@ class Quizzes::QuizQuestion::QuestionData
     question[:name] = question[:question_name]
     question[:question_text] = fields.sanitize(fields.fetch_with_enforced_length(:question_text, default: I18n.t(:default_question_text, "Question text")))
     question[:answers] = fields.fetch_any(:answers, [])
-    question[:text_after_answers] = fields.fetch_any(:text_after_answers)
+    question[:text_after_answers] = fields.sanitize(fields.fetch_any(:text_after_answers))
 
     if question.is_type?(:calculated)
       question[:formulas] = fields.fetch_any(:formulas, [])

spec/models/quizzes/quiz_question/answer_parsers/answer_parser_spec_helper.rb

@@ -47,6 +47,11 @@ shared_examples_for "All answer parsers" do
     ids = @answer_data.answers.map { |a| a[:id] }
     ids.each { |id| expect(id).to be_kind_of(Integer) }
   end
+
+  it "sanitizes answer comments" do
+    expect(@answer_data.first[:comments_html]).to include('<img')
+    expect(@answer_data.first[:comments_html]).not_to include('onerror')
+  end
 end
 
 

spec/models/quizzes/quiz_question/answer_parsers/essay_spec.rb

@@ -26,7 +26,8 @@ describe Quizzes::QuizQuestion::AnswerParsers::Essay do
       [
         {
           answer_text: "Essay Answer",
-          answer_comments: "This is an essay answer"
+          answer_comments: "This is an essay answer",
+          answer_comment_html: '<img src="x" onerror="alert(1)">'
         }
       ]
     end
@@ -39,6 +40,8 @@ describe Quizzes::QuizQuestion::AnswerParsers::Essay do
       question = Quizzes::QuizQuestion::QuestionData.new({})
       essay.parse(question)
       expect(question[:comments]).to eq raw_answers[0][:answer_comments]
+      expect(question[:comments_html]).to include '<img'
+      expect(question[:comments_html]).not_to include 'onerror'
     end
   end
 end

spec/models/quizzes/quiz_question/answer_parsers/fill_in_multiple_blanks_spec.rb

@@ -27,6 +27,7 @@ describe Quizzes::QuizQuestion::AnswerParsers::FillInMultipleBlanks do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
           answer_weight: 100,
           blank_id: "answer1"
         },

spec/models/quizzes/quiz_question/answer_parsers/matching_spec.rb

@@ -29,24 +29,22 @@ describe Quizzes::QuizQuestion::AnswerParsers::Matching do
           answer_match_left: "Answer 1",
           answer_match_right: "Match to Answer 1",
           answer_comment: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_match_left: "Answer 2",
           answer_match_right: "Match to Answer 2",
           answer_comment: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_match_left: "Answer 3",
           answer_match_right: "Match to Answer 3",
           answer_comment: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end
@@ -70,24 +68,21 @@ describe Quizzes::QuizQuestion::AnswerParsers::Matching do
           answer_match_left: "Salt Lake City",
           answer_match_right: "Utah",
           answer_comment: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_weight: 0
         },
         {
           answer_text: "San Diego",
           answer_match_left: "San Diego",
           answer_match_right: "California",
           answer_comment: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Los Angeles",
           answer_match_left: "Los Angeles",
           answer_match_right: "California",
           answer_comment: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/missing_word_spec.rb

@@ -28,20 +28,18 @@ describe Quizzes::QuizQuestion::AnswerParsers::MissingWord do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/multiple_answers_spec.rb

@@ -27,20 +27,18 @@ describe Quizzes::QuizQuestion::AnswerParsers::MultipleAnswers do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/multiple_choice_spec.rb

@@ -28,22 +28,20 @@ describe Quizzes::QuizQuestion::AnswerParsers::MultipleChoice do
           answer_text: "Answer 1",
           answer_html: "",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_html: "",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_html: "",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/multiple_dropdowns_spec.rb

@@ -27,8 +27,8 @@ describe Quizzes::QuizQuestion::AnswerParsers::MultipleDropdowns do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
           answer_weight: 0,
-          text_after_answers: "Text after Answer 1",
           id: '1000'
 
         },
@@ -36,14 +36,12 @@ describe Quizzes::QuizQuestion::AnswerParsers::MultipleDropdowns do
           answer_text: "Answer 2",
           answer_comments: "This is answer 2",
           answer_weight: 100,
-          text_after_answers: "Text after Answer 2",
           id: 1001
         },
         {
           answer_text: "Answer 3",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/numerical_spec.rb

@@ -27,20 +27,18 @@ describe Quizzes::QuizQuestion::AnswerParsers::Numerical do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/short_answer_spec.rb

@@ -27,20 +27,18 @@ describe Quizzes::QuizQuestion::AnswerParsers::ShortAnswer do
         {
           answer_text: "Answer 1",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
           answer_text: "Answer 2",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
           answer_text: "Answer 3",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question/answer_parsers/true_false_spec.rb

@@ -25,22 +25,20 @@ describe Quizzes::QuizQuestion::AnswerParsers::TrueFalse do
     let(:raw_answers) do
       [
         {
-          answer_text: "Answer 1",
+          answer_text: "True",
           answer_comments: "This is answer 1",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 1"
+          answer_comment_html: '<img src="x" onerror="alert(1)">',
+          answer_weight: 0
         },
         {
-          answer_text: "Answer 2",
+          answer_text: "False",
           answer_comments: "This is answer 2",
-          answer_weight: 100,
-          text_after_answers: "Text after Answer 2"
+          answer_weight: 100
         },
         {
-          answer_text: "Answer 3",
+          answer_text: "File not found",
           answer_comments: "This is answer 3",
-          answer_weight: 0,
-          text_after_answers: "Text after Answer 3"
+          answer_weight: 0
         }
       ]
     end

spec/models/quizzes/quiz_question_spec.rb

@@ -86,6 +86,45 @@ describe Quizzes::QuizQuestion do
       expect(question_regrade).to be
       expect(question_regrade.regrade_option).to eq 'full_credit'
     end
+
+    it "sanitizes all the html" do
+      question_data = {
+        "id"=>nil,
+        "regrade_option"=>"",
+        "points_possible"=>1.0,
+        "correct_comments_html"=>"<img src=\"x\" onerror=\"alert(1)\">",
+        "incorrect_comments_html"=>"<img src=\"x\" onerror=\"alert(2)\">",
+        "neutral_comments_html"=>"<img src=\"x\" onerror=\"alert(3)\">",
+        "question_type"=>"multiple_choice_question",
+        "question_name"=>"Question",
+        "name"=>"Question",
+        "question_text"=>"<img src=\"x\" onerror=\"alert(4)\">",
+        "answers"=>
+        [
+          {
+            "id"=>8206,
+            "html"=>"<img src=\"x\" onerror=\"alert(5)\">",
+            "comments_html"=>"<img src=\"x\" onerror=\"alert(6)\">",
+            "weight"=>100.0
+          },
+          {
+            "id"=>6973,
+            "html"=>"<img src=\"x\" onerror=\"alert(7)\">",
+            "comments_html"=>"<img src=\"x\" onerror=\"alert(8)\">",
+            "weight"=>0.0
+          }
+        ]
+      }
+      qq = @quiz.quiz_questions.create(:question_data => question_data)
+      expect(qq.question_data['correct_comments_html']).not_to include('onerror')
+      expect(qq.question_data['incorrect_comments_html']).not_to include('onerror')
+      expect(qq.question_data['neutral_comments_html']).not_to include('onerror')
+      expect(qq.question_data['question_text']).not_to include('onerror')
+      expect(qq.question_data['answers'][0]['html']).not_to include('onerror')
+      expect(qq.question_data['answers'][0]['comments_html']).not_to include('onerror')
+      expect(qq.question_data['answers'][1]['html']).not_to include('onerror')
+      expect(qq.question_data['answers'][1]['comments_html']).not_to include('onerror')
+    end
   end
 
   describe ".update_all_positions" do