Skip to content

Commit b1ca671

Browse files
lawrence3699lawrence3699
committed
fix: validate GitHub API responses in SSO authentication
Check response.ok before calling .json() on all three GitHub API fetch calls in GithubSSO, preventing crashes when GitHub returns error statuses.
1 parent 56f272a commit b1ca671

File tree

2 files changed

+125
-12
lines changed

2 files changed

+125
-12
lines changed

‎packages/server/src/enterprise/sso/GithubSSO.test.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import { describe, expect, it } from '@jest/globals'
2+
3+
/**
4+
* These tests verify the error-handling logic that GithubSSO uses when
5+
* calling GitHub's API. The SSO class itself depends on the running Express
6+
* app through deep transitive imports (SSOBase → getRunningExpressApp → …),
7+
* making direct class-level unit tests impractical without a full app context.
8+
*
9+
* Instead we replicate the exact fetch-and-parse sequences from GithubSSO and
10+
* show that they crash without the response.ok guard and succeed with it.
11+
*/
12+
describe('GithubSSO fetch error handling', () => {
13+
describe('email fetch (passport callback)', () => {
14+
it('crashes with TypeError when GitHub returns a 401 error object instead of an array', () => {
15+
// GitHub API returns this JSON body for 401 Unauthorized:
16+
const githubErrorBody = {
17+
message: 'Bad credentials',
18+
documentation_url: 'https://docs.github.com/rest'
19+
}
20+
21+
// Without response.ok check, code calls .find() on the error object
22+
expect(() => {
23+
const emails = githubErrorBody
24+
;(emails as any).find((email: any) => email.primary && email.verified)
25+
}).toThrow('emails.find is not a function')
26+
})
27+
28+
it('does not crash when response.ok is checked first', () => {
29+
const mockResponse = {
30+
ok: false,
31+
status: 401,
32+
statusText: 'Unauthorized'
33+
}
34+
35+
// With the fix, we check response.ok before parsing
36+
if (!mockResponse.ok) {
37+
const error = {
38+
name: 'SSO_LOGIN_FAILED',
39+
message: `Failed to fetch emails from GitHub: ${mockResponse.status} ${mockResponse.statusText}`
40+
}
41+
expect(error.message).toBe('Failed to fetch emails from GitHub: 401 Unauthorized')
42+
return
43+
}
44+
})
45+
})
46+
47+
describe('testSetup', () => {
48+
it('crashes with SyntaxError when GitHub returns HTML error page', async () => {
49+
// If GitHub is down and returns HTML, JSON.parse fails
50+
const htmlBody = '<html><body>503 Service Unavailable</body></html>'
51+
52+
expect(() => {
53+
JSON.parse(htmlBody)
54+
}).toThrow(SyntaxError)
55+
})
56+
57+
it('returns clear error with response.ok check', () => {
58+
const mockResponse = {
59+
ok: false,
60+
status: 503,
61+
statusText: 'Service Unavailable'
62+
}
63+
64+
if (!mockResponse.ok) {
65+
const result = { error: `GitHub API error: ${mockResponse.status} ${mockResponse.statusText}` }
66+
expect(result).toEqual({ error: 'GitHub API error: 503 Service Unavailable' })
67+
}
68+
})
69+
})
70+
71+
describe('refreshToken', () => {
72+
it('returns clear error instead of crashing on non-200 response', () => {
73+
const mockResponse = {
74+
ok: false,
75+
status: 401,
76+
statusText: 'Unauthorized'
77+
}
78+
79+
if (!mockResponse.ok) {
80+
const result = { error: `GitHub token refresh failed: ${mockResponse.status} ${mockResponse.statusText}` }
81+
expect(result).toEqual({ error: 'GitHub token refresh failed: 401 Unauthorized' })
82+
}
83+
})
84+
})
85+
})

‎packages/server/src/enterprise/sso/GithubSSO.ts‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,20 +34,42 @@ class GithubSSO extends SSOBase {
3434
scope: ['user:email']
3535
},
3636
async (accessToken: string, refreshToken: string, profile: Profile, done: any) => {
37-
// Fetch emails from GitHub API using the access token.
38-
const emailResponse = await fetch('https://api.github.com/user/emails', {
39-
headers: {
40-
Authorization: `token ${accessToken}`,
41-
'User-Agent': 'Node.js'
37+
try {
38+
// Fetch emails from GitHub API using the access token.
39+
const emailResponse = await fetch('https://api.github.com/user/emails', {
40+
headers: {
41+
Authorization: `token ${accessToken}`,
42+
'User-Agent': 'Node.js'
43+
}
44+
})
45+
if (!emailResponse.ok) {
46+
return done(
47+
{
48+
name: 'SSO_LOGIN_FAILED',
49+
message: `Failed to fetch emails from GitHub: ${emailResponse.status} ${emailResponse.statusText}`
50+
},
51+
undefined
52+
)
4253
}
43-
})
44-
const emails = await emailResponse.json()
45-
// Look for a verified primary email.
46-
let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email
47-
if (!primaryEmail && Array.isArray(emails) && emails.length > 0) {
48-
primaryEmail = emails[0].email
54+
const emails = await emailResponse.json()
55+
if (!Array.isArray(emails)) {
56+
return done(
57+
{ name: 'SSO_LOGIN_FAILED', message: 'Unexpected response from GitHub emails API' },
58+
undefined
59+
)
60+
}
61+
// Look for a verified primary email.
62+
let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email
63+
if (!primaryEmail && emails.length > 0) {
64+
primaryEmail = emails[0].email
65+
}
66+
return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken)
67+
} catch (error) {
68+
return done(
69+
{ name: 'SSO_LOGIN_FAILED', message: 'Failed to complete GitHub authentication' },
70+
undefined
71+
)
4972
}
50-
return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken)
5173
}
5274
)
5375
)
@@ -115,6 +137,9 @@ class GithubSSO extends SSOBase {
115137
code: 'dummy_code_for_testing'
116138
})
117139
})
140+
if (!response.ok) {
141+
return { error: `GitHub API error: ${response.status} ${response.statusText}` }
142+
}
118143
const data = await response.json()
119144
if (data.error === 'bad_verification_code') {
120145
return { message: 'ClientID and clientSecret are valid.' }
@@ -143,6 +168,9 @@ class GithubSSO extends SSOBase {
143168
refresh_token: currentRefreshToken
144169
})
145170
})
171+
if (!response.ok) {
172+
return { error: `GitHub token refresh failed: ${response.status} ${response.statusText}` }
173+
}
146174
const data = await response.json()
147175
if (data.error || !data.access_token) {
148176
return { error: 'Failed to get refreshToken from Github.' }

0 commit comments

Comments
 (0)