feat(dashboard): add data lifecycle feedback and purge capability#812
feat(dashboard): add data lifecycle feedback and purge capability#812yasinBursali wants to merge 4 commits intoLight-Heart-Labs:mainfrom
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Audit: APPROVE
Path traversal prevention is solid: resolve() + is_relative_to(). Service ID validated via _SERVICE_ID_RE. Core service guard (403). Confirmation gate (confirm: true required). The _get_service_data_info() helper is clean and reusable.
Minor notes (non-blocking):
shutil.rmtree(data_path, ignore_errors=True)silently swallows errors — the follow-up existence check catches incomplete removal, but root cause (locked files) won't be loggeddir_size_gb()is synchronous in an async endpoint — wrap inasyncio.to_threadto avoid blocking the event loop on large data directories- Purge button may appear for services with no data directory (user gets 404) — consider hiding when
data_infois null orphaned_storageendpoint also scans synchronously — same blocking concern
Depends on #804. Should merge after it.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Revised Audit: REQUEST CHANGES — race condition + missing path check + blocking I/O
Withdrawing my earlier approval after deeper review.
Bug 1 (MEDIUM — Security): Purge does not acquire _extensions_lock()
Race condition with enable_extension: (1) purge checks compose.yaml is gone → proceeds, (2) concurrent enable renames compose.yaml back + starts container, (3) purge shutil.rmtrees data directory under the running container. The purge endpoint should acquire _extensions_lock() and re-check enabled state inside the lock.
Bug 2 (LOW-MEDIUM — Security): _get_service_data_info() has no path traversal protection
The purge endpoint correctly uses resolve() + is_relative_to(), but _get_service_data_info() (called from disable/uninstall responses) constructs the same path WITHOUT those checks. A symlink planted at data/{service_id} would cause dir_size_gb to recursively walk the symlink target, leaking its size.
Fix: Add resolve() + is_relative_to() check in _get_service_data_info() before calling dir_size_gb().
Bug 3 (MEDIUM — Performance): Async endpoints with synchronous blocking I/O
Both purge_extension_data and orphaned_storage are async def but call dir_size_gb() and shutil.rmtree() synchronously. These block the event loop — for large model directories (tens of GB), the entire API hangs for seconds.
Fix: Either change to def (FastAPI auto-runs in threadpool) or wrap blocking calls in asyncio.to_thread().
Bug 4 (LOW — Info disclosure): 500 error leaks absolute path
raise HTTPException(status_code=500, detail=f"Could not fully remove {data_path}...")data_path is the resolved absolute path. Replace with data/{service_id}.
BLOCKER: Depends on PR #804 for from helpers import dir_size_gb.
What's confirmed solid: Purge path traversal check (resolve + is_relative_to), _SERVICE_ID_RE regex, server-side confirm gate, shutil.rmtree is safe against symlink-swap TOCTOU (resolved path is used), core service guard is dynamic from manifests, frontend purge button correctly scoped to disabled user extensions.
Addressing review feedbackAll 4 bugs fixed: Bug 1 (Race condition — purge without lock) — Fixed: Bug 2 (Path traversal in Bug 3 (Blocking I/O in async endpoints) — Fixed: Bug 4 (Absolute path in 500 error) — Fixed: Additionally: Added |
|
Note: The Rust dashboard-api rewrite (#821) merged and deleted the Python files this PR modifies. The Python router changes in this PR are dead ( Please rebase or rewrite against the current |
Add data_info to disable/uninstall API responses showing preserved data
size. Add DELETE /api/extensions/{id}/data purge endpoint with confirm
gate, path traversal prevention, and core service protection. Add GET
/api/storage/orphaned for detecting abandoned data directories. Update
Extensions UI with purge button and data-aware confirmation dialogs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…path leak Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove duplicate dir_size_gb (now in helpers.py from upstream) - Use dir_size_gb from helpers import in main.py instead of local def - Send JSON body in path traversal test for purge endpoint to avoid FastAPI 422 from missing PurgeRequest body Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
89d173f to
d53ccd2
Compare
|
All 4 issues from the revised audit are addressed in the current code:
Also fixed CI failures after rebase:
Rebased on latest main, all checks green. |
What
data_infoto disable/uninstall API responses showing preserved data size and purge commandDELETE /api/extensions/{id}/datapurge endpoint with confirm gateGET /api/storage/orphanedfor detecting abandoned data directoriesWhy
dream purgecommand (PR 2) in the web UIHow
_get_service_data_info()scansdata/{service_id}viadir_size_gb(), returns size/path/purge_command or nullresolve()+is_relative_to()), requires{"confirm": true}in body, measures size beforeshutil.rmtree, verifies removaldata/directories, excludes SERVICES keys + system dirs (models, config, user-extensions, extensions-library)Three Pillars Impact
Modified Files
dashboard-api/routers/extensions.py—_get_service_data_info(), data_info in responses, purge + orphaned endpointsdashboard/src/pages/Extensions.jsx— purge button, confirmation dialog, data_info displayTesting
Automated
Manual
data_infowith sizeDELETE /api/extensions/n8n/datawithoutconfirm: true→ 400DELETE /api/extensions/dashboard/data→ 403 (core service)DELETE /api/extensions/n8n/data(enabled) → 400DELETE /api/extensions/n8n/datawith{"confirm": true}→ success + size freedGET /api/storage/orphaned→ lists dirs not in SERVICESReview
Platform Impact
Sequence
PR 7 of 7 (Phase 3). Depends on PR 3 (#804 — dir_size_gb extraction)