Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(adapter-nextjs): use maxAge attribute to set cookie from server to avoid clock drift #14103

Open
wants to merge 1 commit into
base: feat/server-auth/main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ describe('handleSignInCallbackRequest', () => {
httpOnly: true,
sameSite: 'strict' as const,
expires: new Date('2024-9-17'),
maxAge: 3600,
};
mockCreateTokenCookiesSetOptions.mockReturnValueOnce(
mockCreateTokenCookiesSetOptionsResult,
Expand All @@ -231,7 +232,7 @@ describe('handleSignInCallbackRequest', () => {
const mockCreateAuthFlowProofCookiesRemoveOptionsResult = {
domain: 'example.com',
path: '/',
expires: new Date('1970-1-1'),
maxAge: -1,
};
mockCreateAuthFlowProofCookiesRemoveOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesRemoveOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ describe('handleSignInCallbackRequest', () => {
httpOnly: true,
sameSite: 'strict' as const,
expires: new Date('2024-9-17'),
maxAge: 3600,
};
mockCreateTokenCookiesSetOptions.mockReturnValueOnce(
mockCreateTokenCookiesSetOptionsResult,
Expand All @@ -262,7 +263,7 @@ describe('handleSignInCallbackRequest', () => {
const mockCreateAuthFlowProofCookiesRemoveOptionsResult = {
domain: 'example.com',
path: '/',
expires: new Date('1970-1-1'),
maxAge: -1,
};
mockCreateAuthFlowProofCookiesRemoveOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesRemoveOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ describe('handleSignInSignUpRequest', () => {
secure: true,
sameSite: 'lax' as const,
expires: new Date(),
maxAge: 3600,
};
mockCreateAuthFlowProofCookiesSetOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesSetOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ describe('handleSignInSignUpRequest', () => {
secure: true,
sameSite: 'lax' as const,
expires: new Date(),
maxAge: 3600,
};
mockCreateAuthFlowProofCookiesSetOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesSetOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ describe('handleSignOutCallbackRequest', () => {
const mockCreateTokenCookiesRemoveOptionsResult = {
domain: mockSetCookieOptions.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: -1,
};
mockCreateTokenCookiesRemoveOptions.mockReturnValueOnce(
mockCreateTokenCookiesRemoveOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ describe('handleSignOutCallbackRequest', () => {
const mockCreateTokenCookiesRemoveOptionsResult = {
domain: mockSetCookieOptions.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: -1,
};
mockCreateTokenCookiesRemoveOptions.mockReturnValueOnce(
mockCreateTokenCookiesRemoveOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ describe('handleSignOutRequest', () => {
secure: true,
sameSite: 'lax' as const,
expires: new Date('2024-09-18'),
maxAge: 3600,
};
mockCreateAuthFlowProofCookiesSetOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesSetOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ describe('handleSignOutRequest', () => {
secure: true,
sameSite: 'lax' as const,
expires: new Date('2024-09-18'),
maxAge: 3600,
};
mockCreateAuthFlowProofCookiesSetOptions.mockReturnValueOnce(
mockCreateAuthFlowProofCookiesSetOptionsResult,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CookieStorage } from 'aws-amplify/adapter-core';

import {
AUTH_FLOW_PROOF_COOKIE_EXPIRY,
AUTH_FLOW_PROOF_MAX_AGE,
IS_SIGNING_OUT_COOKIE_NAME,
PKCE_COOKIE_NAME,
STATE_COOKIE_NAME,
Expand Down Expand Up @@ -37,16 +37,6 @@ describe('createSignOutFlowProofCookies', () => {
});

describe('createAuthFlowProofCookiesSetOptions', () => {
let nowSpy: jest.SpyInstance;

beforeAll(() => {
nowSpy = jest.spyOn(Date, 'now').mockReturnValue(0);
});

afterAll(() => {
jest.restoreAllMocks();
});

it('returns expected cookie serialization options with specified parameters', () => {
const setCookieOptions: CookieStorage.SetCookieOptions = {
domain: '.example.com',
Expand All @@ -55,14 +45,13 @@ describe('createAuthFlowProofCookiesSetOptions', () => {

const options = createAuthFlowProofCookiesSetOptions(setCookieOptions);

expect(nowSpy).toHaveBeenCalled();
expect(options).toEqual({
domain: setCookieOptions?.domain,
path: '/',
httpOnly: true,
secure: true,
sameSite: 'lax' as const,
expires: new Date(0 + AUTH_FLOW_PROOF_COOKIE_EXPIRY),
maxAge: AUTH_FLOW_PROOF_MAX_AGE,
});
});

Expand All @@ -76,14 +65,13 @@ describe('createAuthFlowProofCookiesSetOptions', () => {
secure: false,
});

expect(nowSpy).toHaveBeenCalled();
expect(options).toEqual({
domain: setCookieOptions?.domain,
path: '/',
httpOnly: true,
secure: false,
sameSite: 'lax' as const,
expires: new Date(0 + AUTH_FLOW_PROOF_COOKIE_EXPIRY),
maxAge: AUTH_FLOW_PROOF_MAX_AGE,
});
});
});
Expand All @@ -99,7 +87,7 @@ describe('createAuthFlowProofCookiesRemoveOptions', () => {
expect(options).toEqual({
domain: setCookieOptions?.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: -1,
});
});
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
AUTH_KEY_PREFIX,
CookieStorage,
DEFAULT_COOKIE_EXPIRY,
DEFAULT_AUTH_TOKEN_COOKIES_MAX_AGE,
} from 'aws-amplify/adapter-core';

import { OAuthTokenResponsePayload } from '../../../src/auth/types';
Expand Down Expand Up @@ -113,7 +113,7 @@ describe('createTokenCookiesSetOptions', () => {
httpOnly: true,
secure: true,
sameSite: 'strict',
expires: new Date(0 + DEFAULT_COOKIE_EXPIRY),
maxAge: DEFAULT_AUTH_TOKEN_COOKIES_MAX_AGE,
});

dateNowSpy.mockRestore();
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('createTokenCookiesRemoveOptions', () => {
expect(result).toEqual({
domain: mockSetCookieOptions?.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: -1,
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ describe('createCookieStorageAdapterFromNextServerContext', () => {
httpOnly: true,
secure: true,
path: '/a-path',
maxAge: 3600,
};

it('gets cookie by calling `get` method of the underlying cookie store', () => {
Expand Down Expand Up @@ -185,7 +186,7 @@ describe('createCookieStorageAdapterFromNextServerContext', () => {
mockSerializeOptions.domain
};Expires=${mockSerializeOptions.expires.toUTCString()};HttpOnly;SameSite=${
mockSerializeOptions.sameSite
};Secure;Path=${mockSerializeOptions.path}`,
};Secure;Path=${mockSerializeOptions.path};Max-Age=${mockSerializeOptions.maxAge}`,
);
});

Expand All @@ -197,7 +198,7 @@ describe('createCookieStorageAdapterFromNextServerContext', () => {
mockSerializeOptions.domain
};Expires=${mockSerializeOptions.expires.toUTCString()};HttpOnly;SameSite=${
mockSerializeOptions.sameSite
};Secure;Path=${mockSerializeOptions.path}`,
};Secure;Path=${mockSerializeOptions.path};Max-Age=${mockSerializeOptions.maxAge}`,
);
});

Expand Down
3 changes: 2 additions & 1 deletion packages/adapter-nextjs/src/auth/constant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ export const PKCE_COOKIE_NAME = 'com.amplify.server_auth.pkce';
export const STATE_COOKIE_NAME = 'com.amplify.server_auth.state';
export const IS_SIGNING_OUT_COOKIE_NAME =
'com.amplify.server_auth.isSigningOut';
export const AUTH_FLOW_PROOF_COOKIE_EXPIRY = 10 * 60 * 1000; // 10 mins
export const AUTH_FLOW_PROOF_MAX_AGE = 10 * 60; // 10 mins in seconds
export const REMOVE_COOKIE_MAX_AGE = -1; // -1 to remove the cookie immediately (0 ==> session cookie)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(0 ==> session cookie)

I'm curious whether this is a common behavior by the specs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to MDN setting 0 should also make the cookie expire immediately - however, when I set the cookie from the server with 0, the cookie gets marked as session, and remaining in cookie store until closing the tab.

export const OAUTH_GRANT_TYPE = 'authorization_code';
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
import { CookieStorage } from 'aws-amplify/adapter-core';

import {
AUTH_FLOW_PROOF_COOKIE_EXPIRY,
AUTH_FLOW_PROOF_MAX_AGE,
IS_SIGNING_OUT_COOKIE_NAME,
PKCE_COOKIE_NAME,
REMOVE_COOKIE_MAX_AGE,
STATE_COOKIE_NAME,
} from '../constant';

Expand Down Expand Up @@ -43,13 +44,13 @@ export const createAuthFlowProofCookiesSetOptions = (
httpOnly: true,
secure: overrides?.secure ?? true,
sameSite: 'lax' as const,
expires: new Date(Date.now() + AUTH_FLOW_PROOF_COOKIE_EXPIRY),
maxAge: AUTH_FLOW_PROOF_MAX_AGE,
});

export const createAuthFlowProofCookiesRemoveOptions = (
setCookieOptions: CookieStorage.SetCookieOptions,
) => ({
domain: setCookieOptions?.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: REMOVE_COOKIE_MAX_AGE,
});
34 changes: 22 additions & 12 deletions packages/adapter-nextjs/src/auth/utils/tokenCookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
import {
AUTH_KEY_PREFIX,
CookieStorage,
DEFAULT_COOKIE_EXPIRY,
DEFAULT_AUTH_TOKEN_COOKIES_MAX_AGE,
createKeysForAuthStorage,
} from 'aws-amplify/adapter-core';

import { OAuthTokenResponsePayload } from '../types';
import { REMOVE_COOKIE_MAX_AGE } from '../constant';

import { getAccessTokenUsernameAndClockDrift } from './getAccessTokenUsernameAndClockDrift';

Expand Down Expand Up @@ -55,22 +56,31 @@ export const createTokenRemoveCookies = (keys: string[]) =>
keys.map(key => ({ name: key, value: '' }));

export const createTokenCookiesSetOptions = (
setCookieOptions: CookieStorage.SetCookieOptions,
{ domain, sameSite, expires, maxAge }: CookieStorage.SetCookieOptions,
overrides?: Pick<CookieStorage.SetCookieOptions, 'secure'>,
) => ({
domain: setCookieOptions?.domain,
path: '/',
httpOnly: true,
secure: overrides?.secure ?? true,
sameSite: setCookieOptions.sameSite ?? 'strict',
expires:
setCookieOptions?.expires ?? new Date(Date.now() + DEFAULT_COOKIE_EXPIRY),
});
) => {
const result = {
domain,
path: '/',
httpOnly: true,
secure: overrides?.secure ?? true,
sameSite: sameSite ?? 'strict',
expires,
maxAge,
};

// when expires and maxAge both are not specified, we set a default maxAge
if (!result.expires && !result.maxAge) {
result.maxAge = DEFAULT_AUTH_TOKEN_COOKIES_MAX_AGE;
}

return result;
};

export const createTokenCookiesRemoveOptions = (
setCookieOptions: CookieStorage.SetCookieOptions,
) => ({
domain: setCookieOptions?.domain,
path: '/',
expires: new Date('1970-01-01'),
maxAge: REMOVE_COOKIE_MAX_AGE, // Expire immediately (remove the cookie)
});
2 changes: 1 addition & 1 deletion packages/adapter-nextjs/src/types/NextServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export declare namespace NextServer {
export interface CreateServerRunnerRuntimeOptions {
cookies?: Pick<
CookieStorage.SetCookieOptions,
'domain' | 'expires' | 'sameSite'
'domain' | 'expires' | 'sameSite' | 'maxAge'
>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const serializeCookie = (
const serializeSetCookieOptions = (
options: CookieStorage.SetCookieOptions,
): string => {
const { expires, domain, httpOnly, sameSite, secure, path } = options;
const { expires, domain, httpOnly, sameSite, secure, path, maxAge } = options;
const serializedOptions: string[] = [];
if (domain) {
serializedOptions.push(`Domain=${domain}`);
Expand All @@ -33,6 +33,9 @@ const serializeSetCookieOptions = (
if (path) {
serializedOptions.push(`Path=${path}`);
}
if (maxAge) {
serializedOptions.push(`Max-Age=${maxAge}`);
}

return serializedOptions.join(';');
};
Loading
Loading