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 potentially privileged pull request medium query #19085

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

JarLob
Copy link
Contributor

@JarLob JarLob commented Mar 20, 2025

No description provided.

@Copilot Copilot bot review requested due to automatic review settings March 20, 2025 20:23
@JarLob JarLob requested a review from a team as a code owner March 20, 2025 20:23
Copy link
Contributor

@Copilot 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 fixes the handling of pull request code injection alerts by updating the associated configuration files and change notes.

  • Updated change notes to document the fix for the pull_request medium query.
  • Modified externally_triggereable_events.yml to include the "pull_request" event.
  • Updated context_event_map.yml with mappings for the "pull_request" event.

Reviewed Changes

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

File Description
actions/ql/lib/change-notes/released/2025-03-20-pullrequest.md Adds a release note for the pull_request medium query fix
actions/ql/lib/ext/config/externally_triggereable_events.yml Adds the "pull_request" event to the externally triggereable events list
actions/ql/lib/ext/config/context_event_map.yml Adds mappings for the "pull_request" event to the context event map
Files not reviewed (2)
  • actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected: Language not supported
  • actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected: Language not supported

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@github-actions github-actions bot added documentation Actions Analysis of GitHub Actions labels Mar 20, 2025
adityasharad
adityasharad previously approved these changes Mar 20, 2025
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks reasonable, one change note suggestion.

Is there a reason we didn't include these to begin with - were they considered trusted?

@JarLob
Copy link
Contributor Author

JarLob commented Mar 20, 2025

Workflows are considered running without permissions and secrets when pull_request is triggered from a fork. But they run privileged when triggered from branches in the same repository. It is trusted because creation of a branch requires the contributor to have write permissions to the repository already.
Previously there was an attack vector when non-maintainer could create a pull request between branches in the target repository he don't own to exploit the injection. Now it is allowed to maintainers only. But overall I think it is better to fix injections always. This is what the medium query is about, it explicitly checks inNonPrivilegedContext.

@JarLob
Copy link
Contributor Author

JarLob commented Mar 25, 2025

Please merge. I don't have the permission.

@adityasharad adityasharad merged commit fe7660f into github:main Mar 25, 2025
10 checks passed
@JarLob JarLob deleted the nonpriv branch March 25, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions Analysis of GitHub Actions documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants