Replacing the CSRF package(CSURF) with Double CSRF package #420

pushyamig · 2024-03-22T13:20:00Z

Fixes #410
(Test Plan is updated in the Issue #410
Latest Build from this PR https://github.com/pushyamig/canvas-course-manager-next/actions)

The approach taken here is the double csrf library is setting the CSRF token cookie (with hash) and also HttpOnly flag is enabled so that FE code doesn't read the cookie. The CSRF token is then passed in as part of GET call as response object (As lib author recommendation) and stored in the react state (As props). The token is then sent with POST/PUT/DELETE call and double csrf validate it and then those https calls are processed. please refer to step#4 for the example implementation.

The new Package that is chose for this Double CSRF
.env have a property added called CSRF_SECRET. For local development if is not set it has default. This is something in line with TOKEN_SECRET and 'COOKIE_SECRET`. Non-Prod and Prod will always be set with this token
This PR is running in the Canvas Dev. For example
POC of the CSRF strategy adopted by this Library is provided by Package author is here
1. Author suggesting is to not to set the CSRF TOKEN in Cookie but instead send it as Http Response and store a state attribute in Frontend like React props or state management design of the app
2. CSRF token is only needed for state change HTTP methods. so for our app implementation POST, PUT, DELETE.
As per author the CSRF cookie name should be either _Host and _Secure.
1. I adopted `_Secure since _Host did not fit out needs since we were setting Path=/ and Domain to CCM URL. More info here what is correct params with _HOST

Note: For Local development you will see 2 call in Network as this is by Design when running React In StrictMode, So you won't see this behavior in CCMDev or Prod
https://stackoverflow.com/questions/72238175/why-useeffect-running-twice-and-how-to-handle-it-well-in-react

In development, you will see two fetches in the Network tab. There is nothing wrong with that. With the approach above, the first Effect will immediately get cleaned... So even though there is an extra request, it won’t affect the state thanks to the abort.

In production, there will only be one request. If the second request in development is bothering you, the best approach is to use a solution that deduplicates requests and caches their responses between components:

pushyamig · 2024-03-22T13:20:59Z

ccm_web/server/src/auth/double.csrf.middleware.ts

@@ -0,0 +1,24 @@
+import { Injectable, NestMiddleware } from '@nestjs/common'


Create a new middleware support of double csrf package

pushyamig · 2024-03-22T13:22:08Z

ccm_web/server/src/auth/double.csrf.middleware.ts

+  use(req: Request, res: Response, next: NextFunction) {
+    const { doubleCsrfProtection } = doubleCsrf({
+      getSecret: () => this.configService.get<string>('server.csrfSecret', { infer: true }),
+      cookieName: '_Secure-ccm.x-csrf-token',


I added a Cookie name with _Secure instead of _Host as per Authors recommendation. See the PR description for reasoning

pushyamig · 2024-03-22T13:26:10Z

.github/workflows/ccm.yml

@@ -6,6 +6,7 @@ on:
    branches: 


This will be removed

pushyamig · 2024-03-27T13:45:30Z

This PR is up to date with the recent features commits

jaydonkrooss · 2024-03-28T16:10:55Z

Following the local testing steps you listed, everything behaves as expected. Removing CSRF token in the API causes errors. Code looks good, only thing left is the three codacy errors of unused imports, if you want to clear those warnings.

pushyamig · 2024-03-28T16:16:40Z

only thing left is the three codacy errors of unused imports, if you want to clear those warnings.

I looked at those error, I don't think those warning are accurate. So I don't know why that is warning. So I left as it

For example:

import { APP_FILTER } from '@nestjs/core'

APP_FILTER is used in the auth.module.ts as part of NestJS module declaration. So I don't think i am doing any different or syntactically wrong.

jaydonkrooss · 2024-03-28T16:18:00Z

only thing left is the three codacy errors of unused imports, if you want to clear those warnings.

I looked at those error, I don't think those warning are accurate. So I don't know why that is warning. So I left as it

For example:

import { APP_FILTER } from '@nestjs/core'

APP_FILTER is used in the auth.module.ts as part of NestJS module declaration. So I don't think i am doing any different or syntactically wrong.

I see, that's a little strange. Ok otherwise this looks good I'll approve

pushyamig · 2024-03-28T16:20:20Z

@jaydonkrooss , Thanks for the review. I will let @jonespm take a look as well before merging it.

jonespm · 2024-03-28T16:31:03Z

I'm not sure if it's possible to fix this check with Codacy but I think we could add to ignore it in the .eslintrc.js.
"no-unused-vars": "off"

or declaring everything that's a global.

That's probably a separate issue. I'm not sure if there's a better workaround that works as I haven't configured this.

jonespm · 2024-03-28T16:35:40Z

I'm not going to be able to re-test this and will rely on Jaydons feedback. It looks like you've done the most research on this and I don't know how it could have been done any differently here.

pushyamig · 2024-03-28T16:46:29Z

I'm not sure if it's possible to fix this check with Codacy but I think we could add to ignore it in the .eslintrc.js.
"no-unused-vars": "off"

In this example I described above the variable is being used in the file. So I don't know having no-unused-vars: off would do anything. Plus I did not configure the Codacy for it. WHen I looked at it I see ESlint and ESlint(Old) and bunch of other linters (which I never heard of) may be added by default by Codacy. Sam did this and don't know how much he configured and/or added by COdacy.

I need to comeback to Codacy configuration CCM. I will create an issue but will deal with it separately.
We do have local linting from VsCodeeslintrc.js

pushyamig · 2024-03-28T16:50:14Z

I'm not going to be able to re-test this and will rely on Jaydons feedback. It looks like you've done the most research on this and I don't know how it could have been done any differently here.

Thanks @jonespm for considering for review. I will take that as approval from you if you don't want to click the approve button.

This PR has being a learning curve so I wanted more people looking at it and aware of the implementation.

jonespm · 2024-03-28T16:54:43Z

@pushyamig I think Codacy is also in "Configuration file" mode and also using that file. But Sam set it up and I haven't tested or tried to change anything. But I also see what you mean about it being used.

Maybe need to use the plugin it suggests @typescript-eslint/no-unused-vars? But yeah can experiment on a separate issue. These aren't the only 3 instances of that warning in this code.

pushyamig · 2024-03-28T16:55:30Z

I looked at the Myla Codacy configuration for FE seems very similar to what CCM has https://app.codacy.com/gh/tl-its-umich-edu/my-learning-analytics/patterns/list

I would assume we might need to come up with the approach for Codacy configuration for FE. Both project has linter configured as part of the code and VSCode integration. So Don't know how much is same/different with either of these configuration.

pushyamig · 2024-03-28T17:39:30Z

.github/workflows/ccm.yml

@@ -7,7 +7,6 @@ on:
      - main


Removing this ccm.yml changes before merging

pushyamig commented Mar 22, 2024

View reviewed changes

.github/workflows/ccm.yml Outdated

@@ -6,6 +6,7 @@ on:

branches:

Copy link

Contributor Author

pushyamig Mar 22, 2024

Choose a reason for hiding this comment

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

This will be removed

pushyamig force-pushed the i410_replacement_csrf branch from 9051a13 to 90afaff Compare March 22, 2024 13:33

pushyamig requested review from jaydonkrooss and jonespm March 22, 2024 14:18

pushyamig force-pushed the i410_replacement_csrf branch from 735ee0c to dbf8b3c Compare March 26, 2024 14:34

pushyamig added 15 commits March 27, 2024 09:25

Moving to double csrf package

dfc7274

adding the double csrd middleware

6f5a539

Initial working wiring of double csrf

86b347c

Csrf working with Put

2bfe4eb

End-to-end wiring of double csrf lib

d96c604

Removing the wiring from globals

15aee96

Secure Cookie

6efcd12

Adding the csrf token to env sample

12d696c

removing the extra comma

22a6355

Removing res property from globals

547538b

Codacy fixes

bbc7d83

More codacy fixes

53cd6ff

Rebasing from feature branch

81eedff

Removing js-cookie library

5e063bc

Rebasing and removing js-cookie types

b117b3e

pushyamig force-pushed the i410_replacement_csrf branch from d20d2ef to b117b3e Compare March 27, 2024 13:37

pushyamig added config change Involves a change to the configuration file/format back end Involves changes to the Express application or other server-related files labels Mar 27, 2024

jaydonkrooss approved these changes Mar 28, 2024

View reviewed changes

Removing the ccm.yml pointing to my repo

7d3e61b

pushyamig commented Mar 28, 2024

View reviewed changes

.github/workflows/ccm.yml

@@ -7,7 +7,6 @@ on:

- main

Copy link

Contributor Author

pushyamig Mar 28, 2024

Choose a reason for hiding this comment

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

Removing this ccm.yml changes before merging

pushyamig merged commit e30cb1c into tl-its-umich-edu:2024-03-01-dep-update Mar 28, 2024
1 check failed

pushyamig added a commit that referenced this pull request May 21, 2024

Replacing the CSRF package(CSURF) with Double CSRF package (#420)

2b15037

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing the CSRF package(CSURF) with Double CSRF package #420

Replacing the CSRF package(CSURF) with Double CSRF package #420

pushyamig commented Mar 22, 2024 •

edited

Loading

pushyamig Mar 22, 2024

pushyamig Mar 22, 2024

pushyamig Mar 22, 2024

pushyamig commented Mar 27, 2024

jaydonkrooss commented Mar 28, 2024

pushyamig commented Mar 28, 2024

jaydonkrooss commented Mar 28, 2024

pushyamig commented Mar 28, 2024

jonespm commented Mar 28, 2024

jonespm commented Mar 28, 2024

pushyamig commented Mar 28, 2024

pushyamig commented Mar 28, 2024 •

edited

Loading

jonespm commented Mar 28, 2024

pushyamig commented Mar 28, 2024

pushyamig Mar 28, 2024

		@@ -0,0 +1,24 @@
		import { Injectable, NestMiddleware } from '@nestjs/common'

Replacing the CSRF package(CSURF) with Double CSRF package #420

Replacing the CSRF package(CSURF) with Double CSRF package #420

Conversation

pushyamig commented Mar 22, 2024 • edited Loading

pushyamig Mar 22, 2024

Choose a reason for hiding this comment

pushyamig Mar 22, 2024

Choose a reason for hiding this comment

pushyamig Mar 22, 2024

Choose a reason for hiding this comment

pushyamig commented Mar 27, 2024

jaydonkrooss commented Mar 28, 2024

pushyamig commented Mar 28, 2024

jaydonkrooss commented Mar 28, 2024

pushyamig commented Mar 28, 2024

jonespm commented Mar 28, 2024

jonespm commented Mar 28, 2024

pushyamig commented Mar 28, 2024

pushyamig commented Mar 28, 2024 • edited Loading

jonespm commented Mar 28, 2024

pushyamig commented Mar 28, 2024

pushyamig Mar 28, 2024

Choose a reason for hiding this comment

pushyamig commented Mar 22, 2024 •

edited

Loading

pushyamig commented Mar 28, 2024 •

edited

Loading