tl-its-umich-edu / canvas-course-manager-next

Canvas Course Manager Next: A redesign of the existing CCM application. It extends Canvas features, makes cumbersome features easier to use, and adds new features.
8 stars 9 forks source link

Replace Csurf package with its equivalent #410

Closed pushyamig closed 6 months ago

pushyamig commented 8 months ago

This package is deprecated and need to replace with a new one. We have chosen to us Double CSRF Package Here is some of the reading and recommendation on this

https://dev-academy.com/csurf-vulnerability/ https://security.stackexchange.com/questions/271190/secure-alternative-to-csurf-npm-package https://github.com/expressjs/csurf


Test Plan:

Function Point of View

  1. All the PUT/POST/DELETE Call should be testing. below are the features that needs to be tested
    1. Course Rename, Create section, Create external user (single and Bulk), Enroll Users To course section, Unmerge sections, Merge Sections
    2. Delete the Canvas API token, re-authenticate and test if CSRF token is set properly. Any one of the feature listed in the 1.1 should be tested ( not all is needed)
  2. App is functional and regression testing

Developer Only Testing: All the above testing and all things listed below. This needs to be done during the PR review

  1. Remove the CSRF token from PUT/POST/DELETE ccm_web/client/src/api.ts and see the Backend handles missing CSRF token properly
  2. Swagger can show the CSRF token and use it with POST/PUT/DELETE calls
ktowneUM commented 4 months ago

Tested Course Rename Create section Create external user (single and Bulk) Enroll Users To course section Unmerge sections Merge Sections Deleted the Canvas API token, re-authenticate and tested course rename

pushyamig commented 4 months ago

@ktowneUM you moved the issue to Dev/QA after testing, did you mean to move it to Done?

ktowneUM commented 4 months ago

There is some developer only testing in the test plan I can't do

pushyamig commented 4 months ago

Dev test plan was provided is for when reviewing PR and checking that. I think it is already taken care so I will move the issue to Done column