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: hui/feat/adapter-nextjs/6-secure-for-non-ssl
Choose a base branch
from

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Dec 30, 2024

Description of changes

Use maxAge cookie attribute to set cookies from the server side to avoid clock drift issues.

Since the access token usage are carried onto the server-side, and tokens expiries check and refresh are all performed on the server side of a Next.js app, the client-side incorrect system time becomes irrelevant.

Issue #, if available

Description of how you validated changes

  • unit tests
  • manual tests with a Next.js sample app

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.

@HuiSF HuiSF requested review from sktimalsina, cshfang, pranavosu and a team as code owners December 30, 2024 18:50
Copy link
Member

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Thank you for addresing this! I only have one nit question.

@@ -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

@HuiSF HuiSF force-pushed the hui/feat/adapter-nextjs/6-secure-for-non-ssl branch from 43e6b22 to 48e7052 Compare January 2, 2025 22:15
@HuiSF HuiSF force-pushed the hui/feat/adapter-nextjs/7-use-max-age branch from c850f31 to ab50e6c Compare January 2, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants