Skip to content

fix: validate GitHub API responses in SSO authentication #6137

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.

Sign up for GitHub

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

Open
lawrence3699 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: validate GitHub API responses in SSO authentication #6137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1ca671
fix: validate GitHub API responses in SSO authentication
lawrence3699 Apr 3, 2026
24c012b
fix: use Bearer auth header and guard undefined primaryEmail in GitHu…
lawrence3699 Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions packages/server/src/enterprise/sso/GithubSSO.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { describe, expect, it } from '@jest/globals'

/**
* These tests verify the error-handling logic that GithubSSO uses when
* calling GitHub's API. The SSO class itself depends on the running Express
* app through deep transitive imports (SSOBase → getRunningExpressApp → …),
* making direct class-level unit tests impractical without a full app context.
*
* Instead we replicate the exact fetch-and-parse sequences from GithubSSO and
* show that they crash without the response.ok guard and succeed with it.
*/
describe('GithubSSO fetch error handling', () => {
describe('email fetch (passport callback)', () => {
it('crashes with TypeError when GitHub returns a 401 error object instead of an array', () => {
// GitHub API returns this JSON body for 401 Unauthorized:
const githubErrorBody = {
message: 'Bad credentials',
documentation_url: 'https://docs.github.com/rest'
}

// Without response.ok check, code calls .find() on the error object
expect(() => {
const emails = githubErrorBody
;(emails as any).find((email: any) => email.primary && email.verified)
}).toThrow('emails.find is not a function')
})

it('does not crash when response.ok is checked first', () => {
const mockResponse = {
ok: false,
status: 401,
statusText: 'Unauthorized'
}

// With the fix, we check response.ok before parsing
if (!mockResponse.ok) {
const error = {
name: 'SSO_LOGIN_FAILED',
message: `Failed to fetch emails from GitHub: ${mockResponse.status} ${mockResponse.statusText}`
}
expect(error.message).toBe('Failed to fetch emails from GitHub: 401 Unauthorized')
return
}
})
})

describe('testSetup', () => {
it('crashes with SyntaxError when GitHub returns HTML error page', async () => {
// If GitHub is down and returns HTML, JSON.parse fails
const htmlBody = '<html><body>503 Service Unavailable</body></html>'

expect(() => {
JSON.parse(htmlBody)
}).toThrow(SyntaxError)
})

it('returns clear error with response.ok check', () => {
const mockResponse = {
ok: false,
status: 503,
statusText: 'Service Unavailable'
}

if (!mockResponse.ok) {
const result = { error: `GitHub API error: ${mockResponse.status} ${mockResponse.statusText}` }
expect(result).toEqual({ error: 'GitHub API error: 503 Service Unavailable' })
}
})
})

describe('refreshToken', () => {
it('returns clear error instead of crashing on non-200 response', () => {
const mockResponse = {
ok: false,
status: 401,
statusText: 'Unauthorized'
}

if (!mockResponse.ok) {
const result = { error: `GitHub token refresh failed: ${mockResponse.status} ${mockResponse.statusText}` }
expect(result).toEqual({ error: 'GitHub token refresh failed: 401 Unauthorized' })
}
})
})
})
58 changes: 46 additions & 12 deletions packages/server/src/enterprise/sso/GithubSSO.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,48 @@ class GithubSSO extends SSOBase {
scope: ['user:email']
},
async (accessToken: string, refreshToken: string, profile: Profile, done: any) => {
// Fetch emails from GitHub API using the access token.
const emailResponse = await fetch('https://api.github.com/user/emails', {
headers: {
Authorization: `token ${accessToken}`,
'User-Agent': 'Node.js'
try {
// Fetch emails from GitHub API using the access token.
const emailResponse = await fetch('https://api.github.com/user/emails', {
headers: {
Authorization: `Bearer ${accessToken}`,
'User-Agent': 'Node.js'
}
})
if (!emailResponse.ok) {
return done(
{
name: 'SSO_LOGIN_FAILED',
message: `Failed to fetch emails from GitHub: ${emailResponse.status} ${emailResponse.statusText}`
},
undefined
)
}
})
const emails = await emailResponse.json()
// Look for a verified primary email.
let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email
if (!primaryEmail && Array.isArray(emails) && emails.length > 0) {
primaryEmail = emails[0].email
const emails = await emailResponse.json()
if (!Array.isArray(emails)) {
return done(
{ name: 'SSO_LOGIN_FAILED', message: 'Unexpected response from GitHub emails API' },
undefined
)
}
// Look for a verified primary email.
let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email
if (!primaryEmail && emails.length > 0) {
primaryEmail = emails[0].email
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

primaryEmail can still end up undefined (e.g., if the /user/emails array is empty or entries are missing email). This then gets passed into verifyAndLogin even though it expects a string, leading to a DB lookup with an invalid email and a generic failure. Consider explicitly validating primaryEmail is a non-empty string and returning SSO_LOGIN_FAILED (with a clear message) when no usable email is available.

Suggested change
}
}
if (typeof primaryEmail !== 'string' || primaryEmail.trim() === '') {
return done(
{ name: 'SSO_LOGIN_FAILED', message: 'No usable email address returned by GitHub' },
undefined
)
}

Copilot uses AI. Check for mistakes.
if (!primaryEmail) {
return done(
{ name: 'SSO_LOGIN_FAILED', message: 'No usable email address found in GitHub account' },
undefined
)
}
return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken)
Comment on lines +62 to +72
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

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

medium

If no email is found in the GitHub response (e.g., an empty array or no primary/verified email), primaryEmail will be undefined. Passing undefined to verifyAndLogin can cause database errors or unexpected behavior during the registration/login flow. It is better to validate the presence of an email address and return a specific error message if none is found.

                            let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email
                            if (!primaryEmail && emails.length > 0) {
                                primaryEmail = emails[0].email
                            }

                            if (!primaryEmail) {
                                return done(
                                    { name: 'SSO_LOGIN_FAILED', message: 'No email address found in your GitHub account.' },
                                    undefined
                                )
                            }

                            return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken)
References
  1. Multiple early returns for validation can be preferable to a single error-aggregating block if the latter is considered more confusing or less readable.

} catch (error) {
return done(
{ name: 'SSO_LOGIN_FAILED', message: 'Failed to complete GitHub authentication' },
undefined
)
}
return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken)
}
)
)
Expand Down Expand Up @@ -115,6 +143,9 @@ class GithubSSO extends SSOBase {
code: 'dummy_code_for_testing'
})
})
if (!response.ok) {
return { error: `GitHub API error: ${response.status} ${response.statusText}` }
}
const data = await response.json()
if (data.error === 'bad_verification_code') {
return { message: 'ClientID and clientSecret are valid.' }
Expand Down Expand Up @@ -143,6 +174,9 @@ class GithubSSO extends SSOBase {
refresh_token: currentRefreshToken
})
})
if (!response.ok) {
return { error: `GitHub token refresh failed: ${response.status} ${response.statusText}` }
}
const data = await response.json()
if (data.error || !data.access_token) {
return { error: 'Failed to get refreshToken from Github.' }
Expand Down