Skip to content

Commit

Permalink
Fix issue with CSRF black/whitelists and routes containing optional p…
Browse files Browse the repository at this point in the history
…arams when the requested URL contains a querystring but NOT the optional param (whew!)

Also add tests for the above case and others involving querystrings.
  • Loading branch information
sgress454 committed Jan 26, 2018
1 parent 663527f commit b3afed7
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 4 deletions.
4 changes: 2 additions & 2 deletions lib/hooks/security/csrf/index.js
Expand Up @@ -87,7 +87,7 @@ module.exports = function(sails) {
if (sails.config.security.csrf === false) {
// If nothing in the whitelist matches, continue on without checking for a CSRF token.
if (!_.any(whitelist, function(whitelistedRoute) {
return req.url.match(whitelistedRoute.regex) && (!whitelistedRoute.method || whitelistedRoute.method === req.method.toLowerCase());
return req.path.match(whitelistedRoute.regex) && (!whitelistedRoute.method || whitelistedRoute.method === req.method.toLowerCase());
})) {
return next();
}
Expand All @@ -96,7 +96,7 @@ module.exports = function(sails) {
else {
// If anything in the blacklist matches, continue on without checking for a CSRF token.
if (_.any(blacklist, function(blacklistedRoute) {
return req.url.match(blacklistedRoute.regex) && (!blacklistedRoute.method || blacklistedRoute.method === req.method.toLowerCase());
return req.path.match(blacklistedRoute.regex) && (!blacklistedRoute.method || blacklistedRoute.method === req.method.toLowerCase());
})) {
return next();
}
Expand Down
51 changes: 49 additions & 2 deletions test/integration/hook.csrf.test.js
Expand Up @@ -168,7 +168,7 @@ describe('CSRF ::', function() {
});
});

describe('with CSRF set to true, blacklisting \'POST /foo/:id, /user\'}', function() {
describe('with CSRF set to true, blacklisting \'POST /foo/:id, POST /bar/:id?, /user\'}', function() {

before(function() {

Expand All @@ -177,7 +177,9 @@ describe('CSRF ::', function() {
csrf: true
},
routes: {
// Note -- since we don't actually define a target for these, requests that pass CSRF should return a 404.
'POST /foo/:id': {csrf: false},
'/bar/:id?': {csrf: false},
'/user': {csrf: false}
}
};
Expand All @@ -202,7 +204,52 @@ describe('CSRF ::', function() {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + err || response.body));
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

it('a POST request on /foo/12?abc=123 without a CSRF token should result in a 404 response', function(done) {
sailsApp.request({url: '/foo/12?abc=123', method: 'post'}, function(err, response) {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

it('a POST request on /bar/12 without a CSRF token should result in a 404 response', function(done) {
sailsApp.request({url: '/bar/12', method: 'post'}, function(err, response) {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

it('a POST request on /bar/12?abc=123 without a CSRF token should result in a 404 response', function(done) {
sailsApp.request({url: '/bar/12?abc=123', method: 'post'}, function(err, response) {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

it('a POST request on /bar without a CSRF token should result in a 404 response', function(done) {
sailsApp.request({url: '/bar', method: 'post'}, function(err, response) {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

it('a POST request on /bar?abc=123 without a CSRF token should result in a 404 response', function(done) {
sailsApp.request({url: '/bar?abc=123', method: 'post'}, function(err, response) {
if (err && err.status === 404) {
return done();
}
done(new Error('Expected a 404 error, instead got: ' + (err || response.body)));
});
});

Expand Down

0 comments on commit b3afed7

Please sign in to comment.