fix(web): send CSRF token on fetch-based webGUI requests#2039
fix(web): send CSRF token on fetch-based webGUI requests#2039elibosley wants to merge 3 commits into
Conversation
The API Status panel (Settings → Management Access) rendered "Not Running" on first paint and logged a red `wrong csrf_token` error in syslog, even though the API was running. Pressing "Refresh Status" worked, but revisiting the page reproduced it. Root cause: ApiStatus.vue runs its status check in onMounted using `serverStore.csrf`, which is empty until async server-state hydration completes. The old jQuery status call got its token auto-injected by the webGUI's `$.ajaxPrefilter`; the new fetch/wretch client injects nothing, so the first-paint POST carried a blank token. local_prepend.php rejected it, logged the error, and exited before unraid-api.php ran — so the response was empty and the panel showed "Not Running". - ApiStatus.vue: resolve the token from the synchronously-available page global (`globalThis.csrf_token`) with the store as fallback. - request.ts: attach `x-csrf-token` on every same-origin request from the shared wretch client (mirroring Apollo and `$.ajaxPrefilter`), so no fetch-based webGUI caller has to remember the token. Same-origin only, so the token is never sent to external hosts. Adds tests for both.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughApiStatus now resolves CSRF tokens from server-store or page-global state, while the request composable adds tokens to same-origin requests. New Vitest suites verify token selection, header gating, and running-status rendering. Accordion model updates now accept undefined. ChangesCSRF propagation
Accordion typing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ApiStatus
participant request
participant fetch
ApiStatus->>request: Initiate API request
request->>request: Check target origin
request->>fetch: Add x-csrf-token for same-origin request
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2039 +/- ##
==========================================
+ Coverage 52.77% 52.90% +0.13%
==========================================
Files 1035 1035
Lines 72060 72071 +11
Branches 8303 8318 +15
==========================================
+ Hits 38031 38132 +101
+ Misses 33903 33812 -91
- Partials 126 127 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/composables/services/request.ts`:
- Around line 8-23: Update isSameOriginRequest so its catch branch returns false
when URL parsing or location resolution fails. Preserve the existing same-origin
comparison for successfully parsed URLs, ensuring CSRF tokens are not attached
when the request origin cannot be verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1bae1867-cbc9-4607-a4e2-282bcf9ff2e3
📒 Files selected for processing (4)
web/__test__/components/ApiStatus.test.tsweb/__test__/composables/request.test.tsweb/src/components/ApiStatus/ApiStatus.vueweb/src/composables/services/request.ts
| /** | ||
| * 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; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 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. isSameOriginRequest already blocks absolute external URLs, but the catch still treats malformed or SSR/non-browser URLs as same-origin and attaches x-csrf-token. Return false there to keep the “never sent to external hosts” rule intact.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/composables/services/request.ts` around lines 8 - 23, Update
isSameOriginRequest so its catch branch returns false when URL parsing or
location resolution fails. Preserve the existing same-origin comparison for
successfully parsed URLs, ensuring CSRF tokens are not attached when the request
origin cannot be verified.
The `Build Web App` CI step (strict vue-tsc) failed on two pre-existing type errors in the Accordion component, which blocked artifact/plugin builds for PRs: - `computed` was imported but never used (TS6133). - The `update:modelValue` handler did not accept `undefined`, but AccordionRoot emits `string | string[] | undefined` (TS2322). Drop the unused import and widen the emit/handler to include `undefined`, which reflects the real "nothing selected" model value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@unraid-ui/src/components/common/accordion/Accordion.vue`:
- Line 34: Update the watcher around openValue to propagate externally cleared
modelValue values, including undefined, instead of ignoring them. Ensure both
the single-value and array-value branches assign the incoming undefined so
openValue is cleared and the accordion closes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd3a370-5d36-4d92-bea4-e91cd1983e32
📒 Files selected for processing (1)
unraid-ui/src/components/common/accordion/Accordion.vue
|
|
||
| const emit = defineEmits<{ | ||
| 'update:modelValue': [value: string | string[]]; | ||
| 'update:modelValue': [value: string | string[] | undefined]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize externally cleared model values.
Now that undefined is a valid update payload, the watcher at Line [42] must also assign undefined. Otherwise, changing a controlled modelValue from a selected value to undefined leaves openValue stale and the accordion remains open.
(val) => {
- if (val !== undefined) openValue.value = val;
+ openValue.value = val;
}Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@unraid-ui/src/components/common/accordion/Accordion.vue` at line 34, Update
the watcher around openValue to propagate externally cleared modelValue values,
including undefined, instead of ignoring them. Ensure both the single-value and
array-value branches assign the incoming undefined so openValue is cleared and
the accordion closes.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
1 participant
Problem
Settings → Management Access → Unraid API Status shows Not Running on first page load even though the API is running, and syslog logs a red error on every load:
Pressing Refresh Status flips it to Running, but revisiting the page reproduces both the false status and the log error. Reported independently by multiple users on the 7.3.2 forum thread.
Root cause
ApiStatus.vueruns its status check inonMountedusingserverStore.csrf. That store value starts empty ('') and is only populated after async server-state hydration completes. The webGUI's CSRF gate (local_prepend.php) validates every same-origin POST against$var['csrf_token'].The previous jQuery-based status call got its token auto-injected by the webGUI's
$.ajaxPrefilter. The new fetch/wretchclient injects nothing, so the first-paint POST carried a blank token. The gate rejected it, logged the error, andexited beforeunraid-api.phpran — so the response was empty and the panel rendered "Not Running". By the time the user clicks Refresh, hydration has populated the token, so it succeeds.This is not a webGUI CSRF regression — a blank token was always rejected. The trigger is the status panel moving from jQuery (auto-CSRF) to fetch (no auto-CSRF).
Fix
ApiStatus.vue— resolve the token from the synchronously-available page globalglobalThis.csrf_token(same source Apollo and onboarding already use), falling back to the store.request.ts— attachx-csrf-tokenon every same-origin request from the sharedwretchclient, mirroring what Apollo and$.ajaxPrefilteralready do, so no fetch-based webGUI caller has to remember the token. Same-origin only, so the token is never sent to external hosts.Tests
__test__/components/ApiStatus.test.ts— global token used on mount when the store is empty, never-blank token, store preferred once hydrated, renders "Running".__test__/composables/request.test.ts— header attached on same-origin GET/POST, not attached cross-origin, not attached when the page global is unset.Local: eslint clean,
vue-tscclean, full web suite green.Note
There is a companion webGUI-side PR (returns an explicit 403 on CSRF rejection instead of a silent empty 200, so a blocked read can't masquerade as "service down"). The two are independent; this client-side change alone removes the red log and fixes first paint.
Summary by CodeRabbit
update:modelValuenow supportsundefinedvalues.ApiStatusCSRF handling and “Running” status rendering.