[feat] Added Lusca middleware for CSRF [fixes #828] #997

yilenpan · 2015-10-16T04:32:23Z

Added Lusca middleware for CSRF

ilanbiala · 2015-10-16T04:50:42Z

config/env/default.js

+  // Lusca CSRF config
+  csrf: {
+    csrf: false,
+    csp: { /* ... */},


Why the comment?

pgrodrigues · 2015-10-16T08:38:57Z

I didn't test but I believe the server route tests might break unless the csrf token is passed. Tests might need to be updated in order to handle the inclusion of this middleware. Just a heads-up before merging this in.

ilanbiala · 2015-10-16T16:00:26Z

config/env/default.js

+    p3p: 'ABCDEF',
+    hsts: { maxAge: 31536000, // Forces HTTPS for one year
+            includeSubDomains: true,
+            preload: true },


Between line 28/29?

The comment is on 37, the newline should be before the } in 37.

lirantal · 2015-10-17T17:26:25Z

@yilenpan see my comments.

Overall, this is a great addition.
We do need to augment it with a frontend PR to add those tokens for all those API calls that we're making, and then we can enable CSRF protection on too.

codydaig · 2015-10-26T16:07:50Z

Bumping milestone to 0.5.0 so we're not holding up 0.4.2

ilanbiala · 2015-12-31T21:01:59Z

@lirantal any progress on this?

lirantal · 2016-01-02T11:43:21Z

I haven't had a chance to try out the changes introduced in this PR yet.
This is till on my to-do list.

ilanbiala · 2016-02-11T01:53:43Z

@yilenpan can you rebase this? @lirantal can you review this one more time before we merge?

lirantal · 2016-02-13T07:22:03Z

@ilanbiala sure but let's see if @yilenpan wishes to get back to this for the required fixes (been a couple of months since), otherwise I'll take over.

yilenpan · 2016-02-13T07:42:37Z

Hey fellas, sorry for being a little MIA, I've rebased pushed up the new changes. Seems to trip up the linter for some reason, so I'll try and figure that one out too :(

yilenpan · 2016-02-13T08:10:54Z

Hmm.. Looks like there's an error in the changeProfilePicture method in /modules/users/server/controllers/users/users.profile.server.controller.js, could be the config?

yilenpan · 2016-02-15T18:14:08Z

Hey guys, middleware has been added and build is passing.

Best

-Y

codydaig · 2016-02-15T21:25:20Z

package.json

@@ -43,6 +43,7 @@
    "helmet": "~0.9.1",
    "jasmine-core": "~2.3.4",
    "lodash": "~3.10.0",
+    "lusca": "^1.3.0",


codydaig · 2016-02-15T21:26:16Z

Left a line comment. Also, make sure the commit message matches the guidelines outlined in CONTRIBUTING.md.

Added lusca for CSRF protection as per issue meanjs#828 Fixes meanjs#828

yilenpan · 2016-02-16T19:23:31Z

The Node.js 0.12 macosx build failed when trying to install protractor, all other builds passed. May want to rerun that test?

ilanbiala · 2016-02-16T19:58:58Z

Reran it.

ilanbiala · 2016-02-18T20:10:27Z

@lirantal LGTM, wanna take a look?

lirantal · 2016-02-20T11:12:11Z

Great.
Some follow-up items for afterwards:

Frontend side needs to make use of the CSRF tokens
@yilenpan didn't change the config object from csrf to lusca and I think we should but I'm in favor of merging often, and don't want to delay this PR due to my personal favorites.

[feat] Added Lusca middleware for CSRF [fixes #828]

ilanbiala · 2016-02-20T15:48:05Z

Who is going to implement the frontend for the tokens?

codydaig mentioned this pull request Oct 16, 2015

CSRF protection middleware for the app? #828

Closed

ilanbiala reviewed Oct 16, 2015
View reviewed changes

config/env/default.js

// Lusca CSRF config

csrf: {

csrf: false,

csp: { /* ... */},

Copy link

Member

ilanbiala Oct 16, 2015

Choose a reason for hiding this comment

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

Why the comment?

ilanbiala reviewed Oct 16, 2015
View reviewed changes

lirantal self-assigned this Oct 17, 2015

lirantal added this to the 0.4.2 milestone Oct 17, 2015

lirantal added the enhance:feature label Oct 17, 2015

codydaig modified the milestones: 0.5.0, 0.4.2 Oct 26, 2015

jloveland mentioned this pull request Nov 3, 2015

CSRF Protection #1045

Closed

yilenpan force-pushed the feat/csrf branch from 1ce79af to 2679172 Compare February 13, 2016 07:33

yilenpan force-pushed the feat/csrf branch from 2679172 to ee08c81 Compare February 13, 2016 07:48

yilenpan force-pushed the feat/csrf branch from ee08c81 to 91684b1 Compare February 15, 2016 08:22

codydaig reviewed Feb 15, 2016
View reviewed changes

package.json

@@ -43,6 +43,7 @@

"helmet": "~0.9.1",

"jasmine-core": "~2.3.4",

"lodash": "~3.10.0",

"lusca": "^1.3.0",

Copy link

Member

codydaig Feb 15, 2016

Choose a reason for hiding this comment

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

~

feat(csrf): Added Lusca CSRF Prevention

b2db312

Added lusca for CSRF protection as per issue meanjs#828 Fixes meanjs#828

yilenpan force-pushed the feat/csrf branch from 91684b1 to b2db312 Compare February 16, 2016 19:00

lirantal added a commit that referenced this pull request Feb 20, 2016

Merge pull request #997 from yilenpan/feat/csrf

35d7501

[feat] Added Lusca middleware for CSRF [fixes #828]

lirantal merged commit 35d7501 into meanjs:master Feb 20, 2016

tejas002 mentioned this pull request Oct 10, 2024

[Snyk] Fix for 3 vulnerabilities tejas002/mean#73

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Added Lusca middleware for CSRF [fixes #828] #997

[feat] Added Lusca middleware for CSRF [fixes #828] #997

yilenpan commented Oct 16, 2015

ilanbiala Oct 16, 2015

pgrodrigues commented Oct 16, 2015

ilanbiala Oct 16, 2015

yilenpan Oct 16, 2015

ilanbiala Oct 16, 2015

lirantal commented Oct 17, 2015

codydaig commented Oct 26, 2015

ilanbiala commented Dec 31, 2015

lirantal commented Jan 2, 2016

ilanbiala commented Feb 11, 2016

lirantal commented Feb 13, 2016

yilenpan commented Feb 13, 2016

yilenpan commented Feb 13, 2016

yilenpan commented Feb 15, 2016

codydaig Feb 15, 2016

codydaig commented Feb 15, 2016

yilenpan commented Feb 16, 2016

ilanbiala commented Feb 16, 2016

ilanbiala commented Feb 18, 2016

lirantal commented Feb 20, 2016

ilanbiala commented Feb 20, 2016

[feat] Added Lusca middleware for CSRF [fixes #828] #997

[feat] Added Lusca middleware for CSRF [fixes #828] #997

Conversation

yilenpan commented Oct 16, 2015

ilanbiala Oct 16, 2015

Choose a reason for hiding this comment

pgrodrigues commented Oct 16, 2015

ilanbiala Oct 16, 2015

Choose a reason for hiding this comment

yilenpan Oct 16, 2015

Choose a reason for hiding this comment

ilanbiala Oct 16, 2015

Choose a reason for hiding this comment

lirantal commented Oct 17, 2015

codydaig commented Oct 26, 2015

ilanbiala commented Dec 31, 2015

lirantal commented Jan 2, 2016

ilanbiala commented Feb 11, 2016

lirantal commented Feb 13, 2016

yilenpan commented Feb 13, 2016

yilenpan commented Feb 13, 2016

yilenpan commented Feb 15, 2016

codydaig Feb 15, 2016

Choose a reason for hiding this comment

codydaig commented Feb 15, 2016

yilenpan commented Feb 16, 2016

ilanbiala commented Feb 16, 2016

ilanbiala commented Feb 18, 2016

lirantal commented Feb 20, 2016

ilanbiala commented Feb 20, 2016