-
Notifications
You must be signed in to change notification settings - Fork 22
fix(web): send CSRF token on fetch-based webGUI requests #2039
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,83 @@ | ||
| /** | ||
| * ApiStatus Component Test Coverage | ||
| * | ||
| * Regression coverage for the "wrong csrf_token" bug: the on-mount status | ||
| * check must use the page-global csrf_token when the server store has not yet | ||
| * hydrated its own `csrf` value, otherwise it sends a blank token and both | ||
| * fails the status read and logs a red error in syslog. | ||
| */ | ||
|
|
||
| import { flushPromises, mount } from '@vue/test-utils'; | ||
|
|
||
| import { createTestingPinia } from '@pinia/testing'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import ApiStatus from '~/components/ApiStatus/ApiStatus.vue'; | ||
|
|
||
| const mockWebguiUnraidApiCommand = vi.fn(); | ||
|
|
||
| vi.mock('~/composables/services/webgui', () => ({ | ||
| WebguiUnraidApiCommand: (...args: unknown[]) => mockWebguiUnraidApiCommand(...args), | ||
| })); | ||
|
|
||
| const mockServerStore = { csrf: '' }; | ||
| vi.mock('~/store/server', () => ({ | ||
| useServerStore: () => mockServerStore, | ||
| })); | ||
|
|
||
| const mountComponent = () => | ||
| mount(ApiStatus, { | ||
| global: { | ||
| plugins: [createTestingPinia({ createSpy: vi.fn })], | ||
| }, | ||
| }); | ||
|
|
||
| describe('ApiStatus.vue', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockServerStore.csrf = ''; | ||
| mockWebguiUnraidApiCommand.mockResolvedValue({ result: 'API is running' }); | ||
| globalThis.csrf_token = 'global-token-123'; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| globalThis.csrf_token = ''; | ||
| }); | ||
|
|
||
| it('sends the page-global csrf_token on mount when the store has not hydrated', async () => { | ||
| mountComponent(); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockWebguiUnraidApiCommand).toHaveBeenCalledWith({ | ||
| csrf_token: 'global-token-123', | ||
| command: 'status', | ||
| }); | ||
| }); | ||
|
|
||
| it('never sends a blank csrf_token on mount', async () => { | ||
| mountComponent(); | ||
| await flushPromises(); | ||
|
|
||
| const payload = mockWebguiUnraidApiCommand.mock.calls[0]?.[0]; | ||
| expect(payload.csrf_token).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('prefers the store csrf value once it is populated', async () => { | ||
| mockServerStore.csrf = 'store-token-456'; | ||
| mountComponent(); | ||
| await flushPromises(); | ||
|
|
||
| expect(mockWebguiUnraidApiCommand).toHaveBeenCalledWith({ | ||
| csrf_token: 'store-token-456', | ||
| command: 'status', | ||
| }); | ||
| }); | ||
|
|
||
| it('renders Running when the status result reports the service is running', async () => { | ||
| const wrapper = mountComponent(); | ||
| await flushPromises(); | ||
|
|
||
| expect(wrapper.text()).toContain('Running'); | ||
| expect(wrapper.text()).not.toContain('Not Running'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /** | ||
| * request composable — global CSRF header attachment | ||
| * | ||
| * The webGUI CSRF gate rejects same-origin POSTs that lack a valid token. The | ||
| * shared wretch instance must attach the page-global csrf_token as an | ||
| * `x-csrf-token` header automatically (and only for same-origin requests). | ||
| */ | ||
|
|
||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { request } from '~/composables/services/request'; | ||
|
|
||
| const okResponse = () => | ||
| new Response('{}', { status: 200, headers: { 'Content-Type': 'application/json' } }); | ||
|
|
||
| describe('request csrf attachment', () => { | ||
| let fetchMock: ReturnType<typeof vi.fn>; | ||
|
|
||
| beforeEach(() => { | ||
| fetchMock = vi.fn().mockResolvedValue(okResponse()); | ||
| vi.stubGlobal('fetch', fetchMock); | ||
| globalThis.csrf_token = 'token-abc'; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| globalThis.csrf_token = ''; | ||
| }); | ||
|
|
||
| const headerFromCall = (index = 0) => { | ||
| const opts = fetchMock.mock.calls[index]?.[1] ?? {}; | ||
| return new Headers(opts.headers).get('x-csrf-token'); | ||
| }; | ||
|
|
||
| it('attaches the page-global token on same-origin requests', async () => { | ||
| await request.url('/plugins/dynamix.my.servers/include/unraid-api.php').post(); | ||
| expect(headerFromCall()).toBe('token-abc'); | ||
| }); | ||
|
|
||
| it('attaches the token on same-origin GET requests too', async () => { | ||
| await request.url('/plugins/dynamix.my.servers/data/server-state.php').get(); | ||
| expect(headerFromCall()).toBe('token-abc'); | ||
| }); | ||
|
|
||
| it('does not attach the token to cross-origin requests', async () => { | ||
| await request.url('https://wanip4.unraid.net/').get(); | ||
| expect(headerFromCall()).toBeNull(); | ||
| }); | ||
|
|
||
| it('attaches no header when the page global is unset', async () => { | ||
| globalThis.csrf_token = ''; | ||
| await request.url('/webGui/include/Notify.php').post(); | ||
| expect(headerFromCall()).toBeNull(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,32 @@ import queryString from 'wretch/addons/queryString'; | |
|
|
||
| import { useErrorsStore } from '~/store/errors'; | ||
|
|
||
| /** | ||
| * The webGUI CSRF gate (local_prepend.php) validates same-origin POSTs against | ||
| * `$var['csrf_token']`, accepting the token via an `x-csrf-token` header for | ||
| * XHR/fetch callers. jQuery callers get this for free via `$.ajaxPrefilter`, and | ||
| * the Apollo client already sets the header itself — attach it here so every | ||
| * fetch-based webGUI request is covered too, rather than each caller having to | ||
| * remember to pass the token. Restricted to same-origin requests so the token is | ||
| * never sent to external hosts. | ||
| */ | ||
| const isSameOriginRequest = (url: string): boolean => { | ||
| try { | ||
| return new URL(url, globalThis.location?.href).origin === globalThis.location?.origin; | ||
| } catch { | ||
| return true; | ||
| } | ||
| }; | ||
|
Comment on lines
+8
to
+23
Contributor
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. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the target file and inspect nearby context.
ast-grep outline web/src/composables/services/request.ts --view expanded || true
echo '--- FILE ---'
cat -n web/src/composables/services/request.ts | sed -n '1,220p'
echo '--- SEARCH CALLERS / URL SHAPES ---'
rg -n "isSameOriginRequest|csrf_token|x-csrf-token|new URL\(" web/src -SRepository: unraid/api Length of output: 7783 🏁 Script executed: #!/bin/bash
set -euo pipefail
git ls-files 'web/src/composables/services/request.ts' 'web/src/**' | sed -n '1,20p'
echo '---'
wc -l web/src/composables/services/request.ts
echo '---'
cat -n web/src/composables/services/request.ts | sed -n '1,220p'Repository: unraid/api Length of output: 3333 🏁 Script executed: node - <<'JS'
const cases = [
['relative with base href', ['/foo', 'https://example.com/app/']],
['absolute', ['https://api.example.com/x', 'https://example.com/app/']],
['relative no base', ['/foo', undefined]],
['bad url', ['http://[bad', 'https://example.com/app/']],
['empty string', ['', 'https://example.com/app/']],
['non-string', [null, 'https://example.com/app/']],
];
for (const [label, [u, b]] of cases) {
try {
const r = new URL(u, b);
console.log(label, '=>', r.origin, r.href);
} catch (e) {
console.log(label, '=> THROW', e.message);
}
}
JSRepository: unraid/api Length of output: 472 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "from ['\"].*composables/services/request['\"]|from ['\"]~/composables/services/request['\"]|request\." web/src -SRepository: unraid/api Length of output: 1525 Fail closed on parse errors. 🤖 Prompt for AI Agents |
||
|
|
||
| export const request = wretch() | ||
| .addon(formData) | ||
| .addon(formUrl) | ||
| .addon(queryString) | ||
| .errorType('json') | ||
| .defer((w, url) => { | ||
| const token = globalThis.csrf_token; | ||
| return token && isSameOriginRequest(url) ? w.headers({ 'x-csrf-token': token }) : w; | ||
| }) | ||
| .resolve((response) => { | ||
| return response | ||
| .error('Error', (error) => { | ||
|
|
||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize externally cleared model values.
Now that
undefinedis a valid update payload, the watcher at Line [42] must also assignundefined. Otherwise, changing a controlledmodelValuefrom a selected value toundefinedleavesopenValuestale and the accordion remains open.(val) => { - if (val !== undefined) openValue.value = val; + openValue.value = val; }Also applies to: 52-52
🤖 Prompt for AI Agents