fix(api): confine notification, log, and API-key file operations - #2076
Conversation
Validate notification filenames before filesystem access for delete, archive, restore, and creation. Preserve supported filenames and malformed notification cleanup. Work intent: FeatureOS security report 482091. Regression coverage reproduces deletion outside the notification directory on the original implementation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe API adds a shared file path validation utility. Notification, API-key, and log services use it to reject unsafe paths and prevent directory traversal. Tests cover invalid inputs, safe filenames, and filesystem operations. ChangesFile path validation
Priority: ⬇️ Low — Defer this API path-validation change because its supplied scope is limited to confining notification, log, and API-key file operations within storage directories. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to File-backed notification, API-key, and log operations now reject unsafe paths while preserving supported filename and log-path behavior. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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. A rabbit reads each line, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2076 +/- ##
==========================================
+ Coverage 53.33% 53.36% +0.03%
==========================================
Files 1043 1043
Lines 72664 72674 +10
Branches 8387 8395 +8
==========================================
+ Hits 38758 38786 +28
+ Misses 33779 33761 -18
Partials 127 127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Reuse notification filename validation at filesystem consumers. Reject directory-only log targets and unsafe stored key IDs, and validate deletion batches before unlinking. Work intent: extend FeatureOS report 482091 remediation with a filesystem-boundary audit. Verified 177 focused tests and API type checks.
Eli Bosley (elibosley)
changed the title
🔄 PR Merged - Plugin Redirected to StagingThis PR has been merged and the preview plugin has been updated to redirect to the staging version. For users testing this PR:
Staging URL: Thank you for testing! 🚀 |
1 participant
Summary
Confine notification, log, and API-key file operations to their storage directories. Notification IDs previously allowed outside-file deletion; the broader audit also found invalid log watcher paths and missing validation of stored API-key IDs.
Why This Exists
Work intent: FeatureOS security report 482091, expanded to audit other API filesystem boundaries.
The notification regression reproduced deletion of an outside temporary file. Log subscriptions accepted parent-directory paths. API-key saves and deletes trusted IDs supplied internally or loaded from disk. The public API generates new key IDs and looks up existing IDs, so the API-key change is additional protection against unsafe stored/internal data, not a demonstrated remote key-creation exploit.
Resolution
Extract the notification filename check into
resolveFileInDirectory. Reject empty filenames, dot directory entries, separators, colons, and null bytes; require the resolved parent to equal the storage directory. Apply it before notification operations, log reads/subscriptions, and API-key reads/writes/deletes. Validate all deletion paths before deleting any key.Reviewer Considerations
PrefixedIDgeneral: valid resource IDs such as network metric IDs contain slashes. Enforce filename rules at filesystem consumers.Audit Scope
Inspected filesystem calls and path construction in
api/srcand shipped workspace package sources, then traced candidate values to their callers..and..usable as watcher targets; reads and subscriptions now validate.writeToBootVerification
Node 22, pnpm 10.15.0, from the repository root:
pnpm --filter ./api test src/core/utils/files/resolve-file-in-directory.spec.ts src/unraid-api/auth/api-key src/unraid-api/auth/cookie.service.spec.ts src/unraid-api/graph/resolvers/api-key src/unraid-api/graph/resolvers/logs src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts src/unraid-api/rest/rest.service.spec.ts src/unraid-api/graph/resolvers/metrics/network/network.service.spec.ts— 177 tests passed across 12 files.pnpm --filter ./api type-check— passed.git diff --check— passed.The original notification tests passed before the fix, while the outside-file regression failed. Twelve additional log/API-key regression cases failed before their protections were added.
Risk and Limits
Main compatibility concern: locally supplied API-key IDs containing filename separators or colons now fail. Valid IDs and full log paths remain supported by tests. This audit addresses application path traversal, not malicious replacement of trusted directories or symlinks by a local writer. It is not a guarantee that every filesystem-related security issue is absent. No live NAS was modified.
Summary by CodeRabbit