fix(adapter-nextjs): wrong cookie attributes get set sometimes #14169

HuiSF · 2025-01-28T21:59:20Z

Description of changes

Change 1
Create a global context store on the server start (when createServerRunner gets called) for separated processes (the runWithAmplifyServerContext, and the runWithAmplifyServerContext used by the data server-side client factories) of the library, so they can look up flags and configurations as the following:

isServerSideAuthEnabled (determined by whether the AMPLIFY_APP_ORIGIN env var is set as it's the prerequisite of enabling server-side auth)
isSSLOrigin (determined by whether AMPLIFY_APP_ORIGIN contains a https non localhost origin)
runtimeOptions (the runtimeOptions object passed in when calling createServerRunner)

At runtime, when some Amplify APIs are called within a closure created by a runWithAmplifyServerContext, globalRuntimeContext is used to retrieve relevant information from the above.

For example: when server side is enabled, runtimeOptions.cookies is always merged with { httpOnly: true } to enforce the token cookies are HTTP only.

Change 2

Instruct the cookie adapters created from Next.js server context that when server-side auth is enabled, when Amplify tokenStore interface attempts to set extra cookies, the adapters should ignore these cookies. (tradeoff of reusing the preexisting token store interface)

Issue #, if available

Description of how you validated changes

manual testing with sample apps
unit tests
E2E test pending to be added

Checklist

PR description included
yarn test passes
Unit Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When server-side auth is enabled, only acessToken, idToken, refreshToken and LastAuthUser cookies are set in cookie store. When calling Amplify APIs with runWithAmplifyServerContext then tokens need to be refreshed, the underlying tokenStore interface sets extra cookies for the original client-side use cases which is NOT ideal.

cshfang · 2025-01-29T18:09:55Z

packages/adapter-nextjs/src/utils/globalSettings.ts

+	isServerSideAuthEnabled() {
+		return isServerSideAuthEnabled;
+	},
+	enableServerSideAuth() {


I suspect the answer is no but is there a world where someone would want to freely flip this back and forth?

cshfang · 2025-01-29T18:10:49Z

packages/adapter-nextjs/src/utils/globalSettings.ts

+let isSSLOrigin = false;
+
+export const globalSettings: NextServer.GlobalSettings = {
+	isServerSideAuthEnabled() {


Super nit: Group setters together and getters together, or keep them in the same order i.e. setter first then getter. Just to aid readability

cshfang · 2025-01-29T18:14:47Z

packages/adapter-nextjs/src/utils/createRunWithAmplifyServerContext.ts

+
+	const mergedSetCookieOptions = {
+		// default options when not specified
+		...DEFAULT_SERVER_SIDE_AUTH_SET_COOKIE_OPTIONS,


I think I'm a little thrown off by the naming - these are default setCookie options for server side auth - but it doesn't require isServerSideAuthEnabled to be true?

cshfang · 2025-01-29T18:16:39Z

packages/adapter-nextjs/src/utils/createCookieStorageAdapterFromNextServerContext.ts


 import { ensureEncodedForJSCookie, serializeCookie } from './cookie';

 export const DATE_IN_THE_PAST = new Date(0);

 export const createCookieStorageAdapterFromNextServerContext = async (
 	context: NextServer.Context,
+	ignoreNonServerSideCookies = false,


What kinds of cookies qualify as "non-server-side"?

cshfang · 2025-01-29T18:20:36Z

packages/adapter-nextjs/src/utils/createCookieStorageAdapterFromNextServerContext.ts

+			if (ignoreNonServerSideCookies && isServerSideAuthIgnoredCookie(name)) {
+				return;
+			}


Nit: Does this need to be in the try? Ditto below

cshfang · 2025-01-29T18:23:36Z

packages/adapter-nextjs/src/types/NextServer.ts

+		isServerSideAuthEnabled(): boolean;
+		enableServerSideAuth(): void;


Super nit: Switch so each grouping has setter first then getter just for ease of understanding

cshfang · 2025-01-29T18:25:44Z

packages/adapter-nextjs/src/createServerRunner.ts

@@ -35,6 +36,15 @@ export const createServerRunner: NextServer.CreateServerRunner = ({
 	const amplifyConfig = parseAmplifyConfig(config);
 	const amplifyAppOrigin = process.env.AMPLIFY_APP_ORIGIN;

+	globalSettings.setRuntimeOptions(runtimeOptions || {});


Can runtimeOptions be falsy in a way that ?? wouldn't work?

cshfang · 2025-01-29T18:28:19Z

packages/adapter-nextjs/src/createServerRunner.ts

@@ -35,6 +36,15 @@ export const createServerRunner: NextServer.CreateServerRunner = ({
 	const amplifyConfig = parseAmplifyConfig(config);
 	const amplifyAppOrigin = process.env.AMPLIFY_APP_ORIGIN;

+	globalSettings.setRuntimeOptions(runtimeOptions || {});
+
+	if (amplifyAppOrigin !== undefined && isValidOrigin(amplifyAppOrigin)) {


Nit: Would undefined ever be a valid origin? Or can isValidOrigin just encapsulate that guard/check?

+1 it seems as though isValidOrigin could be adapted to handle an undefined input.

cshfang · 2025-01-29T18:29:44Z

packages/adapter-nextjs/src/createServerRunner.ts

+	if (amplifyAppOrigin !== undefined && isValidOrigin(amplifyAppOrigin)) {
+		globalSettings.setIsSSLOrigin(isSSLOrigin(amplifyAppOrigin));
+
+		// update the serverSideAuthEnabled flag that exists in the closure of createServerRunner() to true


Is this flag still only in the createServerRunner closure?

cshfang · 2025-01-29T18:31:56Z

packages/adapter-nextjs/src/auth/utils/tokenCookies.ts

@@ -79,3 +82,8 @@ export const createTokenCookiesRemoveOptions = (
 	path: '/',
 	maxAge: REMOVE_COOKIE_MAX_AGE, // Expire immediately (remove the cookie)
 });
+
+export const isServerSideAuthIgnoredCookie = (cookieName: string) =>


Nit: Would it be more intuitive for this to be server side auth allowed cookie given that that's the list we have? This might be invalidated by a different comment I have below about possibly consolidating the function purpose

jjarvisp · 2025-01-29T20:04:05Z

packages/adapter-nextjs/src/utils/createRunWithAmplifyServerContext.ts

 								),
 								tokenValidator,
-								setCookieOptions,
+								mergedSetCookieOptions,


Nit: It's getting kind of difficult to track what the default set cookie options will be when server side auth is enabled vs disabled. We have defaults in the cookie storage adaptor and here in the runWithAmplifyServerContext factory. Is there a way we can unify or clarify this?

jjarvisp · 2025-01-29T20:05:33Z

packages/adapter-nextjs/src/createServerRunner.ts

@@ -35,6 +36,15 @@ export const createServerRunner: NextServer.CreateServerRunner = ({
 	const amplifyConfig = parseAmplifyConfig(config);
 	const amplifyAppOrigin = process.env.AMPLIFY_APP_ORIGIN;

+	globalSettings.setRuntimeOptions(runtimeOptions || {});
+
+	if (amplifyAppOrigin !== undefined && isValidOrigin(amplifyAppOrigin)) {


+1 it seems as though isValidOrigin could be adapted to handle an undefined input.

fix(adapter-nextjs): refresh token not using server-side cookie attrs

a1b8453

HuiSF requested review from sktimalsina, cshfang and pranavosu as code owners January 28, 2025 21:59

HuiSF changed the title ~~feat(adapter-nextjs): add runtimeOptions.cookies to createServerRunner (~~ fix(adapter-nextjs): wrong cookie attributes get set sometimes Jan 28, 2025

chore(adapter-nextjs): rename globalRuntimeContext to globalSettings

5f5355b

cshfang reviewed Jan 29, 2025

View reviewed changes

jjarvisp reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(adapter-nextjs): wrong cookie attributes get set sometimes #14169

fix(adapter-nextjs): wrong cookie attributes get set sometimes #14169

HuiSF commented Jan 28, 2025 •

edited

Loading

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

jjarvisp Jan 29, 2025

cshfang Jan 29, 2025

cshfang Jan 29, 2025

jjarvisp Jan 29, 2025

jjarvisp Jan 29, 2025

		isServerSideAuthEnabled(): boolean;
		enableServerSideAuth(): void;

fix(adapter-nextjs): wrong cookie attributes get set sometimes #14169

Are you sure you want to change the base?

fix(adapter-nextjs): wrong cookie attributes get set sometimes #14169

Conversation

HuiSF commented Jan 28, 2025 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

Checklist for repo maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuiSF commented Jan 28, 2025 •

edited

Loading