Stage

[evereq]

PR

• Have you followed the contributing guidelines?
• Have you explained what your changes do, and why they add value?

Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.

---

---

Summary by cubic

Introduces a full plugin marketplace with subscriptions, user access control, and a multi‑step installation flow tracked by installationId. Also improves desktop UI/state, download reliability, and streamlines CI builds.

• New Features

  • Plugin marketplace domain and tables; expanded contracts (PluginScope, setting data types).
  • Subscription access: assign/revoke users, scoped subscriptions (user/organization/tenant), pending/trial treated as active for access.
  • Multi‑step installation: server registration returns installationId, new IPC channel (plugin::complete::installation), per‑plugin operation states, optimistic toggles.
  • Settings management: grouped, validated plugin settings with CRUD and dialog‑driven effects.
  • User management: infinite scroll directive, paginated assignments, dedicated Akita stores/effects/facades.
  • Safer CDN downloads with streaming, size limits, timeouts; plugin soft delete and paginated APIs.

• Migration

  • Run server DB migration 1766045577607-CreatePluginMarketplaceTables.
  • Build and run the desktop app to apply the local migration adding installationId to the plugins table.
  • If your client relies on organization context, ensure the organization-id header is sent (RequestContext supports it).
  • CI/Docker: NODE_OPTIONS lowered to 30000; GitHub runners switched to ubicloud-standard-8 (and -8-arm). Adjust custom pipelines if they assume previous memory/runner sizes.

^{Written for commit 28628e2. Summary will update automatically on new commits.}

In general, to fix Zip Slip issues you must never trust archive entry paths directly. Instead, compute the target path with path.join/path.resolve, normalize it, and then verify that the resulting path is still under the intended extraction directory. Reject or skip any entry whose resolved path does not start with the normalized extraction root. This check must be done on the final path passed to fs operations like createWriteStream.

For this specific code, the best fix without changing existing functionality is:

• After joining extractDir and entryPath to form fullPath, compute its normalized absolute path (e.g., path.resolve(extractDir, entryPath)).
• Compute the normalized absolute path of extractDir once.
• Check that the resolved file path starts with the normalized extraction directory path (taking care to include the path separator to avoid prefix tricks, e.g., /tmp/out vs /tmp/outside).
• If the check fails, log a warning, drain the entry, and skip writing.
• Otherwise, proceed to create directories as needed (if that is already handled elsewhere) and write the file.

Because we can’t rely on the (unshown) isSafePath implementation, the simplest and most robust approach is to move the safety check to use the resolved fullPath directly in this method. Concretely, we will:

• Replace the call to this.isSafePath(extractDir, entryPath) with an inline safety check using path.resolve and a normalized root.
• Compute const fullPath = path.resolve(extractDir, entryPath); (using resolve instead of a separate join).
• Verify that fullPath starts with the normalized extractDir path (e.g., extractRootWithSep) and skip the entry if not.
• Maintain all existing logging, size checks, and behavior for valid entries.

All changes occur within unzip in packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts; no new imports are needed because path is already imported.

In general, the fix is to ensure that any path taken from a ZIP entry is validated and normalized before being used for file system operations. We should reject archive paths that attempt directory traversal (e.g., containing .. segments), are absolute, or that end up resolving outside the intended extraction directory after normalization. We should base all file system writes on a validated path, not on the raw entry.path.

The best targeted fix here is:

1. Add a private helper method isSafePath(root: string, entryPath: string): boolean (and optionally a getSafePath helper) to CdnDownloadStrategy in this file. This method should:
   • Use path.isAbsolute(entryPath) to reject absolute paths.
   • Use path.normalize(entryPath) and reject anything containing .. segments or path separators at the start that would move above the root.
   • Build fullPath = path.join(root, entryPath) and then compare path.resolve(fullPath) to path.resolve(root) to ensure fullPath is still inside root (e.g., startsWith(rootResolved + path.sep) or equals rootResolved).
2. Use that helper when processing ZIP entries:
   • Maintain the existing check if (!this.isSafePath(extractPath, filePath)) { ... return; }.
   • After passing this check, compute fullPath in a way consistent with the validation (using the same normalized path used or implied by isSafePath).
3. Ensure that we still allow directories and files to be created within extractPath as before, so existing functionality remains the same except that malicious paths are skipped.

Because the snippet already calls this.isSafePath, we will implement that method within the CdnDownloadStrategy class (no external dependencies needed beyond Node’s built-in path module, which is already imported). We will also slightly adjust the fullPath computation to use path.resolve(extractPath, filePath) to align with the validation logic, which makes the relationship between the sanitized path and the write operation obvious.

In general, to fix externally-controlled format string issues with console.log/console.error/util.format, you should avoid putting untrusted data directly in the format string position. Instead, use a constant format string with %s placeholders (or string concatenation) and pass the untrusted data as subsequent arguments, or log a structured object.

For this specific case in plugin-subscription-plan.service.ts, we should remove the template literal interpolation of pluginId and instead either: (a) use a constant format string with %s and pass pluginId as an argument, or (b) concatenate strings so the first argument no longer contains % from untrusted data. Since the code already passes a second argument (error), the safest and most idiomatic fix is to keep using multiple arguments but make the first one a fixed string and move the plugin ID into an additional argument. This preserves logging behavior (message + error object) while eliminating the possibility that pluginId changes how error is formatted.

Concretely, in packages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.ts, within the isSubscriptionRequired method’s catch block, change:

console.error(`Error checking if plugin ${pluginId} requires subscription:`, error);

to something like:

console.error('Error checking if plugin requires subscription for pluginId:', pluginId, error);

Now the first argument is a static string; pluginId is a separate argument and cannot affect formatting. No new imports or helper methods are needed, and this single change addresses all alert variants because they all flow into this same sink.

[greptile-apps]

Skipped: This PR changes more files than the configured file change limit: (717 files found, 100 file limit)

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch develop

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​types/​fs-extra@​5.0.2
	bcryptjs@​3.0.2

View full report

[cubic-dev-ai]

10 issues found across 717 files

Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name=".cspell.json">

<violation number="1" location=".cspell.json:805">
P0: Missing comma after `&quot;Liskov&quot;` creates invalid JSON. This will cause the cspell configuration file to fail parsing.</violation>
</file>

<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-user-assignment.effects.ts">

<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-user-assignment.effects.ts:110">
P1: Incrementing skip before the HTTP request completes causes pagination inconsistency. If `switchMap` cancels the request (due to rapid successive calls), skip will be incremented but data won&#39;t be loaded. Move `incrementSkip()` to the success handler, or use `exhaustMap` instead of `switchMap` to prevent request cancellation.</violation>
</file>

<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-marketplace.effect.ts">

<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-marketplace.effect.ts:109">
P2: Debug `console.log` statement should be removed before merging to production code.</violation>
</file>

<file name="packages/desktop-lib/src/lib/plugin-system/data-access/plugin-manager.ts">

<violation number="1" location="packages/desktop-lib/src/lib/plugin-system/data-access/plugin-manager.ts:109">
P2: Missing null coalescing for `installationId` - other optional fields (`marketplaceId`, `versionId`) use `? ... : null` pattern to ensure consistent database storage. Consider applying the same pattern here.</violation>
</file>

<file name="packages/core/src/lib/core/context/request-context.ts">

<violation number="1" location="packages/core/src/lib/core/context/request-context.ts:195">
P1: Security concern: This method falls back to reading `organization-id` from untrusted HTTP headers when the user&#39;s employee organization ID is not available. Unlike similar methods (`currentTenantId`, `currentUserId`, `currentRoleId`) which only read from the authenticated user object, this pattern allows clients to potentially spoof organization context by setting the header. If this value is used for authorization or data filtering, it could lead to unauthorized cross-organization access. Consider validating the header value against the user&#39;s allowed organizations or removing the header fallback entirely.</violation>
</file>

<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-category.effect.ts">

<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-category.effect.ts:35">
P1: Stale state bug: `state` is captured before `incrementPage()` is called, so `state.pagination.page` will have the old page number, not the incremented value. This will cause the same page to be fetched repeatedly.</violation>
</file>

<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts">

<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts:297">
P1: Missing `pluginId` in `serverInstallationFailed` action dispatch. The `handleServerInstallationFailure$` effect checks `if (pluginId)` before cleanup and rollback, so without it, state won&#39;t be reset and `revertFailedInstallation` won&#39;t be called.</violation>

<violation number="2" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts:353">
P1: Missing `pluginId` in `installationFailed` action dispatch. The `handleInstallationFailure$` effect checks `if (pluginId)` before cleanup, so without it, installation state won&#39;t be properly reset.</violation>

<violation number="3" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts:388">
P1: Missing `marketplaceId` in `activationFailed` action dispatch. The `handleActivationFailure$` effect checks `if (pluginId)` before cleanup, so without it, state won&#39;t be properly reset. Note: `executeActivation$` correctly passes it, but this effect does not.</violation>
</file>

<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/available-users.effects.ts">

<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/available-users.effects.ts:174">
P1: Missing `{ dispatch: false }` on effect. This effect only performs side effects via `tap()` and doesn&#39;t dispatch a new action. Following the pattern in plugin-settings.effects.ts, add `{ dispatch: false }` as the second argument to `createEffect()`.</violation>
</file>

_{Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR}

[cubic-dev-ai]

P0: Missing comma after "Liskov" creates invalid JSON. This will cause the cspell configuration file to fail parsing.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .cspell.json, line 805:

<comment>Missing comma after `&quot;Liskov&quot;` creates invalid JSON. This will cause the cspell configuration file to fail parsing.</comment>

<file context>
@@ -658,6 +658,151 @@
+		&quot;prepopulate&quot;,
+		&quot;prepopulates&quot;,
+		&quot;prepopulation&quot;,
+		&quot;Liskov&quot;
 		&quot;Zrdm&quot;
 	],
</file context>

Suggested change

	"Liskov"
	"Liskov",

[cubic-dev-ai]

P1: Incrementing skip before the HTTP request completes causes pagination inconsistency. If switchMap cancels the request (due to rapid successive calls), skip will be incremented but data won't be loaded. Move incrementSkip() to the success handler, or use exhaustMap instead of switchMap to prevent request cancellation.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-user-assignment.effects.ts, line 110:

<comment>Incrementing skip before the HTTP request completes causes pagination inconsistency. If `switchMap` cancels the request (due to rapid successive calls), skip will be incremented but data won&#39;t be loaded. Move `incrementSkip()` to the success handler, or use `exhaustMap` instead of `switchMap` to prevent request cancellation.</comment>

<file context>
@@ -0,0 +1,794 @@
+				ofType(PluginUserAssignmentActions.loadMoreAssignments),
+				tap(() =&gt; {
+					this.store.setLoadingMore(true);
+					this.store.incrementSkip();
+				}),
+				switchMap(({ pluginId }) =&gt; {
</file context>

[cubic-dev-ai]

P2: Debug console.log statement should be removed before merging to production code.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-marketplace.effect.ts, line 109:

<comment>Debug `console.log` statement should be removed before merging to production code.</comment>

<file context>
@@ -58,14 +105,30 @@ export class PluginMarketplaceEffects {
-						}))
-					),
+					tap((response) =&gt; {
+						console.log(&#39;API Response:&#39;, response); // Debug log
+						const items = Array.isArray(response?.items)
+							? response.items
</file context>

[cubic-dev-ai]

P2: Missing null coalescing for installationId - other optional fields (marketplaceId, versionId) use ? ... : null pattern to ensure consistent database storage. Consider applying the same pattern here.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/plugin-system/data-access/plugin-manager.ts, line 109:

<comment>Missing null coalescing for `installationId` - other optional fields (`marketplaceId`, `versionId`) use `? ... : null` pattern to ensure consistent database storage. Consider applying the same pattern here.</comment>

<file context>
@@ -100,6 +106,7 @@ export class PluginManager implements IPluginManager {
 				version: pluginMetadata.version,
 				description: pluginMetadata.description,
 				main: pluginMetadata.main,
+				installationId: pluginMetadata.installationId,
 				marketplaceId: pluginMetadata.marketplaceId ? pluginMetadata.marketplaceId : null,
 				versionId: pluginMetadata.versionId ? pluginMetadata.versionId : null,
</file context>

[cubic-dev-ai]

P1: Security concern: This method falls back to reading organization-id from untrusted HTTP headers when the user's employee organization ID is not available. Unlike similar methods (currentTenantId, currentUserId, currentRoleId) which only read from the authenticated user object, this pattern allows clients to potentially spoof organization context by setting the header. If this value is used for authorization or data filtering, it could lead to unauthorized cross-organization access. Consider validating the header value against the user's allowed organizations or removing the header fallback entirely.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/core/context/request-context.ts, line 195:

<comment>Security concern: This method falls back to reading `organization-id` from untrusted HTTP headers when the user&#39;s employee organization ID is not available. Unlike similar methods (`currentTenantId`, `currentUserId`, `currentRoleId`) which only read from the authenticated user object, this pattern allows clients to potentially spoof organization context by setting the header. If this value is used for authorization or data filtering, it could lead to unauthorized cross-organization access. Consider validating the header value against the user&#39;s allowed organizations or removing the header fallback entirely.</comment>

<file context>
@@ -182,6 +182,21 @@ export class RequestContext {
+		const user: IUser | null = RequestContext.currentUser();
+		return (
+			user?.employee?.organizationId ||
+			(RequestContext.currentRequest()?.headers[&#39;organization-id&#39;] as ID) ||
+			null
+		);
</file context>

[cubic-dev-ai]

P1: Stale state bug: state is captured before incrementPage() is called, so state.pagination.page will have the old page number, not the incremented value. This will cause the same page to be fetched repeatedly.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-category.effect.ts, line 35:

<comment>Stale state bug: `state` is captured before `incrementPage()` is called, so `state.pagination.page` will have the old page number, not the incremented value. This will cause the same page to be fetched repeatedly.</comment>

<file context>
@@ -0,0 +1,576 @@
+			}),
+			switchMap(({ params = {} }) =&gt; {
+				const state = this.pluginCategoryStore.getValue();
+				const paginationParams = {
+					...(params as object),
+					page: state.pagination.page,
</file context>

[cubic-dev-ai]

P1: Missing marketplaceId in activationFailed action dispatch. The handleActivationFailure$ effect checks if (pluginId) before cleanup, so without it, state won't be properly reset. Note: executeActivation$ correctly passes it, but this effect does not.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts, line 388:

<comment>Missing `marketplaceId` in `activationFailed` action dispatch. The `handleActivationFailure$` effect checks `if (pluginId)` before cleanup, so without it, state won&#39;t be properly reset. Note: `executeActivation$` correctly passes it, but this effect does not.</comment>

<file context>
@@ -22,167 +47,743 @@ export class PluginInstallationEffects {
+						),
+						catchError((error) =&gt;
+							of(
+								PluginInstallationActions.activationFailed(
+									error?.message || &#39;Failed to find installed plugin&#39;
+								)
</file context>

Suggested change

	PluginInstallationActions.activationFailed(
	error?.message || 'Failed to find installed plugin'
	PluginInstallationActions.activationFailed(
	error?.message || 'Failed to find installed plugin',
	marketplaceId

[cubic-dev-ai]

P1: Missing pluginId in installationFailed action dispatch. The handleInstallationFailure$ effect checks if (pluginId) before cleanup, so without it, installation state won't be properly reset.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts, line 353:

<comment>Missing `pluginId` in `installationFailed` action dispatch. The `handleInstallationFailure$` effect checks `if (pluginId)` before cleanup, so without it, installation state won&#39;t be properly reset.</comment>

<file context>
@@ -22,167 +47,743 @@ export class PluginInstallationEffects {
-						catchError((error) =&gt; this.handleError(error))
+						catchError((error) =&gt;
+							of(
+								PluginInstallationActions.installationFailed(
+									error?.message ||
+										this.translateService.instant(&#39;PLUGIN.TOASTR.ERROR.COMPLETE_INSTALLATION&#39;)
</file context>

Suggested change

	PluginInstallationActions.installationFailed(
	error?.message ||
	this.translateService.instant('PLUGIN.TOASTR.ERROR.COMPLETE_INSTALLATION')
	PluginInstallationActions.installationFailed(
	error?.message ||
	this.translateService.instant('PLUGIN.TOASTR.ERROR.COMPLETE_INSTALLATION'),
	marketplaceId

[cubic-dev-ai]

P1: Missing pluginId in serverInstallationFailed action dispatch. The handleServerInstallationFailure$ effect checks if (pluginId) before cleanup and rollback, so without it, state won't be reset and revertFailedInstallation won't be called.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-installation.effect.ts, line 297:

<comment>Missing `pluginId` in `serverInstallationFailed` action dispatch. The `handleServerInstallationFailure$` effect checks `if (pluginId)` before cleanup and rollback, so without it, state won&#39;t be reset and `revertFailedInstallation` won&#39;t be called.</comment>

<file context>
@@ -22,167 +47,743 @@ export class PluginInstallationEffects {
+						finalize(() =&gt; this.pluginInstallationStore.setServerInstalling(pluginId, false)),
+						catchError((error) =&gt;
+							of(
+								PluginInstallationActions.serverInstallationFailed(
+									error?.message || &#39;Server installation failed&#39;
+								)
</file context>

Suggested change

	PluginInstallationActions.serverInstallationFailed(
	error?.message || 'Server installation failed'
	PluginInstallationActions.serverInstallationFailed(
	error?.message || 'Server installation failed',
	pluginId

[cubic-dev-ai]

P1: Missing { dispatch: false } on effect. This effect only performs side effects via tap() and doesn't dispatch a new action. Following the pattern in plugin-settings.effects.ts, add { dispatch: false } as the second argument to createEffect().

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/available-users.effects.ts, line 174:

<comment>Missing `{ dispatch: false }` on effect. This effect only performs side effects via `tap()` and doesn&#39;t dispatch a new action. Following the pattern in plugin-settings.effects.ts, add `{ dispatch: false }` as the second argument to `createEffect()`.</comment>

<file context>
@@ -0,0 +1,402 @@
+	 * Handle load users success
+	 * Updates store with loaded users
+	 */
+	handleLoadUsersSuccess$ = createEffect(() =&gt; {
+		return this.actions$.pipe(
+			ofType(AvailableUsersActions.loadUsersSuccess),
</file context>