fix: prevent data leak in getConnectedApps by explicitly picking safe fields#28924
fix: prevent data leak in getConnectedApps by explicitly picking safe fields#28924arpittkhandelwal wants to merge 2 commits into
Conversation
|
Welcome to Cal.diy, @arpittkhandelwal! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
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)
📝 WalkthroughWalkthroughThe file 🚥 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-store/_utils/getConnectedApps.ts`:
- Around line 150-179: The safeApp object in getConnectedApps is untyped, which
can silently diverge from AppFrontendPayload/ConnectedApps; add an explicit type
annotation for safeApp (e.g., a Pick of AppFrontendPayload listing the allowed
keys) plus the extra fields (enabled: boolean and locationOption typed from
rawApp.locationOption) so the compiler enforces the allow-list and prevents
accidental inclusion of sensitive fields (reference symbols: safeApp,
getConnectedApps, AppFrontendPayload, ConnectedApps, App).
🪄 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: 6211c491-c756-4f1d-bf49-fcb8539679a0
📒 Files selected for processing (1)
packages/app-store/_utils/getConnectedApps.ts
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
I am active I am waiting for someone to review this pr |
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
|
Arpit Khandelwal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
Fixes #28923
Description
This PR resolves a potential data leak vulnerability in
packages/app-store/_utils/getConnectedApps.tsidentified by an existing developerTODO.What changed:
Removed the object spread operator (
...app) and replaced it with a strict, explicitly-mappedsafeApppayload dictionary.Why it matters:
Previously, the backend used a "deny-list" approach using object destructuring, which would silently expose any newly developed internal codebase flags or secret
AppMetavariables straight to the frontend HTTP response payload. By switching to a rigorous "allow-list", we guarantee that only explicitly authorized, public-safe properties are ever returned out of the API.Type of change
How to test
getConnectedAppsquery (e.g. from the App Store or user settings page).