fix(ci): resolve the actions of a merge with the rights of the merger
A merge was recorded as pushed by the owner, which reads every repository, so a token limited to some repositories could run a workflow using a private action of another one by merging it. OnMerge records the merging principal as the pusher the actions are resolved on behalf of; the actor shown on the run stays the owner, as GitHub shows it.
- commit
- d024c7fa48490795cbb84df9a30cdbbabd0b9b0e
- author
- brice · · 2026-10-13 11:34 UTC
- parent
- 3d22057
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +80 -3
M internal/ports/ci.go +5 -0
| @@ -193,6 +193,11 @@ type CILogStore interface { | |||
| 193 | 193 | // errors never reach the push. | |
| 194 | 194 | type WorkflowTrigger interface { | |
| 195 | 195 | OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) | |
| 196 | + | // OnMerge is OnPush for the move of a target branch by a merge. GitHub shows | |
| 197 | + | // the owner as the actor of the push of a merge, whoever merged; but the | |
| 198 | + | // rights the run's actions are resolved with are those of merger, the | |
| 199 | + | // principal that merged, never the owner's. | |
| 200 | + | OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) | |
| 196 | 201 | } | |
| 197 | 202 | ||
| 198 | 203 | // CIGit is what planning needs from Git beyond RepoReader and MergeGit. | |
| @@ -193,6 +193,11 @@ type CILogStore interface { | |||||
| 193 | // errors never reach the push. | 193 | // errors never reach the push. | ||
| 194 | type WorkflowTrigger interface { | 194 | type WorkflowTrigger interface { | ||
| 195 | OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) | 195 | OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) | ||
| 196 | + | // OnMerge is OnPush for the move of a target branch by a merge. GitHub shows | |||
| 197 | + | // the owner as the actor of the push of a merge, whoever merged; but the | |||
| 198 | + | // rights the run's actions are resolved with are those of merger, the | |||
| 199 | + | // principal that merged, never the owner's. | |||
| 200 | + | OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) | |||
| 196 | } | 201 | } | ||
| 197 | 202 | ||||
| 198 | // CIGit is what planning needs from Git beyond RepoReader and MergeGit. | 203 | // CIGit is what planning needs from Git beyond RepoReader and MergeGit. | ||
M internal/ports/mocks/WorkflowTrigger.go +58 -0
| @@ -47,6 +47,64 @@ func (_m *WorkflowTrigger) EXPECT() *WorkflowTrigger_Expecter { | |||
| 47 | 47 | return &WorkflowTrigger_Expecter{mock: &_m.Mock} | |
| 48 | 48 | } | |
| 49 | 49 | ||
| 50 | + | // OnMerge provides a mock function for the type WorkflowTrigger | |
| 51 | + | func (_mock *WorkflowTrigger) OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) { | |
| 52 | + | _mock.Called(ctx, repo, updates, merger) | |
| 53 | + | return | |
| 54 | + | } | |
| 55 | + | ||
| 56 | + | // WorkflowTrigger_OnMerge_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'OnMerge' | |
| 57 | + | type WorkflowTrigger_OnMerge_Call struct { | |
| 58 | + | *mock.Call | |
| 59 | + | } | |
| 60 | + | ||
| 61 | + | // OnMerge is a helper method to define mock.On call | |
| 62 | + | // - ctx context.Context | |
| 63 | + | // - repo domain.RepoName | |
| 64 | + | // - updates []domain.RefUpdate | |
| 65 | + | // - merger domain.Principal | |
| 66 | + | func (_e *WorkflowTrigger_Expecter) OnMerge(ctx any, repo any, updates any, merger any) *WorkflowTrigger_OnMerge_Call { | |
| 67 | + | return &WorkflowTrigger_OnMerge_Call{Call: _e.mock.On("OnMerge", ctx, repo, updates, merger)} | |
| 68 | + | } | |
| 69 | + | ||
| 70 | + | func (_c *WorkflowTrigger_OnMerge_Call) Run(run func(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal)) *WorkflowTrigger_OnMerge_Call { | |
| 71 | + | _c.Call.Run(func(args mock.Arguments) { | |
| 72 | + | var arg0 context.Context | |
| 73 | + | if args[0] != nil { | |
| 74 | + | arg0 = args[0].(context.Context) | |
| 75 | + | } | |
| 76 | + | var arg1 domain.RepoName | |
| 77 | + | if args[1] != nil { | |
| 78 | + | arg1 = args[1].(domain.RepoName) | |
| 79 | + | } | |
| 80 | + | var arg2 []domain.RefUpdate | |
| 81 | + | if args[2] != nil { | |
| 82 | + | arg2 = args[2].([]domain.RefUpdate) | |
| 83 | + | } | |
| 84 | + | var arg3 domain.Principal | |
| 85 | + | if args[3] != nil { | |
| 86 | + | arg3 = args[3].(domain.Principal) | |
| 87 | + | } | |
| 88 | + | run( | |
| 89 | + | arg0, | |
| 90 | + | arg1, | |
| 91 | + | arg2, | |
| 92 | + | arg3, | |
| 93 | + | ) | |
| 94 | + | }) | |
| 95 | + | return _c | |
| 96 | + | } | |
| 97 | + | ||
| 98 | + | func (_c *WorkflowTrigger_OnMerge_Call) Return() *WorkflowTrigger_OnMerge_Call { | |
| 99 | + | _c.Call.Return() | |
| 100 | + | return _c | |
| 101 | + | } | |
| 102 | + | ||
| 103 | + | func (_c *WorkflowTrigger_OnMerge_Call) RunAndReturn(run func(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal)) *WorkflowTrigger_OnMerge_Call { | |
| 104 | + | _c.Run(run) | |
| 105 | + | return _c | |
| 106 | + | } | |
| 107 | + | ||
| 50 | 108 | // OnPush provides a mock function for the type WorkflowTrigger | |
| 51 | 109 | func (_mock *WorkflowTrigger) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | |
| 52 | 110 | _mock.Called(ctx, repo, updates, pusher) | |
| @@ -47,6 +47,64 @@ func (_m *WorkflowTrigger) EXPECT() *WorkflowTrigger_Expecter { | |||||
| 47 | return &WorkflowTrigger_Expecter{mock: &_m.Mock} | 47 | return &WorkflowTrigger_Expecter{mock: &_m.Mock} | ||
| 48 | } | 48 | } | ||
| 49 | 49 | ||||
| 50 | + | // OnMerge provides a mock function for the type WorkflowTrigger | |||
| 51 | + | func (_mock *WorkflowTrigger) OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) { | |||
| 52 | + | _mock.Called(ctx, repo, updates, merger) | |||
| 53 | + | return | |||
| 54 | + | } | |||
| 55 | + | ||||
| 56 | + | // WorkflowTrigger_OnMerge_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'OnMerge' | |||
| 57 | + | type WorkflowTrigger_OnMerge_Call struct { | |||
| 58 | + | *mock.Call | |||
| 59 | + | } | |||
| 60 | + | ||||
| 61 | + | // OnMerge is a helper method to define mock.On call | |||
| 62 | + | // - ctx context.Context | |||
| 63 | + | // - repo domain.RepoName | |||
| 64 | + | // - updates []domain.RefUpdate | |||
| 65 | + | // - merger domain.Principal | |||
| 66 | + | func (_e *WorkflowTrigger_Expecter) OnMerge(ctx any, repo any, updates any, merger any) *WorkflowTrigger_OnMerge_Call { | |||
| 67 | + | return &WorkflowTrigger_OnMerge_Call{Call: _e.mock.On("OnMerge", ctx, repo, updates, merger)} | |||
| 68 | + | } | |||
| 69 | + | ||||
| 70 | + | func (_c *WorkflowTrigger_OnMerge_Call) Run(run func(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal)) *WorkflowTrigger_OnMerge_Call { | |||
| 71 | + | _c.Call.Run(func(args mock.Arguments) { | |||
| 72 | + | var arg0 context.Context | |||
| 73 | + | if args[0] != nil { | |||
| 74 | + | arg0 = args[0].(context.Context) | |||
| 75 | + | } | |||
| 76 | + | var arg1 domain.RepoName | |||
| 77 | + | if args[1] != nil { | |||
| 78 | + | arg1 = args[1].(domain.RepoName) | |||
| 79 | + | } | |||
| 80 | + | var arg2 []domain.RefUpdate | |||
| 81 | + | if args[2] != nil { | |||
| 82 | + | arg2 = args[2].([]domain.RefUpdate) | |||
| 83 | + | } | |||
| 84 | + | var arg3 domain.Principal | |||
| 85 | + | if args[3] != nil { | |||
| 86 | + | arg3 = args[3].(domain.Principal) | |||
| 87 | + | } | |||
| 88 | + | run( | |||
| 89 | + | arg0, | |||
| 90 | + | arg1, | |||
| 91 | + | arg2, | |||
| 92 | + | arg3, | |||
| 93 | + | ) | |||
| 94 | + | }) | |||
| 95 | + | return _c | |||
| 96 | + | } | |||
| 97 | + | ||||
| 98 | + | func (_c *WorkflowTrigger_OnMerge_Call) Return() *WorkflowTrigger_OnMerge_Call { | |||
| 99 | + | _c.Call.Return() | |||
| 100 | + | return _c | |||
| 101 | + | } | |||
| 102 | + | ||||
| 103 | + | func (_c *WorkflowTrigger_OnMerge_Call) RunAndReturn(run func(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal)) *WorkflowTrigger_OnMerge_Call { | |||
| 104 | + | _c.Run(run) | |||
| 105 | + | return _c | |||
| 106 | + | } | |||
| 107 | + | ||||
| 50 | // OnPush provides a mock function for the type WorkflowTrigger | 108 | // OnPush provides a mock function for the type WorkflowTrigger | ||
| 51 | func (_mock *WorkflowTrigger) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | 109 | func (_mock *WorkflowTrigger) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | ||
| 52 | _mock.Called(ctx, repo, updates, pusher) | 110 | _mock.Called(ctx, repo, updates, pusher) | ||
M internal/usecase/ci_trigger.go +14 -1
| @@ -45,6 +45,19 @@ type ciTrigger struct { | |||
| 45 | 45 | // a worker plans them. A full queue drops the trigger, logged at error: a push | |
| 46 | 46 | // is never slowed down or failed by CI. | |
| 47 | 47 | func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | |
| 48 | + | c.enqueue(ctx, repo, updates, authorOf(pusher), domain.PusherOf(pusher)) | |
| 49 | + | } | |
| 50 | + | ||
| 51 | + | // OnMerge queues the move of a target branch by a merge. The actor of the run is | |
| 52 | + | // the owner, as GitHub shows; the pusher recorded with it, whose rights the | |
| 53 | + | // actions of other repositories are resolved with, is the principal that merged: | |
| 54 | + | // a token limited to some repositories does not gain the owner's reach by | |
| 55 | + | // merging. | |
| 56 | + | func (c *CI) OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) { | |
| 57 | + | c.enqueue(ctx, repo, updates, authorOf(domain.Owner()), domain.PusherOf(merger)) | |
| 58 | + | } | |
| 59 | + | ||
| 60 | + | func (c *CI) enqueue(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, actor string, pusher domain.CIPusher) { | |
| 48 | 61 | var kept []domain.RefUpdate | |
| 49 | 62 | for _, u := range updates { | |
| 50 | 63 | if !u.IsDelete() && (strings.HasPrefix(u.Ref, "refs/heads/") || strings.HasPrefix(u.Ref, "refs/tags/")) { | |
| @@ -54,7 +67,7 @@ func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain. | |||
| 54 | 67 | if len(kept) == 0 { | |
| 55 | 68 | return | |
| 56 | 69 | } | |
| 57 | - | t := ciTrigger{repo: repo, updates: kept, actor: authorOf(pusher), pusher: domain.PusherOf(pusher), at: c.now(), log: zerowrap.FromCtx(ctx)} | |
| 70 | + | t := ciTrigger{repo: repo, updates: kept, actor: actor, pusher: pusher, at: c.now(), log: zerowrap.FromCtx(ctx)} | |
| 58 | 71 | c.mu.Lock() | |
| 59 | 72 | closed := c.closed | |
| 60 | 73 | c.mu.Unlock() | |
| @@ -45,6 +45,19 @@ type ciTrigger struct { | |||||
| 45 | // a worker plans them. A full queue drops the trigger, logged at error: a push | 45 | // a worker plans them. A full queue drops the trigger, logged at error: a push | ||
| 46 | // is never slowed down or failed by CI. | 46 | // is never slowed down or failed by CI. | ||
| 47 | func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | 47 | func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, pusher domain.Principal) { | ||
| 48 | + | c.enqueue(ctx, repo, updates, authorOf(pusher), domain.PusherOf(pusher)) | |||
| 49 | + | } | |||
| 50 | + | ||||
| 51 | + | // OnMerge queues the move of a target branch by a merge. The actor of the run is | |||
| 52 | + | // the owner, as GitHub shows; the pusher recorded with it, whose rights the | |||
| 53 | + | // actions of other repositories are resolved with, is the principal that merged: | |||
| 54 | + | // a token limited to some repositories does not gain the owner's reach by | |||
| 55 | + | // merging. | |||
| 56 | + | func (c *CI) OnMerge(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, merger domain.Principal) { | |||
| 57 | + | c.enqueue(ctx, repo, updates, authorOf(domain.Owner()), domain.PusherOf(merger)) | |||
| 58 | + | } | |||
| 59 | + | ||||
| 60 | + | func (c *CI) enqueue(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, actor string, pusher domain.CIPusher) { | |||
| 48 | var kept []domain.RefUpdate | 61 | var kept []domain.RefUpdate | ||
| 49 | for _, u := range updates { | 62 | for _, u := range updates { | ||
| 50 | if !u.IsDelete() && (strings.HasPrefix(u.Ref, "refs/heads/") || strings.HasPrefix(u.Ref, "refs/tags/")) { | 63 | if !u.IsDelete() && (strings.HasPrefix(u.Ref, "refs/heads/") || strings.HasPrefix(u.Ref, "refs/tags/")) { | ||
| @@ -54,7 +67,7 @@ func (c *CI) OnPush(ctx context.Context, repo domain.RepoName, updates []domain. | |||||
| 54 | if len(kept) == 0 { | 67 | if len(kept) == 0 { | ||
| 55 | return | 68 | return | ||
| 56 | } | 69 | } | ||
| 57 | - | t := ciTrigger{repo: repo, updates: kept, actor: authorOf(pusher), pusher: domain.PusherOf(pusher), at: c.now(), log: zerowrap.FromCtx(ctx)} | 70 | + | t := ciTrigger{repo: repo, updates: kept, actor: actor, pusher: pusher, at: c.now(), log: zerowrap.FromCtx(ctx)} |
| 58 | c.mu.Lock() | 71 | c.mu.Lock() | ||
| 59 | closed := c.closed | 72 | closed := c.closed | ||
| 60 | c.mu.Unlock() | 73 | c.mu.Unlock() | ||
M internal/usecase/mergerequests.go +3 -2
| @@ -814,8 +814,9 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma | |||
| 814 | 814 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | |
| 815 | 815 | }) | |
| 816 | 816 | if m.ci != nil { | |
| 817 | - | // GitHub runs the "push" workflows of a merge, with the owner as pusher. | |
| 818 | - | m.ci.OnPush(ctx, name, updates[:1], domain.Owner()) | |
| 817 | + | // GitHub runs the "push" workflows of a merge, with the owner as actor; | |
| 818 | + | // the run is trusted with what the merging principal may read. | |
| 819 | + | m.ci.OnMerge(ctx, name, updates[:1], p) | |
| 819 | 820 | } | |
| 820 | 821 | return mr, deleted, nil | |
| 821 | 822 | } | |
| @@ -814,8 +814,9 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma | |||||
| 814 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | 814 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | ||
| 815 | }) | 815 | }) | ||
| 816 | if m.ci != nil { | 816 | if m.ci != nil { | ||
| 817 | - | // GitHub runs the "push" workflows of a merge, with the owner as pusher. | 817 | + | // GitHub runs the "push" workflows of a merge, with the owner as actor; |
| 818 | - | m.ci.OnPush(ctx, name, updates[:1], domain.Owner()) | 818 | + | // the run is trusted with what the merging principal may read. |
| 819 | + | m.ci.OnMerge(ctx, name, updates[:1], p) | |||
| 819 | } | 820 | } | ||
| 820 | return mr, deleted, nil | 821 | return mr, deleted, nil | ||
| 821 | } | 822 | } | ||