okupter / kitforstartups

The Open Source SvelteKit SaaS boilerplate.
https://kitforstartups.com
MIT License
684 stars 31 forks source link

Github auth flow can fail because githubUser.email can be null #8

Closed ocluf closed 8 months ago

ocluf commented 8 months ago

When you use the github oauth flow src/routes/api/oauth/github/callback/+server.ts it currently fails when the user does not have a public emailaddress set in their github account. (githubUser.email becomes null and it throws a 400)

https://stackoverflow.com/questions/35373995/github-user-email-is-null-despite-useremail-scope

I ran into this during testing because I don't have a public email on my github, but it could be a footgun for developers who do and think everything is working fine.

The template should probably be adjusted to either:

  1. Not require an email when logging in with github
  2. Include the ['user:email'] scope in githubAuthOptions and then using the user/emailsendpoint retrieve the email in case of a null.

Example

     // in src/routes/api/oauth/github/callback/+server.ts
    const { getExistingUser, githubUser, createUser, createKey, githubTokens } =
            await githubAuth.validateCallback(code);

        const getUser = async () => {
            const existingUser = await getExistingUser();

            if (existingUser) {
                return existingUser;
            }

            let githubUserEmail = githubUser.email;
            if (!githubUserEmail) {
                const email = await getPrimaryEmail(githubTokens.accessToken);
                if (!email) {
                    throw error(400, 'No email provided by GitHub');
                } else {
                    githubUserEmail = email;
                }
            }
// github.ts
import { Octokit } from 'octokit';

export async function getPrimaryEmail(accesstoken: string) {
    const octokit = new Octokit({
        auth: accesstoken
    });

    let emails = await octokit.request('GET /user/emails', {
        headers: {
            'X-GitHub-Api-Version': '2022-11-28'
        }
    });

    let primary = emails.data.find((email) => email.primary)?.email;

    return primary;
}
JustinyAhin commented 8 months ago

@ocluf this is such a good catch. I had no idea that the public email on GitHub could be null. I made this PR #9 to fix the issue based on your feedback (thanks for providing the code snippets).

Could you please make a test and let me know if it works fine before I merge the PR?

Thanks.

ocluf commented 8 months ago

Hey @JustinyAhin

I had already implemented it for my application with those code snippets and it worked for me. Had a quick look at the pull request and LGTM

JustinyAhin commented 8 months ago

Fixed by #9