fix(server): guard collection lookup against prototype keys#488
fix(server): guard collection lookup against prototype keys#488itsmelouis wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe server icon API adds an explicit existence check before accessing the bundled collections map. Instead of relying solely on optional chaining to handle potentially missing collections, the code now verifies the collection exists using Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
src/runtime/server/api.ts (1)
31-31: 💤 Low valueOptional: Remove redundant optional chaining.
Since
Object.hasOwn(collections, collectionName)guarantees the property exists, the optional chaining?.()on line 31 is technically redundant. You could simplify tocollections[collectionName]()for clarity. However, keeping the defensive optional chaining is perfectly acceptable.♻️ Optional simplification
const collection = collectionName && Object.hasOwn(collections, collectionName) - ? await collections[collectionName]?.() + ? await collections[collectionName]() : null🤖 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 `@src/runtime/server/api.ts` at line 31, The call uses redundant optional chaining: after confirming Object.hasOwn(collections, collectionName), replace the defensive call expression collections[collectionName]?.() with a direct call collections[collectionName]() to simplify the code; locate the expression using the symbols collections and collectionName and update that invocation accordingly (you may keep the optional chaining only if you prefer the extra defensive style).
🤖 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.
Nitpick comments:
In `@src/runtime/server/api.ts`:
- Line 31: The call uses redundant optional chaining: after confirming
Object.hasOwn(collections, collectionName), replace the defensive call
expression collections[collectionName]?.() with a direct call
collections[collectionName]() to simplify the code; locate the expression using
the symbols collections and collectionName and update that invocation
accordingly (you may keep the optional chaining only if you prefer the extra
defensive style).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17722e55-2ecb-488e-9131-4def31b92bc6
📒 Files selected for processing (1)
src/runtime/server/api.ts
🔗 Linked issue
No related issue.
📚 Description
src/runtime/server/api.tslooks the collection up viacollections[collectionName], which also resolves inherited keys fromObject.prototype(__proto__,toString,constructor, ...). The resolved value gets passed down togetIcons(),which is not designed to handle it and crashes --> 500.
The fix is one line: gate the lookup with
Object.hasOwn(collections, collectionName)so only own keys are considered. Unknown / prototype keys now fall through to the API fallback (or 404 iffallbackToApiis disabled), same as any other unknown collection.Repro in the playground:
pnpm dev curl -i 'http://localhost:3000/api/_nuxt_icon/toString.json?icons=bad'