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

Feature/self open ai key #491

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kervanaslan
Copy link

@kervanaslan kervanaslan commented Dec 15, 2024

What kind of change does this PR introduce?

eg: feature

Why was this change needed?

New feature: Use ENABLE_OPENAI_SELF="true" to display a field on the user profile page where users can enter their own OpenAI API key. This allows users to use their own OpenAI key for accessing OpenAI services.

Other information:

eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?

No.

Checklist:

Put a "X" in the boxes below to indicate you have followed the checklist;

  • [ x ] I have read the CONTRIBUTING guide.
  • [ x ] I checked that there were not similar issues or PRs already open for this.
  • [ x ] This PR fixes just ONE issue (do not include multiple issues or types of change in the same PR) For example, don't try and fix a UI issue and include new dependencies in the same PR.

Summary by CodeRabbit

  • New Features

    • Introduced new configuration variables for OpenAI and X integrations.
    • Added a new provider, XSelfProvider, for enhanced social media integration.
    • Included a conditional input field for OpenAI API Key in the settings.
  • Bug Fixes

    • Improved error handling during file uploads in Cloudflare storage.
  • Documentation

    • Updated environment variable documentation in .env.example.
  • Refactor

    • Enhanced readability and formatting in various components and methods.
  • Chores

    • Added migration script for new database schema changes, including OpenAI API Key storage.

Kervan Aslan added 2 commits December 13, 2024 20:32
…own APIKEY and Access Token. By setting ENABLE_X_SELF="true" in the .env file, each user can use their own API Key.
…user profile page where users can enter their own OpenAI API key. This allows users to use their own OpenAI key for accessing OpenAI services.
Copy link

vercel bot commented Dec 15, 2024

Someone is attempting to deploy a commit to the Listinai Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

coderabbitai bot commented Dec 15, 2024

Walkthrough

This pull request introduces a comprehensive feature for enabling user-specific OpenAI API key management and a new X (Twitter) self-integration provider. The changes span multiple components across the frontend and backend, adding configuration variables, updating database schemas, and implementing new provider functionality. The primary focus is on allowing users to use their personal OpenAI API keys and providing a more flexible social media integration mechanism.

Changes

File Change Summary
.env.example Added ENABLE_OPENAI_SELF and ENABLE_X_SELF configuration variables
apps/backend/src/api/routes/copilot.controller.ts Updated to support user-specific OpenAI API keys
apps/frontend/src/app/layout.tsx Added enabledOpenaiSelf context variable
apps/frontend/src/components/launches/providers/show.all.providers.tsx Added XSelfProvider to providers list
libraries/nestjs-libraries/src/database/prisma/schema.prisma Added optional openAIAPIKey field to User model
libraries/nestjs-libraries/src/integrations/integration.manager.ts Conditionally load XSelfProvider based on environment variable

Sequence Diagram

sequenceDiagram
    participant User
    participant Frontend
    participant Backend
    participant OpenAI
    participant Database

    User->>Frontend: Configure OpenAI API Key
    Frontend->>Backend: Submit API Key
    Backend->>Database: Store User's OpenAI API Key
    User->>Frontend: Request AI Copilot
    Frontend->>Backend: Request with User Context
    Backend->>Database: Retrieve User's API Key
    Backend->>OpenAI: Use User's Specific API Key
    OpenAI-->>Backend: AI Response
    Backend-->>Frontend: Return AI Response
Loading

Possibly related PRs

Poem

🐰 A Rabbit's Ode to API Keys and Tweets

With OpenAI's might and Twitter's sweet beats,
Our code now dances to personalized feats!
Self-powered providers, keys shining bright,
Empowering users with technological might!

Hop, hop, hooray! 🚀🔑

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. This feature will be included in our Pro Plan when released.


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

@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: 9

🔭 Outside diff range comments (1)
libraries/nestjs-libraries/src/upload/cloudflare.storage.ts (1)

Line range hint 86-93: Implement removeFile method to prevent resource leaks.

The commented-out removeFile implementation could lead to resource leaks and increased storage costs over time.

Consider implementing the method:

  async removeFile(filePath: string): Promise<void> {
-   // const fileName = filePath.split('/').pop(); // Extract the filename from the path
-   // const command = new DeleteObjectCommand({
-   //   Bucket: this._bucketName,
-   //   Key: fileName,
-   // });
-   // await this._client.send(command);
+   const fileName = filePath.split('/').pop();
+   if (!fileName) {
+     throw new Error('Invalid file path');
+   }
+   const command = new DeleteObjectCommand({
+     Bucket: this._bucketName,
+     Key: fileName,
+   });
+   try {
+     await this._client.send(command);
+   } catch (e) {
+     const error = e instanceof Error ? e : new Error('Unknown error during file removal');
+     this.logger.error('Failed to remove file from Cloudflare R2', {
+       error: error.message,
+       filePath,
+       fileName,
+     });
+     throw error;
+   }
  }
🧹 Nitpick comments (5)
apps/frontend/src/components/launches/providers/xself/xself.provider.tsx (1)

13-14: Use endsWith instead of indexOf for file extension checks

Using indexOf('mp4') > -1 can incorrectly match filenames that contain 'mp4' elsewhere in the name (e.g., 'example_mp4_video.txt'). Consider using endsWith('.mp4') for accurate file extension checks.

Apply the following diff to update the file extension checks:

- (p) => p.some((m) => m.path.indexOf('mp4') > -1) && p.length > 1
+ (p) => p.some((m) => m.path.endsWith('.mp4')) && p.length > 1
- if (load.indexOf('mp4') > -1) {
+ if (load.endsWith('.mp4')) {

Also applies to: 20-21

apps/backend/src/api/routes/users.controller.ts (1)

55-62: Consider extracting subscription tier logic.

The tier determination logic mixes environment checks with subscription status. Consider extracting this into a separate service method for better maintainability.

-      tier:
-        // @ts-ignore
-        organization?.subscription?.subscriptionTier ||
-        (!process.env.STRIPE_PUBLISHABLE_KEY ? 'ULTIMATE' : 'FREE'),
+      tier: this._subscriptionService.determineUserTier(organization?.subscription, process.env.STRIPE_PUBLISHABLE_KEY),
libraries/nestjs-libraries/src/database/prisma/integrations/integration.service.ts (1)

413-413: Consider explicit return type and values.

The method implicitly returns undefined in multiple places. Consider adding a return type to the method and being explicit about the return values for better code clarity.

-  async processPlugs(data: {
+  async processPlugs(): Promise<void> {
     // ... existing code ...
-    return;
+    return undefined;

Also applies to: 439-439, 443-443

apps/frontend/src/components/layout/settings.component.tsx (1)

80-81: Remove debug console.log statement

The console.log statement appears to be leftover debugging code and should be removed.

-  console.log('zaaa', enabledOpenaiSelf);
libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql (1)

393-400: Fix typo in table name: "ExisingPlugData" → "ExistingPlugData"

There's a typo in the table name. This should be fixed to maintain code quality and prevent confusion.

-CREATE TABLE "ExisingPlugData" (
+CREATE TABLE "ExistingPlugData" (
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3d185ad and 1eb30af.

⛔ Files ignored due to path filters (1)
  • apps/frontend/public/icons/platforms/xself.png is excluded by !**/*.png
📒 Files selected for processing (18)
  • .env.example (2 hunks)
  • apps/backend/src/api/routes/copilot.controller.ts (1 hunks)
  • apps/backend/src/api/routes/users.controller.ts (2 hunks)
  • apps/frontend/src/app/layout.tsx (2 hunks)
  • apps/frontend/src/components/launches/providers/show.all.providers.tsx (2 hunks)
  • apps/frontend/src/components/launches/providers/xself/xself.provider.tsx (1 hunks)
  • apps/frontend/src/components/layout/layout.settings.tsx (2 hunks)
  • apps/frontend/src/components/layout/settings.component.tsx (5 hunks)
  • libraries/nestjs-libraries/src/database/prisma/integrations/integration.service.ts (3 hunks)
  • libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql (1 hunks)
  • libraries/nestjs-libraries/src/database/prisma/migrations/migration_lock.toml (1 hunks)
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma (2 hunks)
  • libraries/nestjs-libraries/src/database/prisma/users/users.repository.ts (2 hunks)
  • libraries/nestjs-libraries/src/dtos/users/user.details.dto.ts (2 hunks)
  • libraries/nestjs-libraries/src/integrations/integration.manager.ts (1 hunks)
  • libraries/nestjs-libraries/src/integrations/social/xself.provider.ts (1 hunks)
  • libraries/nestjs-libraries/src/upload/cloudflare.storage.ts (3 hunks)
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • libraries/nestjs-libraries/src/database/prisma/migrations/migration_lock.toml
🔇 Additional comments (17)
apps/frontend/src/components/launches/providers/xself/xself.provider.tsx (1)

1-52: Code implementation is correct and functions as expected

The validation logic and the checkVideoDuration function are implemented correctly. The code effectively enforces media constraints for posts.

apps/frontend/src/app/layout.tsx (2)

41-41: LGTM! Environment variable coercion is correct.

The use of !! operator correctly coerces the ENABLE_OPENAI_SELF environment variable to a boolean value.


28-28: LGTM! Favicon link formatting improved.

The favicon link element formatting change improves readability without affecting functionality.

libraries/nestjs-libraries/src/upload/cloudflare.storage.ts (1)

33-35: LGTM! Robust content-type header access.

The header access is well-implemented, checking both case variants of 'content-type'.

.env.example (1)

77-77: LGTM! The OpenAI self-key feature toggle is properly configured.

The ENABLE_OPENAI_SELF environment variable is correctly placed in the Misc Settings section alongside other OpenAI-related configurations, with a safe default value of "false".

apps/frontend/src/components/launches/providers/show.all.providers.tsx (1)

20-20: LGTM! XSelfProvider integration looks correct.

The XSelfProvider is properly imported and added to the Providers array, maintaining consistency with other provider implementations.

Also applies to: 41-41

libraries/nestjs-libraries/src/integrations/integration.manager.ts (1)

28-28: Verify the impact of the conditional provider initialization.

The provider selection logic needs careful testing to ensure it works correctly in all environments.

✅ Verification successful

Let me gather more context about the provider implementations and their usage.


Based on the code analysis, I can now provide a final response:

The conditional provider initialization is well-designed and safe to use

The code review reveals that both XProvider and XSelfProvider implement the same interface and provide identical functionality with different authentication approaches:

  • XProvider: Uses environment variables (X_API_KEY, X_API_SECRET) for app credentials
  • XSelfProvider: Uses user-provided API credentials through custom fields

The conditional initialization is safe because:

  • Both providers implement the same SocialProvider interface
  • They share identical posting, auto-repost, and auto-plug functionality
  • The only difference is in how they handle authentication and API credentials
  • The toggle is used only once at initialization, reducing complexity
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential issues with X provider initialization

# Check if XSelfProvider is used in other places
rg -l "XSelfProvider"

# Check for environment variable usage patterns
rg "ENABLE_X_SELF"

# Look for provider initialization patterns
ast-grep --pattern 'new XProvider()' 
ast-grep --pattern 'new XSelfProvider()'

Length of output: 776


Script:

#!/bin/bash
# Check the implementation of both providers
cat libraries/nestjs-libraries/src/integrations/social/xself.provider.ts
cat libraries/nestjs-libraries/src/integrations/social/x.provider.ts

# Check if there are any tests related to these providers
rg -l "XProvider.test|XProvider.spec"
rg -l "XSelfProvider.test|XSelfProvider.spec"

# Check for any configuration files that might set ENABLE_X_SELF
fd -e env -e config -e yaml -e yml | xargs rg "ENABLE_X_SELF"

Length of output: 16551

libraries/nestjs-libraries/src/database/prisma/users/users.repository.ts (1)

131-131: LGTM! Changes align with the feature requirements.

The addition of openAIAPIKey to both getPersonal and changePersonal methods is implemented correctly and consistently.

Also applies to: 152-152

apps/backend/src/api/routes/users.controller.ts (1)

45-45: Verify the necessity of the Request parameter.

The @Req() parameter has been added but its usage appears to be only for accessing cookies. Consider using the more specific @Cookies() decorator if available in your framework, or document why the full request object is needed.

✅ Verification successful

Let me verify if there are any other usages of the Request parameter or if there's a specific reason for using it over cookies.


Let me try a different pattern to find the method implementation.


The Request parameter is necessary for accessing impersonation cookies

The @Req() parameter in the getSelf method is specifically used to check the impersonation state through req.cookies.impersonate, which is a legitimate use case that cannot be replaced with a simpler @Cookies() decorator. The request object's cookies are used to determine if the user is in impersonation mode, which is then included in the response as impersonate: !!req.cookies.impersonate.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the Request object is used for anything other than cookies
rg -A 5 "req\." apps/backend/src/api/routes/users.controller.ts

Length of output: 199


Script:

#!/bin/bash
# Check the full implementation of getSelf method
ast-grep --pattern 'getSelf($$$) {
  $$$
}'

Length of output: 45


Script:

#!/bin/bash
# Check the full implementation of getSelf method with class context
ast-grep --pattern 'class $_ {
  $$$
  getSelf($$$) {
    $$$
  }
  $$$
}'

# Also get the full file content to see the method implementation
cat apps/backend/src/api/routes/users.controller.ts

Length of output: 20366

libraries/nestjs-libraries/src/database/prisma/schema.prisma (1)

45-45: LGTM! The schema change looks good.

The addition of the optional openAIAPIKey field to the User model is appropriate for storing user-specific OpenAI API keys.

Let's verify if there are any migration files for this schema change:

✅ Verification successful

Migration file exists and matches the schema change

The migration file 20241213183825_add_open_aiapi_key_to_user has been properly created for adding the openAIAPIKey field to the User model. The migration is recent (December 13, 2024) and corresponds to the schema change being reviewed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for migration files related to the openAIAPIKey addition
fd --type f ".*openai.*\.sql$" --exec cat {} \;

Length of output: 52


Script:

#!/bin/bash
# Let's check for any migration files in common migration directories
fd --type f "\.sql$" --exec cat {} \;

# Also check for Prisma migration files
fd --type f "migration\.sql$" --exec cat {} \;

Length of output: 48500


Script:

#!/bin/bash
# Let's check for any Prisma migration files in the migrations directory
fd --type d "migrations" --exec ls -la {} \;

# Also check for any recent migration files
fd --type f --changed-within 2weeks ".*\.sql$" --exec ls -la {} \;

Length of output: 556

apps/frontend/src/components/layout/settings.component.tsx (2)

197-201: LGTM! Proper feature flag implementation.

The OpenAI API Key input field is correctly conditionally rendered based on the enabledOpenaiSelf flag.


161-164: LGTM! Improved event handler placement.

The onClick handler is now properly placed on the text element rather than the parent button.

apps/frontend/src/components/layout/layout.settings.tsx (2)

135-138: LGTM! Improved formatting.

The systray-buttons div formatting has been improved for better readability.


90-90: Verify the logo display logic change

The condition for displaying the logo has been inverted from isGeneral ? '/postiz.svg' : '/logo.svg' to !isGeneral ? '/postiz.svg' : '/logo.svg'. Please confirm this change is intentional and matches the expected behavior.

✅ Verification successful

Logo display logic change is consistent with server-side implementation

The change in logo display logic (!isGeneral ? '/postiz.svg' : '/logo.svg') is correct as it maintains consistency with the server-side implementation in auth/layout.tsx. Both files now follow the same pattern where:

  • When isGeneral is false, it shows postiz.svg
  • When isGeneral is true, it shows logo.svg

This is also consistent with other usages of isGeneral in the same component for styling and conditional rendering.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for other instances of logo display logic
rg -A 2 "postiz\.svg|logo\.svg"

Length of output: 583


Script:

#!/bin/bash
# Let's check the git history for this specific line to understand the change
git log -L ':apps/frontend/src/components/layout/layout.settings.tsx' | grep -A 5 "src="

# Also, let's check the isGeneral usage in both files
rg "isGeneral" apps/frontend/src/components/layout/layout.settings.tsx -A 2 -B 2
rg "isGeneralServerSide" apps/frontend/src/app/auth/layout.tsx -A 2 -B 2

Length of output: 2196

libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql (3)

38-63: LGTM: User table schema changes look good

The addition of the nullable openAIAPIKey field to the User table is well-designed:

  • TEXT type is appropriate for storing API keys
  • Nullable constraint aligns with the optional nature of the feature
  • No index on the API key field, which is correct as it won't be used for searching

609-727: LGTM: Database schema design follows best practices

The foreign key constraints and relationships are well-designed:

  • Appropriate use of ON DELETE RESTRICT for required relationships
  • Correct use of ON DELETE SET NULL for optional relationships
  • Proper referential integrity maintained throughout the schema

402-419: LGTM: Index design is well-optimized

The indexing strategy for the User table and related entities is well-thought-out:

  • Appropriate indexes on frequently queried fields
  • Composite unique index on email and providerName prevents duplicate accounts
  • No unnecessary indexes that could slow down write operations

Comment on lines +26 to +38
let openAIAPIKey = '';

//Check if OPEN AI is enabled for Self tokens.
if (process.env.ENABLE_OPENAI_SELF === 'true') {
const userPersonal = await this._userService.getPersonal(user.id);
openAIAPIKey = userPersonal.openAIAPIKey;
} else {
openAIAPIKey = process.env.OPENAI_API_KEY;
}

if (openAIAPIKey === undefined || openAIAPIKey === '') {
Logger.warn('OpenAI API key not set, chat functionality will not work');
return
return;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle missing OpenAI API key by returning an appropriate HTTP response

When the OpenAI API key is missing or invalid, the method logs a warning and returns without sending a response to the client. This may cause the client request to hang or receive an unexpected result. It's important to inform the client of the issue.

Apply this diff to return a 400 Bad Request response with an error message:

 if (openAIAPIKey === undefined || openAIAPIKey === '') {
   Logger.warn('OpenAI API key not set, chat functionality will not work');
+  res.status(400).json({ error: 'OpenAI API key is not set or invalid.' });
+  return;
 }

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

Comment on lines +182 to +215
async authenticate(params: { code: string; codeVerifier: string }) {
const body = JSON.parse(Buffer.from(params.code, 'base64').toString());

const { code, codeVerifier } = params;
const [oauth_token, oauth_token_secret] = codeVerifier.split(':');

const startingClient = new TwitterApi({
appKey: body.apiKey,
appSecret: body.apiSecretKey,
accessToken: body.accessToken,
accessSecret: body.accessTokenSecret,
});

const { id, name, profile_image_url_https, screen_name } =
await startingClient.currentUser(true);

return {
id: String(id),
accessToken:
body.apiKey +
':' +
body.apiSecretKey +
':' +
body.accessToken +
':' +
body.accessTokenSecret,

name,
refreshToken: '',
expiresIn: 999999999,
picture: profile_image_url_https,
username: screen_name,
};
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate inputs and securely handle user API credentials

The authenticate method processes user-provided API keys and tokens without input validation or error handling. This poses security risks, including potential injection attacks and credential leakage.

Apply the following changes to enhance security:

  • Input Validation and Error Handling:

    async authenticate(params: { code: string; codeVerifier: string }) {
  • if (!params.code || !params.codeVerifier) {

  • throw new Error('Missing required authentication parameters.');
    
  • }

  • let body;

  • try {

  • body = JSON.parse(Buffer.from(params.code, 'base64').toString());
    
  • } catch (error) {

  • throw new Error('Invalid code parameter format.');
    
  • }

  • if (

  • !body.apiKey ||
    
  • !body.apiSecretKey ||
    
  • !body.accessToken ||
    
  • !body.accessTokenSecret
    
  • ) {

  • throw new Error('Incomplete API credentials provided.');
    
  • }

    const { codeVerifier } = params;
    const [oauth_token, oauth_token_secret] = codeVerifier.split(':');


- **Secure Storage of Credentials:**

Ensure that API keys and tokens are encrypted before storage and decrypted only when in use.

- **Avoid Logging Sensitive Data:**

Review the code for any logging statements that might output sensitive information and remove or sanitize them.

- **Implement Access Controls:**

Restrict access to methods handling sensitive data to authorized users only.



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

<!-- This is an auto-generated comment by CodeRabbit -->

Comment on lines +18 to +20
@IsString()
@IsOptional()
openAIAPIKey: string;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid exposing sensitive openAIAPIKey in UserDetailDto

Including the openAIAPIKey in UserDetailDto may inadvertently expose the user's API key when user details are sent to the frontend, posing a security risk.

Consider the following steps:

  • Remove openAIAPIKey from UserDetailDto if it's not necessary to send this data to the client.
  • If users need to input their API key, handle it separately and ensure it's not included in responses.
  • Ensure that any transmission of the API key is done securely (e.g., over HTTPS) and that the key is stored encrypted on the server.

@@ -5,9 +5,10 @@ import { createContext, FC, ReactNode, useContext, useEffect } from 'react';
interface VariableContextInterface {
billingEnabled: boolean;
isGeneral: boolean;
enabledOpenaiSelf: boolean;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

The new context property is properly used but has a debug console.log statement

The enabledOpenaiSelf property is correctly integrated and used in appropriate locations:

  • Initialized in layout with an environment variable
  • Used in settings component for conditional rendering
  • Properly defined in the context interface

However, there's a debug console.log statement that should be removed:

  • apps/frontend/src/components/layout/settings.component.tsx: Remove console.log('zaaa', enabledOpenaiSelf);
🔗 Analysis chain

LGTM! Verify usage of the new context property.

The addition of enabledOpenaiSelf to the context interface and its initialization is well-implemented with appropriate typing and a secure default value of false.

Let's verify all components that consume this new context property:

Also applies to: 20-20

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components using the enabledOpenaiSelf property
rg "enabledOpenaiSelf" --type tsx --type ts

Length of output: 71


Script:

#!/bin/bash
# Search for components using the enabledOpenaiSelf property with proper file extensions
rg "enabledOpenaiSelf" -t js -t jsx -t ts -t tsx

Length of output: 76


Script:

#!/bin/bash
# Search for components using the enabledOpenaiSelf property without file type restrictions
rg "enabledOpenaiSelf"

Length of output: 622

Comment on lines +47 to +51
try {
await this._client.send(command);
} catch (e) {
console.log(e);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling in uploadSimple method.

The current error handling simply logs to console and swallows the error. This could mask issues and make debugging difficult.

Consider implementing proper error handling:

  try {
    await this._client.send(command);
  } catch (e) {
-   console.log(e);
+   const error = e instanceof Error ? e : new Error('Unknown error during upload');
+   this.logger.error('Failed to upload file to Cloudflare R2', {
+     error: error.message,
+     path,
+     id,
+   });
+   throw error;
  }

Also consider:

  1. Adding retry logic for transient failures
  2. Adding metrics/monitoring for upload failures
  3. Implementing proper logging using a logger service instead of console.log

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

Comment on lines +93 to +94
#Enable X Integration with Self Generated Tokens.
ENABLE_X_SELF="true"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review the default value and placement of X self-integration toggle.

A few concerns about the ENABLE_X_SELF configuration:

  1. Default value is set to "true" which might not be safe for production environments
  2. It's placed at the end of the file instead of being grouped with other X-related settings (lines 27-31)

Consider these changes:

-#Enable X Integration with Self Generated Tokens.
-ENABLE_X_SELF="true"

+# X Settings
 X_API_KEY=""
 X_API_SECRET=""
 X_CLIENT=""
 X_SECRET=""
+ENABLE_X_SELF="false" # Enable X Integration with Self Generated Tokens

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

@@ -21,10 +21,11 @@ import { DiscordProvider } from '@gitroom/nestjs-libraries/integrations/social/d
import { SlackProvider } from '@gitroom/nestjs-libraries/integrations/social/slack.provider';
import { MastodonProvider } from '@gitroom/nestjs-libraries/integrations/social/mastodon.provider';
import { BlueskyProvider } from '@gitroom/nestjs-libraries/integrations/social/bluesky.provider';
import { XSelfProvider } from '@gitroom/nestjs-libraries/integrations/social/xself.provider';
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving the provider initialization logic.

The current implementation has several potential issues:

  1. The provider selection is fixed at startup and cannot be changed dynamically
  2. The environment variable check is not type-safe
  3. No fallback handling if the environment variable is undefined

Consider this more robust implementation:

-  process.env.ENABLE_X_SELF === 'true' ? new XSelfProvider() : new XProvider(),
+  process.env.ENABLE_X_SELF?.toLowerCase() === 'true' ? new XSelfProvider() : new XProvider(),

Also, consider implementing a factory pattern to allow dynamic provider switching:

class XProviderFactory {
  static createProvider(enableSelf: boolean): SocialProvider {
    return enableSelf ? new XSelfProvider() : new XProvider();
  }
}

Also applies to: 28-28

Comment on lines +57 to +61
console.log('XXXX UPLOADING');
const uploadedPicture = picture
? await this.storage.uploadSimple(picture)
: undefined;
console.log('XXXX UPLOAD DONE');
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove debug console.log statements.

Debug console.log statements should not be committed to production code. Consider replacing them with proper logging using a logging service if upload tracking is needed.

-    console.log('XXXX UPLOADING');
     const uploadedPicture = picture
       ? await this.storage.uploadSimple(picture)
       : undefined;
-    console.log('XXXX UPLOAD DONE');
📝 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
console.log('XXXX UPLOADING');
const uploadedPicture = picture
? await this.storage.uploadSimple(picture)
: undefined;
console.log('XXXX UPLOAD DONE');
const uploadedPicture = picture
? await this.storage.uploadSimple(picture)
: undefined;

"lastName" TEXT,
"isSuperAdmin" BOOLEAN NOT NULL DEFAULT false,
"bio" TEXT,
"openAIAPIKey" TEXT,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Encrypt OpenAI API keys at rest

The openAIAPIKey field currently stores API keys as plain text. API keys should be encrypted at rest to prevent unauthorized access in case of a data breach.

Consider implementing one of these approaches:

  1. Use a dedicated encryption service
  2. Leverage database-level encryption (e.g., PostgreSQL's pgcrypto)
  3. Implement application-level encryption before storing the keys

Would you like me to provide a detailed implementation guide for any of these approaches?

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.

1 participant