From 970dc5601e0d187093c8ae5d6e38fb485f6ada59 Mon Sep 17 00:00:00 2001 From: Kevin O'Sullivan Date: Thu, 28 Jul 2022 22:06:56 -0400 Subject: [PATCH 1/3] Store state instead of session id in cookie Rather than create a temporary session in order to store a session id in a cookie for the initial OAuth transaction, we can store the state that can be compared against the state provided by Shopify in the callback, and then create the session at that point instead. Fixes https://github.com/Shopify/first-party-library-planning/issues/388 --- CHANGELOG.md | 1 + src/auth/oauth/__tests__/oauth.test.ts | 157 ++++++++++--------------- src/auth/oauth/oauth.ts | 109 ++++++++--------- 3 files changed, 111 insertions(+), 156 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d331f0c0..894c47228 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). - Add support for billing to the library [#449](https://github.com/Shopify/shopify-api-node/pull/449) - Allow dynamically typing the body of REST and GraphQL request responses, so callers don't need to cast it [#447](https://github.com/Shopify/shopify-api-node/pull/447) +- Rather than create a temporary session in order to store a session id in a cookie for the OAuth transaction, we can store the `state` in the cookie instead, that can then be compared against the `state` provided by Shopify in the callback. [#438](https://github.com/Shopify/shopify-api-node/pull/438) ## [5.0.1] - 2022-08-03 diff --git a/src/auth/oauth/__tests__/oauth.test.ts b/src/auth/oauth/__tests__/oauth.test.ts index 5ce455d89..c4037aae1 100644 --- a/src/auth/oauth/__tests__/oauth.test.ts +++ b/src/auth/oauth/__tests__/oauth.test.ts @@ -6,14 +6,16 @@ import Cookies from 'cookies'; import {ShopifyOAuth} from '../oauth'; import {Context} from '../../../context'; +import nonce from '../../../utils/nonce'; import * as ShopifyErrors from '../../../error'; import {AuthQuery} from '../types'; import {generateLocalHmac} from '../../../utils/hmac-validator'; import {JwtPayload} from '../../../utils/decode-session-token'; import loadCurrentSession from '../../../utils/load-current-session'; -import {CustomSessionStorage, Session} from '../../session'; +import {CustomSessionStorage} from '../../session'; jest.mock('cookies'); +jest.mock('../../../utils/nonce', () => jest.fn(() => 'noncenoncenonce')); let shop: string; @@ -39,7 +41,7 @@ describe('beginAuth', () => { Cookies.prototype.set.mockImplementation( (cookieName: string, cookieValue: string) => { - expect(cookieName).toBe('shopify_app_session'); + expect(cookieName).toBe('shopify_app_state'); cookies.id = cookieValue; }, ); @@ -53,42 +55,11 @@ describe('beginAuth', () => { ).rejects.toThrow(ShopifyErrors.UninitializedContextError); }); - test('throws SessionStorageErrors when storeSession returns false', async () => { - const storage = new CustomSessionStorage( - () => Promise.resolve(false), - () => Promise.resolve(new Session('id', shop, 'state', true)), - () => Promise.resolve(true), - ); - Context.SESSION_STORAGE = storage; - - await expect( - ShopifyOAuth.beginAuth(req, res, shop, 'some-callback'), - ).rejects.toThrow(ShopifyErrors.SessionStorageError); - }); - - test('creates and stores a new session for the specified shop', async () => { - const authRoute = await ShopifyOAuth.beginAuth( - req, - res, - shop, - '/some-callback', - ); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); - - expect(Cookies).toHaveBeenCalledTimes(1); - expect(Cookies.prototype.set).toHaveBeenCalledTimes(1); - expect(authRoute).toBeDefined(); - expect(session).toBeDefined(); - expect(session).toHaveProperty('id'); - expect(session).toHaveProperty('shop', shop); - expect(session).toHaveProperty('state'); - expect(session).toHaveProperty('expires', undefined); - }); - - test('sets session id and cookie to shop name prefixed with "offline_" for offline access requests', async () => { + test('sets cookie to state for offline access requests', async () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); - expect(cookies.id).toBe(`offline_${shop}`); + expect(nonce).toHaveBeenCalled(); + expect(cookies.id).toBe(`noncenoncenonce`); }); test('returns the correct auth url for given info', async () => { @@ -99,13 +70,12 @@ describe('beginAuth', () => { '/some-callback', false, ); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const query = { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `${Context.HOST_SCHEME}://${Context.HOST_NAME}/some-callback`, - state: session ? session.state : '', + state: 'noncenoncenonce', 'grant_options[]': '', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -126,13 +96,12 @@ describe('beginAuth', () => { '/some-callback', false, ); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const query = { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `http://${Context.HOST_NAME}/some-callback`, - state: session ? session.state : '', + state: 'noncenoncenonce', 'grant_options[]': '', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -152,14 +121,13 @@ describe('beginAuth', () => { '/some-callback', true, ); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const query = { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `${Context.HOST_SCHEME}://${Context.HOST_NAME}/some-callback`, - state: session ? session.state : '', + state: 'noncenoncenonce', 'grant_options[]': 'per-user', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -202,7 +170,9 @@ describe('validateAuthCallback', () => { Cookies.prototype.set.mockImplementation( (cookieName: string, cookieValue: string, options: {expires: Date}) => { - expect(cookieName).toBe('shopify_app_session'); + expect(cookieName).toEqual( + expect.stringMatching(/^shopify_app_(session|state)/), + ); cookies.id = cookieValue; cookies.expires = options.expires; }, @@ -213,10 +183,9 @@ describe('validateAuthCallback', () => { test('throws Context error when not properly initialized', async () => { Context.API_KEY = ''; - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -236,37 +205,31 @@ describe('validateAuthCallback', () => { ).rejects.toThrow(ShopifyErrors.CookieNotFound); }); - test('throws an error when receiving a callback for a shop with no saved session', async () => { - await ShopifyOAuth.beginAuth( - req, - res, - 'test-shop.myshopify.io', - '/some-callback', - ); - - await Context.SESSION_STORAGE.deleteSession(cookies.id); + test('throws error when callback includes invalid hmac', async () => { + await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback'); + const testCallbackQuery: AuthQuery = { + shop, + state: 'noncenoncenonce', + timestamp: Number(new Date()).toString(), + code: 'some random auth code', + }; + testCallbackQuery.hmac = 'definitely the wrong hmac'; await expect( - ShopifyOAuth.validateAuthCallback(req, res, { - shop: 'I do not exist', - } as AuthQuery), - ).rejects.toThrow(ShopifyErrors.SessionNotFound); + ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery), + ).rejects.toThrow(ShopifyErrors.InvalidOAuthError); }); - test('throws error when callback includes invalid hmac, or state', async () => { - await ShopifyOAuth.beginAuth( - req, - res, - 'test-shop.myshopify.io', - '/some-callback', - ); + test('throws error when callback includes invalid state', async () => { + await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback'); const testCallbackQuery: AuthQuery = { - shop: 'invalidurl.com', - state: 'incorrect', + shop, + state: 'incorrect state', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; - testCallbackQuery.hmac = 'definitely the wrong hmac'; + const expectedHmac = generateLocalHmac(testCallbackQuery); + testCallbackQuery.hmac = expectedHmac; await expect( ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery), @@ -279,7 +242,7 @@ describe('validateAuthCallback', () => { const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -307,12 +270,11 @@ describe('validateAuthCallback', () => { ).rejects.toThrow(ShopifyErrors.SessionStorageError); }); - test('requests access token for valid callbacks with offline access and updates session', async () => { - await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback'); - let session = await Context.SESSION_STORAGE.loadSession(cookies.id); + test('requests access token for valid callbacks with offline access and creates session', async () => { + await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -327,18 +289,18 @@ describe('validateAuthCallback', () => { /* eslint-enable @typescript-eslint/naming-convention */ fetchMock.mockResponse(JSON.stringify(successResponse)); - await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery); - session = await Context.SESSION_STORAGE.loadSession(cookies.id); + await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery, false); + expect(cookies.id).toEqual(expect.stringMatching(`offline_${shop}`)); + const session = await Context.SESSION_STORAGE.loadSession(cookies.id); expect(session?.accessToken).toBe(successResponse.access_token); }); - test('requests access token for valid callbacks with online access and updates session with expiration and onlineAccessInfo', async () => { + test('requests access token for valid callbacks with online access and creates session with expiration and onlineAccessInfo', async () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true); - let session = await Context.SESSION_STORAGE.loadSession(cookies.id); const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -371,8 +333,13 @@ describe('validateAuthCallback', () => { /* eslint-enable @typescript-eslint/naming-convention */ fetchMock.mockResponse(JSON.stringify(successResponse)); - await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery); - session = await Context.SESSION_STORAGE.loadSession(cookies.id); + await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery, true); + expect(cookies.id).toEqual( + expect.stringMatching( + /^[a-f0-9]{8,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{12,}/, + ), + ); + const session = await Context.SESSION_STORAGE.loadSession(cookies.id); expect(session?.accessToken).toBe(successResponse.access_token); expect(session?.expires).toBeInstanceOf(Date); @@ -388,13 +355,11 @@ describe('validateAuthCallback', () => { ).rejects.toThrow(ShopifyErrors.PrivateAppError); }); - test('properly updates the Oauth cookie for online, embedded apps', async () => { + test('properly updates the OAuth cookie for online, embedded apps', async () => { Context.IS_EMBEDDED_APP = true; Context.initialize(Context); await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); - expect(session).not.toBe(null); /* eslint-disable @typescript-eslint/naming-convention */ const successResponse = { @@ -415,7 +380,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -428,6 +393,7 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, + true, ); const jwtPayload: JwtPayload = { @@ -476,12 +442,11 @@ describe('validateAuthCallback', () => { ); }); - test('properly updates the Oauth cookie for online, non-embedded apps', async () => { + test('properly updates the OAuth cookie for online, non-embedded apps', async () => { Context.IS_EMBEDDED_APP = false; Context.initialize(Context); await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const successResponse = { @@ -502,7 +467,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -515,8 +480,14 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, + true, ); expect(returnedSession.id).toEqual(cookies.id); + expect(cookies.id).toEqual( + expect.stringMatching( + /^[a-f0-9]{8,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{12,}/, + ), + ); expect(returnedSession?.expires?.getTime() as number).toBeWithinSecondsOf( new Date(Date.now() + successResponse.expires_in * 1000).getTime(), @@ -531,12 +502,11 @@ describe('validateAuthCallback', () => { expect(cookieSession).not.toBeUndefined(); }); - test('properly updates the Oauth cookie for offline, embedded apps', async () => { + test('properly updates the OAuth cookie for offline, embedded apps', async () => { Context.IS_EMBEDDED_APP = true; Context.initialize(Context); await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const successResponse = { @@ -557,7 +527,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -570,6 +540,7 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, + false, ); expect(returnedSession.id).toEqual(cookies.id); expect(returnedSession.id).toEqual(ShopifyOAuth.getOfflineSessionId(shop)); @@ -583,12 +554,11 @@ describe('validateAuthCallback', () => { expect(returnedSession?.expires?.getTime()).toBeUndefined(); }); - test('properly updates the Oauth cookie for offline, non-embedded apps', async () => { + test('properly updates the OAuth cookie for offline, non-embedded apps', async () => { Context.IS_EMBEDDED_APP = false; Context.initialize(Context); await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); - const session = await Context.SESSION_STORAGE.loadSession(cookies.id); /* eslint-disable @typescript-eslint/naming-convention */ const successResponse = { @@ -609,7 +579,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: session ? session.state : '', + state: 'noncenoncenonce', timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -622,6 +592,7 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, + false, ); expect(returnedSession.id).toEqual(cookies.id); expect(returnedSession.id).toEqual(ShopifyOAuth.getOfflineSessionId(shop)); diff --git a/src/auth/oauth/oauth.ts b/src/auth/oauth/oauth.ts index 7a4b33243..87422d24e 100644 --- a/src/auth/oauth/oauth.ts +++ b/src/auth/oauth/oauth.ts @@ -25,6 +25,7 @@ import { const ShopifyOAuth = { SESSION_COOKIE_NAME: 'shopify_app_session', + STATE_COOKIE_NAME: 'shopify_app_state', /** * Initializes a session and cookie for the OAuth process, and returns the necessary authorization url. @@ -55,22 +56,7 @@ const ShopifyOAuth = { const state = nonce(); - const session = new Session( - isOnline ? uuidv4() : this.getOfflineSessionId(cleanShop), - cleanShop, - state, - isOnline, - ); - - const sessionStored = await Context.SESSION_STORAGE.storeSession(session); - - if (!sessionStored) { - throw new ShopifyErrors.SessionStorageError( - 'OAuth Session could not be saved. Please check your session storage functionality.', - ); - } - - cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, { + cookies.set(ShopifyOAuth.STATE_COOKIE_NAME, state, { signed: true, expires: new Date(Date.now() + 60000), sameSite: 'lax', @@ -101,12 +87,14 @@ const ShopifyOAuth = { * @param response Current HTTP Response * @param query Current HTTP Request Query, containing the information to be validated. * Depending on framework, this may need to be cast as "unknown" before being passed. + * @param isOnline Boolean value. Defaults to true (online access). * @returns SessionInterface */ async validateAuthCallback( request: http.IncomingMessage, response: http.ServerResponse, query: AuthQuery, + isOnline = true, ): Promise { Context.throwIfUninitialized(); Context.throwIfPrivateApp('Cannot perform OAuth for private apps'); @@ -116,23 +104,18 @@ const ShopifyOAuth = { secure: true, }); - const sessionCookie = this.getCookieSessionId(request, response); - if (!sessionCookie) { + const stateFromCookie = this.getValueFromCookie( + request, + response, + this.STATE_COOKIE_NAME, + ); + if (!stateFromCookie) { throw new ShopifyErrors.CookieNotFound( `Cannot complete OAuth process. Could not find an OAuth cookie for shop url: ${query.shop}`, ); } - let currentSession = await Context.SESSION_STORAGE.loadSession( - sessionCookie, - ); - if (!currentSession) { - throw new ShopifyErrors.SessionNotFound( - `Cannot complete OAuth process. No session found for the specified shop url: ${query.shop}`, - ); - } - - if (!validQuery(query, currentSession)) { + if (!validQuery(query, stateFromCookie)) { throw new ShopifyErrors.InvalidOAuthError('Invalid OAuth callback.'); } @@ -150,81 +133,79 @@ const ShopifyOAuth = { data: body, }; - const client = new HttpClient(currentSession.shop); + const client = new HttpClient(query.shop); const postResponse = await client.post(postParams); - if (currentSession.isOnline) { + let session: Session = new Session( + isOnline ? uuidv4() : this.getOfflineSessionId(query.shop), + query.shop, + stateFromCookie, + isOnline, + ); + + if (isOnline) { const responseBody = postResponse.body; const {access_token, scope, ...rest} = responseBody; // eslint-disable-line @typescript-eslint/naming-convention const sessionExpiration = new Date( Date.now() + responseBody.expires_in * 1000, ); - currentSession.accessToken = access_token; - currentSession.expires = sessionExpiration; - currentSession.scope = scope; - currentSession.onlineAccessInfo = rest; + session.accessToken = access_token; + session.expires = sessionExpiration; + session.scope = scope; + session.onlineAccessInfo = rest; // For an online session in an embedded app, we no longer want the cookie session so we delete it if (Context.IS_EMBEDDED_APP) { // If this is an online session for an embedded app, replace the online session with a JWT session - const onlineInfo = currentSession.onlineAccessInfo as OnlineAccessInfo; + const onlineInfo = session.onlineAccessInfo as OnlineAccessInfo; const jwtSessionId = this.getJwtSessionId( - currentSession.shop, + session.shop, `${onlineInfo.associated_user.id}`, ); - const jwtSession = Session.cloneSession(currentSession, jwtSessionId); + const jwtSession = Session.cloneSession(session, jwtSessionId); - const sessionDeleted = await Context.SESSION_STORAGE.deleteSession( - currentSession.id, - ); - if (!sessionDeleted) { - throw new ShopifyErrors.SessionStorageError( - 'OAuth Session could not be deleted. Please check your session storage functionality.', - ); - } - currentSession = jwtSession; + session = jwtSession; } } else { // Offline sessions (embedded / non-embedded) will use the same id so they don't need to be updated const responseBody = postResponse.body as AccessTokenResponse; - currentSession.accessToken = responseBody.access_token; - currentSession.scope = responseBody.scope; + session.accessToken = responseBody.access_token; + session.scope = responseBody.scope; } - cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, currentSession.id, { + cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, { signed: true, - expires: Context.IS_EMBEDDED_APP ? new Date() : currentSession.expires, + expires: Context.IS_EMBEDDED_APP ? new Date() : session.expires, sameSite: 'lax', secure: true, }); - const sessionStored = await Context.SESSION_STORAGE.storeSession( - currentSession, - ); + const sessionStored = await Context.SESSION_STORAGE.storeSession(session); if (!sessionStored) { throw new ShopifyErrors.SessionStorageError( 'OAuth Session could not be saved. Please check your session storage functionality.', ); } - return currentSession; + return session; }, /** - * Loads the current session id from the session cookie. + * Loads a given value from the cookie * * @param request HTTP request object * @param response HTTP response object */ - getCookieSessionId( + getValueFromCookie( request: http.IncomingMessage, response: http.ServerResponse, + name: string, ): string | undefined { const cookies = new Cookies(request, response, { secure: true, keys: [Context.API_SECRET_KEY], }); - return cookies.get(this.SESSION_COOKIE_NAME, {signed: true}); + return cookies.get(name, {signed: true}); }, /** @@ -285,7 +266,11 @@ const ShopifyOAuth = { // JWT. if (!currentSessionId) { // We still want to get the offline session id from the cookie to make sure it's validated - currentSessionId = this.getCookieSessionId(request, response); + currentSessionId = this.getValueFromCookie( + request, + response, + this.SESSION_COOKIE_NAME, + ); } return currentSessionId; @@ -296,12 +281,10 @@ const ShopifyOAuth = { * Uses the validation utils validateHmac, and safeCompare to assess whether the callback is valid. * * @param query Current HTTP Request Query - * @param session Current session + * @param stateFromCookie state value from the current cookie */ -function validQuery(query: AuthQuery, session: Session): boolean { - return ( - validateHmac(query) && safeCompare(query.state, session.state as string) - ); +function validQuery(query: AuthQuery, stateFromCookie: string): boolean { + return validateHmac(query) && safeCompare(query.state, stateFromCookie); } export {ShopifyOAuth}; From 991b937d2adf8d622cf2fa7ff092799cfed67c3b Mon Sep 17 00:00:00 2001 From: Kevin O'Sullivan Date: Wed, 17 Aug 2022 17:56:39 -0400 Subject: [PATCH 2/3] Refactored after review --- src/auth/oauth/__tests__/oauth.test.ts | 45 ++++---- src/auth/oauth/oauth.ts | 138 ++++++++++++++++--------- 2 files changed, 114 insertions(+), 69 deletions(-) diff --git a/src/auth/oauth/__tests__/oauth.test.ts b/src/auth/oauth/__tests__/oauth.test.ts index c4037aae1..443d51ecc 100644 --- a/src/auth/oauth/__tests__/oauth.test.ts +++ b/src/auth/oauth/__tests__/oauth.test.ts @@ -14,8 +14,10 @@ import {JwtPayload} from '../../../utils/decode-session-token'; import loadCurrentSession from '../../../utils/load-current-session'; import {CustomSessionStorage} from '../../session'; +const VALID_NONCE = 'noncenoncenonce'; + jest.mock('cookies'); -jest.mock('../../../utils/nonce', () => jest.fn(() => 'noncenoncenonce')); +jest.mock('../../../utils/nonce', () => jest.fn(() => VALID_NONCE)); let shop: string; @@ -59,7 +61,14 @@ describe('beginAuth', () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); expect(nonce).toHaveBeenCalled(); - expect(cookies.id).toBe(`noncenoncenonce`); + expect(cookies.id).toBe(`offline_${VALID_NONCE}`); + }); + + test('sets cookie to state for online access requests', async () => { + await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true); + + expect(nonce).toHaveBeenCalled(); + expect(cookies.id).toBe(`online_${VALID_NONCE}`); }); test('returns the correct auth url for given info', async () => { @@ -75,7 +84,7 @@ describe('beginAuth', () => { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `${Context.HOST_SCHEME}://${Context.HOST_NAME}/some-callback`, - state: 'noncenoncenonce', + state: `offline_${VALID_NONCE}`, 'grant_options[]': '', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -101,7 +110,7 @@ describe('beginAuth', () => { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `http://${Context.HOST_NAME}/some-callback`, - state: 'noncenoncenonce', + state: `offline_${VALID_NONCE}`, 'grant_options[]': '', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -127,7 +136,7 @@ describe('beginAuth', () => { client_id: Context.API_KEY, scope: Context.SCOPES.toString(), redirect_uri: `${Context.HOST_SCHEME}://${Context.HOST_NAME}/some-callback`, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, 'grant_options[]': 'per-user', }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -185,7 +194,7 @@ describe('validateAuthCallback', () => { Context.API_KEY = ''; const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: VALID_NONCE, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -209,7 +218,7 @@ describe('validateAuthCallback', () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback'); const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -242,7 +251,7 @@ describe('validateAuthCallback', () => { const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -274,7 +283,7 @@ describe('validateAuthCallback', () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false); const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `offline_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -289,7 +298,7 @@ describe('validateAuthCallback', () => { /* eslint-enable @typescript-eslint/naming-convention */ fetchMock.mockResponse(JSON.stringify(successResponse)); - await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery, false); + await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery); expect(cookies.id).toEqual(expect.stringMatching(`offline_${shop}`)); const session = await Context.SESSION_STORAGE.loadSession(cookies.id); @@ -300,7 +309,7 @@ describe('validateAuthCallback', () => { await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true); const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -333,7 +342,7 @@ describe('validateAuthCallback', () => { /* eslint-enable @typescript-eslint/naming-convention */ fetchMock.mockResponse(JSON.stringify(successResponse)); - await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery, true); + await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery); expect(cookies.id).toEqual( expect.stringMatching( /^[a-f0-9]{8,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{4,}-[a-f0-9]{12,}/, @@ -380,7 +389,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -393,7 +402,6 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, - true, ); const jwtPayload: JwtPayload = { @@ -467,7 +475,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `online_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -480,7 +488,6 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, - true, ); expect(returnedSession.id).toEqual(cookies.id); expect(cookies.id).toEqual( @@ -527,7 +534,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `offline_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -540,7 +547,6 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, - false, ); expect(returnedSession.id).toEqual(cookies.id); expect(returnedSession.id).toEqual(ShopifyOAuth.getOfflineSessionId(shop)); @@ -579,7 +585,7 @@ describe('validateAuthCallback', () => { }; const testCallbackQuery: AuthQuery = { shop, - state: 'noncenoncenonce', + state: `offline_${VALID_NONCE}`, timestamp: Number(new Date()).toString(), code: 'some random auth code', }; @@ -592,7 +598,6 @@ describe('validateAuthCallback', () => { req, res, testCallbackQuery, - false, ); expect(returnedSession.id).toEqual(cookies.id); expect(returnedSession.id).toEqual(ShopifyOAuth.getOfflineSessionId(shop)); diff --git a/src/auth/oauth/oauth.ts b/src/auth/oauth/oauth.ts index 87422d24e..fde0a8fe2 100644 --- a/src/auth/oauth/oauth.ts +++ b/src/auth/oauth/oauth.ts @@ -11,7 +11,7 @@ import safeCompare from '../../utils/safe-compare'; import decodeSessionToken from '../../utils/decode-session-token'; import {Session} from '../session'; import {HttpClient} from '../../clients/http_client/http_client'; -import {DataType} from '../../clients/http_client/types'; +import {DataType, RequestReturn} from '../../clients/http_client/types'; import * as ShopifyErrors from '../../error'; import {SessionInterface} from '../session/types'; import {sanitizeShop} from '../../utils/shop-validator'; @@ -54,7 +54,7 @@ const ShopifyOAuth = { secure: true, }); - const state = nonce(); + const state = isOnline ? `online_${nonce()}` : `offline_${nonce()}`; cookies.set(ShopifyOAuth.STATE_COOKIE_NAME, state, { signed: true, @@ -94,7 +94,6 @@ const ShopifyOAuth = { request: http.IncomingMessage, response: http.ServerResponse, query: AuthQuery, - isOnline = true, ): Promise { Context.throwIfUninitialized(); Context.throwIfPrivateApp('Cannot perform OAuth for private apps'); @@ -104,7 +103,7 @@ const ShopifyOAuth = { secure: true, }); - const stateFromCookie = this.getValueFromCookie( + const stateFromCookie = getValueFromCookie( request, response, this.STATE_COOKIE_NAME, @@ -119,6 +118,8 @@ const ShopifyOAuth = { throw new ShopifyErrors.InvalidOAuthError('Invalid OAuth callback.'); } + const isOnline = stateFromCookie.startsWith('online_'); + /* eslint-disable @typescript-eslint/naming-convention */ const body = { client_id: Context.API_KEY, @@ -132,47 +133,18 @@ const ShopifyOAuth = { type: DataType.JSON, data: body, }; + const cleanShop = sanitizeShop(query.shop, true)!; - const client = new HttpClient(query.shop); - const postResponse = await client.post(postParams); + const client = new HttpClient(cleanShop); + const postResponse = await client.post(postParams); - let session: Session = new Session( - isOnline ? uuidv4() : this.getOfflineSessionId(query.shop), - query.shop, + const session: Session = createSession( + postResponse, + cleanShop, stateFromCookie, isOnline, ); - if (isOnline) { - const responseBody = postResponse.body; - const {access_token, scope, ...rest} = responseBody; // eslint-disable-line @typescript-eslint/naming-convention - const sessionExpiration = new Date( - Date.now() + responseBody.expires_in * 1000, - ); - session.accessToken = access_token; - session.expires = sessionExpiration; - session.scope = scope; - session.onlineAccessInfo = rest; - - // For an online session in an embedded app, we no longer want the cookie session so we delete it - if (Context.IS_EMBEDDED_APP) { - // If this is an online session for an embedded app, replace the online session with a JWT session - const onlineInfo = session.onlineAccessInfo as OnlineAccessInfo; - const jwtSessionId = this.getJwtSessionId( - session.shop, - `${onlineInfo.associated_user.id}`, - ); - const jwtSession = Session.cloneSession(session, jwtSessionId); - - session = jwtSession; - } - } else { - // Offline sessions (embedded / non-embedded) will use the same id so they don't need to be updated - const responseBody = postResponse.body as AccessTokenResponse; - session.accessToken = responseBody.access_token; - session.scope = responseBody.scope; - } - cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, { signed: true, expires: Context.IS_EMBEDDED_APP ? new Date() : session.expires, @@ -183,7 +155,7 @@ const ShopifyOAuth = { const sessionStored = await Context.SESSION_STORAGE.storeSession(session); if (!sessionStored) { throw new ShopifyErrors.SessionStorageError( - 'OAuth Session could not be saved. Please check your session storage functionality.', + 'Session could not be saved. Please check your session storage functionality.', ); } @@ -191,21 +163,16 @@ const ShopifyOAuth = { }, /** - * Loads a given value from the cookie + * Loads the current session id from the session cookie. * * @param request HTTP request object * @param response HTTP response object */ - getValueFromCookie( + getCookieSessionId( request: http.IncomingMessage, response: http.ServerResponse, - name: string, ): string | undefined { - const cookies = new Cookies(request, response, { - secure: true, - keys: [Context.API_SECRET_KEY], - }); - return cookies.get(name, {signed: true}); + return getValueFromCookie(request, response, this.SESSION_COOKIE_NAME); }, /** @@ -266,7 +233,7 @@ const ShopifyOAuth = { // JWT. if (!currentSessionId) { // We still want to get the offline session id from the cookie to make sure it's validated - currentSessionId = this.getValueFromCookie( + currentSessionId = getValueFromCookie( request, response, this.SESSION_COOKIE_NAME, @@ -287,4 +254,77 @@ function validQuery(query: AuthQuery, stateFromCookie: string): boolean { return validateHmac(query) && safeCompare(query.state, stateFromCookie); } +/** + * Loads a given value from the cookie + * + * @param request HTTP request object + * @param response HTTP response object + * @param name Name of the cookie to load + */ +function getValueFromCookie( + request: http.IncomingMessage, + response: http.ServerResponse, + name: string, +): string | undefined { + const cookies = new Cookies(request, response, { + secure: true, + keys: [Context.API_SECRET_KEY], + }); + return cookies.get(name, {signed: true}); +} + +/** + * Creates a new session from the response from the access token request. + * + * @param postResponse Response from the access token request + * @param shop Shop url: {shop}.myshopify.com + * @param stateFromCookie State from the cookie received by the OAuth callback + * @param isOnline Boolean indicating if the access token is for online access + * @returns SessionInterface + */ +function createSession( + postResponse: RequestReturn, + shop: string, + stateFromCookie: string, + isOnline: boolean, +): SessionInterface { + let session: Session; + + if (isOnline) { + let sessionId: string; + const responseBody = postResponse.body as OnlineAccessResponse; + const {access_token, scope, ...rest} = responseBody; // eslint-disable-line @typescript-eslint/naming-convention + const sessionExpiration = new Date( + Date.now() + responseBody.expires_in * 1000, + ); + + if (Context.IS_EMBEDDED_APP) { + sessionId = ShopifyOAuth.getJwtSessionId( + shop, + `${(rest as OnlineAccessInfo).associated_user.id}`, + ); + } else { + sessionId = uuidv4(); + } + + session = new Session(sessionId, shop, stateFromCookie, isOnline); + session.accessToken = access_token; + session.scope = scope; + session.expires = sessionExpiration; + session.onlineAccessInfo = rest; + } else { + const responseBody = postResponse.body as AccessTokenResponse; + session = new Session( + ShopifyOAuth.getOfflineSessionId(shop), + shop, + stateFromCookie, + isOnline, + ); + session.accessToken = responseBody.access_token; + session.scope = responseBody.scope; + } + + return session; +} + export {ShopifyOAuth}; From 3fd3ca589dcd95422859d79f6e4e9416b4fb6124 Mon Sep 17 00:00:00 2001 From: Kevin O'Sullivan Date: Thu, 18 Aug 2022 11:41:17 -0400 Subject: [PATCH 3/3] Do not set OAuth cookie for embedded app --- src/auth/oauth/__tests__/oauth.test.ts | 23 ++++--------- src/auth/oauth/oauth.ts | 45 +++++++++++++++++++------- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/auth/oauth/__tests__/oauth.test.ts b/src/auth/oauth/__tests__/oauth.test.ts index 443d51ecc..addd1b300 100644 --- a/src/auth/oauth/__tests__/oauth.test.ts +++ b/src/auth/oauth/__tests__/oauth.test.ts @@ -178,12 +178,12 @@ describe('validateAuthCallback', () => { res = {} as http.ServerResponse; Cookies.prototype.set.mockImplementation( - (cookieName: string, cookieValue: string, options: {expires: Date}) => { + (cookieName: string, cookieValue: string, options?: {expires: Date}) => { expect(cookieName).toEqual( expect.stringMatching(/^shopify_app_(session|state)/), ); cookies.id = cookieValue; - cookies.expires = options.expires; + cookies.expires = options?.expires; }, ); @@ -364,7 +364,7 @@ describe('validateAuthCallback', () => { ).rejects.toThrow(ShopifyErrors.PrivateAppError); }); - test('properly updates the OAuth cookie for online, embedded apps', async () => { + test('does not set an OAuth cookie for online, embedded apps', async () => { Context.IS_EMBEDDED_APP = true; Context.initialize(Context); @@ -444,10 +444,7 @@ describe('validateAuthCallback', () => { const currentSession = await loadCurrentSession(jwtReq, jwtRes); expect(currentSession).not.toBe(null); expect(currentSession?.id).toEqual(jwtSessionId); - expect(cookies?.expires?.getTime() as number).toBeWithinSecondsOf( - new Date().getTime(), - 1, - ); + expect(cookies.id).not.toBeDefined(); }); test('properly updates the OAuth cookie for online, non-embedded apps', async () => { @@ -509,7 +506,7 @@ describe('validateAuthCallback', () => { expect(cookieSession).not.toBeUndefined(); }); - test('properly updates the OAuth cookie for offline, embedded apps', async () => { + test('does not set an OAuth cookie for offline, embedded apps', async () => { Context.IS_EMBEDDED_APP = true; Context.initialize(Context); @@ -548,16 +545,10 @@ describe('validateAuthCallback', () => { res, testCallbackQuery, ); - expect(returnedSession.id).toEqual(cookies.id); expect(returnedSession.id).toEqual(ShopifyOAuth.getOfflineSessionId(shop)); - - const cookieSession = await Context.SESSION_STORAGE.loadSession(cookies.id); - expect(cookieSession).not.toBeUndefined(); - expect(cookies?.expires?.getTime() as number).toBeWithinSecondsOf( - new Date().getTime(), - 1, - ); expect(returnedSession?.expires?.getTime()).toBeUndefined(); + + expect(cookies.id).not.toBeDefined(); }); test('properly updates the OAuth cookie for offline, non-embedded apps', async () => { diff --git a/src/auth/oauth/oauth.ts b/src/auth/oauth/oauth.ts index fde0a8fe2..a7a48ecb7 100644 --- a/src/auth/oauth/oauth.ts +++ b/src/auth/oauth/oauth.ts @@ -98,16 +98,13 @@ const ShopifyOAuth = { Context.throwIfUninitialized(); Context.throwIfPrivateApp('Cannot perform OAuth for private apps'); - const cookies = new Cookies(request, response, { - keys: [Context.API_SECRET_KEY], - secure: true, - }); - const stateFromCookie = getValueFromCookie( request, response, this.STATE_COOKIE_NAME, ); + deleteCookie(request, response, this.STATE_COOKIE_NAME); + if (!stateFromCookie) { throw new ShopifyErrors.CookieNotFound( `Cannot complete OAuth process. Could not find an OAuth cookie for shop url: ${query.shop}`, @@ -145,12 +142,19 @@ const ShopifyOAuth = { isOnline, ); - cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, { - signed: true, - expires: Context.IS_EMBEDDED_APP ? new Date() : session.expires, - sameSite: 'lax', - secure: true, - }); + if (!Context.IS_EMBEDDED_APP) { + const cookies = new Cookies(request, response, { + keys: [Context.API_SECRET_KEY], + secure: true, + }); + + cookies.set(ShopifyOAuth.SESSION_COOKIE_NAME, session.id, { + signed: true, + expires: session.expires, + sameSite: 'lax', + secure: true, + }); + } const sessionStored = await Context.SESSION_STORAGE.storeSession(session); if (!sessionStored) { @@ -273,6 +277,25 @@ function getValueFromCookie( return cookies.get(name, {signed: true}); } +/** + * Loads a given value from the cookie + * + * @param request HTTP request object + * @param response HTTP response object + * @param name Name of the cookie to load + */ +function deleteCookie( + request: http.IncomingMessage, + response: http.ServerResponse, + name: string, +): void { + const cookies = new Cookies(request, response, { + secure: true, + keys: [Context.API_SECRET_KEY], + }); + cookies.set(name); +} + /** * Creates a new session from the response from the access token request. *