From 63e708b56f1803e1f4e40f5f2c0db65c1c6e8463 Mon Sep 17 00:00:00 2001 From: danocmx Date: Mon, 15 Jul 2024 14:39:58 +0200 Subject: [PATCH 1/3] feat: add nonce validation logic --- src/strategy.ts | 32 +++++++++++++++++++++++++++++++- src/type.ts | 9 +++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/strategy.ts b/src/strategy.ts index 08e6acd..3267e66 100644 --- a/src/strategy.ts +++ b/src/strategy.ts @@ -66,6 +66,14 @@ export class SteamOpenIdStrategy< */ protected verify?: VerifyCallback; + /** + * Optional setting for validating nonce time delay, + * in miliseconds. + * + * Measures time between nonce creation date and verification. + */ + protected maxNonceTimeDelay: number | undefined; + /** * @constructor * @@ -81,6 +89,7 @@ export class SteamOpenIdStrategy< this.axios = axios.create(); this.returnURL = options.returnURL; this.profile = options.profile; + this.maxNonceTimeDelay = options.maxNonceTimeDelay; if (options.profile) this.apiKey = options.apiKey; if (verify) this.verify = verify; } @@ -151,7 +160,12 @@ export class SteamOpenIdStrategy< ); } - // TODO: validate nonce time + if (this.hasNonceExpired(query)) { + throw new SteamOpenIdError( + 'Nonce time delay was too big.', + SteamOpenIdErrorType.NonceExpired, + ); + } const valid = await this.validateAgainstSteam(query); if (!valid) { @@ -165,6 +179,22 @@ export class SteamOpenIdStrategy< return await this.getUser(steamId); } + /** + * Check if nonce date has expired against current delay setting, + * if no setting was set, then it is considered as not expired. + * + * @param nonceDate date when nonce was created + * @returns true, if nonce has expired and error should be thrown + */ + protected hasNonceExpired(query: SteamOpenIdQuery): boolean { + if (!this.maxNonceTimeDelay) { + return false; + } + + const nonceDate = new Date(query['openid.response_nonce'].slice(0, 24)); + return Date.now() - nonceDate.getTime() > this.maxNonceTimeDelay; + } + /** * Checks if error is retryable, * meaning user gets redirected to steam openid page. diff --git a/src/type.ts b/src/type.ts index d67aa1c..9860f76 100644 --- a/src/type.ts +++ b/src/type.ts @@ -2,6 +2,11 @@ import { DoneCallback } from 'passport'; export type BaseSteamOpenIdStrategyOptions = { returnURL: string; + /** + * Maximum time delay between the nonce creation and the nonce verification, + * in miliseconds. + */ + maxNonceTimeDelay?: number; }; export type SteamOpenIdStrategyOptionsWithProfile = { @@ -39,6 +44,10 @@ export enum SteamOpenIdErrorType { * SteamId is not valid. */ InvalidSteamId = 3, + /** + * Nonce has expired. + */ + NonceExpired = 4, } /** When profile is not used, we just send a steamid. */ From f45d00157a9aa545b572a55898ce75a06a1f03b3 Mon Sep 17 00:00:00 2001 From: danocmx Date: Mon, 15 Jul 2024 14:55:56 +0200 Subject: [PATCH 2/3] test: add unit tests for nonce validation --- test/setup/data.ts | 20 ++++++++------ test/strategy.ts | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 8 deletions(-) diff --git a/test/setup/data.ts b/test/setup/data.ts index dcee058..17f4b29 100644 --- a/test/setup/data.ts +++ b/test/setup/data.ts @@ -1,12 +1,16 @@ -import { VALID_NONCE, VALID_OPENID_ENDPOINT } from '../../src'; +import { + SteamOpenIdQuery, + VALID_NONCE, + VALID_OPENID_ENDPOINT, +} from '../../src'; export const RETURN_URL = '/auth/steam'; export const query: { - properties: Record; - get(): Record; - change(change: Record): Record; - remove(property: string): Record; + properties: SteamOpenIdQuery; + get(): SteamOpenIdQuery; + change(change: Partial): SteamOpenIdQuery; + remove(property: keyof SteamOpenIdQuery): SteamOpenIdQuery; } = { /** * Valid query properties @@ -25,15 +29,15 @@ export const query: { 'openid.sig': 'dc6e2a79de2c6aceac495ad5f4c6b6e0bfe30', }, - get() { + get(): SteamOpenIdQuery { return { ...this.properties }; }, - change(change: Record) { + change(change: Partial): SteamOpenIdQuery { return { ...this.get(), ...change }; }, - remove(property: string) { + remove(property: keyof SteamOpenIdQuery): SteamOpenIdQuery { const properties = this.get(); delete properties[property]; return properties; diff --git a/test/strategy.ts b/test/strategy.ts index 6d21abd..cbad3f5 100644 --- a/test/strategy.ts +++ b/test/strategy.ts @@ -214,12 +214,14 @@ describe('SteamOpenIdStrategy Unit Test', () => { let getQueryStub: sinon.SinonStub; let hasAuthQueryStub: sinon.SinonStub; let isQueryValidStub: sinon.SinonStub; + let hasNonceExpiredStub: sinon.SinonStub; let validateAgainstSteamStub: sinon.SinonStub; beforeEach(() => { getQueryStub = sinon.stub(strategy as any, 'getQuery').returns(query); hasAuthQueryStub = sinon.stub(strategy as any, 'hasAuthQuery'); isQueryValidStub = sinon.stub(strategy as any, 'isQueryValid'); + hasNonceExpiredStub = sinon.stub(strategy as any, 'hasNonceExpired'); validateAgainstSteamStub = sinon.stub( strategy as any, 'validateAgainstSteam', @@ -237,6 +239,7 @@ describe('SteamOpenIdStrategy Unit Test', () => { hasAuthQueryStub.returns(true); isQueryValidStub.returns(true); validateAgainstSteamStub.resolves(true); + hasNonceExpiredStub.returns(false); const steamid = '76561197960435530'; const user = { steamid: '76561197960435530' }; @@ -253,6 +256,8 @@ describe('SteamOpenIdStrategy Unit Test', () => { expect(getUserStub.calledWithExactly(steamid)).equal(true); expect(isQueryValidStub.callCount).equal(1); expect(isQueryValidStub.calledWithExactly(query)).equal(true); + expect(hasNonceExpiredStub.callCount).equal(1); + expect(hasNonceExpiredStub.calledWithExactly(query)).equal(true); expect(validateAgainstSteamStub.callCount).equal(1); expect(validateAgainstSteamStub.calledWithExactly(query)).equal(true); }); @@ -269,6 +274,9 @@ describe('SteamOpenIdStrategy Unit Test', () => { expect(err).to.be.instanceOf(SteamOpenIdError); expect(err).to.have.property('code', SteamOpenIdErrorType.InvalidMode); + expect(isQueryValidStub.callCount).equal(0); + expect(hasNonceExpiredStub.callCount).equal(0); + expect(validateAgainstSteamStub.callCount).equal(0); }); it('Query is invalid', async () => { @@ -286,11 +294,35 @@ describe('SteamOpenIdStrategy Unit Test', () => { expect(err).to.have.property('code', SteamOpenIdErrorType.InvalidQuery); expect(isQueryValidStub.callCount).equal(1); expect(isQueryValidStub.calledWithExactly(query)).equal(true); + expect(hasNonceExpiredStub.callCount).equal(0); + expect(validateAgainstSteamStub.callCount).equal(0); + }); + + it('Nonce has expired', async () => { + hasAuthQueryStub.returns(true); + isQueryValidStub.returns(true); + hasNonceExpiredStub.returns(true); + + let err: any; + try { + await strategy.handleRequest(request); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceOf(SteamOpenIdError); + expect(err).to.have.property('code', SteamOpenIdErrorType.NonceExpired); + expect(isQueryValidStub.callCount).equal(1); + expect(isQueryValidStub.calledWithExactly(query)).equal(true); + expect(hasNonceExpiredStub.callCount).equal(1); + expect(hasNonceExpiredStub.calledWithExactly(query)).equal(true); + expect(validateAgainstSteamStub.callCount).equal(0); }); it('Steam rejects this authentication request', async () => { hasAuthQueryStub.returns(true); isQueryValidStub.returns(true); + hasNonceExpiredStub.returns(false); validateAgainstSteamStub.resolves(false); let err: any; @@ -304,6 +336,8 @@ describe('SteamOpenIdStrategy Unit Test', () => { expect(err).to.have.property('code', SteamOpenIdErrorType.Unauthorized); expect(isQueryValidStub.callCount).equal(1); expect(isQueryValidStub.calledWithExactly(query)).equal(true); + expect(hasNonceExpiredStub.callCount).equal(1); + expect(hasNonceExpiredStub.calledWithExactly(query)).equal(true); expect(validateAgainstSteamStub.callCount).equal(1); expect(validateAgainstSteamStub.calledWithExactly(query)).equal(true); }); @@ -722,4 +756,39 @@ describe('SteamOpenIdStrategy Unit Test', () => { expect(err).to.have.property('code', SteamOpenIdErrorType.InvalidSteamId); }); }); + + describe('hasNonceExpired', () => { + const HOUR = 1000 * 60 * 60; + + it('No setting set, could not expire', () => { + expect(strategy['hasNonceExpired'](query.properties)).equal(false); + }); + + it('Setting set, nonce expired', () => { + strategy['maxNonceTimeDelay'] = HOUR; + + expect( + strategy['hasNonceExpired']( + query.change({ + 'openid.response_nonce': `${new Date( + Date.now() - 2 * HOUR, + ).toJSON()}8df86bac92ad1addaf3735a5aabdc6e2a7`, + }), + ), + ).equal(true); + }); + + it('Setting set, nonce not expired', () => { + strategy['maxNonceTimeDelay'] = 1000 * 60 * 60; + expect( + strategy['hasNonceExpired']( + query.change({ + 'openid.response_nonce': `${new Date( + Date.now() - HOUR / 2, + ).toJSON()}8df86bac92ad1addaf3735a5aabdc6e2a7`, + }), + ), + ).equal(false); + }); + }); }); From d8a9f5a123a42c334dc3a6137fcdbc8c98a2c39e Mon Sep 17 00:00:00 2001 From: danocmx Date: Mon, 15 Jul 2024 15:42:43 +0200 Subject: [PATCH 3/3] fix: use seconds instead of milliseconds --- src/strategy.ts | 13 +++++++++---- src/type.ts | 7 ++++++- test/setup/data.ts | 4 +++- test/strategy.ts | 19 ++++++++++--------- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/strategy.ts b/src/strategy.ts index 3267e66..65dc4ac 100644 --- a/src/strategy.ts +++ b/src/strategy.ts @@ -68,7 +68,7 @@ export class SteamOpenIdStrategy< /** * Optional setting for validating nonce time delay, - * in miliseconds. + * in seconds. * * Measures time between nonce creation date and verification. */ @@ -80,6 +80,9 @@ export class SteamOpenIdStrategy< * @param options.returnURL where steam redirects after parameters are passed * @param options.profile if set, we will fetch user's profile from steam api * @param options.apiKey api key to fetch user profile, not used if profile is false + * @param options.maxNonceTimeDelay optional setting for validating nonce time delay, + * this is just an extra security measure, it is not required nor recommended, but + * might be extra layer of security you want to have. * @param verify optional callback, called when user is successfully authenticated */ constructor(options: TOptions, verify?: VerifyCallback) { @@ -187,12 +190,14 @@ export class SteamOpenIdStrategy< * @returns true, if nonce has expired and error should be thrown */ protected hasNonceExpired(query: SteamOpenIdQuery): boolean { - if (!this.maxNonceTimeDelay) { + if (typeof this.maxNonceTimeDelay == 'undefined') { return false; } - const nonceDate = new Date(query['openid.response_nonce'].slice(0, 24)); - return Date.now() - nonceDate.getTime() > this.maxNonceTimeDelay; + const nonceDate = new Date(query['openid.response_nonce'].slice(0, 20)); + const nonceSeconds = Math.floor(nonceDate.getTime() / 1000); + const nowSeconds = Math.floor(Date.now() / 1000); + return nowSeconds - nonceSeconds > this.maxNonceTimeDelay; } /** diff --git a/src/type.ts b/src/type.ts index 9860f76..4bf0a0e 100644 --- a/src/type.ts +++ b/src/type.ts @@ -4,7 +4,12 @@ export type BaseSteamOpenIdStrategyOptions = { returnURL: string; /** * Maximum time delay between the nonce creation and the nonce verification, - * in miliseconds. + * in seconds. + * + * nonce includes a timestamp we can validate against the current time, + * while the steam server validates this as well, you might want to + * set maximum number of seconds between the nonce creation and the nonce + * verification. */ maxNonceTimeDelay?: number; }; diff --git a/test/setup/data.ts b/test/setup/data.ts index 17f4b29..c1ab8c7 100644 --- a/test/setup/data.ts +++ b/test/setup/data.ts @@ -6,6 +6,8 @@ import { export const RETURN_URL = '/auth/steam'; +export const getISODate = (date: Date) => date.toISOString().split('.')[0] + 'Z'; + export const query: { properties: SteamOpenIdQuery; get(): SteamOpenIdQuery; @@ -22,7 +24,7 @@ export const query: { 'openid.claimed_id': `https://steamcommunity.com/openid/id/76561197960435530`, 'openid.return_to': RETURN_URL, 'openid.op_endpoint': VALID_OPENID_ENDPOINT, - 'openid.response_nonce': `${new Date().toJSON()}8df86bac92ad1addaf3735a5aabdc6e2a7`, + 'openid.response_nonce': `${getISODate(new Date())}8df86bac92ad1addaf3735a5aabdc6e2a7`, 'openid.assoc_handle': '1234567890', 'openid.signed': 'signed,op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle', diff --git a/test/strategy.ts b/test/strategy.ts index cbad3f5..b191032 100644 --- a/test/strategy.ts +++ b/test/strategy.ts @@ -15,7 +15,7 @@ import { VALID_OPENID_ENDPOINT, PLAYER_SUMMARY_URL, } from '../src'; -import { RETURN_URL, query } from './setup/data'; +import { RETURN_URL, getISODate, query } from './setup/data'; chai.use(chaiAsPromised); @@ -765,27 +765,28 @@ describe('SteamOpenIdStrategy Unit Test', () => { }); it('Setting set, nonce expired', () => { - strategy['maxNonceTimeDelay'] = HOUR; + strategy['maxNonceTimeDelay'] = HOUR / 1000; expect( strategy['hasNonceExpired']( query.change({ - 'openid.response_nonce': `${new Date( - Date.now() - 2 * HOUR, - ).toJSON()}8df86bac92ad1addaf3735a5aabdc6e2a7`, + 'openid.response_nonce': `${getISODate( + new Date(Date.now() - 2 * HOUR), + )}8df86bac92ad1addaf3735a5aabdc6e2a7`, }), ), ).equal(true); }); it('Setting set, nonce not expired', () => { - strategy['maxNonceTimeDelay'] = 1000 * 60 * 60; + strategy['maxNonceTimeDelay'] = HOUR / 1000; + expect( strategy['hasNonceExpired']( query.change({ - 'openid.response_nonce': `${new Date( - Date.now() - HOUR / 2, - ).toJSON()}8df86bac92ad1addaf3735a5aabdc6e2a7`, + 'openid.response_nonce': `${getISODate( + new Date(Date.now() + HOUR / 2), + )}8df86bac92ad1addaf3735a5aabdc6e2a7`, }), ), ).equal(false);