feat: add npm and tarball app distribution with upgrade mechanism#18358
feat: add npm and tarball app distribution with upgrade mechanism#18358FelixMalfait merged 48 commits intomainfrom
Conversation
Implement a comprehensive app distribution system supporting both public (npm registry) and private (tarball upload) installation channels. Backend: - Add AppRegistrationSourceType enum (npm, tarball, none) to track app origin - Add AppPackageResolverService for resolving packages from npm or tarball sources - Add ApplicationInstallService with PostgreSQL advisory locks for safe installs - Add AppUpgradeService with version checking and rollback support - Add tarball upload REST endpoint with secure extraction (path traversal protection) - Add marketplace catalog sync cron job (hourly) from hardcoded catalog index - Add app version check cron job (every 6 hours) to detect available updates - Disable yarn lifecycle scripts (enableScripts: false) to prevent RCE via postinstall - Add database migration for sourceType, sourcePackage, latestAvailableVersion fields Frontend: - Add "Install from npm" modal for manual package installation - Add "Upload tarball" modal for direct .tar.gz uploads - Add upgrade mutation and version container with upgrade button - Add blue "Update" badge on installed apps table when newer version available - Fetch application registrations to compare installed vs latest versions - Migrate styled components from Emotion to Linaria (matching main migration) - Remove redundant sourcePackage mutation argument (derived from universalIdentifier) Testing: - Add integration tests for app distribution (install, upgrade, tarball upload) - Add integration tests for marketplace catalog sync Made-with: Cursor
TODOs/FIXMEs:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📊 API Changes ReportGraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Field Application.description changed type from String! to String |
- Create app-pack.ts and app-push.ts CLI commands (lost during merge) - Remove duplicate app:build command registration in app-command.ts - Fix Prettier formatting in server integration test files - Fix Prettier formatting in app-registration-upload controller Made-with: Cursor
Made-with: Cursor
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:60734 This environment will automatically shut down after 5 hours. |
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…ketplaceApp When universalIdentifier is a UUID and no matching registration exists, throw instead of falling through to create a new npm registration. Made-with: Cursor
Error code changed from BAD_USER_INPUT to FORBIDDEN. Made-with: Cursor
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
...gine/core-modules/application-registration/controllers/app-registration-upload.controller.ts
Outdated
Show resolved
Hide resolved
...erver/src/engine/core-modules/application/application-install/application-install.service.ts
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 67 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/core-modules/application-registration/controllers/app-registration-upload.controller.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application-registration/controllers/app-registration-upload.controller.ts:101">
P2: Wrap `JSON.parse(manifestContent)` in a try-catch to return a proper validation error (400) instead of an unhandled 500 when the manifest contains invalid JSON.</violation>
</file>
<file name="packages/twenty-front/src/pages/settings/applications/components/SettingsUploadTarballModal.tsx">
<violation number="1" location="packages/twenty-front/src/pages/settings/applications/components/SettingsUploadTarballModal.tsx:49">
P2: The modal closes even when install fails because the return value of `install` is ignored. This can dismiss the dialog after a failed install, forcing users to reopen it instead of retrying. Gate the refetch/close on `install` success.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/utils/get-admin-workspace-id.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/utils/get-admin-workspace-id.util.ts:13">
P2: Avoid relying on deletedAt soft-delete columns for core entity logic. This query now depends on deletedAt for user/workspace/userWorkspace, which conflicts with the guidance to avoid soft-delete usage outside applicationRegistration.
(Based on your team's feedback about avoiding deletedAt usage in core entity logic.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/application.exception.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/application.exception.ts:47">
P2: Use a generic user-facing message here; this repeats internal package-resolution details that shouldn't be exposed in the userFriendlyMessage.
(Based on your team's feedback about keeping userFriendlyMessage generic and non-technical.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/twenty-front/src/pages/settings/applications/hooks/useUploadAppTarball.ts">
<violation number="1" location="packages/twenty-front/src/pages/settings/applications/hooks/useUploadAppTarball.ts:26">
P2: Base64 conversion uses per-byte string concatenation, which is quadratic and can freeze the browser on larger tarballs. Use FileReader/readAsDataURL (or chunked encoding) to avoid O(n^2) string building.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/application-rest-api-exception-filter.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/application-rest-api-exception-filter.ts:25">
P2: Add a default/fallback branch in the exception-code switch to guarantee every caught exception sends an HTTP response.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/services/app-upgrade.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/app-upgrade.service.ts:44">
P2: Scoped npm packages like `@scope/name` must be URL-encoded in registry URLs; using the raw `sourcePackage` will 404 and prevent update checks for scoped packages. Encode the package name before building the URL.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/services/app-package-resolver.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/app-package-resolver.service.ts:249">
P2: Error messages expose full internal filesystem paths (e.g., `manifestPath` at line 245, `packageJsonPath` at line 253). If these exceptions propagate to client-facing responses, they leak server directory structure. Use a generic message and log the detailed path server-side only.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/services/application-install.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/application-install.service.ts:102">
P2: Files written to storage inside the transaction cannot be rolled back on failure. If `synchronizeFromManifest` or the commit fails, the DB transaction is rolled back but uploaded files remain as orphans in file storage. Consider moving `writeFilesToStorage` after the commit, or adding compensating cleanup in the `catch` block to delete the uploaded files.</violation>
<violation number="2" location="packages/twenty-server/src/engine/core-modules/application/services/application-install.service.ts:200">
P1: Security: `collectFiles` does not filter out symbolic links, allowing symlink-based path traversal. A malicious tarball can include a symlink pointing to an arbitrary file on the host (e.g., `/etc/shadow`). Since `Dirent.isDirectory()` returns `false` for symlinks, they fall through to the `else` branch and `fs.readFile` follows them. Add a symlink check and skip them:
```ts
if (entry.isSymbolicLink()) {
continue;
}
```</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/resolvers/marketplace.resolver.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/resolvers/marketplace.resolver.ts:42">
P1: Marketplace query isn’t scoped to a workspace (or admin catalog workspace), so it can return NPM registrations from other workspaces. Add workspace scoping (current workspace or admin workspace) to avoid cross-tenant leakage.</violation>
<violation number="2" location="packages/twenty-server/src/engine/core-modules/application/resolvers/marketplace.resolver.ts:50">
P2: Setting `hasSyncedOnce` before the catalog sync means a failed sync permanently disables retries. Move the flag assignment after a successful sync so transient failures don’t lock out marketplace data.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...erver/src/engine/core-modules/application/application-install/application-install.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/application/resolvers/marketplace.resolver.ts
Outdated
Show resolved
Hide resolved
...gine/core-modules/application-registration/controllers/app-registration-upload.controller.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/pages/settings/applications/components/SettingsUploadTarballModal.tsx
Outdated
Show resolved
Hide resolved
...engine/core-modules/application/application-marketplace/utils/get-admin-workspace-id.util.ts
Show resolved
Hide resolved
...s/twenty-server/src/engine/core-modules/application/application-rest-api-exception-filter.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/application/services/app-upgrade.service.ts
Outdated
Show resolved
Hide resolved
...s/twenty-server/src/engine/core-modules/application/services/app-package-resolver.service.ts
Outdated
Show resolved
Hide resolved
...erver/src/engine/core-modules/application/application-install/application-install.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/application/resolvers/marketplace.resolver.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR adds npm and tarball-based app distribution with an upgrade mechanism, marketplace catalog sync, and corresponding frontend UI. It's a substantial feature addition that introduces new services and REST/GraphQL endpoints. However, critical correctness issues were identified that prevent the feature from working as intended: Critical bugs:
Security & data integrity issues:
Multi-tenancy & scalability issues:
Operational & correctness issues:
Confidence Score: 1/5
Last reviewed commit: af9e551 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ); | ||
|
|
||
| const response = await fetch( | ||
| `${serverUrl}/api/application-registrations/upload-tarball`, |
There was a problem hiding this comment.
Mismatched API endpoint URL
The SDK posts to /api/application-registrations/upload-tarball, but the controller is registered at api/app-registrations (line 44 of app-registration-upload.controller.ts). This will result in a 404 error — the CLI push command will never succeed as written.
| `${serverUrl}/api/application-registrations/upload-tarball`, | |
| `${serverUrl}/api/app-registrations/upload-tarball`, |
There was a problem hiding this comment.
Fixed — corrected the endpoint to /api/app-registrations/upload-tarball and switched to JSON body with base64-encoded tarball to match the controller's expected format.
| const formData = new FormData(); | ||
| const fileBuffer = fs.readFileSync(tarballPath); | ||
|
|
||
| formData.append( | ||
| 'file', | ||
| new Blob([fileBuffer], { type: 'application/gzip' }), | ||
| path.basename(tarballPath), | ||
| ); | ||
|
|
||
| const response = await fetch( | ||
| `${serverUrl}/api/application-registrations/upload-tarball`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: formData, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Incompatible request format
The SDK sends a multipart/form-data request with a file field (lines 67–74), but the controller expects a JSON body with a base64-encoded tarball field (see TarballUploadBody type at line 39–42 of app-registration-upload.controller.ts and the base64 decode at line 81). These formats are incompatible — the controller will not find body.tarball from a multipart request, causing the upload to fail.
The SDK should encode the tarball to base64 and send it as a JSON body to match the controller's expectations.
There was a problem hiding this comment.
Fixed — the SDK now sends Content-Type: application/json with a { tarball: base64 } body, matching the controller's TarballUploadBody type.
| if (entryPath.includes('..')) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Incomplete path traversal protection
The filter checks for '..' using includes(), which catches the literal substring but may miss other traversal vectors (e.g., ....// or entries with encoded escape sequences). A more robust approach is to resolve the absolute path and verify it remains within the target directory:
| if (entryPath.includes('..')) { | |
| return false; | |
| } | |
| filter: (entryPath, entry) => { | |
| const resolvedPath = resolve(targetDir, entryPath); | |
| if (!resolvedPath.startsWith(resolve(targetDir) + sep)) { | |
| return false; | |
| } |
(You'll need to import resolve and sep from 'path'.)
Note: Symlinks and hard links are already blocked (lines 27–29), which is good.
There was a problem hiding this comment.
Fixed — now uses resolve(targetDir, entryPath) + startsWith(resolvedTarget) instead of includes('..') for robust path traversal protection.
| } | ||
|
|
||
| private async runYarnInstall(workDir: string): Promise<void> { | ||
| const localYarnPath = join(workDir, '.yarn/releases/yarn-4.9.2.cjs'); |
There was a problem hiding this comment.
Hardcoded yarn release filename
The path to the yarn binary is hardcoded as yarn-4.9.2.cjs. If the yarn version in YARN_ENGINE_DIRNAME changes (e.g., to yarn-5.0.0.cjs in a future update), this call will silently fail with a "file not found" error that surfaces only as a generic "yarn install failed" message. Consider dynamically resolving the CJS filename from the .yarn/releases/ directory contents, or at least add a clear error message if the file is missing.
There was a problem hiding this comment.
The yarn engine directory is a constant that ships bundled with the server — the .cjs filename and the constant are updated together. This won't silently break; a version bump to the engine would necessarily update both. Not a runtime concern.
There was a problem hiding this comment.
Now fixed — runYarnInstall reads .yarnrc.yml to resolve the yarnPath dynamically instead of hardcoding the version.
...ore-modules/application/application-marketplace/services/marketplace-catalog-sync.service.ts
Show resolved
Hide resolved
| export class MarketplaceResolver { | ||
| constructor(private readonly marketplaceService: MarketplaceService) {} | ||
| private readonly logger = new Logger(MarketplaceResolver.name); | ||
| private hasSyncedOnce = false; |
There was a problem hiding this comment.
Instance-level flag causes duplicate syncs in multi-pod deployments
private hasSyncedOnce = false is stored in the resolver instance. In a horizontally scaled deployment with multiple Node processes or pods, each instance maintains its own copy of this flag and will independently trigger the initial catalog sync when findManyMarketplaceApps() is called, potentially causing duplicate upsert races and wasted work.
The existing cron job (MarketplaceCatalogSyncCronJob) is the intended synchronization mechanism; this in-resolver sync should either be removed or replaced with a persisted flag (e.g., a database row or feature flag) to prevent duplicate work across instances.
There was a problem hiding this comment.
Moved the flag assignment to after successful sync. Re: multi-pod — the sync is an idempotent upsert, so duplicate syncs in a scale-out scenario are harmless (just redundant). Adding Redis/DB coordination would be overengineering for a lazy-init optimization that only runs once per cold start.
There was a problem hiding this comment.
Further improved — replaced the boolean flag with a timestamp-based cooldown (5 minutes). This allows retries after transient failures while still avoiding spam. Each pod converges independently via idempotent upserts.
| private computeLockKey( | ||
| workspaceId: string, | ||
| universalIdentifier: string, | ||
| ): number { | ||
| const combined = `${workspaceId}:${universalIdentifier}`; | ||
| let hash = 0; | ||
|
|
||
| for (let i = 0; i < combined.length; i++) { | ||
| const char = combined.charCodeAt(i); | ||
|
|
||
| hash = (hash << 5) - hash + char; | ||
| hash |= 0; | ||
| } | ||
|
|
||
| return hash; |
There was a problem hiding this comment.
Advisory lock key collision risk
computeLockKey produces a 32-bit hash via hash |= 0, but PostgreSQL's pg_advisory_xact_lock() accepts a 64-bit integer. With a 32-bit key space and many potential (workspaceId, universalIdentifier) pairs, hash collisions become likely, causing unrelated install operations to block each other unnecessarily.
Consider using pg_advisory_xact_lock(bigint, bigint) with separate hash values for each component, or implement a proper advisory lock using the full 64-bit space to significantly reduce collision probability.
There was a problem hiding this comment.
Acknowledged — the 32-bit key space is unlikely to cause real collisions in practice (we'd need many concurrent installs of different apps in the same workspace), but I'll track this as a follow-up improvement to use a proper 64-bit hash.
There was a problem hiding this comment.
Now fixed — computeLockKeys returns two separate INT4 hashes (one per input), and we use pg_advisory_xact_lock(key1, key2) for a 64-bit key space.
| } else { | ||
| appRegistration = this.appRegistrationRepository.create({ | ||
| universalIdentifier, | ||
| name: manifest.application?.displayName ?? 'Unknown App', | ||
| sourceType: AppRegistrationSourceType.NONE, | ||
| oAuthClientId: v4(), | ||
| oAuthRedirectUris: [], | ||
| oAuthScopes: [], | ||
| workspaceId, | ||
| }); | ||
|
|
||
| appRegistration = | ||
| await this.appRegistrationRepository.save(appRegistration); | ||
| } |
There was a problem hiding this comment.
Data consistency issue: registration saved before file upload completes
When creating a new registration for a tarball upload, the entity is initially saved with sourceType: AppRegistrationSourceType.NONE (line 142), and then updated to TARBALL afterward (line 159). If any exception occurs between the save and the update calls (e.g., file storage failure at line 151–156), the registration will remain permanently in state NONE, creating an inconsistent record.
Consider setting sourceType: AppRegistrationSourceType.TARBALL directly on the new entity before the first save call, or wrapping both the save and file write in a transaction to ensure atomicity.
There was a problem hiding this comment.
Fixed — new registrations are now created with sourceType: AppRegistrationSourceType.TARBALL directly, eliminating the TOCTOU window where an orphaned NONE record could persist.
| const hasUpdate = | ||
| latestVersion !== undefined && | ||
| application.version !== undefined && | ||
| latestVersion !== application.version; |
There was a problem hiding this comment.
Update badge uses inequality instead of semver comparison
The update badge is shown whenever latestVersion !== application.version (lines 80–83). However, the PR description states the intent is to show the badge "when a newer version is available." This means if latestAvailableVersion is older than the installed version (e.g., after a rollback or stale sync), the badge will incorrectly appear. Use proper semantic versioning comparison to only show the badge when latestVersion > application.version.
There was a problem hiding this comment.
Good catch — for the initial marketplace launch, the simple inequality is intentional since versions should only go up. A proper semver comparison with semver.gt() would be more correct; I'll track this as a follow-up.
There was a problem hiding this comment.
Now fixed — added isNewerSemver() utility that does proper major.minor.patch comparison. The update badge only shows when the latest version is strictly greater.
|
|
||
| const execFilePromise = promisify(execFile); | ||
|
|
||
| const APP_RESOLVER_TMPDIR = join(tmpdir(), 'twenty-app-resolver'); |
There was a problem hiding this comment.
Shared temp directory never cleaned up
APP_RESOLVER_TMPDIR is evaluated once at module load and becomes a shared root directory. While each call creates a unique workDir subdirectory (line 73) that is cleaned up after use (line 140–141), the parent directory /tmp/twenty-app-resolver itself is never removed. Over time, especially after server crashes or OOM kills that interrupt cleanup, orphaned subdirectories will accumulate on the host filesystem.
Consider cleaning up stale work directories on startup, or using OS-managed temp files with automatic expiration.
There was a problem hiding this comment.
The parent /tmp/twenty-app-resolver directory is empty after each operation completes (all subdirectories are cleaned up). The OS cleans /tmp on reboot. Adding explicit cleanup of the parent would be a minor improvement but not a real issue.
There was a problem hiding this comment.
Now fixed — AppPackageResolverService implements OnModuleInit and wipes APP_RESOLVER_TMPDIR on startup, clearing any stale temp files from previous server runs.
1. SDK push: fix endpoint URL (/api/app-registrations/) and send
base64 JSON body instead of multipart/form-data
2. Security: skip symlinks in collectFiles to prevent path traversal
3. Security: use resolve()+startsWith() instead of includes('..')
for tarball path traversal protection
4. Multi-tenant: scope upsertRegistration by workspaceId
5. Data consistency: set sourceType=TARBALL on creation instead of
saving as NONE then updating
6. Correctness: move hasSyncedOnce after successful sync
7. Correctness: URL-encode scoped npm package names in registry URLs
8. Error handling: wrap JSON.parse in try-catch for manifest parsing
9. Security: remove filesystem paths from error messages
10. Robustness: add default branch to REST exception filter
11. UX: gate modal close on install success
12. Robustness: add default case to mapSourceType
13. UX: improve userFriendlyMessage for package resolution failure
Made-with: Cursor
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/core-modules/application/services/application-install.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/application-install.service.ts:225">
P2: Avoid a silent `default` fallback in `mapSourceType`; it masks unsupported source types by coercing them to `'local'` instead of surfacing an error.
(Based on your team's feedback about avoiding silent fallbacks for enum-based mappings.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...es/twenty-server/src/engine/core-modules/application/services/application-install.service.ts
Outdated
Show resolved
Hide resolved
1. Advisory lock: use pg_advisory_xact_lock(key1, key2) with two separate INT4 hashes for 64-bit key space instead of single 32-bit 2. Update badge: use proper semver comparison (isNewerSemver) instead of simple inequality check, preventing false positives on downgrades 3. Catalog sync: replace boolean hasSyncedOnce with timestamp-based cooldown (5min), enabling retries after transient failures 4. Yarn engine: dynamically resolve yarn path from .yarnrc.yml instead of hardcoding the version in the filename 5. Temp directory: clean up APP_RESOLVER_TMPDIR on module init to remove stale files from previous server runs Made-with: Cursor
…ervice Made-with: Cursor
…ntity The per-registration registry URL override was never set by any code path — all npm operations already fall back to the server-level APP_REGISTRY_URL config. Remove the column, entity field, and migration to avoid unnecessary schema complexity. Can be re-added later if per-app private registries become a real use case. Made-with: Cursor
|
Too many files changed for review. ( |
FelixMalfait
force-pushed
the
feat/app-distribution-npm-tarball
branch
from
6edebfd to
7768a9e
Compare
...erver/src/engine/core-modules/application/application-install/app-package-fetcher.service.ts
Outdated
Show resolved
Hide resolved
|
Recreating PR on a fresh branch to fix stuck GitHub Actions CI |
FelixMalfait
force-pushed
the
feat/app-distribution-npm-tarball
branch
from
84894bd to
21c76ba
Compare
...erver/src/engine/core-modules/application/application-install/app-package-fetcher.service.ts
Outdated
Show resolved
Hide resolved
- Merge ApplicationQueryResolver into ApplicationInstallResolver (queries, token renewal, install/uninstall in one resolver) - ApplicationModule stays as pure service module (no resolver deps) because PermissionsModule already imports ApplicationModule, making it impossible for ApplicationModule to import PermissionsModule - Add PermissionsModule import to ApplicationDevelopmentModule - Set sourcePath to 'oauth-install' for OAuth auto-installed apps Made-with: Cursor
FelixMalfait
force-pushed
the
feat/app-distribution-npm-tarball
branch
from
21c76ba to
9ec69c2
Compare
…-npm-tarball Resolve conflicts: - auth.module.ts: keep reorganized import paths - 1-18 upgrade commands: keep our import paths - graphql.ts: merge both branches' additions - FileStorageService: adapt to removed legacy methods by using FileStorageDriverFactory directly for system-level tarball operations Made-with: Cursor
"Resolver" has a strong GraphQL connotation in this codebase. This service fetches and extracts NPM/tarball packages, not GraphQL queries. Made-with: Cursor
- Filter out npm packages without twenty-uid keyword (would fail on uuid column with package names like @scope/name) - Return packageDir instead of workDir for NPM installs so collectFiles finds the actual app files instead of skipping them via the node_modules exclusion - Prevent duplicate sync job enqueue when marketplace is empty - Handle findOrCreateNpmRegistration race with try/catch fallback - Enforce NPM source type check in installMarketplaceApp - Don't leak raw error details to client in upgrade failures - Rename AppPackageResolverService to AppPackageFetcherService Made-with: Cursor
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/core-modules/application/application-marketplace/services/marketplace-query.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/application-marketplace/services/marketplace-query.service.ts:47">
P1: Set the sync-enqueue guard only after the enqueue operation succeeds; currently a transient enqueue failure can permanently suppress future retries in this process.
(Based on your team's feedback about setting sync/retry guard flags only after successful completion.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/twenty-server/src/engine/core-modules/application/application-marketplace/services/marketplace-query.service.ts:114">
P2: Avoid catching all save errors as a not-found exception; rethrow non-concurrency failures so real infrastructure/data errors are not hidden.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (registrations.length === 0) { | ||
| if (!this.hasSyncBeenEnqueued) { | ||
| this.hasSyncBeenEnqueued = true; |
There was a problem hiding this comment.
P1: Set the sync-enqueue guard only after the enqueue operation succeeds; currently a transient enqueue failure can permanently suppress future retries in this process.
(Based on your team's feedback about setting sync/retry guard flags only after successful completion.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/application/application-marketplace/services/marketplace-query.service.ts, line 47:
<comment>Set the sync-enqueue guard only after the enqueue operation succeeds; currently a transient enqueue failure can permanently suppress future retries in this process.
(Based on your team's feedback about setting sync/retry guard flags only after successful completion.) </comment>
<file context>
@@ -42,13 +43,16 @@ export class MarketplaceQueryService {
- {},
- );
+ if (!this.hasSyncBeenEnqueued) {
+ this.hasSyncBeenEnqueued = true;
+ this.logger.log(
+ 'No marketplace registrations found, enqueuing one-time sync job',
</file context>
| } catch { | ||
| const concurrentlyCreated = await this.appRegistrationRepository.findOne({ | ||
| where: { sourcePackage: params.packageName }, | ||
| }); | ||
|
|
||
| if (isDefined(concurrentlyCreated)) { | ||
| return concurrentlyCreated; | ||
| } | ||
|
|
||
| throw new ApplicationRegistrationException( | ||
| `Failed to create registration for package "${params.packageName}"`, | ||
| ApplicationRegistrationExceptionCode.APPLICATION_REGISTRATION_NOT_FOUND, | ||
| ); | ||
| } |
There was a problem hiding this comment.
P2: Avoid catching all save errors as a not-found exception; rethrow non-concurrency failures so real infrastructure/data errors are not hidden.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/application/application-marketplace/services/marketplace-query.service.ts, line 114:
<comment>Avoid catching all save errors as a not-found exception; rethrow non-concurrency failures so real infrastructure/data errors are not hidden.</comment>
<file context>
@@ -94,18 +98,33 @@ export class MarketplaceQueryService {
+ });
+
+ return await this.appRegistrationRepository.save(registration);
+ } catch {
+ const concurrentlyCreated = await this.appRegistrationRepository.findOne({
+ where: { sourcePackage: params.packageName },
</file context>
| } catch { | |
| const concurrentlyCreated = await this.appRegistrationRepository.findOne({ | |
| where: { sourcePackage: params.packageName }, | |
| }); | |
| if (isDefined(concurrentlyCreated)) { | |
| return concurrentlyCreated; | |
| } | |
| throw new ApplicationRegistrationException( | |
| `Failed to create registration for package "${params.packageName}"`, | |
| ApplicationRegistrationExceptionCode.APPLICATION_REGISTRATION_NOT_FOUND, | |
| ); | |
| } | |
| } catch (error) { | |
| const concurrentlyCreated = await this.appRegistrationRepository.findOne({ | |
| where: { sourcePackage: params.packageName }, | |
| }); | |
| if (isDefined(concurrentlyCreated)) { | |
| return concurrentlyCreated; | |
| } | |
| throw error; | |
| } |
- Remove isClosable cast to true by using discriminated union props and conditional rendering (no spread, no cast) - Move file input reset to finally block in tarball upload modal - Extract shared appPack public operation to deduplicate build+pack logic between app:pack and app:push CLI commands Made-with: Cursor
...ore-modules/application/application-marketplace/services/marketplace-catalog-sync.service.ts
Show resolved
Hide resolved
…hip checks - Add FeatureFlagGuard to @UseGuards in all 4 application resolvers (install, development, marketplace, registration) so that @RequireFeatureFlag actually enforces the feature flag at runtime - Add @RequireFeatureFlag to marketplace and registration resolver methods that were missing it - Import FeatureFlagModule in all 4 host modules - Add assertValidNpmPackageName() to reject path traversal and injection via sourcePackage (used in fetcher and marketplace query) - Add ownership check in resolveApplicationRegistrationId to prevent cross-workspace IDOR in app:dev flow - Move syncVariableSchemas inside ownership check block Made-with: Cursor
Made-with: Cursor
findManyApplications and findOneApplication are basic read queries needed for core workspace functionality. They should not be gated behind IS_APPLICATION_ENABLED, which should only protect mutations (install, uninstall, sync, marketplace operations). Made-with: Cursor
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
3 participants
Summary
.tar.gztarballs, withAppRegistrationSourceTypetracking the originAppUpgradeServicechecks for newer versions, supports rollback for npm-sourced apps, and a cron job runs every 6 hours to updatelatestAvailableVersionon registrationsenableScripts: falsein.yarnrc.ymldisables all lifecycle scripts duringyarn installto prevent RCEApplicationRegistrationentitiesBackend changes
ApplicationRegistrationEntity(sourceType, sourcePackage, latestAvailableVersion),ApplicationEntity(applicationRegistrationId), migrationAppPackageResolverService,ApplicationInstallService,AppUpgradeService,MarketplaceCatalogSyncServiceMarketplaceCatalogSyncCronJob(hourly),AppVersionCheckCronJob(every 6h)AppRegistrationUploadController— tarball upload with secure extractionMarketplaceResolver— simplifiedinstallMarketplaceApp(removed redundantsourcePackagearg).yarnrc.yml—enableScripts: falseto block postinstall RCEFrontend changes
SettingsInstallNpmAppModal,SettingsUploadTarballModal,SettingsAppModalLayoutuseUploadAppTarball,useInstallMarketplaceApp(cleaned up)SettingsApplicationVersionContainer,SettingsApplicationDetailAboutTabSettingsApplicationTableRow— blue "Update" tag,SettingsApplicationsInstalledTab— fetches registrations for version comparisonTest plan
.tar.gztarball via the "Upload tarball" modallatestAvailableVersion > versionapp-distribution.integration-spec.ts,marketplace-catalog-sync.integration-spec.tsenableScripts: falseblocks postinstall scripts during yarn installMade with Cursor