Skip to content

Conversation

@6f6d6172
Copy link
Contributor

@6f6d6172 6f6d6172 commented Jan 2, 2026

Backfilling two rules. Normally I'd do separate PRs for each rule, but they're both small enough that I don't think it'll be an issue to review.

  • ImplicitPersistCredentials: Flags uses of actions/checkout where persist-credentials is not explicitly set. Not setting it means we fall back to the default value of true (valid for versions v2 to v6 (current at the time)). This will cause the action to write these temporary credentials to ~/.git/config or a temporary directory, where it can be found by malicious code and used by an attacker from an external system to gain access to the repository being cloned or even other repositories.
  • GlobalPermissionsBlock: Following least privileges, each job should explicitly list out the permissions it needs to do its work. A set of permissions configured at the workflow level means they will apply to all jobs, even jobs that might not actually need all of those permissions.

/task https://betterconfluence.atlassian.net/browse/SEC-385
/task https://betterconfluence.atlassian.net/browse/SEC-390

Both of these flagged behaviors are unfortunately pretty common, so we will need to be particular about the upgrade process here.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds two new security-focused rules for GitHub Actions workflows to help prevent credential leakage and over-permissioned jobs. Both rules address common security issues in CI/CD pipelines that could be exploited during supply chain attacks.

  • Adds ImplicitPersistCredentials rule to flag actions/checkout usage without explicit persist-credentials setting
  • Adds GlobalPermissionsBlock rule to flag workflow-level permissions when multiple jobs exist
  • Introduces a new dig helper function for traversing nested objects in rule expressions

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
lib/claws/rule/implicit_persist_credentials.rb Implements the ImplicitPersistCredentials rule using expression-based step checking
lib/claws/rule/global_permissions_block.rb Implements the GlobalPermissionsBlock rule using custom workflow-level validation
spec/claws/rule/implicit_persist_credentials_spec.rb Adds test coverage for the ImplicitPersistCredentials rule with positive and negative cases
spec/claws/rule/global_permissions_block_spec.rb Adds test coverage for the GlobalPermissionsBlock rule with various job configurations
lib/claws/base_rule.rb Adds the dig helper function and updates rubocop suppressions for complexity metrics
lib/claws/rule.rb Registers the two new rules for loading
README.md Documents both new rules with examples and security rationale

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

6f6d6172 and others added 2 commits January 2, 2026 18:08
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +47
end
end
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The test suite is missing a test case for when persist-credentials is explicitly set to true. According to the documentation, explicitly setting it to true (when needed) is also a valid way to satisfy this rule. Add a test case to verify that persist-credentials: true does not trigger a violation.

Copilot uses AI. Check for mistakes.
@6f6d6172
Copy link
Contributor Author

6f6d6172 commented Jan 2, 2026

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

YAML

expect(violations.count).to eq(1)
expect(violations[0].line).to eq(8)
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The line number expectation appears incorrect. Looking at the YAML structure, the uses: actions/checkout@v6 statement is on line 76 of the YAML, but the test expects the violation to be on line 8. This seems to be checking the wrong line number - should verify the line numbering matches where the violation actually occurs.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😑

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


### GlobalPermissionsBlock

[The permissions block](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack.
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The documentation URL appears to use an older or incorrect path structure. Based on other GitHub Actions documentation links in this file (e.g., line 304), the URL should likely be https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#permissions instead of using the /actions/reference/workflows-and-actions/ path.

Suggested change
[The permissions block](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack.
[The permissions block](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#permissions) dictates which permissions a workflow or a job have or don't have. When permissions are defined at the workflow level, each job gets the same set of permissions, potentially giving more access than a job or a step truly needs. Instead, permissions should be defined at the job level, making sure each job and step has access only to the permissions it needs. This can minimize the impact of untrusted code using an overly permissive `$GITHUB_TOKEN`, such as during a supply chain attack.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the suggested url literally redirects to my url. what r u doin

Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

This is awesome! Very clean PR, easy to review/follow

domain lgtm

Copy link

@rdadlani rdadlani left a comment

Choose a reason for hiding this comment

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

well explained, well documented. tests make it really clear what's happening. love it. domain lgtm. 💪 nice work.

@6f6d6172
Copy link
Contributor Author

6f6d6172 commented Jan 6, 2026

/no-platform

@6f6d6172 6f6d6172 merged commit cb534e2 into main Jan 6, 2026
11 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