RJ Add quota feature change

[rajreddy2345]

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

• PR title and summary are descriptive. (conventions)
• Docs updated or follow-up ticket created.
• Tests included.
• PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ cstuncsik
✅ valya
✅ despairblue
✅ krynble
❌ Valya Bullions

---

Valya Bullions 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.}

[aikido-pr-checks]

60 Open source vulnerabilities detected - critical severity
Aikido detected 60 vulnerabilities across 43 packages, it includes 6 critical, 23 high, 20 medium and 11 low vulnerabilities.

Remediation Aikido suggests bumping the vulnerable packages to a safe version.
^{View details in Aikido Security}

[cubic-dev-ai]

14 issues found across 277 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="packages/cli/src/commands/mfa/disable.ts">

<violation number="1" location="packages/cli/src/commands/mfa/disable.ts:43">
P2: Misleading error message: When MFA is already disabled, calling `reportUserDoesNotExistError()` tells the user they don&#39;t exist, which is incorrect. The user exists but MFA is not enabled. Consider creating a separate method like `reportMfaAlreadyDisabled()` or logging an appropriate message.</violation>
</file>

<file name="packages/cli/src/UserManagement/PermissionChecker.ts">

<violation number="1" location="packages/cli/src/UserManagement/PermissionChecker.ts:21">
P3: Grammar error: &quot;apart of&quot; should be &quot;a part of&quot; (two words). &quot;Apart&quot; means &quot;separate from&quot;, while &quot;a part of&quot; means &quot;belonging to&quot;.</violation>

<violation number="2" location="packages/cli/src/UserManagement/PermissionChecker.ts:31">
P3: Typo in variable name: `accessable` should be `accessible`.</violation>
</file>

<file name="packages/cli/src/CredentialsHelper.ts">

<violation number="1" location="packages/cli/src/CredentialsHelper.ts:459">
P2: Caching `false` values won&#39;t work here. The `CacheService.set()` method has a guard `if (!key || !value) return;` that prevents storing falsy values. When `refreshFn` returns `false` (credential cannot use external secrets), it won&#39;t be cached, causing repeated database queries on every call. Consider returning an object like `{ canUse: false }` instead, or modify the cache key pattern to only cache positive results.</violation>
</file>

<file name="packages/cli/src/commands/import/workflow.ts">

<violation number="1" location="packages/cli/src/commands/import/workflow.ts:143">
P2: Incorrect error message: says &quot;credential&quot; but should say &quot;workflow&quot;. This could confuse users when the error is displayed.</violation>
</file>

<file name="packages/cli/src/InternalHooks.ts">

<violation number="1" location="packages/cli/src/InternalHooks.ts:235">
P2: Hardcoding `sharing_role: &#39;owner&#39;` will produce inaccurate telemetry data. Users who are actually &quot;sharees&quot; (workflow was shared with them) will be incorrectly reported as &quot;owners&quot;. The TODO comment suggests this is placeholder code - consider either keeping the original role-fetching logic, or if intentionally deferring, use `undefined` or `null` instead of an incorrect hardcoded value to avoid misleading analytics.</violation>
</file>

<file name="packages/cli/src/controllers/e2e.controller.ts">

<violation number="1" location="packages/cli/src/controllers/e2e.controller.ts:96">
P2: `numericFeatures` is not reset in `resetFeatures()`. Quota values set via `setQuota()` will persist across E2E test resets, causing test pollution. Consider adding logic to reset numeric features to their default values.</violation>
</file>

<file name="packages/cli/src/commands/import/credentials.ts">

<violation number="1" location="packages/cli/src/commands/import/credentials.ts:174">
P1: When `--userId` is provided (without `--projectId`), the comparison `ownerProject.id !== projectId` compares against `undefined` since `projectId` is the raw flag value. This causes ALL existing credentials to fail the ownership check, even those already owned by the specified user. The comparison should use the resolved project ID from `getProject()` instead.</violation>

<violation number="2" location="packages/cli/src/commands/import/credentials.ts:257">
P1: When a credential is owned by a team project (non-personal), `getCredentialOwner()` returns an empty object, causing `checkRelations()` to skip validation entirely via `if (!ownerProject) continue`. This allows team-project-owned credentials to be re-owned without any checks. The function should return `{ project: sharedCredential.project }` for non-personal projects.</violation>
</file>

<file name="cypress/e2e/39-projects.cy.ts">

<violation number="1" location="cypress/e2e/39-projects.cy.ts:21">
P1: `should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. The `have.length` assertion requires a numeric argument. Use `should(&#39;not.exist&#39;)` (as used elsewhere in the codebase) or `should(&#39;have.length&#39;, 0)` to assert no elements exist.</violation>

<violation number="2" location="cypress/e2e/39-projects.cy.ts:34">
P1: `should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. Use `should(&#39;not.exist&#39;)` to assert no credential cards exist.</violation>

<violation number="3" location="cypress/e2e/39-projects.cy.ts:56">
P1: `should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. Use `should(&#39;not.exist&#39;)` to assert no menu items exist.</violation>
</file>

<file name="packages/cli/src/credentials/credentials.controller.ts">

<violation number="1" location="packages/cli/src/credentials/credentials.controller.ts:279">
P2: Calling `In(toUnshare)` when `toUnshare` might be empty can cause invalid SQL or unexpected behavior. Consider adding a guard to only execute the delete when there are projects to unshare.</violation>
</file>

<file name="packages/@n8n/permissions/src/combineScopes.ts">

<violation number="1" location="packages/@n8n/permissions/src/combineScopes.ts:11">
P2: Rule violated: **Prefer Typeguards over Type casting**

Type assertion used for narrowing. `Object.fromEntries()` returns `{ [k: string]: unknown }` and is cast to `GlobalScopes | ScopeLevels`. Consider using a type guard function or a typed helper that properly preserves the type through the transformation. Alternatively, since the variable already has a type annotation, you could create an assertion function to validate the structure.</violation>
</file>

---

Since this is your first cubic review, here's how it works:

• cubic automatically reviews your code and comments on bugs and improvements
• Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
• Ask questions if you need clarification on any suggestion

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

[cubic-dev-ai]

P2: Misleading error message: When MFA is already disabled, calling reportUserDoesNotExistError() tells the user they don't exist, which is incorrect. The user exists but MFA is not enabled. Consider creating a separate method like reportMfaAlreadyDisabled() or logging an appropriate message.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/mfa/disable.ts, line 43:

<comment>Misleading error message: When MFA is already disabled, calling `reportUserDoesNotExistError()` tells the user they don&#39;t exist, which is incorrect. The user exists but MFA is not enabled. Consider creating a separate method like `reportMfaAlreadyDisabled()` or logging an appropriate message.</comment>

<file context>
@@ -27,16 +27,27 @@ export class DisableMFACommand extends BaseCommand {
+			user.mfaRecoveryCodes.length === 0 &amp;&amp;
+			!user.mfaEnabled
+		) {
+			this.reportUserDoesNotExistError(flags.email);
+			return;
+		}
</file context>

Suggested change

	this.reportUserDoesNotExistError(flags.email);
	this.logger.info(`MFA is already disabled for user with email: ${flags.email}`);

[cubic-dev-ai]

P3: Grammar error: "apart of" should be "a part of" (two words). "Apart" means "separate from", while "a part of" means "belonging to".

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/UserManagement/PermissionChecker.ts, line 21:

<comment>Grammar error: &quot;apart of&quot; should be &quot;a part of&quot; (two words). &quot;Apart&quot; means &quot;separate from&quot;, while &quot;a part of&quot; means &quot;belonging to&quot;.</comment>

<file context>
@@ -5,64 +5,40 @@ import { CredentialAccessError, NodeOperationError, WorkflowOperationError } fro
 
 	/**
-	 * Check if a user is permitted to execute a workflow.
+	 * Check if a workflow has the ability to execute based on the projects it&#39;s apart of.
 	 */
-	async check(workflowId: string, userId: string, nodes: INode[]) {
</file context>

Suggested change

	* Check if a workflow has the ability to execute based on the projects it's apart of.
	* Check if a workflow has the ability to execute based on the projects it's a part of.

[cubic-dev-ai]

P3: Typo in variable name: accessable should be accessible.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/UserManagement/PermissionChecker.ts, line 31:

<comment>Typo in variable name: `accessable` should be `accessible`.</comment>

<file context>
@@ -5,64 +5,40 @@ import { CredentialAccessError, NodeOperationError, WorkflowOperationError } fro
-		// all creds accessible to users who have access to this workflow
-
-		let workflowUserIds = [userId];
+		const accessable = await this.sharedCredentialsRepository.getFilteredAccessibleCredentials(
+			projectIds,
+			workflowCredIds,
</file context>

[cubic-dev-ai]

P2: Caching false values won't work here. The CacheService.set() method has a guard if (!key || !value) return; that prevents storing falsy values. When refreshFn returns false (credential cannot use external secrets), it won't be cached, causing repeated database queries on every call. Consider returning an object like { canUse: false } instead, or modify the cache key pattern to only cache positive results.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/CredentialsHelper.ts, line 459:

<comment>Caching `false` values won&#39;t work here. The `CacheService.set()` method has a guard `if (!key || !value) return;` that prevents storing falsy values. When `refreshFn` returns `false` (credential cannot use external secrets), it won&#39;t be cached, causing repeated database queries on every call. Consider returning an object like `{ canUse: false }` instead, or modify the cache key pattern to only cache positive results.</comment>

<file context>
@@ -457,28 +451,39 @@ export class CredentialsHelper extends ICredentialsHelper {
-				},
-			},
-		});
+		return (
+			(await this.cacheService.get(`credential-can-use-secrets:${nodeCredential.id}`, {
+				refreshFn: async () =&gt; {
</file context>

[cubic-dev-ai]

P2: Incorrect error message: says "credential" but should say "workflow". This could confuse users when the error is displayed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/import/workflow.ts, line 143:

<comment>Incorrect error message: says &quot;credential&quot; but should say &quot;workflow&quot;. This could confuse users when the error is displayed.</comment>

<file context>
@@ -108,15 +121,26 @@ export class ImportWorkflowsCommand extends BaseCommand {
 				return {
 					success: false as const,
-					message: `The credential with id &quot;${workflow.id}&quot; is already owned by the user with the id &quot;${ownerId}&quot;. It can&#39;t be re-owned by the user with the id &quot;${userId}&quot;`,
+					message: `The credential with ID &quot;${workflow.id}&quot; is already owned by ${currentOwner}. It can&#39;t be re-owned by ${newOwner}.`,
 				};
 			}
</file context>

Suggested change

	message: `The credential with ID "${workflow.id}" is already owned by ${currentOwner}. It can't be re-owned by ${newOwner}.`,
	message: `The workflow with ID "${workflow.id}" is already owned by ${currentOwner}. It can't be re-owned by ${newOwner}.`,

[cubic-dev-ai]

P1: should('not.have.length') is not a valid Cypress assertion. Use should('not.exist') to assert no menu items exist.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At cypress/e2e/39-projects.cy.ts, line 56:

<comment>`should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. Use `should(&#39;not.exist&#39;)` to assert no menu items exist.</comment>

<file context>
@@ -0,0 +1,148 @@
+		projects.getProjectTabWorkflows().click();
+		workflowsPage.getters.workflowCards().should(&#39;have.length&#39;, 1);
+
+		projects.getMenuItems().should(&#39;not.have.length&#39;);
+
+		cy.intercept(&#39;POST&#39;, &#39;/rest/projects&#39;).as(&#39;projectCreate&#39;);
</file context>

[cubic-dev-ai]

P1: should('not.have.length') is not a valid Cypress assertion. Use should('not.exist') to assert no credential cards exist.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At cypress/e2e/39-projects.cy.ts, line 34:

<comment>`should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. Use `should(&#39;not.exist&#39;)` to assert no credential cards exist.</comment>

<file context>
@@ -0,0 +1,148 @@
+
+		projects.getHomeButton().click();
+		projects.getProjectTabCredentials().click();
+		credentialsPage.getters.credentialCards().should(&#39;not.have.length&#39;);
+
+		credentialsPage.getters.emptyListCreateCredentialButton().click();
</file context>

[cubic-dev-ai]

P1: should('not.have.length') is not a valid Cypress assertion. The have.length assertion requires a numeric argument. Use should('not.exist') (as used elsewhere in the codebase) or should('have.length', 0) to assert no elements exist.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At cypress/e2e/39-projects.cy.ts, line 21:

<comment>`should(&#39;not.have.length&#39;)` is not a valid Cypress assertion. The `have.length` assertion requires a numeric argument. Use `should(&#39;not.exist&#39;)` (as used elsewhere in the codebase) or `should(&#39;have.length&#39;, 0)` to assert no elements exist.</comment>

<file context>
@@ -0,0 +1,148 @@
+		cy.signin(INSTANCE_ADMIN);
+		cy.visit(workflowsPage.url);
+		projects.getProjectTabs().should(&#39;have.length&#39;, 2);
+		workflowsPage.getters.workflowCards().should(&#39;not.have.length&#39;);
+
+		workflowsPage.getters.newWorkflowButtonCard().click();
</file context>

[cubic-dev-ai]

P2: Calling In(toUnshare) when toUnshare might be empty can cause invalid SQL or unexpected behavior. Consider adding a guard to only execute the delete when there are projects to unshare.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/credentials/credentials.controller.ts, line 279:

<comment>Calling `In(toUnshare)` when `toUnshare` might be empty can cause invalid SQL or unexpected behavior. Consider adding a guard to only execute the delete when there are projects to unshare.</comment>

<file context>
@@ -321,59 +248,45 @@ export class CredentialsController {
+				[currentPersonalProjectIDs, (id) =&gt; id],
+			);
+
+			const deleteResult = await trx.delete(SharedCredentials, {
+				credentialsId: credentialId,
+				projectId: In(toUnshare),
</file context>

Suggested change

	const deleteResult = await trx.delete(SharedCredentials, {
	credentialsId: credentialId,
	projectId: In(toUnshare),
	});
	let deleteResult: { affected?: number | null } = { affected: 0 };
	if (toUnshare.length > 0) {
	deleteResult = await trx.delete(SharedCredentials, {
	credentialsId: credentialId,
	projectId: In(toUnshare),
	});
	}

[cubic-dev-ai]

P2: Rule violated: Prefer Typeguards over Type casting

Type assertion used for narrowing. Object.fromEntries() returns { [k: string]: unknown } and is cast to GlobalScopes | ScopeLevels. Consider using a type guard function or a typed helper that properly preserves the type through the transformation. Alternatively, since the variable already has a type annotation, you could create an assertion function to validate the structure.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/@n8n/permissions/src/combineScopes.ts, line 11:

<comment>Type assertion used for narrowing. `Object.fromEntries()` returns `{ [k: string]: unknown }` and is cast to `GlobalScopes | ScopeLevels`. Consider using a type guard function or a typed helper that properly preserves the type through the transformation. Alternatively, since the variable already has a type annotation, you could create an assertion function to validate the structure.</comment>

<file context>
@@ -0,0 +1,23 @@
+): Set&lt;Scope&gt; {
+	const maskedScopes: GlobalScopes | ScopeLevels = Object.fromEntries(
+		Object.entries(userScopes).map((e) =&gt; [e[0], [...e[1]]]),
+	) as GlobalScopes | ScopeLevels;
+
+	if (masks?.sharing) {
</file context>