mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-09-29 22:25:59 +00:00
fix: Actions workflows triggered by comments or labels to pull requests may access secrets (#9003)
This avoids issue_comment events on pull requests to get that flag set and subsequently not get access to secrets. ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/9003): <!--number 9003 --><!--line 0 --><!--description QWN0aW9ucyB3b3JrZmxvd3MgdHJpZ2dlcmVkIGJ5IGNvbW1lbnRzIG9yIGxhYmVscyB0byBwdWxsIHJlcXVlc3RzIG1heSBhY2Nlc3Mgc2VjcmV0cw==-->Actions workflows triggered by comments or labels to pull requests may access secrets<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9003 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: BtbN <btbn@btbn.de> Co-committed-by: BtbN <btbn@btbn.de>
This commit is contained in:
parent
c258003be9
commit
cf0e697d13
3 changed files with 104 additions and 3 deletions
|
@ -343,7 +343,7 @@ func notifyIssueCommentChange(ctx context.Context, doer *user_model.User, commen
|
||||||
newNotifyInputFromIssue(comment.Issue, event).
|
newNotifyInputFromIssue(comment.Issue, event).
|
||||||
WithDoer(doer).
|
WithDoer(doer).
|
||||||
WithPayload(payload).
|
WithPayload(payload).
|
||||||
WithPullRequest(comment.Issue.PullRequest).
|
WithPullRequestData(comment.Issue.PullRequest).
|
||||||
Notify(ctx)
|
Notify(ctx)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
|
@ -102,6 +102,12 @@ func (input *notifyInput) WithPayload(payload api.Payloader) *notifyInput {
|
||||||
return input
|
return input
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// for cases like issue comments on PRs, which have the PR data, but don't run on its ref
|
||||||
|
func (input *notifyInput) WithPullRequestData(pr *issues_model.PullRequest) *notifyInput {
|
||||||
|
input.PullRequest = pr
|
||||||
|
return input
|
||||||
|
}
|
||||||
|
|
||||||
func (input *notifyInput) WithPullRequest(pr *issues_model.PullRequest) *notifyInput {
|
func (input *notifyInput) WithPullRequest(pr *issues_model.PullRequest) *notifyInput {
|
||||||
input.PullRequest = pr
|
input.PullRequest = pr
|
||||||
if input.Ref == "" {
|
if input.Ref == "" {
|
||||||
|
@ -219,7 +225,7 @@ func notify(ctx context.Context, input *notifyInput) error {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if input.PullRequest != nil {
|
if input.PullRequest != nil && !actions_module.IsDefaultBranchWorkflow(input.Event) {
|
||||||
// detect pull_request_target workflows
|
// detect pull_request_target workflows
|
||||||
baseRef := git.BranchPrefix + input.PullRequest.BaseBranch
|
baseRef := git.BranchPrefix + input.PullRequest.BaseBranch
|
||||||
baseCommit, err := gitRepo.GetCommit(baseRef)
|
baseCommit, err := gitRepo.GetCommit(baseRef)
|
||||||
|
@ -315,7 +321,7 @@ func handleWorkflows(
|
||||||
}
|
}
|
||||||
|
|
||||||
isForkPullRequest := false
|
isForkPullRequest := false
|
||||||
if pr := input.PullRequest; pr != nil {
|
if pr := input.PullRequest; pr != nil && !actions_module.IsDefaultBranchWorkflow(input.Event) {
|
||||||
switch pr.Flow {
|
switch pr.Flow {
|
||||||
case issues_model.PullRequestFlowGithub:
|
case issues_model.PullRequestFlowGithub:
|
||||||
isForkPullRequest = pr.IsFromFork()
|
isForkPullRequest = pr.IsFromFork()
|
||||||
|
|
|
@ -8,9 +8,16 @@ import (
|
||||||
|
|
||||||
actions_model "forgejo.org/models/actions"
|
actions_model "forgejo.org/models/actions"
|
||||||
"forgejo.org/models/db"
|
"forgejo.org/models/db"
|
||||||
|
issues_model "forgejo.org/models/issues"
|
||||||
|
repo_model "forgejo.org/models/repo"
|
||||||
"forgejo.org/models/unittest"
|
"forgejo.org/models/unittest"
|
||||||
|
user_model "forgejo.org/models/user"
|
||||||
|
actions_module "forgejo.org/modules/actions"
|
||||||
|
"forgejo.org/modules/git"
|
||||||
|
api "forgejo.org/modules/structs"
|
||||||
webhook_module "forgejo.org/modules/webhook"
|
webhook_module "forgejo.org/modules/webhook"
|
||||||
|
|
||||||
|
"code.forgejo.org/forgejo/runner/v9/act/jobparser"
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
)
|
)
|
||||||
|
@ -49,3 +56,91 @@ func Test_SkipPullRequestEvent(t *testing.T) {
|
||||||
unittest.AssertSuccessfulInsert(t, run)
|
unittest.AssertSuccessfulInsert(t, run)
|
||||||
assert.True(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA))
|
assert.True(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func Test_IssueCommentOnForkPullRequestEvent(t *testing.T) {
|
||||||
|
require.NoError(t, unittest.PrepareTestDatabase())
|
||||||
|
|
||||||
|
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
|
||||||
|
doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
|
||||||
|
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3})
|
||||||
|
require.NoError(t, pr.LoadIssue(db.DefaultContext))
|
||||||
|
|
||||||
|
require.True(t, pr.IsFromFork())
|
||||||
|
|
||||||
|
commit := &git.Commit{
|
||||||
|
ID: git.MustIDFromString("0000000000000000000000000000000000000000"),
|
||||||
|
CommitMessage: "test",
|
||||||
|
}
|
||||||
|
detectedWorkflows := []*actions_module.DetectedWorkflow{
|
||||||
|
{
|
||||||
|
TriggerEvent: &jobparser.Event{
|
||||||
|
Name: "issue_comment",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
input := ¬ifyInput{
|
||||||
|
Repo: repo,
|
||||||
|
Doer: doer,
|
||||||
|
Event: webhook_module.HookEventIssueComment,
|
||||||
|
PullRequest: pr,
|
||||||
|
Payload: &api.IssueCommentPayload{},
|
||||||
|
}
|
||||||
|
|
||||||
|
unittest.AssertSuccessfulDelete(t, &actions_model.ActionRun{RepoID: repo.ID})
|
||||||
|
|
||||||
|
err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "")
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{
|
||||||
|
RepoID: repo.ID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.Len(t, runs, 1)
|
||||||
|
|
||||||
|
assert.Equal(t, webhook_module.HookEventIssueComment, runs[0].Event)
|
||||||
|
assert.False(t, runs[0].IsForkPullRequest)
|
||||||
|
}
|
||||||
|
|
||||||
|
func Test_OpenForkPullRequestEvent(t *testing.T) {
|
||||||
|
require.NoError(t, unittest.PrepareTestDatabase())
|
||||||
|
|
||||||
|
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
|
||||||
|
doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
|
||||||
|
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3})
|
||||||
|
require.NoError(t, pr.LoadIssue(db.DefaultContext))
|
||||||
|
|
||||||
|
require.True(t, pr.IsFromFork())
|
||||||
|
|
||||||
|
commit := &git.Commit{
|
||||||
|
ID: git.MustIDFromString("0000000000000000000000000000000000000000"),
|
||||||
|
CommitMessage: "test",
|
||||||
|
}
|
||||||
|
detectedWorkflows := []*actions_module.DetectedWorkflow{
|
||||||
|
{
|
||||||
|
TriggerEvent: &jobparser.Event{
|
||||||
|
Name: "pull_request",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
input := ¬ifyInput{
|
||||||
|
Repo: repo,
|
||||||
|
Doer: doer,
|
||||||
|
Event: webhook_module.HookEventPullRequest,
|
||||||
|
PullRequest: pr,
|
||||||
|
Payload: &api.PullRequestPayload{},
|
||||||
|
}
|
||||||
|
|
||||||
|
unittest.AssertSuccessfulDelete(t, &actions_model.ActionRun{RepoID: repo.ID})
|
||||||
|
|
||||||
|
err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "")
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{
|
||||||
|
RepoID: repo.ID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.Len(t, runs, 1)
|
||||||
|
|
||||||
|
assert.Equal(t, webhook_module.HookEventPullRequest, runs[0].Event)
|
||||||
|
assert.True(t, runs[0].IsForkPullRequest)
|
||||||
|
}
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue