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

🔏 feat: add csrf protection #205

Merged
merged 13 commits into from
Aug 30, 2017
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,12 @@ var conf = convict({
'If true, requires requests to be secure. Overridden if server.protocol is set to https.',
format: '*',
default: false
},
csrf: {
doc:
'If true, a CSRF token will be provided, and all form submissions must include this as _csrf',
format: '*',
default: true
}
},
env: {
Expand Down
5 changes: 4 additions & 1 deletion dadi/lib/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,10 @@ function onError (api) {
message: err.message
}

data.stack = err.stack ? err.stack : 'Nothing to see'
data.stack =
err.stack && config.get('env') !== 'production'
? err.stack
: 'Nothing to see'

// look for a page that has been loaded
// that matches the error code and call its handler if it exists
Expand Down
4 changes: 4 additions & 0 deletions dadi/lib/controller/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ Controller.prototype.buildInitialViewData = function (req) {
data.debug = config.get('debug')
data.json = json || false

if (config.get('security.csrf')) {
data.csrfToken = req.csrfToken()
}

delete data.query.json
delete data.params.json

Expand Down
12 changes: 12 additions & 0 deletions dadi/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ var path = require('path')
var serveFavicon = require('serve-favicon')
var serveStatic = require('serve-static')
var session = require('express-session')
var csrf = require('csurf')
Copy link
Contributor

Choose a reason for hiding this comment

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

Bonus points (and a beer) are granted if you replace var with const/let as you go. ⭐️ 🍺

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eduardoboucas I wasn't sure if we were doing that or if the plan was to stick with the existing style. Maybe it'd be worth tidying up the dependencies at least.

var cookieParser = require('cookie-parser')
var toobusy = require('toobusy-js')
var dadiStatus = require('@dadi/status')

Expand Down Expand Up @@ -246,6 +248,16 @@ Server.prototype.start = function (done) {
app.use(session(sessionOptions))
}

// use csrf protection if enabled
if (config.get('security.csrf')) {
if (sessionConfig.enabled) {
app.use(csrf())
} else {
app.use(cookieParser())
app.use(csrf({ cookie: true }))
}
}

// set up cache
var cacheLayer = cache(this)

Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
"connect-redis": "3.2.0",
"console-stamp": "^0.2.2",
"convict": "3.0.0",
"cookie-parser": "^1.4.3",
"csurf": "^1.9.0",
"debug": "^2.6.1",
"deepmerge": "^1.3.2",
"dustjs-helpers": "^1.7.3",
Expand Down
10 changes: 10 additions & 0 deletions test/help.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@ TestHelper.prototype.resetConfig = function() {
})
}

TestHelper.prototype.extractCookieValue = function(res, cookieName) {
var cookies = res.headers["set-cookie"]
var cookie = cookies.find(function(cookie) {
return cookie.startsWith(cookieName + "=")
})
var data = cookie.split(";")[0]
var value = data.split("=")[1]
return value
}

/**
* Tests that the response has the Set-Cookie header equal to "name"
*/
Expand Down
177 changes: 177 additions & 0 deletions test/unit/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,183 @@ describe("Controller", function(done) {
})
})

/*
If CSRF is used (csrf enabled in web config), then any POST requests must provide a valid token under the name _csrf, and if it either isn't present or is incorrect, then a 403 is generated, which currently gets picked up by web for the custom error page.

Also, if csrf is enabled, the csrfToken should be present on the view model.
*/
describe("CSRF", function() {
it("should add a csrfToken to the view context", function(done) {
TestHelper.disableApiConfig().then(() => {
TestHelper.updateConfig({
security: {
csrf: true
}
}).then(() => {
var pages = TestHelper.setUpPages()
pages[0].datasources = []

TestHelper.startServer(pages).then(() => {
var client = request(connectionString)
client
.get(pages[0].routes[0].path + "?json=true")
.expect(200)
.end((err, res) => {
if (err) return done(err)
should.exist(res.body.csrfToken)
done()
})
})
})
})
})

it("should set a cookie with the csrf secret", function(done) {
TestHelper.disableApiConfig().then(() => {
TestHelper.updateConfig({
security: {
csrf: true
}
}).then(() => {
var pages = TestHelper.setUpPages()
pages[0].datasources = []

TestHelper.startServer(pages).then(() => {
var client = request(connectionString)
client
.get(pages[0].routes[0].path + "?json=true")
.expect(200)
.expect(TestHelper.shouldSetCookie("_csrf"))
.end((err, res) => {
if (err) return done(err)
done()
})
})
})
})
})

it("should return a 403 if a POST request is missing a csrf token", function(
done
) {
TestHelper.disableApiConfig().then(() => {
TestHelper.updateConfig({
security: {
csrf: true
}
}).then(() => {
var pages = TestHelper.setUpPages()
pages[0].datasources = ["categories"]

TestHelper.startServer(pages).then(() => {
// provide API response
var results = {
categories: { results: [{ _id: 1, title: "books" }] }
}

sinon
.stub(Controller.Controller.prototype, "loadData")
.yields(null, results)

var client = request(connectionString)
client
.post(pages[0].routes[0].path + "?json=true")
.send()
.expect(403)
.end((err, res) => {
if (err) return done(err)
Controller.Controller.prototype.loadData.restore()
res.text.indexOf("invalid csrf token").should.be.above(0)
done()
})
})
})
})
})

it("should return 403 when POST request contains an invalid csrfToken", function(
done
) {
TestHelper.disableApiConfig().then(() => {
TestHelper.updateConfig({
security: {
csrf: true
}
}).then(() => {
var pages = TestHelper.setUpPages()
pages[0].datasources = []

TestHelper.startServer(pages).then(() => {
var client = request(connectionString)
client
.get(pages[0].routes[0].path + "?json=true")
.expect(200)
.expect(TestHelper.shouldSetCookie("_csrf"))
.end((err, res) => {
if (err) return done(err)
should.exist(res.body.csrfToken)

client
.post(pages[0].routes[0].path)
.set(
"Cookie",
"_csrf=" + TestHelper.extractCookieValue(res, "_csrf")
)
.send({ _csrf: "XXX" })
.expect(403)
.end((err, res) => {
if (err) return done(err)
res.text.indexOf("invalid csrf token").should.be.above(0)
done()
})
})
})
})
})
})

it("should return 200 when POST request contains a valid csrfToken", function(
done
) {
TestHelper.disableApiConfig().then(() => {
TestHelper.updateConfig({
security: {
csrf: true
}
}).then(() => {
var pages = TestHelper.setUpPages()
pages[0].datasources = []

TestHelper.startServer(pages).then(() => {
var client = request(connectionString)
client
.get(pages[0].routes[0].path + "?json=true")
.expect(200)
.expect(TestHelper.shouldSetCookie("_csrf"))
.end((err, res) => {
if (err) return done(err)
should.exist(res.body.csrfToken)

client
.post(pages[0].routes[0].path)
.set(
"Cookie",
"_csrf=" + TestHelper.extractCookieValue(res, "_csrf")
)
.send({ _csrf: res.body.csrfToken.toString() })
.expect(200)
.end((err, res) => {
if (err) return done(err)
res.text.indexOf("invalid csrf token").should.equal(-1)
done()
})
})
})
})
})
})
})

describe("Events", function(done) {
it("should load events in the order they are specified", function(done) {
TestHelper.disableApiConfig().then(() => {
Expand Down
18 changes: 11 additions & 7 deletions test/unit/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,18 @@ describe("Session", function(done) {

TestHelper.startServer(pages).then(() => {
var client = request(connectionString)
var sessionName = config.get("sessions.name")

client
.get(pages[0].routes[0].path)
.expect(TestHelper.shouldNotHaveHeader("Set-Cookie"))
.end(function(err, res) {
if (err) return done(err)
done()
})
client.get(pages[0].routes[0].path).end(function(err, res) {
if (err) return done(err)

var cookieHeader = res.headers["set-cookie"]
if (cookieHeader) {
cookieHeader.indexOf(sessionName).should.equal(-1)
}

done()
})
})
})
})
Expand Down