From a800d0bfe4164fe67fe2f9b32c2599b4602fba7c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 23 Jun 2026 03:07:46 +0000 Subject: [PATCH] auto: bind OAuth login state to browser nonce Co-authored-by: Ummara Ali Syeda --- server/lib/session.js | 39 +++++++++++ server/routes/github.js | 17 ++++- server/routes/google.js | 17 ++++- server/routes/orcid.js | 20 +++++- server/tests/oauth-state.test.js | 113 +++++++++++++++++++++++++++++++ 5 files changed, 201 insertions(+), 5 deletions(-) create mode 100644 server/tests/oauth-state.test.js diff --git a/server/lib/session.js b/server/lib/session.js index af9139b..3d8a224 100644 --- a/server/lib/session.js +++ b/server/lib/session.js @@ -1,6 +1,9 @@ +const crypto = require('crypto') const jwt = require('jsonwebtoken') const JWT_EXPIRY = '7d' +const OAUTH_NONCE_COOKIE = 'oauth_nonce' +const OAUTH_NONCE_MAX_AGE_MS = 10 * 60 * 1000 function signToken(payload) { return jwt.sign(payload, process.env.JWT_SECRET, { expiresIn: JWT_EXPIRY }) @@ -25,10 +28,34 @@ function clearTokenCookie(res) { res.cookie('token', '', tokenCookieOptions(0)) } +function createOAuthNonce() { + return crypto.randomBytes(32).toString('hex') +} + +function hashOAuthNonce(nonce) { + return crypto.createHash('sha256').update(nonce).digest('hex') +} + +function setOAuthNonceCookie(res, nonce) { + res.cookie(OAUTH_NONCE_COOKIE, nonce, tokenCookieOptions(OAUTH_NONCE_MAX_AGE_MS)) +} + +function clearOAuthNonceCookie(res) { + res.cookie(OAUTH_NONCE_COOKIE, '', tokenCookieOptions(0)) +} + function signOAuthState(payload, expiresIn = '10m') { return jwt.sign(payload, process.env.JWT_SECRET, { expiresIn }) } +function signOAuthLoginState(provider, nonce) { + return signOAuthState({ + provider, + mode: 'login', + nonce_hash: hashOAuthNonce(nonce), + }) +} + function verifyOAuthState(token) { try { return jwt.verify(token, process.env.JWT_SECRET) @@ -37,6 +64,12 @@ function verifyOAuthState(token) { } } +function hasValidOAuthNonce(req, statePayload) { + const nonce = req.cookies?.[OAUTH_NONCE_COOKIE] + if (!nonce || !statePayload?.nonce_hash) return false + return hashOAuthNonce(nonce) === statePayload.nonce_hash +} + function signCompletionToken(payload) { return jwt.sign(payload, process.env.JWT_SECRET, { expiresIn: '30m' }) } @@ -53,9 +86,15 @@ module.exports = { signToken, setTokenCookie, clearTokenCookie, + createOAuthNonce, + setOAuthNonceCookie, + clearOAuthNonceCookie, signOAuthState, + signOAuthLoginState, verifyOAuthState, + hasValidOAuthNonce, signCompletionToken, verifyCompletionToken, JWT_EXPIRY, + OAUTH_NONCE_COOKIE, } diff --git a/server/routes/github.js b/server/routes/github.js index 95d16f7..7e59c6c 100644 --- a/server/routes/github.js +++ b/server/routes/github.js @@ -3,8 +3,13 @@ const jwt = require('jsonwebtoken') const { signToken, setTokenCookie, - signOAuthState, + createOAuthNonce, + setOAuthNonceCookie, + clearOAuthNonceCookie, verifyOAuthState, + signOAuthState, + signOAuthLoginState, + hasValidOAuthNonce, } = require('../lib/session') const { formatUserResponse, linkOAuthProvider } = require('../lib/oauthUsers') const { findOrCreateOAuthUser } = require('./google') @@ -39,7 +44,9 @@ router.get('/url', (req, res) => { if (!process.env.GITHUB_CLIENT_ID) { return res.status(503).json({ error: 'GitHub sign-in is not configured' }) } - const state = signOAuthState({ provider: 'github', mode: 'login' }) + const nonce = createOAuthNonce() + const state = signOAuthLoginState('github', nonce) + setOAuthNonceCookie(res, nonce) res.json({ url: buildGithubAuthUrl(state) }) }) @@ -131,6 +138,11 @@ router.post('/callback', async (req, res) => { return res.status(400).json({ error: 'Invalid or expired state' }) } + if (statePayload.mode === 'login' && !hasValidOAuthNonce(req, statePayload)) { + clearOAuthNonceCookie(res) + return res.status(400).json({ error: 'Invalid or expired state' }) + } + const githubData = await fetchGithubProfile(code) if (!githubData) { return res.status(502).json({ error: 'Failed to obtain GitHub access token' }) @@ -165,6 +177,7 @@ router.post('/callback', async (req, res) => { const sessionToken = signToken({ userId: user.id, username: user.username }) setTokenCookie(res, sessionToken) + clearOAuthNonceCookie(res) res.json(formatUserResponse(user)) } catch (err) { if (err.statusCode) { diff --git a/server/routes/google.js b/server/routes/google.js index 1b0f777..8cfbfb5 100644 --- a/server/routes/google.js +++ b/server/routes/google.js @@ -5,8 +5,13 @@ const AppError = require('../lib/AppError') const { signToken, setTokenCookie, - signOAuthState, + createOAuthNonce, + setOAuthNonceCookie, + clearOAuthNonceCookie, verifyOAuthState, + signOAuthState, + signOAuthLoginState, + hasValidOAuthNonce, } = require('../lib/session') const { findAvailableUsername, formatUserResponse, linkOAuthProvider } = require('../lib/oauthUsers') const authenticateToken = require('../middleware/authenticateToken') @@ -44,7 +49,9 @@ router.get('/url', (req, res) => { if (!process.env.GOOGLE_CLIENT_ID) { return res.status(503).json({ error: 'Google sign-in is not configured' }) } - const state = signOAuthState({ provider: 'google', mode: 'login' }) + const nonce = createOAuthNonce() + const state = signOAuthLoginState('google', nonce) + setOAuthNonceCookie(res, nonce) res.json({ url: buildGoogleAuthUrl(state) }) }) @@ -93,6 +100,11 @@ router.post('/callback', async (req, res) => { return res.status(400).json({ error: 'Invalid or expired state' }) } + if (statePayload.mode === 'login' && !hasValidOAuthNonce(req, statePayload)) { + clearOAuthNonceCookie(res) + return res.status(400).json({ error: 'Invalid or expired state' }) + } + const profile = await exchangeGoogleCode(code) if (!profile?.id) { return res.status(502).json({ error: 'Failed to fetch Google profile' }) @@ -127,6 +139,7 @@ router.post('/callback', async (req, res) => { const sessionToken = signToken({ userId: user.id, username: user.username }) setTokenCookie(res, sessionToken) + clearOAuthNonceCookie(res) res.json(formatUserResponse(user)) } catch (err) { if (err.statusCode) { diff --git a/server/routes/orcid.js b/server/routes/orcid.js index ca34445..2fe533f 100644 --- a/server/routes/orcid.js +++ b/server/routes/orcid.js @@ -6,8 +6,13 @@ const authenticateToken = require('../middleware/authenticateToken') const { signToken, setTokenCookie, + createOAuthNonce, + setOAuthNonceCookie, + clearOAuthNonceCookie, signOAuthState, verifyOAuthState, + signOAuthLoginState, + hasValidOAuthNonce, signCompletionToken, } = require('../lib/session') const { formatUserResponse } = require('../lib/oauthUsers') @@ -84,7 +89,9 @@ router.get('/login/url', (req, res) => { if (!process.env.ORCID_CLIENT_ID) { return res.status(503).json({ error: 'ORCID sign-in is not configured' }) } - const state = signOAuthState({ mode: 'login', provider: 'orcid' }) + const nonce = createOAuthNonce() + const state = signOAuthLoginState('orcid', nonce) + setOAuthNonceCookie(res, nonce) return res.json({ url: buildOrcidAuthUrl(state) }) }) @@ -143,6 +150,15 @@ router.post('/callback', async (req, res) => { return res.status(400).json({ error: 'Invalid or expired state' }) } + if (statePayload.mode === 'login') { + if (statePayload.provider !== 'orcid' || !hasValidOAuthNonce(req, statePayload)) { + clearOAuthNonceCookie(res) + return res.status(400).json({ error: 'Invalid or expired state' }) + } + } else if (statePayload.provider && statePayload.provider !== 'orcid') { + return res.status(400).json({ error: 'Invalid or expired state' }) + } + const tokenData = await exchangeOrcidCode(code) if (!tokenData) { return res.status(502).json({ error: 'Failed to exchange ORCID code' }) @@ -188,6 +204,7 @@ async function handleOrcidLogin(req, res, { orcidId, displayName }) { const user = existing.rows[0] const sessionToken = signToken({ userId: user.id, username: user.username }) setTokenCookie(res, sessionToken) + clearOAuthNonceCookie(res) return res.json({ ...formatUserResponse(user), mode: 'login' }) } @@ -198,6 +215,7 @@ async function handleOrcidLogin(req, res, { orcidId, displayName }) { email: null, }) + clearOAuthNonceCookie(res) return res.json({ needs_completion: true, completion_token: completionToken, diff --git a/server/tests/oauth-state.test.js b/server/tests/oauth-state.test.js new file mode 100644 index 0000000..af25da9 --- /dev/null +++ b/server/tests/oauth-state.test.js @@ -0,0 +1,113 @@ +const request = require('supertest') +const app = require('../index') +const { + OAUTH_NONCE_COOKIE, + verifyOAuthState, +} = require('../lib/session') + +const originalEnv = { + JWT_SECRET: process.env.JWT_SECRET, + CLIENT_URL: process.env.CLIENT_URL, + GOOGLE_CLIENT_ID: process.env.GOOGLE_CLIENT_ID, + GITHUB_CLIENT_ID: process.env.GITHUB_CLIENT_ID, + ORCID_CLIENT_ID: process.env.ORCID_CLIENT_ID, +} + +const providers = [ + { + provider: 'google', + urlPath: '/auth/google/url', + callbackPath: '/auth/google/callback', + clientIdEnv: 'GOOGLE_CLIENT_ID', + }, + { + provider: 'github', + urlPath: '/auth/github/url', + callbackPath: '/auth/github/callback', + clientIdEnv: 'GITHUB_CLIENT_ID', + }, + { + provider: 'orcid', + urlPath: '/auth/orcid/login/url', + callbackPath: '/auth/orcid/callback', + clientIdEnv: 'ORCID_CLIENT_ID', + }, +] + +function restoreEnv() { + Object.entries(originalEnv).forEach(([key, value]) => { + if (value === undefined) { + delete process.env[key] + } else { + process.env[key] = value + } + }) +} + +function configureOAuth(provider) { + process.env.JWT_SECRET = 'test-secret' + process.env.CLIENT_URL = 'https://client.example.test' + process.env[provider.clientIdEnv] = `${provider.provider}-client-id` +} + +function stateFromUrl(url) { + return new URL(url).searchParams.get('state') +} + +function nonceCookieFrom(res) { + return res.headers['set-cookie']?.find(cookie => ( + cookie.startsWith(`${OAUTH_NONCE_COOKIE}=`) + )) +} + +afterEach(() => { + jest.restoreAllMocks() + restoreEnv() +}) + +describe('OAuth login state nonce', () => { + it.each(providers)('binds $provider login state to an httpOnly nonce cookie', async (provider) => { + configureOAuth(provider) + + const res = await request(app).get(provider.urlPath) + + expect(res.status).toBe(200) + expect(nonceCookieFrom(res)).toContain('HttpOnly') + + const statePayload = verifyOAuthState(stateFromUrl(res.body.url)) + expect(statePayload).toMatchObject({ + provider: provider.provider, + mode: 'login', + }) + expect(statePayload.nonce_hash).toMatch(/^[a-f0-9]{64}$/) + }) + + it.each(providers)('rejects $provider login callbacks without the nonce cookie', async (provider) => { + configureOAuth(provider) + const fetchSpy = jest.spyOn(global, 'fetch') + + const urlRes = await request(app).get(provider.urlPath) + const callbackRes = await request(app) + .post(provider.callbackPath) + .send({ code: 'attacker-code', state: stateFromUrl(urlRes.body.url) }) + + expect(callbackRes.status).toBe(400) + expect(callbackRes.body.error).toBe('Invalid or expired state') + expect(fetchSpy).not.toHaveBeenCalled() + }) + + it.each(providers)('rejects $provider login callbacks with another browser nonce', async (provider) => { + configureOAuth(provider) + const fetchSpy = jest.spyOn(global, 'fetch') + + const urlRes = await request(app).get(provider.urlPath) + const callbackRes = await request(app) + .post(provider.callbackPath) + .set('Cookie', `${OAUTH_NONCE_COOKIE}=wrong-browser-nonce`) + .send({ code: 'attacker-code', state: stateFromUrl(urlRes.body.url) }) + + expect(callbackRes.status).toBe(400) + expect(callbackRes.body.error).toBe('Invalid or expired state') + expect(fetchSpy).not.toHaveBeenCalled() + }) +})