fix(cache): sanitize cache key segments to prevent ENOTDIR on fs drivers#4156
fix(cache): sanitize cache key segments to prevent ENOTDIR on fs drivers#4156claygeo wants to merge 2 commits intonitrojs:mainfrom
Conversation
When using `fs` or `fs-lite` as the storage driver, caching a route like `/foo` creates a file on disk. Caching `/foo/bar` then needs `/foo` to be a directory, which crashes with ENOTDIR. The root cause is that cache group and name values contain "/" characters (e.g., group "nitro/route-rules", name "/**:/foo") which unstorage's filesystem drivers interpret as directory separators after converting the ":" key separators to "/". Changes: - Replace "/" with "_" in all cache group names (nitro/functions -> nitro-functions, etc.) to prevent unexpected directory nesting - Sanitize the route-rules cache name by replacing "/" with "_" in route path keys before passing to ocache - Add sanitizeCacheKey() helper applied to user-provided cache names in defineCachedFunction/defineCachedHandler public APIs Fixes nitrojs#4142 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@claygeo is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization 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)
📝 WalkthroughWalkthroughSanitized cache group and name values by replacing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/runtime/internal/route-rules.ts (1)
73-80: Avoid duplicate name sanitization in route rules.
defineCachedHandleralready sanitizesopts.nameinsrc/runtime/internal/cache.ts. Keeping another localkey.replace(/\//g, "_")here duplicates behavior and can drift.Suggested simplification
- // Sanitize the name to avoid filesystem path conflicts. - // Route paths contain "/" which unstorage's fs drivers interpret as - // directory separators, causing ENOTDIR when a path like "/foo" is - // cached as a file but "/foo/bar" needs it to be a directory. - const safeName = key.replace(/\//g, "_"); cachedHandler = defineCachedHandler(handler, { group: "nitro-route-rules", - name: safeName, + name: key, ...m.options, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/internal/route-rules.ts` around lines 73 - 80, The local sanitization using safeName (key.replace(/\//g, "_")) is duplicating logic already performed by defineCachedHandler; remove the safeName variable and the key.replace call and pass the original key (or handler key variable) directly to defineCachedHandler when creating cachedHandler so the single sanitization in src/runtime/internal/cache.ts is used; update any references to safeName to use the original key name instead.src/runtime/internal/cache.ts (1)
16-24: Move internal helper(s) to the bottom of the module.
sanitizeCacheKeyis a non-exported helper; place it after exported APIs to match project structure conventions.As per coding guidelines, "Place non-exported/internal helpers at the end of the file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/internal/cache.ts` around lines 16 - 24, sanitizeCacheKey is an internal, non-exported helper and should be moved after the module's exported APIs; relocate the function declaration for sanitizeCacheKey(value: string | undefined): string | undefined to the end of src/runtime/internal/cache.ts (below all exported functions/variables) so file order follows the project convention of exported symbols first and internal helpers last, and ensure any references still resolve after the move.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/internal/cache.ts`:
- Around line 50-53: The code sanitizes opts.name via sanitizeCacheKey but
leaves opts.group unsanitized, allowing user-provided group values (e.g.,
containing "/") to cause filesystem path conflicts; fix by passing opts.group
through sanitizeCacheKey when merging defaults (replace group: "nitro-functions"
/ ...opts with group: sanitizeCacheKey(opts.group ?? "nitro-functions") or
equivalent) and apply the same change at the other occurrence referenced (the
block around lines 63-69) so any place that sets group from opts uses
sanitizeCacheKey.
---
Nitpick comments:
In `@src/runtime/internal/cache.ts`:
- Around line 16-24: sanitizeCacheKey is an internal, non-exported helper and
should be moved after the module's exported APIs; relocate the function
declaration for sanitizeCacheKey(value: string | undefined): string | undefined
to the end of src/runtime/internal/cache.ts (below all exported
functions/variables) so file order follows the project convention of exported
symbols first and internal helpers last, and ensure any references still resolve
after the move.
In `@src/runtime/internal/route-rules.ts`:
- Around line 73-80: The local sanitization using safeName (key.replace(/\//g,
"_")) is duplicating logic already performed by defineCachedHandler; remove the
safeName variable and the key.replace call and pass the original key (or handler
key variable) directly to defineCachedHandler when creating cachedHandler so the
single sanitization in src/runtime/internal/cache.ts is used; update any
references to safeName to use the original key name instead.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3727a061-6631-4730-983f-d8103f807036
📒 Files selected for processing (2)
src/runtime/internal/cache.tssrc/runtime/internal/route-rules.ts
commit: |
Address CodeRabbit review: opts.group was passed through raw, so a custom group containing "/" could still trigger ENOTDIR on fs drivers. Now both group and name are sanitized through sanitizeCacheKey().
|
Thank you for the PR ❤️ Keeping nested namespaces is a delibrate feature it allows faster lookups, easier invalidation and namespaced mounting for using different cache backends per prefix. I haven't looked deeply into this issue but we can either workaround it in ocache key generation for I will keep PR as draft until we move it forward. |
|
Thanks for the context — that makes total sense, keeping the namespace structure for lookups and invalidation is clearly the right call. The ocache approach sounds cleaner to me since it keeps the fix at the key-generation layer rather than adding a flag that only applies to one driver. Happy to take a look at that path if you can point me at the relevant spot in ocache, or I can wait if you'd rather land a different fix there first. Either way I'm happy to update this PR or close it in favour of whatever direction fits best. |
|
Thanks @pi0 — that context makes sense. Keeping the hierarchical namespacing in ocache for performance/invalidation is the right call. The issue is specific to the Happy to move the fix upstream to the |
|
I think we can first start by tackling unstorage fs driver issues. thanks! |
Problem
Fixes #4142
When using
fsorfs-liteas the storage driver, caching routes that share path prefixes crashes withENOTDIR. For example:/foo→ creates a file at.cache/nitro/route-rules/**/foo/foo.abc123.json/foo/bar→ needsfooto be a directory →ENOTDIR: not a directoryThis breaks any Nitro/Nuxt app where a route can be both a leaf and a prefix of another route (e.g.
/enand/en/about,/items/123and/items/123/details).Real-world impact: Nuxt 4.4.0's runtime payload caching (nuxt/nuxt#34410) triggers this for any site with nested routes, as reported in nuxt/nuxt#34547. @danielroe's Nuxt-side workaround (nuxt/nuxt#34569) covers
devStoragebut not productionstoragewithfs-lite, and he suggested fixing this upstream in Nitro.Root Cause
Cache
groupandnamevalues contain/characters:"nitro/route-rules","nitro/handlers","nitro/functions""/**:/foo"(route path keys)Unstorage's fs drivers convert
:(key separator) to/(path separator). But the/characters already embedded in group/name values also become path separators, creating unpredictable directory structures where a path segment can be both a file and a directory prefix.Fix
Three layers of defense:
/with-in all cache group constants (nitro/functions→nitro-functions, etc.)/with_before passing to ocachesanitizeCacheKey()to user-providednamevalues indefineCachedFunctionanddefineCachedHandler, protecting any custom cache names from the same issueNo collision risk: ocache's
getKeyalready appends a hash of the full path to the storage key, so thenameis only a namespace prefix, not the unique identifier.