Skip to content

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Jun 8, 2022

Description

  • Pluralize some strings using pluralize
  • Add flash[:html_safe] = true whenever the flash should be displayed verbatim (if we know the whole string is safe and have a link that we want to display)
  • Add logic on frontend to set html_safe whenever flash[:html_safe] is set, hence not escaping msg

Motivation and Context

Currently, there appears to be some attempts to mark flashes as html_safe in the controllers (from ~2015)

  • In the past, marking a string as html_safe in the controller would ensure the flash message would not be escaped in the view
  • However, since Rails 4.1, the serialization method was changed, so HTML safety was no longer preserved (source)
  • That said, this seems redundant given the view code of the time, which marked everything as html_safe (a terrible idea)

Since #216, sanitize(msg) was used as an alternate way to mark messages as html_safe. This prevents XSS attacks, but can lead to unexpected styling since tags are stripped but not escaped. This led to the removal of sanitize in #1532 altogether, to ensure that emails always appeared as expected.

However, this meant that some legitimate uses of links in flashes would not show up properly. To counteract this, I am proposing the use of a "flag" via flash[:html_safe] that we can set to true should we need a string to display verbatim. Otherwise, by default, Rails will escape the flash message as usual.

How Has This Been Tested?

  • Batch regrade submissions, observe that link appears as expected (Regrading x submissions) -- note "submission" is conditionally pluralized
  • Regrade all submissions, observe that link appears as expected (Regrading the most recent submissions from x students) -- note "student" is conditionally pluralized
  • Make a submission, observe that link appears as expected (Submitted file <user>_<ver>_<file>.c (Job ID = xx) for autograding. Refresh the page to see the results.)
  • A variety of escaped flashes display as usual (e.g. sudo, unsudo)
  • Also check that escaping works (e.g. sudo to a user whose email contains html -- requires inspect element to remove type=email from element)
    • For example, add user to course with email <strong>name</strong>@bademail.com (bypassing validation by removing type from email field)
    • Then, sudo to the user (also removing type from email field)
  • The other code paths are analogous and do not require special testing

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

@damianhxy
Copy link
Member Author

damianhxy commented Jun 9, 2022

Alternative approaches include

  • Reverting to sanitize(msg) and escaping on backend instead -- but this can be error-prone leading to unexpected styling from msg, and sanitize could inhibit more creative uses of flash
  • Creating flash classes such as success_safe, error_safe, etc -- but this seems more troublesome and unnecessarily complicated
  • Check for flash[:html_safe] on frontend instead of inside ApplicationController and manage logic accordingly -- but I feel that the logic is best left to the backend

EDIT: On second thoughts, I have changed the code to option 3, as leaving the logic to the backend can be brittle (if we delete flash[:html_safe], then we will lose the flashes if there are multiple redirects. However, if we do not delete flash[:html_safe], then we have to use special logic to skip it when enumerating the flashes)

@20wildmanj
Copy link
Contributor

I think for now what you're doing in terms of using flash[:html_safe] on frontend is fine, I'm not an expert on where we should be doing this logic but at least this way it's not too complicated, don't need to implement anything extra.

Copy link
Contributor

@20wildmanj 20wildmanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • batch regrade link appears as expected and is pluralized, same with regrade all
  • autograde submission appears as expected
  • flashes still appear to function fine
  • verified that escaping for new user with html in email works

LGTM!

@damianhxy damianhxy merged commit 4107b4d into master Jun 11, 2022
@damianhxy damianhxy deleted the flash-html-safe-fix branch June 11, 2022 23:45
@damianhxy damianhxy mentioned this pull request Jul 10, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants