fix(ci): trust pull_request runs only for heads pushed by a trusted principal (#38)
- commit
- 626ef13a855cf0fea11cf86b51fdc442dc97e118
- author
- brice · · 2026-10-16 08:07 UTC
- committer
- bnema · · 2026-10-16 08:07 UTC
- parent
- 3237aa5
- signature
- SSH Unverified
SHA256:VOCA5lYARHcbKDZ7GhTV45VmcjWnJ6ZQIBazj17LivAThe key is unknown
7 files · +490 -10
M internal/adapters/secretbox/secretbox_test.go +3 -1
| @@ -31,7 +31,9 @@ func TestSealOpenRoundTrip(t *testing.T) { | |||
| 31 | 31 | aad := domain.SecretAAD(domain.RepoScope(repo), "TOKEN") | |
| 32 | 32 | for _, plain := range []string{"x", "multi\nline\r\nvalue", strings.Repeat("é", 20000), "\x00\xff"} { | |
| 33 | 33 | sealed := b.Seal([]byte(plain), aad) | |
| 34 | - | assert.NotContains(t, string(sealed), plain, "the value is not stored in clear") | |
| 34 | + | if len(plain) >= 8 { // a shorter value can turn up in random bytes by chance | |
| 35 | + | assert.NotContains(t, string(sealed), plain, "the value is not stored in clear") | |
| 36 | + | } | |
| 35 | 37 | got, err := b.Open(sealed, aad) | |
| 36 | 38 | require.NoError(t, err) | |
| 37 | 39 | assert.Equal(t, plain, string(got)) | |
| @@ -31,7 +31,9 @@ func TestSealOpenRoundTrip(t *testing.T) { | |||||
| 31 | aad := domain.SecretAAD(domain.RepoScope(repo), "TOKEN") | 31 | aad := domain.SecretAAD(domain.RepoScope(repo), "TOKEN") | ||
| 32 | for _, plain := range []string{"x", "multi\nline\r\nvalue", strings.Repeat("é", 20000), "\x00\xff"} { | 32 | for _, plain := range []string{"x", "multi\nline\r\nvalue", strings.Repeat("é", 20000), "\x00\xff"} { | ||
| 33 | sealed := b.Seal([]byte(plain), aad) | 33 | sealed := b.Seal([]byte(plain), aad) | ||
| 34 | - | assert.NotContains(t, string(sealed), plain, "the value is not stored in clear") | 34 | + | if len(plain) >= 8 { // a shorter value can turn up in random bytes by chance |
| 35 | + | assert.NotContains(t, string(sealed), plain, "the value is not stored in clear") | |||
| 36 | + | } | |||
| 35 | got, err := b.Open(sealed, aad) | 37 | got, err := b.Open(sealed, aad) | ||
| 36 | require.NoError(t, err) | 38 | require.NoError(t, err) | ||
| 37 | assert.Equal(t, plain, string(got)) | 39 | assert.Equal(t, plain, string(got)) | ||
M internal/app/ci_pull_test.go +202 -0
| @@ -307,6 +307,208 @@ func TestPullRequestTrustFollowsTheActor(t *testing.T) { | |||
| 307 | 307 | assert.False(t, job.GetTrusted()) | |
| 308 | 308 | } | |
| 309 | 309 | ||
| 310 | + | // CI-4 D1: the workflow of a pull_request run comes from the head, so the run | |
| 311 | + | // is trusted only if the head was pushed by a trusted principal too. | |
| 312 | + | func TestPullRequestTrustFollowsTheHeadsPusher(t *testing.T) { | |
| 313 | + | c := newCIHarness(t, nil) | |
| 314 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |
| 315 | + | c.push("main", pullWF(" types: [opened, synchronize, closed]", ""), "") | |
| 316 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite, domain.ScopeMRWrite), []string{"agent/"}, "proj") | |
| 317 | + | agentPush := func(branch, file string) string { | |
| 318 | + | c.writeFile(c.wt, file, []byte(file+"\n")) | |
| 319 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), branch) | |
| 320 | + | return strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |
| 321 | + | } | |
| 322 | + | ||
| 323 | + | // The owner opens the request of a branch the agent pushed: not trusted. | |
| 324 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/x") | |
| 325 | + | agentPush("agent/x", "x.txt") | |
| 326 | + | n := c.openMR(c.root, "agent/x") | |
| 327 | + | job := c.finish(runner) | |
| 328 | + | assert.False(t, job.GetTrusted(), "the agent's workflow does not get the owner's secrets") | |
| 329 | + | run := c.runs()[0] | |
| 330 | + | assert.False(t, run.GetTrusted()) | |
| 331 | + | assert.Equal(t, "root", run.GetActor(), "the actor is still who opened it") | |
| 332 | + | ||
| 333 | + | // The owner pushes on top: the synchronize is trusted. | |
| 334 | + | c.push("agent/x", map[string]string{"y.txt": "y\n"}, "") | |
| 335 | + | assert.True(t, c.finish(runner).GetTrusted()) | |
| 336 | + | ||
| 337 | + | // The agent pushes again: not trusted. | |
| 338 | + | agentPush("agent/x", "z.txt") | |
| 339 | + | assert.False(t, c.finish(runner).GetTrusted()) | |
| 340 | + | ||
| 341 | + | // The owner closes it, the head being the agent's: not trusted. | |
| 342 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |
| 343 | + | require.NoError(t, err) | |
| 344 | + | job = c.finish(runner) | |
| 345 | + | assert.Equal(t, "closed", c.ghContext(job)["event"].(map[string]any)["action"]) | |
| 346 | + | assert.False(t, job.GetTrusted()) | |
| 347 | + | ||
| 348 | + | // The owner's own branch: trusted. | |
| 349 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "own", "main") | |
| 350 | + | c.push("own", map[string]string{"o.txt": "o\n"}, "") | |
| 351 | + | c.openMR(c.root, "own") | |
| 352 | + | assert.True(t, c.finish(runner).GetTrusted()) | |
| 353 | + | ||
| 354 | + | // A head whose record names another commit (the branch moved since, or the | |
| 355 | + | // owner's push of it was refused), or that has none: not trusted. | |
| 356 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "other", "main") | |
| 357 | + | c.push("other", map[string]string{"p.txt": "p\n"}, "") | |
| 358 | + | require.NoError(t, c.forge.store.CI().PutRefTip(c.ctx, domain.CIRefTip{ | |
| 359 | + | Repo: c.name("proj"), Ref: "refs/heads/other", Sha: "1111111111111111111111111111111111111111", Actor: "root", Trusted: true, | |
| 360 | + | })) | |
| 361 | + | c.openMR(c.root, "other") | |
| 362 | + | assert.False(t, c.finish(runner).GetTrusted(), "the record names another commit") | |
| 363 | + | ||
| 364 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "none", "main") | |
| 365 | + | c.push("none", map[string]string{"q.txt": "q\n"}, "") | |
| 366 | + | require.NoError(t, c.forge.store.CI().DeleteRefTip(c.ctx, c.name("proj"), "refs/heads/none")) | |
| 367 | + | c.openMR(c.root, "none") | |
| 368 | + | assert.False(t, c.finish(runner).GetTrusted(), "no record") | |
| 369 | + | } | |
| 370 | + | ||
| 371 | + | // Merging takes the head into the target: the closing of a merged request has | |
| 372 | + | // the merger's trust, even with the source branch (and its record) deleted. | |
| 373 | + | func TestPullRequestMergedByTheOwnerIsTrusted(t *testing.T) { | |
| 374 | + | c := newCIHarness(t, nil) | |
| 375 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |
| 376 | + | c.push("main", pullWF(" types: [closed]", ""), "") | |
| 377 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |
| 378 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/m") | |
| 379 | + | c.writeFile(c.wt, "m.txt", []byte("m\n")) | |
| 380 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/m") | |
| 381 | + | n := c.openMR(c.root, "agent/m") | |
| 382 | + | c.waitFor("the head ref", func() bool { return c.ref(domain.PullHeadRef(n)) != "" }) | |
| 383 | + | _, err := c.root.mr.MergeMergeRequest(c.ctx, connect.NewRequest(&pb.MergeMergeRequestRequest{ | |
| 384 | + | Repo: "proj", Number: n, HeadId: c.ref(domain.PullHeadRef(n)), DeleteSourceBranch: true, | |
| 385 | + | })) | |
| 386 | + | require.NoError(t, err) | |
| 387 | + | job := c.finish(runner) | |
| 388 | + | assert.Equal(t, true, c.ghContext(job)["event"].(map[string]any)["pull_request"].(map[string]any)["merged"]) | |
| 389 | + | assert.True(t, job.GetTrusted()) | |
| 390 | + | } | |
| 391 | + | ||
| 392 | + | // openMRInto opens a merge request from source into target as the owner. | |
| 393 | + | func (c *ciHarness) openMRInto(source, target string) int64 { | |
| 394 | + | c.t.Helper() | |
| 395 | + | res, err := c.root.mr.CreateMergeRequest(c.ctx, connect.NewRequest(&pb.CreateMergeRequestRequest{ | |
| 396 | + | Repo: c.repo, Title: "Title of " + source, SourceBranch: source, TargetBranch: target, | |
| 397 | + | })) | |
| 398 | + | require.NoError(c.t, err) | |
| 399 | + | return res.Msg.GetMergeRequest().GetNumber() | |
| 400 | + | } | |
| 401 | + | ||
| 402 | + | // The merge commit carries the target's files too: the base of the event must | |
| 403 | + | // have been pushed by a trusted principal as well, for every activity. | |
| 404 | + | func TestPullRequestTrustFollowsTheBasesPusher(t *testing.T) { | |
| 405 | + | c, hook := newHookedHarness(t) | |
| 406 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |
| 407 | + | c.push("main", pullWF(" types: [opened, synchronize, closed]", ""), "") | |
| 408 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |
| 409 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/base") | |
| 410 | + | c.writeFile(c.wt, "a.txt", []byte("a\n")) | |
| 411 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |
| 412 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat", "main") | |
| 413 | + | c.push("feat", map[string]string{"f.txt": "f\n"}, "") | |
| 414 | + | ||
| 415 | + | // The owner's branch into the agent's: not trusted, opened or synchronized. | |
| 416 | + | n := c.openMRInto("feat", "agent/base") | |
| 417 | + | assert.False(t, c.finish(runner).GetTrusted(), "opened on the agent's target") | |
| 418 | + | c.push("feat", map[string]string{"g.txt": "g\n"}, "") | |
| 419 | + | job := c.finish(runner) | |
| 420 | + | assert.Equal(t, "synchronize", c.ghContext(job)["event"].(map[string]any)["action"]) | |
| 421 | + | assert.False(t, job.GetTrusted(), "a synchronize by the owner on the agent's target") | |
| 422 | + | ||
| 423 | + | // The owner pushes the target: its tip is the owner's, so are the next runs. | |
| 424 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |
| 425 | + | c.push("agent/base", map[string]string{"b.txt": "b\n"}, "") | |
| 426 | + | c.mustGit(c.wt, "checkout", "-q", "feat") | |
| 427 | + | c.push("feat", map[string]string{"h.txt": "h\n"}, "") | |
| 428 | + | assert.True(t, c.finish(runner).GetTrusted(), "owner's source, owner's target") | |
| 429 | + | ||
| 430 | + | // The agent pushes the target again; the owner closes: the run is the last | |
| 431 | + | // merge (refreshed on the agent's tip), not trusted. | |
| 432 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |
| 433 | + | c.writeFile(c.wt, "c.txt", []byte("c\n")) | |
| 434 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |
| 435 | + | agentTip := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |
| 436 | + | c.waitFor("the merge on the agent's tip", func() bool { | |
| 437 | + | m := c.ref(domain.PullMergeRef(n)) | |
| 438 | + | return m != "" && strings.HasPrefix(c.parents(m), agentTip+" ") | |
| 439 | + | }) | |
| 440 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |
| 441 | + | require.NoError(t, err) | |
| 442 | + | job = c.finish(runner) | |
| 443 | + | assert.Equal(t, "closed", c.ghContext(job)["event"].(map[string]any)["action"]) | |
| 444 | + | assert.False(t, job.GetTrusted(), "closed on a merge of the agent's target") | |
| 445 | + | ||
| 446 | + | // The trust is that of the snapshot: the owner moving the agent's target | |
| 447 | + | // after the opening does not make the opening trusted. | |
| 448 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat2", "main") | |
| 449 | + | c.push("feat2", map[string]string{"k.txt": "k\n"}, "") | |
| 450 | + | reached, release := hook.block(t, "event") | |
| 451 | + | c.openMRInto("feat2", "agent/base") | |
| 452 | + | await(t, reached, "the opening event") | |
| 453 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |
| 454 | + | c.push("agent/base", map[string]string{"d.txt": "d\n"}, "") | |
| 455 | + | release() | |
| 456 | + | job = c.finish(runner) | |
| 457 | + | assert.Equal(t, agentTip+" ", c.parents(c.ghContext(job)["sha"].(string))[:41], "the run is for the snapshot's base") | |
| 458 | + | assert.False(t, job.GetTrusted()) | |
| 459 | + | ||
| 460 | + | // And the other way: the agent moving the owner's target after the opening | |
| 461 | + | // does not make it untrusted. | |
| 462 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat3", "main") | |
| 463 | + | c.push("feat3", map[string]string{"m.txt": "m\n"}, "") | |
| 464 | + | ownerTip := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "agent/base")) | |
| 465 | + | reached, release = hook.block(t, "event") | |
| 466 | + | c.openMRInto("feat3", "agent/base") | |
| 467 | + | await(t, reached, "the opening event") | |
| 468 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |
| 469 | + | c.writeFile(c.wt, "e.txt", []byte("e\n")) | |
| 470 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |
| 471 | + | release() | |
| 472 | + | job = c.finish(runner) | |
| 473 | + | assert.Equal(t, ownerTip+" ", c.parents(c.ghContext(job)["sha"].(string))[:41]) | |
| 474 | + | assert.True(t, job.GetTrusted()) | |
| 475 | + | } | |
| 476 | + | ||
| 477 | + | // A closed request runs its last merge commit, which can be of an older head | |
| 478 | + | // than the one its closing is for: the trust of that head says nothing of it. | |
| 479 | + | func TestPullRequestClosedOnAMergeOfAnOlderHeadIsNotTrusted(t *testing.T) { | |
| 480 | + | c := newCIHarness(t, nil) | |
| 481 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |
| 482 | + | files := pullWF(" types: [closed]", "") | |
| 483 | + | files["c.txt"] = "base\n" | |
| 484 | + | c.push("main", files, "") | |
| 485 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |
| 486 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/o") | |
| 487 | + | c.writeFile(c.wt, "x.txt", []byte("x\n")) | |
| 488 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/o") | |
| 489 | + | agentHead := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |
| 490 | + | n := c.openMR(c.root, "agent/o") | |
| 491 | + | ||
| 492 | + | // The target moves, the agent's head still merges; then the owner pushes a | |
| 493 | + | // head that conflicts: the merge ref keeps the merge of the agent's head. | |
| 494 | + | c.mustGit(c.wt, "checkout", "-q", "main") | |
| 495 | + | main2 := c.push("main", map[string]string{"c.txt": "main\n"}, "") | |
| 496 | + | c.waitFor("the merge of the agent's head on the new target", func() bool { | |
| 497 | + | m := c.ref(domain.PullMergeRef(n)) | |
| 498 | + | return m != "" && c.parents(m) == main2+" "+agentHead | |
| 499 | + | }) | |
| 500 | + | c.mustGit(c.wt, "checkout", "-q", "agent/o") | |
| 501 | + | ownerHead := c.push("agent/o", map[string]string{"c.txt": "owner\n"}, "") | |
| 502 | + | c.waitFor("the head ref", func() bool { return c.ref(domain.PullHeadRef(n)) == ownerHead }) | |
| 503 | + | require.Equal(t, main2+" "+agentHead, c.parents(c.ref(domain.PullMergeRef(n)))) | |
| 504 | + | ||
| 505 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |
| 506 | + | require.NoError(t, err) | |
| 507 | + | job := c.finish(runner) | |
| 508 | + | assert.Equal(t, c.ref(domain.PullMergeRef(n)), c.ghContext(job)["sha"], "the run is the merge of the agent's head") | |
| 509 | + | assert.False(t, job.GetTrusted()) | |
| 510 | + | } | |
| 511 | + | ||
| 310 | 512 | func TestPullRefsAreServedHiddenAndNotPushable(t *testing.T) { | |
| 311 | 513 | c := newCIHarness(t, nil) | |
| 312 | 514 | runner := c.registerRunner("alpha", "ubuntu-latest") | |
| @@ -307,6 +307,208 @@ func TestPullRequestTrustFollowsTheActor(t *testing.T) { | |||||
| 307 | assert.False(t, job.GetTrusted()) | 307 | assert.False(t, job.GetTrusted()) | ||
| 308 | } | 308 | } | ||
| 309 | 309 | ||||
| 310 | + | // CI-4 D1: the workflow of a pull_request run comes from the head, so the run | |||
| 311 | + | // is trusted only if the head was pushed by a trusted principal too. | |||
| 312 | + | func TestPullRequestTrustFollowsTheHeadsPusher(t *testing.T) { | |||
| 313 | + | c := newCIHarness(t, nil) | |||
| 314 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |||
| 315 | + | c.push("main", pullWF(" types: [opened, synchronize, closed]", ""), "") | |||
| 316 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite, domain.ScopeMRWrite), []string{"agent/"}, "proj") | |||
| 317 | + | agentPush := func(branch, file string) string { | |||
| 318 | + | c.writeFile(c.wt, file, []byte(file+"\n")) | |||
| 319 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), branch) | |||
| 320 | + | return strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |||
| 321 | + | } | |||
| 322 | + | ||||
| 323 | + | // The owner opens the request of a branch the agent pushed: not trusted. | |||
| 324 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/x") | |||
| 325 | + | agentPush("agent/x", "x.txt") | |||
| 326 | + | n := c.openMR(c.root, "agent/x") | |||
| 327 | + | job := c.finish(runner) | |||
| 328 | + | assert.False(t, job.GetTrusted(), "the agent's workflow does not get the owner's secrets") | |||
| 329 | + | run := c.runs()[0] | |||
| 330 | + | assert.False(t, run.GetTrusted()) | |||
| 331 | + | assert.Equal(t, "root", run.GetActor(), "the actor is still who opened it") | |||
| 332 | + | ||||
| 333 | + | // The owner pushes on top: the synchronize is trusted. | |||
| 334 | + | c.push("agent/x", map[string]string{"y.txt": "y\n"}, "") | |||
| 335 | + | assert.True(t, c.finish(runner).GetTrusted()) | |||
| 336 | + | ||||
| 337 | + | // The agent pushes again: not trusted. | |||
| 338 | + | agentPush("agent/x", "z.txt") | |||
| 339 | + | assert.False(t, c.finish(runner).GetTrusted()) | |||
| 340 | + | ||||
| 341 | + | // The owner closes it, the head being the agent's: not trusted. | |||
| 342 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |||
| 343 | + | require.NoError(t, err) | |||
| 344 | + | job = c.finish(runner) | |||
| 345 | + | assert.Equal(t, "closed", c.ghContext(job)["event"].(map[string]any)["action"]) | |||
| 346 | + | assert.False(t, job.GetTrusted()) | |||
| 347 | + | ||||
| 348 | + | // The owner's own branch: trusted. | |||
| 349 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "own", "main") | |||
| 350 | + | c.push("own", map[string]string{"o.txt": "o\n"}, "") | |||
| 351 | + | c.openMR(c.root, "own") | |||
| 352 | + | assert.True(t, c.finish(runner).GetTrusted()) | |||
| 353 | + | ||||
| 354 | + | // A head whose record names another commit (the branch moved since, or the | |||
| 355 | + | // owner's push of it was refused), or that has none: not trusted. | |||
| 356 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "other", "main") | |||
| 357 | + | c.push("other", map[string]string{"p.txt": "p\n"}, "") | |||
| 358 | + | require.NoError(t, c.forge.store.CI().PutRefTip(c.ctx, domain.CIRefTip{ | |||
| 359 | + | Repo: c.name("proj"), Ref: "refs/heads/other", Sha: "1111111111111111111111111111111111111111", Actor: "root", Trusted: true, | |||
| 360 | + | })) | |||
| 361 | + | c.openMR(c.root, "other") | |||
| 362 | + | assert.False(t, c.finish(runner).GetTrusted(), "the record names another commit") | |||
| 363 | + | ||||
| 364 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "none", "main") | |||
| 365 | + | c.push("none", map[string]string{"q.txt": "q\n"}, "") | |||
| 366 | + | require.NoError(t, c.forge.store.CI().DeleteRefTip(c.ctx, c.name("proj"), "refs/heads/none")) | |||
| 367 | + | c.openMR(c.root, "none") | |||
| 368 | + | assert.False(t, c.finish(runner).GetTrusted(), "no record") | |||
| 369 | + | } | |||
| 370 | + | ||||
| 371 | + | // Merging takes the head into the target: the closing of a merged request has | |||
| 372 | + | // the merger's trust, even with the source branch (and its record) deleted. | |||
| 373 | + | func TestPullRequestMergedByTheOwnerIsTrusted(t *testing.T) { | |||
| 374 | + | c := newCIHarness(t, nil) | |||
| 375 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |||
| 376 | + | c.push("main", pullWF(" types: [closed]", ""), "") | |||
| 377 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |||
| 378 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/m") | |||
| 379 | + | c.writeFile(c.wt, "m.txt", []byte("m\n")) | |||
| 380 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/m") | |||
| 381 | + | n := c.openMR(c.root, "agent/m") | |||
| 382 | + | c.waitFor("the head ref", func() bool { return c.ref(domain.PullHeadRef(n)) != "" }) | |||
| 383 | + | _, err := c.root.mr.MergeMergeRequest(c.ctx, connect.NewRequest(&pb.MergeMergeRequestRequest{ | |||
| 384 | + | Repo: "proj", Number: n, HeadId: c.ref(domain.PullHeadRef(n)), DeleteSourceBranch: true, | |||
| 385 | + | })) | |||
| 386 | + | require.NoError(t, err) | |||
| 387 | + | job := c.finish(runner) | |||
| 388 | + | assert.Equal(t, true, c.ghContext(job)["event"].(map[string]any)["pull_request"].(map[string]any)["merged"]) | |||
| 389 | + | assert.True(t, job.GetTrusted()) | |||
| 390 | + | } | |||
| 391 | + | ||||
| 392 | + | // openMRInto opens a merge request from source into target as the owner. | |||
| 393 | + | func (c *ciHarness) openMRInto(source, target string) int64 { | |||
| 394 | + | c.t.Helper() | |||
| 395 | + | res, err := c.root.mr.CreateMergeRequest(c.ctx, connect.NewRequest(&pb.CreateMergeRequestRequest{ | |||
| 396 | + | Repo: c.repo, Title: "Title of " + source, SourceBranch: source, TargetBranch: target, | |||
| 397 | + | })) | |||
| 398 | + | require.NoError(c.t, err) | |||
| 399 | + | return res.Msg.GetMergeRequest().GetNumber() | |||
| 400 | + | } | |||
| 401 | + | ||||
| 402 | + | // The merge commit carries the target's files too: the base of the event must | |||
| 403 | + | // have been pushed by a trusted principal as well, for every activity. | |||
| 404 | + | func TestPullRequestTrustFollowsTheBasesPusher(t *testing.T) { | |||
| 405 | + | c, hook := newHookedHarness(t) | |||
| 406 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |||
| 407 | + | c.push("main", pullWF(" types: [opened, synchronize, closed]", ""), "") | |||
| 408 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |||
| 409 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/base") | |||
| 410 | + | c.writeFile(c.wt, "a.txt", []byte("a\n")) | |||
| 411 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |||
| 412 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat", "main") | |||
| 413 | + | c.push("feat", map[string]string{"f.txt": "f\n"}, "") | |||
| 414 | + | ||||
| 415 | + | // The owner's branch into the agent's: not trusted, opened or synchronized. | |||
| 416 | + | n := c.openMRInto("feat", "agent/base") | |||
| 417 | + | assert.False(t, c.finish(runner).GetTrusted(), "opened on the agent's target") | |||
| 418 | + | c.push("feat", map[string]string{"g.txt": "g\n"}, "") | |||
| 419 | + | job := c.finish(runner) | |||
| 420 | + | assert.Equal(t, "synchronize", c.ghContext(job)["event"].(map[string]any)["action"]) | |||
| 421 | + | assert.False(t, job.GetTrusted(), "a synchronize by the owner on the agent's target") | |||
| 422 | + | ||||
| 423 | + | // The owner pushes the target: its tip is the owner's, so are the next runs. | |||
| 424 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |||
| 425 | + | c.push("agent/base", map[string]string{"b.txt": "b\n"}, "") | |||
| 426 | + | c.mustGit(c.wt, "checkout", "-q", "feat") | |||
| 427 | + | c.push("feat", map[string]string{"h.txt": "h\n"}, "") | |||
| 428 | + | assert.True(t, c.finish(runner).GetTrusted(), "owner's source, owner's target") | |||
| 429 | + | ||||
| 430 | + | // The agent pushes the target again; the owner closes: the run is the last | |||
| 431 | + | // merge (refreshed on the agent's tip), not trusted. | |||
| 432 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |||
| 433 | + | c.writeFile(c.wt, "c.txt", []byte("c\n")) | |||
| 434 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |||
| 435 | + | agentTip := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |||
| 436 | + | c.waitFor("the merge on the agent's tip", func() bool { | |||
| 437 | + | m := c.ref(domain.PullMergeRef(n)) | |||
| 438 | + | return m != "" && strings.HasPrefix(c.parents(m), agentTip+" ") | |||
| 439 | + | }) | |||
| 440 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |||
| 441 | + | require.NoError(t, err) | |||
| 442 | + | job = c.finish(runner) | |||
| 443 | + | assert.Equal(t, "closed", c.ghContext(job)["event"].(map[string]any)["action"]) | |||
| 444 | + | assert.False(t, job.GetTrusted(), "closed on a merge of the agent's target") | |||
| 445 | + | ||||
| 446 | + | // The trust is that of the snapshot: the owner moving the agent's target | |||
| 447 | + | // after the opening does not make the opening trusted. | |||
| 448 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat2", "main") | |||
| 449 | + | c.push("feat2", map[string]string{"k.txt": "k\n"}, "") | |||
| 450 | + | reached, release := hook.block(t, "event") | |||
| 451 | + | c.openMRInto("feat2", "agent/base") | |||
| 452 | + | await(t, reached, "the opening event") | |||
| 453 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |||
| 454 | + | c.push("agent/base", map[string]string{"d.txt": "d\n"}, "") | |||
| 455 | + | release() | |||
| 456 | + | job = c.finish(runner) | |||
| 457 | + | assert.Equal(t, agentTip+" ", c.parents(c.ghContext(job)["sha"].(string))[:41], "the run is for the snapshot's base") | |||
| 458 | + | assert.False(t, job.GetTrusted()) | |||
| 459 | + | ||||
| 460 | + | // And the other way: the agent moving the owner's target after the opening | |||
| 461 | + | // does not make it untrusted. | |||
| 462 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "feat3", "main") | |||
| 463 | + | c.push("feat3", map[string]string{"m.txt": "m\n"}, "") | |||
| 464 | + | ownerTip := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "agent/base")) | |||
| 465 | + | reached, release = hook.block(t, "event") | |||
| 466 | + | c.openMRInto("feat3", "agent/base") | |||
| 467 | + | await(t, reached, "the opening event") | |||
| 468 | + | c.mustGit(c.wt, "checkout", "-q", "agent/base") | |||
| 469 | + | c.writeFile(c.wt, "e.txt", []byte("e\n")) | |||
| 470 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/base") | |||
| 471 | + | release() | |||
| 472 | + | job = c.finish(runner) | |||
| 473 | + | assert.Equal(t, ownerTip+" ", c.parents(c.ghContext(job)["sha"].(string))[:41]) | |||
| 474 | + | assert.True(t, job.GetTrusted()) | |||
| 475 | + | } | |||
| 476 | + | ||||
| 477 | + | // A closed request runs its last merge commit, which can be of an older head | |||
| 478 | + | // than the one its closing is for: the trust of that head says nothing of it. | |||
| 479 | + | func TestPullRequestClosedOnAMergeOfAnOlderHeadIsNotTrusted(t *testing.T) { | |||
| 480 | + | c := newCIHarness(t, nil) | |||
| 481 | + | runner := c.registerRunner("alpha", "ubuntu-latest") | |||
| 482 | + | files := pullWF(" types: [closed]", "") | |||
| 483 | + | files["c.txt"] = "base\n" | |||
| 484 | + | c.push("main", files, "") | |||
| 485 | + | secret, _ := c.branchToken(scopes(domain.ScopeRepoRead, domain.ScopeRepoWrite), []string{"agent/"}, "proj") | |||
| 486 | + | c.mustGit(c.wt, "checkout", "-q", "-b", "agent/o") | |||
| 487 | + | c.writeFile(c.wt, "x.txt", []byte("x\n")) | |||
| 488 | + | c.mustGit(c.wt, "push", "-q", c.urlAuth(secret, "/proj.git"), "agent/o") | |||
| 489 | + | agentHead := strings.TrimSpace(c.mustGit(c.wt, "rev-parse", "HEAD")) | |||
| 490 | + | n := c.openMR(c.root, "agent/o") | |||
| 491 | + | ||||
| 492 | + | // The target moves, the agent's head still merges; then the owner pushes a | |||
| 493 | + | // head that conflicts: the merge ref keeps the merge of the agent's head. | |||
| 494 | + | c.mustGit(c.wt, "checkout", "-q", "main") | |||
| 495 | + | main2 := c.push("main", map[string]string{"c.txt": "main\n"}, "") | |||
| 496 | + | c.waitFor("the merge of the agent's head on the new target", func() bool { | |||
| 497 | + | m := c.ref(domain.PullMergeRef(n)) | |||
| 498 | + | return m != "" && c.parents(m) == main2+" "+agentHead | |||
| 499 | + | }) | |||
| 500 | + | c.mustGit(c.wt, "checkout", "-q", "agent/o") | |||
| 501 | + | ownerHead := c.push("agent/o", map[string]string{"c.txt": "owner\n"}, "") | |||
| 502 | + | c.waitFor("the head ref", func() bool { return c.ref(domain.PullHeadRef(n)) == ownerHead }) | |||
| 503 | + | require.Equal(t, main2+" "+agentHead, c.parents(c.ref(domain.PullMergeRef(n)))) | |||
| 504 | + | ||||
| 505 | + | _, err := c.root.mr.CloseMergeRequest(c.ctx, connect.NewRequest(&pb.CloseMergeRequestRequest{Repo: "proj", Number: n})) | |||
| 506 | + | require.NoError(t, err) | |||
| 507 | + | job := c.finish(runner) | |||
| 508 | + | assert.Equal(t, c.ref(domain.PullMergeRef(n)), c.ghContext(job)["sha"], "the run is the merge of the agent's head") | |||
| 509 | + | assert.False(t, job.GetTrusted()) | |||
| 510 | + | } | |||
| 511 | + | ||||
| 310 | func TestPullRefsAreServedHiddenAndNotPushable(t *testing.T) { | 512 | func TestPullRefsAreServedHiddenAndNotPushable(t *testing.T) { | ||
| 311 | c := newCIHarness(t, nil) | 513 | c := newCIHarness(t, nil) | ||
| 312 | runner := c.registerRunner("alpha", "ubuntu-latest") | 514 | runner := c.registerRunner("alpha", "ubuntu-latest") | ||
M internal/ports/ci.go +3 -0
| @@ -315,6 +315,9 @@ type PullRequestEvent struct { | |||
| 315 | 315 | // Head is the head of the source branch and Base the tip of the target at | |
| 316 | 316 | // that moment; either may be zero when unknown (the current one is read). | |
| 317 | 317 | Head, Base domain.ObjectID | |
| 318 | + | // Merged is set on the closing a merge makes: its runs have the merger's | |
| 319 | + | // trust (usecase.CI checks the merge against the request). | |
| 320 | + | Merged bool | |
| 318 | 321 | } | |
| 319 | 322 | ||
| 320 | 323 | // PullGate lets the CI change what depends on a merge request staying open | |
| @@ -315,6 +315,9 @@ type PullRequestEvent struct { | |||||
| 315 | // Head is the head of the source branch and Base the tip of the target at | 315 | // Head is the head of the source branch and Base the tip of the target at | ||
| 316 | // that moment; either may be zero when unknown (the current one is read). | 316 | // that moment; either may be zero when unknown (the current one is read). | ||
| 317 | Head, Base domain.ObjectID | 317 | Head, Base domain.ObjectID | ||
| 318 | + | // Merged is set on the closing a merge makes: its runs have the merger's | |||
| 319 | + | // trust (usecase.CI checks the merge against the request). | |||
| 320 | + | Merged bool | |||
| 318 | } | 321 | } | ||
| 319 | 322 | ||||
| 320 | // PullGate lets the CI change what depends on a merge request staying open | 323 | // PullGate lets the CI change what depends on a merge request staying open | ||
M internal/usecase/ci_pull.go +159 -8
| @@ -36,9 +36,10 @@ import ( | |||
| 36 | 36 | // closed or merged. An event carries the commits it is for, fixed when it | |
| 37 | 37 | // happened, and the actor and trust of whoever caused it: it runs for exactly | |
| 38 | 38 | // those commits, with that trust, even when the branches have moved on by the | |
| 39 | - | // time it is handled (GitHub runs each synchronize). The merge commit of a | |
| 40 | - | // run is computed from the head and base of its event, never from the branches | |
| 41 | - | // as they are now. | |
| 39 | + | // time it is handled (GitHub runs each synchronize). A run is trusted only | |
| 40 | + | // if the head and the base of its event were pushed by trusted principals | |
| 41 | + | // too (pullTipsTrusted, pullBaseTrusted). The merge commit of a run is computed from the head and | |
| 42 | + | // base of its event, never from the branches as they are now. | |
| 42 | 43 | // - the refresh worker keeps refs/pull/<n>/{head,merge} at the latest branches | |
| 43 | 44 | // when a source or a target branch moves; the refreshes of one merge request | |
| 44 | 45 | // coalesce, and the work is bounded. | |
| @@ -86,6 +87,9 @@ type pullEvent struct { | |||
| 86 | 87 | actor string | |
| 87 | 88 | pusher domain.CIPusher | |
| 88 | 89 | trusted bool | |
| 90 | + | // merge is the last merge commit the trust of a closing was checked | |
| 91 | + | // against, zero when there was none. | |
| 92 | + | merge domain.ObjectID | |
| 89 | 93 | // run is false when the cause is a job token: no run starts. | |
| 90 | 94 | run bool | |
| 91 | 95 | log zerowrap.Logger | |
| @@ -156,11 +160,141 @@ func (c *CI) OnPullRequest(ctx context.Context, repo domain.RepoName, ev ports.P | |||
| 156 | 160 | if !c.pullEnabled() { | |
| 157 | 161 | return | |
| 158 | 162 | } | |
| 159 | - | c.queuePullEvent(pullEvent{ | |
| 163 | + | e := pullEvent{ | |
| 160 | 164 | repo: repo, number: ev.Number, action: ev.Action, head: ev.Head, base: ev.Base, | |
| 161 | - | actor: authorOf(p), pusher: domain.PusherOf(p), trusted: domain.TrustedActor(p), run: !p.ViaJobToken(), | |
| 162 | - | log: zerowrap.FromCtx(ctx), | |
| 163 | - | }) | |
| 165 | + | actor: authorOf(p), pusher: domain.PusherOf(p), run: !p.ViaJobToken(), log: zerowrap.FromCtx(ctx), | |
| 166 | + | } | |
| 167 | + | if domain.TrustedActor(p) { | |
| 168 | + | e.trusted, e.merge = c.pullTipsTrusted(ctx, repo, ev) | |
| 169 | + | } | |
| 170 | + | c.queuePullEvent(e) | |
| 171 | + | } | |
| 172 | + | ||
| 173 | + | // pullTipsTrusted reports whether the code the runs of an activity that is not | |
| 174 | + | // a push execute was pushed by trusted principals (CI-4, D1), and, for a | |
| 175 | + | // closing, the last merge commit it checked (zero when there is none). The | |
| 176 | + | // workflow of a pull_request run comes from its merge commit, so from the head | |
| 177 | + | // and the base: whoever opens or closes the request did not necessarily write | |
| 178 | + | // either, and without this the owner opening the request of an agent's branch | |
| 179 | + | // (or into an agent's branch) would run the agent's workflow with the owner's | |
| 180 | + | // secrets. Every check is positive: an error, an unknown commit or a missing | |
| 181 | + | // record is not trusted. | |
| 182 | + | // | |
| 183 | + | // The records of the source and target branches (domain.CIRefTip) must name | |
| 184 | + | // exactly the head and the base of the event; they are read here, when the | |
| 185 | + | // activity happens, as the commits of the event are fixed: a push of either | |
| 186 | + | // branch before the event is handled changes nothing (a push of the source has | |
| 187 | + | // its own event and trust). A tip with no record (pushed before the records | |
| 188 | + | // existed: the next trusted push restores it), or a record for another commit | |
| 189 | + | // (the branch was force-pushed, moved or deleted) is not trusted. The base of | |
| 190 | + | // an opening is ev.Base, the target tip of that moment. A closing runs the last | |
| 191 | + | // merge commit published (which no longer changes once the request is closed: | |
| 192 | + | // the event worker checks it is still the one checked here), whose parents are | |
| 193 | + | // its head and base; without one it runs its head alone. | |
| 194 | + | // | |
| 195 | + | // A synchronize carries the trust of the push that set the source to its head | |
| 196 | + | // (onBranchesMoved), and its base is checked there. The closing a merge makes | |
| 197 | + | // is checked by mergedTrusted. | |
| 198 | + | func (c *CI) pullTipsTrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent) (bool, domain.ObjectID) { | |
| 199 | + | mr, err := c.cfg.MRs.Get(ctx, repo, ev.Number) | |
| 200 | + | if err != nil { | |
| 201 | + | pullUntrusted(ctx, repo, ev, err, "the merge request cannot be read") | |
| 202 | + | return false, domain.ObjectID{} | |
| 203 | + | } | |
| 204 | + | base := ev.Base | |
| 205 | + | var merge domain.ObjectID | |
| 206 | + | if ev.Action == domain.PullClosed { | |
| 207 | + | var parents []domain.ObjectID | |
| 208 | + | if merge, parents, err = c.lastMerge(ctx, repo, mr.Number); err != nil { | |
| 209 | + | pullUntrusted(ctx, repo, ev, err, "the last merge commit cannot be read") | |
| 210 | + | return false, merge | |
| 211 | + | } | |
| 212 | + | base = domain.ObjectID{} // no merge commit: the head runs alone | |
| 213 | + | if !merge.IsZero() { | |
| 214 | + | if parents[1] != ev.Head { | |
| 215 | + | pullUntrusted(ctx, repo, ev, nil, "the last merge commit is of another head") | |
| 216 | + | return false, merge | |
| 217 | + | } | |
| 218 | + | base = parents[0] | |
| 219 | + | } | |
| 220 | + | } | |
| 221 | + | if ev.Merged { | |
| 222 | + | return c.mergedTrusted(ctx, repo, ev, mr, base), merge | |
| 223 | + | } | |
| 224 | + | if !c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + mr.Source, commit: ev.Head, object: ev.Head}) { | |
| 225 | + | pullUntrusted(ctx, repo, ev, nil, "the head was not pushed by a trusted principal") | |
| 226 | + | return false, merge | |
| 227 | + | } | |
| 228 | + | if ev.Action == domain.PullClosed && merge.IsZero() { | |
| 229 | + | return true, merge | |
| 230 | + | } | |
| 231 | + | return c.pullBaseTrusted(ctx, repo, ev.Number, ev.Action, mr.Target, base), merge | |
| 232 | + | } | |
| 233 | + | ||
| 234 | + | // pullUntrusted logs why the runs of an activity are not trusted. | |
| 235 | + | func pullUntrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent, err error, why string) { | |
| 236 | + | ctxLog(ctx).Info().Err(err).Str("repo", repo.String()).Int64("number", ev.Number).Str("action", ev.Action). | |
| 237 | + | Str("head", ev.Head.String()).Msg("ci pull request runs are not trusted: " + why) | |
| 238 | + | } | |
| 239 | + | ||
| 240 | + | // lastMerge returns the last merge commit published for a request and its two | |
| 241 | + | // parents (base, head); zero and no error when there is none. | |
| 242 | + | func (c *CI) lastMerge(ctx context.Context, repo domain.RepoName, number int64) (domain.ObjectID, []domain.ObjectID, error) { | |
| 243 | + | merge, err := c.cfg.Pull.PullRef(ctx, repo, domain.PullMergeRef(number)) | |
| 244 | + | if errors.Is(err, domain.ErrNotFound) { | |
| 245 | + | return domain.ObjectID{}, nil, nil | |
| 246 | + | } | |
| 247 | + | if err != nil { | |
| 248 | + | return merge, nil, err | |
| 249 | + | } | |
| 250 | + | cs, _, err := c.cfg.Git.ListRange(ctx, repo, domain.ObjectID{}, merge, 1) | |
| 251 | + | if err != nil { | |
| 252 | + | return merge, nil, err | |
| 253 | + | } | |
| 254 | + | if len(cs) != 1 || len(cs[0].Parents) != 2 { | |
| 255 | + | return merge, nil, fmt.Errorf("%s is not a merge commit", merge) | |
| 256 | + | } | |
| 257 | + | return merge, cs[0].Parents, nil | |
| 258 | + | } | |
| 259 | + | ||
| 260 | + | // mergedTrusted reports whether the closing a merge made runs code the merger | |
| 261 | + | // took into the target: the request is merged, the target was set to its merge | |
| 262 | + | // result by a trusted principal (the merge records the merger), the head the | |
| 263 | + | // run executes is the one merged (the parents of the last merge commit were | |
| 264 | + | // checked against it), and the base it executes, if any, is in the merge | |
| 265 | + | // result's history. The records of the source branch are gone by then. | |
| 266 | + | func (c *CI) mergedTrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent, mr domain.MergeRequest, base domain.ObjectID) bool { | |
| 267 | + | switch { | |
| 268 | + | case ev.Action != domain.PullClosed || mr.State != domain.MRStateMerged || mr.MergedCommitID.IsZero(): | |
| 269 | + | pullUntrusted(ctx, repo, ev, nil, "the request is not merged") | |
| 270 | + | return false | |
| 271 | + | case ev.Head.IsZero() || ev.Head != mr.HeadID: | |
| 272 | + | pullUntrusted(ctx, repo, ev, nil, "the head is not the merged one") | |
| 273 | + | return false | |
| 274 | + | case !c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + mr.Target, commit: mr.MergedCommitID, object: mr.MergedCommitID}): | |
| 275 | + | pullUntrusted(ctx, repo, ev, nil, "the merge result was not set by a trusted principal") | |
| 276 | + | return false | |
| 277 | + | } | |
| 278 | + | if base.IsZero() { | |
| 279 | + | return true | |
| 280 | + | } | |
| 281 | + | if mb, err := c.cfg.Git.MergeBase(ctx, repo, base, mr.MergedCommitID); err != nil || mb != base { | |
| 282 | + | pullUntrusted(ctx, repo, ev, err, "the base is not in the merge result") | |
| 283 | + | return false | |
| 284 | + | } | |
| 285 | + | return true | |
| 286 | + | } | |
| 287 | + | ||
| 288 | + | // pullBaseTrusted reports whether the target branch was set to exactly base by | |
| 289 | + | // a trusted principal: the merge commit of a run carries the target's files | |
| 290 | + | // too, its workflow included. | |
| 291 | + | func (c *CI) pullBaseTrusted(ctx context.Context, repo domain.RepoName, number int64, action, target string, base domain.ObjectID) bool { | |
| 292 | + | if c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + target, commit: base, object: base}) { | |
| 293 | + | return true | |
| 294 | + | } | |
| 295 | + | ctxLog(ctx).Info().Str("repo", repo.String()).Int64("number", number).Str("action", action).Str("base", base.String()). | |
| 296 | + | Msg("ci pull request runs are not trusted: the base was not pushed by a trusted principal") | |
| 297 | + | return false | |
| 164 | 298 | } | |
| 165 | 299 | ||
| 166 | 300 | // onBranchesMoved is called for every push and merge, after its push | |
| @@ -235,6 +369,7 @@ func (c *CI) onBranchesMoved(ctx context.Context, repo domain.RepoName, updates | |||
| 235 | 369 | c.markPullDirty(repo) | |
| 236 | 370 | } | |
| 237 | 371 | tips := map[string]domain.ObjectID{} | |
| 372 | + | baseTrusted := map[string]bool{} | |
| 238 | 373 | for _, n := range numbers { | |
| 239 | 374 | mr, err := c.cfg.MRs.Get(ctx, repo, n) | |
| 240 | 375 | if err != nil || !mr.IsOpen() { | |
| @@ -252,9 +387,17 @@ func (c *CI) onBranchesMoved(ctx context.Context, repo domain.RepoName, updates | |||
| 252 | 387 | tips[mr.Target] = base | |
| 253 | 388 | } | |
| 254 | 389 | before, _ := domain.ParseObjectID(u.Old) // zero for a new branch | |
| 390 | + | // The pusher's trust covers the head; the base of the snapshot | |
| 391 | + | // must have been pushed by a trusted principal too. | |
| 392 | + | bt, checked := baseTrusted[mr.Target] | |
| 393 | + | if trusted && !checked { | |
| 394 | + | bt = c.pullBaseTrusted(ctx, repo, mr.Number, domain.PullSynchronize, mr.Target, base) | |
| 395 | + | baseTrusted[mr.Target] = bt | |
| 396 | + | } | |
| 397 | + | evTrusted := trusted && bt | |
| 255 | 398 | c.queuePullEvent(pullEvent{ | |
| 256 | 399 | repo: repo, number: mr.Number, action: domain.PullSynchronize, head: head, base: base, before: before, | |
| 257 | - | actor: actor, pusher: pusher, trusted: trusted, run: true, log: log, | |
| 400 | + | actor: actor, pusher: pusher, trusted: evTrusted, run: true, log: log, | |
| 258 | 401 | }) | |
| 259 | 402 | } | |
| 260 | 403 | } | |
| @@ -522,6 +665,10 @@ type pullState struct { | |||
| 522 | 665 | // request, the last one published, zero if none. | |
| 523 | 666 | merge domain.ObjectID | |
| 524 | 667 | clean bool | |
| 668 | + | // otherHead is set for a closed request whose last merge commit is of | |
| 669 | + | // another head than its event's: the provenance of the event's head says | |
| 670 | + | // nothing of the code the run executes. | |
| 671 | + | otherHead bool | |
| 525 | 672 | } | |
| 526 | 673 | ||
| 527 | 674 | func (c *CI) handlePullEvent(ctx context.Context, e pullEvent) error { | |
| @@ -555,6 +702,9 @@ func (c *CI) handlePullEvent(ctx context.Context, e pullEvent) error { | |||
| 555 | 702 | if err != nil || !ok { | |
| 556 | 703 | return err | |
| 557 | 704 | } | |
| 705 | + | if st.otherHead || st.merge != e.merge { | |
| 706 | + | e.trusted = false // not the code the trust was checked for | |
| 707 | + | } | |
| 558 | 708 | return c.startPullRuns(ctx, e, mr, st) | |
| 559 | 709 | } | |
| 560 | 710 | return nil | |
| @@ -707,6 +857,7 @@ func (c *CI) closedPull(ctx context.Context, e pullEvent, mr domain.MergeRequest | |||
| 707 | 857 | if st.head.IsZero() { | |
| 708 | 858 | st.head = cs[0].Parents[1] | |
| 709 | 859 | } | |
| 860 | + | st.otherHead = cs[0].Parents[1] != e.head | |
| 710 | 861 | } | |
| 711 | 862 | } | |
| 712 | 863 | if st.base.IsZero() { | |
| @@ -36,9 +36,10 @@ import ( | |||||
| 36 | // closed or merged. An event carries the commits it is for, fixed when it | 36 | // closed or merged. An event carries the commits it is for, fixed when it | ||
| 37 | // happened, and the actor and trust of whoever caused it: it runs for exactly | 37 | // happened, and the actor and trust of whoever caused it: it runs for exactly | ||
| 38 | // those commits, with that trust, even when the branches have moved on by the | 38 | // those commits, with that trust, even when the branches have moved on by the | ||
| 39 | - | // time it is handled (GitHub runs each synchronize). The merge commit of a | 39 | + | // time it is handled (GitHub runs each synchronize). A run is trusted only |
| 40 | - | // run is computed from the head and base of its event, never from the branches | 40 | + | // if the head and the base of its event were pushed by trusted principals |
| 41 | - | // as they are now. | 41 | + | // too (pullTipsTrusted, pullBaseTrusted). The merge commit of a run is computed from the head and |
| 42 | + | // base of its event, never from the branches as they are now. | |||
| 42 | // - the refresh worker keeps refs/pull/<n>/{head,merge} at the latest branches | 43 | // - the refresh worker keeps refs/pull/<n>/{head,merge} at the latest branches | ||
| 43 | // when a source or a target branch moves; the refreshes of one merge request | 44 | // when a source or a target branch moves; the refreshes of one merge request | ||
| 44 | // coalesce, and the work is bounded. | 45 | // coalesce, and the work is bounded. | ||
| @@ -86,6 +87,9 @@ type pullEvent struct { | |||||
| 86 | actor string | 87 | actor string | ||
| 87 | pusher domain.CIPusher | 88 | pusher domain.CIPusher | ||
| 88 | trusted bool | 89 | trusted bool | ||
| 90 | + | // merge is the last merge commit the trust of a closing was checked | |||
| 91 | + | // against, zero when there was none. | |||
| 92 | + | merge domain.ObjectID | |||
| 89 | // run is false when the cause is a job token: no run starts. | 93 | // run is false when the cause is a job token: no run starts. | ||
| 90 | run bool | 94 | run bool | ||
| 91 | log zerowrap.Logger | 95 | log zerowrap.Logger | ||
| @@ -156,11 +160,141 @@ func (c *CI) OnPullRequest(ctx context.Context, repo domain.RepoName, ev ports.P | |||||
| 156 | if !c.pullEnabled() { | 160 | if !c.pullEnabled() { | ||
| 157 | return | 161 | return | ||
| 158 | } | 162 | } | ||
| 159 | - | c.queuePullEvent(pullEvent{ | 163 | + | e := pullEvent{ |
| 160 | repo: repo, number: ev.Number, action: ev.Action, head: ev.Head, base: ev.Base, | 164 | repo: repo, number: ev.Number, action: ev.Action, head: ev.Head, base: ev.Base, | ||
| 161 | - | actor: authorOf(p), pusher: domain.PusherOf(p), trusted: domain.TrustedActor(p), run: !p.ViaJobToken(), | 165 | + | actor: authorOf(p), pusher: domain.PusherOf(p), run: !p.ViaJobToken(), log: zerowrap.FromCtx(ctx), |
| 162 | - | log: zerowrap.FromCtx(ctx), | 166 | + | } |
| 163 | - | }) | 167 | + | if domain.TrustedActor(p) { |
| 168 | + | e.trusted, e.merge = c.pullTipsTrusted(ctx, repo, ev) | |||
| 169 | + | } | |||
| 170 | + | c.queuePullEvent(e) | |||
| 171 | + | } | |||
| 172 | + | ||||
| 173 | + | // pullTipsTrusted reports whether the code the runs of an activity that is not | |||
| 174 | + | // a push execute was pushed by trusted principals (CI-4, D1), and, for a | |||
| 175 | + | // closing, the last merge commit it checked (zero when there is none). The | |||
| 176 | + | // workflow of a pull_request run comes from its merge commit, so from the head | |||
| 177 | + | // and the base: whoever opens or closes the request did not necessarily write | |||
| 178 | + | // either, and without this the owner opening the request of an agent's branch | |||
| 179 | + | // (or into an agent's branch) would run the agent's workflow with the owner's | |||
| 180 | + | // secrets. Every check is positive: an error, an unknown commit or a missing | |||
| 181 | + | // record is not trusted. | |||
| 182 | + | // | |||
| 183 | + | // The records of the source and target branches (domain.CIRefTip) must name | |||
| 184 | + | // exactly the head and the base of the event; they are read here, when the | |||
| 185 | + | // activity happens, as the commits of the event are fixed: a push of either | |||
| 186 | + | // branch before the event is handled changes nothing (a push of the source has | |||
| 187 | + | // its own event and trust). A tip with no record (pushed before the records | |||
| 188 | + | // existed: the next trusted push restores it), or a record for another commit | |||
| 189 | + | // (the branch was force-pushed, moved or deleted) is not trusted. The base of | |||
| 190 | + | // an opening is ev.Base, the target tip of that moment. A closing runs the last | |||
| 191 | + | // merge commit published (which no longer changes once the request is closed: | |||
| 192 | + | // the event worker checks it is still the one checked here), whose parents are | |||
| 193 | + | // its head and base; without one it runs its head alone. | |||
| 194 | + | // | |||
| 195 | + | // A synchronize carries the trust of the push that set the source to its head | |||
| 196 | + | // (onBranchesMoved), and its base is checked there. The closing a merge makes | |||
| 197 | + | // is checked by mergedTrusted. | |||
| 198 | + | func (c *CI) pullTipsTrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent) (bool, domain.ObjectID) { | |||
| 199 | + | mr, err := c.cfg.MRs.Get(ctx, repo, ev.Number) | |||
| 200 | + | if err != nil { | |||
| 201 | + | pullUntrusted(ctx, repo, ev, err, "the merge request cannot be read") | |||
| 202 | + | return false, domain.ObjectID{} | |||
| 203 | + | } | |||
| 204 | + | base := ev.Base | |||
| 205 | + | var merge domain.ObjectID | |||
| 206 | + | if ev.Action == domain.PullClosed { | |||
| 207 | + | var parents []domain.ObjectID | |||
| 208 | + | if merge, parents, err = c.lastMerge(ctx, repo, mr.Number); err != nil { | |||
| 209 | + | pullUntrusted(ctx, repo, ev, err, "the last merge commit cannot be read") | |||
| 210 | + | return false, merge | |||
| 211 | + | } | |||
| 212 | + | base = domain.ObjectID{} // no merge commit: the head runs alone | |||
| 213 | + | if !merge.IsZero() { | |||
| 214 | + | if parents[1] != ev.Head { | |||
| 215 | + | pullUntrusted(ctx, repo, ev, nil, "the last merge commit is of another head") | |||
| 216 | + | return false, merge | |||
| 217 | + | } | |||
| 218 | + | base = parents[0] | |||
| 219 | + | } | |||
| 220 | + | } | |||
| 221 | + | if ev.Merged { | |||
| 222 | + | return c.mergedTrusted(ctx, repo, ev, mr, base), merge | |||
| 223 | + | } | |||
| 224 | + | if !c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + mr.Source, commit: ev.Head, object: ev.Head}) { | |||
| 225 | + | pullUntrusted(ctx, repo, ev, nil, "the head was not pushed by a trusted principal") | |||
| 226 | + | return false, merge | |||
| 227 | + | } | |||
| 228 | + | if ev.Action == domain.PullClosed && merge.IsZero() { | |||
| 229 | + | return true, merge | |||
| 230 | + | } | |||
| 231 | + | return c.pullBaseTrusted(ctx, repo, ev.Number, ev.Action, mr.Target, base), merge | |||
| 232 | + | } | |||
| 233 | + | ||||
| 234 | + | // pullUntrusted logs why the runs of an activity are not trusted. | |||
| 235 | + | func pullUntrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent, err error, why string) { | |||
| 236 | + | ctxLog(ctx).Info().Err(err).Str("repo", repo.String()).Int64("number", ev.Number).Str("action", ev.Action). | |||
| 237 | + | Str("head", ev.Head.String()).Msg("ci pull request runs are not trusted: " + why) | |||
| 238 | + | } | |||
| 239 | + | ||||
| 240 | + | // lastMerge returns the last merge commit published for a request and its two | |||
| 241 | + | // parents (base, head); zero and no error when there is none. | |||
| 242 | + | func (c *CI) lastMerge(ctx context.Context, repo domain.RepoName, number int64) (domain.ObjectID, []domain.ObjectID, error) { | |||
| 243 | + | merge, err := c.cfg.Pull.PullRef(ctx, repo, domain.PullMergeRef(number)) | |||
| 244 | + | if errors.Is(err, domain.ErrNotFound) { | |||
| 245 | + | return domain.ObjectID{}, nil, nil | |||
| 246 | + | } | |||
| 247 | + | if err != nil { | |||
| 248 | + | return merge, nil, err | |||
| 249 | + | } | |||
| 250 | + | cs, _, err := c.cfg.Git.ListRange(ctx, repo, domain.ObjectID{}, merge, 1) | |||
| 251 | + | if err != nil { | |||
| 252 | + | return merge, nil, err | |||
| 253 | + | } | |||
| 254 | + | if len(cs) != 1 || len(cs[0].Parents) != 2 { | |||
| 255 | + | return merge, nil, fmt.Errorf("%s is not a merge commit", merge) | |||
| 256 | + | } | |||
| 257 | + | return merge, cs[0].Parents, nil | |||
| 258 | + | } | |||
| 259 | + | ||||
| 260 | + | // mergedTrusted reports whether the closing a merge made runs code the merger | |||
| 261 | + | // took into the target: the request is merged, the target was set to its merge | |||
| 262 | + | // result by a trusted principal (the merge records the merger), the head the | |||
| 263 | + | // run executes is the one merged (the parents of the last merge commit were | |||
| 264 | + | // checked against it), and the base it executes, if any, is in the merge | |||
| 265 | + | // result's history. The records of the source branch are gone by then. | |||
| 266 | + | func (c *CI) mergedTrusted(ctx context.Context, repo domain.RepoName, ev ports.PullRequestEvent, mr domain.MergeRequest, base domain.ObjectID) bool { | |||
| 267 | + | switch { | |||
| 268 | + | case ev.Action != domain.PullClosed || mr.State != domain.MRStateMerged || mr.MergedCommitID.IsZero(): | |||
| 269 | + | pullUntrusted(ctx, repo, ev, nil, "the request is not merged") | |||
| 270 | + | return false | |||
| 271 | + | case ev.Head.IsZero() || ev.Head != mr.HeadID: | |||
| 272 | + | pullUntrusted(ctx, repo, ev, nil, "the head is not the merged one") | |||
| 273 | + | return false | |||
| 274 | + | case !c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + mr.Target, commit: mr.MergedCommitID, object: mr.MergedCommitID}): | |||
| 275 | + | pullUntrusted(ctx, repo, ev, nil, "the merge result was not set by a trusted principal") | |||
| 276 | + | return false | |||
| 277 | + | } | |||
| 278 | + | if base.IsZero() { | |||
| 279 | + | return true | |||
| 280 | + | } | |||
| 281 | + | if mb, err := c.cfg.Git.MergeBase(ctx, repo, base, mr.MergedCommitID); err != nil || mb != base { | |||
| 282 | + | pullUntrusted(ctx, repo, ev, err, "the base is not in the merge result") | |||
| 283 | + | return false | |||
| 284 | + | } | |||
| 285 | + | return true | |||
| 286 | + | } | |||
| 287 | + | ||||
| 288 | + | // pullBaseTrusted reports whether the target branch was set to exactly base by | |||
| 289 | + | // a trusted principal: the merge commit of a run carries the target's files | |||
| 290 | + | // too, its workflow included. | |||
| 291 | + | func (c *CI) pullBaseTrusted(ctx context.Context, repo domain.RepoName, number int64, action, target string, base domain.ObjectID) bool { | |||
| 292 | + | if c.refTrusted(ctx, repo, pinnedRef{ref: "refs/heads/" + target, commit: base, object: base}) { | |||
| 293 | + | return true | |||
| 294 | + | } | |||
| 295 | + | ctxLog(ctx).Info().Str("repo", repo.String()).Int64("number", number).Str("action", action).Str("base", base.String()). | |||
| 296 | + | Msg("ci pull request runs are not trusted: the base was not pushed by a trusted principal") | |||
| 297 | + | return false | |||
| 164 | } | 298 | } | ||
| 165 | 299 | ||||
| 166 | // onBranchesMoved is called for every push and merge, after its push | 300 | // onBranchesMoved is called for every push and merge, after its push | ||
| @@ -235,6 +369,7 @@ func (c *CI) onBranchesMoved(ctx context.Context, repo domain.RepoName, updates | |||||
| 235 | c.markPullDirty(repo) | 369 | c.markPullDirty(repo) | ||
| 236 | } | 370 | } | ||
| 237 | tips := map[string]domain.ObjectID{} | 371 | tips := map[string]domain.ObjectID{} | ||
| 372 | + | baseTrusted := map[string]bool{} | |||
| 238 | for _, n := range numbers { | 373 | for _, n := range numbers { | ||
| 239 | mr, err := c.cfg.MRs.Get(ctx, repo, n) | 374 | mr, err := c.cfg.MRs.Get(ctx, repo, n) | ||
| 240 | if err != nil || !mr.IsOpen() { | 375 | if err != nil || !mr.IsOpen() { | ||
| @@ -252,9 +387,17 @@ func (c *CI) onBranchesMoved(ctx context.Context, repo domain.RepoName, updates | |||||
| 252 | tips[mr.Target] = base | 387 | tips[mr.Target] = base | ||
| 253 | } | 388 | } | ||
| 254 | before, _ := domain.ParseObjectID(u.Old) // zero for a new branch | 389 | before, _ := domain.ParseObjectID(u.Old) // zero for a new branch | ||
| 390 | + | // The pusher's trust covers the head; the base of the snapshot | |||
| 391 | + | // must have been pushed by a trusted principal too. | |||
| 392 | + | bt, checked := baseTrusted[mr.Target] | |||
| 393 | + | if trusted && !checked { | |||
| 394 | + | bt = c.pullBaseTrusted(ctx, repo, mr.Number, domain.PullSynchronize, mr.Target, base) | |||
| 395 | + | baseTrusted[mr.Target] = bt | |||
| 396 | + | } | |||
| 397 | + | evTrusted := trusted && bt | |||
| 255 | c.queuePullEvent(pullEvent{ | 398 | c.queuePullEvent(pullEvent{ | ||
| 256 | repo: repo, number: mr.Number, action: domain.PullSynchronize, head: head, base: base, before: before, | 399 | repo: repo, number: mr.Number, action: domain.PullSynchronize, head: head, base: base, before: before, | ||
| 257 | - | actor: actor, pusher: pusher, trusted: trusted, run: true, log: log, | 400 | + | actor: actor, pusher: pusher, trusted: evTrusted, run: true, log: log, |
| 258 | }) | 401 | }) | ||
| 259 | } | 402 | } | ||
| 260 | } | 403 | } | ||
| @@ -522,6 +665,10 @@ type pullState struct { | |||||
| 522 | // request, the last one published, zero if none. | 665 | // request, the last one published, zero if none. | ||
| 523 | merge domain.ObjectID | 666 | merge domain.ObjectID | ||
| 524 | clean bool | 667 | clean bool | ||
| 668 | + | // otherHead is set for a closed request whose last merge commit is of | |||
| 669 | + | // another head than its event's: the provenance of the event's head says | |||
| 670 | + | // nothing of the code the run executes. | |||
| 671 | + | otherHead bool | |||
| 525 | } | 672 | } | ||
| 526 | 673 | ||||
| 527 | func (c *CI) handlePullEvent(ctx context.Context, e pullEvent) error { | 674 | func (c *CI) handlePullEvent(ctx context.Context, e pullEvent) error { | ||
| @@ -555,6 +702,9 @@ func (c *CI) handlePullEvent(ctx context.Context, e pullEvent) error { | |||||
| 555 | if err != nil || !ok { | 702 | if err != nil || !ok { | ||
| 556 | return err | 703 | return err | ||
| 557 | } | 704 | } | ||
| 705 | + | if st.otherHead || st.merge != e.merge { | |||
| 706 | + | e.trusted = false // not the code the trust was checked for | |||
| 707 | + | } | |||
| 558 | return c.startPullRuns(ctx, e, mr, st) | 708 | return c.startPullRuns(ctx, e, mr, st) | ||
| 559 | } | 709 | } | ||
| 560 | return nil | 710 | return nil | ||
| @@ -707,6 +857,7 @@ func (c *CI) closedPull(ctx context.Context, e pullEvent, mr domain.MergeRequest | |||||
| 707 | if st.head.IsZero() { | 857 | if st.head.IsZero() { | ||
| 708 | st.head = cs[0].Parents[1] | 858 | st.head = cs[0].Parents[1] | ||
| 709 | } | 859 | } | ||
| 860 | + | st.otherHead = cs[0].Parents[1] != e.head | |||
| 710 | } | 861 | } | ||
| 711 | } | 862 | } | ||
| 712 | if st.base.IsZero() { | 863 | if st.base.IsZero() { | ||
M internal/usecase/ci_pull_test.go +115 -0
| @@ -3,13 +3,19 @@ | |||
| 3 | 3 | package usecase_test | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "context" | |
| 7 | + | "errors" | |
| 8 | + | "strings" | |
| 6 | 9 | "testing" | |
| 7 | 10 | ||
| 8 | 11 | "github.com/stretchr/testify/assert" | |
| 12 | + | "github.com/stretchr/testify/mock" | |
| 9 | 13 | "github.com/stretchr/testify/require" | |
| 10 | 14 | ||
| 11 | 15 | "bnema.dev/ermite-actions/workflow" | |
| 12 | 16 | "bnema.dev/ermite/internal/domain" | |
| 17 | + | "bnema.dev/ermite/internal/ports" | |
| 18 | + | "bnema.dev/ermite/internal/ports/mocks" | |
| 13 | 19 | "bnema.dev/ermite/internal/usecase" | |
| 14 | 20 | ) | |
| 15 | 21 | ||
| @@ -85,3 +91,112 @@ func TestFilePatch(t *testing.T) { | |||
| 85 | 91 | _, ok = usecase.FilePatch(domain.FileDiff{Status: domain.FileRenamed}) | |
| 86 | 92 | assert.False(t, ok, "a pure rename has no hunk") | |
| 87 | 93 | } | |
| 94 | + | ||
| 95 | + | // CI-4 D1: every path of the trust of an activity that is not a push ends | |
| 96 | + | // untrusted unless the provenance of the code it runs is positively known. | |
| 97 | + | func TestPullTipsTrustedFailsClosed(t *testing.T) { | |
| 98 | + | oid := func(c byte) domain.ObjectID { | |
| 99 | + | id, err := domain.ParseObjectID(strings.Repeat(string(c), 40)) | |
| 100 | + | require.NoError(t, err) | |
| 101 | + | return id | |
| 102 | + | } | |
| 103 | + | head, base, merge, merged, other := oid('1'), oid('2'), oid('3'), oid('4'), oid('9') | |
| 104 | + | boom := errors.New("boom") | |
| 105 | + | repo := name(t, "proj") | |
| 106 | + | open := domain.MergeRequest{Repo: repo, Number: 1, Source: "feat", Target: "main", State: domain.MRStateOpen} | |
| 107 | + | closed := open | |
| 108 | + | closed.State = domain.MRStateClosed | |
| 109 | + | mergedMR := open | |
| 110 | + | mergedMR.State, mergedMR.HeadID, mergedMR.MergedCommitID = domain.MRStateMerged, head, merged | |
| 111 | + | tip := func(ref string, sha domain.ObjectID, trusted bool) domain.CIRefTip { | |
| 112 | + | return domain.CIRefTip{Repo: repo, Ref: ref, Sha: sha.String(), Trusted: trusted} | |
| 113 | + | } | |
| 114 | + | goodTips := []domain.CIRefTip{tip("refs/heads/feat", head, true), tip("refs/heads/main", base, true)} | |
| 115 | + | mergeCommit := []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base, head}}} | |
| 116 | + | ||
| 117 | + | type world struct { | |
| 118 | + | mr domain.MergeRequest | |
| 119 | + | mrErr error | |
| 120 | + | tips []domain.CIRefTip | |
| 121 | + | tipErr error | |
| 122 | + | mergeRef domain.ObjectID // zero: none | |
| 123 | + | refErr error | |
| 124 | + | commits []domain.Commit | |
| 125 | + | listErr error | |
| 126 | + | mb domain.ObjectID | |
| 127 | + | mbErr error | |
| 128 | + | } | |
| 129 | + | opened := ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head, Base: base} | |
| 130 | + | closing := ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: head} | |
| 131 | + | merging := ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: head, Merged: true} | |
| 132 | + | mergedTips := []domain.CIRefTip{tip("refs/heads/main", merged, true)} | |
| 133 | + | ||
| 134 | + | cases := []struct { | |
| 135 | + | name string | |
| 136 | + | ev ports.PullRequestEvent | |
| 137 | + | w world | |
| 138 | + | want bool | |
| 139 | + | }{ | |
| 140 | + | {"opened: head and base pushed by the owner", opened, world{mr: open, tips: goodTips}, true}, | |
| 141 | + | {"opened: the request cannot be read", opened, world{mrErr: boom}, false}, | |
| 142 | + | {"opened: the request is gone", opened, world{mrErr: domain.ErrNotFound}, false}, | |
| 143 | + | {"opened: a record cannot be read", opened, world{mr: open, tipErr: boom}, false}, | |
| 144 | + | {"opened: unknown head", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Base: base}, world{mr: open, tips: goodTips}, false}, | |
| 145 | + | {"opened: unknown base", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head}, world{mr: open, tips: goodTips}, false}, | |
| 146 | + | {"opened: head without a record", opened, world{mr: open, tips: goodTips[1:]}, false}, | |
| 147 | + | {"opened: base without a record", opened, world{mr: open, tips: goodTips[:1]}, false}, | |
| 148 | + | {"opened: base pushed by an agent", opened, world{mr: open, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", base, false)}}, false}, | |
| 149 | + | {"opened: base record for another commit", opened, world{mr: open, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", other, true)}}, false}, | |
| 150 | + | {"an unknown action is checked like an opening", ports.PullRequestEvent{Number: 1, Action: "edited", Head: head, Base: base}, world{mr: open, tips: goodTips[:1]}, false}, | |
| 151 | + | {"closed: no merge commit, owner's head", closing, world{mr: closed, tips: goodTips[:1]}, true}, | |
| 152 | + | {"closed: merge commit of owner's tips", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: mergeCommit}, true}, | |
| 153 | + | {"closed: the merge ref cannot be read", closing, world{mr: closed, tips: goodTips, refErr: boom}, false}, | |
| 154 | + | {"closed: the merge commit cannot be read", closing, world{mr: closed, tips: goodTips, mergeRef: merge, listErr: boom}, false}, | |
| 155 | + | {"closed: the merge ref is not a merge", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base}}}}, false}, | |
| 156 | + | {"closed: the merge commit is of another head", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base, other}}}}, false}, | |
| 157 | + | {"closed: merge commit on an agent's base", closing, world{mr: closed, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", base, false)}, mergeRef: merge, commits: mergeCommit}, false}, | |
| 158 | + | {"closed, not by a merge, of a merged request: checked like a closing", closing, world{mr: mergedMR, tips: mergedTips}, false}, | |
| 159 | + | {"merged: the merger's", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mb: base}, true}, | |
| 160 | + | {"merged without a merge commit", merging, world{mr: mergedMR, tips: mergedTips}, true}, | |
| 161 | + | {"merged flag on a request that is not merged", merging, world{mr: closed, tips: append(goodTips, mergedTips...)}, false}, | |
| 162 | + | {"merged flag on an opening", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |
| 163 | + | {"merged: another head than the merged one", ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: other, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |
| 164 | + | {"merged: unknown head", ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |
| 165 | + | {"merged: the target moved since", merging, world{mr: mergedMR, tips: []domain.CIRefTip{tip("refs/heads/main", other, true)}}, false}, | |
| 166 | + | {"merged: the target set by an agent", merging, world{mr: mergedMR, tips: []domain.CIRefTip{tip("refs/heads/main", merged, false)}}, false}, | |
| 167 | + | {"merged: the base is not in the merge result", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mb: other}, false}, | |
| 168 | + | {"merged: the merge base cannot be read", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mbErr: boom}, false}, | |
| 169 | + | } | |
| 170 | + | for _, tc := range cases { | |
| 171 | + | t.Run(tc.name, func(t *testing.T) { | |
| 172 | + | w := tc.w | |
| 173 | + | mrs := mocks.NewMergeRequestStore(t) | |
| 174 | + | mrs.EXPECT().Get(mock.Anything, repo, int64(1)).Return(w.mr, w.mrErr).Maybe() | |
| 175 | + | tips := mocks.NewCIRefTipStore(t) | |
| 176 | + | tips.EXPECT().GetRefTip(mock.Anything, repo, mock.Anything).RunAndReturn(func(_ context.Context, _ domain.RepoName, ref string) (domain.CIRefTip, error) { | |
| 177 | + | if w.tipErr != nil { | |
| 178 | + | return domain.CIRefTip{}, w.tipErr | |
| 179 | + | } | |
| 180 | + | for _, rt := range w.tips { | |
| 181 | + | if rt.Ref == ref { | |
| 182 | + | return rt, nil | |
| 183 | + | } | |
| 184 | + | } | |
| 185 | + | return domain.CIRefTip{}, domain.ErrNotFound | |
| 186 | + | }).Maybe() | |
| 187 | + | pull := mocks.NewPullGit(t) | |
| 188 | + | refErr := w.refErr | |
| 189 | + | if refErr == nil && w.mergeRef.IsZero() { | |
| 190 | + | refErr = domain.ErrNotFound | |
| 191 | + | } | |
| 192 | + | pull.EXPECT().PullRef(mock.Anything, repo, domain.PullMergeRef(1)).Return(w.mergeRef, refErr).Maybe() | |
| 193 | + | git := mocks.NewMergeGit(t) | |
| 194 | + | git.EXPECT().ListRange(mock.Anything, repo, domain.ObjectID{}, merge, 1).Return(w.commits, false, w.listErr).Maybe() | |
| 195 | + | git.EXPECT().MergeBase(mock.Anything, repo, mock.Anything, merged).Return(w.mb, w.mbErr).Maybe() | |
| 196 | + | ||
| 197 | + | cfg := usecase.CIConfig{MRs: mrs, RefTips: tips, Pull: pull, Git: git} | |
| 198 | + | got, _ := usecase.PullTipsTrusted(context.Background(), cfg, repo, tc.ev) | |
| 199 | + | assert.Equal(t, tc.want, got) | |
| 200 | + | }) | |
| 201 | + | } | |
| 202 | + | } | |
| @@ -3,13 +3,19 @@ | |||||
| 3 | package usecase_test | 3 | package usecase_test | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "context" | |||
| 7 | + | "errors" | |||
| 8 | + | "strings" | |||
| 6 | "testing" | 9 | "testing" | ||
| 7 | 10 | ||||
| 8 | "github.com/stretchr/testify/assert" | 11 | "github.com/stretchr/testify/assert" | ||
| 12 | + | "github.com/stretchr/testify/mock" | |||
| 9 | "github.com/stretchr/testify/require" | 13 | "github.com/stretchr/testify/require" | ||
| 10 | 14 | ||||
| 11 | "bnema.dev/ermite-actions/workflow" | 15 | "bnema.dev/ermite-actions/workflow" | ||
| 12 | "bnema.dev/ermite/internal/domain" | 16 | "bnema.dev/ermite/internal/domain" | ||
| 17 | + | "bnema.dev/ermite/internal/ports" | |||
| 18 | + | "bnema.dev/ermite/internal/ports/mocks" | |||
| 13 | "bnema.dev/ermite/internal/usecase" | 19 | "bnema.dev/ermite/internal/usecase" | ||
| 14 | ) | 20 | ) | ||
| 15 | 21 | ||||
| @@ -85,3 +91,112 @@ func TestFilePatch(t *testing.T) { | |||||
| 85 | _, ok = usecase.FilePatch(domain.FileDiff{Status: domain.FileRenamed}) | 91 | _, ok = usecase.FilePatch(domain.FileDiff{Status: domain.FileRenamed}) | ||
| 86 | assert.False(t, ok, "a pure rename has no hunk") | 92 | assert.False(t, ok, "a pure rename has no hunk") | ||
| 87 | } | 93 | } | ||
| 94 | + | ||||
| 95 | + | // CI-4 D1: every path of the trust of an activity that is not a push ends | |||
| 96 | + | // untrusted unless the provenance of the code it runs is positively known. | |||
| 97 | + | func TestPullTipsTrustedFailsClosed(t *testing.T) { | |||
| 98 | + | oid := func(c byte) domain.ObjectID { | |||
| 99 | + | id, err := domain.ParseObjectID(strings.Repeat(string(c), 40)) | |||
| 100 | + | require.NoError(t, err) | |||
| 101 | + | return id | |||
| 102 | + | } | |||
| 103 | + | head, base, merge, merged, other := oid('1'), oid('2'), oid('3'), oid('4'), oid('9') | |||
| 104 | + | boom := errors.New("boom") | |||
| 105 | + | repo := name(t, "proj") | |||
| 106 | + | open := domain.MergeRequest{Repo: repo, Number: 1, Source: "feat", Target: "main", State: domain.MRStateOpen} | |||
| 107 | + | closed := open | |||
| 108 | + | closed.State = domain.MRStateClosed | |||
| 109 | + | mergedMR := open | |||
| 110 | + | mergedMR.State, mergedMR.HeadID, mergedMR.MergedCommitID = domain.MRStateMerged, head, merged | |||
| 111 | + | tip := func(ref string, sha domain.ObjectID, trusted bool) domain.CIRefTip { | |||
| 112 | + | return domain.CIRefTip{Repo: repo, Ref: ref, Sha: sha.String(), Trusted: trusted} | |||
| 113 | + | } | |||
| 114 | + | goodTips := []domain.CIRefTip{tip("refs/heads/feat", head, true), tip("refs/heads/main", base, true)} | |||
| 115 | + | mergeCommit := []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base, head}}} | |||
| 116 | + | ||||
| 117 | + | type world struct { | |||
| 118 | + | mr domain.MergeRequest | |||
| 119 | + | mrErr error | |||
| 120 | + | tips []domain.CIRefTip | |||
| 121 | + | tipErr error | |||
| 122 | + | mergeRef domain.ObjectID // zero: none | |||
| 123 | + | refErr error | |||
| 124 | + | commits []domain.Commit | |||
| 125 | + | listErr error | |||
| 126 | + | mb domain.ObjectID | |||
| 127 | + | mbErr error | |||
| 128 | + | } | |||
| 129 | + | opened := ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head, Base: base} | |||
| 130 | + | closing := ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: head} | |||
| 131 | + | merging := ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: head, Merged: true} | |||
| 132 | + | mergedTips := []domain.CIRefTip{tip("refs/heads/main", merged, true)} | |||
| 133 | + | ||||
| 134 | + | cases := []struct { | |||
| 135 | + | name string | |||
| 136 | + | ev ports.PullRequestEvent | |||
| 137 | + | w world | |||
| 138 | + | want bool | |||
| 139 | + | }{ | |||
| 140 | + | {"opened: head and base pushed by the owner", opened, world{mr: open, tips: goodTips}, true}, | |||
| 141 | + | {"opened: the request cannot be read", opened, world{mrErr: boom}, false}, | |||
| 142 | + | {"opened: the request is gone", opened, world{mrErr: domain.ErrNotFound}, false}, | |||
| 143 | + | {"opened: a record cannot be read", opened, world{mr: open, tipErr: boom}, false}, | |||
| 144 | + | {"opened: unknown head", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Base: base}, world{mr: open, tips: goodTips}, false}, | |||
| 145 | + | {"opened: unknown base", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head}, world{mr: open, tips: goodTips}, false}, | |||
| 146 | + | {"opened: head without a record", opened, world{mr: open, tips: goodTips[1:]}, false}, | |||
| 147 | + | {"opened: base without a record", opened, world{mr: open, tips: goodTips[:1]}, false}, | |||
| 148 | + | {"opened: base pushed by an agent", opened, world{mr: open, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", base, false)}}, false}, | |||
| 149 | + | {"opened: base record for another commit", opened, world{mr: open, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", other, true)}}, false}, | |||
| 150 | + | {"an unknown action is checked like an opening", ports.PullRequestEvent{Number: 1, Action: "edited", Head: head, Base: base}, world{mr: open, tips: goodTips[:1]}, false}, | |||
| 151 | + | {"closed: no merge commit, owner's head", closing, world{mr: closed, tips: goodTips[:1]}, true}, | |||
| 152 | + | {"closed: merge commit of owner's tips", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: mergeCommit}, true}, | |||
| 153 | + | {"closed: the merge ref cannot be read", closing, world{mr: closed, tips: goodTips, refErr: boom}, false}, | |||
| 154 | + | {"closed: the merge commit cannot be read", closing, world{mr: closed, tips: goodTips, mergeRef: merge, listErr: boom}, false}, | |||
| 155 | + | {"closed: the merge ref is not a merge", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base}}}}, false}, | |||
| 156 | + | {"closed: the merge commit is of another head", closing, world{mr: closed, tips: goodTips, mergeRef: merge, commits: []domain.Commit{{ID: merge, Parents: []domain.ObjectID{base, other}}}}, false}, | |||
| 157 | + | {"closed: merge commit on an agent's base", closing, world{mr: closed, tips: []domain.CIRefTip{goodTips[0], tip("refs/heads/main", base, false)}, mergeRef: merge, commits: mergeCommit}, false}, | |||
| 158 | + | {"closed, not by a merge, of a merged request: checked like a closing", closing, world{mr: mergedMR, tips: mergedTips}, false}, | |||
| 159 | + | {"merged: the merger's", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mb: base}, true}, | |||
| 160 | + | {"merged without a merge commit", merging, world{mr: mergedMR, tips: mergedTips}, true}, | |||
| 161 | + | {"merged flag on a request that is not merged", merging, world{mr: closed, tips: append(goodTips, mergedTips...)}, false}, | |||
| 162 | + | {"merged flag on an opening", ports.PullRequestEvent{Number: 1, Action: domain.PullOpened, Head: head, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |||
| 163 | + | {"merged: another head than the merged one", ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Head: other, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |||
| 164 | + | {"merged: unknown head", ports.PullRequestEvent{Number: 1, Action: domain.PullClosed, Merged: true}, world{mr: mergedMR, tips: mergedTips}, false}, | |||
| 165 | + | {"merged: the target moved since", merging, world{mr: mergedMR, tips: []domain.CIRefTip{tip("refs/heads/main", other, true)}}, false}, | |||
| 166 | + | {"merged: the target set by an agent", merging, world{mr: mergedMR, tips: []domain.CIRefTip{tip("refs/heads/main", merged, false)}}, false}, | |||
| 167 | + | {"merged: the base is not in the merge result", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mb: other}, false}, | |||
| 168 | + | {"merged: the merge base cannot be read", merging, world{mr: mergedMR, tips: mergedTips, mergeRef: merge, commits: mergeCommit, mbErr: boom}, false}, | |||
| 169 | + | } | |||
| 170 | + | for _, tc := range cases { | |||
| 171 | + | t.Run(tc.name, func(t *testing.T) { | |||
| 172 | + | w := tc.w | |||
| 173 | + | mrs := mocks.NewMergeRequestStore(t) | |||
| 174 | + | mrs.EXPECT().Get(mock.Anything, repo, int64(1)).Return(w.mr, w.mrErr).Maybe() | |||
| 175 | + | tips := mocks.NewCIRefTipStore(t) | |||
| 176 | + | tips.EXPECT().GetRefTip(mock.Anything, repo, mock.Anything).RunAndReturn(func(_ context.Context, _ domain.RepoName, ref string) (domain.CIRefTip, error) { | |||
| 177 | + | if w.tipErr != nil { | |||
| 178 | + | return domain.CIRefTip{}, w.tipErr | |||
| 179 | + | } | |||
| 180 | + | for _, rt := range w.tips { | |||
| 181 | + | if rt.Ref == ref { | |||
| 182 | + | return rt, nil | |||
| 183 | + | } | |||
| 184 | + | } | |||
| 185 | + | return domain.CIRefTip{}, domain.ErrNotFound | |||
| 186 | + | }).Maybe() | |||
| 187 | + | pull := mocks.NewPullGit(t) | |||
| 188 | + | refErr := w.refErr | |||
| 189 | + | if refErr == nil && w.mergeRef.IsZero() { | |||
| 190 | + | refErr = domain.ErrNotFound | |||
| 191 | + | } | |||
| 192 | + | pull.EXPECT().PullRef(mock.Anything, repo, domain.PullMergeRef(1)).Return(w.mergeRef, refErr).Maybe() | |||
| 193 | + | git := mocks.NewMergeGit(t) | |||
| 194 | + | git.EXPECT().ListRange(mock.Anything, repo, domain.ObjectID{}, merge, 1).Return(w.commits, false, w.listErr).Maybe() | |||
| 195 | + | git.EXPECT().MergeBase(mock.Anything, repo, mock.Anything, merged).Return(w.mb, w.mbErr).Maybe() | |||
| 196 | + | ||||
| 197 | + | cfg := usecase.CIConfig{MRs: mrs, RefTips: tips, Pull: pull, Git: git} | |||
| 198 | + | got, _ := usecase.PullTipsTrusted(context.Background(), cfg, repo, tc.ev) | |||
| 199 | + | assert.Equal(t, tc.want, got) | |||
| 200 | + | }) | |||
| 201 | + | } | |||
| 202 | + | } | |||
M internal/usecase/export_test.go +7 -0
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | "bnema.dev/ermite-actions/template" | |
| 10 | 10 | "bnema.dev/ermite-actions/workflow" | |
| 11 | 11 | "bnema.dev/ermite/internal/domain" | |
| 12 | + | "bnema.dev/ermite/internal/ports" | |
| 12 | 13 | ) | |
| 13 | 14 | ||
| 14 | 15 | // AuthorOf exposes authorOf to the tests. | |
| @@ -83,3 +84,9 @@ func PullFilesLink(base string, page, perPage, total int) string { | |||
| 83 | 84 | ||
| 84 | 85 | // FilePatch exposes the patch text of a file of the files endpoint to the tests. | |
| 85 | 86 | func FilePatch(f domain.FileDiff) (string, bool) { return filePatch(f) } | |
| 87 | + | ||
| 88 | + | // PullTipsTrusted exposes the trust check of an activity of a merge request to | |
| 89 | + | // the tests, on a CI that has cfg and no workers. | |
| 90 | + | func PullTipsTrusted(ctx context.Context, cfg CIConfig, repo domain.RepoName, ev ports.PullRequestEvent) (bool, domain.ObjectID) { | |
| 91 | + | return (&CI{cfg: cfg}).pullTipsTrusted(ctx, repo, ev) | |
| 92 | + | } | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | "bnema.dev/ermite-actions/template" | 9 | "bnema.dev/ermite-actions/template" | ||
| 10 | "bnema.dev/ermite-actions/workflow" | 10 | "bnema.dev/ermite-actions/workflow" | ||
| 11 | "bnema.dev/ermite/internal/domain" | 11 | "bnema.dev/ermite/internal/domain" | ||
| 12 | + | "bnema.dev/ermite/internal/ports" | |||
| 12 | ) | 13 | ) | ||
| 13 | 14 | ||||
| 14 | // AuthorOf exposes authorOf to the tests. | 15 | // AuthorOf exposes authorOf to the tests. | ||
| @@ -83,3 +84,9 @@ func PullFilesLink(base string, page, perPage, total int) string { | |||||
| 83 | 84 | ||||
| 84 | // FilePatch exposes the patch text of a file of the files endpoint to the tests. | 85 | // FilePatch exposes the patch text of a file of the files endpoint to the tests. | ||
| 85 | func FilePatch(f domain.FileDiff) (string, bool) { return filePatch(f) } | 86 | func FilePatch(f domain.FileDiff) (string, bool) { return filePatch(f) } | ||
| 87 | + | ||||
| 88 | + | // PullTipsTrusted exposes the trust check of an activity of a merge request to | |||
| 89 | + | // the tests, on a CI that has cfg and no workers. | |||
| 90 | + | func PullTipsTrusted(ctx context.Context, cfg CIConfig, repo domain.RepoName, ev ports.PullRequestEvent) (bool, domain.ObjectID) { | |||
| 91 | + | return (&CI{cfg: cfg}).pullTipsTrusted(ctx, repo, ev) | |||
| 92 | + | } | |||
M internal/usecase/mergerequests.go +1 -1
| @@ -840,7 +840,7 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma | |||
| 840 | 840 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | |
| 841 | 841 | }) | |
| 842 | 842 | if m.pulls != nil { | |
| 843 | - | m.pulls.OnPullRequest(ctx, name, ports.PullRequestEvent{Number: number, Action: domain.PullClosed, Head: mr.HeadID, Base: mr.BaseID}, p) // closed, merged: true | |
| 843 | + | m.pulls.OnPullRequest(ctx, name, ports.PullRequestEvent{Number: number, Action: domain.PullClosed, Head: mr.HeadID, Base: mr.BaseID, Merged: true}, p) | |
| 844 | 844 | } | |
| 845 | 845 | if m.ci != nil { | |
| 846 | 846 | // GitHub runs the "push" workflows of a merge, with the owner as actor; | |
| @@ -840,7 +840,7 @@ func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name doma | |||||
| 840 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | 840 | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | ||
| 841 | }) | 841 | }) | ||
| 842 | if m.pulls != nil { | 842 | if m.pulls != nil { | ||
| 843 | - | m.pulls.OnPullRequest(ctx, name, ports.PullRequestEvent{Number: number, Action: domain.PullClosed, Head: mr.HeadID, Base: mr.BaseID}, p) // closed, merged: true | 843 | + | m.pulls.OnPullRequest(ctx, name, ports.PullRequestEvent{Number: number, Action: domain.PullClosed, Head: mr.HeadID, Base: mr.BaseID, Merged: true}, p) |
| 844 | } | 844 | } | ||
| 845 | if m.ci != nil { | 845 | if m.ci != nil { | ||
| 846 | // GitHub runs the "push" workflows of a merge, with the owner as actor; | 846 | // GitHub runs the "push" workflows of a merge, with the owner as actor; | ||