Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change refresh_watchlist_instances to POST #1525

Merged
merged 1 commit into from May 20, 2022
Merged

Conversation

damianhxy
Copy link
Member

Description

  • Modify refresh_watchlist_instances route to use POST
  • Modify frontend js to use POST

Motivation and Context

Currently, the refresh endpoint uses GET and is vulnerable to CSRF attacks.

How Has This Been Tested?

  • Go to a course and define a metric
  • Accessing /courses/<course-name>/metrics/refresh_watchlist_instances should no longer work
  • Clicking on refresh button works as expected
  • Fudging CSRF token leads to request failing

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

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.

  • verified that GET no longer works
  • POST from different origin fails
  • functionality remains unchanged
    LGTM!

@damianhxy damianhxy merged commit bd3a9ab into master May 20, 2022
@damianhxy damianhxy deleted the fix-metrics-csrf branch May 20, 2022 04:46
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.

None yet

2 participants