fix restrict same repo ACL perm to trusted context by chmouel · Pull Request #2665 · tektoncd/pipelines-as-code

chmouel · 2026-04-09T08:10:05Z

📝 Description of the Change

Avoid granting issue_comment senders trust based solely on same-repo PR shape.

Keep the same-repo fast path for pull_request events and PR rerequest flows, while forcing comment senders through collaborator, org-membership, or OWNERS checks.

Add regression coverage to ensure issue_comment events do not bypass ACL, add debug-log coverage for the skipped shortcut path, and keep same-repo check_run/check_suite rerequests covered.

The GitHub ok-to-test e2e is intentionally limited to the untrusted comment path. It verifies that an untrusted same-repo issue_comment does not create a new PipelineRun or Repository status and emits the expected debug log. It does not expect a trusted /ok-to-test to create a second run on an already successful PR, because the matcher filters out already successful templates for ok-to-test/retest flows.

🔗 Linked GitHub Issue

Fixes #2664

🧪 Testing Strategy

🤖 AI Assistance

AI assistance can be used for various tasks, such as code generation,
documentation, or testing.

Please indicate whether you have used AI assistance
for this PR and provide details if applicable.

I have not used any AI assistance for this PR.
I have used AI assistance for this PR.

Important

Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.

If the majority of the code in this PR was generated by an AI, please add a Co-authored-by trailer to your commit message.
For example:

Co-authored-by: Claude noreply@anthropic.com

✅ Submitter Checklist

gemini-code-assist

Code Review

This pull request refactors the GitHub ACL logic by introducing a canUseSameRepoPullRequestShortcut helper function. This change ensures that the same-repo shortcut—which allows CI to run for non-members when the branch is within the same repository—is only applied to specific event types like pull requests and check runs, while excluding issue comments for better security. Corresponding unit tests were added to verify the behavior across different event types. I have no feedback to provide as there were no review comments to evaluate.

codecov-commenter · 2026-04-09T08:12:46Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.85%. Comparing base (de6de63) to head (7b9b4b0).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2665      +/-   ##
==========================================
+ Coverage   58.82%   58.85%   +0.03%     
==========================================
  Files         204      204              
  Lines       20134    20149      +15     
==========================================
+ Hits        11844    11859      +15     
  Misses       7525     7525              
  Partials      765      765

☔ 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.

pipelines-as-code · 2026-04-10T14:24:01Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	2	Likely a small diff focusing on conditional logic in ACL checks.
Logic complexity	3	Requires understanding the interaction between different GitHub event types and trust levels.
Risk	4	Security-related: impacts authorization and potential vulnerability to unauthorized command execution.
Cross-cutting	2	Primarily isolated to the authorization/ACL logic and event handling.
Test coverage	1	Explicitly mentions adding regression tests for both negative and positive cases.

Overall difficulty: Moderate

Summary

This PR addresses a security concern where issue_comment events were incorrectly granted "trusted" status based solely on the PR originating from the same repository. It restricts this "same-repo" shortcut to specific events (pull_request, check_run, check_suite) while forcing commenters to undergo standard collaborator/OWNERS validation.

Suggested reviewers focus

ACL Logic: Verify that the logic correctly distinguishes between pull_request (automated trust for same-repo) and issue_comment (required manual/membership trust).
Regression Tests: Ensure the new tests adequately cover the scenario where an external contributor comments on a same-repo PR (should be denied if not in OWNERS).
Side Effects: Confirm that "rerequest" flows for check_run and check_suite still function as intended for legitimate users.

Generated by Pipelines-as-Code LLM Analysis

pipelines-as-code · 2026-04-10T14:31:25Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	2	Likely a surgical fix in the ACL logic plus new test cases.
Logic complexity	3	Involves conditional logic based on event types and repository relationships.
Risk	5	High Risk. Directly modifies Access Control Lists (ACL) and trust models; errors could lead to unauthorized CI execution.
Cross-cutting	2	Primarily impacts the authorization/event-handling layer.
Test coverage	2	Explicitly includes regression tests for the reported bypass.

Overall difficulty: Moderate

Summary

