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

feat: make missing secret an error #3143

Merged
merged 16 commits into from
Nov 15, 2021
Merged

Conversation

balazsorban44
Copy link
Member

@balazsorban44 balazsorban44 commented Nov 7, 2021

If no secret specified in NextAuthOptions, next-auth generates one based on the user configuration. If the user does not use an OAuth provider, the generated secret will not contain enough entropy. Someone could guess the user's config, thus gaining access to the JWT encryption method. We already make this clear in the docs by saying:

The default behaviour is volatile, and it is strongly recommended you explicitly specify a value to avoid invalidating end user sessions when configuration changes are deployed.

This PR makes this an error in production.

The corresponding documentation is also made more clear, see nextauthjs/docs#103

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2021

Codecov Report

Merging #3143 (1d3f5a9) into beta (f9e0ef8) will decrease coverage by 0.25%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             beta    #3143      +/-   ##
==========================================
- Coverage   13.86%   13.60%   -0.26%     
==========================================
  Files          87       88       +1     
  Lines        1363     1389      +26     
  Branches      354      362       +8     
==========================================
  Hits          189      189              
- Misses       1163     1189      +26     
  Partials       11       11              
Impacted Files Coverage Δ
src/core/errors.ts 0.00% <0.00%> (ø)
src/core/index.ts 0.00% <0.00%> (ø)
src/core/init.ts 0.00% <ø> (ø)
src/core/lib/assert.ts 0.00% <0.00%> (ø)
src/core/lib/utils.ts 0.00% <ø> (ø)
src/core/pages/error.tsx 0.00% <0.00%> (ø)
src/core/pages/index.ts 0.00% <0.00%> (ø)
src/core/routes/callback.ts 0.00% <0.00%> (ø)
src/core/routes/signin.ts 0.00% <0.00%> (ø)
src/lib/logger.ts 64.28% <0.00%> (-1.57%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9e0ef8...1d3f5a9. Read the comment docs.

ubbe-xyz
ubbe-xyz previously approved these changes Nov 12, 2021
Copy link
Collaborator

@ubbe-xyz ubbe-xyz left a comment

Choose a reason for hiding this comment

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

Looks good 👏🏽 ✅

@balazsorban44 balazsorban44 dismissed ubbe-xyz’s stale review November 12, 2021 11:59

Rethink "NO_SECRET" in prod

@balazsorban44
Copy link
Member Author

I moved the user config assertion to the core, and refactored the code so that in case of a configuration error, we either render the default error page or redirect to the user's custom one. The error is still logged through logger, but the client won't expose the cause of the actual error, it will show something like this:

image

@balazsorban44 balazsorban44 temporarily deployed to Preview November 14, 2021 00:41 Inactive
@github-actions
Copy link

github-actions bot commented Nov 14, 2021

🎉 Experimental release published on npm!

src/core/lib/assert.ts Outdated Show resolved Hide resolved
src/core/lib/assert.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ubbe-xyz ubbe-xyz left a comment

Choose a reason for hiding this comment

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

Pair review, looking great 💪🏽

@balazsorban44 balazsorban44 temporarily deployed to Preview November 15, 2021 17:42 Inactive
@balazsorban44 balazsorban44 merged commit 76bf524 into beta Nov 15, 2021
@balazsorban44 balazsorban44 deleted the feat/make-no-secret-error branch November 15, 2021 17:45
mnphpexpert added a commit to mnphpexpert/next-auth that referenced this pull request Sep 2, 2024
BREAKING CHANGE:

It is now required to set a `secret` in production.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants