refactor: simplify link-as-an-app template and migrate 24 existing apps #27215
Conversation
- Remove api/, components/, index.ts, package.json from link-as-an-app template - Add externalLink field to AppMetaSchema for external URL configuration - Update API handler to dynamically create handlers for external link apps - External link apps no longer create credentials (just redirect to URL) - Update CLI to handle externalLinkUrl field for link-as-an-app template - CLI now skips package.json update for apps without package.json Co-Authored-By: peer@cal.com <peer@cal.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
External link apps now directly return the redirect URL without going through the AppDeclarativeHandler type, avoiding type conflicts with the createCredential return type requirement. Co-Authored-By: peer@cal.com <peer@cal.com>
Changes SummaryThis PR refactors the 'link-as-an-app' template to be purely configuration-driven, eliminating code files (api/, components/, index.ts, package.json) in favor of just config.json and static assets. External link apps now bypass credential creation entirely by reading the externalLink field from app metadata and returning the redirect URL directly from the API handler. Type: refactoring Components Affected: app-store template system, app integration API handler, app-store CLI, app metadata schema Files Changed
Architecture Impact
Risk Areas: Type assertion at line 70 in [...args].ts could fail if externalLink has unexpected structure, Existing apps created with old link-as-an-app template (zapier, pipedream, baa-for-hipaa mentioned in PR) need verification they still work, No authentication/authorization checks before returning external link URL - relies on existing session check at handler entry, Potential for misconfiguration if externalLinkUrl is malformed during CLI app creation, Generated apps.server.generated.ts file change suggests build process dependency Suggestions
Full review in progress... | Powered by diffray |
| ) as AppMeta & { externalLink?: { url: string; newTab?: boolean } }; | ||
| initialConfig = { | ||
| ...config, | ||
| category: config.categories[0], |
There was a problem hiding this comment.
🟠 HIGH - Unsafe array access without null check on categories
Agent: bugs
Category: bug
Description:
Accessing config.categories[0] without checking if array exists or has elements could assign undefined to initialConfig.category. The schema defines categories as z.array(z.string()) which is always an array but could be empty.
Suggestion:
Add null/empty check: category: config.categories?.[0] || ''
Confidence: 85%
Rule: ts_unsafe_null_access
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| if (template === "link-as-an-app" && externalLinkUrl) { | ||
| config.externalLink = { | ||
| url: externalLinkUrl, | ||
| newTab: true, | ||
| }; | ||
| // Also update the legacy url field for backwards compatibility | ||
| config.url = externalLinkUrl; | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Missing input validation for externalLinkUrl parameter
Agent: security
Category: security
Description:
The externalLinkUrl parameter from user input is written directly to config.json without validation of URL format or protocol. While this is a CLI tool run by developers, malicious or malformed URLs could be persisted and later served to end users.
Suggestion:
Add URL validation: try { const parsed = new URL(externalLinkUrl); if (!['http:', 'https:'].includes(parsed.protocol)) { throw new Error('Only http and https protocols allowed'); } } catch { throw new Error('Invalid URL format'); }
Confidence: 70%
Rule: security_missing_input_validation
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| ].filter((f) => f); | ||
|
|
||
| // Add external link URL field for link-as-an-app template | ||
| // Use initialConfig.template since appInputData is not yet available | ||
| const templateForFields = initialConfig.template || cliTemplate; | ||
| if (templateForFields === "link-as-an-app") { | ||
| // Insert after category field | ||
| const categoryIndex = fields.findIndex((f) => f?.name === "category"); | ||
| if (categoryIndex !== -1) { | ||
| fields.splice(categoryIndex + 1, 0, { | ||
| optional: false, | ||
| label: "External Link URL", | ||
| name: "externalLinkUrl", | ||
| type: "text", | ||
| explainer: "The URL users will be redirected to when they install this app (e.g., https://example.com/signup)", | ||
| defaultValue: "https://example.com", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Array mutation in render function
Agent: react
Category: performance
Description:
The fields array is being mutated using splice() during render. While fields is recreated each render so this doesn't cause bugs, mutating arrays during render is not idiomatic React and could cause issues if the code is refactored.
Suggestion:
Build the fields array declaratively using useMemo or create a new array with spread/concat instead of splice
Confidence: 75%
Rule: react_component_consistency
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| try { | ||
| const config = JSON.parse( | ||
| fs.readFileSync(`${getAppDirPath(givenSlug, isTemplate)}/config.json`).toString() | ||
| ) as AppMeta; | ||
| ) as AppMeta & { externalLink?: { url: string; newTab?: boolean } }; | ||
| initialConfig = { | ||
| ...config, | ||
| category: config.categories[0], | ||
| template: config.__template, | ||
| externalLinkUrl: config.externalLink?.url || "", | ||
| }; | ||
| } catch (e) {} |
There was a problem hiding this comment.
🟠 HIGH - Empty catch block silently swallows errors
Agent: react
Category: bug
Description:
The catch block is completely empty (catch (e) {}), which silently swallows any errors that occur when reading and parsing the config.json file.
Suggestion:
Add error logging: catch (e) { console.error('Failed to load config for', givenSlug, e); }
Confidence: 95%
Rule: ts_avoid_empty_catch_blocks
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| }; | ||
| const currentConfig = JSON.parse(fs.readFileSync(`${appDirPath}/config.json`).toString()); | ||
| config = { | ||
| ...currentConfig, |
There was a problem hiding this comment.
🟡 MEDIUM - Type assertion after JSON.parse without schema validation
Agent: typescript
Category: bug
Description:
The code reads a config.json file and uses JSON.parse without validating the result against a schema. Malformed data bypasses TypeScript's type checking.
Suggestion:
Validate the parsed JSON using Zod schema before merging with new config.
Confidence: 75%
Rule: ts_config_validate_before_cast
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| const config = JSON.parse( | ||
| fs.readFileSync(`${getAppDirPath(givenSlug, isTemplate)}/config.json`).toString() | ||
| ) as AppMeta; | ||
| ) as AppMeta & { externalLink?: { url: string; newTab?: boolean } }; |
There was a problem hiding this comment.
🟡 MEDIUM - Type assertion after JSON.parse without schema validation
Agent: typescript
Category: bug
Description:
The code reads a JSON file and uses a type assertion (as AppMeta & {...}) without validating the parsed data. If the JSON is malformed, TypeScript's type safety is bypassed.
Suggestion:
Validate the parsed JSON using a Zod schema before casting to ensure type safety at runtime.
Confidence: 80%
Rule: ts_config_validate_before_cast
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| initialConfig = { | ||
| ...config, | ||
| category: config.categories[0], | ||
| template: config.__template, |
There was a problem hiding this comment.
🟠 HIGH - Unsafe property access without null check on __template
Agent: bugs
Category: bug
Description:
Accessing config.__template without checking if it exists could assign undefined to initialConfig.template, potentially causing issues later.
Suggestion:
Use nullish coalescing: template: config.__template || ''
Confidence: 70%
Rule: ts_missing_optional_chaining
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| const formCompleted = inputIndex === fields.length; | ||
| if (field?.name === "appCategory") { | ||
| // Use template category as the default category | ||
| fieldValue = Templates.find((t) => t.value === appInputData["template"])?.category || ""; | ||
| } | ||
| const slug = getSlugFromAppName(name) || givenSlug; | ||
|
|
||
| useEffect(() => { | ||
| // When all fields have been filled | ||
| (async () => { | ||
| if (formCompleted) { | ||
| await BaseAppFork.create({ | ||
| category, | ||
| description, | ||
| name, | ||
| slug, | ||
| publisher, | ||
| email, | ||
| template, | ||
| editMode: isEditAction, | ||
| isTemplate, | ||
| oldSlug: givenSlug, | ||
| externalLinkUrl, | ||
| }); | ||
|
|
||
| await generateAppFiles(); |
There was a problem hiding this comment.
🟠 HIGH - Async operations without error handling
Agent: react
Category: bug
Description:
The useEffect hook contains async operations (BaseAppFork.create() and generateAppFiles()) that are not wrapped in a try-catch block. If either of these async operations fail, the error will be unhandled and the component will be left in an inconsistent state with formCompleted=true but status still 'inProgress'.
Suggestion:
Wrap the async operations in try-catch: try { await BaseAppFork.create(...); await generateAppFiles(); setStatus('done'); } catch (error) { console.error('Failed to create app:', error); setStatus('error'); }
Confidence: 90%
Rule: ts_handle_async_operations_with_proper_erro
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| } | ||
|
|
||
| const [appName, apiEndpoint] = args; | ||
| try { |
There was a problem hiding this comment.
🟡 MEDIUM - Missing HTTP method validation in API handler
Agent: quality
Category: quality
Description:
Handler accepts all HTTP methods without validation. Line 84 calls defaultIntegrationAddHandler which creates credentials via GET, which is semantically incorrect.
Suggestion:
Add HTTP method validation: if (req.method !== 'POST') { return res.status(405).json({ message: 'Method not allowed' }); }
Confidence: 75%
Rule: api_wrong_http_method
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| }; | ||
| const currentConfig = JSON.parse(fs.readFileSync(`${appDirPath}/config.json`).toString()); | ||
| config = { | ||
| ...currentConfig, |
There was a problem hiding this comment.
🟡 MEDIUM - JSON.parse without error handling
Agent: react
Category: bug
Description:
Line 121 reads config.json and parses it without try-catch. If the file doesn't exist or contains invalid JSON, the function will throw without graceful error handling.
Suggestion:
Wrap in try-catch or add existence check like line 39: if (!fs.existsSync(configPath)) { throw new Error('config.json not found'); }
Confidence: 75%
Rule: ts_handle_async_operations_with_proper_erro
Review ID: 4f34730a-9b30-4ed7-99c0-82f62e8066fc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 60 issues: 26 kept, 34 filtered Issues Found: 26💬 See 23 individual line comment(s) for details. 📊 20 unique issue type(s) across 26 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Open Redirect vulnerability - partial mitigation onlyAgent: security Category: security File: Description: Line 91 validates that URL uses http/https protocol, but doesn't validate the domain. An attacker who can control externalLink.url in app metadata could redirect users to any http/https malicious site for phishing. Suggestion: Add domain allowlist validation or restrict to same-origin plus known trusted domains. The current regex check Confidence: 80% Rule: 🟠 HIGH - Unsafe array access without null check on categoriesAgent: bugs Category: bug File: Description: Accessing config.categories[0] without checking if array exists or has elements could assign undefined to initialConfig.category. The schema defines categories as z.array(z.string()) which is always an array but could be empty. Suggestion: Add null/empty check: category: config.categories?.[0] || '' Confidence: 85% Rule: 🟠 HIGH - Missing input validation for externalLinkUrl parameter (2 occurrences)Agent: security Category: security 📍 View all locations
Rule: 🟠 HIGH - Empty catch block silently swallows errorsAgent: react Category: bug File: Description: The catch block is completely empty (catch (e) {}), which silently swallows any errors that occur when reading and parsing the config.json file. Suggestion: Add error logging: Confidence: 95% Rule: 🟠 HIGH - Unsafe property access without null check on __templateAgent: bugs Category: bug File: Description: Accessing config.__template without checking if it exists could assign undefined to initialConfig.template, potentially causing issues later. Suggestion: Use nullish coalescing: template: config.__template || '' Confidence: 70% Rule: 🟠 HIGH - Async operations without error handling (2 occurrences)Agent: react Category: bug 📍 View all locations
Rule: 🟠 HIGH - Missing authentication check for external link appsAgent: security Category: security File: Description: The external link handler (lines 65-72) returns URL without verifying user authentication. While getServerSession is called at line 48, there's no check for req.session?.user before returning external link data. Auth validation only occurs in defaultIntegrationAddHandler which isn't called for external links. Suggestion: Add authentication validation before returning external link URLs: if (!req.session?.user?.id) { Confidence: 85% Rule: 🟠 HIGH - Optional chaining with -1 fallback misses undefinedAgent: typescript Category: quality File: Description: The code uses field?.options?.findIndex() which could return undefined (not -1) if field.options is undefined. The subsequent check 'selectedOptionIndex === -1 ? 0 : selectedOptionIndex' at line 353 won't properly handle undefined, potentially passing undefined to initialIndex. Suggestion: Use explicit undefined check or nullish coalescing: Confidence: 85% Rule: 🟡 MEDIUM - Array mutation in render functionAgent: react Category: performance File: Description: The fields array is being mutated using splice() during render. While fields is recreated each render so this doesn't cause bugs, mutating arrays during render is not idiomatic React and could cause issues if the code is refactored. Suggestion: Build the fields array declaratively using useMemo or create a new array with spread/concat instead of splice Confidence: 75% Rule: 🟡 MEDIUM - Hardcoded app categories duplicate existing constantsAgent: architecture Category: quality File: Description: The app categories are hardcoded directly in the form component, duplicating the categories already defined in @calcom/app-store/_utils/getAppCategories.ts. This creates a maintenance burden where changes to categories must be made in multiple locations. Suggestion: Import and reuse the existing app categories from getAppCategories() or create a shared constant. The TODO on line 88 acknowledges this issue. Confidence: 95% Rule: 🟡 MEDIUM - Type assertion after JSON.parse without schema validation (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Missing HTTP method validation in API handlerAgent: quality Category: quality File: Description: Handler accepts all HTTP methods without validation. Line 84 calls defaultIntegrationAddHandler which creates credentials via GET, which is semantically incorrect. Suggestion: Add HTTP method validation: if (req.method !== 'POST') { return res.status(405).json({ message: 'Method not allowed' }); } Confidence: 75% Rule: 🟡 MEDIUM - Reassigning function parameterAgent: react Category: quality File: Description: The function parameter 'cliTemplate' is being reassigned on line 24. This mutates the input parameter and makes the code harder to reason about. Suggestion: Use a different variable name: const processedTemplate = Templates.find((t) => t.value === cliTemplate)?.value || ''; Confidence: 80% Rule: 🟡 MEDIUM - Synchronous file operations in CLI functionAgent: performance Category: performance File: Description: updatePackageJson uses fs.existsSync, fs.readFileSync, and fs.writeFileSync. While blocking, this is a CLI context where it's less critical than server code. Suggestion: Consider refactoring to use fs.promises API for consistency with async pattern already used in BaseAppFork.create Confidence: 60% Rule: 🟡 MEDIUM - Shell command injection risk in file operationsAgent: security Category: security File: Description: Shell commands are constructed using string concatenation with template and slug variables. While slug is sanitized via slugify(), template comes from Templates array without explicit runtime validation before shell execution. Suggestion: Add explicit validation that template is in allowed Templates list, or use Node.js fs module functions (fs.mkdirSync, fs.cpSync) instead of shell commands. Confidence: 60% Rule: 🔵 LOW - Non-strict equality checkAgent: react Category: quality File: Description: Using loose equality (==) instead of strict equality (===) on line 316. Loose equality can lead to unexpected type coercion. Suggestion: Use strict equality: if (field?.type === 'text') Confidence: 95% Rule: 🔵 LOW - Prefer specific type over z.any() (2 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🔵 LOW - Template-specific field injection via array mutationAgent: architecture Category: quality File: Description: The form dynamically injects fields for specific templates by mutating the fields array with splice (line 128). This mixed declarative/imperative approach reduces code clarity. Suggestion: Use a declarative approach: build fields array with showForTemplates property on each field, then filter based on current template. Confidence: 70% Rule: 🔵 LOW - Dual configuration fields for backwards compatibilityAgent: architecture Category: quality File: Description: The code updates both config.externalLink.url and config.url with the same value. This duplication creates ambiguity about which field is the source of truth. Suggestion: Add a deprecation plan with timeline for the legacy url field. Document which field is canonical. Confidence: 60% Rule: 🔵 LOW - New schema field 'externalLink' lacks test coverage (3 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: ℹ️ 3 issue(s) outside PR diff (click to expand)
🔴 CRITICAL - Open Redirect vulnerability - partial mitigation onlyAgent: security Category: security File: Description: Line 91 validates that URL uses http/https protocol, but doesn't validate the domain. An attacker who can control externalLink.url in app metadata could redirect users to any http/https malicious site for phishing. Suggestion: Add domain allowlist validation or restrict to same-origin plus known trusted domains. The current regex check Confidence: 80% Rule: 🟠 HIGH - Optional chaining with -1 fallback misses undefinedAgent: typescript Category: quality File: Description: The code uses field?.options?.findIndex() which could return undefined (not -1) if field.options is undefined. The subsequent check 'selectedOptionIndex === -1 ? 0 : selectedOptionIndex' at line 353 won't properly handle undefined, potentially passing undefined to initialIndex. Suggestion: Use explicit undefined check or nullish coalescing: Confidence: 85% Rule: 🟡 MEDIUM - Hardcoded app categories duplicate existing constantsAgent: architecture Category: quality File: Description: The app categories are hardcoded directly in the form component, duplicating the categories already defined in @calcom/app-store/_utils/getAppCategories.ts. This creates a maintenance burden where changes to categories must be made in multiple locations. Suggestion: Import and reuse the existing app categories from getAppCategories() or create a shared constant. The TODO on line 88 acknowledges this issue. Confidence: 95% Rule: 🔇 6 low-severity issue(s) not posted (min: medium)
📝 7 additional issue(s) shown in summary only (max: 10 inline)
Review ID: |
Migrated the following apps to use config.json with externalLink field instead of api/add.ts handlers: - amie, autocheckin, baa-for-hipaa, bolna, chatbase, clic, cron, deel - elevenlabs, fonio-ai, framer, granola, greetmate-ai, lindy, millis-ai - monobot, n8n, pipedream, raycast, retell-ai, synthflow, telli, vimcal - wordpress Each app now only contains: config.json, DESCRIPTION.md, static/ Removed: api/, components/, index.ts, package.json Co-Authored-By: peer@cal.com <peer@cal.com>
devin-ai-integration
bot
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
What does this PR do?
Simplifies the
link-as-an-appapp template from generating multiple code files to just aconfig.json,DESCRIPTION.md, and/static/folder. External link apps are now purely configuration-driven and don't create credentials since they're just redirects to external URLs.Additionally, this PR migrates all 24 existing external link apps to the new simplified structure.
Key changes:
api/,components/,index.ts,package.jsonfrom the link-as-an-app templateexternalLinkfield toAppMetaSchemafor external URL configurationexternalLinkUrlwhen using link-as-an-app templatepackage.jsonupdate for apps without oneMigrated apps (24 total):
amie, autocheckin, baa-for-hipaa, bolna, chatbase, clic, cron, deel, elevenlabs, fonio-ai, framer, granola, greetmate-ai, lindy, millis-ai, monobot, n8n, pipedream, raycast, retell-ai, synthflow, telli, vimcal, wordpress
Link to Devin run: https://app.devin.ai/sessions/4c197fcfb40540df9c42a120d6178a27
Requested by: @PeerRich
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Test a migrated app (e.g., baa-for-hipaa):
Test creating a new external link app:
yarn create-appand select thelink-as-an-apptemplateconfig.json,DESCRIPTION.md, andstatic/folderconfig.jsoncontains theexternalLinkfield with the URLVerify non-external-link apps still work:
api/subscriptions/) should continue to work normallyChecklist
Human Review Checklist
externalLinkin the API handler is safe (line 70 in[...args].ts)externalLink.urlmatches the originalredirect.urlfrom their deletedapi/add.ts