Encrypt/decrypt app secret variables by ijreilly · Pull Request #17394 · twentyhq/twenty

ijreilly · 2026-01-23T10:57:43Z

Closes twentyhq/core-team-issues#1724
From PR #15283, followed same implementation

Introduce EnvironmentModule to provide type safety for env-only variables
Encrypt secret variables
When querying app secret variable, display up to first 5 characters (depending on secret length) then ******

greptile-apps · 2026-01-23T11:03:06Z

Greptile Overview

Greptile Summary

introduces encryption for application variables marked as secrets, using AES-256-CTR with a random IV. The implementation follows the existing pattern from PR #15283, creating a centralized SecretEncryptionService and consolidating ConfigModule setup into a new EnvironmentModule.

Key changes:

Created SecretEncryptionService that wraps encryption/decryption with silent error fallback behavior
Application variables with the isSecret flag are now encrypted at rest in the database
When querying secrets via GraphQL, partial masking shows first 0-5 characters followed by ********
Serverless functions receive decrypted plaintext values via the buildEnvVar utility
Refactored module dependencies to use the new EnvironmentModule as single global source for environment configuration

Issues found:

In upsertManyApplicationVariableEntities at line 119, existing variables are updated but their value field is not included in the update, meaning values cannot be changed once created via this method
The SecretEncryptionService silently falls back to returning original values when encryption fails, which could lead to data being stored in plaintext without detection

Confidence Score: 3/5

This PR has solid implementation but contains a critical bug in the upsert logic and a concerning silent failure mode
The encryption implementation is sound and well-tested, following established patterns. However, there's a logic bug where existing variable values cannot be updated via upsertManyApplicationVariableEntities, and the silent fallback behavior when encryption fails could lead to secrets being stored unencrypted without error detection
Pay close attention to packages/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts (lines 111-128) for the upsert bug, and packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts (lines 28-38 and 46-56) for the silent fallback behavior

Important Files Changed

Filename	Overview
packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts	introduces new encryption service with silent fallback behavior when encryption fails
packages/twenty-server/src/engine/core-modules/environment/environment.module.ts	centralizes ConfigModule setup as global module for environment variables
packages/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts	adds encryption for secret variables and masking logic, but `upsertManyApplicationVariableEntities` doesn't update existing variable values
packages/twenty-server/src/engine/core-modules/serverless/drivers/utils/build-env-var.ts	decrypts secret variables when building environment variables for serverless functions
packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts	refactored to use `SecretEncryptionService` instead of direct encryption function calls

Sequence Diagram

sequenceDiagram
    participant Client
    participant Resolver as ApplicationVariableEntityResolver
    participant Service as ApplicationVariableEntityService
    participant Encryption as SecretEncryptionService
    participant EnvDriver as EnvironmentConfigDriver
    participant AuthUtil as auth.util
    participant DB as Database

    Note over Client,DB: Update Application Variable Flow
    Client->>Resolver: updateOneApplicationVariable(key, value, appId)
    Resolver->>Service: update({key, plainTextValue, appId, workspaceId})
    Service->>DB: findOne({key, appId})
    DB-->>Service: existingVariable (with isSecret flag)
    
    alt isSecret = true
        Service->>Encryption: encrypt(plainTextValue)
        Encryption->>EnvDriver: get('APP_SECRET')
        EnvDriver-->>Encryption: appSecret
        Encryption->>AuthUtil: encryptText(value, appSecret)
        AuthUtil-->>Encryption: encryptedValue
        Encryption-->>Service: encryptedValue
        Service->>DB: update({key, appId}, {value: encryptedValue})
    else isSecret = false
        Service->>DB: update({key, appId}, {value: plainTextValue})
    end
    
    Service->>DB: invalidateAndRecompute cache
    Service-->>Resolver: success
    Resolver-->>Client: true

    Note over Client,DB: Query Application Variable Flow (with masking)
    Client->>Resolver: query applicationVariable
    DB-->>Resolver: variable (encrypted if isSecret)
    Resolver->>Service: getDisplayValue(variable)
    
    alt isSecret = false
        Service-->>Resolver: plainValue
    else isSecret = true
        Service->>Encryption: decrypt(variable.value)
        Encryption->>EnvDriver: get('APP_SECRET')
        EnvDriver-->>Encryption: appSecret
        Encryption->>AuthUtil: decryptText(value, appSecret)
        AuthUtil-->>Encryption: decryptedValue
        Encryption-->>Service: decryptedValue
        Service->>Service: calculate visibleChars = min(5, floor(length/10))
        Service->>Service: mask = decrypted.slice(0, visibleChars) + "********"
        Service-->>Resolver: maskedValue
    end
    
    Resolver-->>Client: displayValue

    Note over Client,DB: Serverless Function Execution Flow
    participant SLS as ServerlessFunctionService
    participant BuildEnv as buildEnvVar
    
    SLS->>DB: fetch flatApplicationVariables
    DB-->>SLS: variables (encrypted if isSecret)
    SLS->>BuildEnv: buildEnvVar(variables, encryptionService)
    
    loop for each variable
        alt isSecret = true
            BuildEnv->>Encryption: decrypt(variable.value)
            Encryption-->>BuildEnv: decryptedValue
        else isSecret = false
            BuildEnv->>BuildEnv: use value as-is
        end
    end
    
    BuildEnv-->>SLS: envVars (all decrypted)
    SLS->>SLS: execute function with plaintext env vars

greptile-apps

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

...es/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts

greptile-apps · 2026-01-23T11:03:11Z

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

+    try {
+      const appSecret = this.getAppSecret();
+
+      return encryptText(value, appSecret);
+    } catch (error) {
+      this.logger.debug(
+        `Encryption failed: ${error.message}. Using original value.`,
+      );
+
+      return value;
+    }


silently returns the original value when encryption fails. If APP_SECRET is missing or invalid, this could lead to secrets being stored unencrypted without the caller knowing. Consider throwing an error or logging at a higher level than debug.

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts Line: 28:38 Comment: silently returns the original value when encryption fails. If `APP_SECRET` is missing or invalid, this could lead to secrets being stored unencrypted without the caller knowing. Consider throwing an error or logging at a higher level than debug. How can I resolve this? If you propose a fix, please make it concise.

github-actions · 2026-01-23T11:09:12Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:50978

This environment will automatically shut down when the PR is closed or after 5 hours.

ijreilly · 2026-01-23T13:16:34Z

packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts

-
-        return convertedValue;
-      }
+      return isDecrypt


I dont understand why we had that try / catch behaviour before, swallowing the encryption/decryption error with a fallback which seems problematic to me - if we mean to encrypt a value but leave it decrypted, we will have errors when trying to decrypt again, won't we ? And getting the encrypted value when we try to fetch the decrypted value does not help either - if i understood correctly whats happening

cubic-dev-ai

2 issues found across 19 files

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/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts">

<violation number="1" location="packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts:25">
P1: Encryption failures should not silently fall back to storing plaintext; this can leave secrets unencrypted at rest if APP_SECRET is misconfigured. Consider failing hard so callers can handle the error.</violation>
</file>

<file name="packages/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts">

<violation number="1" location="packages/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts:125">
P2: When updating an existing variable, the value isn’t updated/encrypted. If isSecret becomes true, the stored plaintext value will later be decrypted and fail. Include value: encryptedValue in the update payload.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

...es/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

...erver/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts

-  encryptText: jest.fn((text) => `encrypted:${text}`),
-  decryptText: jest.fn((text) => text.replace('encrypted:', '')),
+  encryptText: jest.fn((text) => `${text}`),
+  decryptText: jest.fn((text) => text.replace('', '')),


In general, this kind of problem is fixed by removing or correcting the ineffective string replacement. If the goal is to transform the string (e.g., strip certain characters, unescape sequences), the search and replacement arguments to replace must be adjusted so the replacement is not identical to the matched substring. If the goal is simply to return the input unchanged, the replace call should be removed and the argument returned directly.

Here, the mock implementation lives in config-storage.service.spec.ts:

jest.mock('src/engine/core-modules/auth/auth.util', () => ({ encryptText: jest.fn((text) => `${text}`), decryptText: jest.fn((text) => text.replace('', '')), }));

The intended behavior for decryptText in tests is almost certainly to be an identity function, just like encryptText. The best fix, preserving existing test behavior but removing the problematic pattern, is to change the implementation of decryptText to simply return its input. That can be done by replacing text.replace('', '') with text, or, for consistency with encryptText, with a template literal such as `${text}`. No new imports or helper methods are required; it's a one-line change in this file only.

Concretely:

Edit line 27 in packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts.

Replace decryptText: jest.fn((text) => text.replace('', '')), with decryptText: jest.fn((text) => text), (or the template literal variant).

This preserves the identity behavior while removing the ineffective replacement.

Weiko · 2026-01-27T11:01:18Z

Seems we removed the save button and it saves on blur, regardless of that we should make sure that if tmr we add more editable stuff in the application settings, we don't send the "encrypted" value that is returned by the backend. Can we flag that somewhere?

Weiko

Left some comments but overall LGTM! @ijreilly

Weiko · 2026-01-27T10:23:52Z

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

+
+      return encryptText(value, appSecret);
+    } catch (error) {
+      this.logger.debug(


I think we should not swallow here as this is not expected to happen. If this fails we will save non-encrypted value and we won't know (this should not happen but still)

totally agree, i have changed the behavior in config-storage (see my comment - would like some validation on this btw :)) but did not think to do here too

Ah yes perfect 👍 @ijreilly

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

Weiko · 2026-01-27T10:28:13Z

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts

+import { EnvironmentConfigDriver } from 'src/engine/core-modules/twenty-config/drivers/environment-config.driver';
+
+@Injectable()
+export class SecretEncryptionService {


I know I originally introduced this service in my PR but I'm wondering if it's bringing much value 🤔

Open to discussion

kept it and added decryptAndMask in it

...es/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts

packages/twenty-server/src/engine/core-modules/serverless/drivers/utils/build-env-var.ts

Weiko

LGTM

twenty-eng-sync · 2026-01-27T16:12:03Z

Hey @ijreilly! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

twenty-eng-sync · 2026-01-27T16:12:05Z

Hey @ijreilly! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

implementation

583c3c3

greptile-apps bot reviewed Jan 23, 2026

View reviewed changes

ijreilly added 2 commits January 23, 2026 14:13

adapt test

bd24943

revert failure handling

b179bd3

ijreilly commented Jan 23, 2026

View reviewed changes

cubic-dev-ai bot reviewed Jan 23, 2026

View reviewed changes

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts Show resolved Hide resolved

...es/twenty-server/src/engine/core-modules/applicationVariable/application-variable.service.ts Show resolved Hide resolved

sentry bot reviewed Jan 23, 2026

View reviewed changes

packages/twenty-server/src/engine/core-modules/secret-encryption/secret-encryption.service.ts Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jan 23, 2026

View reviewed changes

Weiko reviewed Jan 27, 2026

View reviewed changes

fixes

bb1b123

sentry bot reviewed Jan 27, 2026

View reviewed changes

packages/twenty-server/src/engine/core-modules/serverless/drivers/utils/build-env-var.ts Show resolved Hide resolved

Weiko approved these changes Jan 27, 2026

View reviewed changes

fix tests

da34963

ijreilly enabled auto-merge January 27, 2026 15:50

ijreilly added this pull request to the merge queue Jan 27, 2026

Merged via the queue into main with commit 7e3d9cd Jan 27, 2026
59 of 60 checks passed

ijreilly deleted the ft--app-secret-var branch January 27, 2026 16:11

@@ -24,7 +24,7 @@
             jest.mock('src/engine/core-modules/auth/auth.util', () => ({
               encryptText: jest.fn((text) => `${text}`),
-              decryptText: jest.fn((text) => text.replace('', '')),
+              decryptText: jest.fn((text) => text),
             }));
             describe('ConfigStorageService', () => {

Conversation

ijreilly commented Jan 23, 2026

Uh oh!

greptile-apps bot commented Jan 23, 2026

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijreilly Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Copilot Autofix

Weiko commented Jan 27, 2026

Uh oh!

Weiko left a comment

Choose a reason for hiding this comment

Uh oh!

Weiko Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

ijreilly Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Weiko Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Weiko Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Weiko Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

ijreilly Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Weiko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

twenty-eng-sync bot commented Jan 27, 2026

Uh oh!

twenty-eng-sync bot commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Jan 23, 2026 •

edited

Loading