diff --git a/forge/db/controllers/AccessToken.js b/forge/db/controllers/AccessToken.js index 151d753430..c8278429d5 100644 --- a/forge/db/controllers/AccessToken.js +++ b/forge/db/controllers/AccessToken.js @@ -6,12 +6,18 @@ const DEFAULT_TOKEN_SESSION_EXPIRY = 1000 * 60 * 30 // 30 mins session - with re const DEFAULT_REFRESH_TOKEN_EXPIRY = 1000 * 60 * 60 * 24 * 30 // 30 days - sliding refresh token lifetime -// Concurrent refreshes of the same refresh token reuse the cached access token -// rather than each minting a new one and overwriting the row. Re-mint once the -// cached token is within this window of expiry. +// Concurrent refreshes of the same refresh token converge on one rotation result +// via this shared cache (Valkey in production) rather than each minting its own +// and racing on the row. The result carries the new refresh token so every caller +// is handed the same one; a stale cached access token is re-minted once within +// this window of expiry so a client is never handed a token about to die. const MCP_ACCESS_TOKEN_CACHE = 'mcp-oauth-access-token' const MCP_ACCESS_TOKEN_REMAINING_LIMIT = 1000 * 60 * 5 // 5 minutes +// A rotated-out refresh token stays valid for this long so a concurrent or retried +// refresh still succeeds; presenting it after the window is treated as a replay. +const MCP_REFRESH_TOKEN_GRACE = 1000 * 60 // 60 seconds + const DEFAULT_DEVICE_OTC_EXPIRY = 1000 * 60 * 60 * 24 // 24 hours /* @@ -442,14 +448,12 @@ module.exports = { }, refreshToken: async function (app, refreshToken) { - const existingToken = await app.db.models.AccessToken.byRefreshToken(refreshToken) - if (!existingToken) { - return null - } const [prefix] = refreshToken.split('_') + const existingToken = await app.db.models.AccessToken.byRefreshToken(refreshToken) - // Editor sessions have no refresh lifetime: rotate the refresh token each use. - if (!existingToken.refreshTokenExpiresAt) { + // Tokens without their own refresh lifetime (e.g. editor sessions) rotate + // the refresh token on each use and have no replay handling. + if (existingToken && !existingToken.refreshTokenExpiresAt) { const tokenUpdates = { token: generateToken(32, prefix), refreshToken: generateToken(32, prefix), @@ -459,29 +463,73 @@ module.exports = { return tokenUpdates } - // Past its lifetime the refresh token is dead: remove the row. - if (existingToken.refreshTokenExpiresAt.getTime() < Date.now()) { - await existingToken.destroy() - return null - } - - // Stable refresh token: concurrent refreshes reuse the cached access token - // instead of each minting one and overwriting the row. const cache = app.caches?.getCache?.(MCP_ACCESS_TOKEN_CACHE, { ttl: DEFAULT_TOKEN_SESSION_EXPIRY, max: 10000 }) const cacheKey = sha256(refreshToken) - const cached = await cache?.get(cacheKey) - if (cached && cached.expiresAt - Date.now() > MCP_ACCESS_TOKEN_REMAINING_LIMIT) { - return { token: cached.token, expiresAt: cached.expiresAt, refreshToken } + + // The presented token is the current one: rotate it. + if (existingToken) { + // Once the refresh lifetime has passed the grant is over: remove it + // rather than issuing a new access token. + if (existingToken.refreshTokenExpiresAt.getTime() < Date.now()) { + await existingToken.destroy() + return null + } + // A concurrent refresh may already have rotated this token; reuse its + // result so both callers receive the same new refresh token. + const cached = await cache?.get(cacheKey) + if (cached && cached.expiresAt - Date.now() > MCP_ACCESS_TOKEN_REMAINING_LIMIT) { + return { token: cached.token, expiresAt: cached.expiresAt, refreshToken: cached.refreshToken } + } + const token = generateToken(32, prefix) + const newRefreshToken = generateToken(32, prefix) + const expiresAt = Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY + // Compare-and-swap on the current refresh token: the update only matches + // while this token is still current, so of two simultaneous refreshes + // exactly one rotates and the other sees zero rows affected. + const [rotatedCount] = await app.db.models.AccessToken.update( + { + token, + expiresAt, + refreshToken: newRefreshToken, + refreshTokenExpiresAt: Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY, + previousRefreshToken: existingToken.refreshToken, + previousRefreshTokenRotatedAt: new Date() + }, + { where: { refreshToken: existingToken.refreshToken } } + ) + if (rotatedCount === 0) { + // A concurrent refresh won the swap; return its result rather than a + // token that never made it onto the row. + const winner = await cache?.get(cacheKey) + if (winner) { + return { token: winner.token, expiresAt: winner.expiresAt, refreshToken: winner.refreshToken } + } + return null + } + await cache?.set(cacheKey, { token, expiresAt, refreshToken: newRefreshToken }) + return { token, expiresAt, refreshToken: newRefreshToken } } - const token = generateToken(32, prefix) - const expiresAt = Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY - await app.db.models.AccessToken.update( - { token, expiresAt, refreshTokenExpiresAt: Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY }, - { where: { refreshToken: existingToken.refreshToken } } - ) - await cache?.set(cacheKey, { token, expiresAt }) - return { token, expiresAt, refreshToken } + // Not the current token: it may be one just rotated out. byRefreshToken + // stores the sha256, so match the presented token's hash here too. + const rotated = await app.db.models.AccessToken.findOne({ where: { previousRefreshToken: cacheKey } }) + if (!rotated) { + return null + } + const rotatedAt = rotated.previousRefreshTokenRotatedAt ? rotated.previousRefreshTokenRotatedAt.getTime() : 0 + if (Date.now() - rotatedAt <= MCP_REFRESH_TOKEN_GRACE) { + // Within the grace window this is a retry or a lagging concurrent + // refresh: hand back the tokens the rotation produced. + const cached = await cache?.get(cacheKey) + if (cached) { + return { token: cached.token, expiresAt: cached.expiresAt, refreshToken: cached.refreshToken } + } + return null + } + // After the grace window the same token is a replay: revoke the grant so + // its access tokens stop working and the client must re-authorize. + await rotated.destroy() + return null }, /** diff --git a/forge/db/migrations/20260825-03-add-refresh-token-rotation-tracking.js b/forge/db/migrations/20260825-03-add-refresh-token-rotation-tracking.js new file mode 100644 index 0000000000..35d2e8443e --- /dev/null +++ b/forge/db/migrations/20260825-03-add-refresh-token-rotation-tracking.js @@ -0,0 +1,36 @@ +/** + * Track the previously rotated-out refresh token so rotation can tell a + * legitimate concurrent or retried refresh from a replay. + * + * On each MCP refresh the refresh token is rotated: a new one is issued and the + * presented one is recorded here. Presenting the recorded token again within a + * short grace window is treated as a retry and returns the current tokens; + * presenting it after the window is a replay and revokes the grant. + * + * previousRefreshToken - sha256 of the last rotated-out refresh token. + * previousRefreshTokenRotatedAt - when that rotation happened, used to bound + * the grace window. + * + * Both are null for tokens that do not rotate (for example editor sessions). + */ + +const { DataTypes } = require('sequelize') + +module.exports = { + up: async (context) => { + await context.addColumn('AccessTokens', 'previousRefreshToken', { + type: DataTypes.STRING, + allowNull: true, + defaultValue: null + }) + await context.addColumn('AccessTokens', 'previousRefreshTokenRotatedAt', { + type: DataTypes.DATE, + allowNull: true, + defaultValue: null + }) + }, + down: async (context) => { + await context.removeColumn('AccessTokens', 'previousRefreshToken') + await context.removeColumn('AccessTokens', 'previousRefreshTokenRotatedAt') + } +} diff --git a/forge/db/models/AccessToken.js b/forge/db/models/AccessToken.js index f1a6c0e254..837956e48f 100644 --- a/forge/db/models/AccessToken.js +++ b/forge/db/models/AccessToken.js @@ -46,6 +46,10 @@ module.exports = { } }, refreshTokenExpiresAt: { type: DataTypes.DATE }, + // Holds the sha256 of the last rotated-out refresh token (set directly, + // already hashed) so rotation can distinguish a retry from a replay. + previousRefreshToken: { type: DataTypes.STRING }, + previousRefreshTokenRotatedAt: { type: DataTypes.DATE }, name: { type: DataTypes.STRING }, readOnly: { type: DataTypes.BOOLEAN, defaultValue: false, allowNull: false }, adminOptIn: { type: DataTypes.BOOLEAN, defaultValue: false, allowNull: false } diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index 8519c35e58..9050374df8 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -598,13 +598,8 @@ module.exports = async function (app) { reply.send(response) } } else if (grant_type === 'refresh_token') { - const existingToken = await app.db.models.AccessToken.byRefreshToken(refresh_token) - if (!existingToken) { - badRequest(reply, 'invalid_request', 'Invalid refresh_token') - return - } - // Only project/device clients re-check resource ownership on refresh; - // ff-plugin and MCP clients are user-scoped. + // ff-plugin and MCP clients are user-scoped; only project/device + // clients need their resource ownership re-checked on refresh. let refreshAuthClient = null if (client_id !== 'ff-plugin') { refreshAuthClient = await app.db.controllers.AuthClient.getAuthClient(client_id, client_secret) @@ -612,6 +607,16 @@ module.exports = async function (app) { return badRequest(reply, 'invalid_request', 'Invalid client_id') } } + const isMcpClient = refreshAuthClient?.ownerType === 'mcp' + const existingToken = await app.db.models.AccessToken.byRefreshToken(refresh_token) + // A rotated-out MCP refresh token is no longer the row's current token, so + // byRefreshToken cannot find it. refreshToken() resolves the current-or-previous + // token, including the grace window and replay detection, so defer to it for + // MCP clients rather than rejecting an already-rotated token here. + if (!existingToken && !isMcpClient) { + badRequest(reply, 'invalid_request', 'Invalid refresh_token') + return + } if (refreshAuthClient && refreshAuthClient.ownerType !== 'mcp') { // Check the owner of the existing session still has access to the project // this client is owned by diff --git a/test/unit/forge/db/controllers/AccessToken_spec.js b/test/unit/forge/db/controllers/AccessToken_spec.js index 8424a09a1a..22c83a17c3 100644 --- a/test/unit/forge/db/controllers/AccessToken_spec.js +++ b/test/unit/forge/db/controllers/AccessToken_spec.js @@ -342,7 +342,7 @@ describe('AccessToken controller', function () { // Move the row's access and/or refresh token expiry, so the refresh // behaviour can be exercised without waiting for real time to pass. - async function setRowExpiry (refreshToken, { accessMs, refreshMs } = {}) { + async function setRowExpiry (refreshToken, { accessMs, refreshMs, rotatedMs } = {}) { const row = await app.db.models.AccessToken.byRefreshToken(refreshToken) const updates = {} if (accessMs !== undefined) { @@ -351,6 +351,9 @@ describe('AccessToken controller', function () { if (refreshMs !== undefined) { updates.refreshTokenExpiresAt = new Date(Date.now() + refreshMs) } + if (rotatedMs !== undefined) { + updates.previousRefreshTokenRotatedAt = new Date(Date.now() + rotatedMs) + } await app.db.models.AccessToken.update(updates, { where: { id: row.id } }) } @@ -388,7 +391,7 @@ describe('AccessToken controller', function () { ;(await app.db.models.AccessToken.count()).should.equal(0) }) - it('keeps the refresh token stable and slides its expiry on refresh', async function () { + it('rotates the refresh token and slides its expiry on refresh', async function () { const original = await createToken() // Bring the refresh expiry close so the slide back out to the full // lifetime is observable rather than a same-millisecond tie. @@ -397,20 +400,23 @@ describe('AccessToken controller', function () { const refreshed = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) refreshed.token.should.not.equal(original.token) - // The refresh token is not rotated. - refreshed.refreshToken.should.equal(original.refreshToken) + // The refresh token is rotated, and the new access token authenticates. + refreshed.refreshToken.should.be.a.String().and.not.equal(original.refreshToken) + should.exist(await app.db.controllers.AccessToken.getOrExpire(refreshed.token)) - const after = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) + const after = await app.db.models.AccessToken.byRefreshToken(refreshed.refreshToken) after.refreshTokenExpiresAt.getTime().should.be.greaterThan(before.refreshTokenExpiresAt.getTime()) }) - it('reuses the same access token for a repeat refresh within its lifetime', async function () { + it('returns the same tokens for a retried refresh within the grace window', async function () { const original = await createToken() await setRowExpiry(original.refreshToken, { accessMs: -5000 }) const first = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) - const second = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) - second.token.should.equal(first.token) + // The rotated-out token is presented again (a retry or lagging concurrent refresh). + const retry = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + retry.token.should.equal(first.token) + retry.refreshToken.should.equal(first.refreshToken) should.exist(await app.db.controllers.AccessToken.getOrExpire(first.token)) }) @@ -421,6 +427,44 @@ describe('AccessToken controller', function () { should.not.exist(await app.db.controllers.AccessToken.refreshToken(original.refreshToken)) ;(await app.db.models.AccessToken.count()).should.equal(0) }) + + it('revokes the grant when a rotated-out token is replayed after the grace window', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + const first = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + + // Age the rotation beyond the grace window, then present the old token again. + await setRowExpiry(first.refreshToken, { rotatedMs: -120000 }) + should.not.exist(await app.db.controllers.AccessToken.refreshToken(original.refreshToken)) + // The whole grant is revoked, so the current access token stops working too. + ;(await app.db.models.AccessToken.count()).should.equal(0) + should.not.exist(await app.db.controllers.AccessToken.getOrExpire(first.token)) + }) + + it('returns null for an unknown refresh token', async function () { + should.not.exist(await app.db.controllers.AccessToken.refreshToken('ffpat_unknown')) + }) + + it('lets only one of two simultaneous refreshes rotate the token', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + + const [a, b] = await Promise.all([ + app.db.controllers.AccessToken.refreshToken(original.refreshToken), + app.db.controllers.AccessToken.refreshToken(original.refreshToken) + ]) + + // The grant is neither duplicated nor revoked by the race. + ;(await app.db.models.AccessToken.count()).should.equal(1) + const results = [a, b].filter(Boolean) + results.length.should.be.aboveOrEqual(1) + // Every token handed back is the single winner's: it authenticates, and + // the refresh tokens returned never diverge (no caller gets a dead token). + for (const result of results) { + should.exist(await app.db.controllers.AccessToken.getOrExpire(result.token)) + } + new Set(results.map(result => result.refreshToken)).size.should.equal(1) + }) }) describe('getOrExpire', function () { diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index 52eff92c3e..806107e39f 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -463,6 +463,57 @@ describe('OAuth', async function () { refreshed.should.have.property('refresh_token') }) + it('rotates the refresh token, honours the grace window, and revokes on replay', async function () { + const clientID = (await register()).json().client_id + const { verifier, challenge } = pkce() + const authResponse = await mcpApp.inject({ method: 'GET', url: authorizeURL(clientID, redirectURI, challenge), cookies: { sid } }) + const requestId = /\/account\/request\/([^/]+)\/mcp$/.exec(authResponse.headers.location)[1] + await mcpApp.inject({ method: 'PUT', url: `/account/authorize/${requestId}/consent`, payload: { readOnly: false, teamIds: [] }, cookies: { sid } }) + const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) + const authCode = new URL(completeResponse.headers.location).searchParams.get('code') + const first = (await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'authorization_code', code: authCode, redirect_uri: redirectURI, client_id: clientID, code_verifier: verifier } + })).json() + + // refresh rotates: a new refresh token is issued in place of the presented one + const rotateResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'refresh_token', client_id: clientID, refresh_token: first.refresh_token } + }) + rotateResponse.should.have.property('statusCode', 200) + const rotated = rotateResponse.json() + rotated.access_token.should.be.a.String().and.startWith('ffpat') + rotated.refresh_token.should.be.a.String().and.not.equal(first.refresh_token) + + // presenting the rotated-out token again within the grace window returns the + // current tokens rather than an error, so a retried or racing refresh still succeeds + const graceResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'refresh_token', client_id: clientID, refresh_token: first.refresh_token } + }) + graceResponse.should.have.property('statusCode', 200) + graceResponse.json().refresh_token.should.equal(rotated.refresh_token) + + // push the rotation past the grace window so the rotated-out token reads as a replay + const row = await mcpApp.db.models.AccessToken.byRefreshToken(rotated.refresh_token) + await row.update({ previousRefreshTokenRotatedAt: new Date(Date.now() - 1000 * 60 * 60) }) + + const replayResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'refresh_token', client_id: clientID, refresh_token: first.refresh_token } + }) + replayResponse.should.have.property('statusCode', 400) + + // the grant is revoked, so the current refresh token no longer resolves a row + const revoked = await mcpApp.db.models.AccessToken.byRefreshToken(rotated.refresh_token) + should.not.exist(revoked) + }) + it('rejects an authorize redirect_uri that was not registered', async function () { const reg = (await register(['http://localhost:9876/oauth/callback'])).json() const { challenge } = pkce()