Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cookie session #3039

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

fix: cookie session #3039

wants to merge 11 commits into from

Conversation

liorzam
Copy link
Collaborator

@liorzam liorzam commented Feb 8, 2025

Summary by CodeRabbit

  • New Features
    • Enhanced session management with new environment variables for improved cookie configuration and security settings.
    • Streamlined user authentication by reducing the exposure of personal data during session serialization.
  • Chores
    • Updated environment configuration to include additional session settings for more flexible session control.

Copy link

changeset-bot bot commented Feb 8, 2025

⚠️ No Changeset found

Latest commit: e54d476

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Feb 8, 2025

Walkthrough

This pull request updates the workflows service by introducing new session-related environment variables and corresponding schema validations for enhanced session configuration. The cookie session configuration in the main application file is refactored into a modular object with conditional proxy trust settings. Additionally, user serialization in the session serializer has been streamlined by omitting extra properties, retaining only the essential fields.

Changes

File(s) Change Summary
services/workflows-service/.env.example
services/workflows-service/src/env.ts
Added new session environment variables (SESSION_HTTP_ONLY, SESSION_SAME_SITE, SESSION_SECURE_COOKIE, SESSION_SECURE_PROXY) with defaults and transformations; updated HASHING_KEY_SECRET_BASE64 value in the sample file and extended the serverEnvSchema to support these variables.
services/workflows-service/src/main.ts Refactored inline cookie session settings into a cookieSessionConfig object; added a conditional check to enable proxy trust based on the SESSION_SECURE_PROXY variable and logged the configuration for debugging.
services/workflows-service/src/auth/session-serializer.ts Modified the serializeUser method to exclude email, firstName, and lastName, returning a streamlined user object with only id and expires.
apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuBadge.tsx
apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuSubButton.tsx
apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.Provider.tsx
Made minor adjustments to the order of class names in the CSS class strings of the components without altering their functionality.

Sequence Diagram(s)

sequenceDiagram
    participant Main as Main.ts
    participant Config as CookieSessionConfig
    participant HTTP as HTTP Adapter
    participant Middleware as Session Middleware

    Main->>Config: Build session config using env vars
    Config-->>Main: Return cookieSessionConfig
    Main->>HTTP: Check SESSION_SECURE_PROXY variable
    HTTP-->>Main: Set trust proxy if enabled
    Main->>Middleware: Apply cookieSessionConfig for session handling
Loading
sequenceDiagram
    participant Auth as Authentication Middleware
    participant Serializer as SessionSerializer
    participant UserSvc as User Service

    Auth->>Serializer: Call serializeUser(user)
    Serializer->>UserSvc: Retrieve user details
    UserSvc-->>Serializer: Return full user object
    Serializer->>Auth: Return serialized user (id, expires only)
Loading

Possibly related PRs

  • fix: base64 for hashing key #2429: Adjusts the HASHING_KEY_SECRET_BASE64 environment variable across configuration files, which aligns with the changes made in this pull request.

Suggested reviewers

  • alonp99
  • tomer-shvadron

Poem

Hop along in lines of code,
New sessions bloom in every node.
Configs refined with care and art,
Stripping details, keeping core at heart.
CodeRabbit leaps—bug woes depart! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5cfc198 and e54d476.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuBadge.tsx (1 hunks)
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuSubButton.tsx (1 hunks)
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.Provider.tsx (1 hunks)
  • services/workflows-service/.env.example (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.Provider.tsx
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuBadge.tsx
  • apps/backoffice-v2/src/common/components/organisms/Sidebar/Sidebar.MenuSubButton.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • services/workflows-service/.env.example
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: spell_check
  • GitHub Check: build (windows-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: lint
  • GitHub Check: Analyze (javascript)
  • GitHub Check: format

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (4)
services/workflows-service/src/env.ts (2)

27-47: Simplify SESSION_SAME_SITE validation.

The current implementation has redundant boolean literals. Consider simplifying the validation.

-  SESSION_SAME_SITE: z
-    .union([
-      z.literal('strict'),
-      z.literal('lax'),
-      z.literal('none'),
-      z.literal('true'),
-      z.literal('false'),
-      z.boolean(),
-    ])
-    .transform(val => {
-      if (val === 'true') {
-        return true;
-      }
-
-      if (val === 'false') {
-        return false;
-      }
-
-      return val;
-    })
-    .default(false),
+  SESSION_SAME_SITE: z
+    .union([
+      z.enum(['strict', 'lax', 'none']),
+      z.boolean(),
+      z.enum(['true', 'false']).transform(val => val === 'true'),
+    ])
+    .default(false),

49-66: Extract common boolean transformation logic.

The boolean transformation logic is duplicated across multiple fields. Consider extracting it into a reusable transformer.

+const booleanTransformer = z
+  .union([z.literal('true'), z.literal('false'), z.boolean()])
+  .transform(val => val === 'true')
+  .default(false);
+
-  SESSION_HTTP_ONLY: z
-    .union([z.literal('true'), z.literal('false'), z.boolean()])
-    .transform(val => {
-      return val === 'true';
-    })
-    .default(false),
+  SESSION_HTTP_ONLY: booleanTransformer,
-  SESSION_SECURE_COOKIE: z
-    .union([z.literal('true'), z.literal('false'), z.boolean()])
-    .transform((val: unknown) => val === 'true')
-    .default(false),
+  SESSION_SECURE_COOKIE: booleanTransformer,
-  SESSION_SECURE_PROXY: z
-    .union([z.literal('true'), z.literal('false'), z.boolean()])
-    .transform(val => {
-      return val === 'true';
-    })
-    .default(false),
+  SESSION_SECURE_PROXY: booleanTransformer,
services/workflows-service/src/main.ts (1)

98-104: Enhance cookie session security configuration.

Consider adding additional security options to the cookie configuration.

 const cookieSessionConfig = {
   name: 'session',
   httpOnly: env.SESSION_SECURE_COOKIE,
   secure: env.SESSION_SECURE_COOKIE,
   sameSite: env.SESSION_SAME_SITE,
   maxAge: 1000 * 60 * env.SESSION_EXPIRATION_IN_MINUTES,
+  signed: true,
+  domain: env.COOKIE_DOMAIN, // Add this to env.ts
+  path: '/api',
 };
scripts/generate-salt.sh (1)

61-64: Updating Environment Files with SESSION_ENCRYPTION_SECRET
Within the update_env_file function, the script removes any existing SESSION_ENCRYPTION_SECRET entry from the target .env files and appends the new key. This approach maintains consistency across multiple environment files. As a good-to-have improvement, consider backing up the original files before modification to aid in troubleshooting if needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9f31b28 and d06607b.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • deploy/.env (1 hunks)
  • scripts/generate-salt.sh (3 hunks)
  • services/workflows-service/.env.example (2 hunks)
  • services/workflows-service/package.json (2 hunks)
  • services/workflows-service/scripts/generate-encryption-key.ts (1 hunks)
  • services/workflows-service/src/auth/crypto.ts (1 hunks)
  • services/workflows-service/src/auth/local/local-auth.guard.ts (1 hunks)
  • services/workflows-service/src/env.ts (1 hunks)
  • services/workflows-service/src/main.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • services/workflows-service/scripts/generate-encryption-key.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: build (windows-latest)
  • GitHub Check: lint
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: spell_check
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
services/workflows-service/src/main.ts (1)

108-111: LGTM! Proxy trust configuration is secure.

Setting trust proxy to 1 is appropriate when running behind a load balancer.

deploy/.env (1)

33-33: New SESSION_ENCRYPTION_SECRET Added
The new environment variable SESSION_ENCRYPTION_SECRET is introduced with a specific encryption key. This key will be used for the encryption and decryption processes across the platform. Please verify that it is managed securely and, in production, consider using a secrets management solution rather than hardcoding sensitive values in version-controlled files.

services/workflows-service/.env.example (2)

10-13: New Session Cookie Settings Added
Four new environment variables related to session cookies (SESSION_HTTP_ONLY, SESSION_SAME_SITE, SESSION_SECURE_COOKIE, and SESSION_SECURE_PROXY) have been introduced with default values set to false. These settings will help refine cookie session security and behavior. Ensure that these defaults align with your deployment and security requirements, especially when moving to production.


43-44: Updated Security Variables
HASHING_KEY_SECRET_BASE64 has been updated and SESSION_ENCRYPTION_SECRET has been added. It is important to confirm that all parts of the system reference these secret values consistently for both hashing and encryption operations.

scripts/generate-salt.sh (3)

22-24: Integration of Encryption Key Generation
The script now invokes generate-encryption-key.ts (via npx tsx) to obtain an encryption key, storing it in encryption_key_value. This new step complements the existing bcrypt salt generation and supports the enhanced session encryption functionality. Please verify that generate-encryption-key.ts produces a secure and sufficiently random key.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 24-24: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


32-35: Validation for Encryption Key
A conditional check ensures that encryption_key_value is not empty, mirroring the salt generation validation. This error handling is crucial to avoid proceeding with an undefined encryption key.


68-69: Confirmation Echo Statements
The echo statements at the end of the update_env_file function confirm that both HASHING_KEY_SECRET_BASE64 and SESSION_ENCRYPTION_SECRET have been set as intended. These messages are useful for verification during deployment.

services/workflows-service/package.json (2)

88-88: Added Dependency: crypto-js
The new dependency "crypto-js" (version ^4.2.0) has been added to support cryptographic functions—particularly the AES encryption/decryption needed for secure session handling. Please review its usage in the related crypto functions (in services/workflows-service/src/auth/crypto.ts) for adherence to cryptographic best practices.


127-127: Added Dev Dependency: @types/crypto-js
The addition of "@types/crypto-js" as a dev dependency improves type safety and the overall developer experience when working with crypto-js in TypeScript.

@@ -22,6 +22,48 @@ export const serverEnvSchema = {
BCRYPT_SALT: z.coerce.number().int().nonnegative().or(z.string()),
PORT: z.coerce.number(),
DB_URL: z.string().url(),

SESSION_ENCRYPTION_SECRET: z.string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for SESSION_ENCRYPTION_SECRET.

The encryption secret should be validated for minimum length and complexity to ensure strong encryption.

-  SESSION_ENCRYPTION_SECRET: z.string(),
+  SESSION_ENCRYPTION_SECRET: z.string().min(32).regex(/^[A-Za-z0-9+/=]+$/),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
SESSION_ENCRYPTION_SECRET: z.string(),
SESSION_ENCRYPTION_SECRET: z.string().min(32).regex(/^[A-Za-z0-9+/=]+$/),

Comment on lines 4 to 9
export const encryptData = (data: string): string => {
// Encrypt the data using AES encryption
const encrypted = CryptoJS.AES.encrypt(data, env.SESSION_ENCRYPTION_SECRET).toString();

return encrypted;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance encryption security with IV.

The current implementation doesn't use an IV (Initialization Vector), which is recommended for AES encryption.

 export const encryptData = (data: string): string => {
+  if (!data) throw new Error('Data to encrypt cannot be empty');
+
   // Encrypt the data using AES encryption
-  const encrypted = CryptoJS.AES.encrypt(data, env.SESSION_ENCRYPTION_SECRET).toString();
+  const iv = CryptoJS.lib.WordArray.random(16);
+  const encrypted = CryptoJS.AES.encrypt(data, env.SESSION_ENCRYPTION_SECRET, {
+    iv: iv,
+    mode: CryptoJS.mode.CBC,
+    padding: CryptoJS.pad.Pkcs7
+  });
+
+  // Return both the IV and encrypted data
+  return iv.toString() + ':' + encrypted.toString();
 
   return encrypted;
 };

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 11 to 17
export const decryptData = (encryptedData: string): string => {
// Decrypt the data using AES decryption
const bytes = CryptoJS.AES.decrypt(encryptedData, env.SESSION_ENCRYPTION_SECRET);
const decrypted = bytes.toString(CryptoJS.enc.Utf8); // Convert decrypted bytes back to UTF-8 string

return decrypted;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for decryption.

The decryption function should handle potential errors gracefully.

 export const decryptData = (encryptedData: string): string => {
+  if (!encryptedData) throw new Error('Data to decrypt cannot be empty');
+
+  try {
+    const [ivStr, encryptedStr] = encryptedData.split(':');
+    if (!ivStr || !encryptedStr) throw new Error('Invalid encrypted data format');
+
+    const iv = CryptoJS.enc.Hex.parse(ivStr);
     // Decrypt the data using AES decryption
-    const bytes = CryptoJS.AES.decrypt(encryptedData, env.SESSION_ENCRYPTION_SECRET);
+    const bytes = CryptoJS.AES.decrypt(encryptedStr, env.SESSION_ENCRYPTION_SECRET, {
+      iv: iv,
+      mode: CryptoJS.mode.CBC,
+      padding: CryptoJS.pad.Pkcs7
+    });
     const decrypted = bytes.toString(CryptoJS.enc.Utf8); // Convert decrypted bytes back to UTF-8 string
 
+    if (!decrypted) throw new Error('Decryption failed');
     return decrypted;
+  } catch (error) {
+    throw new Error('Failed to decrypt data: ' + (error instanceof Error ? error.message : 'Unknown error'));
+  }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const decryptData = (encryptedData: string): string => {
// Decrypt the data using AES decryption
const bytes = CryptoJS.AES.decrypt(encryptedData, env.SESSION_ENCRYPTION_SECRET);
const decrypted = bytes.toString(CryptoJS.enc.Utf8); // Convert decrypted bytes back to UTF-8 string
return decrypted;
};
export const decryptData = (encryptedData: string): string => {
if (!encryptedData) throw new Error('Data to decrypt cannot be empty');
try {
const [ivStr, encryptedStr] = encryptedData.split(':');
if (!ivStr || !encryptedStr) throw new Error('Invalid encrypted data format');
const iv = CryptoJS.enc.Hex.parse(ivStr);
// Decrypt the data using AES decryption
const bytes = CryptoJS.AES.decrypt(encryptedStr, env.SESSION_ENCRYPTION_SECRET, {
iv: iv,
mode: CryptoJS.mode.CBC,
padding: CryptoJS.pad.Pkcs7
});
const decrypted = bytes.toString(CryptoJS.enc.Utf8); // Convert decrypted bytes back to UTF-8 string
if (!decrypted) throw new Error('Decryption failed');
return decrypted;
} catch (error) {
throw new Error('Failed to decrypt data: ' + (error instanceof Error ? error.message : 'Unknown error'));
}
};

Comment on lines 6 to 11
async canActivate(context: ExecutionContext) {
const result = await super.canActivate(context);
const request = context.switchToHttp().getRequest<Request>();

await super.logIn(request);

return result as boolean;
return (await super.canActivate(context)) as boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Reorder authentication operations for better security.

The current implementation logs in the user before verifying if they can activate. This could lead to an inconsistent state if activation fails.

 async canActivate(context: ExecutionContext) {
   const request = context.switchToHttp().getRequest<Request>();
 
-    await super.logIn(request);
+    const canActivate = (await super.canActivate(context)) as boolean;
+    
+    if (canActivate) {
+      await super.logIn(request);
+    }
 
-    return (await super.canActivate(context)) as boolean;
+    return canActivate;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async canActivate(context: ExecutionContext) {
const result = await super.canActivate(context);
const request = context.switchToHttp().getRequest<Request>();
await super.logIn(request);
return result as boolean;
return (await super.canActivate(context)) as boolean;
async canActivate(context: ExecutionContext) {
const request = context.switchToHttp().getRequest<Request>();
const canActivate = (await super.canActivate(context)) as boolean;
if (canActivate) {
await super.logIn(request);
}
return canActivate;
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9f46f9 and d683a1d.

📒 Files selected for processing (1)
  • packages/common/src/utils/crypto/index.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: build (windows-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: lint
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/common/src/utils/crypto/index.ts (3)

1-1: LGTM!

The import statement is correctly typed and matches the package dependency.


10-16: ⚠️ Potential issue

Add error handling and update decryption to match encryption changes.

The decryption function needs error handling and must be updated to handle the IV-based encryption:

Here's the complementary implementation for the updated encryption:

-export const decryptData = (encryptedData: string, secret: string): string => {
-  // Decrypt the data using AES decryption
-  const bytes = CryptoJS.AES.decrypt(encryptedData, secret);
-  const decrypted = bytes.toString(CryptoJS.enc.Utf8); // Convert decrypted bytes back to UTF-8 string
-
-  return decrypted;
-};
+export const decryptData = (encryptedData: string, secret: string): string => {
+  try {
+    if (!encryptedData || !secret) {
+      throw new Error('Encrypted data and secret are required');
+    }
+    if (secret.length < 32) {
+      throw new Error('Secret key must be at least 32 characters long');
+    }
+
+    // Convert from Base64
+    const rawData = CryptoJS.enc.Base64.parse(encryptedData);
+    
+    // Extract IV and encrypted data
+    const iv = CryptoJS.lib.WordArray.create(rawData.words.slice(0, 4));
+    const encrypted = CryptoJS.lib.WordArray.create(rawData.words.slice(4));
+
+    // Decrypt the data using AES decryption with IV
+    const decrypted = CryptoJS.AES.decrypt(
+      { ciphertext: encrypted },
+      secret,
+      {
+        iv: iv,
+        mode: CryptoJS.mode.CBC,
+        padding: CryptoJS.pad.Pkcs7
+      }
+    ).toString(CryptoJS.enc.Utf8);
+
+    if (!decrypted) {
+      throw new Error('Decryption failed: Invalid result');
+    }
+
+    return decrypted;
+  } catch (error) {
+    throw new Error(`Decryption failed: ${error.message}`);
+  }
+};

Additionally, consider adding unit tests to verify the encryption/decryption cycle works correctly with various inputs.

Would you like me to generate comprehensive unit tests for these functions?

✅ Verification successful

Enhance decryption with proper error handling and IV extraction

The updated implementation correctly addresses the concerns raised in the review:

  • It now validates input parameters (ensuring both the encrypted data and a secret key of at least 32 characters are provided).
  • It converts the Base64 string into a word array, properly extracts the IV, and uses it for IV-based decryption.
  • Decryption failures are caught and rethrown with a general error message—which prevents inadvertent leakage of sensitive details.
  • The suggestion to add unit tests remains important to ensure the encryption–decryption cycle works correctly with various inputs.

These changes are essential for both security and robustness. Do let me know if you’d like me to generate comprehensive unit tests to further verify the crypto functions.


1-16: 🏁 Script executed:

#!/bin/bash
# Description: Verify the SESSION_ENCRYPTION_SECRET environment variable

# Check if SESSION_ENCRYPTION_SECRET is defined in environment files
echo "Checking environment files for SESSION_ENCRYPTION_SECRET..."
rg "SESSION_ENCRYPTION_SECRET" -A 1

# Check the length and entropy of the secret if found
python3 << 'EOF'
import os
import math

def calculate_entropy(s):
    prob = [float(s.count(c)) / len(s) for c in dict.fromkeys(list(s))]
    entropy = -sum(p * math.log2(p) for p in prob)
    return entropy

secret = os.getenv('SESSION_ENCRYPTION_SECRET', '')
if secret:
    print(f"\nSecret length: {len(secret)} characters")
    print(f"Entropy: {calculate_entropy(secret):.2f} bits")
    print(f"Minimum recommended: 256 bits of entropy")
EOF

Length of output: 847


🏁 Script executed:

#!/bin/bash
# Description: Find where these crypto functions are used

echo "Checking usage of encryption functions..."
ast-grep --pattern 'encryptData($$$)' --pattern 'decryptData($$$)'

Length of output: 328

Comment on lines 3 to 8
export const encryptData = (data: string, secret: string): string => {
// Encrypt the data using AES encryption
const encrypted = CryptoJS.AES.encrypt(data, secret).toString();

return encrypted;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance encryption security and add error handling.

While the basic implementation works, several security improvements are recommended:

  1. Use an IV (Initialization Vector) to prevent deterministic encryption
  2. Add error handling for invalid inputs
  3. Validate the secret key's strength
  4. Sanitize and validate input data

Here's a more secure implementation:

-export const encryptData = (data: string, secret: string): string => {
-  // Encrypt the data using AES encryption
-  const encrypted = CryptoJS.AES.encrypt(data, secret).toString();
-
-  return encrypted;
-};
+export const encryptData = (data: string, secret: string): string => {
+  try {
+    if (!data || !secret) {
+      throw new Error('Data and secret are required');
+    }
+    if (secret.length < 32) {
+      throw new Error('Secret key must be at least 32 characters long');
+    }
+    
+    // Generate a random IV
+    const iv = CryptoJS.lib.WordArray.random(16);
+    
+    // Encrypt the data using AES encryption with IV
+    const encrypted = CryptoJS.AES.encrypt(data, secret, {
+      iv: iv,
+      mode: CryptoJS.mode.CBC,
+      padding: CryptoJS.pad.Pkcs7
+    });
+    
+    // Combine IV and encrypted data
+    const ivAndEncrypted = iv.concat(encrypted.ciphertext).toString(CryptoJS.enc.Base64);
+    
+    return ivAndEncrypted;
+  } catch (error) {
+    throw new Error(`Encryption failed: ${error.message}`);
+  }
+};

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
services/workflows-service/.env.example (1)

43-43: Document the HASHING_KEY_SECRET_BASE64 format and usage.

Add a comment explaining the expected format and purpose of this Base64-encoded secret.

-HASHING_KEY_SECRET_BASE64=JDJiJDEwJDFzT1VQckUxeWdsZEFZZ3I3MjRaaC4=
+# Base64-encoded secret used for hashing. Generate a secure random value in production
+HASHING_KEY_SECRET_BASE64=JDJiJDEwJDFzT1VQckUxeWdsZEFZZ3I3MjRaaC4=
🧰 Tools
🪛 Gitleaks (8.21.2)

43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d683a1d and 5bd3a13.

📒 Files selected for processing (2)
  • services/workflows-service/.env.example (2 hunks)
  • services/workflows-service/src/env.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • services/workflows-service/src/env.ts
🧰 Additional context used
🪛 Gitleaks (8.21.2)
services/workflows-service/.env.example

43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test_windows
  • GitHub Check: Analyze (javascript)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5bd3a13 and 5cfc198.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • services/workflows-service/.env.example (2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
services/workflows-service/.env.example

43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: test_windows
  • GitHub Check: test_linux
  • GitHub Check: spell_check
  • GitHub Check: build (windows-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: format
  • GitHub Check: Analyze (javascript)
  • GitHub Check: lint
🔇 Additional comments (1)
services/workflows-service/.env.example (1)

10-13: Document Production Considerations for Session Security Settings.
The new session variables (SESSION_HTTP_ONLY, SESSION_SAME_SITE, SESSION_SECURE_COOKIE, SESSION_SECURE_PROXY) are currently set to false which is acceptable for local development but pose security risks in production. Please add inline comments that clearly document the implications of these settings and provide recommendations for production environments (e.g., setting these to true or more secure alternatives).

Comment on lines 42 to 43
HASHING_KEY_SECRET_BASE64=JDJiJDEwJDFzT1VQckUxeWdsZEFZZ3I3MjRaaC4=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review the HASHING_KEY_SECRET_BASE64 Value for Security.
The updated HASHING_KEY_SECRET_BASE64 on line 43 contains a generic API key value. Static analysis indicates that generic keys may expose sensitive operations if used in production. Consider generating a securely randomized key or clearly marking this as a non-production sample value with an accompanying comment.

🧰 Tools
🪛 Gitleaks (8.21.2)

43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants