From 0b817e3bc0e749f35c905e5e38bdc0a4a4e775d7 Mon Sep 17 00:00:00 2001 From: Paul Rastoin <45004772+prastoin@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:15:31 +0200 Subject: [PATCH] Restrict which workspace fields can be updated before activation (#23781) ## What `validateWorkspaceUpdatePermissions` returned early with no checks at all when the workspace was in `PENDING_CREATION`, so `updateWorkspace` accepted any field during that window. It now allows only the fields needed to set the workspace up (`displayName`, `subdomain`, `logo`) and rejects everything else until the workspace is activated. Note that `updateWorkspace`'s resolver guard is `CustomPermissionGuard`, which always returns true and only documents that the check lives in the resolver/service, so this service method is the actual enforcement point. ## Why A workspace stays in `PENDING_CREATION` from signup until onboarding completes, and the JWT strategy issues an authenticated context for it without resolving member permissions. During that window every field was writable with no permission check, including security relevant ones such as `allowImpersonation`, `isTwoFactorAuthenticationEnforced` and `isPublicInviteLinkEnabled`. In practice the only principal present before activation is the workspace creator, who is granted the Admin role (`canUpdateAllSettings`) the moment activation completes, so there is no privilege escalation over another user today. This is defense in depth: the early return was broader than it needed to be, and it becomes a real gap if the "only the creator exists before activation" assumption ever stops holding, for example a workspace left pending or a future flow that adds members before activation. The bypass exists because a pending workspace has no roles yet, so permissions cannot be resolved for it. Keeping a small explicit allowlist preserves that while removing the blanket skip. ## Scope Only the `updateWorkspace` path. `SettingsPermissionGuard` has a similar bypass for `PENDING_CREATION` / `ONGOING_CREATION`, but it covers 62 resolvers including billing endpoints that onboarding legitimately calls before activation, so narrowing it needs its own analysis and is deliberately left out. ## Tests `workspace-update-before-activation.integration-spec.ts`, run against a real database with the seeded workspace flipped to `PENDING_CREATION`: - a security sensitive field (`allowImpersonation`) is rejected and the stored value is unchanged - mixing a setup field with a security sensitive one rejects the whole update, and `displayName` is not persisted - setup fields (`displayName`) still apply, so the restriction does not break workspace setup Both rejection tests were verified to fail when the old blanket early return is put back, while the positive control keeps passing. The existing `settings-permissions/workspace*` suites still pass (38 tests total), confirming no change for activated workspaces. --- _Generated by [Claude Code](https://claude.ai/code/session_01Qf58T7Lm7PazNbS3UwaZPd)_ Review in cubic --- ...ds-updatable-before-activation.constant.ts | 7 ++ .../workspace/services/workspace.service.ts | 29 ++++- ...before-activation.integration-spec.ts.snap | 25 ++++ ...date-before-activation.integration-spec.ts | 116 ++++++++++++++++++ ...update-workspace-operation-factory.util.ts | 18 +++ .../graphql/utils/update-workspace.util.ts | 42 +++++++ 6 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant.ts create mode 100644 packages/twenty-server/test/integration/graphql/suites/settings-permissions/__snapshots__/workspace-update-before-activation.integration-spec.ts.snap create mode 100644 packages/twenty-server/test/integration/graphql/suites/settings-permissions/workspace-update-before-activation.integration-spec.ts create mode 100644 packages/twenty-server/test/integration/graphql/utils/update-workspace-operation-factory.util.ts create mode 100644 packages/twenty-server/test/integration/graphql/utils/update-workspace.util.ts diff --git a/packages/twenty-server/src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant.ts b/packages/twenty-server/src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant.ts new file mode 100644 index 0000000000..541a355854 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant.ts @@ -0,0 +1,7 @@ +import { type WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity'; + +export const WORKSPACE_FIELDS_UPDATABLE_BEFORE_ACTIVATION = { + displayName: true, + subdomain: true, + logo: true, +} as const satisfies Partial>; diff --git a/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts b/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts index fa0ab9bdbc..b4f3388b05 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts @@ -49,6 +49,7 @@ import { UpgradeSequenceReaderService } from 'src/engine/core-modules/upgrade/se import { UserWorkspaceEntity } from 'src/engine/core-modules/user-workspace/user-workspace.entity'; import { UserWorkspaceService } from 'src/engine/core-modules/user-workspace/user-workspace.service'; import { UserEntity } from 'src/engine/core-modules/user/user.entity'; +import { WORKSPACE_FIELDS_UPDATABLE_BEFORE_ACTIVATION } from 'src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant'; import { WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity'; import { WorkspaceException, @@ -818,12 +819,6 @@ export class WorkspaceService { apiKey: ApiKeyEntity | undefined; workspaceActivationStatus: WorkspaceActivationStatus; }) { - if ( - workspaceActivationStatus === WorkspaceActivationStatus.PENDING_CREATION - ) { - return; - } - const systemFields = new Set(['id', 'createdAt', 'updatedAt', 'deletedAt']); const fieldsBeingUpdated = Object.keys(payload).filter( @@ -834,6 +829,28 @@ export class WorkspaceService { return; } + if ( + workspaceActivationStatus === WorkspaceActivationStatus.PENDING_CREATION + ) { + const fieldsRequiringActivation = fieldsBeingUpdated.filter( + (field) => !(field in WORKSPACE_FIELDS_UPDATABLE_BEFORE_ACTIVATION), + ); + + if (fieldsRequiringActivation.length > 0) { + const fieldsList = fieldsRequiringActivation.join(', '); + + throw new PermissionsException( + PermissionsExceptionMessage.PERMISSION_DENIED, + PermissionsExceptionCode.PERMISSION_DENIED, + { + userFriendlyMessage: msg`These fields cannot be updated before the workspace is activated: ${fieldsList}.`, + }, + ); + } + + return; + } + if (!userWorkspaceId) { throw new Error('Missing userWorkspaceId in authContext'); } diff --git a/packages/twenty-server/test/integration/graphql/suites/settings-permissions/__snapshots__/workspace-update-before-activation.integration-spec.ts.snap b/packages/twenty-server/test/integration/graphql/suites/settings-permissions/__snapshots__/workspace-update-before-activation.integration-spec.ts.snap new file mode 100644 index 0000000000..4db86ada6c --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/settings-permissions/__snapshots__/workspace-update-before-activation.integration-spec.ts.snap @@ -0,0 +1,25 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`updateWorkspace while the workspace is pending creation rejects security sensitive fields 1`] = ` +{ + "extensions": { + "code": "FORBIDDEN", + "subCode": "PERMISSION_DENIED", + "userFriendlyMessage": "User does not have permission.", + }, + "message": "Entity performing the request does not have permission", + "name": "ForbiddenError", +} +`; + +exports[`updateWorkspace while the workspace is pending creation rejects the whole update when a setup field is mixed with a security sensitive one 1`] = ` +{ + "extensions": { + "code": "FORBIDDEN", + "subCode": "PERMISSION_DENIED", + "userFriendlyMessage": "User does not have permission.", + }, + "message": "Entity performing the request does not have permission", + "name": "ForbiddenError", +} +`; diff --git a/packages/twenty-server/test/integration/graphql/suites/settings-permissions/workspace-update-before-activation.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/settings-permissions/workspace-update-before-activation.integration-spec.ts new file mode 100644 index 0000000000..6f5f2a8a32 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/suites/settings-permissions/workspace-update-before-activation.integration-spec.ts @@ -0,0 +1,116 @@ +import { expectOneNotInternalServerErrorSnapshot } from 'test/integration/graphql/utils/expect-one-not-internal-server-error-snapshot.util'; +import { updateWorkspace } from 'test/integration/graphql/utils/update-workspace.util'; +import { WorkspaceActivationStatus } from 'twenty-shared/workspace'; + +import { WORKSPACE_FIELDS_UPDATABLE_BEFORE_ACTIVATION } from 'src/engine/core-modules/workspace/constants/workspace-fields-updatable-before-activation.constant'; +import { type UpdateWorkspaceInput } from 'src/engine/core-modules/workspace/dtos/update-workspace-input'; +import { SEED_APPLE_WORKSPACE_ID } from 'src/engine/workspace-manager/dev-seeder/core/constants/seeder-workspaces.constant'; + +// Every allowlisted field needs a value here, so adding one to the constant +// without covering it fails to compile. +const SETUP_FIELD_VALUES = { + displayName: `Pending setup ${Date.now()}`, + subdomain: `pending-setup-${Date.now()}`, + logo: 'pending-setup-logo.png', +} satisfies Record< + keyof typeof WORKSPACE_FIELDS_UPDATABLE_BEFORE_ACTIVATION, + string +>; + +const setActivationStatus = (activationStatus: WorkspaceActivationStatus) => + testDataSource.query( + 'UPDATE core.workspace SET "activationStatus" = $1 WHERE id = $2', + [activationStatus, SEED_APPLE_WORKSPACE_ID], + ); + +describe('updateWorkspace while the workspace is pending creation', () => { + let originalDisplayName: string; + let originalSubdomain: string; + let originalLogo: string | null; + let originalAllowImpersonation: boolean; + + beforeAll(async () => { + const [workspace] = await testDataSource.query( + 'SELECT "displayName", "subdomain", "logo", "allowImpersonation" FROM core.workspace WHERE id = $1', + [SEED_APPLE_WORKSPACE_ID], + ); + + originalDisplayName = workspace.displayName; + originalSubdomain = workspace.subdomain; + originalLogo = workspace.logo; + originalAllowImpersonation = workspace.allowImpersonation; + + await setActivationStatus(WorkspaceActivationStatus.PENDING_CREATION); + }); + + afterAll(async () => { + await setActivationStatus(WorkspaceActivationStatus.ACTIVE); + + await testDataSource.query( + 'UPDATE core.workspace SET "displayName" = $1, "subdomain" = $2, "logo" = $3 WHERE id = $4', + [ + originalDisplayName, + originalSubdomain, + originalLogo, + SEED_APPLE_WORKSPACE_ID, + ], + ); + }); + + it('rejects security sensitive fields', async () => { + const { data, errors } = await updateWorkspace({ + data: { allowImpersonation: !originalAllowImpersonation }, + expectToFail: true, + }); + + expect(data?.updateWorkspace).toBeFalsy(); + expectOneNotInternalServerErrorSnapshot({ errors }); + + const [workspace] = await testDataSource.query( + 'SELECT "allowImpersonation" FROM core.workspace WHERE id = $1', + [SEED_APPLE_WORKSPACE_ID], + ); + + expect(workspace.allowImpersonation).toBe(originalAllowImpersonation); + }); + + it('rejects the whole update when a setup field is mixed with a security sensitive one', async () => { + const { data, errors } = await updateWorkspace({ + data: { + displayName: 'Should not be applied', + isPublicInviteLinkEnabled: true, + }, + expectToFail: true, + }); + + expect(data?.updateWorkspace).toBeFalsy(); + expectOneNotInternalServerErrorSnapshot({ errors }); + + const [workspace] = await testDataSource.query( + 'SELECT "displayName" FROM core.workspace WHERE id = $1', + [SEED_APPLE_WORKSPACE_ID], + ); + + expect(workspace.displayName).toBe(originalDisplayName); + }); + + it.each(Object.entries(SETUP_FIELD_VALUES))( + 'still allows %s, which is needed to set the workspace up', + async (field, value) => { + const { data, errors } = await updateWorkspace({ + data: { [field]: value } as UpdateWorkspaceInput, + expectToFail: false, + }); + + expect(errors).toBeUndefined(); + expect(data.updateWorkspace.id).toBe(SEED_APPLE_WORKSPACE_ID); + + const [workspace] = await testDataSource.query( + `SELECT "${field}" FROM core.workspace WHERE id = $1`, + [SEED_APPLE_WORKSPACE_ID], + ); + + expect(workspace[field]).toBe(value); + }, + ); +}); diff --git a/packages/twenty-server/test/integration/graphql/utils/update-workspace-operation-factory.util.ts b/packages/twenty-server/test/integration/graphql/utils/update-workspace-operation-factory.util.ts new file mode 100644 index 0000000000..27b71ab65f --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/utils/update-workspace-operation-factory.util.ts @@ -0,0 +1,18 @@ +import gql from 'graphql-tag'; + +import { type UpdateWorkspaceInput } from 'src/engine/core-modules/workspace/dtos/update-workspace-input'; + +export const updateWorkspaceOperationFactory = ({ + data, +}: { + data: UpdateWorkspaceInput; +}) => ({ + query: gql` + mutation UpdateWorkspace($data: UpdateWorkspaceInput!) { + updateWorkspace(data: $data) { + id + } + } + `, + variables: { data }, +}); diff --git a/packages/twenty-server/test/integration/graphql/utils/update-workspace.util.ts b/packages/twenty-server/test/integration/graphql/utils/update-workspace.util.ts new file mode 100644 index 0000000000..7406e7a190 --- /dev/null +++ b/packages/twenty-server/test/integration/graphql/utils/update-workspace.util.ts @@ -0,0 +1,42 @@ +import { updateWorkspaceOperationFactory } from 'test/integration/graphql/utils/update-workspace-operation-factory.util'; +import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util'; +import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type'; +import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util'; +import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util'; + +import { type UpdateWorkspaceInput } from 'src/engine/core-modules/workspace/dtos/update-workspace-input'; + +type UpdateWorkspaceUtilArgs = { + data: UpdateWorkspaceInput; + expectToFail?: boolean; + token?: string; +}; + +export const updateWorkspace = async ({ + data, + expectToFail = false, + token, +}: UpdateWorkspaceUtilArgs): CommonResponseBody<{ + updateWorkspace: { id: string }; +}> => { + const response = await makeMetadataAPIRequest( + updateWorkspaceOperationFactory({ data }), + token, + ); + + if (expectToFail === true) { + warnIfNoErrorButExpectedToFail({ + response, + errorMessage: 'updateWorkspace should have failed but did not', + }); + } + + if (expectToFail === false) { + warnIfErrorButNotExpectedToFail({ + response, + errorMessage: 'updateWorkspace has failed but should not', + }); + } + + return { data: response.body.data, errors: response.body.errors }; +};