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

chore: add csrf token handling #289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions docs/usage/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ request.get(restServiceMockUrl +'/usersWithAuth')
.auth('<user>','<password>').do();
```

## CSRF Tokens
// ---todo

# OData Helpers
Full OData ORM is out of scope but the following samples can simplify basic OData scenarios. For better oData support, please use [TBD]().

Expand Down
2 changes: 1 addition & 1 deletion e2e/scenario/api.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe('API tests', function() {
appMock = app;
})
});

afterAll(() => {
appMock.server.close();
});
Expand Down
20 changes: 19 additions & 1 deletion e2e/scenario/fixture/api.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,23 @@ describe('api', function() {
var res = request.get(restServiceMockUrl +'/usersWithAuth').auth('testUser','testPass');
expect(res).toHaveHTTPBody({status: 'Authenticated'});
});


it('Should set csrf token', function () {
request.post(restServiceMockUrl + '/form').send({
field: 'value'
}).do().catch(function (err) {
expect(err.status).toBe(403);
});

request.authenticate(new CsrfAuthenticator({
csrfFetchUrl: restServiceMockUrl + '/form'
}));

request.post(restServiceMockUrl + '/form').send({
field: 'value'
}).do().then(function (res) {
expect(res.status).toBe(200);
});
});

});
35 changes: 34 additions & 1 deletion e2e/scenario/fixture/mock/apiServiceMock.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
var express = require('express');
var bodyParser = require('body-parser')
var bodyParser = require('body-parser');
var cookieParser = require('cookie-parser');
var csrf = require('csurf');

module.exports = function() {
var app = express();
// will also use a _csrf cookie (secret) and validate against it
var csrfProtection = csrf({
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea to use standard module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the standard module?

cookie: true
});

var response = 1;
app.get('/user/', function(req, res) {
Expand Down Expand Up @@ -68,5 +74,32 @@ module.exports = function() {
res.send({deleted: req.params.user});
});

app.use(cookieParser());
app.use(function (err, req, res, next) {
if (err.code === 'EBADCSRFTOKEN') {
// CSRF token error
res.status(403);
res.send('form tampered with');
} else {
return next();
}
});

app.get('/form', csrfProtection, function (req, res) {
if (req.headers['x-csrf-token'].toLowerCase() === 'fetch') {
var csrfToken = req.csrfToken();
res.set('x-csrf-token', csrfToken);
res.send({
csrfToken: csrfToken
});
} else {
res.sendStatus(200);
}
});

app.post('/form', csrfProtection, function (req, res) {
res.send('data is being processed')
});

return app;
};
4 changes: 4 additions & 0 deletions e2e/scenario/fixture/mock/mockServer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
var Runner = require('../../../Runner');
var restServiceMock = require('./apiServiceMock');

Runner.startApp(restServiceMock);
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
"yargs": "^3.8.0"
},
"devDependencies": {
"cookie-parser": "^1.4.5",
"csurf": "^1.11.0",
"eslint": "^5.2.0",
"express": "^4.13.4",
"grunt": "^1.0.2",
Expand Down
39 changes: 39 additions & 0 deletions src/api/csrfAuthenticator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
var logger = require('../logger')(3);

var CSRF_HEADER = 'x-csrf-token';

function CsrfAuthenticator(options){
Copy link
Contributor

Choose a reason for hiding this comment

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

I am also fine with declarative setup like request.authenticate({crsf: {user,pass,url})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no user or pass that could be associated with csrf

Copy link
Contributor

Choose a reason for hiding this comment

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

There is for sure, the call for getting the crsf token should be authenticated, see the reference impl: https://github.wdf.sap.corp/I533952/uiveri5_iscoper/blob/9f08dcbbf05b692184aa619266ea1b3904ad8e61/iscoperPlugin.js#L43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for this link but as you know I've had it for 2 months and looked at it every day since.
please read a bit more about csrf, bearer tokens and relaystate param (as it also came up) and that they are not the same e.g.
https://owasp.org/www-community/attacks/csrf
https://portswigger.net/web-security/csrf/tokens
https://portswigger.net/web-security/csrf
https://blogs.sap.com/2019/02/19/what-is-relaystate-in-saml-and-how-to-configure-relaystate-on-as-abap/
https://stackoverflow.com/questions/25838183/what-is-the-oauth-2-0-bearer-token-exactly/25843058

if you want to add support for authentication, it would be a separate topic. besides, user:auth in the url is accepted https://visionmedia.github.io/superagent/#authentication

options = options || {};
this.user = options.user;
this.pass = options.pass;
this.csrfHeader = options.csrfHeader;
this.csrfFetchUrl = options.csrfFetchUrl;
}

CsrfAuthenticator.prototype.authenticate = function () {
var that = this;
if (this.csrfFetchUrl) {
return request.get(this.csrfFetchUrl)
.set(CSRF_HEADER, 'Fetch')
.do()
.then(function (res) {
if (res.headers[CSRF_HEADER]) {
that.csrfHeader = res.headers[CSRF_HEADER];
} else {
logger.error('Cannot generate CSRF token: missing X-CSRF-Token header');
}
}).catch(function (err) {
logger.error('Error in generating CSRF token. Details: ' + err);
});
}
};

CsrfAuthenticator.prototype.modifyCall = function (req) {
if (this.csrfHeader) {
req.set(CSRF_HEADER, this.csrfHeader);
}
// superagent will add the cookies e.g. _csrf cookie
return req;
};

module.exports = CsrfAuthenticator;
17 changes: 15 additions & 2 deletions src/api/requestPlugin.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
var superagent = require('superagent');

var CsrfAuthenticator = require('./csrfAuthenticator');
function RequestPlugin() {

}
Expand All @@ -16,7 +16,20 @@ RequestPlugin.prototype.setup = function() {
};
};

global.request = superagent.agent().use(flow);
var request = superagent.agent().use(flow);

request.authenticate = function (authenticator) {
var originalPost = request.post;
request.post = function () {
authenticator.modifyCall(this);
return originalPost.apply(this, arguments);
};

return authenticator.authenticate();
};

global.request = request;
global.CsrfAuthenticator = CsrfAuthenticator;
};

module.exports = function () {
Expand Down