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(api): Added validation for reason field #190

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

JosephFak
Copy link
Contributor

@JosephFak JosephFak commented Apr 23, 2024

User description

Description

Introduced a validation pipe for the reason field
Introduced two tests alongside this

Implemented the validation across 6 files

Fixes #161

Dependencies

N/A

Future Improvements

I dont think much can change about this, its a relatively simple process.

Screenshots of relevant screens

image

Photo of code passing test

Developer's checklist

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

If changes are made in the code:

  • I have followed the coding guidelines
  • [X ] My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • [ X] I have added test cases to show that my feature works
  • [X ] 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
  • [X ] I have made the necessary updates to the documentation, or no documentation changes are required.

Type

enhancement, bug_fix


Description

  • Implemented AlphanumericReasonValidationPipe to ensure that the reason query parameter across various controllers only contains alphanumeric characters and spaces.
  • Added this validation to controllers for approval, environment, project, secret, variable, and workspace management.
  • Created unit tests for the new validation pipe to ensure it correctly allows valid inputs and rejects invalid ones.

Changes walkthrough

Relevant files
Enhancement
approval.controller.ts
Integrate Alphanumeric Validation in Approval Controller 

apps/api/src/approval/controller/approval.controller.ts

  • Added AlphanumericReasonValidationPipe to the reason query parameter
    in updateApproval method.
  • +2/-1     
    alphanumeric-reason-pipe.ts
    Implement Alphanumeric Reason Validation Pipe                       

    apps/api/src/common/alphanumeric-reason-pipe.ts

  • Implemented AlphanumericReasonValidationPipe which validates
    alphanumeric strings.
  • Throws BadRequestException if validation fails.
  • +11/-0   
    environment.controller.ts
    Apply Alphanumeric Validation in Environment Controller   

    apps/api/src/environment/controller/environment.controller.ts

  • Applied AlphanumericReasonValidationPipe to reason query parameter in
    environment-related methods.
  • +3/-2     
    project.controller.ts
    Integrate Alphanumeric Validation in Project Controller   

    apps/api/src/project/controller/project.controller.ts

  • Integrated AlphanumericReasonValidationPipe in project management
    methods.
  • +4/-3     
    secret.controller.ts
    Apply Alphanumeric Validation in Secret Controller             

    apps/api/src/secret/controller/secret.controller.ts

  • Added AlphanumericReasonValidationPipe to all secret management
    methods.
  • +6/-5     
    variable.controller.ts
    Enforce Alphanumeric Validation in Variable Controller     

    apps/api/src/variable/controller/variable.controller.ts

  • Enforced alphanumeric validation on reason query parameter across
    variable management methods.
  • +7/-7     
    workspace.controller.ts
    Integrate Alphanumeric Validation in Workspace Controller

    apps/api/src/workspace/controller/workspace.controller.ts

  • Integrated AlphanumericReasonValidationPipe to workspace update
    method.
  • +3/-2     
    Tests
    alphanumeric-reason-pipe.spec.ts
    Add Tests for Alphanumeric Reason Validation Pipe               

    apps/api/src/common/alphanumeric-reason-pipe.spec.ts

  • Created tests for AlphanumericReasonValidationPipe.
  • Tests include validation for both valid and invalid inputs.
  • +25/-0   

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @JosephFak JosephFak requested a review from rajdip-b as a code owner April 23, 2024 05:52
    Copy link
    Contributor

    PR Description updated to latest commit (5a99355)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and involve adding a validation pipe across multiple controllers. The code is well-organized, and the addition of unit tests helps in understanding the impact and functionality of the changes.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The regex used in AlphanumericReasonValidationPipe allows spaces, which might not be intended as it could allow strings of only spaces. Consider adjusting the regex or adding a check to ensure the string is not just empty spaces.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add string trimming before validation to handle leading and trailing spaces.

    Consider adding a trim operation before validating the string to ensure that leading and
    trailing spaces do not cause a valid input to be rejected.

    apps/api/src/common/alphanumeric-reason-pipe.ts [6]

    -if (typeof value === 'string' && /^[a-zA-Z0-9\s]*$/.test(value)) {
    -  return value;
    +if (typeof value === 'string' && /^[a-zA-Z0-9\s]*$/.test(value.trim())) {
    +  return value.trim();
     }
     
    Adjust the regex to disallow leading and trailing spaces while allowing spaces between words.

    Modify the regular expression to allow for spaces between alphanumeric characters but not
    at the beginning or end of the string.

    apps/api/src/common/alphanumeric-reason-pipe.ts [6]

    -if (typeof value === 'string' && /^[a-zA-Z0-9\s]*$/.test(value)) {
    +if (typeof value === 'string' && /^[a-zA-Z0-9]+(?:\s[a-zA-Z0-9]+)*$/.test(value)) {
       return value;
     }
     
    Improve error messages for better debugging and user feedback.

    Include more detailed error information in the BadRequestException to aid debugging and
    user feedback.

    apps/api/src/common/alphanumeric-reason-pipe.ts [9]

    -throw new BadRequestException('Reason must only contain alphanumeric characters and spaces.');
    +throw new BadRequestException(`Invalid reason: '${value}'. Reason must only contain alphanumeric characters and spaces.`);
     
    Best practice
    Enforce type safety by specifying the parameter type.

    Add a specific type for the value parameter in the transform method to enforce that it
    must be a string, improving type safety.

    apps/api/src/common/alphanumeric-reason-pipe.ts [5]

    -transform(value: any): string {
    +transform(value: string): string {
     
    Simplify the string type check for clarity and conciseness.

    Consider using a more concise and modern approach to check if the value is a string using
    typeof value === 'string'.

    apps/api/src/common/alphanumeric-reason-pipe.ts [6]

    -if (typeof value === 'string' && /^[a-zA-Z0-9\s]*$/.test(value)) {
    +if (/^[a-zA-Z0-9\s]*$/.test(value)) {
       return value;
     }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @rajdip-b rajdip-b changed the title feat: added validation for reason field feat(api): Added validation for reason field Apr 23, 2024
    @rajdip-b
    Copy link
    Member

    The E2E tests are failing. Can you confirm if they run on your device?

    @JosephFak
    Copy link
    Contributor Author

    I have made all the changes above, however I am not sure why the e2e is not running

    Copy link

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    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.

    All good bro!

    @rajdip-b rajdip-b merged commit 90b8ff2 into keyshade-xyz:develop Apr 24, 2024
    5 checks passed
    rajdip-b pushed a commit that referenced this pull request May 12, 2024
    ## [1.3.0](v1.2.0...v1.3.0) (2024-05-12)
    
    ### 🚀 Features
    
    * Add approval support ([#158](#158)) ([e09ae60](e09ae60))
    * **api:** Add configuration live update support ([#181](#181)) ([f7d6684](f7d6684))
    * **api:** Add feature to export data of a workspace ([#152](#152)) ([46833aa](46833aa))
    * **api:** Add Integration support ([#203](#203)) ([f1ae87e](f1ae87e))
    * **api:** Add note to [secure] and variable ([#151](#151)) ([2e62351](2e62351))
    * **api:** Add OAuth redirection and polished authentication ([#212](#212)) ([d2968bc](d2968bc))
    * **api:** Add support for storing and managing variables ([#149](#149)) ([963a8ae](963a8ae))
    * **api:** Added GitLab OAuth ([#188](#188)) ([4d3bbe4](4d3bbe4))
    * **api:** Added validation for reason field ([#190](#190)) ([90b8ff2](90b8ff2))
    * **api:** Create default workspace on user's creation ([#182](#182)) ([3dc0c4c](3dc0c4c))
    * **api:** Reading `port` Dynamically ([#170](#170)) ([fd46e3e](fd46e3e))
    * **auth:** Add Google OAuth ([#156](#156)) ([cf387ea](cf387ea))
    * **web:** Added waitlist ([#168](#168)) ([1084c77](1084c77))
    * **web:** Landing revamp ([#165](#165)) ([0bc723b](0bc723b))
    
    ### 🐛 Bug Fixes
    
    * **web:** alignment issue in “Collaboration made easy” section ([#178](#178)) ([df5ca75](df5ca75))
    * **workspace:** delete duplicate tailwind config ([99d922a](99d922a))
    
    ### 📚 Documentation
    
    * add contributor list ([f37569a](f37569a))
    * Add integration docs ([#204](#204)) ([406ddb7](406ddb7))
    * Added integration docs to gitbook summary ([ab37530](ab37530))
    * **api:** Add swagger docs of API key controller ([#167](#167)) ([2910476](2910476))
    * **api:** Add swagger docs of User Controller ([#166](#166)) ([fd59522](fd59522))
    * fix typo in environment-variables.md ([#163](#163)) ([48294c9](48294c9))
    * Remove supabase from docs ([#169](#169)) ([eddbce8](eddbce8))
    * **setup:** replace NX with Turbo in setup instructions ([#175](#175)) ([af8a460](af8a460))
    * Update README.md ([b59f16b](b59f16b))
    * Update running-the-api.md ([177dbbf](177dbbf))
    * Update running-the-api.md ([#193](#193)) ([3d5bcac](3d5bcac))
    
    ### 🔧 Miscellaneous Chores
    
    * Added lockfile ([60a3b9b](60a3b9b))
    * Added lockfile ([6bb512c](6bb512c))
    * **api:** Added type inference and runtime validation to `process.env` ([#200](#200)) ([249e07d](249e07d))
    * **api:** Fixed prisma script env errors ([#209](#209)) ([8762354](8762354))
    * **API:** Refactor authority check functions in API ([#189](#189)) ([e9d710d](e9d710d))
    * **api:** Refactor user e2e tests ([b38d45a](b38d45a))
    * **ci:** Disabled api stage release ([97877c4](97877c4))
    * **ci:** Update stage deployment config ([868a6a1](868a6a1))
    * **codecov:** update api-e2e project coverage ([1e90d7e](1e90d7e))
    * **dockerfile:** Fixed web dockerfile ([6134bb2](6134bb2))
    * **docker:** Optimized web Dockerfile to reduct image size ([#173](#173)) ([444286a](444286a))
    * **release:** Downgraded package version ([c173fee](c173fee))
    * **release:** Fix failing release ([#213](#213)) ([40f64f3](40f64f3))
    * **release:** Install pnpm ([1081bea](1081bea))
    * **release:** Updated release commit ([b8958e7](b8958e7))
    * **release:** Updated release commit ([e270eb8](e270eb8))
    * Update deprecated husky Install command ([#202](#202)) ([e61102c](e61102c))
    * Upgrade @million/lint from 0.0.66 to 0.0.73 ([#172](#172)) ([dd43ed9](dd43ed9))
    * **web:** Updated fly memory config ([4debc66](4debc66))
    
    ### 🔨 Code Refactoring
    
    * **api:** Made events central to workspace ([#159](#159)) ([9bc00ae](9bc00ae))
    * **api:** Migrated to cookie based authentication ([#206](#206)) ([ad6911f](ad6911f))
    * **monorepo:** Migrate from nx to turbo ([#153](#153)) ([88b4b00](88b4b00))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 1.3.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type: enhancement New feature or request
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Sanitize the reason field in API
    2 participants