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: Empty name ("") accepted as a valid name while creating environments #583

Merged
merged 6 commits into from
Dec 9, 2024

Conversation

zaheer-Khan3260
Copy link
Contributor

@zaheer-Khan3260 zaheer-Khan3260 commented Dec 7, 2024

User description

Description

Add class-validator to check the property(name) is not empty before making the environmet.

Fixes #576


PR Type

Bug fix, Enhancement


Description

  • Added validation to ensure that the name property is not empty when creating an environment.
  • Utilized the @IsNotEmpty() decorator from class-validator to enforce this rule.
  • Addresses the issue where environments could be created with empty names, which is not allowed.

Changes walkthrough 📝

Relevant files
Bug fix
create.environment.ts
Enforce non-empty name for environment creation                   

apps/api/src/environment/dto/create.environment/create.environment.ts

  • Added @IsNotEmpty() decorator to the name property.
  • Ensures that the name field is not empty when creating an environment.

  • +2/-1     

    💡 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 🔶

    576 - Partially compliant

    Compliant requirements:

    Non-compliant requirements:

    • Missing validation for other entities (Workspace, Project, Secret, etc)
    • Missing tests to verify the validation
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Fix
    The PR only adds validation for environment names, but the ticket requires similar validation for all other entities. The fix should be extended to other DTOs.

    Copy link
    Contributor

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

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    ✅ Add proper string pattern validation to prevent invalid characters and ensure reasonable length limits

    Add @matches() decorator with a regex pattern to ensure the name contains valid
    characters and meets length requirements, not just non-empty validation.

    apps/api/src/environment/dto/create.environment/create.environment.ts [4-6]

     @IsString()
     @IsNotEmpty()
    +@Matches(/^[a-zA-Z0-9-_]{1,64}$/)
     name: string

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: Adding pattern validation for the name field is a crucial security measure that prevents potential injection attacks and ensures data consistency by restricting characters and length. This is especially important for environment names which might be used in sensitive operations.

    8

    💡 Need additional feedback ? start a PR chat

    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.

    Could you also add relevant tests for this under environment.e2e.spec.ts?

    @zaheer-Khan3260
    Copy link
    Contributor Author

    Yeah sure,

    @zaheer-Khan3260
    Copy link
    Contributor Author

    Hey @rajdip-b i added the test case under environment.e2e.spec.ts.

    @zaheer-Khan3260
    Copy link
    Contributor Author

    Hey @rajdip-b I updated the code according your suggestion. Review it please

    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.

    LGTM

    @rajdip-b rajdip-b merged commit a33f4b5 into keyshade-xyz:develop Dec 9, 2024
    2 checks passed
    muntaxir4 added a commit to muntaxir4/keyshade that referenced this pull request Dec 10, 2024
    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.

    API: Empty name ("") accepted as a valid name while creating environments
    2 participants