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: restrict sensitive configuration parameters in CLI arguments #1425

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

mrT23
Copy link
Collaborator

@mrT23 mrT23 commented Dec 30, 2024

PR Type

Enhancement, Security


Description

  • Enhanced security by expanding the list of restricted sensitive configuration parameters in CLI arguments
  • Added protection for critical parameters including:
    • Authentication tokens and secrets
    • Base URLs and endpoints
    • Deployment and provider configurations
  • Forces these sensitive parameters to be set via configuration files instead of CLI for better security
  • Refactored code to use local variable instead of instance variable for forbidden arguments list

Changes walkthrough 📝

Relevant files
Security
pr_agent.py
Enhance security by restricting sensitive CLI parameters 

pr_agent/agent/pr_agent.py

  • Moved forbidden CLI arguments from instance variable to local variable
  • Expanded list of sensitive configuration parameters that are
    restricted from CLI usage
  • Added security check for parameters like tokens, secrets, URLs and
    deployment settings
  • +4/-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 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 Security concerns

    No - The changes actually improve security by expanding the list of sensitive parameters that are restricted from being passed via CLI arguments, forcing them to be set via more secure configuration files instead. The implementation properly validates and blocks these sensitive parameters.

    ⚡ Recommended focus areas for review

    Case Sensitivity

    The forbidden arguments check should be case-insensitive to prevent bypassing restrictions by using different letter casing

    for forbidden_arg in forbidden_cli_args:
        for arg in args:
            if forbidden_arg in arg:

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Implement precise matching for forbidden CLI arguments to prevent false positives

    The current string matching for forbidden arguments is too permissive. It will match
    any substring, potentially blocking valid arguments. Use exact matching or word
    boundaries to prevent false positives.

    pr_agent/agent/pr_agent.py [69-71]

    -if forbidden_arg in arg:
    +if arg.startswith(f"--{forbidden_arg}=") or arg == f"--{forbidden_arg}":
         get_logger().error(
             f"CLI argument for param '{forbidden_arg}' is forbidden. Use instead a configuration file."
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The current substring matching could lead to false positives, blocking legitimate arguments. The suggested fix using exact prefix matching significantly improves security by preventing bypass attempts while allowing valid arguments.

    9
    Possible issue
    Enforce security restrictions by halting execution when forbidden arguments are detected

    The error handling for forbidden arguments doesn't stop execution, allowing the
    program to continue with potentially sensitive parameters. Add a system exit or
    raise an exception.

    pr_agent/agent/pr_agent.py [69-71]

     if forbidden_arg in arg:
         get_logger().error(
    -        f"CLI argument for param '{forbidden_arg}' is forbidden. Use instead a configuration file."
    +        f"CLI argument for param '{forbidden_arg}' is forbidden. Use instead a configuration file.")
    +    raise SecurityError(f"Forbidden CLI argument '{forbidden_arg}' detected")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Currently, the code only logs an error but continues execution, which could lead to security vulnerabilities. Adding an exception ensures the program stops when detecting forbidden arguments.

    8
    General
    Define security-sensitive configuration as an immutable constant at module level

    The list of sensitive parameters is defined inside a function, making it recreated
    on each call. Move it to a module-level constant to improve performance and
    maintainability.

    pr_agent/agent/pr_agent.py [63-65]

    -forbidden_cli_args = ['enable_auto_approval', 'base_url', 'url', 'app_name', 'secret_provider',
    -                      'git_provider', 'skip_keys', 'key', 'ANALYTICS_FOLDER', 'uri', 'app_id', 'webhook_secret',
    -                      'bearer_token', 'PERSONAL_ACCESS_TOKEN', 'override_deployment_type', 'private_key']
    +FORBIDDEN_CLI_ARGS = frozenset([
    +    'enable_auto_approval', 'base_url', 'url', 'app_name', 'secret_provider',
    +    'git_provider', 'skip_keys', 'key', 'ANALYTICS_FOLDER', 'uri', 'app_id', 'webhook_secret',
    +    'bearer_token', 'PERSONAL_ACCESS_TOKEN', 'override_deployment_type', 'private_key'
    +])
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a module-level frozenset constant improves code maintainability and prevents accidental modifications to the sensitive parameters list, while providing minor performance benefits.

    6
    • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

    @mrT23 mrT23 merged commit b3d4af6 into main Dec 30, 2024
    2 checks passed
    @mrT23 mrT23 deleted the tr/limit_online_commenting branch December 30, 2024 12:11
    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.

    2 participants