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: Expand CORS bypass for /clusters/:id/calls endpoint #415

Closed
wants to merge 1 commit into from

Conversation

nadeesha
Copy link
Contributor

@nadeesha nadeesha commented Dec 29, 2024

PR Type

Enhancement


Description

  • Enhanced CORS configuration by adding support for multiple bypass patterns
  • Added new CORS bypass for '/workflows/:id/calls' endpoint while maintaining existing '/clusters/:id/runs' pattern
  • Implemented array-based regex pattern matching using some() method for more flexible CORS bypass checks

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Expand CORS bypass patterns for workflow calls endpoint   

control-plane/src/index.ts

  • Replaced single CORS bypass regex with an array of regex patterns
  • Added new regex pattern for '/workflows/:id/calls' endpoint
  • Updated CORS check to test URL against multiple regex patterns
  • +5/-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:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    CORS Security Risk:
    The implementation uses broad regex patterns ('/clusters//runs' and '/workflows//calls') that could potentially allow unintended cross-origin access to sensitive endpoints. Consider:

    1. Adding more specific path constraints
    2. Implementing additional validation checks before allowing cross-origin requests
    3. Adding rate limiting for these endpoints
    ⚡ Recommended focus areas for review

    Security Risk

    The CORS bypass patterns are quite broad and may allow unintended access. Consider using more specific patterns or additional validation.

    const corsBypassRegexes = [
      new RegExp(/\/clusters\/.*\/runs/),
      new RegExp(/\/workflows\/.*\/calls/),
    ];

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Use literal regex notation instead of RegExp constructor for better performance and security

    Compile regex patterns once at initialization to improve performance and prevent
    potential regex injection attacks.

    control-plane/src/index.ts [38-41]

     const corsBypassRegexes = [
    -  new RegExp(/\/clusters\/.*\/runs/),
    -  new RegExp(/\/workflows\/.*\/calls/),
    +  /\/clusters\/.*\/runs/,
    +  /\/workflows\/.*\/calls/,
     ];
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This is a valid security and performance improvement. Using regex literals instead of RegExp constructor prevents potential regex injection risks and provides better performance as patterns are compiled at parse time.

    7
    General
    Add explicit null check for URL before regex testing to prevent potential runtime issues

    Add input validation to ensure req.url is not null before testing against regex
    patterns to prevent potential runtime errors.

    control-plane/src/index.ts [52]

    -if (corsBypassRegexes.some(regex => regex.test(req.url ?? ""))) {
    +if (req.url && corsBypassRegexes.some(regex => regex.test(req.url))) {
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion is technically correct, the existing code already handles null/undefined cases using the nullish coalescing operator (??) which makes the explicit null check redundant and less concise.

    3

    @nadeesha nadeesha closed this Dec 29, 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.

    1 participant