Skip to content

Add addEmptyTopLevelPermissions parameter for workflow permissions #2524

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

varunsh-coder
Copy link
Member

  • Added new query parameter addEmptyTopLevelPermissions to control permission handling
  • When set to true, adds empty permissions {} at workflow level
  • When set to true, includes contents:read at job level even if it's the only permission
  • Added tests for the new functionality
  • Updated existing tests to handle the new parameter

This allows more flexible control over permission configuration for workflows that need specific empty permission patterns.

- Added new query parameter addEmptyTopLevelPermissions to control permission handling
- When set to true, adds empty permissions {} at workflow level
- When set to true, includes contents:read at job level even if it's the only permission
- Added tests for the new functionality
- Updated existing tests to handle the new parameter

This allows more flexible control over permission configuration for workflows
that need specific empty permission patterns.
Copy link
Contributor

@step-security-bot step-security-bot left a comment

Choose a reason for hiding this comment

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

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

testfiles/secureworkflow/output/empty-permissions.yml

[
    {
        "Severity": "High",
        "Recommendation": "Avoid using empty permissions in GitHub Actions workflows",
        "Description": "Leaving permissions empty can lead to unintended consequences and security vulnerabilities, as it grants excessive access. It's recommended to explicitly define permissions for increased security.",
        "Remediation": "Define specific permissions needed for each action being used in the workflow to minimize the attack surface. For example, if only read access is required, explicitly specify 'read' permission."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Ensure that GitHub Actions workflows are triggered only by trusted events",
        "Description": "Allowing all push events to trigger workflows can potentially lead to unauthorized code execution. It's best practice to restrict workflow triggers to specific branches or users to mitigate security risks.",
        "Remediation": "Specify the branches that should trigger the workflow for better control over when it gets executed. For example, limit the trigger to specific branches like 'main' or 'master'."
    },
    {
        "Severity": "Low",
        "Recommendation": "Ensure there is a newline at the end of the file",
        "Description": "Having a newline at the end of a file is a common convention in many programming languages and can prevent issues with certain tools or commands.",
        "Remediation": "Add a newline character at the end of the file to adhere to standard coding practices."
    }
]

testfiles/toplevelperms/input/empty-permissions.yml

[]

testfiles/toplevelperms/output/empty-permissions.yml

[]

remediation/workflow/permissions/permissions.go

[
    {
        "Severity": "High",
        "Recommendation": "Sensitive data exposure",
        "Description": "The function AddWorkflowLevelPermissions adds sensitive information to the output when specific conditions are met, potentially exposing sensitive data.",
        "Remediation": "Ensure sensitive information is properly handled and avoid exposing it in the output. Consider adding proper access controls and encryption for sensitive data before including it in the output."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Insecure data handling",
        "Description": "There is a potential risk of insecure data handling in the function AddWorkflowLevelPermissions when dealing with permissions. The code may not properly manage and secure the permissions data.",
        "Remediation": "Implement secure data handling practices such as input validation, proper sanitization, and verification of permissions data before processing it to prevent potential vulnerabilities."
    },
    {
        "Severity": "Low",
        "Recommendation": "Inconsistent code structure",
        "Description": "The code structure in the AddWorkflowLevelPermissions function is inconsistent, with varying indentation levels and formatting styles, impacting code readability and maintenance.",
        "Remediation": "Standardize the code structure by following a consistent coding style guide, including indentation practices, commenting conventions, and overall formatting for improved code maintainability and readability."
    }
]

remediation/workflow/permissions/permissions_test.go

[
    {
        "Severity": "High",
        "Recommendation": "Avoid hardcoding sensitive data in code",
        "Description": "Sensitive data like file paths or API keys should not be hardcoded in the codebase as it poses a security risk.",
        "Remediation": "Store sensitive data like file paths or API keys in environment variables or a secure configuration file."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Avoid using 'os.Setenv' for setting environment variables in tests",
        "Description": "'os.Setenv' can lead to environment variable leakage and affect the stability of tests.",
        "Remediation": "Instead of using 'os.Setenv' directly, consider using a testing utility to manage test-specific environment variables."
    }
]

remediation/workflow/secureworkflow.go

