From 501637bfc189e31d0f5a4334a54b6169fb667133 Mon Sep 17 00:00:00 2001 From: Dave Liu <7david12liu@gmail.com> Date: Tue, 14 Jul 2026 00:22:54 -0700 Subject: [PATCH] Bound Strava authorization codes --- functions/strava.js | 9 ++- functions/strava.test.js | 138 +++++++++++++++++++++++++++++++++------ 2 files changed, 124 insertions(+), 23 deletions(-) diff --git a/functions/strava.js b/functions/strava.js index 7a50a3e..1f5064e 100644 --- a/functions/strava.js +++ b/functions/strava.js @@ -8,6 +8,7 @@ const STRAVA_DEAUTH_URL = 'https://www.strava.com/oauth/deauthorize'; const STRAVA_ACTIVITIES_URL = 'https://www.strava.com/api/v3/athlete/activities'; const STRAVA_STATS_URL = (id) => `https://www.strava.com/api/v3/athletes/${id}/stats`; const STRAVA_AUTHORIZATION_ERROR_MESSAGE = 'Strava authorization could not be completed.'; +const STRAVA_AUTHORIZATION_CODE_MAX_LENGTH = 1_024; const STRAVA_REFRESH_ERROR_MESSAGE = 'Strava connection could not be refreshed.'; const STRAVA_DATA_ERROR_MESSAGE = 'Strava activity data could not be loaded.'; @@ -131,8 +132,12 @@ exports.stravaExchangeCode = functions throw new functions.https.HttpsError('unauthenticated', 'Sign-in required'); } const { code } = data || {}; - if (!code || typeof code !== 'string') { - throw new functions.https.HttpsError('invalid-argument', 'code required'); + if ( + typeof code !== 'string' + || code.length === 0 + || code.length > STRAVA_AUTHORIZATION_CODE_MAX_LENGTH + ) { + throw stravaAuthorizationError('invalid-argument'); } const { uid } = context.auth; diff --git a/functions/strava.test.js b/functions/strava.test.js index a0a6b76..d1a19fc 100644 --- a/functions/strava.test.js +++ b/functions/strava.test.js @@ -98,6 +98,7 @@ const CONTEXT = Object.freeze({ auth: Object.freeze({ uid: 'synthetic-member-000001' }), }); const CODE = 'synthetic_authorization_code'; +const MAX_AUTHORIZATION_CODE_LENGTH = 1_024; const CONNECTION_PATH = 'members/synthetic-member-000001/connections/strava'; const SECRET_PATH = 'members/synthetic-member-000001/secrets/strava'; @@ -126,6 +127,7 @@ describe('Strava authorization exchange failure boundary', () => { beforeEach(() => { process.env.STRAVA_CLIENT_ID = 'strava_client_test'; process.env.STRAVA_CLIENT_SECRET = 'strava_secret_test'; + admin.__clearDeletes(); admin.__clearDocuments(); admin.__clearReads(); admin.__clearWrites(); @@ -144,11 +146,31 @@ describe('Strava authorization exchange failure boundary', () => { }); function expectNoSideEffects() { + expect(admin.__getDeletes()).toEqual([]); expect(admin.__getReads()).toEqual([]); expect(admin.__getWrites()).toEqual([]); consoleSpies.forEach((spy) => expect(spy).not.toHaveBeenCalled()); } + function mockSuccessfulExchange() { + fetchMock.mockResolvedValue({ + ok: true, + json: jest.fn().mockResolvedValue({ + access_token: 'access_token_test', + refresh_token: 'refresh_token_test', + expires_at: 1_900_000_000, + scope: 'read', + athlete: { + id: 123456, + firstname: 'Synthetic', + lastname: 'Athlete', + username: 'synthetic-athlete', + profile: 'https://images.example.test/synthetic-athlete.png', + }, + }), + }); + } + test('runs App Check before Auth, provider exchange, or Firestore', async () => { const appCheckFailure = new functions.https.HttpsError( 'failed-precondition', @@ -158,15 +180,19 @@ describe('Strava authorization exchange failure boundary', () => { throw appCheckFailure; }); - await expect(stravaExchangeCode({ code: CODE }, CONTEXT)).rejects.toBe(appCheckFailure); + await expect(stravaExchangeCode({ + code: 'x'.repeat(MAX_AUTHORIZATION_CODE_LENGTH + 1), + }, CONTEXT)).rejects.toBe(appCheckFailure); expect(requireAppCheck).toHaveBeenCalledWith(CONTEXT); expect(fetchMock).not.toHaveBeenCalled(); expectNoSideEffects(); }); - test('rejects a missing caller before provider exchange or Firestore', async () => { - await expect(stravaExchangeCode({ code: CODE }, { ...CONTEXT, auth: null })) + test('rejects a missing caller before code validation, provider exchange, or Firestore', async () => { + await expect(stravaExchangeCode({ + code: 'x'.repeat(MAX_AUTHORIZATION_CODE_LENGTH + 1), + }, { ...CONTEXT, auth: null })) .rejects.toMatchObject({ code: 'unauthenticated' }); expect(requireAppCheck).toHaveBeenCalledTimes(1); @@ -178,14 +204,99 @@ describe('Strava authorization exchange failure boundary', () => { delete process.env.STRAVA_CLIENT_ID; delete process.env.STRAVA_CLIENT_SECRET; - await expect(stravaExchangeCode({}, CONTEXT)) - .rejects.toMatchObject({ code: 'invalid-argument' }); + const error = await captureFailure(() => stravaExchangeCode({}, CONTEXT)); + expect(publicError(error)).toEqual({ + code: 'invalid-argument', + message: FIXED_AUTHORIZATION_ERROR, + details: undefined, + cause: undefined, + }); expect(requireAppCheck).toHaveBeenCalledTimes(1); expect(fetchMock).not.toHaveBeenCalled(); expectNoSideEffects(); }); + test.each([ + ['an undefined', undefined], + ['a null', null], + ['an empty string', ''], + ['a boolean', false], + ['a number', 0], + ['an array', []], + ['a plain object', {}], + ['a boxed string', Object('synthetic-boxed-code')], + ])('rejects %s code before reading credentials, provider access, or Firestore', async ( + _case, + code, + ) => { + delete process.env.STRAVA_CLIENT_ID; + delete process.env.STRAVA_CLIENT_SECRET; + + const error = await captureFailure(() => stravaExchangeCode({ code }, CONTEXT)); + + expect(publicError(error)).toEqual({ + code: 'invalid-argument', + message: FIXED_AUTHORIZATION_ERROR, + details: undefined, + cause: undefined, + }); + expect(fetchMock).not.toHaveBeenCalled(); + expectNoSideEffects(); + }); + + test('rejects an oversized code before provider access or Firestore', async () => { + mockSuccessfulExchange(); + + const error = await captureFailure(() => stravaExchangeCode({ + code: 'x'.repeat(MAX_AUTHORIZATION_CODE_LENGTH + 1), + }, CONTEXT)); + + expect(publicError(error)).toEqual({ + code: 'invalid-argument', + message: FIXED_AUTHORIZATION_ERROR, + details: undefined, + cause: undefined, + }); + expect(fetchMock).not.toHaveBeenCalled(); + expectNoSideEffects(); + }); + + test('rejects an oversized code before reading credentials', async () => { + delete process.env.STRAVA_CLIENT_ID; + delete process.env.STRAVA_CLIENT_SECRET; + + const error = await captureFailure(() => stravaExchangeCode({ + code: 'x'.repeat(MAX_AUTHORIZATION_CODE_LENGTH + 1), + }, CONTEXT)); + + expect(publicError(error)).toEqual({ + code: 'invalid-argument', + message: FIXED_AUTHORIZATION_ERROR, + details: undefined, + cause: undefined, + }); + expect(fetchMock).not.toHaveBeenCalled(); + expectNoSideEffects(); + }); + + test.each([ + ['one character', 'x'], + ['exact maximum length', 'x'.repeat(MAX_AUTHORIZATION_CODE_LENGTH)], + ['opaque whitespace, Unicode, case, and punctuation', ' \tΩaA+/_-?&= '], + ])('preserves an admitted %s code-unit-for-code-unit', async (_case, code) => { + mockSuccessfulExchange(); + + const result = await stravaExchangeCode({ code }, CONTEXT); + + expect(result).toEqual({ ok: true, athleteId: 123456 }); + expect(fetchMock).toHaveBeenCalledTimes(1); + const request = fetchMock.mock.calls[0][1]; + expect(JSON.parse(request.body).code).toBe(code); + expect(admin.__getWrites()).toHaveLength(2); + consoleSpies.forEach((spy) => expect(spy).not.toHaveBeenCalled()); + }); + test('does not read or expose a failed provider response body or status', async () => { const text = jest.fn().mockResolvedValue( 'provider-body-canary refresh_token=provider-secret-canary', @@ -254,22 +365,7 @@ describe('Strava authorization exchange failure boundary', () => { }); test('preserves the successful server-only exchange, writes, and minimal result', async () => { - fetchMock.mockResolvedValue({ - ok: true, - json: jest.fn().mockResolvedValue({ - access_token: 'access_token_test', - refresh_token: 'refresh_token_test', - expires_at: 1_900_000_000, - scope: 'read', - athlete: { - id: 123456, - firstname: 'Synthetic', - lastname: 'Athlete', - username: 'synthetic-athlete', - profile: 'https://images.example.test/synthetic-athlete.png', - }, - }), - }); + mockSuccessfulExchange(); const result = await stravaExchangeCode({ code: CODE }, CONTEXT);