Skip to content

fix: use allow-list in getConnectedApps to prevent accidental data leakage#29472

Closed
Bhavya-jain07 wants to merge 3 commits into
calcom:mainfrom
Bhavya-jain07:fix/getConnectedApps-allowlist
Closed

fix: use allow-list in getConnectedApps to prevent accidental data leakage#29472
Bhavya-jain07 wants to merge 3 commits into
calcom:mainfrom
Bhavya-jain07:fix/getConnectedApps-allowlist

Conversation

@Bhavya-jain07
Copy link
Copy Markdown

What does this PR do?

Replaces the unsafe ...app spread in getConnectedApps.ts with an
explicit allow-list of safe fields. This prevents any future internal
fields from being accidentally leaked to the frontend.

Why?

The existing code used a deny-list pattern (excluding credentials and
key) and then spread the rest via ...app. A // TODO comment in
the file already flagged this as a known issue.

Changes

  • packages/app-store/_utils/getConnectedApps.ts: replaced ...app
    spread with an explicit field allow-list

Fixes #28923

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to Cal.diy, @Bhavya-jain07! Thanks for opening this pull request.

A few things to keep in mind:

  • This is Cal.diy, not Cal.com. Cal.diy is a community-driven, fully open-source fork of Cal.com licensed under MIT. Your changes here will be part of Cal.diy — they will not be deployed to the Cal.com production app.
  • Please review our Contributing Guidelines if you haven't already.
  • Make sure your PR title follows the Conventional Commits format.

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added the 🐛 bug Something isn't working label May 27, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bbf37718-ecb9-4ae8-a06a-521d42e29b1a

📥 Commits

Reviewing files that changed from the base of the PR and between a9b47fa and 2556978.

📒 Files selected for processing (1)
  • packages/app-store/_utils/getConnectedApps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/app-store/_utils/getConnectedApps.ts

📝 Walkthrough

Walkthrough

The getConnectedApps utility removes an inline TODO and now constructs returned items by explicitly enumerating selected app fields (slug, name, logo, categories, variant, type, description, dirName, isGlobal, dependencies, extendsFeature, locationOption, and key) instead of spreading the full app; computed credential, team, install, and setup properties are unchanged.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing unsafe spread operator with explicit field allow-list to prevent data leakage in getConnectedApps.
Description check ✅ Passed The description clearly explains what the PR does, why it's necessary, and references the specific file and issue being addressed.
Linked Issues check ✅ Passed The PR fully addresses issue #28923 by replacing the deny-list pattern with explicit allow-list enumeration of safe fields (slug, name, logo, categories, variant, type, description, dirName, isGlobal, dependencies, extendsFeature) and removing the unsafe ...app spread.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the data leakage vulnerability in getConnectedApps.ts by replacing the spread operator with explicit field enumeration, with no unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@packages/app-store/_utils/getConnectedApps.ts`:
- Around line 206-217: The returned allow-listed app object from
getConnectedApps is missing locationOption so the UI can't read
item?.locationOption?.value; update the object returned in the function (the
allow-list/mapper that currently returns slug, name, logo, categories, variant,
type, description, dirName, isGlobal, dependencies, extendsFeature, etc.) to
include locationOption: app.locationOption (safe because it's derived in
packages/app-store/utils.ts), ensuring you keep the existing conditional spreads
like ...(teams.length && { credentialOwner }) and ...(app.dependencies && {
dependencyData }) intact.
🪄 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: 0d0e0542-206a-487a-a56d-7500d0c61a5e

📥 Commits

Reviewing files that changed from the base of the PR and between 180ede2 and 0f3af64.

📒 Files selected for processing (1)
  • packages/app-store/_utils/getConnectedApps.ts

Comment thread packages/app-store/_utils/getConnectedApps.ts
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 27, 2026

CLA assistant check
All committers have signed the CLA.

@Bhavya-jain07 Bhavya-jain07 force-pushed the fix/getConnectedApps-allowlist branch from db523f4 to a9b47fa Compare May 27, 2026 19:25
@Bhavya-jain07 Bhavya-jain07 force-pushed the fix/getConnectedApps-allowlist branch from a9b47fa to 2556978 Compare May 27, 2026 19:35
@Bhavya-jain07
Copy link
Copy Markdown
Author

@cla-assistant recheck

@Bhavya-jain07
Copy link
Copy Markdown
Author

Hi, all checks are passing and the CLA is signed. Could a maintainer please add the run-ci label and review when you get a chance? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🔒 Security/Refactor: Prevent potential data leak in getConnectedApps by explicitly picking safe fields

2 participants