fix: validate GitHub API responses in SSO authentication#6137
fix: validate GitHub API responses in SSO authentication#6137lawrence3699 wants to merge 1 commit intoFlowiseAI:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Improves robustness of the GitHub SSO OAuth callback by handling non-OK responses and unexpected payloads from the GitHub /user/emails API, preventing callback crashes and enabling user-facing error redirects.
Changes:
- Wrap GitHub email-fetch logic in
try/catchand returnSSO_LOGIN_FAILEDon failure. - Check
emailResponse.okbefore parsing JSON and validate the parsed payload is an array. - Keep existing primary-email selection logic while avoiding
.find()on non-arrays.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email | ||
| if (!primaryEmail && emails.length > 0) { | ||
| primaryEmail = emails[0].email | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| } | |
| if (typeof primaryEmail !== 'string' || primaryEmail.trim() === '') { | |
| return done( | |
| { name: 'SSO_LOGIN_FAILED', message: 'No usable email address returned by GitHub' }, | |
| undefined | |
| ) | |
| } |
There was a problem hiding this comment.
Code Review
This pull request enhances the GitHub SSO implementation by adding comprehensive error handling, response validation, and try-catch blocks to the email fetching logic. Feedback suggests updating the deprecated "token" prefix in the Authorization header to "Bearer" to align with current API standards and adding a validation check to ensure an email address is found before proceeding with the login flow to prevent potential downstream errors.
| // Fetch emails from GitHub API using the access token. | ||
| const emailResponse = await fetch('https://api.github.com/user/emails', { | ||
| headers: { | ||
| Authorization: `token ${accessToken}`, |
There was a problem hiding this comment.
| let primaryEmail = emails.find((email: any) => email.primary && email.verified)?.email | ||
| if (!primaryEmail && emails.length > 0) { | ||
| primaryEmail = emails[0].email | ||
| } | ||
| return this.verifyAndLogin(this.app, primaryEmail, done, profile, accessToken, refreshToken) |
There was a problem hiding this comment.
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
- Multiple early returns for validation can be preferable to a single error-aggregating block if the latter is considered more confusing or less readable.
Check response.ok before calling .json() on all three GitHub API fetch calls in GithubSSO, preventing crashes when GitHub returns error statuses.
c54704a to
b1ca671
Compare
Ran into this while setting up GitHub SSO on a self-hosted Flowise instance. When the GitHub token was misconfigured, the login flow crashed with
TypeError: emails.find is not a functioninstead of showing a meaningful error.The root cause:
GithubSSO.tscallsfetch()against GitHub APIs in three places but never checksresponse.okbefore calling.json(). When GitHub returns a non-2xx status (401, 403, 500), the response body is an error object like{"message":"Bad credentials"}, not an array — soemails.find()blows up. If GitHub returns a 5xx HTML error page,.json()itself throws aSyntaxError.This affects:
TypeError, and since there's no try-catch wrapper, the error propagates unhandledThe fix adds
response.okchecks before.json()in all three fetch calls, following the same pattern already used inpackages/components/src/utils.ts(line 1490):For the email fetch callback, I also wrapped the whole body in try-catch and added an
Array.isArrayguard so that an unexpected response shape gets surfaced asSSO_LOGIN_FAILEDwith a clear message rather than a raw TypeError.Reproduction:
Tests: Added
GithubSSO.test.tswith 5 tests demonstrating the crash scenarios (TypeError on non-array response, SyntaxError on HTML error page) and verifying the fix produces clear error messages.