This PR addresses a security-sensitive bug (Fixes #2664) where issue_comment events were incorrectly granted "trusted" status based solely on the repository relationship. The fix enforces stricter identity verification (collaborator/org-membership/OWNERS) for commenters while maintaining the existing fast-track for standard PR events.

Suggested reviewers focus

ACL Logic: Closely inspect the conditions that distinguish issue_comment from pull_request and check_run events to ensure no legitimate workflows are broken while the bypass is closed.
Regression Tests: Verify that the added tests effectively simulate the unauthorized comment scenario and confirm that the ACL now correctly rejects untrusted senders in a same-repo context.

Generated by Pipelines-as-Code LLM Analysis

pipelines-as-code · 2026-04-13T05:21:22Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	2	Likely focused on ACL logic and test files; specific fix for #2664.
Logic complexity	3	Introduces conditional trust logic based on event types (`issue_comment` vs `pull_request`).
Risk	4	Modifies security-critical Access Control List (ACL) logic to prevent permission bypasses.
Cross-cutting	2	Primarily impacts the auth/ACL layer and event handling logic.
Test coverage	2	Commit message explicitly mentions the addition of regression coverage.

Overall difficulty: Moderate

Summary

This PR fixes a security loophole where issue_comment senders could bypass ACL checks if the PR originated from the same repository. It forces comment-based triggers through standard collaborator/OWNERS verification while preserving existing "same-repo" trust for automated PR flows.

Suggested reviewers focus

ACL Logic: Ensure the distinction between pull_request events and issue_comment events is robust and doesn't accidentally block legitimate re-request flows.
Regression Tests: Verify that the new tests specifically simulate an unauthorized user commenting on a same-repo PR to confirm the bypass is closed.

Generated by Pipelines-as-Code LLM Analysis

pipelines-as-code · 2026-04-13T05:53:40Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	2	Likely a small-to-moderate diff focused on conditional logic and expanded test cases.
Logic complexity	4	nuanced handling of event types (`issue_comment` vs `pull_request`) and trust contexts.
Risk	5	Directly modifies Access Control Logic (ACL). High impact on security and user permissions.
Cross-cutting	3	Spans event info parsing and policy enforcement layers.
Test coverage	5	Commit specifically mentions adding regression coverage for the ACL bypass.

Overall difficulty: Hard

Summary

This PR fixes a security/ACL vulnerability where issue_comment senders could bypass trust checks if the Pull Request originated from the same repository. It restricts the "same-repo" fast-path to pull_request events and re-requests, forcing commenters to undergo standard collaborator, organization, or OWNERS verification.

Suggested reviewers focus

ACL Logic: Closely inspect the conditional logic that distinguishes between pull_request events and issue_comment events to ensure no new bypasses are introduced.
Rerequest Flows: Verify that check_run and check_suite re-requests from the same repo still function correctly as intended.
Regression Tests: Ensure the new tests accurately simulate malicious comment events from non-collaborators in a "same-repo" scenario.

Generated by Pipelines-as-Code LLM Analysis

chmouel · 2026-04-13T06:03:51Z

The AI Analysis should update the comment, there is something buggy here... and it runs too many times.

zakisk · 2026-04-13T06:07:15Z

The AI Analysis should update the comment, there is something buggy here... and it runs too many times.

yeah, was thinking same and I think it is running on every iteration.

chmouel · 2026-04-13T07:50:44Z

caputred the issue in this jira https://redhat.atlassian.net/browse/SRVKP-11549

pipelines-as-code · 2026-04-13T07:52:52Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	1	Metadata indicates a merge commit from `main` into the feature branch.
Logic complexity	2

Generated by Pipelines-as-Code LLM Analysis

pipelines-as-code · 2026-04-13T08:18:11Z

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Dimension	Score	Rationale
Size	1	Metadata suggests a merge commit; no files changed are visible in the provided diff.
Logic complexity	1

Generated by Pipelines-as-Code LLM Analysis

Avoid granting issue_comment senders trust based solely on same-repo PR shape. Keep the same-repo fast path for pull_request events and PR rerequest flows, while forcing comment senders through collaborator, org-membership, or OWNERS checks. Add regression coverage to ensure issue_comment events do not bypass ACL and same-repo check_run/check_suite rerequests continue to work. Adjust the GitHub ok-to-test e2e to assert the untrusted comment path without waiting for MinNumberStatus=1. That wait only revalidated the original pull request status and did not prove that the issue_comment event triggered anything. The updated test now verifies that an untrusted same-repo issue_comment does not create a new PipelineRun or Repository status and emits the expected debug log. Fixes tektoncd#2664 Co-authored-by: Cursor <cursor@users.noreply.github.com> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

chmouel · 2026-04-14T10:05:17Z

FYI, this doesn't address any real security bug, it just tight things down a bit more

chmouel · 2026-04-14T10:32:28Z

This should be good to merge i believe... added a further e2e and a logging,

gemini-code-assist Bot reviewed Apr 9, 2026

View reviewed changes

zakisk approved these changes Apr 10, 2026

View reviewed changes

zakisk force-pushed the issue-2664-same-repo-shortcut-comment-sender branch from fa3a5fe to fd77c9e Compare April 10, 2026 14:21

zakisk reviewed Apr 10, 2026

View reviewed changes

Comment thread pkg/provider/github/acl.go

zakisk force-pushed the issue-2664-same-repo-shortcut-comment-sender branch from fd77c9e to a84715e Compare April 13, 2026 05:19

chmouel force-pushed the issue-2664-same-repo-shortcut-comment-sender branch 4 times, most recently from 3f47fbd to 097221f Compare April 14, 2026 09:33

chmouel force-pushed the issue-2664-same-repo-shortcut-comment-sender branch from 097221f to 7b9b4b0 Compare April 14, 2026 09:57

chmouel merged commit a6e4aab into tektoncd:main Apr 14, 2026
13 checks passed

chmouel deleted the issue-2664-same-repo-shortcut-comment-sender branch April 14, 2026 13:18

Conversation

chmouel commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description of the Change

🔗 Linked GitHub Issue

🧪 Testing Strategy

🤖 AI Assistance

✅ Submitter Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

codecov-commenter commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

pipelines-as-code Bot commented Apr 10, 2026

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Summary

Suggested reviewers focus

Uh oh!

pipelines-as-code Bot commented Apr 10, 2026

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Summary

Suggested reviewers focus

Uh oh!

pipelines-as-code Bot commented Apr 13, 2026

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Summary

Suggested reviewers focus

Uh oh!

pipelines-as-code Bot commented Apr 13, 2026

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Summary

Suggested reviewers focus

Uh oh!

chmouel commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakisk commented Apr 13, 2026

Uh oh!

chmouel commented Apr 13, 2026

Uh oh!

pipelines-as-code Bot commented Apr 13, 2026

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Uh oh!

pipelines-as-code Bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 AI Analysis - pr-complexity-rating

📊 PR Review Complexity

Uh oh!

chmouel commented Apr 14, 2026

Uh oh!

chmouel commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chmouel commented Apr 9, 2026 •

edited

Loading

codecov-commenter commented Apr 9, 2026 •

edited

Loading

chmouel commented Apr 13, 2026 •

edited

Loading

pipelines-as-code Bot commented Apr 13, 2026 •

edited

Loading