[
    {
        "Severity": "High",
        "Recommendation": "Avoid hard-coding sensitive information such as API keys directly in the source code.",
        "Description": "The 'addEmptyTopLevelPermissions' variable seems to be a configuration switch, which should ideally be passed as a parameter or read from a config file instead of being hard-coded in the codebase.",
        "Remediation": "Pass 'addEmptyTopLevelPermissions' as a parameter to the 'SecureWorkflow' function or retrieve it from a configuration file."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Follow naming conventions and maintain consistency for variables and functions.",
        "Description": "The variable name 'addEmptyTopLevelPermissions' is inconsistent with the surrounding variable names. It's a good practice to maintain naming consistency.",
        "Remediation": "Rename 'addEmptyTopLevelPermissions' variable to follow the naming convention used in the codebase."
    }
]

testfiles/joblevelpermskb/output/empty-top-level-permissions.yml

[
    {
        "Severity": "High",
        "Recommendation": "Avoid using generic names like 'checkout'",
        "Description": "Using generic names can lead to confusion and potential conflicts with existing commands or keywords.",
        "Remediation": "Rename 'checkout-job' to a more specific and descriptive name to avoid any potential conflicts or confusion with other commands or keywords."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Explicitly define triggers for the workflow",
        "Description": "Explicitly defining triggers provides clarity on when the workflow should be executed, reducing the risk of unexpected execution.",
        "Remediation": "Define specific events that should trigger the workflow execution, e.g., specify the branches or tags for which the workflow should be triggered."
    },
    {
        "Severity": "Low",
        "Recommendation": "Ensure newlines at the end of the file",
        "Description": "Maintaining consistent newline usage at the end of files can improve readability and avoid potential issues with certain tools.",
        "Remediation": "Add a newline at the end of the file to ensure consistency and compatibility with different systems."
    }
]

testfiles/secureworkflow/input/empty-permissions.yml

  • [High]Use a specific commit hash or version tag for actions/checkout@v2 to ensure version stability and security
    Using a specific commit hash or version tag helps prevent unexpected changes or security vulnerabilities introduced by updates. Change 'uses: actions/checkout@v2' to 'uses: actions/checkout@<specific_commit_hash_or_version>'.
  • [Medium]Add validation or verification steps to ensure the integrity and authenticity of the downloaded setup-node@v1 action
    Verifying the integrity of third-party actions before usage reduces the risk of potential supply chain attacks. Implement a verification step using GPG signatures or checksums provided by the action authors.

remediation/workflow/secureworkflow_test.go

[
    {
        "Severity": "High",
        "Recommendation": "Avoid hardcoding sensitive data like API endpoints and keys directly in the code.",
        "Description": "Sensitive data such as API endpoints and keys should not be hardcoded within the source code. This can lead to security vulnerabilities if the code is exposed or leaked.",
        "Remediation": "Store sensitive data in environment variables or configuration files outside of the source code. Use a secure secrets management solution if needed."
    },
    {
        "Severity": "Medium",
        "Recommendation": "Avoid logging sensitive information like errors and file paths directly.",
        "Description": "Logging sensitive information such as errors and file paths can lead to unintentional exposure of data that could be exploited by malicious actors.",
        "Remediation": "Avoid logging sensitive information directly. Implement proper error handling mechanisms and ensure that only necessary information is logged securely."
    },
    {
        "Severity": "Low",
        "Recommendation": "Avoid using hardcoded file paths within the codebase.",
        "Description": "Hardcoded file paths can lead to issues when moving or deploying the code to different environments where paths may differ.",
        "Remediation": "Use relative paths or configurable paths that can be set externally to avoid hardcoded paths within the code."
    }
]

testfiles/joblevelpermskb/input/empty-top-level-permissions.yml

[]

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.

Copy link

codecov bot commented May 19, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.82%. Comparing base (7b9f651) to head (b072f04).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
remediation/workflow/permissions/permissions.go 80.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2524      +/-   ##
==========================================
+ Coverage   64.78%   64.82%   +0.04%     
==========================================
  Files          19       19              
  Lines        2002     2013      +11     
==========================================
+ Hits         1297     1305       +8     
- Misses        594      595       +1     
- Partials      111      113       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@varunsh-coder varunsh-coder merged commit 005fba8 into main May 19, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants