From 65fe31a8b4b88ffb3dd482942921b8e09bec7314 Mon Sep 17 00:00:00 2001 From: Matthew White Date: Wed, 12 Jul 2023 16:58:08 -0400 Subject: [PATCH] Add ability to specify CSRF token in header --- lib/http/preprocessors.js | 16 ++- test/integration/api/sessions.js | 74 ++++++++++---- test/unit/http/preprocessors.js | 167 ++++++++++++++++++++++++------- 3 files changed, 198 insertions(+), 59 deletions(-) diff --git a/lib/http/preprocessors.js b/lib/http/preprocessors.js index a4f84e062..1704128a0 100644 --- a/lib/http/preprocessors.js +++ b/lib/http/preprocessors.js @@ -118,14 +118,22 @@ const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => { // if authentication failed anyway, just do nothing. if ((cxt == null) || !cxt.auth.session.isDefined()) return; + // The CSRF token may be specified in a header or in the request body. We + // used to require the token to be in the request body, but that isn't + // compatible with all request bodies (for example, a file). There are + // times when Central specifies the token in a header, and other times + // when it specifies it in the request body: it depends on the use case. + const csrf = cxt.headers['x-csrf-token'] ?? cxt.body.__csrf; + // if csrf missing or mismatch; fail outright. - const csrf = cxt.body.__csrf; if (isBlank(csrf) || (cxt.auth.session.get().csrf !== decodeURIComponent(csrf))) return reject(Problem.user.authenticationFailed()); - // delete the token off the body so it doesn't mess with downstream - // payload expectations. - return cxt.with({ body: without([ '__csrf' ], cxt.body) }); + return cxt.headers['x-csrf-token'] != null + ? cxt + // delete the token off the body so it doesn't mess with downstream + // payload expectations. + : cxt.with({ body: without([ '__csrf' ], cxt.body) }); }); } }; diff --git a/test/integration/api/sessions.js b/test/integration/api/sessions.js index 004c663d6..5a5025368 100644 --- a/test/integration/api/sessions.js +++ b/test/integration/api/sessions.js @@ -271,7 +271,19 @@ describe('api: /sessions', () => { // this isn't exactly the right place for this but i just want to check the // whole stack in addition to the unit tests. - describe('cookie CSRF auth', () => { + describe('CSRF protection for cookie auth used for non-GET requests', () => { + it('should reject with a 403 if session token is invalid', testService(async (service) => { + const { body: session } = await service.post('/v1/sessions') + .send({ email: 'alice@getodk.org', password: 'alice' }) + .expect(200); + await service.post('/v1/projects') + .send({ name: 'my project' }) + .set('X-Forwarded-Proto', 'https') + .set('Cookie', '__Host-session=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa') + .set('X-Csrf-Token', session.csrf) + .expect(403); + })); + it('should reject if the CSRF token is missing', testService((service) => service.post('/v1/sessions') .send({ email: 'alice@getodk.org', password: 'alice' }) @@ -282,25 +294,53 @@ describe('api: /sessions', () => { .set('Cookie', '__Host-session=' + body.token) .expect(401)))); - it('should reject if the CSRF token is wrong', testService((service) => - service.post('/v1/sessions') - .send({ email: 'alice@getodk.org', password: 'alice' }) - .expect(200) - .then(({ body }) => service.post('/v1/projects') - .send({ name: 'my project', __csrf: 'nope' }) + describe('CSRF token in the request body', () => { + it('should reject if the CSRF token is wrong', testService((service) => + service.post('/v1/sessions') + .send({ email: 'alice@getodk.org', password: 'alice' }) + .expect(200) + .then(({ body }) => service.post('/v1/projects') + .send({ name: 'my project', __csrf: 'nope' }) + .set('X-Forwarded-Proto', 'https') + .set('Cookie', '__Host-session=' + body.token) + .expect(401)))); + + it('should succeed if the CSRF token is correct', testService((service) => + service.post('/v1/sessions') + .send({ email: 'alice@getodk.org', password: 'alice' }) + .expect(200) + .then(({ body }) => service.post('/v1/projects') + .send({ name: 'my project', __csrf: body.csrf }) + .set('X-Forwarded-Proto', 'https') + .set('Cookie', '__Host-session=' + body.token) + .expect(200)))); + }); + + describe('CSRF token in a header', () => { + it('should reject if the token is incorrect', testService(async (service) => { + const { body: session } = await service.post('/v1/sessions') + .send({ email: 'alice@getodk.org', password: 'alice' }) + .expect(200); + await service.post('/v1/projects') + .send({ name: 'my project' }) .set('X-Forwarded-Proto', 'https') - .set('Cookie', '__Host-session=' + body.token) - .expect(401)))); + .set('Cookie', `__Host-session=${session.token}`) + .set('X-Csrf-Token', 'nope') + .expect(401); + })); - it('should succeed if the CSRF token is correct', testService((service) => - service.post('/v1/sessions') - .send({ email: 'alice@getodk.org', password: 'alice' }) - .expect(200) - .then(({ body }) => service.post('/v1/projects') - .send({ name: 'my project', __csrf: body.csrf }) + it('should succeed if the token is correct', testService(async (service) => { + const { body: session } = await service.post('/v1/sessions') + .send({ email: 'alice@getodk.org', password: 'alice' }) + .expect(200); + await service.post('/v1/projects') + .send({ name: 'my project' }) .set('X-Forwarded-Proto', 'https') - .set('Cookie', '__Host-session=' + body.token) - .expect(200)))); + .set('Cookie', `__Host-session=${session.token}`) + .set('X-Csrf-Token', session.csrf) + .expect(200); + })); + }); }); }); diff --git a/test/unit/http/preprocessors.js b/test/unit/http/preprocessors.js index 1feb478cd..9c8eb259c 100644 --- a/test/unit/http/preprocessors.js +++ b/test/unit/http/preprocessors.js @@ -279,7 +279,7 @@ describe('preprocessors', () => { context.auth.session.should.eql(Option.of(new Session({ test: 'session' }))); })); - describe('CSRF protection', () => { + describe('CSRF protection for non-GET requests', () => { const mockSessionsWithCsrf = (expectedToken, csrf) => ({ getByBearerToken: (token) => Promise.resolve((token === expectedToken) ? Option.of(new Session({ csrf })) @@ -298,18 +298,6 @@ describe('preprocessors', () => { ) )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); - it('should reject cookie auth with incorrect CSRF token for non-GET requests', () => - Promise.resolve(authHandler( - { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, - new Context( - createRequest({ method: 'POST', headers: { - 'X-Forwarded-Proto': 'https', - Cookie: '__Host-session=alohomora' - }, body: { __csrf: 'notsecretcsrf' } }), - { fieldKey: Option.none() } - ) - )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); - it('should do nothing on cookie auth with incorrect session token for non-GET requests', () => Promise.resolve(authHandler( { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, @@ -325,17 +313,134 @@ describe('preprocessors', () => { should.not.exist(context); })); - it('should accept cookie auth with correct CSRF token for non-GET requests', () => - Promise.resolve(authHandler( - { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, - new Context( - createRequest({ method: 'POST', headers: { - 'X-Forwarded-Proto': 'https', - Cookie: '__Host-session=alohomora' - }, body: { __csrf: 'secretcsrf' } }), - { fieldKey: Option.none() } - ) - )).should.be.fulfilled()); + describe('CSRF token in request body', () => { + it('should reject cookie auth with incorrect CSRF token for non-GET requests', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ method: 'POST', headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora' + }, body: { __csrf: 'notsecretcsrf' } }), + { fieldKey: Option.none() } + ) + )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); + + it('should accept cookie auth with correct CSRF token for non-GET requests', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ method: 'POST', headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora' + }, body: { __csrf: 'secretcsrf' } }), + { fieldKey: Option.none() } + ) + )).should.be.fulfilled()); + + it('should remove CSRF token from data payload on success', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ method: 'POST', headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora' + }, body: { __csrf: 'secretcsrf', other: 'data' } }), + { fieldKey: Option.none() } + ) + )).then((context) => { + context.body.should.eql({ other: 'data' }); + })); + }); + + describe('CSRF token in header', () => { + it('should reject if the token is incorrect', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ + method: 'POST', + headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora', + 'X-Csrf-Token': 'notsecretcsrf' + } + }), + { fieldKey: Option.none() } + ) + )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); + + it('should accept if the token is correct', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ + method: 'POST', + headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora', + 'X-Csrf-Token': 'secretcsrf' + } + }), + { fieldKey: Option.none() } + ) + )).should.be.fulfilled()); + }); + + describe('precedence of CSRF token in header over request body', () => { + it('should accept if token in header is correct even if token in request body is not', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ + method: 'POST', + headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora', + 'X-Csrf-Token': 'secretcsrf' + }, + body: { __csrf: 'notsecretcsrf' } + }), + { fieldKey: Option.none() } + ) + )).should.be.fulfilled()); + + it('should reject if token in header is incorrect even if token in request body is correct', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ + method: 'POST', + headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora', + 'X-Csrf-Token': 'notsecretcsrf' + }, + body: { __csrf: 'secretcsrf' } + }), + { fieldKey: Option.none() } + ) + )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); + + it('should not remove token from request body if header was specified', async () => { + const context = await authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ + method: 'POST', + headers: { + 'X-Forwarded-Proto': 'https', + Cookie: '__Host-session=alohomora', + 'X-Csrf-Token': 'secretcsrf' + }, + body: { __csrf: 'secretcsrf', other: 'data' } + }), + { fieldKey: Option.none() } + ) + ); + context.body.should.eql({ __csrf: 'secretcsrf', other: 'data' }); + }); + }); it('should url-decode the CSRF token', () => Promise.resolve(authHandler( @@ -348,20 +453,6 @@ describe('preprocessors', () => { { fieldKey: Option.none() } ) )).should.be.fulfilled()); - - it('should remove CSRF token from data payload on success', () => - Promise.resolve(authHandler( - { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, - new Context( - createRequest({ method: 'POST', headers: { - 'X-Forwarded-Proto': 'https', - Cookie: '__Host-session=alohomora' - }, body: { __csrf: 'secretcsrf', other: 'data' } }), - { fieldKey: Option.none() } - ) - )).then((context) => { - context.body.should.eql({ other: 'data' }); - })); }); });