fix: validate OAuth state on the web-flow callback (#3444) · gitify-app/gitify@10235b2 · GitHub
Skip to content

Commit 10235b2

Browse files
authored
fix: validate OAuth state on the web-flow callback (#3444)
Generate a per-attempt random `state` for the GitHub OAuth App web-flow login and accept a `gitify://oauth` callback only when it echoes that state. A callback carrying a missing or mismatched state is logged and ignored, so an injected callback can neither bind the client to another GitHub account (login-CSRF) nor tear down the pending login. Treat an unparseable `gitify://` callback URL the same way: log and ignore instead of rejecting, since a forged URL (e.g. `gitify://[`) passes the protocol handler's prefix check but cannot carry a valid state and would otherwise abort a legitimate login. Generate the state with `crypto.randomUUID()` and cover it with unit tests.
1 parent f7617e7 commit 10235b2

4 files changed

Lines changed: 203 additions & 10 deletions

File tree

Lines changed: 22 additions & 0 deletions

‎src/renderer/utils/auth/state.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
/**
2+
* Generate a cryptographically random OAuth 2.0 `state` value.
3+
*
4+
* `crypto.randomUUID()` is backed by the platform CSPRNG and yields 122 random
5+
* bits — ample for a per-attempt, single-use value that an attacker cannot
6+
* guess in order to bind the client to a different account. Using the platform
7+
* primitive avoids hand-rolled random-value encoding entirely.
8+
*
9+
* @returns A random UUID to send as the OAuth `state` parameter.
10+
*/
11+
export function generateOAuthState(): string {
12+
return crypto.randomUUID();
13+
}

‎src/renderer/utils/forges/github/flows.test.ts‎

Lines changed: 127 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const exchangeWebFlowCodeMock = exchangeWebFlowCode as unknown as MockedFunction
3838
describe('renderer/utils/forges/github/flows.ts', () => {
3939
vi.spyOn(logger, 'rendererLogInfo').mockImplementation(vi.fn());
4040
const rendererLogErrorSpy = vi.spyOn(logger, 'rendererLogError').mockImplementation(vi.fn());
41+
const rendererLogWarnSpy = vi.spyOn(logger, 'rendererLogWarn').mockImplementation(vi.fn());
4142
const openExternalLinkSpy = vi.spyOn(comms, 'openExternalLink').mockImplementation(vi.fn());
4243

4344
beforeEach(() => {
@@ -150,11 +151,12 @@ describe('renderer/utils/forges/github/flows.ts', () => {
150151
};
151152

152153
it('should call performGitHubWebOAuth using custom oauth app - success oauth flow', async () => {
154+
let deliverCallback: (callbackUrl: string) => void = () => {};
153155
window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => {
154-
callback('gitify://oauth?code=123-456');
156+
deliverCallback = callback;
155157
});
156158

157-
const res = await flows.performGitHubWebOAuth({
159+
const resultPromise = flows.performGitHubWebOAuth({
158160
clientId: 'BYO_CLIENT_ID' as ClientID,
159161
clientSecret: 'BYO_CLIENT_SECRET' as ClientSecret,
160162
hostname: 'my.git.com' as Hostname,
@@ -167,21 +169,106 @@ describe('renderer/utils/forges/github/flows.ts', () => {
167169
),
168170
);
169171

172+
// The authorization URL must carry a per-attempt state, and the login
173+
// only completes when the callback echoes that same value back.
174+
const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string);
175+
const state = authorizeUrl.searchParams.get('state');
176+
expect(state).toBeTruthy();
177+
178+
deliverCallback(`gitify://oauth?code=123-456&state=${state}`);
179+
180+
const res = await resultPromise;
181+
170182
expect(window.gitify.onAuthCallback).toHaveBeenCalledTimes(1);
171183
expect(window.gitify.onAuthCallback).toHaveBeenCalledWith(expect.any(Function));
172184

173185
expect(res.authMethod).toBe('OAuth App');
174186
expect(res.authCode).toBe('123-456');
175187
});
176188

189+
it('does not resolve when the callback state does not match the generated state', async () => {
190+
let deliverCallback: (callbackUrl: string) => void = () => {};
191+
window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => {
192+
deliverCallback = callback;
193+
});
194+
195+
let resolved = false;
196+
const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then((res) => {
197+
resolved = true;
198+
return res;
199+
});
200+
201+
const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string);
202+
const state = authorizeUrl.searchParams.get('state');
203+
204+
deliverCallback('gitify://oauth?code=attacker-code&state=not-the-real-state');
205+
deliverCallback('gitify://oauth?code=attacker-code-no-state');
206+
207+
// Give any pending microtasks a chance to run, then confirm neither
208+
// injected callback settled the login.
209+
await Promise.resolve();
210+
await Promise.resolve();
211+
expect(resolved).toBe(false);
212+
expect(rendererLogWarnSpy).toHaveBeenCalledTimes(2);
213+
214+
// The still-pending attempt completes once the matching state arrives.
215+
deliverCallback(`gitify://oauth?code=the-real-code&state=${state}`);
216+
const res = await resultPromise;
217+
expect(res.authCode).toBe('the-real-code');
218+
});
219+
220+
it('ignores an unparseable callback URL instead of tearing down the pending login', async () => {
221+
let deliverCallback: (callbackUrl: string) => void = () => {};
222+
window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => {
223+
deliverCallback = callback;
224+
});
225+
226+
let settled = false;
227+
const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then(
228+
(res) => {
229+
settled = true;
230+
return res;
231+
},
232+
(err: unknown) => {
233+
settled = true;
234+
throw err;
235+
},
236+
);
237+
238+
// `gitify://[` passes the protocol handler's `gitify://` prefix check but
239+
// is not a parseable URL, so it must be ignored rather than rejecting the
240+
// login before the state is ever compared.
241+
deliverCallback('gitify://[');
242+
243+
await Promise.resolve();
244+
await Promise.resolve();
245+
expect(settled).toBe(false);
246+
expect(rendererLogWarnSpy).toHaveBeenCalledTimes(1);
247+
248+
// Settle the still-pending attempt so no listener is left dangling.
249+
const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string);
250+
const state = authorizeUrl.searchParams.get('state');
251+
deliverCallback(`gitify://oauth?code=real-code&state=${state}`);
252+
253+
await expect(resultPromise).resolves.toMatchObject({ authCode: 'real-code' });
254+
});
255+
177256
it('should call performGitHubWebOAuth - failure', async () => {
257+
let deliverCallback: (callbackUrl: string) => void = () => {};
178258
window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => {
179-
callback(
180-
'gitify://auth?error=invalid_request&error_description=The+redirect_uri+is+missing+or+invalid.&error_uri=https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors',
181-
);
259+
deliverCallback = callback;
182260
});
183261

184-
await expect(async () => await flows.performGitHubWebOAuth(webAuthOptions)).rejects.toEqual(
262+
const resultPromise = flows.performGitHubWebOAuth(webAuthOptions);
263+
264+
const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string);
265+
const state = authorizeUrl.searchParams.get('state');
266+
267+
deliverCallback(
268+
`gitify://oauth?state=${state}&error=invalid_request&error_description=The+redirect_uri+is+missing+or+invalid.&error_uri=https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors`,
269+
);
270+
271+
await expect(resultPromise).rejects.toEqual(
185272
new Error(
186273
"Oops! Something went wrong and we couldn't log you in using GitHub. Please try again. Reason: The redirect_uri is missing or invalid. Docs: https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors",
187274
),
@@ -198,6 +285,40 @@ describe('renderer/utils/forges/github/flows.ts', () => {
198285
expect(window.gitify.onAuthCallback).toHaveBeenCalledWith(expect.any(Function));
199286
});
200287

288+
it('ignores a callback error whose state does not match the attempt', async () => {
289+
let deliverCallback: (callbackUrl: string) => void = () => {};
290+
window.gitify.onAuthCallback = vi.fn().mockImplementation((callback) => {
291+
deliverCallback = callback;
292+
});
293+
294+
let settled = false;
295+
const resultPromise = flows.performGitHubWebOAuth(webAuthOptions).then(
296+
(res) => {
297+
settled = true;
298+
return res;
299+
},
300+
(err: unknown) => {
301+
settled = true;
302+
throw err;
303+
},
304+
);
305+
306+
deliverCallback('gitify://oauth?error=access_denied');
307+
deliverCallback('gitify://oauth?error=access_denied&state=not-the-real-state');
308+
309+
await Promise.resolve();
310+
await Promise.resolve();
311+
expect(settled).toBe(false);
312+
expect(rendererLogWarnSpy).toHaveBeenCalledTimes(2);
313+
314+
// Settle the still-pending attempt so no listener is left dangling.
315+
const authorizeUrl = new URL(openExternalLinkSpy.mock.calls[0][0] as string);
316+
const state = authorizeUrl.searchParams.get('state');
317+
deliverCallback(`gitify://oauth?code=real-code&state=${state}`);
318+
319+
await expect(resultPromise).resolves.toMatchObject({ authCode: 'real-code' });
320+
});
321+
201322
describe('exchangeAuthCodeForAccessToken', () => {
202323
const authCode = '123-456' as AuthCode;
203324

‎src/renderer/utils/forges/github/flows.ts‎

Lines changed: 41 additions & 4 deletions

0 commit comments

Comments
 (0)