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

s/OAUTH_CALLBACK_ERROR/CALLBACK_OAUTH_ERROR/ #5027

Closed
wants to merge 1 commit into from

Conversation

sato11
Copy link
Contributor

@sato11 sato11 commented Jul 26, 2022

☕️ Reasoning

There is an inconsistent use of two of the error codes, OAUTH_CALLBACK_ERROR and CALLBACK_OAUTH_ERROR. Especially in routes/callback.ts the two sit next to each other and emit subtly discrepant errors. They apparently do not contain asymmetrical information, and practically look interchangeable. But since errors page only mentions the latter, let's align them and make use of it: https://next-auth.js.org/errors#callback_oauth_error. With this fix I hope anybody like me will no longer navigate unfortunately through the different link with a broken anchor.

Skimming through the history, I found OAUTH_CALLBACK_ERROR had been replaced in #235, reintroduced perhaps accidentally in #332 and #1698. Let me know if I'm missing any context!

Below is the like of what I've seen in the log:

[next-auth][error][OAUTH_CALLBACK_ERROR] 
https://next-auth.js.org/errors#oauth_callback_error connect ECONNREFUSED 127.0.0.1:80 {
  error: {
    message: 'connect ECONNREFUSED 127.0.0.1:80',
    stack: 'Error: connect ECONNREFUSED 127.0.0.1:80\n' +
      '    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1237:16)',
    name: 'Error'
  },
  providerId: 'some-provider',
  message: 'connect ECONNREFUSED 127.0.0.1:80'
}
[next-auth][error][CALLBACK_OAUTH_ERROR] 
https://next-auth.js.org/errors#callback_oauth_error connect ECONNREFUSED 127.0.0.1:80 Error: connect ECONNREFUSED 127.0.0.1:80
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1237:16) {
  name: 'OAuthCallbackError',
  code: 'ECONNREFUSED'
}

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Please scout and link issues that might be solved by this PR.

Fixes: INSERT_ISSUE_LINK_HERE

📌 Resources

@sato11 sato11 requested a review from ThangHuuVu as a code owner July 26, 2022 10:18
@vercel
Copy link

vercel bot commented Jul 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
next-auth ⬜️ Ignored (Inspect) Jul 26, 2022 at 10:18AM (UTC)

@github-actions github-actions bot added the core Refers to `@auth/core` label Jul 26, 2022
@ThangHuuVu
Copy link
Member

Hey, @sato11 thanks for reporting this! Gentle reminder: we prefer you to report issues first before opening PRs, unless it's something trivial like a typo.
So I see that there are several issues that are kind of related:

  1. Doc issue: I saw that OAUTH_CALLBACK_ERROR is a thing and documented here. We could merge the doc content of OAUTH_CALLBACK_ERROR and CALLBACK_OAUTH_ERROR.
  2. Looking at the way current OAUTH_CALLBACK_ERROR is used at
    logger.error("OAUTH_CALLBACK_ERROR", error as Error)
    return { redirect: `${url}/error?error=Callback`, cookies }
    I think we could rename this error code to something like CALLBACK_ERROR, as in general callback errors, and add docs for it cc @balazsorban44 🤔
  3. logger.error("OAUTH_CALLBACK_ERROR", error as Error)
    I believe we don't need this one because we've already logged error: OAuthCallbackError and rethrow at
    logger.error("OAUTH_CALLBACK_ERROR", {
    error: error as Error,
    providerId: provider.id,
    })
    throw new OAuthCallbackError(error as Error)

    So IMO we only need to remove that error log and keep the redirect behavior only.

I'm going to close this one now, please open a new issue and link to it in your follow-up PR!

@ThangHuuVu ThangHuuVu closed this Jul 27, 2022
@sato11
Copy link
Contributor Author

sato11 commented Jul 28, 2022

Oops that was my bad. Sorry that I missed the course! Also thanks for sorting things up! Will look into it 🙂

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.

None yet

2 participants