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

feat(cli): Add import sub commmand for project. #594

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

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Dec 22, 2024

User description

Description

@keyshade/secret-scan: Create a function to detect secrets and variables from JS object containing keys and values.

cli: Add feature to import secrets and variables from .env file into Keyshade.

Fixes #502
Fixes #503

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Added new CLI command keyshade project import to import secrets and variables from .env files
  • Implemented secret detection functionality in @keyshade/secret-scan package:
    • New detectObject function to analyze key-value pairs
    • Classifies values as secrets or variables using detection rules
    • Added comprehensive test coverage
  • Added interactive workflow:
    • Displays detected secrets and variables
    • Requires user confirmation
    • Allows environment selection
  • Handles errors gracefully during secret/variable creation

Changes walkthrough 📝

Relevant files
Enhancement
import.project.ts
Add import command for environment secrets and variables 

apps/cli/src/commands/project/import.project.ts

  • Added new command to import secrets and variables from .env file
  • Implemented file reading, parsing, and detection of secrets/variables
  • Added interactive confirmation and environment selection
  • Handles creation of secrets and variables with error handling
  • +144/-0 
    index.ts
    Add function to detect secrets from objects                           

    packages/secret-scan/src/index.ts

  • Added detectObject method to scan key-value pairs
  • Classifies values as secrets or variables based on detection rules
  • +18/-1   
    project.command.ts
    Register import command in project commands                           

    apps/cli/src/commands/project.command.ts

    • Added ImportEnvs command to project commands list
    +3/-1     
    index.d.ts
    Add types for secret detection results                                     

    packages/secret-scan/src/types/index.d.ts

    • Added ScanObjectResult interface for secret detection results
    +5/-0     
    Tests
    detect-object.test.ts
    Add tests for secret detection from objects                           

    packages/secret-scan/src/test/detect-object.test.ts

  • Added test cases for detecting secrets and variables from objects
  • Tests empty input, only secrets, only variables scenarios
  • Validates correct classification of different types of values
  • +63/-0   
    Dependencies
    package.json
    Add dotenv package dependency                                                       

    apps/cli/package.json

  • Added dotenv package dependency
  • Reordered dependencies alphabetically
  • +3/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    503 - Partially compliant

    Compliant requirements:

    • Import command added with .env file input support
    • Uses secret-scan package to detect secrets/variables
    • Uploads to specified project
    • Shows confirmation prompt
    • Prints success/error messages

    Non-compliant requirements:

    • Error handling stops entire operation on failure instead of continuing

    502 - Fully compliant

    Compliant requirements:

    • detectObject function created to scan objects
    • Detects and flags secrets vs variables
    • Returns proper JSON structure with arrays
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The import process stops on first error instead of continuing and collecting errors in an array as specified in requirements

              Logger.error(
                `Failed to create secret for ${key} : ${error.message}.\nAborting!`
              )
              throw Error('Error while creating secret')
            }
    Missing Validation

    No validation of project slug and environment slug existence before attempting to create secrets/variables

          const environmentSlug = (await text({
            message: 'Enter the environment slug to import to:'
          })) as string
    
          Logger.info(
            `Importing secrets and variables to project: ${projectSlug} and environment: ${environmentSlug} with default settings`
          )

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Dec 22, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Validate parsed environment variables to ensure non-empty input before processing

    Add validation for empty or invalid environment variables before processing. Check
    if envVariables object contains any entries after parsing.

    apps/cli/src/commands/project/import.project.ts [49-51]

     const envVariables = dotenv.parse(envFileContent)
    +if (Object.keys(envVariables).length === 0) {
    +  throw new Error('No environment variables found in the provided file')
    +}
     const secretsAndVariables = secretDetector.detectObject(envVariables)

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: This is a critical validation that prevents processing empty files and provides clear error feedback. Missing this check could lead to silent failures or unnecessary processing.

    8
    ✅ Verify file existence before attempting to read its contents

    Add file existence check before attempting to read the .env file to provide a
    clearer error message.

    apps/cli/src/commands/project/import.project.ts [44-47]

    -const envFileContent = await fs.readFile(
    -  path.resolve(dotEnvPath),
    -  'utf-8'
    -)
    +const resolvedPath = path.resolve(dotEnvPath)
    +const exists = await fs.access(resolvedPath).then(() => true).catch(() => false)
    +if (!exists) {
    +  throw new Error(`The .env file does not exist at path: ${resolvedPath}`)
    +}
    +const envFileContent = await fs.readFile(resolvedPath, 'utf-8')

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Important defensive programming practice that provides better error handling and user feedback when the .env file is missing, preventing cryptic filesystem errors.

    7
    Validate user input to prevent empty or invalid values

    Add validation for the environment slug to ensure it's not empty or just whitespace.

    apps/cli/src/commands/project/import.project.ts [77-79]

     const environmentSlug = (await text({
       message: 'Enter the environment slug to import to:'
     })) as string
    +if (!environmentSlug?.trim()) {
    +  throw new Error('Environment slug cannot be empty')
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Essential input validation that prevents potential issues with empty environment slugs, which could cause problems in downstream operations or database entries.

    7

    @muntaxir4
    Copy link
    Contributor Author

    These are the initial changes, and some updates might be required.

    apps/cli/src/commands/project.command.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/project/import.project.ts Outdated Show resolved Hide resolved
    packages/secret-scan/src/types/index.d.ts Outdated Show resolved Hide resolved
    @muntaxir4 muntaxir4 requested a review from rajdip-b December 23, 2024 20:26
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    I tried importing the env file. seems like anything that gets enclosed in "" gets detected as a variable. We need to strip values off "" before scanning

    apps/cli/src/commands/project/import.project.ts Outdated Show resolved Hide resolved
    Copy link

    codecov bot commented Dec 31, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 99.76%. Comparing base (ce50743) to head (f62b41c).
    Report is 258 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #594      +/-   ##
    ===========================================
    + Coverage    91.71%   99.76%   +8.05%     
    ===========================================
      Files          111       71      -40     
      Lines         2510      432    -2078     
      Branches       469        3     -466     
    ===========================================
    - Hits          2302      431    -1871     
    + Misses         208        1     -207     
    Flag Coverage Δ
    api-e2e-tests ?
    secret-scan 99.76% <100.00%> (?)

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

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