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: Added back missing endpoints #798

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

rajdip-b
Copy link
Member

@rajdip-b rajdip-b commented Feb 25, 2025

PR Type

Enhancement, Tests


Description

  • Added endpoints to fetch secrets and variables by project and environment.

    • Implemented getAllSecretsOfEnvironment in SecretController.
    • Implemented getAllVariablesOfEnvironment in VariableController.
  • Added corresponding service methods for secrets and variables retrieval.

  • Introduced new e2e tests for secrets and variables fetching functionality.

  • Updated schema and API client to support new endpoints.


Changes walkthrough 📝

Relevant files
Tests
3 files
project.e2e.spec.ts
Commented out a test for forked project access levels       
+29/-29 
secret.e2e.spec.ts
Added e2e tests for fetching secrets by project and environment
+98/-0   
variable.e2e.spec.ts
Added e2e tests for fetching variables by project and environment
+56/-0   
Formatting
1 files
project.service.ts
Minor formatting adjustment in project service                     
+1/-1     
Enhancement
11 files
secret.controller.ts
Added endpoint to fetch secrets by project and environment
+14/-0   
secret.service.ts
Implemented service method to fetch secrets by project and environment
+87/-1   
variable.controller.ts
Added endpoint to fetch variables by project and environment
+14/-0   
variable.service.ts
Implemented service method to fetch variables by project and
environment
+81/-1   
run.command.ts
Adjusted environmental variable handling in CLI command   
+2/-1     
secret.ts
Added API client method for fetching secrets by project and
environment
+15/-1   
variable.ts
Added API client method for fetching variables by project and
environment
+15/-1   
index.ts
Added schemas for fetching secrets by project and environment
+13/-0   
index.types.ts
Added types for fetching secrets by project and environment
+11/-1   
index.ts
Added schemas for fetching variables by project and environment
+13/-0   
index.types.ts
Added types for fetching variables by project and environment
+11/-1   
Documentation
2 files
Fetch all by project and environment.bru
Added API collection for fetching secrets by project and environment
+26/-0   
Fetch all by project and environment.bru
Added API collection for fetching variables by project and environment
+26/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The new endpoints expose all secrets and variables for a given project/environment in a single request. This could enable credential harvesting if an attacker gains access. Consider:

    1. Adding rate limiting
    2. Implementing access logging
    3. Adding pagination to limit data exposure
    4. Requiring additional authentication for bulk secret retrieval
    ⚡ Recommended focus areas for review

    Security Concern

    The getAllSecretsOfProjectAndEnvironment method returns decrypted secret values without any rate limiting or access logging, which could enable credential harvesting.

    async getAllSecretsOfProjectAndEnvironment(
      user: AuthenticatedUser,
      projectSlug: Project['slug'],
      environmentSlug: Environment['slug']
    ) {
      // Fetch the project
      const project =
        await this.authorizationService.authorizeUserAccessToProject({
          user,
          entity: { slug: projectSlug },
          authorities: [Authority.READ_SECRET]
        })
      const projectId = project.id
    
      // Check access to the environment
      const environment =
        await this.authorizationService.authorizeUserAccessToEnvironment({
          user,
          entity: { slug: environmentSlug },
          authorities: [Authority.READ_ENVIRONMENT]
        })
      const environmentId = environment.id
    
      const secrets = await this.prisma.secret.findMany({
        where: {
          projectId,
          versions: {
            some: {
              environmentId
            }
          }
        },
        include: {
          lastUpdatedBy: {
            select: {
              id: true,
              name: true
            }
          },
          versions: {
            where: {
              environmentId
            },
            orderBy: {
              version: 'desc'
            },
            take: 1,
            include: {
              environment: {
                select: {
                  id: true,
                  slug: true
                }
              }
            }
          }
        }
      })
    
      const response: ChangeNotification[] = []
    
      for (const secret of secrets) {
        response.push({
          name: secret.name,
          value: project.storePrivateKey
            ? await decrypt(project.privateKey, secret.versions[0].value)
            : secret.versions[0].value,
          isPlaintext: project.storePrivateKey
        })
      }
    
      return response
    Type Safety

    The @ts-expect-error comment suppresses a TypeScript error without proper type handling, which could lead to runtime issues.

    // @ts-expect-error this just works
    Input Validation

    The getAllVariablesOfEnvironment endpoint lacks input validation for projectSlug and environmentSlug parameters.

    async getAllVariablesOfEnvironment(
      @CurrentUser() user: AuthenticatedUser,
      @Param('projectSlug') projectSlug: string,
      @Param('environmentSlug') environmentSlug: string
    ) {

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 25, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle decryption failures gracefully

    The decrypt function call should be wrapped in a try-catch block to handle
    potential decryption failures gracefully, preventing unhandled exceptions.

    apps/api/src/secret/service/secret.service.ts [849-851]

     value: project.storePrivateKey
    -      ? await decrypt(project.privateKey, secret.versions[0].value)
    +      ? await decrypt(project.privateKey, secret.versions[0].value).catch(() => secret.versions[0].value)
           : secret.versions[0].value,
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding error handling for decryption is crucial for system stability. Without it, failed decryption attempts could crash the service instead of gracefully falling back to the encrypted value.

    Medium
    Safer environment variable handling

    The current environment variable handling could lead to overwriting of existing
    environment variables. Use Object.assign() to properly merge environment
    variables while preserving existing ones.

    apps/cli/src/commands/run.command.ts [166]

    -env: { ...process.env, ...this.processEnvironmentalVariables },
    +env: Object.assign({}, process.env, this.processEnvironmentalVariables),
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: While both spread operator and Object.assign() achieve similar results, Object.assign() can be slightly more explicit and safer for environment variable merging. However, the impact is minimal as both approaches are valid.

    Low
    • Update

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 25, 2025

    CI Feedback 🧐

    (Feedback updated until commit ad9459e)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Validate API

    Failed stage: E2E tests [❌]

    Failed test name: auth.e2e.spec.ts

    Failure summary:

    The action failed during end-to-end (e2e) testing with the following issues:

  • 1 test suite out of 14 failed
  • 1 test out of 370 total tests failed
  • The logs show multiple authentication-related errors in the AuthService:
    - Invalid empty email
    address error
    - Invalid email address "abcdef" error
    - Invalid login code "123456" for user
    "[email protected]"
  • The test process exited with code 1, suggesting unhandled async operations

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    944:  console.info
    945:  secp256k1 unavailable, reverting to browser version
    946:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    947:  PASS api src/api-key/api-key.e2e.spec.ts
    948:  ● Console
    949:  console.info
    950:  secp256k1 unavailable, reverting to browser version
    951:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    952:  �[31m[Nest] 3698  - �[39m02/25/2025, 12:13:23 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid email address: �[39m
    953:  �[31m[Nest] 3698  - �[39m02/25/2025, 12:13:23 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid email address: abcdef�[39m
    954:  �[31m[Nest] 3698  - �[39m02/25/2025, 12:13:23 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid login code for [email protected]: 123456�[39m
    ...
    
    962:  console.info
    963:  secp256k1 unavailable, reverting to browser version
    964:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    965:  PASS api src/app/app.e2e.spec.ts
    966:  ● Console
    967:  console.info
    968:  secp256k1 unavailable, reverting to browser version
    969:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    970:  Test Suites: 1 failed, 13 passed, 14 total
    971:  Tests:       1 failed, 369 passed, 370 total
    972:  Snapshots:   0 total
    973:  Time:        109.459 s
    974:  Ran all test suites.
    975:  Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?
    976:  /home/runner/work/keyshade/keyshade/apps/api:
    977:  ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @keyshade/[email protected] e2e: `pnpm run e2e:prepare && cross-env NODE_ENV='e2e' DATABASE_URL='***localhost:5432/tests' jest --runInBand --config=jest.e2e-config.ts && pnpm run e2e:teardown`
    978:  Exit status 1
    979:  ELIFECYCLE  Command failed with exit code 1.
    980:  ##[error]Process completed with exit code 1.
    

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

    Successfully merging this pull request may close these issues.

    1 participant