-
Notifications
You must be signed in to change notification settings - Fork 108
fix(frontend): centralize cookie credentials in fetchWrapper and route callers through it #322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { getAuthConfig, DEFAULT_AUTH_CONFIG, listUserPasskeys } from './authInfo' | ||
|
|
||
| describe('getAuthConfig', () => { | ||
| const fetchMock = vi.fn() | ||
|
|
||
| beforeEach(() => { | ||
| fetchMock.mockReset() | ||
| vi.stubGlobal('fetch', fetchMock) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| }) | ||
|
|
||
| it('returns the parsed config on success', async () => { | ||
| const config = { | ||
| enabledProviders: ['credentials', 'github'], | ||
| registrationEnabled: true, | ||
| isFirstUser: false, | ||
| adminConfigured: true, | ||
| } | ||
| fetchMock.mockResolvedValue( | ||
| new Response(JSON.stringify(config), { status: 200 }), | ||
| ) | ||
|
|
||
| const result = await getAuthConfig() | ||
|
|
||
| expect(result).toEqual(config) | ||
|
|
||
| const callOptions = fetchMock.mock.calls[0][1] | ||
| expect(callOptions.credentials).toBe('include') | ||
| }) | ||
|
|
||
| it('falls back to DEFAULT_AUTH_CONFIG on a non-OK response', async () => { | ||
| fetchMock.mockResolvedValue( | ||
| new Response(JSON.stringify({ error: 'server error' }), { status: 500 }), | ||
| ) | ||
|
|
||
| const result = await getAuthConfig() | ||
|
|
||
| expect(result).toEqual(DEFAULT_AUTH_CONFIG) | ||
| }) | ||
|
|
||
| it('falls back to DEFAULT_AUTH_CONFIG when the network rejects', async () => { | ||
| fetchMock.mockRejectedValue(new Error('offline')) | ||
|
|
||
| const result = await getAuthConfig() | ||
|
|
||
| expect(result).toEqual(DEFAULT_AUTH_CONFIG) | ||
| }) | ||
|
|
||
| it('defaults isFirstUser to false so the setup flow is not offered when config is unknown', () => { | ||
| expect(DEFAULT_AUTH_CONFIG.isFirstUser).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('listUserPasskeys', () => { | ||
| const fetchMock = vi.fn() | ||
|
|
||
| beforeEach(() => { | ||
| fetchMock.mockReset() | ||
| vi.stubGlobal('fetch', fetchMock) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| }) | ||
|
|
||
| it('returns the passkey list and sends credentials', async () => { | ||
| fetchMock.mockResolvedValue( | ||
| new Response(JSON.stringify([{ id: 'p1', name: 'laptop' }]), { status: 200 }), | ||
| ) | ||
|
|
||
| const result = await listUserPasskeys() | ||
|
|
||
| expect(result).toEqual([{ id: 'p1', name: 'laptop' }]) | ||
|
|
||
| const callOptions = fetchMock.mock.calls[0][1] | ||
| expect(callOptions.credentials).toBe('include') | ||
| }) | ||
|
|
||
| it('returns an empty array when the request fails', async () => { | ||
| fetchMock.mockResolvedValue( | ||
| new Response(JSON.stringify({ error: 'unauthorized' }), { status: 401 }), | ||
| ) | ||
|
|
||
| const result = await listUserPasskeys() | ||
|
|
||
| expect(result).toEqual([]) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { API_BASE_URL } from '@/config' | ||
| import { fetchWrapper } from './fetchWrapper' | ||
|
|
||
| export interface AuthConfig { | ||
| enabledProviders: string[] | ||
| registrationEnabled: boolean | ||
| isFirstUser: boolean | ||
| adminConfigured: boolean | ||
| } | ||
|
|
||
| export const DEFAULT_AUTH_CONFIG: AuthConfig = { | ||
| enabledProviders: ['credentials'], | ||
| registrationEnabled: true, | ||
| isFirstUser: false, | ||
| adminConfigured: false, | ||
| } | ||
|
|
||
| export async function getAuthConfig(): Promise<AuthConfig> { | ||
| try { | ||
| return await fetchWrapper<AuthConfig>(`${API_BASE_URL}/api/auth-info/config`) | ||
| } catch { | ||
| return DEFAULT_AUTH_CONFIG | ||
| } | ||
| } | ||
|
|
||
| export interface Passkey { | ||
| id: string | ||
| name?: string | ||
| credentialID: string | ||
| createdAt: string | ||
| deviceType: string | ||
| } | ||
|
|
||
| export async function listUserPasskeys(): Promise<Passkey[]> { | ||
| try { | ||
| return await fetchWrapper<Passkey[]>(`${API_BASE_URL}/api/auth/passkey/list-user-passkeys`) | ||
| } catch { | ||
| return [] | ||
| } | ||
|
Comment on lines
+34
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Do not present passkey-load failures as an empty passkey list. Catching
📍 Affects 3 files
🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { FetchError } from '@opencode-manager/shared' | ||
| import { fetchWrapper } from './fetchWrapper' | ||
|
|
||
| describe('fetchWrapper', () => { | ||
| const fetchMock = vi.fn() | ||
|
|
||
| beforeEach(() => { | ||
| fetchMock.mockReset() | ||
| vi.stubGlobal('fetch', fetchMock) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| }) | ||
|
|
||
| it('sends credentials: include on every request', async () => { | ||
| fetchMock.mockResolvedValue(new Response(JSON.stringify({ ok: true }), { status: 200 })) | ||
|
|
||
| await fetchWrapper('/api/x') | ||
|
|
||
| expect(fetchMock.mock.calls[0][1].credentials).toBe('include') | ||
| }) | ||
|
|
||
| it('keeps credentials: include when the caller passes its own options', async () => { | ||
| fetchMock.mockResolvedValue(new Response(JSON.stringify({ ok: true }), { status: 200 })) | ||
|
|
||
| await fetchWrapper('/api/x', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: '{}', | ||
| }) | ||
|
|
||
| const init = fetchMock.mock.calls[0][1] | ||
| expect(init.credentials).toBe('include') | ||
| expect(init.method).toBe('POST') | ||
| }) | ||
|
|
||
| it('aborts the request when the caller signal aborts', async () => { | ||
| fetchMock.mockImplementation((_url, init) => | ||
| new Promise((_resolve, reject) => { | ||
| init.signal.addEventListener('abort', () => | ||
| reject(new DOMException('Aborted', 'AbortError')), | ||
| ) | ||
| }), | ||
| ) | ||
|
|
||
| const controller = new AbortController() | ||
| const promise = fetchWrapper('/api/x', { signal: controller.signal }) | ||
| controller.abort() | ||
| await expect(promise).rejects.toThrow() | ||
| }) | ||
|
|
||
| it('rejects with a 408 TIMEOUT FetchError when the timeout elapses', async () => { | ||
| vi.useFakeTimers() | ||
| fetchMock.mockImplementation((_url, init) => { | ||
| const signal = init.signal as AbortSignal | ||
| return new Promise((_resolve, reject) => { | ||
| if (signal.aborted) { | ||
| const err = new Error('The operation was aborted') | ||
| err.name = 'AbortError' | ||
| reject(err) | ||
| return | ||
| } | ||
| signal.addEventListener('abort', () => { | ||
| const err = new Error('The operation was aborted') | ||
| err.name = 'AbortError' | ||
| reject(err) | ||
| }, { once: true }) | ||
| }) | ||
| }) | ||
|
|
||
| const promise = fetchWrapper('/api/x', { timeout: 100 }) | ||
| vi.advanceTimersByTime(101) | ||
|
|
||
| await expect(promise).rejects.toBeInstanceOf(FetchError) | ||
| await expect(promise).rejects.toMatchObject({ | ||
| statusCode: 408, | ||
| code: 'TIMEOUT', | ||
| }) | ||
|
|
||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('rejects with 499 CANCELED when the caller aborts, not 408 TIMEOUT', async () => { | ||
| fetchMock.mockImplementation((_url, init) => { | ||
| const signal = init.signal as AbortSignal | ||
| return new Promise((_resolve, reject) => { | ||
| if (signal.aborted) { | ||
| const err = new Error('The operation was aborted') | ||
| err.name = 'AbortError' | ||
| reject(err) | ||
| return | ||
| } | ||
| signal.addEventListener('abort', () => { | ||
| const err = new Error('The operation was aborted') | ||
| err.name = 'AbortError' | ||
| reject(err) | ||
| }, { once: true }) | ||
| }) | ||
| }) | ||
|
|
||
| const controller = new AbortController() | ||
| const promise = fetchWrapper('/api/x', { signal: controller.signal }) | ||
| controller.abort() | ||
|
|
||
| await expect(promise).rejects.toBeInstanceOf(FetchError) | ||
| await expect(promise).rejects.toMatchObject({ | ||
| statusCode: 499, | ||
| code: 'CANCELED', | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: chriswritescode-dev/opencode-manager
Length of output: 174
🏁 Script executed:
Repository: chriswritescode-dev/opencode-manager
Length of output: 7645
Clarify the cross-site auth note:
backend/src/auth/index.tsonly setsadvanced.useSecureCookies; it does not overridesameSite, so the defaultSameSite=Laxbehavior still blocks cross-site session cookies. If cross-site access isn’t supported, say that explicitly; otherwise document the requiredsameSite: 'none'+secure: truesettings.🤖 Prompt for AI Agents