From 306ff79182609fb38bada5d46cef17857534519b Mon Sep 17 00:00:00 2001 From: Jesse Chan Date: Mon, 26 Oct 2020 20:05:30 +0800 Subject: [PATCH] API: auth: don't include token in JSON objects Token is already sent by Set-Cookie. It is unneccessary and insecure to include them in JSON response. Doing so also introduce the token into Javascript VM which is not protected as well as the httpOnly cookies. --- client/src/javascript/stores/AuthStore.ts | 3 -- server/routes/api/auth.test.ts | 52 ++++++++++------------- server/routes/api/auth.ts | 7 +-- shared/schema/api/auth.ts | 1 - 4 files changed, 26 insertions(+), 37 deletions(-) diff --git a/client/src/javascript/stores/AuthStore.ts b/client/src/javascript/stores/AuthStore.ts index e07a9ad7..9bf2572a 100644 --- a/client/src/javascript/stores/AuthStore.ts +++ b/client/src/javascript/stores/AuthStore.ts @@ -11,7 +11,6 @@ import FloodActions from '../actions/FloodActions'; class AuthStore { isAuthenticating = false; isAuthenticated = false; - token: string | null | undefined = null; users: Array = []; optimisticUsers: Array<{username: string}> = []; currentUser: { @@ -43,13 +42,11 @@ class AuthStore { this.currentUser.username = response.username; this.currentUser.isAdmin = response.level === AccessLevel.ADMINISTRATOR; this.currentUser.isInitialUser = false; - this.token = response.token; this.isAuthenticating = true; this.isAuthenticated = true; } handleLoginError(): void { - this.token = null; this.isAuthenticated = false; this.isAuthenticating = true; } diff --git a/server/routes/api/auth.test.ts b/server/routes/api/auth.test.ts index f9d3c332..35cc5a55 100644 --- a/server/routes/api/auth.test.ts +++ b/server/routes/api/auth.test.ts @@ -7,7 +7,6 @@ import app from '../../app'; import {getAuthToken} from './auth'; import type { - AuthAuthenticationResponse, AuthRegistrationOptions, AuthUpdateUserOptions, AuthVerificationResponse, @@ -68,15 +67,12 @@ describe('POST /api/auth/register', () => { .set('Accept', 'application/json') .expect(200) .expect('Content-Type', /json/) + .expect('Set-Cookie', /jwt=.*;/) .end((err, res) => { if (err) done(err); - const authResponse: AuthAuthenticationResponse = res.body; - - expect(typeof authResponse.token === 'string').toBe(true); - expect(authResponse.token.includes('JWT')).toBe(true); - - [, testAdminUserToken] = authResponse.token.split('JWT '); + [testAdminUserToken] = res.headers['set-cookie']; + expect(typeof testAdminUserToken).toBe('string'); done(); }); @@ -104,19 +100,15 @@ describe('POST /api/auth/register', () => { .post('/api/auth/register') .send(options) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .expect('Content-Type', /json/) - .expect('Set-Cookie', /jwt/) + .expect('Set-Cookie', /jwt=.*;/) .end((err, res) => { if (err) done(err); - const authResponse: AuthAuthenticationResponse = res.body; - - expect(typeof authResponse.token === 'string').toBe(true); - expect(authResponse.token.includes('JWT')).toBe(true); - - [, testNonAdminUserToken] = authResponse.token.split('JWT '); + [testNonAdminUserToken] = res.headers['set-cookie']; + expect(typeof testNonAdminUserToken).toBe('string'); done(); }); @@ -128,7 +120,7 @@ describe('POST /api/auth/register', () => { .post('/api/auth/register') .send(options) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testNonAdminUserToken}`]) + .set('Cookie', [testNonAdminUserToken]) .expect(403) .end((err, res) => { if (err) done(err); @@ -145,7 +137,7 @@ describe('POST /api/auth/register', () => { .post('/api/auth/register') .send(options) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(500) .expect('Content-Type', /json/) .end((err, res) => { @@ -166,7 +158,7 @@ describe('POST /api/auth/register', () => { .post('/api/auth/register?cookie=false') .send(options) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .expect('Content-Type', /json/) .end((err, res) => { @@ -189,7 +181,7 @@ describe('POST /api/auth/register', () => { }, }) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(422) .expect('Content-Type', /json/) .end((err, res) => { @@ -220,7 +212,7 @@ describe('GET /api/auth/verify', () => { .get('/api/auth/verify') .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .end((err, res) => { if (err) done(err); @@ -259,7 +251,7 @@ describe('GET /api/auth/logout', () => { .get('/api/auth/logout') .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .expect('Set-Cookie', /jwt=;/) .end((err, _res) => { @@ -349,7 +341,7 @@ describe('GET /api/auth/users', () => { .get('/api/auth/users') .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testNonAdminUserToken}`]) + .set('Cookie', [testNonAdminUserToken]) .expect(403) .end((err, res) => { if (err) done(err); @@ -365,7 +357,7 @@ describe('GET /api/auth/users', () => { .get('/api/auth/users') .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .end((err, res) => { if (err) done(err); @@ -393,7 +385,7 @@ describe('PATCH /api/auth/users/{username}', () => { .patch(`/api/auth/users/${`nonExistentUser`}`) .send(patch) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(500) .end((err, _res) => { if (err) done(err); @@ -406,7 +398,7 @@ describe('PATCH /api/auth/users/{username}', () => { .patch(`/api/auth/users/${testAdminUser.username}`) .send(patch) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testNonAdminUserToken}`]) + .set('Cookie', [testNonAdminUserToken]) .expect(403) .end((err, _res) => { if (err) done(err); @@ -419,7 +411,7 @@ describe('PATCH /api/auth/users/{username}', () => { .patch(`/api/auth/users/${testNonAdminUser.username}`) .send(patch) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .end((err, _res) => { if (err) done(err); @@ -437,7 +429,7 @@ describe('PATCH /api/auth/users/{username}', () => { }, }) .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(422) .end((err, _res) => { if (err) done(err); @@ -452,7 +444,7 @@ describe('DELETE /api/auth/users/{username}', () => { .delete(`/api/auth/users/${`nonExistentUser`}`) .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(500) .end((err, _res) => { if (err) done(err); @@ -465,7 +457,7 @@ describe('DELETE /api/auth/users/{username}', () => { .delete(`/api/auth/users/${testAdminUser.username}`) .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testNonAdminUserToken}`]) + .set('Cookie', [testNonAdminUserToken]) .expect(403) .end((err, _res) => { if (err) done(err); @@ -478,7 +470,7 @@ describe('DELETE /api/auth/users/{username}', () => { .delete(`/api/auth/users/${testNonAdminUser.username}`) .send() .set('Accept', 'application/json') - .set('Cookie', [`jwt=${testAdminUserToken}`]) + .set('Cookie', [testAdminUserToken]) .expect(200) .end((err, res) => { if (err) done(err); diff --git a/server/routes/api/auth.ts b/server/routes/api/auth.ts index 9fbfcd6c..ea316e2c 100644 --- a/server/routes/api/auth.ts +++ b/server/routes/api/auth.ts @@ -56,11 +56,11 @@ const sendAuthenticationResponse = ( credentials: Required>, ): void => { const {username, level} = credentials; - const token = getAuthToken(username, res); + + getAuthToken(username, res); const response: AuthAuthenticationResponse = { success: true, - token: `JWT ${token}`, username, level, }; @@ -192,7 +192,8 @@ router.use('/verify', (req, res, next) => { // Unconditionally provide a token if auth is disabled if (config.disableUsersAndAuth) { const {username, level} = Users.getConfigUser(); - const token = getAuthToken(username, res); + + getAuthToken(username, res); const response: AuthVerificationResponse = { initialUser: false, diff --git a/shared/schema/api/auth.ts b/shared/schema/api/auth.ts index 046cb471..cbdf1171 100644 --- a/shared/schema/api/auth.ts +++ b/shared/schema/api/auth.ts @@ -11,7 +11,6 @@ export type AuthAuthenticationOptions = Required