diff --git a/web/service/base.signin-redirect.spec.ts b/web/service/base.signin-redirect.spec.ts new file mode 100644 index 0000000000..6ec5f379fb --- /dev/null +++ b/web/service/base.signin-redirect.spec.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from 'vitest' +import { buildSigninUrlWithRedirect } from './base' + +describe('buildSigninUrlWithRedirect', () => { + describe('OAuth callback preservation', () => { + it('should include the full current OAuth authorize URL in oauth_redirect_url', () => { + // Arrange + const currentLocation = { + origin: 'https://skills.bash-is-all-you-need.dify.dev', + pathname: '/account/oauth/authorize', + search: '?client_id=dcfcd6a4-5799-405a-a6d7-04261b24dd02&redirect_uri=https%3A%2F%2Fcreators.dify.dev%2Fapi%2Fv1%2Foauth%2Fcallback%2Fdify&response_type=code', + } as const + + // Act + const signinUrl = buildSigninUrlWithRedirect(currentLocation, '') + const parsedSigninUrl = new URL(signinUrl) + + // Assert + expect(parsedSigninUrl.pathname).toBe('/signin') + const encodedRedirect = parsedSigninUrl.searchParams.get('oauth_redirect_url') + expect(encodedRedirect).toBeTruthy() + expect(decodeURIComponent(encodedRedirect!)).toBe(`${currentLocation.origin}${currentLocation.pathname}${currentLocation.search}`) + }) + }) + + describe('Signin self-redirect guard', () => { + it('should return plain signin URL when current path is already signin', () => { + // Arrange + const currentLocation = { + origin: 'https://skills.bash-is-all-you-need.dify.dev', + pathname: '/signin', + search: '?oauth_redirect_url=https%3A%2F%2Fskills.bash-is-all-you-need.dify.dev%2Faccount%2Foauth%2Fauthorize', + } as const + + // Act + const signinUrl = buildSigninUrlWithRedirect(currentLocation, '') + + // Assert + expect(signinUrl).toBe('https://skills.bash-is-all-you-need.dify.dev/signin') + }) + }) + + describe('basePath support', () => { + it('should respect basePath when building signin URL', () => { + // Arrange + const currentLocation = { + origin: 'https://example.com', + pathname: '/console/apps', + search: '?tab=all', + } as const + + // Act + const signinUrl = buildSigninUrlWithRedirect(currentLocation, '/console') + + // Assert + expect(signinUrl.startsWith('https://example.com/console/signin?')).toBe(true) + const encodedRedirect = new URL(signinUrl).searchParams.get('oauth_redirect_url') + expect(decodeURIComponent(encodedRedirect || '')).toBe('https://example.com/console/apps?tab=all') + }) + }) +}) diff --git a/web/service/base.ts b/web/service/base.ts index 8265376825..f01940942e 100644 --- a/web/service/base.ts +++ b/web/service/base.ts @@ -37,6 +37,7 @@ import { refreshAccessTokenOrReLogin } from './refresh-token' import { getWebAppPassport } from './webapp-auth' const TIME_OUT = 100000 +const SIGNIN_REDIRECT_URL_KEY = 'oauth_redirect_url' export type IconObject = { background: string @@ -156,6 +157,20 @@ function jumpTo(url: string) { globalThis.location.href = url } +export function buildSigninUrlWithRedirect(currentLocation: Pick, currentBasePath: string) { + const signinPath = `${currentBasePath}/signin` + const signinUrl = `${currentLocation.origin}${signinPath}` + + // Keep signin as-is to avoid redirect loops. + if (currentLocation.pathname === signinPath) + return signinUrl + + const currentUrl = `${currentLocation.origin}${currentLocation.pathname}${currentLocation.search}` + const params = new URLSearchParams() + params.set(SIGNIN_REDIRECT_URL_KEY, encodeURIComponent(currentUrl)) + return `${signinUrl}?${params.toString()}` +} + function unicodeToChar(text: string) { if (!text) return '' @@ -781,7 +796,7 @@ export const request = async(url: string, options = {}, otherOptions?: IOther const errResp: Response = err as any if (errResp.status === 401) { const [parseErr, errRespData] = await asyncRunSafe(errResp.json()) - const loginUrl = `${globalThis.location.origin}${basePath}/signin` + const loginUrl = buildSigninUrlWithRedirect(globalThis.location, basePath) if (parseErr) { globalThis.location.href = loginUrl return Promise.reject(err) diff --git a/web/service/refresh-token.spec.ts b/web/service/refresh-token.spec.ts new file mode 100644 index 0000000000..4c423f8d3b --- /dev/null +++ b/web/service/refresh-token.spec.ts @@ -0,0 +1,86 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { refreshAccessTokenOrReLogin } from './refresh-token' + +const mockFetchWithRetry = vi.fn() + +vi.mock('@/utils', () => ({ + fetchWithRetry: (...args: unknown[]) => mockFetchWithRetry(...args), +})) + +function createDeferred() { + let resolve!: (value: T) => void + let reject!: (reason?: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + + return { promise, resolve, reject } +} + +describe('refreshAccessTokenOrReLogin', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.useRealTimers() + localStorage.clear() + globalThis.fetch = vi.fn() + }) + + describe('stale cross-tab lock handling', () => { + it('should clean stale lock and execute refresh request', async () => { + // Arrange + localStorage.setItem('is_other_tab_refreshing', '1') + localStorage.setItem('last_refresh_time', `${Date.now() - 30_000}`) + mockFetchWithRetry.mockResolvedValue([null, new Response(null, { status: 200 })]) + + // Act + await refreshAccessTokenOrReLogin(5_000) + + // Assert + expect(mockFetchWithRetry).toHaveBeenCalledTimes(1) + expect(localStorage.getItem('is_other_tab_refreshing')).toBeNull() + expect(localStorage.getItem('last_refresh_time')).toBeNull() + }) + }) + + describe('concurrent refresh requests', () => { + it('should avoid duplicate refresh calls when a refresh is already in progress', async () => { + // Arrange + const deferredRefresh = createDeferred<[null, Response]>() + mockFetchWithRetry.mockImplementation(() => deferredRefresh.promise) + + // Act + const firstRefresh = refreshAccessTokenOrReLogin(5_000) + const secondRefresh = refreshAccessTokenOrReLogin(5_000) + deferredRefresh.resolve([null, new Response(null, { status: 200 })]) + + // Assert + await expect(firstRefresh).resolves.toBeUndefined() + await expect(secondRefresh).resolves.toBeUndefined() + expect(mockFetchWithRetry).toHaveBeenCalledTimes(1) + expect(localStorage.getItem('is_other_tab_refreshing')).toBeNull() + expect(localStorage.getItem('last_refresh_time')).toBeNull() + }) + }) + + describe('waiter behavior', () => { + it('should wait for another tab lock to release without issuing duplicate refresh', async () => { + // Arrange + vi.useFakeTimers() + localStorage.setItem('is_other_tab_refreshing', 'other-tab-token') + localStorage.setItem('last_refresh_time', `${Date.now()}`) + + // Act + const waitingRefresh = refreshAccessTokenOrReLogin(5_000) + setTimeout(() => { + localStorage.removeItem('is_other_tab_refreshing') + localStorage.removeItem('last_refresh_time') + }, 300) + await vi.advanceTimersByTimeAsync(1_000) + + // Assert + await expect(waitingRefresh).resolves.toBeUndefined() + expect(mockFetchWithRetry).not.toHaveBeenCalled() + }) + }) +}) diff --git a/web/service/refresh-token.ts b/web/service/refresh-token.ts index b00a46eb6e..21259900c7 100644 --- a/web/service/refresh-token.ts +++ b/web/service/refresh-token.ts @@ -2,16 +2,55 @@ import { API_PREFIX } from '@/config' import { fetchWithRetry } from '@/utils' const LOCAL_STORAGE_KEY = 'is_other_tab_refreshing' +const LAST_REFRESH_TIME_KEY = 'last_refresh_time' +const REFRESH_LOCK_MAX_AGE_MS = 10 * 1000 +const REFRESH_LOCK_POLL_INTERVAL_MS = 200 let isRefreshing = false -function waitUntilTokenRefreshed() { +let refreshLockToken: string | null = null + +const getCurrentTime = () => Date.now() + +function getRefreshLockAge() { + const lastTime = globalThis.localStorage.getItem(LAST_REFRESH_TIME_KEY) || '0' + const parsedLastTime = Number.parseInt(lastTime, 10) + if (Number.isNaN(parsedLastTime) || parsedLastTime <= 0) + return Number.POSITIVE_INFINITY + + return getCurrentTime() - parsedLastTime +} + +function hasValidRefreshLock() { + const refreshLock = globalThis.localStorage.getItem(LOCAL_STORAGE_KEY) + if (!refreshLock) + return false + + if (getRefreshLockAge() <= REFRESH_LOCK_MAX_AGE_MS) + return true + + // stale lock from another tab/session: clean it up to avoid deadlock + globalThis.localStorage.removeItem(LOCAL_STORAGE_KEY) + globalThis.localStorage.removeItem(LAST_REFRESH_TIME_KEY) + return false +} + +function waitUntilTokenRefreshed(maxWaitMs: number) { + const startedAt = getCurrentTime() return new Promise((resolve) => { function _check() { - const isRefreshingSign = globalThis.localStorage.getItem(LOCAL_STORAGE_KEY) - if ((isRefreshingSign && isRefreshingSign === '1') || isRefreshing) { + if (getCurrentTime() - startedAt >= maxWaitMs) { + if (!isRefreshing) { + globalThis.localStorage.removeItem(LOCAL_STORAGE_KEY) + globalThis.localStorage.removeItem(LAST_REFRESH_TIME_KEY) + } + resolve() + return + } + + if (hasValidRefreshLock() || isRefreshing) { setTimeout(() => { _check() - }, 1000) + }, REFRESH_LOCK_POLL_INTERVAL_MS) } else { resolve() @@ -21,45 +60,46 @@ function waitUntilTokenRefreshed() { }) } -const isRefreshingSignAvailable = function (delta: number) { - const nowTime = new Date().getTime() - const lastTime = globalThis.localStorage.getItem('last_refresh_time') || '0' - return nowTime - Number.parseInt(lastTime) <= delta +function acquireRefreshLock() { + refreshLockToken = `${getCurrentTime()}-${Math.random().toString(36).slice(2)}` + globalThis.localStorage.setItem(LOCAL_STORAGE_KEY, refreshLockToken) + globalThis.localStorage.setItem(LAST_REFRESH_TIME_KEY, getCurrentTime().toString()) } // only one request can send async function getNewAccessToken(timeout: number): Promise { + let lockAcquired = false + try { - const isRefreshingSign = globalThis.localStorage.getItem(LOCAL_STORAGE_KEY) - if ((isRefreshingSign && isRefreshingSign === '1' && isRefreshingSignAvailable(timeout)) || isRefreshing) { - await waitUntilTokenRefreshed() + if (hasValidRefreshLock() || isRefreshing) { + await waitUntilTokenRefreshed(Math.min(timeout, REFRESH_LOCK_MAX_AGE_MS)) + return + } + + isRefreshing = true + acquireRefreshLock() + lockAcquired = true + globalThis.addEventListener('beforeunload', releaseRefreshLock) + + // Do not use baseFetch to refresh tokens. + // If a 401 response occurs and baseFetch itself attempts to refresh the token, + // it can lead to an infinite loop if the refresh attempt also returns 401. + // To avoid this, handle token refresh separately in a dedicated function + // that does not call baseFetch and uses a single retry mechanism. + const [error, ret] = await fetchWithRetry(globalThis.fetch(`${API_PREFIX}/refresh-token`, { + method: 'POST', + credentials: 'include', // Important: include cookies in the request + headers: { + 'Content-Type': 'application/json;utf-8', + }, + // No body needed - refresh token is in cookie + })) + if (error) { + return Promise.reject(error) } else { - isRefreshing = true - globalThis.localStorage.setItem(LOCAL_STORAGE_KEY, '1') - globalThis.localStorage.setItem('last_refresh_time', new Date().getTime().toString()) - globalThis.addEventListener('beforeunload', releaseRefreshLock) - - // Do not use baseFetch to refresh tokens. - // If a 401 response occurs and baseFetch itself attempts to refresh the token, - // it can lead to an infinite loop if the refresh attempt also returns 401. - // To avoid this, handle token refresh separately in a dedicated function - // that does not call baseFetch and uses a single retry mechanism. - const [error, ret] = await fetchWithRetry(globalThis.fetch(`${API_PREFIX}/refresh-token`, { - method: 'POST', - credentials: 'include', // Important: include cookies in the request - headers: { - 'Content-Type': 'application/json;utf-8', - }, - // No body needed - refresh token is in cookie - })) - if (error) { - return Promise.reject(error) - } - else { - if (ret.status === 401) - return Promise.reject(ret) - } + if (ret.status === 401) + return Promise.reject(ret) } } catch (error) { @@ -67,16 +107,21 @@ async function getNewAccessToken(timeout: number): Promise { return Promise.reject(error) } finally { - releaseRefreshLock() + if (lockAcquired) + releaseRefreshLock() } } function releaseRefreshLock() { - // Always clear the refresh lock to avoid cross-tab deadlocks. - // This is safe to call multiple times and from tabs that were only waiting. + const currentLockToken = globalThis.localStorage.getItem(LOCAL_STORAGE_KEY) + isRefreshing = false - globalThis.localStorage.removeItem(LOCAL_STORAGE_KEY) - globalThis.localStorage.removeItem('last_refresh_time') + if (refreshLockToken && currentLockToken === refreshLockToken) { + globalThis.localStorage.removeItem(LOCAL_STORAGE_KEY) + globalThis.localStorage.removeItem(LAST_REFRESH_TIME_KEY) + } + + refreshLockToken = null globalThis.removeEventListener('beforeunload', releaseRefreshLock) }