Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Store state instead of session id in cookie #438

Merged
merged 3 commits into from
Aug 18, 2022
Merged

Store state instead of session id in cookie #438

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
970dc56
Store state instead of session id in cookie
mkevinosullivan Jul 29, 2022
991b937
Refactored after review
mkevinosullivan Aug 17, 2022
3fd3ca5
Do not set OAuth cookie for embedded app
mkevinosullivan Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
154 changes: 65 additions & 89 deletions src/auth/oauth/__tests__/oauth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ 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';

const VALID_NONCE = 'noncenoncenonce';

jest.mock('cookies');
jest.mock('../../../utils/nonce', () => jest.fn(() => VALID_NONCE));

let shop: string;

Expand All @@ -39,7 +43,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;
},
);
Expand All @@ -53,42 +57,18 @@ 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);
test('sets cookie to state for offline access requests', async () => {
mkevinosullivan marked this conversation as resolved.
Show resolved Hide resolved
await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false);

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);
expect(nonce).toHaveBeenCalled();
expect(cookies.id).toBe(`offline_${VALID_NONCE}`);
});

test('sets session id and cookie to shop name prefixed with "offline_" for offline access requests', async () => {
await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', false);
test('sets cookie to state for online access requests', async () => {
await ShopifyOAuth.beginAuth(req, res, shop, '/some-callback', true);

expect(cookies.id).toBe(`offline_${shop}`);
expect(nonce).toHaveBeenCalled();
expect(cookies.id).toBe(`online_${VALID_NONCE}`);
});

test('returns the correct auth url for given info', async () => {
Expand All @@ -99,13 +79,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: `offline_${VALID_NONCE}`,
'grant_options[]': '',
};
/* eslint-enable @typescript-eslint/naming-convention */
Expand All @@ -126,13 +105,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: `offline_${VALID_NONCE}`,
'grant_options[]': '',
};
/* eslint-enable @typescript-eslint/naming-convention */
Expand All @@ -152,14 +130,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: `online_${VALID_NONCE}`,
'grant_options[]': 'per-user',
};
/* eslint-enable @typescript-eslint/naming-convention */
Expand Down Expand Up @@ -202,7 +179,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;
},
Expand All @@ -213,10 +192,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: VALID_NONCE,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand All @@ -236,37 +214,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: `online_${VALID_NONCE}`,
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),
Expand All @@ -279,7 +251,7 @@ describe('validateAuthCallback', () => {

const testCallbackQuery: AuthQuery = {
shop,
state: session ? session.state : '',
state: `online_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand Down Expand Up @@ -307,12 +279,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: `offline_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand All @@ -328,17 +299,17 @@ describe('validateAuthCallback', () => {

fetchMock.mockResponse(JSON.stringify(successResponse));
await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery);
session = await Context.SESSION_STORAGE.loadSession(cookies.id);
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: `online_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand Down Expand Up @@ -372,7 +343,12 @@ describe('validateAuthCallback', () => {

fetchMock.mockResponse(JSON.stringify(successResponse));
await ShopifyOAuth.validateAuthCallback(req, res, testCallbackQuery);
session = await Context.SESSION_STORAGE.loadSession(cookies.id);
expect(cookies.id).toEqual(
expect.stringMatching(
paulomarg marked this conversation as resolved.
Show resolved Hide resolved
/^[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);
Expand All @@ -388,13 +364,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 = {
Expand All @@ -415,7 +389,7 @@ describe('validateAuthCallback', () => {
};
const testCallbackQuery: AuthQuery = {
shop,
state: session ? session.state : '',
state: `online_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand Down Expand Up @@ -476,12 +450,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 = {
Expand All @@ -502,7 +475,7 @@ describe('validateAuthCallback', () => {
};
const testCallbackQuery: AuthQuery = {
shop,
state: session ? session.state : '',
state: `online_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand All @@ -517,6 +490,11 @@ describe('validateAuthCallback', () => {
testCallbackQuery,
);
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(),
Expand All @@ -531,12 +509,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 = {
Expand All @@ -557,7 +534,7 @@ describe('validateAuthCallback', () => {
};
const testCallbackQuery: AuthQuery = {
shop,
state: session ? session.state : '',
state: `offline_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand All @@ -583,12 +560,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 = {
Expand All @@ -609,7 +585,7 @@ describe('validateAuthCallback', () => {
};
const testCallbackQuery: AuthQuery = {
shop,
state: session ? session.state : '',
state: `offline_${VALID_NONCE}`,
timestamp: Number(new Date()).toString(),
code: 'some random auth code',
};
Expand Down
Loading