feat(githttp): stream push bodies, allowlist CGI input and revalidate hook tokens
- Request bodies are no longer spooled to a temporary file: a body of unknown length is streamed to the CGI with ContentLength -1 and cut off by MaxBytesReader (fetch 64 MiB, push the configured 4 GiB); known lengths over the limit are still a 413. Config.TempDir and run/tmp are gone. - The CGI request is rebuilt: the query is service=<validated> (exactly one service value is required), headers come from an allowlist (Content-Type, Content-Encoding, Accept, User-Agent, a strictly shaped Git-Protocol), and the library path variables net/http/cgi inherits are emptied. - pre-receive re-validates the push's stored token (Pushes.Check via Auth.Revalidate), failing closed; post-receive recording runs past server shutdown with a 30 s timeout. - ctx first on HookServer.Register/Listen/Close and Maintainer.Schedule; Pushes.Open returns only an error; Config.Hooks and the maintenance scheduler are required; the handler's recorder is removed (the request log has status and duration); fixDefaultBranch ignores a repository deleted meanwhile. - e2e: one clientEnv for every git client, statusAuth reuses status, and a chunked push over a tiny MaxBodyBytes fails without moving a ref.
- commit
- 423aa4f6e61dea01f9bbec6998197e5f75ff6d94
- author
- brice · · 2026-10-10 08:11 UTC
- parent
- 474d85e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
12 files · +488 -274
M cmd/ermite/datadir.go +4 -6
| @@ -18,18 +18,16 @@ func prepareDataDir(root string) error { | |||
| 18 | 18 | return nil | |
| 19 | 19 | } | |
| 20 | 20 | ||
| 21 | - | // resetRunDir recreates run/ with git-home/ and tmp/, which hold nothing | |
| 22 | - | // that survives a start. Call it only once the kv directory lock is held: a second | |
| 21 | + | // resetRunDir recreates run/ with git-home/, which holds nothing that survives a | |
| 22 | + | // start. Call it only once the kv directory lock is held: a second | |
| 23 | 23 | // instance on the same data root must fail before touching run/. | |
| 24 | 24 | func resetRunDir(root string) error { | |
| 25 | 25 | run := filepath.Join(root, "run") | |
| 26 | 26 | if err := os.RemoveAll(run); err != nil { | |
| 27 | 27 | return fmt.Errorf("reset %s: %w", run, err) | |
| 28 | 28 | } | |
| 29 | - | for _, d := range []string{"git-home", "tmp"} { | |
| 30 | - | if err := os.MkdirAll(filepath.Join(run, d), 0o700); err != nil { | |
| 31 | - | return fmt.Errorf("prepare %s: %w", run, err) | |
| 32 | - | } | |
| 29 | + | if err := os.MkdirAll(filepath.Join(run, "git-home"), 0o700); err != nil { | |
| 30 | + | return fmt.Errorf("prepare %s: %w", run, err) | |
| 33 | 31 | } | |
| 34 | 32 | return nil | |
| 35 | 33 | } | |
| @@ -18,18 +18,16 @@ func prepareDataDir(root string) error { | |||||
| 18 | return nil | 18 | return nil | ||
| 19 | } | 19 | } | ||
| 20 | 20 | ||||
| 21 | - | // resetRunDir recreates run/ with git-home/ and tmp/, which hold nothing | 21 | + | // resetRunDir recreates run/ with git-home/, which holds nothing that survives a |
| 22 | - | // that survives a start. Call it only once the kv directory lock is held: a second | 22 | + | // start. Call it only once the kv directory lock is held: a second |
| 23 | // instance on the same data root must fail before touching run/. | 23 | // instance on the same data root must fail before touching run/. | ||
| 24 | func resetRunDir(root string) error { | 24 | func resetRunDir(root string) error { | ||
| 25 | run := filepath.Join(root, "run") | 25 | run := filepath.Join(root, "run") | ||
| 26 | if err := os.RemoveAll(run); err != nil { | 26 | if err := os.RemoveAll(run); err != nil { | ||
| 27 | return fmt.Errorf("reset %s: %w", run, err) | 27 | return fmt.Errorf("reset %s: %w", run, err) | ||
| 28 | } | 28 | } | ||
| 29 | - | for _, d := range []string{"git-home", "tmp"} { | 29 | + | if err := os.MkdirAll(filepath.Join(run, "git-home"), 0o700); err != nil { |
| 30 | - | if err := os.MkdirAll(filepath.Join(run, d), 0o700); err != nil { | 30 | + | return fmt.Errorf("prepare %s: %w", run, err) |
| 31 | - | return fmt.Errorf("prepare %s: %w", run, err) | |||
| 32 | - | } | |||
| 33 | } | 31 | } | ||
| 34 | return nil | 32 | return nil | ||
| 35 | } | 33 | } | ||
M cmd/ermite/e2e_helpers_test.go +15 -22
| @@ -42,6 +42,8 @@ type harnessOpts struct { | |||
| 42 | 42 | maintenance time.Duration | |
| 43 | 43 | // hookExe is the executable the hook scripts exec; empty builds ermite. | |
| 44 | 44 | hookExe string | |
| 45 | + | // maxBody overrides the limit of a push body when positive. | |
| 46 | + | maxBody int64 | |
| 45 | 47 | } | |
| 46 | 48 | ||
| 47 | 49 | // newHarness starts a forge. The data directory is short: the hook socket | |
| @@ -75,7 +77,7 @@ func newHarness(t *testing.T, o harnessOpts) *harness { | |||
| 75 | 77 | if hookExe == "" { | |
| 76 | 78 | hookExe = binary(t) | |
| 77 | 79 | } | |
| 78 | - | f, err := newForge(ctx, cfg, options{hookExe: hookExe, maintenanceDelay: o.maintenance}) | |
| 80 | + | f, err := newForge(ctx, cfg, options{hookExe: hookExe, maintenanceDelay: o.maintenance, maxBodyBytes: o.maxBody}) | |
| 79 | 81 | require.NoError(t, err) | |
| 80 | 82 | ||
| 81 | 83 | h := &harness{t: t, forge: f, data: data, logs: logs, ctx: ctx, cancel: cancel, hooksDone: make(chan error, 1)} | |
| @@ -112,14 +114,12 @@ func (h *harness) name(s string) domain.RepoName { | |||
| 112 | 114 | return n | |
| 113 | 115 | } | |
| 114 | 116 | ||
| 115 | - | // git runs the git client in dir with an isolated configuration. | |
| 116 | - | func (h *harness) git(dir string, args ...string) (string, error) { | |
| 117 | - | h.t.Helper() | |
| 118 | - | cmd := exec.Command("git", args...) | |
| 119 | - | cmd.Dir = dir | |
| 120 | - | cmd.Env = []string{ | |
| 117 | + | // clientEnv is the complete environment of the git client in tests: nothing | |
| 118 | + | // of the machine's configuration, a fixed identity and locale. | |
| 119 | + | func clientEnv(home string) []string { | |
| 120 | + | return []string{ | |
| 121 | 121 | "PATH=" + os.Getenv("PATH"), | |
| 122 | - | "HOME=" + h.home, | |
| 122 | + | "HOME=" + home, | |
| 123 | 123 | "GIT_CONFIG_GLOBAL=/dev/null", | |
| 124 | 124 | "GIT_CONFIG_NOSYSTEM=1", | |
| 125 | 125 | "GIT_TERMINAL_PROMPT=0", | |
| @@ -127,10 +127,12 @@ func (h *harness) git(dir string, args ...string) (string, error) { | |||
| 127 | 127 | "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@example.test", | |
| 128 | 128 | "LC_ALL=C", | |
| 129 | 129 | } | |
| 130 | - | var out bytes.Buffer | |
| 131 | - | cmd.Stdout, cmd.Stderr = &out, &out | |
| 132 | - | err := cmd.Run() | |
| 133 | - | return out.String(), err | |
| 130 | + | } | |
| 131 | + | ||
| 132 | + | // git runs the git client in dir with an isolated configuration. | |
| 133 | + | func (h *harness) git(dir string, args ...string) (string, error) { | |
| 134 | + | h.t.Helper() | |
| 135 | + | return h.gitEnv(nil, dir, args...) | |
| 134 | 136 | } | |
| 135 | 137 | ||
| 136 | 138 | func (h *harness) mustGit(dir string, args ...string) string { | |
| @@ -187,16 +189,7 @@ func (h *harness) gitEnv(extra []string, dir string, args ...string) (string, er | |||
| 187 | 189 | h.t.Helper() | |
| 188 | 190 | cmd := exec.Command("git", args...) | |
| 189 | 191 | cmd.Dir = dir | |
| 190 | - | cmd.Env = append([]string{ | |
| 191 | - | "PATH=" + os.Getenv("PATH"), | |
| 192 | - | "HOME=" + h.home, | |
| 193 | - | "GIT_CONFIG_GLOBAL=/dev/null", | |
| 194 | - | "GIT_CONFIG_NOSYSTEM=1", | |
| 195 | - | "GIT_TERMINAL_PROMPT=0", | |
| 196 | - | "GIT_AUTHOR_NAME=T", "GIT_AUTHOR_EMAIL=t@example.test", | |
| 197 | - | "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@example.test", | |
| 198 | - | "LC_ALL=C", | |
| 199 | - | }, extra...) | |
| 192 | + | cmd.Env = append(clientEnv(h.home), extra...) | |
| 200 | 193 | var out bytes.Buffer | |
| 201 | 194 | cmd.Stdout, cmd.Stderr = &out, &out | |
| 202 | 195 | err := cmd.Run() | |
| @@ -42,6 +42,8 @@ type harnessOpts struct { | |||||
| 42 | maintenance time.Duration | 42 | maintenance time.Duration | ||
| 43 | // hookExe is the executable the hook scripts exec; empty builds ermite. | 43 | // hookExe is the executable the hook scripts exec; empty builds ermite. | ||
| 44 | hookExe string | 44 | hookExe string | ||
| 45 | + | // maxBody overrides the limit of a push body when positive. | |||
| 46 | + | maxBody int64 | |||
| 45 | } | 47 | } | ||
| 46 | 48 | ||||
| 47 | // newHarness starts a forge. The data directory is short: the hook socket | 49 | // newHarness starts a forge. The data directory is short: the hook socket | ||
| @@ -75,7 +77,7 @@ func newHarness(t *testing.T, o harnessOpts) *harness { | |||||
| 75 | if hookExe == "" { | 77 | if hookExe == "" { | ||
| 76 | hookExe = binary(t) | 78 | hookExe = binary(t) | ||
| 77 | } | 79 | } | ||
| 78 | - | f, err := newForge(ctx, cfg, options{hookExe: hookExe, maintenanceDelay: o.maintenance}) | 80 | + | f, err := newForge(ctx, cfg, options{hookExe: hookExe, maintenanceDelay: o.maintenance, maxBodyBytes: o.maxBody}) |
| 79 | require.NoError(t, err) | 81 | require.NoError(t, err) | ||
| 80 | 82 | ||||
| 81 | h := &harness{t: t, forge: f, data: data, logs: logs, ctx: ctx, cancel: cancel, hooksDone: make(chan error, 1)} | 83 | h := &harness{t: t, forge: f, data: data, logs: logs, ctx: ctx, cancel: cancel, hooksDone: make(chan error, 1)} | ||
| @@ -112,14 +114,12 @@ func (h *harness) name(s string) domain.RepoName { | |||||
| 112 | return n | 114 | return n | ||
| 113 | } | 115 | } | ||
| 114 | 116 | ||||
| 115 | - | // git runs the git client in dir with an isolated configuration. | 117 | + | // clientEnv is the complete environment of the git client in tests: nothing |
| 116 | - | func (h *harness) git(dir string, args ...string) (string, error) { | 118 | + | // of the machine's configuration, a fixed identity and locale. |
| 117 | - | h.t.Helper() | 119 | + | func clientEnv(home string) []string { |
| 118 | - | cmd := exec.Command("git", args...) | 120 | + | return []string{ |
| 119 | - | cmd.Dir = dir | |||
| 120 | - | cmd.Env = []string{ | |||
| 121 | "PATH=" + os.Getenv("PATH"), | 121 | "PATH=" + os.Getenv("PATH"), | ||
| 122 | - | "HOME=" + h.home, | 122 | + | "HOME=" + home, |
| 123 | "GIT_CONFIG_GLOBAL=/dev/null", | 123 | "GIT_CONFIG_GLOBAL=/dev/null", | ||
| 124 | "GIT_CONFIG_NOSYSTEM=1", | 124 | "GIT_CONFIG_NOSYSTEM=1", | ||
| 125 | "GIT_TERMINAL_PROMPT=0", | 125 | "GIT_TERMINAL_PROMPT=0", | ||
| @@ -127,10 +127,12 @@ func (h *harness) git(dir string, args ...string) (string, error) { | |||||
| 127 | "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@example.test", | 127 | "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@example.test", | ||
| 128 | "LC_ALL=C", | 128 | "LC_ALL=C", | ||
| 129 | } | 129 | } | ||
| 130 | - | var out bytes.Buffer | 130 | + | } |
| 131 | - | cmd.Stdout, cmd.Stderr = &out, &out | 131 | + | |
| 132 | - | err := cmd.Run() | 132 | + | // git runs the git client in dir with an isolated configuration. |
| 133 | - | return out.String(), err | 133 | + | func (h *harness) git(dir string, args ...string) (string, error) { |
| 134 | + | h.t.Helper() | |||
| 135 | + | return h.gitEnv(nil, dir, args...) | |||
| 134 | } | 136 | } | ||
| 135 | 137 | ||||
| 136 | func (h *harness) mustGit(dir string, args ...string) string { | 138 | func (h *harness) mustGit(dir string, args ...string) string { | ||
| @@ -187,16 +189,7 @@ func (h *harness) gitEnv(extra []string, dir string, args ...string) (string, er | |||||
| 187 | h.t.Helper() | 189 | h.t.Helper() | ||
| 188 | cmd := exec.Command("git", args...) | 190 | cmd := exec.Command("git", args...) | ||
| 189 | cmd.Dir = dir | 191 | cmd.Dir = dir | ||
| 190 | - | cmd.Env = append([]string{ | 192 | + | cmd.Env = append(clientEnv(h.home), extra...) |
| 191 | - | "PATH=" + os.Getenv("PATH"), | |||
| 192 | - | "HOME=" + h.home, | |||
| 193 | - | "GIT_CONFIG_GLOBAL=/dev/null", | |||
| 194 | - | "GIT_CONFIG_NOSYSTEM=1", | |||
| 195 | - | "GIT_TERMINAL_PROMPT=0", | |||
| 196 | - | "GIT_AUTHOR_NAME=T", "GIT_AUTHOR_EMAIL=t@example.test", | |||
| 197 | - | "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@example.test", | |||
| 198 | - | "LC_ALL=C", | |||
| 199 | - | }, extra...) | |||
| 200 | var out bytes.Buffer | 193 | var out bytes.Buffer | ||
| 201 | cmd.Stdout, cmd.Stderr = &out, &out | 194 | cmd.Stdout, cmd.Stderr = &out, &out | ||
| 202 | err := cmd.Run() | 195 | err := cmd.Run() | ||
M cmd/ermite/e2e_test.go +39 -11
| @@ -274,13 +274,7 @@ func TestAC3CreateOnPush(t *testing.T) { | |||
| 274 | 274 | // statusAuth is status with the token as Basic credentials. | |
| 275 | 275 | func (h *harness) statusAuth(method, path, secret string) int { | |
| 276 | 276 | h.t.Helper() | |
| 277 | - | req, err := http.NewRequest(method, h.url(path), nil) | |
| 278 | - | require.NoError(h.t, err) | |
| 279 | - | req.SetBasicAuth("x", secret) | |
| 280 | - | resp, err := http.DefaultClient.Do(req) | |
| 281 | - | require.NoError(h.t, err) | |
| 282 | - | _ = resp.Body.Close() | |
| 283 | - | return resp.StatusCode | |
| 277 | + | return h.status(method, path, http.Header{"Authorization": {"Basic " + base64.StdEncoding.EncodeToString([]byte("x:"+secret))}}) | |
| 284 | 278 | } | |
| 285 | 279 | ||
| 286 | 280 | // AC4: hooks fail closed, and post-receive records exactly what was pushed. | |
| @@ -292,7 +286,7 @@ func TestAC4HooksFailClosed(t *testing.T) { | |||
| 292 | 286 | before := h.bareGit("proj", "rev-parse", "main") | |
| 293 | 287 | ||
| 294 | 288 | // The hook socket goes away: pre-receive cannot ask ermite and rejects. | |
| 295 | - | h.forge.hooks.Close() | |
| 289 | + | h.forge.hooks.Close(h.ctx) | |
| 296 | 290 | select { | |
| 297 | 291 | case <-h.hooksDone: | |
| 298 | 292 | case <-time.After(5 * time.Second): | |
| @@ -600,7 +594,7 @@ func TestAC8TokenCreationThroughTheForge(t *testing.T) { | |||
| 600 | 594 | } | |
| 601 | 595 | ||
| 602 | 596 | // Large pushes arrive chunked (git's http.postBuffer is 1 MiB): the handler | |
| 603 | - | // spools them. Hooks, a path with quotes in the hook executable, and the | |
| 597 | + | // streams them to Git. Hooks, a path with quotes in the hook executable, and the | |
| 604 | 598 | // debounced maintenance are exercised on the way. | |
| 605 | 599 | func TestLargePushHookPathQuotingAndMaintenance(t *testing.T) { | |
| 606 | 600 | dir := filepath.Join(t.TempDir(), "it's a \"dir\" with spaces") | |
| @@ -653,6 +647,41 @@ func TestLargePushHookPathQuotingAndMaintenance(t *testing.T) { | |||
| 653 | 647 | assert.NotContains(t, string(conf), "receivepack") | |
| 654 | 648 | } | |
| 655 | 649 | ||
| 650 | + | // A chunked push has no declared length, so the limit cuts the body off while | |
| 651 | + | // it streams: the push fails and no ref moves. A small push, which git sends | |
| 652 | + | // with a Content-Length, is refused up front. | |
| 653 | + | func TestPushOverTheBodyLimitFailsAndMovesNoRef(t *testing.T) { | |
| 654 | + | const limit = 256 << 10 | |
| 655 | + | h := newHarness(t, harnessOpts{maxBody: limit}) | |
| 656 | + | small := h.workTree("main", "a") | |
| 657 | + | out, err := h.git(small, "push", "-q", h.urlAuth(adminToken, "/proj.git"), "main") | |
| 658 | + | require.NoError(t, err, "a push under the limit works\n"+out) | |
| 659 | + | before := h.bareGit("proj", "rev-parse", "main") | |
| 660 | + | ||
| 661 | + | big := make([]byte, 3<<20) // over postBuffer: git sends it chunked | |
| 662 | + | _, err = rand.Read(big) | |
| 663 | + | require.NoError(t, err) | |
| 664 | + | require.NoError(t, os.WriteFile(filepath.Join(small, "big.bin"), big, 0o644)) | |
| 665 | + | h.mustGit(small, "add", "big.bin") | |
| 666 | + | h.mustGit(small, "commit", "-q", "-m", "big") | |
| 667 | + | ||
| 668 | + | out, err = h.git(small, "push", h.urlAuth(adminToken, "/proj.git"), "main") | |
| 669 | + | require.Error(t, err, "a chunked push over the limit fails\n"+out) | |
| 670 | + | assert.Equal(t, before, h.bareGit("proj", "rev-parse", "main"), "no ref moved") | |
| 671 | + | ||
| 672 | + | // A new branch of the same objects fails the same way and is not created. | |
| 673 | + | h.mustGit(small, "branch", "other") | |
| 674 | + | out, err = h.git(small, "push", h.urlAuth(adminToken, "/proj.git"), "other") | |
| 675 | + | require.Error(t, err, out) | |
| 676 | + | assert.Empty(t, h.bareGit("proj", "for-each-ref", "refs/heads/other")) | |
| 677 | + | ||
| 678 | + | // With a declared length (postBuffer raised above the pack) it is a 413. | |
| 679 | + | out, err = h.git(small, "-c", "http.postBuffer=104857600", "push", h.urlAuth(adminToken, "/proj.git"), "main") | |
| 680 | + | require.Error(t, err, out) | |
| 681 | + | assert.Contains(t, out, "413") | |
| 682 | + | assert.Equal(t, before, h.bareGit("proj", "rev-parse", "main")) | |
| 683 | + | } | |
| 684 | + | ||
| 656 | 685 | // receive.fsckObjects is on: a push with a malformed object is refused, and | |
| 657 | 686 | // nothing is stored in the repository's own configuration to get there. | |
| 658 | 687 | func TestFsckRejectsMalformedObjects(t *testing.T) { | |
| @@ -724,8 +753,7 @@ func TestBuiltBinaryPushAndClone(t *testing.T) { | |||
| 724 | 753 | run := func(dir string, args ...string) string { | |
| 725 | 754 | c := exec.Command("git", args...) | |
| 726 | 755 | c.Dir = dir | |
| 727 | - | c.Env = []string{"PATH=" + os.Getenv("PATH"), "HOME=" + home, "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1", | |
| 728 | - | "GIT_TERMINAL_PROMPT=0", "GIT_AUTHOR_NAME=T", "GIT_AUTHOR_EMAIL=t@x", "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@x"} | |
| 756 | + | c.Env = clientEnv(home) | |
| 729 | 757 | out, err := c.CombinedOutput() | |
| 730 | 758 | require.NoError(t, err, "git %v\n%s\nserver logs:\n%s", args, out, logs.String()) | |
| 731 | 759 | return string(out) | |
| @@ -274,13 +274,7 @@ func TestAC3CreateOnPush(t *testing.T) { | |||||
| 274 | // statusAuth is status with the token as Basic credentials. | 274 | // statusAuth is status with the token as Basic credentials. | ||
| 275 | func (h *harness) statusAuth(method, path, secret string) int { | 275 | func (h *harness) statusAuth(method, path, secret string) int { | ||
| 276 | h.t.Helper() | 276 | h.t.Helper() | ||
| 277 | - | req, err := http.NewRequest(method, h.url(path), nil) | 277 | + | return h.status(method, path, http.Header{"Authorization": {"Basic " + base64.StdEncoding.EncodeToString([]byte("x:"+secret))}}) |
| 278 | - | require.NoError(h.t, err) | |||
| 279 | - | req.SetBasicAuth("x", secret) | |||
| 280 | - | resp, err := http.DefaultClient.Do(req) | |||
| 281 | - | require.NoError(h.t, err) | |||
| 282 | - | _ = resp.Body.Close() | |||
| 283 | - | return resp.StatusCode | |||
| 284 | } | 278 | } | ||
| 285 | 279 | ||||
| 286 | // AC4: hooks fail closed, and post-receive records exactly what was pushed. | 280 | // AC4: hooks fail closed, and post-receive records exactly what was pushed. | ||
| @@ -292,7 +286,7 @@ func TestAC4HooksFailClosed(t *testing.T) { | |||||
| 292 | before := h.bareGit("proj", "rev-parse", "main") | 286 | before := h.bareGit("proj", "rev-parse", "main") | ||
| 293 | 287 | ||||
| 294 | // The hook socket goes away: pre-receive cannot ask ermite and rejects. | 288 | // The hook socket goes away: pre-receive cannot ask ermite and rejects. | ||
| 295 | - | h.forge.hooks.Close() | 289 | + | h.forge.hooks.Close(h.ctx) |
| 296 | select { | 290 | select { | ||
| 297 | case <-h.hooksDone: | 291 | case <-h.hooksDone: | ||
| 298 | case <-time.After(5 * time.Second): | 292 | case <-time.After(5 * time.Second): | ||
| @@ -600,7 +594,7 @@ func TestAC8TokenCreationThroughTheForge(t *testing.T) { | |||||
| 600 | } | 594 | } | ||
| 601 | 595 | ||||
| 602 | // Large pushes arrive chunked (git's http.postBuffer is 1 MiB): the handler | 596 | // Large pushes arrive chunked (git's http.postBuffer is 1 MiB): the handler | ||
| 603 | - | // spools them. Hooks, a path with quotes in the hook executable, and the | 597 | + | // streams them to Git. Hooks, a path with quotes in the hook executable, and the |
| 604 | // debounced maintenance are exercised on the way. | 598 | // debounced maintenance are exercised on the way. | ||
| 605 | func TestLargePushHookPathQuotingAndMaintenance(t *testing.T) { | 599 | func TestLargePushHookPathQuotingAndMaintenance(t *testing.T) { | ||
| 606 | dir := filepath.Join(t.TempDir(), "it's a \"dir\" with spaces") | 600 | dir := filepath.Join(t.TempDir(), "it's a \"dir\" with spaces") | ||
| @@ -653,6 +647,41 @@ func TestLargePushHookPathQuotingAndMaintenance(t *testing.T) { | |||||
| 653 | assert.NotContains(t, string(conf), "receivepack") | 647 | assert.NotContains(t, string(conf), "receivepack") | ||
| 654 | } | 648 | } | ||
| 655 | 649 | ||||
| 650 | + | // A chunked push has no declared length, so the limit cuts the body off while | |||
| 651 | + | // it streams: the push fails and no ref moves. A small push, which git sends | |||
| 652 | + | // with a Content-Length, is refused up front. | |||
| 653 | + | func TestPushOverTheBodyLimitFailsAndMovesNoRef(t *testing.T) { | |||
| 654 | + | const limit = 256 << 10 | |||
| 655 | + | h := newHarness(t, harnessOpts{maxBody: limit}) | |||
| 656 | + | small := h.workTree("main", "a") | |||
| 657 | + | out, err := h.git(small, "push", "-q", h.urlAuth(adminToken, "/proj.git"), "main") | |||
| 658 | + | require.NoError(t, err, "a push under the limit works\n"+out) | |||
| 659 | + | before := h.bareGit("proj", "rev-parse", "main") | |||
| 660 | + | ||||
| 661 | + | big := make([]byte, 3<<20) // over postBuffer: git sends it chunked | |||
| 662 | + | _, err = rand.Read(big) | |||
| 663 | + | require.NoError(t, err) | |||
| 664 | + | require.NoError(t, os.WriteFile(filepath.Join(small, "big.bin"), big, 0o644)) | |||
| 665 | + | h.mustGit(small, "add", "big.bin") | |||
| 666 | + | h.mustGit(small, "commit", "-q", "-m", "big") | |||
| 667 | + | ||||
| 668 | + | out, err = h.git(small, "push", h.urlAuth(adminToken, "/proj.git"), "main") | |||
| 669 | + | require.Error(t, err, "a chunked push over the limit fails\n"+out) | |||
| 670 | + | assert.Equal(t, before, h.bareGit("proj", "rev-parse", "main"), "no ref moved") | |||
| 671 | + | ||||
| 672 | + | // A new branch of the same objects fails the same way and is not created. | |||
| 673 | + | h.mustGit(small, "branch", "other") | |||
| 674 | + | out, err = h.git(small, "push", h.urlAuth(adminToken, "/proj.git"), "other") | |||
| 675 | + | require.Error(t, err, out) | |||
| 676 | + | assert.Empty(t, h.bareGit("proj", "for-each-ref", "refs/heads/other")) | |||
| 677 | + | ||||
| 678 | + | // With a declared length (postBuffer raised above the pack) it is a 413. | |||
| 679 | + | out, err = h.git(small, "-c", "http.postBuffer=104857600", "push", h.urlAuth(adminToken, "/proj.git"), "main") | |||
| 680 | + | require.Error(t, err, out) | |||
| 681 | + | assert.Contains(t, out, "413") | |||
| 682 | + | assert.Equal(t, before, h.bareGit("proj", "rev-parse", "main")) | |||
| 683 | + | } | |||
| 684 | + | ||||
| 656 | // receive.fsckObjects is on: a push with a malformed object is refused, and | 685 | // receive.fsckObjects is on: a push with a malformed object is refused, and | ||
| 657 | // nothing is stored in the repository's own configuration to get there. | 686 | // nothing is stored in the repository's own configuration to get there. | ||
| 658 | func TestFsckRejectsMalformedObjects(t *testing.T) { | 687 | func TestFsckRejectsMalformedObjects(t *testing.T) { | ||
| @@ -724,8 +753,7 @@ func TestBuiltBinaryPushAndClone(t *testing.T) { | |||||
| 724 | run := func(dir string, args ...string) string { | 753 | run := func(dir string, args ...string) string { | ||
| 725 | c := exec.Command("git", args...) | 754 | c := exec.Command("git", args...) | ||
| 726 | c.Dir = dir | 755 | c.Dir = dir | ||
| 727 | - | c.Env = []string{"PATH=" + os.Getenv("PATH"), "HOME=" + home, "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1", | 756 | + | c.Env = clientEnv(home) |
| 728 | - | "GIT_TERMINAL_PROMPT=0", "GIT_AUTHOR_NAME=T", "GIT_AUTHOR_EMAIL=t@x", "GIT_COMMITTER_NAME=T", "GIT_COMMITTER_EMAIL=t@x"} | |||
| 729 | out, err := c.CombinedOutput() | 757 | out, err := c.CombinedOutput() | ||
| 730 | require.NoError(t, err, "git %v\n%s\nserver logs:\n%s", args, out, logs.String()) | 758 | require.NoError(t, err, "git %v\n%s\nserver logs:\n%s", args, out, logs.String()) | ||
| 731 | return string(out) | 759 | return string(out) | ||
M cmd/ermite/forge.go +15 -5
| @@ -23,6 +23,10 @@ import ( | |||
| 23 | 23 | "bnema.dev/ermite/internal/ports" | |
| 24 | 24 | ) | |
| 25 | 25 | ||
| 26 | + | // maxReceiveBytes bounds the body of a push, both at the HTTP handler and in | |
| 27 | + | // Git (receive.maxInputSize). | |
| 28 | + | const maxReceiveBytes = githttp.DefaultMaxBodyBytes | |
| 29 | + | ||
| 26 | 30 | // options are the parts of the wiring that tests replace. | |
| 27 | 31 | type options struct { | |
| 28 | 32 | // hookExe is the executable the hook scripts exec; empty means this | |
| @@ -30,6 +34,8 @@ type options struct { | |||
| 30 | 34 | hookExe string | |
| 31 | 35 | // maintenanceDelay overrides app.MaintenanceDelay when positive. | |
| 32 | 36 | maintenanceDelay time.Duration | |
| 37 | + | // maxBodyBytes overrides the limit of a push body when positive. | |
| 38 | + | maxBodyBytes int64 | |
| 33 | 39 | } | |
| 34 | 40 | ||
| 35 | 41 | // forge is the wired server: every adapter and use case, ready to serve. | |
| @@ -75,7 +81,11 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||
| 75 | 81 | if path == "" { | |
| 76 | 82 | path = "/usr/local/bin:/usr/bin:/bin" | |
| 77 | 83 | } | |
| 78 | - | git := gitcli.New(gitcli.Config{Bin: cfg.Git, DataDir: cfg.DataDir, Path: path}) | |
| 84 | + | maxBody := maxReceiveBytes | |
| 85 | + | if opts.maxBodyBytes > 0 { | |
| 86 | + | maxBody = opts.maxBodyBytes | |
| 87 | + | } | |
| 88 | + | git := gitcli.New(gitcli.Config{Bin: cfg.Git, DataDir: cfg.DataDir, Path: path, MaxReceiveBytes: maxBody}) | |
| 79 | 89 | gitVersion, err := git.CheckVersion(ctx) | |
| 80 | 90 | if err != nil { | |
| 81 | 91 | return nil, fmt.Errorf("%s=%q: %w", config.EnvGit, cfg.Git, err) | |
| @@ -101,7 +111,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||
| 101 | 111 | f.Auth = app.NewAuth(cfg.AdminTokenHash, store.Tokens(), clock) | |
| 102 | 112 | f.Events = eventbus.New() | |
| 103 | 113 | f.maint = app.NewMaintainer(ctx, git, opts.maintenanceDelay) | |
| 104 | - | f.Pushes = app.NewPushes(store, f.Repos, git, f.Events, clock, f.maint) | |
| 114 | + | f.Pushes = app.NewPushes(store, f.Repos, git, f.Events, clock, f.maint, f.Auth) | |
| 105 | 115 | ||
| 106 | 116 | exe := opts.hookExe | |
| 107 | 117 | if exe == "" { | |
| @@ -113,7 +123,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||
| 113 | 123 | return nil, err | |
| 114 | 124 | } | |
| 115 | 125 | f.hooks = githttp.NewHookServer(filepath.Join(cfg.DataDir, "run", "hook.sock"), f.Pushes) | |
| 116 | - | if err := f.hooks.Listen(); err != nil { | |
| 126 | + | if err := f.hooks.Listen(ctx); err != nil { | |
| 117 | 127 | return nil, err | |
| 118 | 128 | } | |
| 119 | 129 | ||
| @@ -124,7 +134,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||
| 124 | 134 | Backend: git, | |
| 125 | 135 | Hooks: f.hooks, | |
| 126 | 136 | TrustedProxies: cfg.TrustedProxies, | |
| 127 | - | TempDir: filepath.Join(cfg.DataDir, "run", "tmp"), | |
| 137 | + | MaxBodyBytes: maxBody, | |
| 128 | 138 | }) | |
| 129 | 139 | routes := make([]httpserver.Route, 0, len(githttp.Patterns)) | |
| 130 | 140 | for _, p := range githttp.Patterns { | |
| @@ -172,7 +182,7 @@ func (f *forge) Serve(ctx context.Context, ln net.Listener) error { | |||
| 172 | 182 | func (f *forge) Close(ctx context.Context) { | |
| 173 | 183 | log := zerowrap.FromCtx(ctx) | |
| 174 | 184 | if f.hooks != nil { | |
| 175 | - | f.hooks.Close() | |
| 185 | + | f.hooks.Close(ctx) | |
| 176 | 186 | } | |
| 177 | 187 | if f.maint != nil { | |
| 178 | 188 | f.maint.Close(ctx) | |
| @@ -23,6 +23,10 @@ import ( | |||||
| 23 | "bnema.dev/ermite/internal/ports" | 23 | "bnema.dev/ermite/internal/ports" | ||
| 24 | ) | 24 | ) | ||
| 25 | 25 | ||||
| 26 | + | // maxReceiveBytes bounds the body of a push, both at the HTTP handler and in | |||
| 27 | + | // Git (receive.maxInputSize). | |||
| 28 | + | const maxReceiveBytes = githttp.DefaultMaxBodyBytes | |||
| 29 | + | ||||
| 26 | // options are the parts of the wiring that tests replace. | 30 | // options are the parts of the wiring that tests replace. | ||
| 27 | type options struct { | 31 | type options struct { | ||
| 28 | // hookExe is the executable the hook scripts exec; empty means this | 32 | // hookExe is the executable the hook scripts exec; empty means this | ||
| @@ -30,6 +34,8 @@ type options struct { | |||||
| 30 | hookExe string | 34 | hookExe string | ||
| 31 | // maintenanceDelay overrides app.MaintenanceDelay when positive. | 35 | // maintenanceDelay overrides app.MaintenanceDelay when positive. | ||
| 32 | maintenanceDelay time.Duration | 36 | maintenanceDelay time.Duration | ||
| 37 | + | // maxBodyBytes overrides the limit of a push body when positive. | |||
| 38 | + | maxBodyBytes int64 | |||
| 33 | } | 39 | } | ||
| 34 | 40 | ||||
| 35 | // forge is the wired server: every adapter and use case, ready to serve. | 41 | // forge is the wired server: every adapter and use case, ready to serve. | ||
| @@ -75,7 +81,11 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||||
| 75 | if path == "" { | 81 | if path == "" { | ||
| 76 | path = "/usr/local/bin:/usr/bin:/bin" | 82 | path = "/usr/local/bin:/usr/bin:/bin" | ||
| 77 | } | 83 | } | ||
| 78 | - | git := gitcli.New(gitcli.Config{Bin: cfg.Git, DataDir: cfg.DataDir, Path: path}) | 84 | + | maxBody := maxReceiveBytes |
| 85 | + | if opts.maxBodyBytes > 0 { | |||
| 86 | + | maxBody = opts.maxBodyBytes | |||
| 87 | + | } | |||
| 88 | + | git := gitcli.New(gitcli.Config{Bin: cfg.Git, DataDir: cfg.DataDir, Path: path, MaxReceiveBytes: maxBody}) | |||
| 79 | gitVersion, err := git.CheckVersion(ctx) | 89 | gitVersion, err := git.CheckVersion(ctx) | ||
| 80 | if err != nil { | 90 | if err != nil { | ||
| 81 | return nil, fmt.Errorf("%s=%q: %w", config.EnvGit, cfg.Git, err) | 91 | return nil, fmt.Errorf("%s=%q: %w", config.EnvGit, cfg.Git, err) | ||
| @@ -101,7 +111,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||||
| 101 | f.Auth = app.NewAuth(cfg.AdminTokenHash, store.Tokens(), clock) | 111 | f.Auth = app.NewAuth(cfg.AdminTokenHash, store.Tokens(), clock) | ||
| 102 | f.Events = eventbus.New() | 112 | f.Events = eventbus.New() | ||
| 103 | f.maint = app.NewMaintainer(ctx, git, opts.maintenanceDelay) | 113 | f.maint = app.NewMaintainer(ctx, git, opts.maintenanceDelay) | ||
| 104 | - | f.Pushes = app.NewPushes(store, f.Repos, git, f.Events, clock, f.maint) | 114 | + | f.Pushes = app.NewPushes(store, f.Repos, git, f.Events, clock, f.maint, f.Auth) |
| 105 | 115 | ||||
| 106 | exe := opts.hookExe | 116 | exe := opts.hookExe | ||
| 107 | if exe == "" { | 117 | if exe == "" { | ||
| @@ -113,7 +123,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||||
| 113 | return nil, err | 123 | return nil, err | ||
| 114 | } | 124 | } | ||
| 115 | f.hooks = githttp.NewHookServer(filepath.Join(cfg.DataDir, "run", "hook.sock"), f.Pushes) | 125 | f.hooks = githttp.NewHookServer(filepath.Join(cfg.DataDir, "run", "hook.sock"), f.Pushes) | ||
| 116 | - | if err := f.hooks.Listen(); err != nil { | 126 | + | if err := f.hooks.Listen(ctx); err != nil { |
| 117 | return nil, err | 127 | return nil, err | ||
| 118 | } | 128 | } | ||
| 119 | 129 | ||||
| @@ -124,7 +134,7 @@ func newForge(ctx context.Context, cfg config.Config, opts options) (_ *forge, e | |||||
| 124 | Backend: git, | 134 | Backend: git, | ||
| 125 | Hooks: f.hooks, | 135 | Hooks: f.hooks, | ||
| 126 | TrustedProxies: cfg.TrustedProxies, | 136 | TrustedProxies: cfg.TrustedProxies, | ||
| 127 | - | TempDir: filepath.Join(cfg.DataDir, "run", "tmp"), | 137 | + | MaxBodyBytes: maxBody, |
| 128 | }) | 138 | }) | ||
| 129 | routes := make([]httpserver.Route, 0, len(githttp.Patterns)) | 139 | routes := make([]httpserver.Route, 0, len(githttp.Patterns)) | ||
| 130 | for _, p := range githttp.Patterns { | 140 | for _, p := range githttp.Patterns { | ||
| @@ -172,7 +182,7 @@ func (f *forge) Serve(ctx context.Context, ln net.Listener) error { | |||||
| 172 | func (f *forge) Close(ctx context.Context) { | 182 | func (f *forge) Close(ctx context.Context) { | ||
| 173 | log := zerowrap.FromCtx(ctx) | 183 | log := zerowrap.FromCtx(ctx) | ||
| 174 | if f.hooks != nil { | 184 | if f.hooks != nil { | ||
| 175 | - | f.hooks.Close() | 185 | + | f.hooks.Close(ctx) |
| 176 | } | 186 | } | ||
| 177 | if f.maint != nil { | 187 | if f.maint != nil { | ||
| 178 | f.maint.Close(ctx) | 188 | f.maint.Close(ctx) | ||
M internal/adapters/githttp/handler.go +73 -96
| @@ -9,16 +9,14 @@ package githttp | |||
| 9 | 9 | import ( | |
| 10 | 10 | "context" | |
| 11 | 11 | "errors" | |
| 12 | - | "io" | |
| 13 | 12 | "log" | |
| 14 | 13 | "math" | |
| 15 | 14 | "net/http" | |
| 16 | 15 | "net/http/cgi" | |
| 17 | 16 | "net/netip" | |
| 18 | - | "os" | |
| 17 | + | "regexp" | |
| 19 | 18 | "strconv" | |
| 20 | 19 | "strings" | |
| 21 | - | "time" | |
| 22 | 20 | ||
| 23 | 21 | "github.com/bnema/zerowrap" | |
| 24 | 22 | ||
| @@ -38,6 +36,11 @@ var Patterns = []string{ | |||
| 38 | 36 | // is zero. | |
| 39 | 37 | const DefaultMaxBodyBytes int64 = 4 << 30 | |
| 40 | 38 | ||
| 39 | + | // uploadPackMaxBody bounds the body of git-upload-pack, which is only a list | |
| 40 | + | // of wants and haves: git http-backend buffers it whole when its length is | |
| 41 | + | // unknown, so it must stay small whatever Config.MaxBodyBytes says. | |
| 42 | + | const uploadPackMaxBody int64 = 64 << 20 | |
| 43 | + | ||
| 41 | 44 | // Names of the environment variables that carry the hook callback settings | |
| 42 | 45 | // from ermite, through git, to the hook process. | |
| 43 | 46 | const ( | |
| @@ -70,7 +73,7 @@ type Repos interface { | |||
| 70 | 73 | // domain.ErrForbidden for one it can see but not write. With create, a missing | |
| 71 | 74 | // repository the principal may write is created. app.Pushes fits. | |
| 72 | 75 | type Pushes interface { | |
| 73 | - | Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) (repo domain.Repo, created bool, err error) | |
| 76 | + | Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) error | |
| 74 | 77 | } | |
| 75 | 78 | ||
| 76 | 79 | // HookTokens hands out the per-request tokens the hooks of a push present | |
| @@ -78,7 +81,7 @@ type Pushes interface { | |||
| 78 | 81 | type HookTokens interface { | |
| 79 | 82 | // Register binds a new token to the repository and principal of a push. | |
| 80 | 83 | // release revokes it and must be called when the request ends. | |
| 81 | - | Register(repo domain.RepoName, p domain.Principal) (token string, release func()) | |
| 84 | + | Register(ctx context.Context, repo domain.RepoName, p domain.Principal) (token string, release func()) | |
| 82 | 85 | // Socket is the path of the hook socket. | |
| 83 | 86 | Socket() string | |
| 84 | 87 | } | |
| @@ -89,17 +92,14 @@ type Config struct { | |||
| 89 | 92 | Repos Repos | |
| 90 | 93 | Pushes Pushes | |
| 91 | 94 | Backend ports.GitBackend | |
| 92 | - | // Hooks provides hook tokens for pushes. Nil runs pushes without hook | |
| 93 | - | // environment (tests of the handler alone). | |
| 95 | + | // Hooks provides hook tokens for pushes. It is required. | |
| 94 | 96 | Hooks HookTokens | |
| 95 | 97 | // TrustedProxies are the networks allowed to set X-Forwarded-For. | |
| 96 | 98 | TrustedProxies []netip.Prefix | |
| 97 | 99 | // Limiter counts failed authentications; nil builds a default one. | |
| 98 | 100 | Limiter *Limiter | |
| 99 | - | // TempDir holds request bodies that arrive chunked, which CGI cannot | |
| 100 | - | // stream. Empty uses the system temporary directory. | |
| 101 | - | TempDir string | |
| 102 | - | // MaxBodyBytes bounds a request body; 0 means DefaultMaxBodyBytes. | |
| 101 | + | // MaxBodyBytes bounds the body of a push; 0 means DefaultMaxBodyBytes. | |
| 102 | + | // The body of a fetch is bounded by 64 MiB at most. | |
| 103 | 103 | MaxBodyBytes int64 | |
| 104 | 104 | } | |
| 105 | 105 | ||
| @@ -145,9 +145,13 @@ func parseRoute(r *http.Request) (route, int) { | |||
| 145 | 145 | if r.Method != http.MethodGet && r.Method != http.MethodHead { | |
| 146 | 146 | return route{}, http.StatusMethodNotAllowed | |
| 147 | 147 | } | |
| 148 | - | switch svc := r.URL.Query().Get("service"); svc { | |
| 149 | - | case serviceUpload, serviceReceive: | |
| 150 | - | rt.service = svc | |
| 148 | + | // Exactly one service value: a repeated parameter is ambiguous, and | |
| 149 | + | // what is handed to the backend is rebuilt from this one anyway. | |
| 150 | + | switch svcs := r.URL.Query()["service"]; { | |
| 151 | + | case len(svcs) != 1: | |
| 152 | + | return route{}, http.StatusNotFound | |
| 153 | + | case svcs[0] == serviceUpload, svcs[0] == serviceReceive: | |
| 154 | + | rt.service = svcs[0] | |
| 151 | 155 | default: | |
| 152 | 156 | return route{}, http.StatusNotFound | |
| 153 | 157 | } | |
| @@ -164,8 +168,6 @@ func parseRoute(r *http.Request) (route, int) { | |||
| 164 | 168 | ||
| 165 | 169 | // ServeHTTP implements http.Handler. | |
| 166 | 170 | func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |
| 167 | - | start := time.Now() | |
| 168 | - | rec := &recorder{ResponseWriter: w} | |
| 169 | 171 | var rt route | |
| 170 | 172 | var name domain.RepoName | |
| 171 | 173 | var principal domain.Principal | |
| @@ -174,13 +176,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| 174 | 176 | log.Debug(). | |
| 175 | 177 | Str("repo", name.String()). | |
| 176 | 178 | Str("service", rt.service). | |
| 177 | - | Int("status", rec.code()). | |
| 178 | - | Dur("duration", time.Since(start)). | |
| 179 | 179 | Str("principal", principal.Label()). | |
| 180 | 180 | Str("token_id", principal.TokenID()). | |
| 181 | 181 | Msg("git request") | |
| 182 | 182 | }() | |
| 183 | - | w = rec | |
| 184 | 183 | ||
| 185 | 184 | rt, status := parseRoute(r) | |
| 186 | 185 | if status != 0 { | |
| @@ -216,12 +215,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| 216 | 215 | ||
| 217 | 216 | if rt.push() { | |
| 218 | 217 | // Only the first request of a push may create the repository. | |
| 219 | - | _, _, err = h.cfg.Pushes.Open(ctx, principal, name, rt.suffix == "info/refs") | |
| 218 | + | err = h.cfg.Pushes.Open(ctx, principal, name, rt.suffix == "info/refs") | |
| 220 | 219 | } else { | |
| 221 | 220 | _, err = h.cfg.Repos.Get(ctx, principal, name) | |
| 222 | 221 | } | |
| 223 | 222 | if err != nil { | |
| 224 | - | h.denied(r.Context(), w, principal, err) | |
| 223 | + | h.denied(ctx, w, principal, err) | |
| 225 | 224 | return | |
| 226 | 225 | } | |
| 227 | 226 | ||
| @@ -295,40 +294,22 @@ func challenge(w http.ResponseWriter) { | |||
| 295 | 294 | ||
| 296 | 295 | func notFound(w http.ResponseWriter) { http.Error(w, "not found", http.StatusNotFound) } | |
| 297 | 296 | ||
| 298 | - | // recorder remembers the status code of a response and keeps Flush working. | |
| 299 | - | type recorder struct { | |
| 300 | - | http.ResponseWriter | |
| 301 | - | status int | |
| 302 | - | } | |
| 303 | - | ||
| 304 | - | func (r *recorder) WriteHeader(code int) { | |
| 305 | - | if r.status == 0 { | |
| 306 | - | r.status = code | |
| 307 | - | } | |
| 308 | - | r.ResponseWriter.WriteHeader(code) | |
| 309 | - | } | |
| 297 | + | // gitProtocolRe is what the Git-Protocol header may carry: colon-separated | |
| 298 | + | // key=value pairs such as "version=2". Git passes it on as GIT_PROTOCOL to | |
| 299 | + | // its subprocesses, so anything else is dropped. | |
| 300 | + | var gitProtocolRe = regexp.MustCompile(`^[a-z0-9._-]+=[a-z0-9._-]+(:[a-z0-9._-]+=[a-z0-9._-]+)*$`) | |
| 310 | 301 | ||
| 311 | - | func (r *recorder) Write(b []byte) (int, error) { | |
| 312 | - | if r.status == 0 { | |
| 313 | - | r.status = http.StatusOK | |
| 314 | - | } | |
| 315 | - | return r.ResponseWriter.Write(b) | |
| 316 | - | } | |
| 302 | + | // maxGitProtocolLen bounds the Git-Protocol header. | |
| 303 | + | const maxGitProtocolLen = 128 | |
| 317 | 304 | ||
| 318 | - | func (r *recorder) code() int { | |
| 319 | - | if r.status == 0 { | |
| 320 | - | return http.StatusOK | |
| 321 | - | } | |
| 322 | - | return r.status | |
| 305 | + | // inheritedEnv are the variables net/http/cgi copies from the ermite process | |
| 306 | + | // on some platforms (osDefaultInheritEnv). Setting them empty keeps the | |
| 307 | + | // environment of the process out of Git and its hooks. | |
| 308 | + | var inheritedEnv = []string{ | |
| 309 | + | "LD_LIBRARY_PATH=", "LD_LIBRARYN32_PATH=", "LD_LIBRARY64_PATH=", | |
| 310 | + | "LD_LIBRARY_PATH_32=", "LD_LIBRARY_PATH_64=", "DYLD_LIBRARY_PATH=", "SHLIB_PATH=", | |
| 323 | 311 | } | |
| 324 | 312 | ||
| 325 | - | // Unwrap lets http.ResponseController reach Flush. | |
| 326 | - | func (r *recorder) Unwrap() http.ResponseWriter { return r.ResponseWriter } | |
| 327 | - | ||
| 328 | - | // sensitiveHeaders never reach the Git process: CGI would put them in its | |
| 329 | - | // environment, which the hooks inherit. | |
| 330 | - | var sensitiveHeaders = []string{"Authorization", "Proxy-Authorization", "Cookie"} | |
| 331 | - | ||
| 332 | 313 | // serveBackend runs git http-backend for an authorized request. | |
| 333 | 314 | func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName, p domain.Principal) { | |
| 334 | 315 | ctx := r.Context() | |
| @@ -340,15 +321,14 @@ func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, | |||
| 340 | 321 | http.Error(w, "internal error", http.StatusInternalServerError) | |
| 341 | 322 | return | |
| 342 | 323 | } | |
| 343 | - | env := be.Env | |
| 344 | - | if rt.suffix == serviceReceive && h.cfg.Hooks != nil { | |
| 345 | - | token, release := h.cfg.Hooks.Register(name, p) | |
| 324 | + | env := append(append([]string(nil), be.Env...), inheritedEnv...) | |
| 325 | + | if rt.suffix == serviceReceive { | |
| 326 | + | token, release := h.cfg.Hooks.Register(ctx, name, p) | |
| 346 | 327 | defer release() | |
| 347 | 328 | env = append(env, EnvHookSocket+"="+h.cfg.Hooks.Socket(), EnvHookToken+"="+token) | |
| 348 | 329 | } | |
| 349 | 330 | ||
| 350 | - | req, cleanup, status := h.backendRequest(w, r, rt, name) | |
| 351 | - | defer cleanup() | |
| 331 | + | req, status := h.backendRequest(w, r, rt, name) | |
| 352 | 332 | if status != 0 { | |
| 353 | 333 | http.Error(w, http.StatusText(status), status) | |
| 354 | 334 | return | |
| @@ -363,54 +343,51 @@ func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, | |||
| 363 | 343 | }).ServeHTTP(w, req) | |
| 364 | 344 | } | |
| 365 | 345 | ||
| 366 | - | // backendRequest builds the request handed to CGI: the path names the | |
| 367 | - | // directory of the repository, credentials are removed, and a body of unknown | |
| 368 | - | // length (chunked, which CGI refuses) is spooled to a temporary file so that | |
| 369 | - | // its length is known. status is non-zero when the request must be refused; | |
| 370 | - | // cleanup is always safe to call. | |
| 371 | - | func (h *Handler) backendRequest(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName) (req *http.Request, cleanup func(), status int) { | |
| 372 | - | cleanup = func() {} | |
| 346 | + | // bodyLimit is the largest request body accepted for the route. | |
| 347 | + | func (h *Handler) bodyLimit(rt route) int64 { | |
| 348 | + | if rt.push() { | |
| 349 | + | return h.maxBody | |
| 350 | + | } | |
| 351 | + | return min(h.maxBody, uploadPackMaxBody) | |
| 352 | + | } | |
| 353 | + | ||
| 354 | + | // backendRequest builds the request handed to CGI. The path names the | |
| 355 | + | // directory of the repository, the query is rebuilt from the validated | |
| 356 | + | // service, and the headers are rebuilt from an allowlist: CGI turns every | |
| 357 | + | // header into an environment variable of Git and of its hooks, so nothing | |
| 358 | + | // else (credentials, cookies, forwarding headers, anything arbitrary) goes | |
| 359 | + | // through. The body is streamed and cut off at the limit; a body of unknown | |
| 360 | + | // length (chunked, which CGI refuses) is passed on with its length unknown. | |
| 361 | + | // status is non-zero when the request must be refused. | |
| 362 | + | func (h *Handler) backendRequest(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName) (req *http.Request, status int) { | |
| 363 | + | limit := h.bodyLimit(rt) | |
| 364 | + | if r.ContentLength > limit { | |
| 365 | + | return nil, http.StatusRequestEntityTooLarge | |
| 366 | + | } | |
| 373 | 367 | req = r.Clone(r.Context()) | |
| 374 | 368 | req.URL.Path = "/" + name.String() + ".git/" + rt.suffix | |
| 375 | 369 | req.URL.RawPath = "" | |
| 376 | - | for _, k := range sensitiveHeaders { | |
| 377 | - | req.Header.Del(k) | |
| 378 | - | } | |
| 379 | - | if r.ContentLength > h.maxBody { | |
| 380 | - | return nil, cleanup, http.StatusRequestEntityTooLarge | |
| 381 | - | } | |
| 382 | - | if r.ContentLength >= 0 && len(r.TransferEncoding) == 0 { | |
| 383 | - | req.Body = http.MaxBytesReader(w, r.Body, h.maxBody) | |
| 384 | - | return req, cleanup, 0 | |
| 370 | + | req.URL.RawQuery = "" | |
| 371 | + | if rt.suffix == "info/refs" { | |
| 372 | + | req.URL.RawQuery = "service=" + rt.service | |
| 385 | 373 | } | |
| 386 | 374 | ||
| 387 | - | f, err := os.CreateTemp(h.cfg.TempDir, "body-*") | |
| 388 | - | if err != nil { | |
| 389 | - | logger := zerowrap.FromCtx(r.Context()) | |
| 390 | - | logger.Error().Err(err).Msg("spool request body") | |
| 391 | - | return nil, cleanup, http.StatusInternalServerError | |
| 392 | - | } | |
| 393 | - | cleanup = func() { | |
| 394 | - | _ = f.Close() | |
| 395 | - | _ = os.Remove(f.Name()) | |
| 396 | - | } | |
| 397 | - | n, err := io.Copy(f, http.MaxBytesReader(w, r.Body, h.maxBody)) | |
| 398 | - | if err != nil { | |
| 399 | - | var tooBig *http.MaxBytesError | |
| 400 | - | if errors.As(err, &tooBig) { | |
| 401 | - | return nil, cleanup, http.StatusRequestEntityTooLarge | |
| 375 | + | req.Header = make(http.Header, 5) | |
| 376 | + | for _, k := range []string{"Content-Type", "Content-Encoding", "Accept", "User-Agent"} { | |
| 377 | + | if v := r.Header.Get(k); v != "" { | |
| 378 | + | req.Header.Set(k, v) | |
| 402 | 379 | } | |
| 403 | - | return nil, cleanup, http.StatusBadRequest | |
| 404 | 380 | } | |
| 405 | - | if _, err := f.Seek(0, io.SeekStart); err != nil { | |
| 406 | - | logger := zerowrap.FromCtx(r.Context()) | |
| 407 | - | logger.Error().Err(err).Msg("spool request body") | |
| 408 | - | return nil, cleanup, http.StatusInternalServerError | |
| 381 | + | if v := r.Header.Get("Git-Protocol"); v != "" && len(v) <= maxGitProtocolLen && gitProtocolRe.MatchString(v) { | |
| 382 | + | req.Header.Set("Git-Protocol", v) | |
| 383 | + | } | |
| 384 | + | ||
| 385 | + | req.Body = http.MaxBytesReader(w, r.Body, limit) | |
| 386 | + | if r.ContentLength < 0 || len(r.TransferEncoding) > 0 { | |
| 387 | + | req.ContentLength = -1 | |
| 388 | + | req.TransferEncoding = nil | |
| 409 | 389 | } | |
| 410 | - | req.Body = f | |
| 411 | - | req.ContentLength = n | |
| 412 | - | req.TransferEncoding = nil | |
| 413 | - | return req, cleanup, 0 | |
| 390 | + | return req, 0 | |
| 414 | 391 | } | |
| 415 | 392 | ||
| 416 | 393 | // logWriter sends what the Git process (or the CGI layer) writes to its | |
| @@ -9,16 +9,14 @@ package githttp | |||||
| 9 | import ( | 9 | import ( | ||
| 10 | "context" | 10 | "context" | ||
| 11 | "errors" | 11 | "errors" | ||
| 12 | - | "io" | |||
| 13 | "log" | 12 | "log" | ||
| 14 | "math" | 13 | "math" | ||
| 15 | "net/http" | 14 | "net/http" | ||
| 16 | "net/http/cgi" | 15 | "net/http/cgi" | ||
| 17 | "net/netip" | 16 | "net/netip" | ||
| 18 | - | "os" | 17 | + | "regexp" |
| 19 | "strconv" | 18 | "strconv" | ||
| 20 | "strings" | 19 | "strings" | ||
| 21 | - | "time" | |||
| 22 | 20 | ||||
| 23 | "github.com/bnema/zerowrap" | 21 | "github.com/bnema/zerowrap" | ||
| 24 | 22 | ||||
| @@ -38,6 +36,11 @@ var Patterns = []string{ | |||||
| 38 | // is zero. | 36 | // is zero. | ||
| 39 | const DefaultMaxBodyBytes int64 = 4 << 30 | 37 | const DefaultMaxBodyBytes int64 = 4 << 30 | ||
| 40 | 38 | ||||
| 39 | + | // uploadPackMaxBody bounds the body of git-upload-pack, which is only a list | |||
| 40 | + | // of wants and haves: git http-backend buffers it whole when its length is | |||
| 41 | + | // unknown, so it must stay small whatever Config.MaxBodyBytes says. | |||
| 42 | + | const uploadPackMaxBody int64 = 64 << 20 | |||
| 43 | + | ||||
| 41 | // Names of the environment variables that carry the hook callback settings | 44 | // Names of the environment variables that carry the hook callback settings | ||
| 42 | // from ermite, through git, to the hook process. | 45 | // from ermite, through git, to the hook process. | ||
| 43 | const ( | 46 | const ( | ||
| @@ -70,7 +73,7 @@ type Repos interface { | |||||
| 70 | // domain.ErrForbidden for one it can see but not write. With create, a missing | 73 | // domain.ErrForbidden for one it can see but not write. With create, a missing | ||
| 71 | // repository the principal may write is created. app.Pushes fits. | 74 | // repository the principal may write is created. app.Pushes fits. | ||
| 72 | type Pushes interface { | 75 | type Pushes interface { | ||
| 73 | - | Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) (repo domain.Repo, created bool, err error) | 76 | + | Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) error |
| 74 | } | 77 | } | ||
| 75 | 78 | ||||
| 76 | // HookTokens hands out the per-request tokens the hooks of a push present | 79 | // HookTokens hands out the per-request tokens the hooks of a push present | ||
| @@ -78,7 +81,7 @@ type Pushes interface { | |||||
| 78 | type HookTokens interface { | 81 | type HookTokens interface { | ||
| 79 | // Register binds a new token to the repository and principal of a push. | 82 | // Register binds a new token to the repository and principal of a push. | ||
| 80 | // release revokes it and must be called when the request ends. | 83 | // release revokes it and must be called when the request ends. | ||
| 81 | - | Register(repo domain.RepoName, p domain.Principal) (token string, release func()) | 84 | + | Register(ctx context.Context, repo domain.RepoName, p domain.Principal) (token string, release func()) |
| 82 | // Socket is the path of the hook socket. | 85 | // Socket is the path of the hook socket. | ||
| 83 | Socket() string | 86 | Socket() string | ||
| 84 | } | 87 | } | ||
| @@ -89,17 +92,14 @@ type Config struct { | |||||
| 89 | Repos Repos | 92 | Repos Repos | ||
| 90 | Pushes Pushes | 93 | Pushes Pushes | ||
| 91 | Backend ports.GitBackend | 94 | Backend ports.GitBackend | ||
| 92 | - | // Hooks provides hook tokens for pushes. Nil runs pushes without hook | 95 | + | // Hooks provides hook tokens for pushes. It is required. |
| 93 | - | // environment (tests of the handler alone). | |||
| 94 | Hooks HookTokens | 96 | Hooks HookTokens | ||
| 95 | // TrustedProxies are the networks allowed to set X-Forwarded-For. | 97 | // TrustedProxies are the networks allowed to set X-Forwarded-For. | ||
| 96 | TrustedProxies []netip.Prefix | 98 | TrustedProxies []netip.Prefix | ||
| 97 | // Limiter counts failed authentications; nil builds a default one. | 99 | // Limiter counts failed authentications; nil builds a default one. | ||
| 98 | Limiter *Limiter | 100 | Limiter *Limiter | ||
| 99 | - | // TempDir holds request bodies that arrive chunked, which CGI cannot | 101 | + | // MaxBodyBytes bounds the body of a push; 0 means DefaultMaxBodyBytes. |
| 100 | - | // stream. Empty uses the system temporary directory. | 102 | + | // The body of a fetch is bounded by 64 MiB at most. |
| 101 | - | TempDir string | |||
| 102 | - | // MaxBodyBytes bounds a request body; 0 means DefaultMaxBodyBytes. | |||
| 103 | MaxBodyBytes int64 | 103 | MaxBodyBytes int64 | ||
| 104 | } | 104 | } | ||
| 105 | 105 | ||||
| @@ -145,9 +145,13 @@ func parseRoute(r *http.Request) (route, int) { | |||||
| 145 | if r.Method != http.MethodGet && r.Method != http.MethodHead { | 145 | if r.Method != http.MethodGet && r.Method != http.MethodHead { | ||
| 146 | return route{}, http.StatusMethodNotAllowed | 146 | return route{}, http.StatusMethodNotAllowed | ||
| 147 | } | 147 | } | ||
| 148 | - | switch svc := r.URL.Query().Get("service"); svc { | 148 | + | // Exactly one service value: a repeated parameter is ambiguous, and |
| 149 | - | case serviceUpload, serviceReceive: | 149 | + | // what is handed to the backend is rebuilt from this one anyway. |
| 150 | - | rt.service = svc | 150 | + | switch svcs := r.URL.Query()["service"]; { |
| 151 | + | case len(svcs) != 1: | |||
| 152 | + | return route{}, http.StatusNotFound | |||
| 153 | + | case svcs[0] == serviceUpload, svcs[0] == serviceReceive: | |||
| 154 | + | rt.service = svcs[0] | |||
| 151 | default: | 155 | default: | ||
| 152 | return route{}, http.StatusNotFound | 156 | return route{}, http.StatusNotFound | ||
| 153 | } | 157 | } | ||
| @@ -164,8 +168,6 @@ func parseRoute(r *http.Request) (route, int) { | |||||
| 164 | 168 | ||||
| 165 | // ServeHTTP implements http.Handler. | 169 | // ServeHTTP implements http.Handler. | ||
| 166 | func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | 170 | func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| 167 | - | start := time.Now() | |||
| 168 | - | rec := &recorder{ResponseWriter: w} | |||
| 169 | var rt route | 171 | var rt route | ||
| 170 | var name domain.RepoName | 172 | var name domain.RepoName | ||
| 171 | var principal domain.Principal | 173 | var principal domain.Principal | ||
| @@ -174,13 +176,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||||
| 174 | log.Debug(). | 176 | log.Debug(). | ||
| 175 | Str("repo", name.String()). | 177 | Str("repo", name.String()). | ||
| 176 | Str("service", rt.service). | 178 | Str("service", rt.service). | ||
| 177 | - | Int("status", rec.code()). | |||
| 178 | - | Dur("duration", time.Since(start)). | |||
| 179 | Str("principal", principal.Label()). | 179 | Str("principal", principal.Label()). | ||
| 180 | Str("token_id", principal.TokenID()). | 180 | Str("token_id", principal.TokenID()). | ||
| 181 | Msg("git request") | 181 | Msg("git request") | ||
| 182 | }() | 182 | }() | ||
| 183 | - | w = rec | |||
| 184 | 183 | ||||
| 185 | rt, status := parseRoute(r) | 184 | rt, status := parseRoute(r) | ||
| 186 | if status != 0 { | 185 | if status != 0 { | ||
| @@ -216,12 +215,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||||
| 216 | 215 | ||||
| 217 | if rt.push() { | 216 | if rt.push() { | ||
| 218 | // Only the first request of a push may create the repository. | 217 | // Only the first request of a push may create the repository. | ||
| 219 | - | _, _, err = h.cfg.Pushes.Open(ctx, principal, name, rt.suffix == "info/refs") | 218 | + | err = h.cfg.Pushes.Open(ctx, principal, name, rt.suffix == "info/refs") |
| 220 | } else { | 219 | } else { | ||
| 221 | _, err = h.cfg.Repos.Get(ctx, principal, name) | 220 | _, err = h.cfg.Repos.Get(ctx, principal, name) | ||
| 222 | } | 221 | } | ||
| 223 | if err != nil { | 222 | if err != nil { | ||
| 224 | - | h.denied(r.Context(), w, principal, err) | 223 | + | h.denied(ctx, w, principal, err) |
| 225 | return | 224 | return | ||
| 226 | } | 225 | } | ||
| 227 | 226 | ||||
| @@ -295,40 +294,22 @@ func challenge(w http.ResponseWriter) { | |||||
| 295 | 294 | ||||
| 296 | func notFound(w http.ResponseWriter) { http.Error(w, "not found", http.StatusNotFound) } | 295 | func notFound(w http.ResponseWriter) { http.Error(w, "not found", http.StatusNotFound) } | ||
| 297 | 296 | ||||
| 298 | - | // recorder remembers the status code of a response and keeps Flush working. | 297 | + | // gitProtocolRe is what the Git-Protocol header may carry: colon-separated |
| 299 | - | type recorder struct { | 298 | + | // key=value pairs such as "version=2". Git passes it on as GIT_PROTOCOL to |
| 300 | - | http.ResponseWriter | 299 | + | // its subprocesses, so anything else is dropped. |
| 301 | - | status int | 300 | + | var gitProtocolRe = regexp.MustCompile(`^[a-z0-9._-]+=[a-z0-9._-]+(:[a-z0-9._-]+=[a-z0-9._-]+)*$`) |
| 302 | - | } | |||
| 303 | - | ||||
| 304 | - | func (r *recorder) WriteHeader(code int) { | |||
| 305 | - | if r.status == 0 { | |||
| 306 | - | r.status = code | |||
| 307 | - | } | |||
| 308 | - | r.ResponseWriter.WriteHeader(code) | |||
| 309 | - | } | |||
| 310 | 301 | ||||
| 311 | - | func (r *recorder) Write(b []byte) (int, error) { | 302 | + | // maxGitProtocolLen bounds the Git-Protocol header. |
| 312 | - | if r.status == 0 { | 303 | + | const maxGitProtocolLen = 128 |
| 313 | - | r.status = http.StatusOK | |||
| 314 | - | } | |||
| 315 | - | return r.ResponseWriter.Write(b) | |||
| 316 | - | } | |||
| 317 | 304 | ||||
| 318 | - | func (r *recorder) code() int { | 305 | + | // inheritedEnv are the variables net/http/cgi copies from the ermite process |
| 319 | - | if r.status == 0 { | 306 | + | // on some platforms (osDefaultInheritEnv). Setting them empty keeps the |
| 320 | - | return http.StatusOK | 307 | + | // environment of the process out of Git and its hooks. |
| 321 | - | } | 308 | + | var inheritedEnv = []string{ |
| 322 | - | return r.status | 309 | + | "LD_LIBRARY_PATH=", "LD_LIBRARYN32_PATH=", "LD_LIBRARY64_PATH=", |
| 310 | + | "LD_LIBRARY_PATH_32=", "LD_LIBRARY_PATH_64=", "DYLD_LIBRARY_PATH=", "SHLIB_PATH=", | |||
| 323 | } | 311 | } | ||
| 324 | 312 | ||||
| 325 | - | // Unwrap lets http.ResponseController reach Flush. | |||
| 326 | - | func (r *recorder) Unwrap() http.ResponseWriter { return r.ResponseWriter } | |||
| 327 | - | ||||
| 328 | - | // sensitiveHeaders never reach the Git process: CGI would put them in its | |||
| 329 | - | // environment, which the hooks inherit. | |||
| 330 | - | var sensitiveHeaders = []string{"Authorization", "Proxy-Authorization", "Cookie"} | |||
| 331 | - | ||||
| 332 | // serveBackend runs git http-backend for an authorized request. | 313 | // serveBackend runs git http-backend for an authorized request. | ||
| 333 | func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName, p domain.Principal) { | 314 | func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName, p domain.Principal) { | ||
| 334 | ctx := r.Context() | 315 | ctx := r.Context() | ||
| @@ -340,15 +321,14 @@ func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, | |||||
| 340 | http.Error(w, "internal error", http.StatusInternalServerError) | 321 | http.Error(w, "internal error", http.StatusInternalServerError) | ||
| 341 | return | 322 | return | ||
| 342 | } | 323 | } | ||
| 343 | - | env := be.Env | 324 | + | env := append(append([]string(nil), be.Env...), inheritedEnv...) |
| 344 | - | if rt.suffix == serviceReceive && h.cfg.Hooks != nil { | 325 | + | if rt.suffix == serviceReceive { |
| 345 | - | token, release := h.cfg.Hooks.Register(name, p) | 326 | + | token, release := h.cfg.Hooks.Register(ctx, name, p) |
| 346 | defer release() | 327 | defer release() | ||
| 347 | env = append(env, EnvHookSocket+"="+h.cfg.Hooks.Socket(), EnvHookToken+"="+token) | 328 | env = append(env, EnvHookSocket+"="+h.cfg.Hooks.Socket(), EnvHookToken+"="+token) | ||
| 348 | } | 329 | } | ||
| 349 | 330 | ||||
| 350 | - | req, cleanup, status := h.backendRequest(w, r, rt, name) | 331 | + | req, status := h.backendRequest(w, r, rt, name) |
| 351 | - | defer cleanup() | |||
| 352 | if status != 0 { | 332 | if status != 0 { | ||
| 353 | http.Error(w, http.StatusText(status), status) | 333 | http.Error(w, http.StatusText(status), status) | ||
| 354 | return | 334 | return | ||
| @@ -363,54 +343,51 @@ func (h *Handler) serveBackend(w http.ResponseWriter, r *http.Request, rt route, | |||||
| 363 | }).ServeHTTP(w, req) | 343 | }).ServeHTTP(w, req) | ||
| 364 | } | 344 | } | ||
| 365 | 345 | ||||
| 366 | - | // backendRequest builds the request handed to CGI: the path names the | 346 | + | // bodyLimit is the largest request body accepted for the route. |
| 367 | - | // directory of the repository, credentials are removed, and a body of unknown | 347 | + | func (h *Handler) bodyLimit(rt route) int64 { |
| 368 | - | // length (chunked, which CGI refuses) is spooled to a temporary file so that | 348 | + | if rt.push() { |
| 369 | - | // its length is known. status is non-zero when the request must be refused; | 349 | + | return h.maxBody |
| 370 | - | // cleanup is always safe to call. | 350 | + | } |
| 371 | - | func (h *Handler) backendRequest(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName) (req *http.Request, cleanup func(), status int) { | 351 | + | return min(h.maxBody, uploadPackMaxBody) |
| 372 | - | cleanup = func() {} | 352 | + | } |
| 353 | + | ||||
| 354 | + | // backendRequest builds the request handed to CGI. The path names the | |||
| 355 | + | // directory of the repository, the query is rebuilt from the validated | |||
| 356 | + | // service, and the headers are rebuilt from an allowlist: CGI turns every | |||
| 357 | + | // header into an environment variable of Git and of its hooks, so nothing | |||
| 358 | + | // else (credentials, cookies, forwarding headers, anything arbitrary) goes | |||
| 359 | + | // through. The body is streamed and cut off at the limit; a body of unknown | |||
| 360 | + | // length (chunked, which CGI refuses) is passed on with its length unknown. | |||
| 361 | + | // status is non-zero when the request must be refused. | |||
| 362 | + | func (h *Handler) backendRequest(w http.ResponseWriter, r *http.Request, rt route, name domain.RepoName) (req *http.Request, status int) { | |||
| 363 | + | limit := h.bodyLimit(rt) | |||
| 364 | + | if r.ContentLength > limit { | |||
| 365 | + | return nil, http.StatusRequestEntityTooLarge | |||
| 366 | + | } | |||
| 373 | req = r.Clone(r.Context()) | 367 | req = r.Clone(r.Context()) | ||
| 374 | req.URL.Path = "/" + name.String() + ".git/" + rt.suffix | 368 | req.URL.Path = "/" + name.String() + ".git/" + rt.suffix | ||
| 375 | req.URL.RawPath = "" | 369 | req.URL.RawPath = "" | ||
| 376 | - | for _, k := range sensitiveHeaders { | 370 | + | req.URL.RawQuery = "" |
| 377 | - | req.Header.Del(k) | 371 | + | if rt.suffix == "info/refs" { |
| 378 | - | } | 372 | + | req.URL.RawQuery = "service=" + rt.service |
| 379 | - | if r.ContentLength > h.maxBody { | |||
| 380 | - | return nil, cleanup, http.StatusRequestEntityTooLarge | |||
| 381 | - | } | |||
| 382 | - | if r.ContentLength >= 0 && len(r.TransferEncoding) == 0 { | |||
| 383 | - | req.Body = http.MaxBytesReader(w, r.Body, h.maxBody) | |||
| 384 | - | return req, cleanup, 0 | |||
| 385 | } | 373 | } | ||
| 386 | 374 | ||||
| 387 | - | f, err := os.CreateTemp(h.cfg.TempDir, "body-*") | 375 | + | req.Header = make(http.Header, 5) |
| 388 | - | if err != nil { | 376 | + | for _, k := range []string{"Content-Type", "Content-Encoding", "Accept", "User-Agent"} { |
| 389 | - | logger := zerowrap.FromCtx(r.Context()) | 377 | + | if v := r.Header.Get(k); v != "" { |
| 390 | - | logger.Error().Err(err).Msg("spool request body") | 378 | + | req.Header.Set(k, v) |
| 391 | - | return nil, cleanup, http.StatusInternalServerError | |||
| 392 | - | } | |||
| 393 | - | cleanup = func() { | |||
| 394 | - | _ = f.Close() | |||
| 395 | - | _ = os.Remove(f.Name()) | |||
| 396 | - | } | |||
| 397 | - | n, err := io.Copy(f, http.MaxBytesReader(w, r.Body, h.maxBody)) | |||
| 398 | - | if err != nil { | |||
| 399 | - | var tooBig *http.MaxBytesError | |||
| 400 | - | if errors.As(err, &tooBig) { | |||
| 401 | - | return nil, cleanup, http.StatusRequestEntityTooLarge | |||
| 402 | } | 379 | } | ||
| 403 | - | return nil, cleanup, http.StatusBadRequest | |||
| 404 | } | 380 | } | ||
| 405 | - | if _, err := f.Seek(0, io.SeekStart); err != nil { | 381 | + | if v := r.Header.Get("Git-Protocol"); v != "" && len(v) <= maxGitProtocolLen && gitProtocolRe.MatchString(v) { |
| 406 | - | logger := zerowrap.FromCtx(r.Context()) | 382 | + | req.Header.Set("Git-Protocol", v) |
| 407 | - | logger.Error().Err(err).Msg("spool request body") | 383 | + | } |
| 408 | - | return nil, cleanup, http.StatusInternalServerError | 384 | + | |
| 385 | + | req.Body = http.MaxBytesReader(w, r.Body, limit) | |||
| 386 | + | if r.ContentLength < 0 || len(r.TransferEncoding) > 0 { | |||
| 387 | + | req.ContentLength = -1 | |||
| 388 | + | req.TransferEncoding = nil | |||
| 409 | } | 389 | } | ||
| 410 | - | req.Body = f | 390 | + | return req, 0 |
| 411 | - | req.ContentLength = n | |||
| 412 | - | req.TransferEncoding = nil | |||
| 413 | - | return req, cleanup, 0 | |||
| 414 | } | 391 | } | ||
| 415 | 392 | ||||
| 416 | // logWriter sends what the Git process (or the CGI layer) writes to its | 393 | // logWriter sends what the Git process (or the CGI layer) writes to its | ||
M internal/adapters/githttp/handler_test.go +93 -22
| @@ -63,14 +63,14 @@ type fakePushes struct { | |||
| 63 | 63 | err error | |
| 64 | 64 | } | |
| 65 | 65 | ||
| 66 | - | func (f *fakePushes) Open(_ context.Context, p domain.Principal, n domain.RepoName, create bool) (domain.Repo, bool, error) { | |
| 66 | + | func (f *fakePushes) Open(_ context.Context, p domain.Principal, _ domain.RepoName, create bool) error { | |
| 67 | 67 | f.mu.Lock() | |
| 68 | 68 | defer f.mu.Unlock() | |
| 69 | 69 | f.calls = append(f.calls, create) | |
| 70 | 70 | if p.IsAnonymous() { | |
| 71 | - | return domain.Repo{}, false, domain.ErrUnauthenticated | |
| 71 | + | return domain.ErrUnauthenticated | |
| 72 | 72 | } | |
| 73 | - | return domain.Repo{Name: n}, false, f.err | |
| 73 | + | return f.err | |
| 74 | 74 | } | |
| 75 | 75 | ||
| 76 | 76 | type fakeTokens struct { | |
| @@ -79,7 +79,7 @@ type fakeTokens struct { | |||
| 79 | 79 | repo domain.RepoName | |
| 80 | 80 | } | |
| 81 | 81 | ||
| 82 | - | func (f *fakeTokens) Register(r domain.RepoName, _ domain.Principal) (string, func()) { | |
| 82 | + | func (f *fakeTokens) Register(_ context.Context, r domain.RepoName, _ domain.Principal) (string, func()) { | |
| 83 | 83 | f.mu.Lock() | |
| 84 | 84 | defer f.mu.Unlock() | |
| 85 | 85 | f.repo = r | |
| @@ -88,11 +88,13 @@ func (f *fakeTokens) Register(r domain.RepoName, _ domain.Principal) (string, fu | |||
| 88 | 88 | func (f *fakeTokens) Socket() string { return "/run/hook.sock" } | |
| 89 | 89 | ||
| 90 | 90 | // cgiScript is a stand-in for git-http-backend: it answers with the | |
| 91 | - | // environment and the body it received. | |
| 91 | + | // environment and the body it received. Like Git, it reads the whole request | |
| 92 | + | // before it answers (net/http cannot read a request body once the response | |
| 93 | + | // has started). | |
| 92 | 94 | func cgiScript(t *testing.T) string { | |
| 93 | 95 | t.Helper() | |
| 94 | 96 | p := filepath.Join(t.TempDir(), "backend") | |
| 95 | - | body := "#!/bin/sh\nprintf 'Content-Type: text/plain\\r\\n\\r\\n'\nenv | sort\nprintf '\\n--body--\\n'\ncat\n" | |
| 97 | + | body := "#!/bin/sh\nin=$(mktemp)\ncat > \"$in\"\nprintf 'Content-Type: text/plain\\r\\n\\r\\n'\nenv | sort\nprintf '\\n--body--\\n'\ncat \"$in\"\nrm -f \"$in\"\n" | |
| 96 | 98 | require.NoError(t, os.WriteFile(p, []byte(body), 0o755)) | |
| 97 | 99 | return p | |
| 98 | 100 | } | |
| @@ -104,7 +106,6 @@ type rig struct { | |||
| 104 | 106 | backend *mocks.GitBackend | |
| 105 | 107 | hooks *fakeTokens | |
| 106 | 108 | auth *fakeAuth | |
| 107 | - | tmp string | |
| 108 | 109 | ||
| 109 | 110 | pushFlags []bool | |
| 110 | 111 | backendErr error | |
| @@ -112,10 +113,9 @@ type rig struct { | |||
| 112 | 113 | ||
| 113 | 114 | func newRig(t *testing.T, mut func(*githttp.Config)) *rig { | |
| 114 | 115 | t.Helper() | |
| 115 | - | r := &rig{pushes: &fakePushes{}, backend: mocks.NewGitBackend(t), hooks: &fakeTokens{}, auth: &fakeAuth{}, tmp: t.TempDir()} | |
| 116 | + | r := &rig{pushes: &fakePushes{}, backend: mocks.NewGitBackend(t), hooks: &fakeTokens{}, auth: &fakeAuth{}} | |
| 116 | 117 | cfg := githttp.Config{ | |
| 117 | 118 | Auth: r.auth, Repos: fakeRepos{}, Pushes: r.pushes, Backend: r.backend, Hooks: r.hooks, | |
| 118 | - | TempDir: r.tmp, | |
| 119 | 119 | } | |
| 120 | 120 | script := cgiScript(t) | |
| 121 | 121 | r.backend.EXPECT().HTTPBackend(mock.Anything, mock.Anything).RunAndReturn( | |
| @@ -254,6 +254,9 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||
| 254 | 254 | "Git-Protocol": "version=2", | |
| 255 | 255 | "Content-Type": "application/x-git-receive-pack-request", | |
| 256 | 256 | "X-Forwarded-For": "1.2.3.4", | |
| 257 | + | "X-Auth-Token": "arbitrary-header-secret", | |
| 258 | + | "Accept": "application/x-git-upload-pack-result", | |
| 259 | + | "User-Agent": "git/2.50", | |
| 257 | 260 | } | |
| 258 | 261 | ||
| 259 | 262 | // A read: no hook environment. | |
| @@ -272,6 +275,8 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||
| 272 | 275 | assert.NotContains(t, e, "HTTP_AUTHORIZATION") | |
| 273 | 276 | assert.NotContains(t, e, "HTTP_PROXY_AUTHORIZATION") | |
| 274 | 277 | assert.NotContains(t, e, "HTTP_COOKIE") | |
| 278 | + | assert.Equal(t, "application/x-git-upload-pack-result", e["HTTP_ACCEPT"]) | |
| 279 | + | assert.Equal(t, "git/2.50", e["HTTP_USER_AGENT"]) | |
| 275 | 280 | assert.Equal(t, []bool{false}, r.pushFlags) | |
| 276 | 281 | ||
| 277 | 282 | // A receive-pack POST gets the hook environment and releases the token. | |
| @@ -286,6 +291,9 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||
| 286 | 291 | assert.Equal(t, "8", e["CONTENT_LENGTH"]) | |
| 287 | 292 | assert.True(t, strings.HasSuffix(body, "\n--body--\nPACKDATA"), "the body reaches the backend") | |
| 288 | 293 | assert.NotContains(t, body, "root-secret") | |
| 294 | + | assert.NotContains(t, body, "arbitrary-header-secret", "only allowlisted headers reach the backend, and so its hooks") | |
| 295 | + | assert.NotContains(t, e, "HTTP_X_AUTH_TOKEN") | |
| 296 | + | assert.NotContains(t, e, "HTTP_X_FORWARDED_FOR") | |
| 289 | 297 | assert.Equal(t, 1, r.hooks.released, "the hook token is revoked when the request ends") | |
| 290 | 298 | assert.Equal(t, "pub", r.hooks.repo.String()) | |
| 291 | 299 | ||
| @@ -300,7 +308,7 @@ func TestHandlerNormalizesRepositoryPath(t *testing.T) { | |||
| 300 | 308 | assert.Equal(t, "/pub.git/info/refs", envOf(body)["PATH_INFO"], "the backend always sees <name>.git") | |
| 301 | 309 | } | |
| 302 | 310 | ||
| 303 | - | func TestHandlerChunkedBodyIsSpooled(t *testing.T) { | |
| 311 | + | func TestHandlerChunkedBodyIsStreamed(t *testing.T) { | |
| 304 | 312 | r := newRig(t, nil) | |
| 305 | 313 | payload := strings.Repeat("x", 300_000) | |
| 306 | 314 | // An io.Reader of unknown length makes the client send chunked. | |
| @@ -308,26 +316,89 @@ func TestHandlerChunkedBodyIsSpooled(t *testing.T) { | |||
| 308 | 316 | code, out, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", body, map[string]string{"Content-Type": "application/x-git-upload-pack-request"}) | |
| 309 | 317 | require.Equal(t, 200, code) | |
| 310 | 318 | e := envOf(out) | |
| 311 | - | assert.Equal(t, "300000", e["CONTENT_LENGTH"], "the length is known to the backend") | |
| 319 | + | assert.NotContains(t, e, "CONTENT_LENGTH", "the length is unknown to the backend: the body is streamed, not buffered") | |
| 320 | + | assert.NotContains(t, e, "HTTP_TRANSFER_ENCODING") | |
| 312 | 321 | _, got, _ := strings.Cut(out, "\n--body--\n") | |
| 313 | - | assert.Equal(t, payload, got) | |
| 314 | - | entries, err := os.ReadDir(r.tmp) | |
| 315 | - | require.NoError(t, err) | |
| 316 | - | assert.Empty(t, entries, "spool files are removed") | |
| 322 | + | assert.Equal(t, len(payload), len(got)) | |
| 323 | + | assert.True(t, payload == got, "the body arrives intact") | |
| 317 | 324 | } | |
| 318 | 325 | ||
| 319 | 326 | func TestHandlerBodyLimit(t *testing.T) { | |
| 320 | 327 | r := newRig(t, func(c *githttp.Config) { c.MaxBodyBytes = 1000 }) | |
| 321 | 328 | big := strings.Repeat("x", 5000) | |
| 322 | - | code, _, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", strings.NewReader(big), nil) | |
| 329 | + | code, _, _ := r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", strings.NewReader(big), nil) | |
| 323 | 330 | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "declared length over the limit") | |
| 324 | - | code, _, _ = r.do(t, "POST", "/pub.git/git-upload-pack", "", "", io.MultiReader(strings.NewReader(big)), nil) | |
| 325 | - | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "chunked body over the limit") | |
| 326 | - | code, _, _ = r.do(t, "POST", "/pub.git/git-upload-pack", "", "", strings.NewReader("ok"), nil) | |
| 331 | + | ||
| 332 | + | // A chunked body has no declared length: it is cut off at the limit, so | |
| 333 | + | // the backend sees at most the limit and never the rest. | |
| 334 | + | code, out, _ := r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", io.MultiReader(strings.NewReader(big)), nil) | |
| 335 | + | _, got, _ := strings.Cut(out, "\n--body--\n") | |
| 336 | + | assert.LessOrEqual(t, len(got), 1000, "the body is cut off at the limit (status %d)", code) | |
| 337 | + | assert.NotEqual(t, big, got) | |
| 338 | + | ||
| 339 | + | code, _, _ = r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", strings.NewReader("ok"), nil) | |
| 327 | 340 | assert.Equal(t, 200, code) | |
| 328 | - | entries, err := os.ReadDir(r.tmp) | |
| 329 | - | require.NoError(t, err) | |
| 330 | - | assert.Empty(t, entries) | |
| 341 | + | } | |
| 342 | + | ||
| 343 | + | func TestHandlerUploadPackBodyIsBoundedBelowThePushLimit(t *testing.T) { | |
| 344 | + | r := newRig(t, nil) // the push limit is 4 GiB | |
| 345 | + | big := strings.NewReader(strings.Repeat("x", 64<<20+1)) | |
| 346 | + | code, _, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", big, nil) | |
| 347 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "a fetch request is a short list of wants") | |
| 348 | + | } | |
| 349 | + | ||
| 350 | + | func TestHandlerServiceQuery(t *testing.T) { | |
| 351 | + | r := newRig(t, nil) | |
| 352 | + | for _, q := range []string{ | |
| 353 | + | "service=git-upload-pack&service=git-receive-pack", | |
| 354 | + | "service=git-upload-pack&service=git-upload-pack", | |
| 355 | + | "service=git-upload-pack&service=", | |
| 356 | + | "service=", | |
| 357 | + | } { | |
| 358 | + | code, _, _ := r.do(t, "GET", "/pub.git/info/refs?"+q, "u", "root-secret", nil, nil) | |
| 359 | + | assert.Equal(t, 404, code, q) | |
| 360 | + | } | |
| 361 | + | // Other parameters do not reach the backend; the service is rebuilt. | |
| 362 | + | code, body, _ := r.do(t, "GET", "/pub.git/info/refs?x=1&service=git-upload-pack&y=%0a", "", "", nil, nil) | |
| 363 | + | require.Equal(t, 200, code) | |
| 364 | + | assert.Equal(t, "service=git-upload-pack", envOf(body)["QUERY_STRING"]) | |
| 365 | + | } | |
| 366 | + | ||
| 367 | + | func TestHandlerGitProtocolHeader(t *testing.T) { | |
| 368 | + | r := newRig(t, nil) | |
| 369 | + | tests := []struct { | |
| 370 | + | value string | |
| 371 | + | want string | |
| 372 | + | }{ | |
| 373 | + | {"version=2", "version=2"}, | |
| 374 | + | {"version=1", "version=1"}, | |
| 375 | + | {"version=2:object-format=sha1", "version=2:object-format=sha1"}, | |
| 376 | + | {"VERSION=2", ""}, | |
| 377 | + | {"version=2;rm", ""}, | |
| 378 | + | {"version", ""}, | |
| 379 | + | {"version=2:", ""}, | |
| 380 | + | {"a=b c", ""}, | |
| 381 | + | {"version=" + strings.Repeat("2", 200), ""}, | |
| 382 | + | } | |
| 383 | + | for _, tt := range tests { | |
| 384 | + | _, body, _ := r.do(t, "GET", "/pub.git/info/refs?service=git-upload-pack", "", "", nil, map[string]string{"Git-Protocol": tt.value}) | |
| 385 | + | e := envOf(body) | |
| 386 | + | if tt.want == "" { | |
| 387 | + | assert.NotContains(t, e, "HTTP_GIT_PROTOCOL", tt.value) | |
| 388 | + | } else { | |
| 389 | + | assert.Equal(t, tt.want, e["HTTP_GIT_PROTOCOL"], tt.value) | |
| 390 | + | } | |
| 391 | + | } | |
| 392 | + | } | |
| 393 | + | ||
| 394 | + | func TestHandlerDoesNotInheritTheProcessEnvironment(t *testing.T) { | |
| 395 | + | t.Setenv("LD_LIBRARY_PATH", "/evil/lib") | |
| 396 | + | t.Setenv("ERMITE_ADMIN_TOKEN", "must-not-leak") | |
| 397 | + | r := newRig(t, nil) | |
| 398 | + | _, body, _ := r.do(t, "GET", "/pub.git/info/refs?service=git-upload-pack", "", "", nil, nil) | |
| 399 | + | e := envOf(body) | |
| 400 | + | assert.Empty(t, e["LD_LIBRARY_PATH"], "net/http/cgi inherits it by default") | |
| 401 | + | assert.NotContains(t, body, "must-not-leak") | |
| 331 | 402 | } | |
| 332 | 403 | ||
| 333 | 404 | func TestHandlerBackendFailure(t *testing.T) { | |
| @@ -63,14 +63,14 @@ type fakePushes struct { | |||||
| 63 | err error | 63 | err error | ||
| 64 | } | 64 | } | ||
| 65 | 65 | ||||
| 66 | - | func (f *fakePushes) Open(_ context.Context, p domain.Principal, n domain.RepoName, create bool) (domain.Repo, bool, error) { | 66 | + | func (f *fakePushes) Open(_ context.Context, p domain.Principal, _ domain.RepoName, create bool) error { |
| 67 | f.mu.Lock() | 67 | f.mu.Lock() | ||
| 68 | defer f.mu.Unlock() | 68 | defer f.mu.Unlock() | ||
| 69 | f.calls = append(f.calls, create) | 69 | f.calls = append(f.calls, create) | ||
| 70 | if p.IsAnonymous() { | 70 | if p.IsAnonymous() { | ||
| 71 | - | return domain.Repo{}, false, domain.ErrUnauthenticated | 71 | + | return domain.ErrUnauthenticated |
| 72 | } | 72 | } | ||
| 73 | - | return domain.Repo{Name: n}, false, f.err | 73 | + | return f.err |
| 74 | } | 74 | } | ||
| 75 | 75 | ||||
| 76 | type fakeTokens struct { | 76 | type fakeTokens struct { | ||
| @@ -79,7 +79,7 @@ type fakeTokens struct { | |||||
| 79 | repo domain.RepoName | 79 | repo domain.RepoName | ||
| 80 | } | 80 | } | ||
| 81 | 81 | ||||
| 82 | - | func (f *fakeTokens) Register(r domain.RepoName, _ domain.Principal) (string, func()) { | 82 | + | func (f *fakeTokens) Register(_ context.Context, r domain.RepoName, _ domain.Principal) (string, func()) { |
| 83 | f.mu.Lock() | 83 | f.mu.Lock() | ||
| 84 | defer f.mu.Unlock() | 84 | defer f.mu.Unlock() | ||
| 85 | f.repo = r | 85 | f.repo = r | ||
| @@ -88,11 +88,13 @@ func (f *fakeTokens) Register(r domain.RepoName, _ domain.Principal) (string, fu | |||||
| 88 | func (f *fakeTokens) Socket() string { return "/run/hook.sock" } | 88 | func (f *fakeTokens) Socket() string { return "/run/hook.sock" } | ||
| 89 | 89 | ||||
| 90 | // cgiScript is a stand-in for git-http-backend: it answers with the | 90 | // cgiScript is a stand-in for git-http-backend: it answers with the | ||
| 91 | - | // environment and the body it received. | 91 | + | // environment and the body it received. Like Git, it reads the whole request |
| 92 | + | // before it answers (net/http cannot read a request body once the response | |||
| 93 | + | // has started). | |||
| 92 | func cgiScript(t *testing.T) string { | 94 | func cgiScript(t *testing.T) string { | ||
| 93 | t.Helper() | 95 | t.Helper() | ||
| 94 | p := filepath.Join(t.TempDir(), "backend") | 96 | p := filepath.Join(t.TempDir(), "backend") | ||
| 95 | - | body := "#!/bin/sh\nprintf 'Content-Type: text/plain\\r\\n\\r\\n'\nenv | sort\nprintf '\\n--body--\\n'\ncat\n" | 97 | + | body := "#!/bin/sh\nin=$(mktemp)\ncat > \"$in\"\nprintf 'Content-Type: text/plain\\r\\n\\r\\n'\nenv | sort\nprintf '\\n--body--\\n'\ncat \"$in\"\nrm -f \"$in\"\n" |
| 96 | require.NoError(t, os.WriteFile(p, []byte(body), 0o755)) | 98 | require.NoError(t, os.WriteFile(p, []byte(body), 0o755)) | ||
| 97 | return p | 99 | return p | ||
| 98 | } | 100 | } | ||
| @@ -104,7 +106,6 @@ type rig struct { | |||||
| 104 | backend *mocks.GitBackend | 106 | backend *mocks.GitBackend | ||
| 105 | hooks *fakeTokens | 107 | hooks *fakeTokens | ||
| 106 | auth *fakeAuth | 108 | auth *fakeAuth | ||
| 107 | - | tmp string | |||
| 108 | 109 | ||||
| 109 | pushFlags []bool | 110 | pushFlags []bool | ||
| 110 | backendErr error | 111 | backendErr error | ||
| @@ -112,10 +113,9 @@ type rig struct { | |||||
| 112 | 113 | ||||
| 113 | func newRig(t *testing.T, mut func(*githttp.Config)) *rig { | 114 | func newRig(t *testing.T, mut func(*githttp.Config)) *rig { | ||
| 114 | t.Helper() | 115 | t.Helper() | ||
| 115 | - | r := &rig{pushes: &fakePushes{}, backend: mocks.NewGitBackend(t), hooks: &fakeTokens{}, auth: &fakeAuth{}, tmp: t.TempDir()} | 116 | + | r := &rig{pushes: &fakePushes{}, backend: mocks.NewGitBackend(t), hooks: &fakeTokens{}, auth: &fakeAuth{}} |
| 116 | cfg := githttp.Config{ | 117 | cfg := githttp.Config{ | ||
| 117 | Auth: r.auth, Repos: fakeRepos{}, Pushes: r.pushes, Backend: r.backend, Hooks: r.hooks, | 118 | Auth: r.auth, Repos: fakeRepos{}, Pushes: r.pushes, Backend: r.backend, Hooks: r.hooks, | ||
| 118 | - | TempDir: r.tmp, | |||
| 119 | } | 119 | } | ||
| 120 | script := cgiScript(t) | 120 | script := cgiScript(t) | ||
| 121 | r.backend.EXPECT().HTTPBackend(mock.Anything, mock.Anything).RunAndReturn( | 121 | r.backend.EXPECT().HTTPBackend(mock.Anything, mock.Anything).RunAndReturn( | ||
| @@ -254,6 +254,9 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||||
| 254 | "Git-Protocol": "version=2", | 254 | "Git-Protocol": "version=2", | ||
| 255 | "Content-Type": "application/x-git-receive-pack-request", | 255 | "Content-Type": "application/x-git-receive-pack-request", | ||
| 256 | "X-Forwarded-For": "1.2.3.4", | 256 | "X-Forwarded-For": "1.2.3.4", | ||
| 257 | + | "X-Auth-Token": "arbitrary-header-secret", | |||
| 258 | + | "Accept": "application/x-git-upload-pack-result", | |||
| 259 | + | "User-Agent": "git/2.50", | |||
| 257 | } | 260 | } | ||
| 258 | 261 | ||||
| 259 | // A read: no hook environment. | 262 | // A read: no hook environment. | ||
| @@ -272,6 +275,8 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||||
| 272 | assert.NotContains(t, e, "HTTP_AUTHORIZATION") | 275 | assert.NotContains(t, e, "HTTP_AUTHORIZATION") | ||
| 273 | assert.NotContains(t, e, "HTTP_PROXY_AUTHORIZATION") | 276 | assert.NotContains(t, e, "HTTP_PROXY_AUTHORIZATION") | ||
| 274 | assert.NotContains(t, e, "HTTP_COOKIE") | 277 | assert.NotContains(t, e, "HTTP_COOKIE") | ||
| 278 | + | assert.Equal(t, "application/x-git-upload-pack-result", e["HTTP_ACCEPT"]) | |||
| 279 | + | assert.Equal(t, "git/2.50", e["HTTP_USER_AGENT"]) | |||
| 275 | assert.Equal(t, []bool{false}, r.pushFlags) | 280 | assert.Equal(t, []bool{false}, r.pushFlags) | ||
| 276 | 281 | ||||
| 277 | // A receive-pack POST gets the hook environment and releases the token. | 282 | // A receive-pack POST gets the hook environment and releases the token. | ||
| @@ -286,6 +291,9 @@ func TestHandlerBackendEnvironment(t *testing.T) { | |||||
| 286 | assert.Equal(t, "8", e["CONTENT_LENGTH"]) | 291 | assert.Equal(t, "8", e["CONTENT_LENGTH"]) | ||
| 287 | assert.True(t, strings.HasSuffix(body, "\n--body--\nPACKDATA"), "the body reaches the backend") | 292 | assert.True(t, strings.HasSuffix(body, "\n--body--\nPACKDATA"), "the body reaches the backend") | ||
| 288 | assert.NotContains(t, body, "root-secret") | 293 | assert.NotContains(t, body, "root-secret") | ||
| 294 | + | assert.NotContains(t, body, "arbitrary-header-secret", "only allowlisted headers reach the backend, and so its hooks") | |||
| 295 | + | assert.NotContains(t, e, "HTTP_X_AUTH_TOKEN") | |||
| 296 | + | assert.NotContains(t, e, "HTTP_X_FORWARDED_FOR") | |||
| 289 | assert.Equal(t, 1, r.hooks.released, "the hook token is revoked when the request ends") | 297 | assert.Equal(t, 1, r.hooks.released, "the hook token is revoked when the request ends") | ||
| 290 | assert.Equal(t, "pub", r.hooks.repo.String()) | 298 | assert.Equal(t, "pub", r.hooks.repo.String()) | ||
| 291 | 299 | ||||
| @@ -300,7 +308,7 @@ func TestHandlerNormalizesRepositoryPath(t *testing.T) { | |||||
| 300 | assert.Equal(t, "/pub.git/info/refs", envOf(body)["PATH_INFO"], "the backend always sees <name>.git") | 308 | assert.Equal(t, "/pub.git/info/refs", envOf(body)["PATH_INFO"], "the backend always sees <name>.git") | ||
| 301 | } | 309 | } | ||
| 302 | 310 | ||||
| 303 | - | func TestHandlerChunkedBodyIsSpooled(t *testing.T) { | 311 | + | func TestHandlerChunkedBodyIsStreamed(t *testing.T) { |
| 304 | r := newRig(t, nil) | 312 | r := newRig(t, nil) | ||
| 305 | payload := strings.Repeat("x", 300_000) | 313 | payload := strings.Repeat("x", 300_000) | ||
| 306 | // An io.Reader of unknown length makes the client send chunked. | 314 | // An io.Reader of unknown length makes the client send chunked. | ||
| @@ -308,26 +316,89 @@ func TestHandlerChunkedBodyIsSpooled(t *testing.T) { | |||||
| 308 | code, out, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", body, map[string]string{"Content-Type": "application/x-git-upload-pack-request"}) | 316 | code, out, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", body, map[string]string{"Content-Type": "application/x-git-upload-pack-request"}) | ||
| 309 | require.Equal(t, 200, code) | 317 | require.Equal(t, 200, code) | ||
| 310 | e := envOf(out) | 318 | e := envOf(out) | ||
| 311 | - | assert.Equal(t, "300000", e["CONTENT_LENGTH"], "the length is known to the backend") | 319 | + | assert.NotContains(t, e, "CONTENT_LENGTH", "the length is unknown to the backend: the body is streamed, not buffered") |
| 320 | + | assert.NotContains(t, e, "HTTP_TRANSFER_ENCODING") | |||
| 312 | _, got, _ := strings.Cut(out, "\n--body--\n") | 321 | _, got, _ := strings.Cut(out, "\n--body--\n") | ||
| 313 | - | assert.Equal(t, payload, got) | 322 | + | assert.Equal(t, len(payload), len(got)) |
| 314 | - | entries, err := os.ReadDir(r.tmp) | 323 | + | assert.True(t, payload == got, "the body arrives intact") |
| 315 | - | require.NoError(t, err) | |||
| 316 | - | assert.Empty(t, entries, "spool files are removed") | |||
| 317 | } | 324 | } | ||
| 318 | 325 | ||||
| 319 | func TestHandlerBodyLimit(t *testing.T) { | 326 | func TestHandlerBodyLimit(t *testing.T) { | ||
| 320 | r := newRig(t, func(c *githttp.Config) { c.MaxBodyBytes = 1000 }) | 327 | r := newRig(t, func(c *githttp.Config) { c.MaxBodyBytes = 1000 }) | ||
| 321 | big := strings.Repeat("x", 5000) | 328 | big := strings.Repeat("x", 5000) | ||
| 322 | - | code, _, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", strings.NewReader(big), nil) | 329 | + | code, _, _ := r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", strings.NewReader(big), nil) |
| 323 | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "declared length over the limit") | 330 | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "declared length over the limit") | ||
| 324 | - | code, _, _ = r.do(t, "POST", "/pub.git/git-upload-pack", "", "", io.MultiReader(strings.NewReader(big)), nil) | 331 | + | |
| 325 | - | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "chunked body over the limit") | 332 | + | // A chunked body has no declared length: it is cut off at the limit, so |
| 326 | - | code, _, _ = r.do(t, "POST", "/pub.git/git-upload-pack", "", "", strings.NewReader("ok"), nil) | 333 | + | // the backend sees at most the limit and never the rest. |
| 334 | + | code, out, _ := r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", io.MultiReader(strings.NewReader(big)), nil) | |||
| 335 | + | _, got, _ := strings.Cut(out, "\n--body--\n") | |||
| 336 | + | assert.LessOrEqual(t, len(got), 1000, "the body is cut off at the limit (status %d)", code) | |||
| 337 | + | assert.NotEqual(t, big, got) | |||
| 338 | + | ||||
| 339 | + | code, _, _ = r.do(t, "POST", "/pub.git/git-receive-pack", "u", "root-secret", strings.NewReader("ok"), nil) | |||
| 327 | assert.Equal(t, 200, code) | 340 | assert.Equal(t, 200, code) | ||
| 328 | - | entries, err := os.ReadDir(r.tmp) | 341 | + | } |
| 329 | - | require.NoError(t, err) | 342 | + | |
| 330 | - | assert.Empty(t, entries) | 343 | + | func TestHandlerUploadPackBodyIsBoundedBelowThePushLimit(t *testing.T) { |
| 344 | + | r := newRig(t, nil) // the push limit is 4 GiB | |||
| 345 | + | big := strings.NewReader(strings.Repeat("x", 64<<20+1)) | |||
| 346 | + | code, _, _ := r.do(t, "POST", "/pub.git/git-upload-pack", "", "", big, nil) | |||
| 347 | + | assert.Equal(t, http.StatusRequestEntityTooLarge, code, "a fetch request is a short list of wants") | |||
| 348 | + | } | |||
| 349 | + | ||||
| 350 | + | func TestHandlerServiceQuery(t *testing.T) { | |||
| 351 | + | r := newRig(t, nil) | |||
| 352 | + | for _, q := range []string{ | |||
| 353 | + | "service=git-upload-pack&service=git-receive-pack", | |||
| 354 | + | "service=git-upload-pack&service=git-upload-pack", | |||
| 355 | + | "service=git-upload-pack&service=", | |||
| 356 | + | "service=", | |||
| 357 | + | } { | |||
| 358 | + | code, _, _ := r.do(t, "GET", "/pub.git/info/refs?"+q, "u", "root-secret", nil, nil) | |||
| 359 | + | assert.Equal(t, 404, code, q) | |||
| 360 | + | } | |||
| 361 | + | // Other parameters do not reach the backend; the service is rebuilt. | |||
| 362 | + | code, body, _ := r.do(t, "GET", "/pub.git/info/refs?x=1&service=git-upload-pack&y=%0a", "", "", nil, nil) | |||
| 363 | + | require.Equal(t, 200, code) | |||
| 364 | + | assert.Equal(t, "service=git-upload-pack", envOf(body)["QUERY_STRING"]) | |||
| 365 | + | } | |||
| 366 | + | ||||
| 367 | + | func TestHandlerGitProtocolHeader(t *testing.T) { | |||
| 368 | + | r := newRig(t, nil) | |||
| 369 | + | tests := []struct { | |||
| 370 | + | value string | |||
| 371 | + | want string | |||
| 372 | + | }{ | |||
| 373 | + | {"version=2", "version=2"}, | |||
| 374 | + | {"version=1", "version=1"}, | |||
| 375 | + | {"version=2:object-format=sha1", "version=2:object-format=sha1"}, | |||
| 376 | + | {"VERSION=2", ""}, | |||
| 377 | + | {"version=2;rm", ""}, | |||
| 378 | + | {"version", ""}, | |||
| 379 | + | {"version=2:", ""}, | |||
| 380 | + | {"a=b c", ""}, | |||
| 381 | + | {"version=" + strings.Repeat("2", 200), ""}, | |||
| 382 | + | } | |||
| 383 | + | for _, tt := range tests { | |||
| 384 | + | _, body, _ := r.do(t, "GET", "/pub.git/info/refs?service=git-upload-pack", "", "", nil, map[string]string{"Git-Protocol": tt.value}) | |||
| 385 | + | e := envOf(body) | |||
| 386 | + | if tt.want == "" { | |||
| 387 | + | assert.NotContains(t, e, "HTTP_GIT_PROTOCOL", tt.value) | |||
| 388 | + | } else { | |||
| 389 | + | assert.Equal(t, tt.want, e["HTTP_GIT_PROTOCOL"], tt.value) | |||
| 390 | + | } | |||
| 391 | + | } | |||
| 392 | + | } | |||
| 393 | + | ||||
| 394 | + | func TestHandlerDoesNotInheritTheProcessEnvironment(t *testing.T) { | |||
| 395 | + | t.Setenv("LD_LIBRARY_PATH", "/evil/lib") | |||
| 396 | + | t.Setenv("ERMITE_ADMIN_TOKEN", "must-not-leak") | |||
| 397 | + | r := newRig(t, nil) | |||
| 398 | + | _, body, _ := r.do(t, "GET", "/pub.git/info/refs?service=git-upload-pack", "", "", nil, nil) | |||
| 399 | + | e := envOf(body) | |||
| 400 | + | assert.Empty(t, e["LD_LIBRARY_PATH"], "net/http/cgi inherits it by default") | |||
| 401 | + | assert.NotContains(t, body, "must-not-leak") | |||
| 331 | } | 402 | } | ||
| 332 | 403 | ||||
| 333 | func TestHandlerBackendFailure(t *testing.T) { | 404 | func TestHandlerBackendFailure(t *testing.T) { | ||
M internal/adapters/githttp/hooks.go +19 -8
| @@ -33,6 +33,10 @@ type PushHooks interface { | |||
| 33 | 33 | // readTimeout bounds how long a connection may take to send its request line. | |
| 34 | 34 | const readTimeout = 30 * time.Second | |
| 35 | 35 | ||
| 36 | + | // recordTimeout bounds the post-receive recording of a push, which outlives | |
| 37 | + | // the shutdown of the server. | |
| 38 | + | const recordTimeout = 30 * time.Second | |
| 39 | + | ||
| 36 | 40 | type hookEntry struct { | |
| 37 | 41 | repo domain.RepoName | |
| 38 | 42 | principal domain.Principal | |
| @@ -65,16 +69,15 @@ func (s *HookServer) Socket() string { return s.path } | |||
| 65 | 69 | ||
| 66 | 70 | // Register binds a fresh random token to the repository and principal of a | |
| 67 | 71 | // push. The returned release revokes it. | |
| 68 | - | func (s *HookServer) Register(repo domain.RepoName, p domain.Principal) (string, func()) { | |
| 72 | + | func (s *HookServer) Register(ctx context.Context, repo domain.RepoName, p domain.Principal) (string, func()) { | |
| 69 | 73 | var raw [32]byte | |
| 70 | - | if _, err := rand.Read(raw[:]); err != nil { | |
| 71 | - | // No entropy: hand out a token that matches nothing, so the hooks fail closed. | |
| 72 | - | return "unavailable", func() {} | |
| 73 | - | } | |
| 74 | + | _, _ = rand.Read(raw[:]) // never fails | |
| 74 | 75 | token := hex.EncodeToString(raw[:]) | |
| 75 | 76 | s.mu.Lock() | |
| 76 | 77 | s.tokens[token] = hookEntry{repo: repo, principal: p} | |
| 77 | 78 | s.mu.Unlock() | |
| 79 | + | log := zerowrap.FromCtx(ctx) | |
| 80 | + | log.Debug().Str("repo", repo.String()).Str("token_id", p.TokenID()).Msg("hook token registered") | |
| 78 | 81 | return token, func() { | |
| 79 | 82 | s.mu.Lock() | |
| 80 | 83 | delete(s.tokens, token) | |
| @@ -91,7 +94,7 @@ func (s *HookServer) lookup(token string) (hookEntry, bool) { | |||
| 91 | 94 | ||
| 92 | 95 | // Listen creates the socket with mode 0600 (replacing a stale file) and | |
| 93 | 96 | // returns once it accepts connections. Serve then handles them. | |
| 94 | - | func (s *HookServer) Listen() error { | |
| 97 | + | func (s *HookServer) Listen(ctx context.Context) error { | |
| 95 | 98 | // sun_path holds 108 bytes on Linux, 104 on the BSDs and macOS. | |
| 96 | 99 | if len(s.path) >= 100 { | |
| 97 | 100 | return fmt.Errorf("hook socket path is too long for a Unix socket (%d bytes, limit about 100): %s; use a shorter %s", len(s.path), s.path, "ERMITE_DATA_DIR") | |
| @@ -110,6 +113,8 @@ func (s *HookServer) Listen() error { | |||
| 110 | 113 | s.connMu.Lock() | |
| 111 | 114 | s.ln = ln | |
| 112 | 115 | s.connMu.Unlock() | |
| 116 | + | log := zerowrap.FromCtx(ctx) | |
| 117 | + | log.Debug().Str("socket", s.path).Msg("hook socket created") | |
| 113 | 118 | return nil | |
| 114 | 119 | } | |
| 115 | 120 | ||
| @@ -164,7 +169,9 @@ func (s *HookServer) Serve(ctx context.Context) error { | |||
| 164 | 169 | } | |
| 165 | 170 | ||
| 166 | 171 | // Close stops listening and drops the connections in progress. | |
| 167 | - | func (s *HookServer) Close() { | |
| 172 | + | func (s *HookServer) Close(ctx context.Context) { | |
| 173 | + | log := zerowrap.FromCtx(ctx) | |
| 174 | + | log.Debug().Str("socket", s.path).Msg("closing hook socket") | |
| 168 | 175 | s.connMu.Lock() | |
| 169 | 176 | defer s.connMu.Unlock() | |
| 170 | 177 | if s.ln != nil { | |
| @@ -221,7 +228,11 @@ func (s *HookServer) handle(ctx context.Context, conn net.Conn) { | |||
| 221 | 228 | s.reply(ctx, conn, HookReply{Message: "ermite: push refused: internal error"}) | |
| 222 | 229 | } | |
| 223 | 230 | case HookPostReceive: | |
| 224 | - | if err := s.hooks.Record(ctx, entry.principal, entry.repo, updates); err != nil { | |
| 231 | + | // The push is already in the repository: recording it must not be | |
| 232 | + | // dropped because the server is shutting down. | |
| 233 | + | rctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), recordTimeout) | |
| 234 | + | defer cancel() | |
| 235 | + | if err := s.hooks.Record(rctx, entry.principal, entry.repo, updates); err != nil { | |
| 225 | 236 | rlog.Error().Err(err).Msg("post-receive recording failed") | |
| 226 | 237 | s.reply(ctx, conn, HookReply{OK: true, Message: "ermite: the push succeeded but recording it failed"}) | |
| 227 | 238 | return | |
| @@ -33,6 +33,10 @@ type PushHooks interface { | |||||
| 33 | // readTimeout bounds how long a connection may take to send its request line. | 33 | // readTimeout bounds how long a connection may take to send its request line. | ||
| 34 | const readTimeout = 30 * time.Second | 34 | const readTimeout = 30 * time.Second | ||
| 35 | 35 | ||||
| 36 | + | // recordTimeout bounds the post-receive recording of a push, which outlives | |||
| 37 | + | // the shutdown of the server. | |||
| 38 | + | const recordTimeout = 30 * time.Second | |||
| 39 | + | ||||
| 36 | type hookEntry struct { | 40 | type hookEntry struct { | ||
| 37 | repo domain.RepoName | 41 | repo domain.RepoName | ||
| 38 | principal domain.Principal | 42 | principal domain.Principal | ||
| @@ -65,16 +69,15 @@ func (s *HookServer) Socket() string { return s.path } | |||||
| 65 | 69 | ||||
| 66 | // Register binds a fresh random token to the repository and principal of a | 70 | // Register binds a fresh random token to the repository and principal of a | ||
| 67 | // push. The returned release revokes it. | 71 | // push. The returned release revokes it. | ||
| 68 | - | func (s *HookServer) Register(repo domain.RepoName, p domain.Principal) (string, func()) { | 72 | + | func (s *HookServer) Register(ctx context.Context, repo domain.RepoName, p domain.Principal) (string, func()) { |
| 69 | var raw [32]byte | 73 | var raw [32]byte | ||
| 70 | - | if _, err := rand.Read(raw[:]); err != nil { | 74 | + | _, _ = rand.Read(raw[:]) // never fails |
| 71 | - | // No entropy: hand out a token that matches nothing, so the hooks fail closed. | |||
| 72 | - | return "unavailable", func() {} | |||
| 73 | - | } | |||
| 74 | token := hex.EncodeToString(raw[:]) | 75 | token := hex.EncodeToString(raw[:]) | ||
| 75 | s.mu.Lock() | 76 | s.mu.Lock() | ||
| 76 | s.tokens[token] = hookEntry{repo: repo, principal: p} | 77 | s.tokens[token] = hookEntry{repo: repo, principal: p} | ||
| 77 | s.mu.Unlock() | 78 | s.mu.Unlock() | ||
| 79 | + | log := zerowrap.FromCtx(ctx) | |||
| 80 | + | log.Debug().Str("repo", repo.String()).Str("token_id", p.TokenID()).Msg("hook token registered") | |||
| 78 | return token, func() { | 81 | return token, func() { | ||
| 79 | s.mu.Lock() | 82 | s.mu.Lock() | ||
| 80 | delete(s.tokens, token) | 83 | delete(s.tokens, token) | ||
| @@ -91,7 +94,7 @@ func (s *HookServer) lookup(token string) (hookEntry, bool) { | |||||
| 91 | 94 | ||||
| 92 | // Listen creates the socket with mode 0600 (replacing a stale file) and | 95 | // Listen creates the socket with mode 0600 (replacing a stale file) and | ||
| 93 | // returns once it accepts connections. Serve then handles them. | 96 | // returns once it accepts connections. Serve then handles them. | ||
| 94 | - | func (s *HookServer) Listen() error { | 97 | + | func (s *HookServer) Listen(ctx context.Context) error { |
| 95 | // sun_path holds 108 bytes on Linux, 104 on the BSDs and macOS. | 98 | // sun_path holds 108 bytes on Linux, 104 on the BSDs and macOS. | ||
| 96 | if len(s.path) >= 100 { | 99 | if len(s.path) >= 100 { | ||
| 97 | return fmt.Errorf("hook socket path is too long for a Unix socket (%d bytes, limit about 100): %s; use a shorter %s", len(s.path), s.path, "ERMITE_DATA_DIR") | 100 | return fmt.Errorf("hook socket path is too long for a Unix socket (%d bytes, limit about 100): %s; use a shorter %s", len(s.path), s.path, "ERMITE_DATA_DIR") | ||
| @@ -110,6 +113,8 @@ func (s *HookServer) Listen() error { | |||||
| 110 | s.connMu.Lock() | 113 | s.connMu.Lock() | ||
| 111 | s.ln = ln | 114 | s.ln = ln | ||
| 112 | s.connMu.Unlock() | 115 | s.connMu.Unlock() | ||
| 116 | + | log := zerowrap.FromCtx(ctx) | |||
| 117 | + | log.Debug().Str("socket", s.path).Msg("hook socket created") | |||
| 113 | return nil | 118 | return nil | ||
| 114 | } | 119 | } | ||
| 115 | 120 | ||||
| @@ -164,7 +169,9 @@ func (s *HookServer) Serve(ctx context.Context) error { | |||||
| 164 | } | 169 | } | ||
| 165 | 170 | ||||
| 166 | // Close stops listening and drops the connections in progress. | 171 | // Close stops listening and drops the connections in progress. | ||
| 167 | - | func (s *HookServer) Close() { | 172 | + | func (s *HookServer) Close(ctx context.Context) { |
| 173 | + | log := zerowrap.FromCtx(ctx) | |||
| 174 | + | log.Debug().Str("socket", s.path).Msg("closing hook socket") | |||
| 168 | s.connMu.Lock() | 175 | s.connMu.Lock() | ||
| 169 | defer s.connMu.Unlock() | 176 | defer s.connMu.Unlock() | ||
| 170 | if s.ln != nil { | 177 | if s.ln != nil { | ||
| @@ -221,7 +228,11 @@ func (s *HookServer) handle(ctx context.Context, conn net.Conn) { | |||||
| 221 | s.reply(ctx, conn, HookReply{Message: "ermite: push refused: internal error"}) | 228 | s.reply(ctx, conn, HookReply{Message: "ermite: push refused: internal error"}) | ||
| 222 | } | 229 | } | ||
| 223 | case HookPostReceive: | 230 | case HookPostReceive: | ||
| 224 | - | if err := s.hooks.Record(ctx, entry.principal, entry.repo, updates); err != nil { | 231 | + | // The push is already in the repository: recording it must not be |
| 232 | + | // dropped because the server is shutting down. | |||
| 233 | + | rctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), recordTimeout) | |||
| 234 | + | defer cancel() | |||
| 235 | + | if err := s.hooks.Record(rctx, entry.principal, entry.repo, updates); err != nil { | |||
| 225 | rlog.Error().Err(err).Msg("post-receive recording failed") | 236 | rlog.Error().Err(err).Msg("post-receive recording failed") | ||
| 226 | s.reply(ctx, conn, HookReply{OK: true, Message: "ermite: the push succeeded but recording it failed"}) | 237 | s.reply(ctx, conn, HookReply{OK: true, Message: "ermite: the push succeeded but recording it failed"}) | ||
| 227 | return | 238 | return | ||
M internal/adapters/githttp/hooks_test.go +72 -12
| @@ -41,6 +41,8 @@ type fakeHooks struct { | |||
| 41 | 41 | calls []call | |
| 42 | 42 | checkErr error | |
| 43 | 43 | recErr error | |
| 44 | + | // onRecord, when set, runs inside Record with its context. | |
| 45 | + | onRecord func(ctx context.Context) | |
| 44 | 46 | } | |
| 45 | 47 | ||
| 46 | 48 | func (f *fakeHooks) Check(_ context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | |
| @@ -50,11 +52,15 @@ func (f *fakeHooks) Check(_ context.Context, p domain.Principal, n domain.RepoNa | |||
| 50 | 52 | return f.checkErr | |
| 51 | 53 | } | |
| 52 | 54 | ||
| 53 | - | func (f *fakeHooks) Record(_ context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | |
| 55 | + | func (f *fakeHooks) Record(ctx context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | |
| 54 | 56 | f.mu.Lock() | |
| 55 | - | defer f.mu.Unlock() | |
| 56 | 57 | f.calls = append(f.calls, call{"record", p, n, u}) | |
| 57 | - | return f.recErr | |
| 58 | + | err, on := f.recErr, f.onRecord | |
| 59 | + | f.mu.Unlock() | |
| 60 | + | if on != nil { | |
| 61 | + | on(ctx) | |
| 62 | + | } | |
| 63 | + | return err | |
| 58 | 64 | } | |
| 59 | 65 | ||
| 60 | 66 | func (f *fakeHooks) last() call { | |
| @@ -78,7 +84,7 @@ func startHooks(t *testing.T, h githttp.PushHooks) (*githttp.HookServer, string) | |||
| 78 | 84 | t.Cleanup(func() { _ = os.RemoveAll(dir) }) | |
| 79 | 85 | sock := filepath.Join(dir, "hook.sock") | |
| 80 | 86 | s := githttp.NewHookServer(sock, h) | |
| 81 | - | require.NoError(t, s.Listen()) | |
| 87 | + | require.NoError(t, s.Listen(context.Background())) | |
| 82 | 88 | ctx, cancel := context.WithCancel(context.Background()) | |
| 83 | 89 | done := make(chan error, 1) | |
| 84 | 90 | go func() { done <- s.Serve(ctx) }() | |
| @@ -125,13 +131,13 @@ func TestHookServerReplacesStaleSocketFile(t *testing.T) { | |||
| 125 | 131 | sock := filepath.Join(dir, "hook.sock") | |
| 126 | 132 | require.NoError(t, os.WriteFile(sock, []byte("stale"), 0o600)) | |
| 127 | 133 | s := githttp.NewHookServer(sock, &fakeHooks{}) | |
| 128 | - | require.NoError(t, s.Listen()) | |
| 129 | - | s.Close() | |
| 134 | + | require.NoError(t, s.Listen(context.Background())) | |
| 135 | + | s.Close(context.Background()) | |
| 130 | 136 | } | |
| 131 | 137 | ||
| 132 | 138 | func TestHookServerRejectsTooLongSocketPath(t *testing.T) { | |
| 133 | 139 | s := githttp.NewHookServer("/"+strings.Repeat("a", 120)+"/hook.sock", &fakeHooks{}) | |
| 134 | - | err := s.Listen() | |
| 140 | + | err := s.Listen(context.Background()) | |
| 135 | 141 | require.Error(t, err) | |
| 136 | 142 | assert.Contains(t, err.Error(), "too long") | |
| 137 | 143 | } | |
| @@ -140,8 +146,8 @@ func TestHookProtocol(t *testing.T) { | |||
| 140 | 146 | fh := &fakeHooks{} | |
| 141 | 147 | srv, sock := startHooks(t, fh) | |
| 142 | 148 | p := domain.TokenPrincipal(domain.Token{ID: "tok1", Scopes: []domain.Scope{domain.ScopeRepoWrite}}) | |
| 143 | - | tokenA, releaseA := srv.Register(repoName(t, "alpha"), p) | |
| 144 | - | tokenB, releaseB := srv.Register(repoName(t, "beta"), domain.Root()) | |
| 149 | + | tokenA, releaseA := srv.Register(context.Background(), repoName(t, "alpha"), p) | |
| 150 | + | tokenB, releaseB := srv.Register(context.Background(), repoName(t, "beta"), domain.Root()) | |
| 145 | 151 | assert.NotEqual(t, tokenA, tokenB) | |
| 146 | 152 | assert.Len(t, tokenA, 64) | |
| 147 | 153 | ||
| @@ -208,7 +214,7 @@ func TestHookProtocol(t *testing.T) { | |||
| 208 | 214 | }) | |
| 209 | 215 | ||
| 210 | 216 | t.Run("malformed requests are refused", func(t *testing.T) { | |
| 211 | - | tok, rel := srv.Register(repoName(t, "gamma"), domain.Root()) | |
| 217 | + | tok, rel := srv.Register(context.Background(), repoName(t, "gamma"), domain.Root()) | |
| 212 | 218 | defer rel() | |
| 213 | 219 | before := len(fh.calls) | |
| 214 | 220 | for _, line := range []string{ | |
| @@ -233,6 +239,60 @@ func TestHookProtocol(t *testing.T) { | |||
| 233 | 239 | }) | |
| 234 | 240 | } | |
| 235 | 241 | ||
| 242 | + | // A shutdown while a push is being recorded must not drop the recording: the | |
| 243 | + | // push is already in the repository. | |
| 244 | + | func TestHookRecordSurvivesShutdown(t *testing.T) { | |
| 245 | + | fh := &fakeHooks{} | |
| 246 | + | started, proceed := make(chan struct{}), make(chan struct{}) | |
| 247 | + | var cancelled, hasDeadline bool | |
| 248 | + | var ctxMu sync.Mutex | |
| 249 | + | fh.onRecord = func(ctx context.Context) { | |
| 250 | + | close(started) | |
| 251 | + | <-proceed | |
| 252 | + | ctxMu.Lock() | |
| 253 | + | defer ctxMu.Unlock() | |
| 254 | + | cancelled = ctx.Err() != nil | |
| 255 | + | _, hasDeadline = ctx.Deadline() | |
| 256 | + | } | |
| 257 | + | ||
| 258 | + | dir, err := os.MkdirTemp("", "hk") | |
| 259 | + | require.NoError(t, err) | |
| 260 | + | t.Cleanup(func() { _ = os.RemoveAll(dir) }) | |
| 261 | + | sock := filepath.Join(dir, "hook.sock") | |
| 262 | + | srv := githttp.NewHookServer(sock, fh) | |
| 263 | + | require.NoError(t, srv.Listen(context.Background())) | |
| 264 | + | ctx, cancel := context.WithCancel(context.Background()) | |
| 265 | + | done := make(chan error, 1) | |
| 266 | + | go func() { done <- srv.Serve(ctx) }() | |
| 267 | + | ||
| 268 | + | tok, rel := srv.Register(context.Background(), repoName(t, "alpha"), domain.Root()) | |
| 269 | + | defer rel() | |
| 270 | + | replies := make(chan githttp.HookReply, 1) | |
| 271 | + | go func() { | |
| 272 | + | replies <- raw(t, sock, `{"token":"`+tok+`","hook":"post-receive","updates":[]}`) | |
| 273 | + | }() | |
| 274 | + | <-started | |
| 275 | + | cancel() // the server is shutting down while Record runs | |
| 276 | + | close(proceed) | |
| 277 | + | ||
| 278 | + | select { | |
| 279 | + | case r := <-replies: | |
| 280 | + | assert.True(t, r.OK) | |
| 281 | + | case <-time.After(5 * time.Second): | |
| 282 | + | t.Fatal("no reply") | |
| 283 | + | } | |
| 284 | + | select { | |
| 285 | + | case err := <-done: | |
| 286 | + | assert.NoError(t, err) | |
| 287 | + | case <-time.After(5 * time.Second): | |
| 288 | + | t.Fatal("hook server did not stop") | |
| 289 | + | } | |
| 290 | + | ctxMu.Lock() | |
| 291 | + | defer ctxMu.Unlock() | |
| 292 | + | assert.False(t, cancelled, "Record is not cancelled by the shutdown") | |
| 293 | + | assert.True(t, hasDeadline, "but it is bounded") | |
| 294 | + | } | |
| 295 | + | ||
| 236 | 296 | func TestHookConcurrentTokens(t *testing.T) { | |
| 237 | 297 | fh := &fakeHooks{} | |
| 238 | 298 | srv, sock := startHooks(t, fh) | |
| @@ -242,7 +302,7 @@ func TestHookConcurrentTokens(t *testing.T) { | |||
| 242 | 302 | go func() { | |
| 243 | 303 | defer wg.Done() | |
| 244 | 304 | name := repoName(t, "r"+string(rune('a'+i))) | |
| 245 | - | tok, rel := srv.Register(name, domain.Root()) | |
| 305 | + | tok, rel := srv.Register(context.Background(), name, domain.Root()) | |
| 246 | 306 | defer rel() | |
| 247 | 307 | r := raw(t, sock, `{"token":"`+tok+`","hook":"post-receive","updates":[{"old":"`+domain.ZeroOID+`","new":"`+oidB+`","ref":"refs/heads/x"}]}`) | |
| 248 | 308 | assert.True(t, r.OK) | |
| @@ -260,7 +320,7 @@ func TestRunHook(t *testing.T) { | |||
| 260 | 320 | ctx := context.Background() | |
| 261 | 321 | fh := &fakeHooks{} | |
| 262 | 322 | srv, sock := startHooks(t, fh) | |
| 263 | - | tok, rel := srv.Register(repoName(t, "alpha"), domain.Root()) | |
| 323 | + | tok, rel := srv.Register(context.Background(), repoName(t, "alpha"), domain.Root()) | |
| 264 | 324 | defer rel() | |
| 265 | 325 | input := domain.ZeroOID + " " + oidA + " refs/heads/main\n" + oidA + " " + oidB + " refs/heads/dev\n" | |
| 266 | 326 | ||
| @@ -41,6 +41,8 @@ type fakeHooks struct { | |||||
| 41 | calls []call | 41 | calls []call | ||
| 42 | checkErr error | 42 | checkErr error | ||
| 43 | recErr error | 43 | recErr error | ||
| 44 | + | // onRecord, when set, runs inside Record with its context. | |||
| 45 | + | onRecord func(ctx context.Context) | |||
| 44 | } | 46 | } | ||
| 45 | 47 | ||||
| 46 | func (f *fakeHooks) Check(_ context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | 48 | func (f *fakeHooks) Check(_ context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | ||
| @@ -50,11 +52,15 @@ func (f *fakeHooks) Check(_ context.Context, p domain.Principal, n domain.RepoNa | |||||
| 50 | return f.checkErr | 52 | return f.checkErr | ||
| 51 | } | 53 | } | ||
| 52 | 54 | ||||
| 53 | - | func (f *fakeHooks) Record(_ context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { | 55 | + | func (f *fakeHooks) Record(ctx context.Context, p domain.Principal, n domain.RepoName, u []domain.RefUpdate) error { |
| 54 | f.mu.Lock() | 56 | f.mu.Lock() | ||
| 55 | - | defer f.mu.Unlock() | |||
| 56 | f.calls = append(f.calls, call{"record", p, n, u}) | 57 | f.calls = append(f.calls, call{"record", p, n, u}) | ||
| 57 | - | return f.recErr | 58 | + | err, on := f.recErr, f.onRecord |
| 59 | + | f.mu.Unlock() | |||
| 60 | + | if on != nil { | |||
| 61 | + | on(ctx) | |||
| 62 | + | } | |||
| 63 | + | return err | |||
| 58 | } | 64 | } | ||
| 59 | 65 | ||||
| 60 | func (f *fakeHooks) last() call { | 66 | func (f *fakeHooks) last() call { | ||
| @@ -78,7 +84,7 @@ func startHooks(t *testing.T, h githttp.PushHooks) (*githttp.HookServer, string) | |||||
| 78 | t.Cleanup(func() { _ = os.RemoveAll(dir) }) | 84 | t.Cleanup(func() { _ = os.RemoveAll(dir) }) | ||
| 79 | sock := filepath.Join(dir, "hook.sock") | 85 | sock := filepath.Join(dir, "hook.sock") | ||
| 80 | s := githttp.NewHookServer(sock, h) | 86 | s := githttp.NewHookServer(sock, h) | ||
| 81 | - | require.NoError(t, s.Listen()) | 87 | + | require.NoError(t, s.Listen(context.Background())) |
| 82 | ctx, cancel := context.WithCancel(context.Background()) | 88 | ctx, cancel := context.WithCancel(context.Background()) | ||
| 83 | done := make(chan error, 1) | 89 | done := make(chan error, 1) | ||
| 84 | go func() { done <- s.Serve(ctx) }() | 90 | go func() { done <- s.Serve(ctx) }() | ||
| @@ -125,13 +131,13 @@ func TestHookServerReplacesStaleSocketFile(t *testing.T) { | |||||
| 125 | sock := filepath.Join(dir, "hook.sock") | 131 | sock := filepath.Join(dir, "hook.sock") | ||
| 126 | require.NoError(t, os.WriteFile(sock, []byte("stale"), 0o600)) | 132 | require.NoError(t, os.WriteFile(sock, []byte("stale"), 0o600)) | ||
| 127 | s := githttp.NewHookServer(sock, &fakeHooks{}) | 133 | s := githttp.NewHookServer(sock, &fakeHooks{}) | ||
| 128 | - | require.NoError(t, s.Listen()) | 134 | + | require.NoError(t, s.Listen(context.Background())) |
| 129 | - | s.Close() | 135 | + | s.Close(context.Background()) |
| 130 | } | 136 | } | ||
| 131 | 137 | ||||
| 132 | func TestHookServerRejectsTooLongSocketPath(t *testing.T) { | 138 | func TestHookServerRejectsTooLongSocketPath(t *testing.T) { | ||
| 133 | s := githttp.NewHookServer("/"+strings.Repeat("a", 120)+"/hook.sock", &fakeHooks{}) | 139 | s := githttp.NewHookServer("/"+strings.Repeat("a", 120)+"/hook.sock", &fakeHooks{}) | ||
| 134 | - | err := s.Listen() | 140 | + | err := s.Listen(context.Background()) |
| 135 | require.Error(t, err) | 141 | require.Error(t, err) | ||
| 136 | assert.Contains(t, err.Error(), "too long") | 142 | assert.Contains(t, err.Error(), "too long") | ||
| 137 | } | 143 | } | ||
| @@ -140,8 +146,8 @@ func TestHookProtocol(t *testing.T) { | |||||
| 140 | fh := &fakeHooks{} | 146 | fh := &fakeHooks{} | ||
| 141 | srv, sock := startHooks(t, fh) | 147 | srv, sock := startHooks(t, fh) | ||
| 142 | p := domain.TokenPrincipal(domain.Token{ID: "tok1", Scopes: []domain.Scope{domain.ScopeRepoWrite}}) | 148 | p := domain.TokenPrincipal(domain.Token{ID: "tok1", Scopes: []domain.Scope{domain.ScopeRepoWrite}}) | ||
| 143 | - | tokenA, releaseA := srv.Register(repoName(t, "alpha"), p) | 149 | + | tokenA, releaseA := srv.Register(context.Background(), repoName(t, "alpha"), p) |
| 144 | - | tokenB, releaseB := srv.Register(repoName(t, "beta"), domain.Root()) | 150 | + | tokenB, releaseB := srv.Register(context.Background(), repoName(t, "beta"), domain.Root()) |
| 145 | assert.NotEqual(t, tokenA, tokenB) | 151 | assert.NotEqual(t, tokenA, tokenB) | ||
| 146 | assert.Len(t, tokenA, 64) | 152 | assert.Len(t, tokenA, 64) | ||
| 147 | 153 | ||||
| @@ -208,7 +214,7 @@ func TestHookProtocol(t *testing.T) { | |||||
| 208 | }) | 214 | }) | ||
| 209 | 215 | ||||
| 210 | t.Run("malformed requests are refused", func(t *testing.T) { | 216 | t.Run("malformed requests are refused", func(t *testing.T) { | ||
| 211 | - | tok, rel := srv.Register(repoName(t, "gamma"), domain.Root()) | 217 | + | tok, rel := srv.Register(context.Background(), repoName(t, "gamma"), domain.Root()) |
| 212 | defer rel() | 218 | defer rel() | ||
| 213 | before := len(fh.calls) | 219 | before := len(fh.calls) | ||
| 214 | for _, line := range []string{ | 220 | for _, line := range []string{ | ||
| @@ -233,6 +239,60 @@ func TestHookProtocol(t *testing.T) { | |||||
| 233 | }) | 239 | }) | ||
| 234 | } | 240 | } | ||
| 235 | 241 | ||||
| 242 | + | // A shutdown while a push is being recorded must not drop the recording: the | |||
| 243 | + | // push is already in the repository. | |||
| 244 | + | func TestHookRecordSurvivesShutdown(t *testing.T) { | |||
| 245 | + | fh := &fakeHooks{} | |||
| 246 | + | started, proceed := make(chan struct{}), make(chan struct{}) | |||
| 247 | + | var cancelled, hasDeadline bool | |||
| 248 | + | var ctxMu sync.Mutex | |||
| 249 | + | fh.onRecord = func(ctx context.Context) { | |||
| 250 | + | close(started) | |||
| 251 | + | <-proceed | |||
| 252 | + | ctxMu.Lock() | |||
| 253 | + | defer ctxMu.Unlock() | |||
| 254 | + | cancelled = ctx.Err() != nil | |||
| 255 | + | _, hasDeadline = ctx.Deadline() | |||
| 256 | + | } | |||
| 257 | + | ||||
| 258 | + | dir, err := os.MkdirTemp("", "hk") | |||
| 259 | + | require.NoError(t, err) | |||
| 260 | + | t.Cleanup(func() { _ = os.RemoveAll(dir) }) | |||
| 261 | + | sock := filepath.Join(dir, "hook.sock") | |||
| 262 | + | srv := githttp.NewHookServer(sock, fh) | |||
| 263 | + | require.NoError(t, srv.Listen(context.Background())) | |||
| 264 | + | ctx, cancel := context.WithCancel(context.Background()) | |||
| 265 | + | done := make(chan error, 1) | |||
| 266 | + | go func() { done <- srv.Serve(ctx) }() | |||
| 267 | + | ||||
| 268 | + | tok, rel := srv.Register(context.Background(), repoName(t, "alpha"), domain.Root()) | |||
| 269 | + | defer rel() | |||
| 270 | + | replies := make(chan githttp.HookReply, 1) | |||
| 271 | + | go func() { | |||
| 272 | + | replies <- raw(t, sock, `{"token":"`+tok+`","hook":"post-receive","updates":[]}`) | |||
| 273 | + | }() | |||
| 274 | + | <-started | |||
| 275 | + | cancel() // the server is shutting down while Record runs | |||
| 276 | + | close(proceed) | |||
| 277 | + | ||||
| 278 | + | select { | |||
| 279 | + | case r := <-replies: | |||
| 280 | + | assert.True(t, r.OK) | |||
| 281 | + | case <-time.After(5 * time.Second): | |||
| 282 | + | t.Fatal("no reply") | |||
| 283 | + | } | |||
| 284 | + | select { | |||
| 285 | + | case err := <-done: | |||
| 286 | + | assert.NoError(t, err) | |||
| 287 | + | case <-time.After(5 * time.Second): | |||
| 288 | + | t.Fatal("hook server did not stop") | |||
| 289 | + | } | |||
| 290 | + | ctxMu.Lock() | |||
| 291 | + | defer ctxMu.Unlock() | |||
| 292 | + | assert.False(t, cancelled, "Record is not cancelled by the shutdown") | |||
| 293 | + | assert.True(t, hasDeadline, "but it is bounded") | |||
| 294 | + | } | |||
| 295 | + | ||||
| 236 | func TestHookConcurrentTokens(t *testing.T) { | 296 | func TestHookConcurrentTokens(t *testing.T) { | ||
| 237 | fh := &fakeHooks{} | 297 | fh := &fakeHooks{} | ||
| 238 | srv, sock := startHooks(t, fh) | 298 | srv, sock := startHooks(t, fh) | ||
| @@ -242,7 +302,7 @@ func TestHookConcurrentTokens(t *testing.T) { | |||||
| 242 | go func() { | 302 | go func() { | ||
| 243 | defer wg.Done() | 303 | defer wg.Done() | ||
| 244 | name := repoName(t, "r"+string(rune('a'+i))) | 304 | name := repoName(t, "r"+string(rune('a'+i))) | ||
| 245 | - | tok, rel := srv.Register(name, domain.Root()) | 305 | + | tok, rel := srv.Register(context.Background(), name, domain.Root()) |
| 246 | defer rel() | 306 | defer rel() | ||
| 247 | r := raw(t, sock, `{"token":"`+tok+`","hook":"post-receive","updates":[{"old":"`+domain.ZeroOID+`","new":"`+oidB+`","ref":"refs/heads/x"}]}`) | 307 | r := raw(t, sock, `{"token":"`+tok+`","hook":"post-receive","updates":[{"old":"`+domain.ZeroOID+`","new":"`+oidB+`","ref":"refs/heads/x"}]}`) | ||
| 248 | assert.True(t, r.OK) | 308 | assert.True(t, r.OK) | ||
| @@ -260,7 +320,7 @@ func TestRunHook(t *testing.T) { | |||||
| 260 | ctx := context.Background() | 320 | ctx := context.Background() | ||
| 261 | fh := &fakeHooks{} | 321 | fh := &fakeHooks{} | ||
| 262 | srv, sock := startHooks(t, fh) | 322 | srv, sock := startHooks(t, fh) | ||
| 263 | - | tok, rel := srv.Register(repoName(t, "alpha"), domain.Root()) | 323 | + | tok, rel := srv.Register(context.Background(), repoName(t, "alpha"), domain.Root()) |
| 264 | defer rel() | 324 | defer rel() | ||
| 265 | input := domain.ZeroOID + " " + oidA + " refs/heads/main\n" + oidA + " " + oidB + " refs/heads/dev\n" | 325 | input := domain.ZeroOID + " " + oidA + " refs/heads/main\n" + oidA + " " + oidB + " refs/heads/dev\n" | ||
| 266 | 326 | ||||
M internal/app/maintenance.go +5 -2
| @@ -23,7 +23,8 @@ const MaintenanceDelay = 5 * time.Second | |||
| 23 | 23 | // Schedule during a run queues exactly one more run. | |
| 24 | 24 | // | |
| 25 | 25 | // Its goroutines use the logger of the context given to NewMaintainer and | |
| 26 | - | // live until Close. | |
| 26 | + | // live until Close. A run is cancelled by Close, and Git bounds it otherwise | |
| 27 | + | // (gitcli applies its command timeout to a context without deadline). | |
| 27 | 28 | type Maintainer struct { | |
| 28 | 29 | git ports.GitRepos | |
| 29 | 30 | delay time.Duration | |
| @@ -53,7 +54,7 @@ func NewMaintainer(ctx context.Context, git ports.GitRepos, delay time.Duration) | |||
| 53 | 54 | } | |
| 54 | 55 | ||
| 55 | 56 | // Schedule queues maintenance of the repository. | |
| 56 | - | func (m *Maintainer) Schedule(name domain.RepoName) { | |
| 57 | + | func (m *Maintainer) Schedule(ctx context.Context, name domain.RepoName) { | |
| 57 | 58 | m.mu.Lock() | |
| 58 | 59 | defer m.mu.Unlock() | |
| 59 | 60 | if m.closed { | |
| @@ -65,6 +66,8 @@ func (m *Maintainer) Schedule(name domain.RepoName) { | |||
| 65 | 66 | e = &maintEntry{} | |
| 66 | 67 | m.entries[name] = e | |
| 67 | 68 | m.arm(name, e) | |
| 69 | + | log := zerowrap.FromCtx(ctx) | |
| 70 | + | log.Debug().Str("repo", name.String()).Dur("delay", m.delay).Msg("repository maintenance scheduled") | |
| 68 | 71 | case e.running: | |
| 69 | 72 | e.again = true | |
| 70 | 73 | } | |
| @@ -23,7 +23,8 @@ const MaintenanceDelay = 5 * time.Second | |||||
| 23 | // Schedule during a run queues exactly one more run. | 23 | // Schedule during a run queues exactly one more run. | ||
| 24 | // | 24 | // | ||
| 25 | // Its goroutines use the logger of the context given to NewMaintainer and | 25 | // Its goroutines use the logger of the context given to NewMaintainer and | ||
| 26 | - | // live until Close. | 26 | + | // live until Close. A run is cancelled by Close, and Git bounds it otherwise |
| 27 | + | // (gitcli applies its command timeout to a context without deadline). | |||
| 27 | type Maintainer struct { | 28 | type Maintainer struct { | ||
| 28 | git ports.GitRepos | 29 | git ports.GitRepos | ||
| 29 | delay time.Duration | 30 | delay time.Duration | ||
| @@ -53,7 +54,7 @@ func NewMaintainer(ctx context.Context, git ports.GitRepos, delay time.Duration) | |||||
| 53 | } | 54 | } | ||
| 54 | 55 | ||||
| 55 | // Schedule queues maintenance of the repository. | 56 | // Schedule queues maintenance of the repository. | ||
| 56 | - | func (m *Maintainer) Schedule(name domain.RepoName) { | 57 | + | func (m *Maintainer) Schedule(ctx context.Context, name domain.RepoName) { |
| 57 | m.mu.Lock() | 58 | m.mu.Lock() | ||
| 58 | defer m.mu.Unlock() | 59 | defer m.mu.Unlock() | ||
| 59 | if m.closed { | 60 | if m.closed { | ||
| @@ -65,6 +66,8 @@ func (m *Maintainer) Schedule(name domain.RepoName) { | |||||
| 65 | e = &maintEntry{} | 66 | e = &maintEntry{} | ||
| 66 | m.entries[name] = e | 67 | m.entries[name] = e | ||
| 67 | m.arm(name, e) | 68 | m.arm(name, e) | ||
| 69 | + | log := zerowrap.FromCtx(ctx) | |||
| 70 | + | log.Debug().Str("repo", name.String()).Dur("delay", m.delay).Msg("repository maintenance scheduled") | |||
| 68 | case e.running: | 71 | case e.running: | ||
| 69 | e.again = true | 72 | e.again = true | ||
| 70 | } | 73 | } | ||
M internal/app/maintenance_test.go +13 -13
| @@ -41,7 +41,7 @@ func TestMaintainerDebounces(t *testing.T) { | |||
| 41 | 41 | defer m.Close(ctx) | |
| 42 | 42 | ||
| 43 | 43 | for range 10 { | |
| 44 | - | m.Schedule(n) | |
| 44 | + | m.Schedule(ctx, n) | |
| 45 | 45 | } | |
| 46 | 46 | time.Sleep(10 * time.Millisecond) | |
| 47 | 47 | assert.Equal(t, int32(0), runs.Load(), "waits for the delay") | |
| @@ -49,7 +49,7 @@ func TestMaintainerDebounces(t *testing.T) { | |||
| 49 | 49 | time.Sleep(150 * time.Millisecond) | |
| 50 | 50 | assert.Equal(t, int32(1), runs.Load(), "a burst makes one run") | |
| 51 | 51 | ||
| 52 | - | m.Schedule(n) | |
| 52 | + | m.Schedule(ctx, n) | |
| 53 | 53 | waitFor(t, func() bool { return runs.Load() == 2 }, "a second burst, a second run") | |
| 54 | 54 | } | |
| 55 | 55 | ||
| @@ -80,11 +80,11 @@ func TestMaintainerOneRunAtATimePerRepo(t *testing.T) { | |||
| 80 | 80 | m := app.NewMaintainer(ctx, git, 10*time.Millisecond) | |
| 81 | 81 | defer m.Close(ctx) | |
| 82 | 82 | ||
| 83 | - | m.Schedule(n) | |
| 83 | + | m.Schedule(ctx, n) | |
| 84 | 84 | waitFor(t, func() bool { return runs.Load() == 1 }, "first run to start") | |
| 85 | - | m.Schedule(n) // during the run: queues exactly one more | |
| 86 | - | m.Schedule(n) | |
| 87 | - | m.Schedule(other) | |
| 85 | + | m.Schedule(ctx, n) // during the run: queues exactly one more | |
| 86 | + | m.Schedule(ctx, n) | |
| 87 | + | m.Schedule(ctx, other) | |
| 88 | 88 | waitFor(t, otherRan.Load, "another repository is not blocked") | |
| 89 | 89 | time.Sleep(60 * time.Millisecond) | |
| 90 | 90 | assert.Equal(t, int32(1), runs.Load(), "the second run waits for the first") | |
| @@ -108,9 +108,9 @@ func TestMaintainerFailureIsLoggedNotFatal(t *testing.T) { | |||
| 108 | 108 | }) | |
| 109 | 109 | m := app.NewMaintainer(ctx, git, 5*time.Millisecond) | |
| 110 | 110 | defer m.Close(ctx) | |
| 111 | - | m.Schedule(n) | |
| 111 | + | m.Schedule(ctx, n) | |
| 112 | 112 | waitFor(t, func() bool { return runs.Load() == 1 }, "run") | |
| 113 | - | m.Schedule(n) | |
| 113 | + | m.Schedule(ctx, n) | |
| 114 | 114 | waitFor(t, func() bool { return runs.Load() == 2 }, "later runs still happen") | |
| 115 | 115 | } | |
| 116 | 116 | ||
| @@ -121,10 +121,10 @@ func TestMaintainerClose(t *testing.T) { | |||
| 121 | 121 | t.Run("pending runs are dropped", func(t *testing.T) { | |
| 122 | 122 | git := mocks.NewGitRepos(t) // no Maintain expected | |
| 123 | 123 | m := app.NewMaintainer(ctx, git, time.Hour) | |
| 124 | - | m.Schedule(n) | |
| 124 | + | m.Schedule(ctx, n) | |
| 125 | 125 | m.Close(ctx) | |
| 126 | - | m.Schedule(n) // after Close: ignored | |
| 127 | - | m.Close(ctx) // twice is fine | |
| 126 | + | m.Schedule(ctx, n) // after Close: ignored | |
| 127 | + | m.Close(ctx) // twice is fine | |
| 128 | 128 | }) | |
| 129 | 129 | ||
| 130 | 130 | t.Run("a run in progress is cancelled and awaited", func(t *testing.T) { | |
| @@ -138,7 +138,7 @@ func TestMaintainerClose(t *testing.T) { | |||
| 138 | 138 | return ctx.Err() | |
| 139 | 139 | }) | |
| 140 | 140 | m := app.NewMaintainer(ctx, git, time.Millisecond) | |
| 141 | - | m.Schedule(n) | |
| 141 | + | m.Schedule(ctx, n) | |
| 142 | 142 | <-started | |
| 143 | 143 | done := make(chan struct{}) | |
| 144 | 144 | go func() { m.Close(ctx); close(done) }() | |
| @@ -162,7 +162,7 @@ func TestMaintainerConcurrentSchedule(t *testing.T) { | |||
| 162 | 162 | go func() { | |
| 163 | 163 | defer wg.Done() | |
| 164 | 164 | for range 50 { | |
| 165 | - | m.Schedule(name(t, string(rune('a'+i%3)))) | |
| 165 | + | m.Schedule(ctx, name(t, string(rune('a'+i%3)))) | |
| 166 | 166 | time.Sleep(100 * time.Microsecond) | |
| 167 | 167 | } | |
| 168 | 168 | }() | |
| @@ -41,7 +41,7 @@ func TestMaintainerDebounces(t *testing.T) { | |||||
| 41 | defer m.Close(ctx) | 41 | defer m.Close(ctx) | ||
| 42 | 42 | ||||
| 43 | for range 10 { | 43 | for range 10 { | ||
| 44 | - | m.Schedule(n) | 44 | + | m.Schedule(ctx, n) |
| 45 | } | 45 | } | ||
| 46 | time.Sleep(10 * time.Millisecond) | 46 | time.Sleep(10 * time.Millisecond) | ||
| 47 | assert.Equal(t, int32(0), runs.Load(), "waits for the delay") | 47 | assert.Equal(t, int32(0), runs.Load(), "waits for the delay") | ||
| @@ -49,7 +49,7 @@ func TestMaintainerDebounces(t *testing.T) { | |||||
| 49 | time.Sleep(150 * time.Millisecond) | 49 | time.Sleep(150 * time.Millisecond) | ||
| 50 | assert.Equal(t, int32(1), runs.Load(), "a burst makes one run") | 50 | assert.Equal(t, int32(1), runs.Load(), "a burst makes one run") | ||
| 51 | 51 | ||||
| 52 | - | m.Schedule(n) | 52 | + | m.Schedule(ctx, n) |
| 53 | waitFor(t, func() bool { return runs.Load() == 2 }, "a second burst, a second run") | 53 | waitFor(t, func() bool { return runs.Load() == 2 }, "a second burst, a second run") | ||
| 54 | } | 54 | } | ||
| 55 | 55 | ||||
| @@ -80,11 +80,11 @@ func TestMaintainerOneRunAtATimePerRepo(t *testing.T) { | |||||
| 80 | m := app.NewMaintainer(ctx, git, 10*time.Millisecond) | 80 | m := app.NewMaintainer(ctx, git, 10*time.Millisecond) | ||
| 81 | defer m.Close(ctx) | 81 | defer m.Close(ctx) | ||
| 82 | 82 | ||||
| 83 | - | m.Schedule(n) | 83 | + | m.Schedule(ctx, n) |
| 84 | waitFor(t, func() bool { return runs.Load() == 1 }, "first run to start") | 84 | waitFor(t, func() bool { return runs.Load() == 1 }, "first run to start") | ||
| 85 | - | m.Schedule(n) // during the run: queues exactly one more | 85 | + | m.Schedule(ctx, n) // during the run: queues exactly one more |
| 86 | - | m.Schedule(n) | 86 | + | m.Schedule(ctx, n) |
| 87 | - | m.Schedule(other) | 87 | + | m.Schedule(ctx, other) |
| 88 | waitFor(t, otherRan.Load, "another repository is not blocked") | 88 | waitFor(t, otherRan.Load, "another repository is not blocked") | ||
| 89 | time.Sleep(60 * time.Millisecond) | 89 | time.Sleep(60 * time.Millisecond) | ||
| 90 | assert.Equal(t, int32(1), runs.Load(), "the second run waits for the first") | 90 | assert.Equal(t, int32(1), runs.Load(), "the second run waits for the first") | ||
| @@ -108,9 +108,9 @@ func TestMaintainerFailureIsLoggedNotFatal(t *testing.T) { | |||||
| 108 | }) | 108 | }) | ||
| 109 | m := app.NewMaintainer(ctx, git, 5*time.Millisecond) | 109 | m := app.NewMaintainer(ctx, git, 5*time.Millisecond) | ||
| 110 | defer m.Close(ctx) | 110 | defer m.Close(ctx) | ||
| 111 | - | m.Schedule(n) | 111 | + | m.Schedule(ctx, n) |
| 112 | waitFor(t, func() bool { return runs.Load() == 1 }, "run") | 112 | waitFor(t, func() bool { return runs.Load() == 1 }, "run") | ||
| 113 | - | m.Schedule(n) | 113 | + | m.Schedule(ctx, n) |
| 114 | waitFor(t, func() bool { return runs.Load() == 2 }, "later runs still happen") | 114 | waitFor(t, func() bool { return runs.Load() == 2 }, "later runs still happen") | ||
| 115 | } | 115 | } | ||
| 116 | 116 | ||||
| @@ -121,10 +121,10 @@ func TestMaintainerClose(t *testing.T) { | |||||
| 121 | t.Run("pending runs are dropped", func(t *testing.T) { | 121 | t.Run("pending runs are dropped", func(t *testing.T) { | ||
| 122 | git := mocks.NewGitRepos(t) // no Maintain expected | 122 | git := mocks.NewGitRepos(t) // no Maintain expected | ||
| 123 | m := app.NewMaintainer(ctx, git, time.Hour) | 123 | m := app.NewMaintainer(ctx, git, time.Hour) | ||
| 124 | - | m.Schedule(n) | 124 | + | m.Schedule(ctx, n) |
| 125 | m.Close(ctx) | 125 | m.Close(ctx) | ||
| 126 | - | m.Schedule(n) // after Close: ignored | 126 | + | m.Schedule(ctx, n) // after Close: ignored |
| 127 | - | m.Close(ctx) // twice is fine | 127 | + | m.Close(ctx) // twice is fine |
| 128 | }) | 128 | }) | ||
| 129 | 129 | ||||
| 130 | t.Run("a run in progress is cancelled and awaited", func(t *testing.T) { | 130 | t.Run("a run in progress is cancelled and awaited", func(t *testing.T) { | ||
| @@ -138,7 +138,7 @@ func TestMaintainerClose(t *testing.T) { | |||||
| 138 | return ctx.Err() | 138 | return ctx.Err() | ||
| 139 | }) | 139 | }) | ||
| 140 | m := app.NewMaintainer(ctx, git, time.Millisecond) | 140 | m := app.NewMaintainer(ctx, git, time.Millisecond) | ||
| 141 | - | m.Schedule(n) | 141 | + | m.Schedule(ctx, n) |
| 142 | <-started | 142 | <-started | ||
| 143 | done := make(chan struct{}) | 143 | done := make(chan struct{}) | ||
| 144 | go func() { m.Close(ctx); close(done) }() | 144 | go func() { m.Close(ctx); close(done) }() | ||
| @@ -162,7 +162,7 @@ func TestMaintainerConcurrentSchedule(t *testing.T) { | |||||
| 162 | go func() { | 162 | go func() { | ||
| 163 | defer wg.Done() | 163 | defer wg.Done() | ||
| 164 | for range 50 { | 164 | for range 50 { | ||
| 165 | - | m.Schedule(name(t, string(rune('a'+i%3)))) | 165 | + | m.Schedule(ctx, name(t, string(rune('a'+i%3)))) |
| 166 | time.Sleep(100 * time.Microsecond) | 166 | time.Sleep(100 * time.Microsecond) | ||
| 167 | } | 167 | } | ||
| 168 | }() | 168 | }() | ||
M internal/app/pushes.go +45 -22
| @@ -20,7 +20,13 @@ const pushRetries = 5 | |||
| 20 | 20 | ||
| 21 | 21 | // MaintenanceScheduler queues repository maintenance. *Maintainer fits. | |
| 22 | 22 | type MaintenanceScheduler interface { | |
| 23 | - | Schedule(name domain.RepoName) | |
| 23 | + | Schedule(ctx context.Context, name domain.RepoName) | |
| 24 | + | } | |
| 25 | + | ||
| 26 | + | // Revalidator confirms that a principal obtained earlier is still | |
| 27 | + | // authorized. *Auth fits. | |
| 28 | + | type Revalidator interface { | |
| 29 | + | Revalidate(ctx context.Context, p domain.Principal) error | |
| 24 | 30 | } | |
| 25 | 31 | ||
| 26 | 32 | // Pushes are the use cases around a Git push. | |
| @@ -31,12 +37,14 @@ type Pushes struct { | |||
| 31 | 37 | bus ports.EventBus | |
| 32 | 38 | clock ports.Clock | |
| 33 | 39 | maint MaintenanceScheduler | |
| 40 | + | auth Revalidator | |
| 34 | 41 | } | |
| 35 | 42 | ||
| 36 | - | // NewPushes builds the push use cases. maint may be nil, which disables | |
| 37 | - | // maintenance after a push. | |
| 38 | - | func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus ports.EventBus, clock ports.Clock, maint MaintenanceScheduler) *Pushes { | |
| 39 | - | return &Pushes{meta: meta, repos: repos, git: git, bus: bus, clock: clock, maint: maint} | |
| 43 | + | // NewPushes builds the push use cases. Every argument is required: auth | |
| 44 | + | // re-validates the token of a push when its pre-receive hook runs, maint | |
| 45 | + | // queues the maintenance that follows a push. | |
| 46 | + | func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus ports.EventBus, clock ports.Clock, maint MaintenanceScheduler, auth Revalidator) *Pushes { | |
| 47 | + | return &Pushes{meta: meta, repos: repos, git: git, bus: bus, clock: clock, maint: maint, auth: auth} | |
| 40 | 48 | } | |
| 41 | 49 | ||
| 42 | 50 | // Open decides whether the principal may push to the repository and, with | |
| @@ -47,51 +55,61 @@ func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus p | |||
| 47 | 55 | // cannot read and cannot write, or a missing one it may not create, answers | |
| 48 | 56 | // domain.ErrNotFound, so that a private repository never shows itself. One it | |
| 49 | 57 | // can read but not write answers domain.ErrForbidden. | |
| 50 | - | func (u *Pushes) Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) (domain.Repo, bool, error) { | |
| 58 | + | func (u *Pushes) Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) error { | |
| 51 | 59 | if p.IsAnonymous() { | |
| 52 | - | return domain.Repo{}, false, domain.ErrUnauthenticated | |
| 60 | + | return domain.ErrUnauthenticated | |
| 53 | 61 | } | |
| 54 | 62 | repo, err := u.meta.Get(ctx, name) | |
| 55 | 63 | switch { | |
| 56 | 64 | case err == nil: | |
| 57 | - | return u.openExisting(p, repo) | |
| 65 | + | return openExisting(p, repo) | |
| 58 | 66 | case !errors.Is(err, domain.ErrNotFound): | |
| 59 | - | return domain.Repo{}, false, fmt.Errorf("get repository %q: %w", name, err) | |
| 67 | + | return fmt.Errorf("get repository %q: %w", name, err) | |
| 60 | 68 | } | |
| 61 | 69 | ||
| 62 | 70 | if !create || !canWrite(p, name) { | |
| 63 | - | return domain.Repo{}, false, domain.ErrNotFound | |
| 71 | + | return domain.ErrNotFound | |
| 64 | 72 | } | |
| 65 | 73 | repo, err = u.repos.Create(ctx, p, CreateParams{Name: name, Visibility: domain.VisibilityPrivate}) | |
| 66 | 74 | switch { | |
| 67 | 75 | case err == nil: | |
| 68 | 76 | u.bus.Publish(ctx, domain.RepoCreated{Repo: repo.Name, Visibility: repo.Visibility, At: repo.CreatedAt}) | |
| 69 | - | return repo, true, nil | |
| 77 | + | return nil | |
| 70 | 78 | case errors.Is(err, domain.ErrAlreadyExists): | |
| 71 | 79 | // A concurrent first push won the race: this one pushes to its repository. | |
| 72 | 80 | repo, gerr := u.meta.Get(ctx, name) | |
| 73 | 81 | if gerr != nil { | |
| 74 | - | return domain.Repo{}, false, fmt.Errorf("get repository %q: %w", name, gerr) | |
| 82 | + | return fmt.Errorf("get repository %q: %w", name, gerr) | |
| 75 | 83 | } | |
| 76 | - | return u.openExisting(p, repo) | |
| 84 | + | return openExisting(p, repo) | |
| 77 | 85 | } | |
| 78 | - | return domain.Repo{}, false, err | |
| 86 | + | return err | |
| 79 | 87 | } | |
| 80 | 88 | ||
| 81 | - | func (u *Pushes) openExisting(p domain.Principal, repo domain.Repo) (domain.Repo, bool, error) { | |
| 89 | + | func openExisting(p domain.Principal, repo domain.Repo) error { | |
| 82 | 90 | switch { | |
| 83 | 91 | case canWrite(p, repo.Name): | |
| 84 | - | return repo, false, nil | |
| 92 | + | return nil | |
| 85 | 93 | case canRead(p, repo): | |
| 86 | - | return domain.Repo{}, false, domain.ErrForbidden | |
| 94 | + | return domain.ErrForbidden | |
| 87 | 95 | } | |
| 88 | - | return domain.Repo{}, false, domain.ErrNotFound | |
| 96 | + | return domain.ErrNotFound | |
| 89 | 97 | } | |
| 90 | 98 | ||
| 91 | 99 | // Check is the pre-receive decision. v1 accepts every push of a principal that | |
| 92 | 100 | // may write the repository; protected branches and limits will refuse here by | |
| 93 | 101 | // returning an error that matches domain.ErrRejected. | |
| 102 | + | // | |
| 103 | + | // The token of the push was valid when the request began, but a push can run | |
| 104 | + | // for long: it is re-validated here, so a token revoked or expired meanwhile | |
| 105 | + | // stops the push before Git applies it. A failure to tell refuses it too. | |
| 94 | 106 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate) error { | |
| 107 | + | if err := u.auth.Revalidate(ctx, p); err != nil { | |
| 108 | + | if errors.Is(err, domain.ErrUnauthenticated) { | |
| 109 | + | return fmt.Errorf("%w: the token is no longer valid", domain.ErrRejected) | |
| 110 | + | } | |
| 111 | + | return fmt.Errorf("revalidate token: %w", err) | |
| 112 | + | } | |
| 95 | 113 | if !canWrite(p, name) { | |
| 96 | 114 | return fmt.Errorf("%w: no write access to %s", domain.ErrRejected, name) | |
| 97 | 115 | } | |
| @@ -116,9 +134,7 @@ func (u *Pushes) Record(ctx context.Context, p domain.Principal, name domain.Rep | |||
| 116 | 134 | errs = append(errs, err) | |
| 117 | 135 | } | |
| 118 | 136 | u.bus.Publish(ctx, domain.PushEvent{Repo: name, Updates: append([]domain.RefUpdate(nil), updates...), At: now}) | |
| 119 | - | if u.maint != nil { | |
| 120 | - | u.maint.Schedule(name) | |
| 121 | - | } | |
| 137 | + | u.maint.Schedule(ctx, name) | |
| 122 | 138 | log.Info(). | |
| 123 | 139 | Str("repo", name.String()).Int("refs", len(updates)). | |
| 124 | 140 | Str("principal", p.Label()).Str("token_id", p.TokenID()). | |
| @@ -160,6 +176,9 @@ func (u *Pushes) touchPushedAt(ctx context.Context, name domain.RepoName, now ti | |||
| 160 | 176 | // missing one: main when it was pushed, else the first pushed branch. | |
| 161 | 177 | func (u *Pushes) fixDefaultBranch(ctx context.Context, name domain.RepoName, updates []domain.RefUpdate) error { | |
| 162 | 178 | branch, exists, err := u.git.DefaultBranch(ctx, name) | |
| 179 | + | if errors.Is(err, domain.ErrNotFound) { | |
| 180 | + | return nil // deleted meanwhile | |
| 181 | + | } | |
| 163 | 182 | if err != nil { | |
| 164 | 183 | return fmt.Errorf("read default branch of %q: %w", name, err) | |
| 165 | 184 | } | |
| @@ -183,7 +202,11 @@ func (u *Pushes) fixDefaultBranch(ctx context.Context, name domain.RepoName, upd | |||
| 183 | 202 | if target == "" || target == branch { | |
| 184 | 203 | return nil | |
| 185 | 204 | } | |
| 186 | - | if err := u.git.SetDefaultBranch(ctx, name, target); err != nil { | |
| 205 | + | err = u.git.SetDefaultBranch(ctx, name, target) | |
| 206 | + | if errors.Is(err, domain.ErrNotFound) { | |
| 207 | + | return nil // deleted meanwhile | |
| 208 | + | } | |
| 209 | + | if err != nil { | |
| 187 | 210 | return fmt.Errorf("set default branch of %q to %q: %w", name, target, err) | |
| 188 | 211 | } | |
| 189 | 212 | log := zerowrap.FromCtx(ctx) | |
| @@ -20,7 +20,13 @@ const pushRetries = 5 | |||||
| 20 | 20 | ||||
| 21 | // MaintenanceScheduler queues repository maintenance. *Maintainer fits. | 21 | // MaintenanceScheduler queues repository maintenance. *Maintainer fits. | ||
| 22 | type MaintenanceScheduler interface { | 22 | type MaintenanceScheduler interface { | ||
| 23 | - | Schedule(name domain.RepoName) | 23 | + | Schedule(ctx context.Context, name domain.RepoName) |
| 24 | + | } | |||
| 25 | + | ||||
| 26 | + | // Revalidator confirms that a principal obtained earlier is still | |||
| 27 | + | // authorized. *Auth fits. | |||
| 28 | + | type Revalidator interface { | |||
| 29 | + | Revalidate(ctx context.Context, p domain.Principal) error | |||
| 24 | } | 30 | } | ||
| 25 | 31 | ||||
| 26 | // Pushes are the use cases around a Git push. | 32 | // Pushes are the use cases around a Git push. | ||
| @@ -31,12 +37,14 @@ type Pushes struct { | |||||
| 31 | bus ports.EventBus | 37 | bus ports.EventBus | ||
| 32 | clock ports.Clock | 38 | clock ports.Clock | ||
| 33 | maint MaintenanceScheduler | 39 | maint MaintenanceScheduler | ||
| 40 | + | auth Revalidator | |||
| 34 | } | 41 | } | ||
| 35 | 42 | ||||
| 36 | - | // NewPushes builds the push use cases. maint may be nil, which disables | 43 | + | // NewPushes builds the push use cases. Every argument is required: auth |
| 37 | - | // maintenance after a push. | 44 | + | // re-validates the token of a push when its pre-receive hook runs, maint |
| 38 | - | func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus ports.EventBus, clock ports.Clock, maint MaintenanceScheduler) *Pushes { | 45 | + | // queues the maintenance that follows a push. |
| 39 | - | return &Pushes{meta: meta, repos: repos, git: git, bus: bus, clock: clock, maint: maint} | 46 | + | func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus ports.EventBus, clock ports.Clock, maint MaintenanceScheduler, auth Revalidator) *Pushes { |
| 47 | + | return &Pushes{meta: meta, repos: repos, git: git, bus: bus, clock: clock, maint: maint, auth: auth} | |||
| 40 | } | 48 | } | ||
| 41 | 49 | ||||
| 42 | // Open decides whether the principal may push to the repository and, with | 50 | // Open decides whether the principal may push to the repository and, with | ||
| @@ -47,51 +55,61 @@ func NewPushes(meta ports.RepoMetaStore, repos *Repos, git ports.GitRepos, bus p | |||||
| 47 | // cannot read and cannot write, or a missing one it may not create, answers | 55 | // cannot read and cannot write, or a missing one it may not create, answers | ||
| 48 | // domain.ErrNotFound, so that a private repository never shows itself. One it | 56 | // domain.ErrNotFound, so that a private repository never shows itself. One it | ||
| 49 | // can read but not write answers domain.ErrForbidden. | 57 | // can read but not write answers domain.ErrForbidden. | ||
| 50 | - | func (u *Pushes) Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) (domain.Repo, bool, error) { | 58 | + | func (u *Pushes) Open(ctx context.Context, p domain.Principal, name domain.RepoName, create bool) error { |
| 51 | if p.IsAnonymous() { | 59 | if p.IsAnonymous() { | ||
| 52 | - | return domain.Repo{}, false, domain.ErrUnauthenticated | 60 | + | return domain.ErrUnauthenticated |
| 53 | } | 61 | } | ||
| 54 | repo, err := u.meta.Get(ctx, name) | 62 | repo, err := u.meta.Get(ctx, name) | ||
| 55 | switch { | 63 | switch { | ||
| 56 | case err == nil: | 64 | case err == nil: | ||
| 57 | - | return u.openExisting(p, repo) | 65 | + | return openExisting(p, repo) |
| 58 | case !errors.Is(err, domain.ErrNotFound): | 66 | case !errors.Is(err, domain.ErrNotFound): | ||
| 59 | - | return domain.Repo{}, false, fmt.Errorf("get repository %q: %w", name, err) | 67 | + | return fmt.Errorf("get repository %q: %w", name, err) |
| 60 | } | 68 | } | ||
| 61 | 69 | ||||
| 62 | if !create || !canWrite(p, name) { | 70 | if !create || !canWrite(p, name) { | ||
| 63 | - | return domain.Repo{}, false, domain.ErrNotFound | 71 | + | return domain.ErrNotFound |
| 64 | } | 72 | } | ||
| 65 | repo, err = u.repos.Create(ctx, p, CreateParams{Name: name, Visibility: domain.VisibilityPrivate}) | 73 | repo, err = u.repos.Create(ctx, p, CreateParams{Name: name, Visibility: domain.VisibilityPrivate}) | ||
| 66 | switch { | 74 | switch { | ||
| 67 | case err == nil: | 75 | case err == nil: | ||
| 68 | u.bus.Publish(ctx, domain.RepoCreated{Repo: repo.Name, Visibility: repo.Visibility, At: repo.CreatedAt}) | 76 | u.bus.Publish(ctx, domain.RepoCreated{Repo: repo.Name, Visibility: repo.Visibility, At: repo.CreatedAt}) | ||
| 69 | - | return repo, true, nil | 77 | + | return nil |
| 70 | case errors.Is(err, domain.ErrAlreadyExists): | 78 | case errors.Is(err, domain.ErrAlreadyExists): | ||
| 71 | // A concurrent first push won the race: this one pushes to its repository. | 79 | // A concurrent first push won the race: this one pushes to its repository. | ||
| 72 | repo, gerr := u.meta.Get(ctx, name) | 80 | repo, gerr := u.meta.Get(ctx, name) | ||
| 73 | if gerr != nil { | 81 | if gerr != nil { | ||
| 74 | - | return domain.Repo{}, false, fmt.Errorf("get repository %q: %w", name, gerr) | 82 | + | return fmt.Errorf("get repository %q: %w", name, gerr) |
| 75 | } | 83 | } | ||
| 76 | - | return u.openExisting(p, repo) | 84 | + | return openExisting(p, repo) |
| 77 | } | 85 | } | ||
| 78 | - | return domain.Repo{}, false, err | 86 | + | return err |
| 79 | } | 87 | } | ||
| 80 | 88 | ||||
| 81 | - | func (u *Pushes) openExisting(p domain.Principal, repo domain.Repo) (domain.Repo, bool, error) { | 89 | + | func openExisting(p domain.Principal, repo domain.Repo) error { |
| 82 | switch { | 90 | switch { | ||
| 83 | case canWrite(p, repo.Name): | 91 | case canWrite(p, repo.Name): | ||
| 84 | - | return repo, false, nil | 92 | + | return nil |
| 85 | case canRead(p, repo): | 93 | case canRead(p, repo): | ||
| 86 | - | return domain.Repo{}, false, domain.ErrForbidden | 94 | + | return domain.ErrForbidden |
| 87 | } | 95 | } | ||
| 88 | - | return domain.Repo{}, false, domain.ErrNotFound | 96 | + | return domain.ErrNotFound |
| 89 | } | 97 | } | ||
| 90 | 98 | ||||
| 91 | // Check is the pre-receive decision. v1 accepts every push of a principal that | 99 | // Check is the pre-receive decision. v1 accepts every push of a principal that | ||
| 92 | // may write the repository; protected branches and limits will refuse here by | 100 | // may write the repository; protected branches and limits will refuse here by | ||
| 93 | // returning an error that matches domain.ErrRejected. | 101 | // returning an error that matches domain.ErrRejected. | ||
| 102 | + | // | |||
| 103 | + | // The token of the push was valid when the request began, but a push can run | |||
| 104 | + | // for long: it is re-validated here, so a token revoked or expired meanwhile | |||
| 105 | + | // stops the push before Git applies it. A failure to tell refuses it too. | |||
| 94 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate) error { | 106 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate) error { | ||
| 107 | + | if err := u.auth.Revalidate(ctx, p); err != nil { | |||
| 108 | + | if errors.Is(err, domain.ErrUnauthenticated) { | |||
| 109 | + | return fmt.Errorf("%w: the token is no longer valid", domain.ErrRejected) | |||
| 110 | + | } | |||
| 111 | + | return fmt.Errorf("revalidate token: %w", err) | |||
| 112 | + | } | |||
| 95 | if !canWrite(p, name) { | 113 | if !canWrite(p, name) { | ||
| 96 | return fmt.Errorf("%w: no write access to %s", domain.ErrRejected, name) | 114 | return fmt.Errorf("%w: no write access to %s", domain.ErrRejected, name) | ||
| 97 | } | 115 | } | ||
| @@ -116,9 +134,7 @@ func (u *Pushes) Record(ctx context.Context, p domain.Principal, name domain.Rep | |||||
| 116 | errs = append(errs, err) | 134 | errs = append(errs, err) | ||
| 117 | } | 135 | } | ||
| 118 | u.bus.Publish(ctx, domain.PushEvent{Repo: name, Updates: append([]domain.RefUpdate(nil), updates...), At: now}) | 136 | u.bus.Publish(ctx, domain.PushEvent{Repo: name, Updates: append([]domain.RefUpdate(nil), updates...), At: now}) | ||
| 119 | - | if u.maint != nil { | 137 | + | u.maint.Schedule(ctx, name) |
| 120 | - | u.maint.Schedule(name) | |||
| 121 | - | } | |||
| 122 | log.Info(). | 138 | log.Info(). | ||
| 123 | Str("repo", name.String()).Int("refs", len(updates)). | 139 | Str("repo", name.String()).Int("refs", len(updates)). | ||
| 124 | Str("principal", p.Label()).Str("token_id", p.TokenID()). | 140 | Str("principal", p.Label()).Str("token_id", p.TokenID()). | ||
| @@ -160,6 +176,9 @@ func (u *Pushes) touchPushedAt(ctx context.Context, name domain.RepoName, now ti | |||||
| 160 | // missing one: main when it was pushed, else the first pushed branch. | 176 | // missing one: main when it was pushed, else the first pushed branch. | ||
| 161 | func (u *Pushes) fixDefaultBranch(ctx context.Context, name domain.RepoName, updates []domain.RefUpdate) error { | 177 | func (u *Pushes) fixDefaultBranch(ctx context.Context, name domain.RepoName, updates []domain.RefUpdate) error { | ||
| 162 | branch, exists, err := u.git.DefaultBranch(ctx, name) | 178 | branch, exists, err := u.git.DefaultBranch(ctx, name) | ||
| 179 | + | if errors.Is(err, domain.ErrNotFound) { | |||
| 180 | + | return nil // deleted meanwhile | |||
| 181 | + | } | |||
| 163 | if err != nil { | 182 | if err != nil { | ||
| 164 | return fmt.Errorf("read default branch of %q: %w", name, err) | 183 | return fmt.Errorf("read default branch of %q: %w", name, err) | ||
| 165 | } | 184 | } | ||
| @@ -183,7 +202,11 @@ func (u *Pushes) fixDefaultBranch(ctx context.Context, name domain.RepoName, upd | |||||
| 183 | if target == "" || target == branch { | 202 | if target == "" || target == branch { | ||
| 184 | return nil | 203 | return nil | ||
| 185 | } | 204 | } | ||
| 186 | - | if err := u.git.SetDefaultBranch(ctx, name, target); err != nil { | 205 | + | err = u.git.SetDefaultBranch(ctx, name, target) |
| 206 | + | if errors.Is(err, domain.ErrNotFound) { | |||
| 207 | + | return nil // deleted meanwhile | |||
| 208 | + | } | |||
| 209 | + | if err != nil { | |||
| 187 | return fmt.Errorf("set default branch of %q to %q: %w", name, target, err) | 210 | return fmt.Errorf("set default branch of %q to %q: %w", name, target, err) | ||
| 188 | } | 211 | } | ||
| 189 | log := zerowrap.FromCtx(ctx) | 212 | log := zerowrap.FromCtx(ctx) | ||
M internal/app/pushes_test.go +95 -55
| @@ -24,7 +24,7 @@ type scheduler struct { | |||
| 24 | 24 | names []domain.RepoName | |
| 25 | 25 | } | |
| 26 | 26 | ||
| 27 | - | func (s *scheduler) Schedule(n domain.RepoName) { | |
| 27 | + | func (s *scheduler) Schedule(_ context.Context, n domain.RepoName) { | |
| 28 | 28 | s.mu.Lock() | |
| 29 | 29 | defer s.mu.Unlock() | |
| 30 | 30 | s.names = append(s.names, n) | |
| @@ -35,6 +35,7 @@ type pushFixture struct { | |||
| 35 | 35 | git *mocks.GitRepos | |
| 36 | 36 | bus *mocks.EventBus | |
| 37 | 37 | clock *mocks.Clock | |
| 38 | + | tokens *mocks.TokenStore | |
| 38 | 39 | sched *scheduler | |
| 39 | 40 | pushes *app.Pushes | |
| 40 | 41 | } | |
| @@ -42,15 +43,17 @@ type pushFixture struct { | |||
| 42 | 43 | func newPushFixture(t *testing.T) *pushFixture { | |
| 43 | 44 | t.Helper() | |
| 44 | 45 | f := &pushFixture{ | |
| 45 | - | meta: mocks.NewRepoMetaStore(t), | |
| 46 | - | git: mocks.NewGitRepos(t), | |
| 47 | - | bus: mocks.NewEventBus(t), | |
| 48 | - | clock: mocks.NewClock(t), | |
| 49 | - | sched: &scheduler{}, | |
| 46 | + | meta: mocks.NewRepoMetaStore(t), | |
| 47 | + | git: mocks.NewGitRepos(t), | |
| 48 | + | bus: mocks.NewEventBus(t), | |
| 49 | + | clock: mocks.NewClock(t), | |
| 50 | + | tokens: mocks.NewTokenStore(t), | |
| 51 | + | sched: &scheduler{}, | |
| 50 | 52 | } | |
| 51 | 53 | f.clock.EXPECT().Now().Return(now).Maybe() | |
| 52 | 54 | repos := app.NewRepos(f.meta, f.git, f.clock) | |
| 53 | - | f.pushes = app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, f.sched) | |
| 55 | + | auth := app.NewAuth(domain.HashTokenSecret(rootSecret), f.tokens, f.clock) | |
| 56 | + | f.pushes = app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, f.sched, auth) | |
| 54 | 57 | return f | |
| 55 | 58 | } | |
| 56 | 59 | ||
| @@ -66,25 +69,24 @@ func TestPushesOpen(t *testing.T) { | |||
| 66 | 69 | pub := domain.Repo{Name: n, Visibility: domain.VisibilityPublic} | |
| 67 | 70 | ||
| 68 | 71 | tests := []struct { | |
| 69 | - | name string | |
| 70 | - | p domain.Principal | |
| 71 | - | repo *domain.Repo // nil: missing | |
| 72 | - | create bool | |
| 73 | - | want error | |
| 74 | - | created bool | |
| 72 | + | name string | |
| 73 | + | p domain.Principal | |
| 74 | + | repo *domain.Repo // nil: missing | |
| 75 | + | create bool | |
| 76 | + | want error | |
| 75 | 77 | }{ | |
| 76 | - | {"anonymous", domain.Anonymous(), &pub, false, domain.ErrUnauthenticated, false}, | |
| 77 | - | {"root, existing", domain.Root(), &priv, false, nil, false}, | |
| 78 | - | {"writer, existing", tokenPrincipal(domain.ScopeRepoWrite), &priv, false, nil, false}, | |
| 79 | - | {"writer limited to the repo", tokenPrincipal(domain.ScopeRepoWrite, "tools"), &priv, false, nil, false}, | |
| 80 | - | {"writer limited to another repo, private", tokenPrincipal(domain.ScopeRepoWrite, "other"), &priv, false, domain.ErrNotFound, false}, | |
| 81 | - | {"writer limited to another repo, public", tokenPrincipal(domain.ScopeRepoWrite, "other"), &pub, false, domain.ErrForbidden, false}, | |
| 82 | - | {"reader of a private repo", tokenPrincipal(domain.ScopeRepoRead), &priv, false, domain.ErrForbidden, false}, | |
| 83 | - | {"reader of a public repo", tokenPrincipal(domain.ScopeRepoRead), &pub, false, domain.ErrForbidden, false}, | |
| 84 | - | {"token with no relevant scope, private", tokenPrincipal(domain.ScopeMRWrite), &priv, false, domain.ErrNotFound, false}, | |
| 85 | - | {"missing, no create", domain.Root(), nil, false, domain.ErrNotFound, false}, | |
| 86 | - | {"missing, reader may not create", tokenPrincipal(domain.ScopeRepoRead), nil, true, domain.ErrNotFound, false}, | |
| 87 | - | {"missing, writer limited elsewhere", tokenPrincipal(domain.ScopeRepoWrite, "other"), nil, true, domain.ErrNotFound, false}, | |
| 78 | + | {"anonymous", domain.Anonymous(), &pub, false, domain.ErrUnauthenticated}, | |
| 79 | + | {"root, existing", domain.Root(), &priv, false, nil}, | |
| 80 | + | {"writer, existing", tokenPrincipal(domain.ScopeRepoWrite), &priv, false, nil}, | |
| 81 | + | {"writer limited to the repo", tokenPrincipal(domain.ScopeRepoWrite, "tools"), &priv, false, nil}, | |
| 82 | + | {"writer limited to another repo, private", tokenPrincipal(domain.ScopeRepoWrite, "other"), &priv, false, domain.ErrNotFound}, | |
| 83 | + | {"writer limited to another repo, public", tokenPrincipal(domain.ScopeRepoWrite, "other"), &pub, false, domain.ErrForbidden}, | |
| 84 | + | {"reader of a private repo", tokenPrincipal(domain.ScopeRepoRead), &priv, false, domain.ErrForbidden}, | |
| 85 | + | {"reader of a public repo", tokenPrincipal(domain.ScopeRepoRead), &pub, false, domain.ErrForbidden}, | |
| 86 | + | {"token with no relevant scope, private", tokenPrincipal(domain.ScopeMRWrite), &priv, false, domain.ErrNotFound}, | |
| 87 | + | {"missing, no create", domain.Root(), nil, false, domain.ErrNotFound}, | |
| 88 | + | {"missing, reader may not create", tokenPrincipal(domain.ScopeRepoRead), nil, true, domain.ErrNotFound}, | |
| 89 | + | {"missing, writer limited elsewhere", tokenPrincipal(domain.ScopeRepoWrite, "other"), nil, true, domain.ErrNotFound}, | |
| 88 | 90 | } | |
| 89 | 91 | for _, tt := range tests { | |
| 90 | 92 | t.Run(tt.name, func(t *testing.T) { | |
| @@ -96,15 +98,13 @@ func TestPushesOpen(t *testing.T) { | |||
| 96 | 98 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |
| 97 | 99 | } | |
| 98 | 100 | } | |
| 99 | - | repo, created, err := f.pushes.Open(ctx, tt.p, n, tt.create) | |
| 101 | + | // The bus mock expects no Publish: nothing is created here. | |
| 102 | + | err := f.pushes.Open(ctx, tt.p, n, tt.create) | |
| 100 | 103 | if tt.want != nil { | |
| 101 | 104 | assert.ErrorIs(t, err, tt.want) | |
| 102 | - | assert.False(t, created) | |
| 103 | 105 | return | |
| 104 | 106 | } | |
| 105 | 107 | require.NoError(t, err) | |
| 106 | - | assert.Equal(t, tt.created, created) | |
| 107 | - | assert.Equal(t, n, repo.Name) | |
| 108 | 108 | }) | |
| 109 | 109 | } | |
| 110 | 110 | ||
| @@ -117,10 +117,7 @@ func TestPushesOpen(t *testing.T) { | |||
| 117 | 117 | f.git.EXPECT().Publish(ctx, ports.StagedID("s1"), n).Return(nil) | |
| 118 | 118 | f.bus.EXPECT().Publish(ctx, domain.RepoCreated{Repo: n, Visibility: domain.VisibilityPrivate, At: now}).Return() | |
| 119 | 119 | ||
| 120 | - | repo, created, err := f.pushes.Open(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, true) | |
| 121 | - | require.NoError(t, err) | |
| 122 | - | assert.True(t, created) | |
| 123 | - | assert.Equal(t, want, repo) | |
| 120 | + | require.NoError(t, f.pushes.Open(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, true)) | |
| 124 | 121 | }) | |
| 125 | 122 | ||
| 126 | 123 | t.Run("a concurrent first push wins: this one pushes to its repository", func(t *testing.T) { | |
| @@ -131,17 +128,15 @@ func TestPushesOpen(t *testing.T) { | |||
| 131 | 128 | f.git.EXPECT().Discard(mock.Anything, ports.StagedID("s1")).Return(nil) | |
| 132 | 129 | f.meta.EXPECT().Get(ctx, n).Return(priv, nil).Once() | |
| 133 | 130 | ||
| 134 | - | repo, created, err := f.pushes.Open(ctx, domain.Root(), n, true) | |
| 135 | - | require.NoError(t, err) | |
| 136 | - | assert.False(t, created) | |
| 137 | - | assert.Equal(t, priv, repo) | |
| 131 | + | // The bus mock expects no Publish: the repository was not created here. | |
| 132 | + | require.NoError(t, f.pushes.Open(ctx, domain.Root(), n, true)) | |
| 138 | 133 | }) | |
| 139 | 134 | ||
| 140 | 135 | t.Run("store failure", func(t *testing.T) { | |
| 141 | 136 | f := newPushFixture(t) | |
| 142 | 137 | boom := errors.New("boom") | |
| 143 | 138 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, boom) | |
| 144 | - | _, _, err := f.pushes.Open(ctx, domain.Root(), n, true) | |
| 139 | + | err := f.pushes.Open(ctx, domain.Root(), n, true) | |
| 145 | 140 | assert.ErrorIs(t, err, boom) | |
| 146 | 141 | }) | |
| 147 | 142 | ||
| @@ -150,7 +145,7 @@ func TestPushesOpen(t *testing.T) { | |||
| 150 | 145 | boom := errors.New("boom") | |
| 151 | 146 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |
| 152 | 147 | f.git.EXPECT().Stage(ctx).Return(ports.StagedID(""), boom) | |
| 153 | - | _, _, err := f.pushes.Open(ctx, domain.Root(), n, true) | |
| 148 | + | err := f.pushes.Open(ctx, domain.Root(), n, true) | |
| 154 | 149 | assert.ErrorIs(t, err, boom) | |
| 155 | 150 | }) | |
| 156 | 151 | } | |
| @@ -158,13 +153,52 @@ func TestPushesOpen(t *testing.T) { | |||
| 158 | 153 | func TestPushesCheck(t *testing.T) { | |
| 159 | 154 | ctx := context.Background() | |
| 160 | 155 | n := name(t, "tools") | |
| 161 | - | f := newPushFixture(t) | |
| 162 | 156 | ups := []domain.RefUpdate{{Old: domain.ZeroOID, New: oidA, Ref: "refs/heads/main"}} | |
| 157 | + | writer := domain.TokenPrincipal(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite), Repos: names(t, "tools")}) | |
| 158 | + | reader := domain.TokenPrincipal(domain.Token{ID: "r1", Scopes: scopes(domain.ScopeRepoRead)}) | |
| 159 | + | ||
| 160 | + | t.Run("root is always valid and needs no lookup", func(t *testing.T) { | |
| 161 | + | f := newPushFixture(t) // the token store mock expects no call | |
| 162 | + | require.NoError(t, f.pushes.Check(ctx, domain.Root(), n, ups)) | |
| 163 | + | }) | |
| 164 | + | ||
| 165 | + | t.Run("a writer whose token still exists", func(t *testing.T) { | |
| 166 | + | f := newPushFixture(t) | |
| 167 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite)}, nil) | |
| 168 | + | require.NoError(t, f.pushes.Check(ctx, writer, n, ups)) | |
| 169 | + | }) | |
| 170 | + | ||
| 171 | + | t.Run("a token revoked during the push is rejected", func(t *testing.T) { | |
| 172 | + | f := newPushFixture(t) | |
| 173 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{}, domain.ErrNotFound) | |
| 174 | + | assert.ErrorIs(t, f.pushes.Check(ctx, writer, n, ups), domain.ErrRejected) | |
| 175 | + | }) | |
| 176 | + | ||
| 177 | + | t.Run("a token that expired during the push is rejected", func(t *testing.T) { | |
| 178 | + | f := newPushFixture(t) | |
| 179 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite), ExpiresAt: now.Add(-time.Second)}, nil) | |
| 180 | + | assert.ErrorIs(t, f.pushes.Check(ctx, writer, n, ups), domain.ErrRejected) | |
| 181 | + | }) | |
| 182 | + | ||
| 183 | + | t.Run("a store failure fails closed", func(t *testing.T) { | |
| 184 | + | f := newPushFixture(t) | |
| 185 | + | boom := errors.New("boom") | |
| 186 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{}, boom) | |
| 187 | + | err := f.pushes.Check(ctx, writer, n, ups) | |
| 188 | + | require.ErrorIs(t, err, boom) | |
| 189 | + | assert.NotErrorIs(t, err, domain.ErrRejected, "an internal failure, not a refusal") | |
| 190 | + | }) | |
| 191 | + | ||
| 192 | + | t.Run("a token without write access is rejected", func(t *testing.T) { | |
| 193 | + | f := newPushFixture(t) | |
| 194 | + | f.tokens.EXPECT().Get(ctx, "r1").Return(domain.Token{ID: "r1", Scopes: scopes(domain.ScopeRepoRead)}, nil) | |
| 195 | + | assert.ErrorIs(t, f.pushes.Check(ctx, reader, n, ups), domain.ErrRejected) | |
| 196 | + | }) | |
| 163 | 197 | ||
| 164 | - | require.NoError(t, f.pushes.Check(ctx, domain.Root(), n, ups)) | |
| 165 | - | require.NoError(t, f.pushes.Check(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, ups)) | |
| 166 | - | assert.ErrorIs(t, f.pushes.Check(ctx, tokenPrincipal(domain.ScopeRepoRead), n, ups), domain.ErrRejected) | |
| 167 | - | assert.ErrorIs(t, f.pushes.Check(ctx, domain.Anonymous(), n, ups), domain.ErrRejected) | |
| 198 | + | t.Run("anonymous is rejected", func(t *testing.T) { | |
| 199 | + | f := newPushFixture(t) | |
| 200 | + | assert.ErrorIs(t, f.pushes.Check(ctx, domain.Anonymous(), n, ups), domain.ErrRejected) | |
| 201 | + | }) | |
| 168 | 202 | } | |
| 169 | 203 | ||
| 170 | 204 | func TestPushesRecord(t *testing.T) { | |
| @@ -209,6 +243,23 @@ func TestPushesRecord(t *testing.T) { | |||
| 209 | 243 | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | |
| 210 | 244 | }) | |
| 211 | 245 | ||
| 246 | + | t.Run("a repository deleted before the default branch step is not an error", func(t *testing.T) { | |
| 247 | + | f := newPushFixture(t) | |
| 248 | + | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |
| 249 | + | f.git.EXPECT().DefaultBranch(ctx, n).Return("", false, domain.ErrNotFound) | |
| 250 | + | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |
| 251 | + | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | |
| 252 | + | }) | |
| 253 | + | ||
| 254 | + | t.Run("a repository deleted before HEAD is set is not an error", func(t *testing.T) { | |
| 255 | + | f := newPushFixture(t) | |
| 256 | + | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |
| 257 | + | f.git.EXPECT().DefaultBranch(ctx, n).Return("trunk", false, nil) | |
| 258 | + | f.git.EXPECT().SetDefaultBranch(ctx, n, "main").Return(domain.ErrNotFound) | |
| 259 | + | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |
| 260 | + | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | |
| 261 | + | }) | |
| 262 | + | ||
| 212 | 263 | t.Run("every step runs when one fails", func(t *testing.T) { | |
| 213 | 264 | f := newPushFixture(t) | |
| 214 | 265 | boom := errors.New("boom") | |
| @@ -219,17 +270,6 @@ func TestPushesRecord(t *testing.T) { | |||
| 219 | 270 | assert.ErrorIs(t, err, boom) | |
| 220 | 271 | assert.Equal(t, []domain.RepoName{n}, f.sched.names, "maintenance still scheduled") | |
| 221 | 272 | }) | |
| 222 | - | ||
| 223 | - | t.Run("nil scheduler is allowed", func(t *testing.T) { | |
| 224 | - | f := newPushFixture(t) | |
| 225 | - | repos := app.NewRepos(f.meta, f.git, f.clock) | |
| 226 | - | pushes := app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, nil) | |
| 227 | - | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{Name: n}, nil) | |
| 228 | - | f.meta.EXPECT().Update(ctx, mock.Anything).Return(nil) | |
| 229 | - | f.git.EXPECT().DefaultBranch(ctx, n).Return("main", true, nil) | |
| 230 | - | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |
| 231 | - | require.NoError(t, pushes.Record(ctx, p, n, ups)) | |
| 232 | - | }) | |
| 233 | 273 | } | |
| 234 | 274 | ||
| 235 | 275 | func TestPushesDefaultBranchRule(t *testing.T) { | |
| @@ -24,7 +24,7 @@ type scheduler struct { | |||||
| 24 | names []domain.RepoName | 24 | names []domain.RepoName | ||
| 25 | } | 25 | } | ||
| 26 | 26 | ||||
| 27 | - | func (s *scheduler) Schedule(n domain.RepoName) { | 27 | + | func (s *scheduler) Schedule(_ context.Context, n domain.RepoName) { |
| 28 | s.mu.Lock() | 28 | s.mu.Lock() | ||
| 29 | defer s.mu.Unlock() | 29 | defer s.mu.Unlock() | ||
| 30 | s.names = append(s.names, n) | 30 | s.names = append(s.names, n) | ||
| @@ -35,6 +35,7 @@ type pushFixture struct { | |||||
| 35 | git *mocks.GitRepos | 35 | git *mocks.GitRepos | ||
| 36 | bus *mocks.EventBus | 36 | bus *mocks.EventBus | ||
| 37 | clock *mocks.Clock | 37 | clock *mocks.Clock | ||
| 38 | + | tokens *mocks.TokenStore | |||
| 38 | sched *scheduler | 39 | sched *scheduler | ||
| 39 | pushes *app.Pushes | 40 | pushes *app.Pushes | ||
| 40 | } | 41 | } | ||
| @@ -42,15 +43,17 @@ type pushFixture struct { | |||||
| 42 | func newPushFixture(t *testing.T) *pushFixture { | 43 | func newPushFixture(t *testing.T) *pushFixture { | ||
| 43 | t.Helper() | 44 | t.Helper() | ||
| 44 | f := &pushFixture{ | 45 | f := &pushFixture{ | ||
| 45 | - | meta: mocks.NewRepoMetaStore(t), | 46 | + | meta: mocks.NewRepoMetaStore(t), |
| 46 | - | git: mocks.NewGitRepos(t), | 47 | + | git: mocks.NewGitRepos(t), |
| 47 | - | bus: mocks.NewEventBus(t), | 48 | + | bus: mocks.NewEventBus(t), |
| 48 | - | clock: mocks.NewClock(t), | 49 | + | clock: mocks.NewClock(t), |
| 49 | - | sched: &scheduler{}, | 50 | + | tokens: mocks.NewTokenStore(t), |
| 51 | + | sched: &scheduler{}, | |||
| 50 | } | 52 | } | ||
| 51 | f.clock.EXPECT().Now().Return(now).Maybe() | 53 | f.clock.EXPECT().Now().Return(now).Maybe() | ||
| 52 | repos := app.NewRepos(f.meta, f.git, f.clock) | 54 | repos := app.NewRepos(f.meta, f.git, f.clock) | ||
| 53 | - | f.pushes = app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, f.sched) | 55 | + | auth := app.NewAuth(domain.HashTokenSecret(rootSecret), f.tokens, f.clock) |
| 56 | + | f.pushes = app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, f.sched, auth) | |||
| 54 | return f | 57 | return f | ||
| 55 | } | 58 | } | ||
| 56 | 59 | ||||
| @@ -66,25 +69,24 @@ func TestPushesOpen(t *testing.T) { | |||||
| 66 | pub := domain.Repo{Name: n, Visibility: domain.VisibilityPublic} | 69 | pub := domain.Repo{Name: n, Visibility: domain.VisibilityPublic} | ||
| 67 | 70 | ||||
| 68 | tests := []struct { | 71 | tests := []struct { | ||
| 69 | - | name string | 72 | + | name string |
| 70 | - | p domain.Principal | 73 | + | p domain.Principal |
| 71 | - | repo *domain.Repo // nil: missing | 74 | + | repo *domain.Repo // nil: missing |
| 72 | - | create bool | 75 | + | create bool |
| 73 | - | want error | 76 | + | want error |
| 74 | - | created bool | |||
| 75 | }{ | 77 | }{ | ||
| 76 | - | {"anonymous", domain.Anonymous(), &pub, false, domain.ErrUnauthenticated, false}, | 78 | + | {"anonymous", domain.Anonymous(), &pub, false, domain.ErrUnauthenticated}, |
| 77 | - | {"root, existing", domain.Root(), &priv, false, nil, false}, | 79 | + | {"root, existing", domain.Root(), &priv, false, nil}, |
| 78 | - | {"writer, existing", tokenPrincipal(domain.ScopeRepoWrite), &priv, false, nil, false}, | 80 | + | {"writer, existing", tokenPrincipal(domain.ScopeRepoWrite), &priv, false, nil}, |
| 79 | - | {"writer limited to the repo", tokenPrincipal(domain.ScopeRepoWrite, "tools"), &priv, false, nil, false}, | 81 | + | {"writer limited to the repo", tokenPrincipal(domain.ScopeRepoWrite, "tools"), &priv, false, nil}, |
| 80 | - | {"writer limited to another repo, private", tokenPrincipal(domain.ScopeRepoWrite, "other"), &priv, false, domain.ErrNotFound, false}, | 82 | + | {"writer limited to another repo, private", tokenPrincipal(domain.ScopeRepoWrite, "other"), &priv, false, domain.ErrNotFound}, |
| 81 | - | {"writer limited to another repo, public", tokenPrincipal(domain.ScopeRepoWrite, "other"), &pub, false, domain.ErrForbidden, false}, | 83 | + | {"writer limited to another repo, public", tokenPrincipal(domain.ScopeRepoWrite, "other"), &pub, false, domain.ErrForbidden}, |
| 82 | - | {"reader of a private repo", tokenPrincipal(domain.ScopeRepoRead), &priv, false, domain.ErrForbidden, false}, | 84 | + | {"reader of a private repo", tokenPrincipal(domain.ScopeRepoRead), &priv, false, domain.ErrForbidden}, |
| 83 | - | {"reader of a public repo", tokenPrincipal(domain.ScopeRepoRead), &pub, false, domain.ErrForbidden, false}, | 85 | + | {"reader of a public repo", tokenPrincipal(domain.ScopeRepoRead), &pub, false, domain.ErrForbidden}, |
| 84 | - | {"token with no relevant scope, private", tokenPrincipal(domain.ScopeMRWrite), &priv, false, domain.ErrNotFound, false}, | 86 | + | {"token with no relevant scope, private", tokenPrincipal(domain.ScopeMRWrite), &priv, false, domain.ErrNotFound}, |
| 85 | - | {"missing, no create", domain.Root(), nil, false, domain.ErrNotFound, false}, | 87 | + | {"missing, no create", domain.Root(), nil, false, domain.ErrNotFound}, |
| 86 | - | {"missing, reader may not create", tokenPrincipal(domain.ScopeRepoRead), nil, true, domain.ErrNotFound, false}, | 88 | + | {"missing, reader may not create", tokenPrincipal(domain.ScopeRepoRead), nil, true, domain.ErrNotFound}, |
| 87 | - | {"missing, writer limited elsewhere", tokenPrincipal(domain.ScopeRepoWrite, "other"), nil, true, domain.ErrNotFound, false}, | 89 | + | {"missing, writer limited elsewhere", tokenPrincipal(domain.ScopeRepoWrite, "other"), nil, true, domain.ErrNotFound}, |
| 88 | } | 90 | } | ||
| 89 | for _, tt := range tests { | 91 | for _, tt := range tests { | ||
| 90 | t.Run(tt.name, func(t *testing.T) { | 92 | t.Run(tt.name, func(t *testing.T) { | ||
| @@ -96,15 +98,13 @@ func TestPushesOpen(t *testing.T) { | |||||
| 96 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | 98 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | ||
| 97 | } | 99 | } | ||
| 98 | } | 100 | } | ||
| 99 | - | repo, created, err := f.pushes.Open(ctx, tt.p, n, tt.create) | 101 | + | // The bus mock expects no Publish: nothing is created here. |
| 102 | + | err := f.pushes.Open(ctx, tt.p, n, tt.create) | |||
| 100 | if tt.want != nil { | 103 | if tt.want != nil { | ||
| 101 | assert.ErrorIs(t, err, tt.want) | 104 | assert.ErrorIs(t, err, tt.want) | ||
| 102 | - | assert.False(t, created) | |||
| 103 | return | 105 | return | ||
| 104 | } | 106 | } | ||
| 105 | require.NoError(t, err) | 107 | require.NoError(t, err) | ||
| 106 | - | assert.Equal(t, tt.created, created) | |||
| 107 | - | assert.Equal(t, n, repo.Name) | |||
| 108 | }) | 108 | }) | ||
| 109 | } | 109 | } | ||
| 110 | 110 | ||||
| @@ -117,10 +117,7 @@ func TestPushesOpen(t *testing.T) { | |||||
| 117 | f.git.EXPECT().Publish(ctx, ports.StagedID("s1"), n).Return(nil) | 117 | f.git.EXPECT().Publish(ctx, ports.StagedID("s1"), n).Return(nil) | ||
| 118 | f.bus.EXPECT().Publish(ctx, domain.RepoCreated{Repo: n, Visibility: domain.VisibilityPrivate, At: now}).Return() | 118 | f.bus.EXPECT().Publish(ctx, domain.RepoCreated{Repo: n, Visibility: domain.VisibilityPrivate, At: now}).Return() | ||
| 119 | 119 | ||||
| 120 | - | repo, created, err := f.pushes.Open(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, true) | 120 | + | require.NoError(t, f.pushes.Open(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, true)) |
| 121 | - | require.NoError(t, err) | |||
| 122 | - | assert.True(t, created) | |||
| 123 | - | assert.Equal(t, want, repo) | |||
| 124 | }) | 121 | }) | ||
| 125 | 122 | ||||
| 126 | t.Run("a concurrent first push wins: this one pushes to its repository", func(t *testing.T) { | 123 | t.Run("a concurrent first push wins: this one pushes to its repository", func(t *testing.T) { | ||
| @@ -131,17 +128,15 @@ func TestPushesOpen(t *testing.T) { | |||||
| 131 | f.git.EXPECT().Discard(mock.Anything, ports.StagedID("s1")).Return(nil) | 128 | f.git.EXPECT().Discard(mock.Anything, ports.StagedID("s1")).Return(nil) | ||
| 132 | f.meta.EXPECT().Get(ctx, n).Return(priv, nil).Once() | 129 | f.meta.EXPECT().Get(ctx, n).Return(priv, nil).Once() | ||
| 133 | 130 | ||||
| 134 | - | repo, created, err := f.pushes.Open(ctx, domain.Root(), n, true) | 131 | + | // The bus mock expects no Publish: the repository was not created here. |
| 135 | - | require.NoError(t, err) | 132 | + | require.NoError(t, f.pushes.Open(ctx, domain.Root(), n, true)) |
| 136 | - | assert.False(t, created) | |||
| 137 | - | assert.Equal(t, priv, repo) | |||
| 138 | }) | 133 | }) | ||
| 139 | 134 | ||||
| 140 | t.Run("store failure", func(t *testing.T) { | 135 | t.Run("store failure", func(t *testing.T) { | ||
| 141 | f := newPushFixture(t) | 136 | f := newPushFixture(t) | ||
| 142 | boom := errors.New("boom") | 137 | boom := errors.New("boom") | ||
| 143 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, boom) | 138 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, boom) | ||
| 144 | - | _, _, err := f.pushes.Open(ctx, domain.Root(), n, true) | 139 | + | err := f.pushes.Open(ctx, domain.Root(), n, true) |
| 145 | assert.ErrorIs(t, err, boom) | 140 | assert.ErrorIs(t, err, boom) | ||
| 146 | }) | 141 | }) | ||
| 147 | 142 | ||||
| @@ -150,7 +145,7 @@ func TestPushesOpen(t *testing.T) { | |||||
| 150 | boom := errors.New("boom") | 145 | boom := errors.New("boom") | ||
| 151 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | 146 | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | ||
| 152 | f.git.EXPECT().Stage(ctx).Return(ports.StagedID(""), boom) | 147 | f.git.EXPECT().Stage(ctx).Return(ports.StagedID(""), boom) | ||
| 153 | - | _, _, err := f.pushes.Open(ctx, domain.Root(), n, true) | 148 | + | err := f.pushes.Open(ctx, domain.Root(), n, true) |
| 154 | assert.ErrorIs(t, err, boom) | 149 | assert.ErrorIs(t, err, boom) | ||
| 155 | }) | 150 | }) | ||
| 156 | } | 151 | } | ||
| @@ -158,13 +153,52 @@ func TestPushesOpen(t *testing.T) { | |||||
| 158 | func TestPushesCheck(t *testing.T) { | 153 | func TestPushesCheck(t *testing.T) { | ||
| 159 | ctx := context.Background() | 154 | ctx := context.Background() | ||
| 160 | n := name(t, "tools") | 155 | n := name(t, "tools") | ||
| 161 | - | f := newPushFixture(t) | |||
| 162 | ups := []domain.RefUpdate{{Old: domain.ZeroOID, New: oidA, Ref: "refs/heads/main"}} | 156 | ups := []domain.RefUpdate{{Old: domain.ZeroOID, New: oidA, Ref: "refs/heads/main"}} | ||
| 157 | + | writer := domain.TokenPrincipal(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite), Repos: names(t, "tools")}) | |||
| 158 | + | reader := domain.TokenPrincipal(domain.Token{ID: "r1", Scopes: scopes(domain.ScopeRepoRead)}) | |||
| 159 | + | ||||
| 160 | + | t.Run("root is always valid and needs no lookup", func(t *testing.T) { | |||
| 161 | + | f := newPushFixture(t) // the token store mock expects no call | |||
| 162 | + | require.NoError(t, f.pushes.Check(ctx, domain.Root(), n, ups)) | |||
| 163 | + | }) | |||
| 164 | + | ||||
| 165 | + | t.Run("a writer whose token still exists", func(t *testing.T) { | |||
| 166 | + | f := newPushFixture(t) | |||
| 167 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite)}, nil) | |||
| 168 | + | require.NoError(t, f.pushes.Check(ctx, writer, n, ups)) | |||
| 169 | + | }) | |||
| 170 | + | ||||
| 171 | + | t.Run("a token revoked during the push is rejected", func(t *testing.T) { | |||
| 172 | + | f := newPushFixture(t) | |||
| 173 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{}, domain.ErrNotFound) | |||
| 174 | + | assert.ErrorIs(t, f.pushes.Check(ctx, writer, n, ups), domain.ErrRejected) | |||
| 175 | + | }) | |||
| 176 | + | ||||
| 177 | + | t.Run("a token that expired during the push is rejected", func(t *testing.T) { | |||
| 178 | + | f := newPushFixture(t) | |||
| 179 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{ID: "w1", Scopes: scopes(domain.ScopeRepoWrite), ExpiresAt: now.Add(-time.Second)}, nil) | |||
| 180 | + | assert.ErrorIs(t, f.pushes.Check(ctx, writer, n, ups), domain.ErrRejected) | |||
| 181 | + | }) | |||
| 182 | + | ||||
| 183 | + | t.Run("a store failure fails closed", func(t *testing.T) { | |||
| 184 | + | f := newPushFixture(t) | |||
| 185 | + | boom := errors.New("boom") | |||
| 186 | + | f.tokens.EXPECT().Get(ctx, "w1").Return(domain.Token{}, boom) | |||
| 187 | + | err := f.pushes.Check(ctx, writer, n, ups) | |||
| 188 | + | require.ErrorIs(t, err, boom) | |||
| 189 | + | assert.NotErrorIs(t, err, domain.ErrRejected, "an internal failure, not a refusal") | |||
| 190 | + | }) | |||
| 191 | + | ||||
| 192 | + | t.Run("a token without write access is rejected", func(t *testing.T) { | |||
| 193 | + | f := newPushFixture(t) | |||
| 194 | + | f.tokens.EXPECT().Get(ctx, "r1").Return(domain.Token{ID: "r1", Scopes: scopes(domain.ScopeRepoRead)}, nil) | |||
| 195 | + | assert.ErrorIs(t, f.pushes.Check(ctx, reader, n, ups), domain.ErrRejected) | |||
| 196 | + | }) | |||
| 163 | 197 | ||||
| 164 | - | require.NoError(t, f.pushes.Check(ctx, domain.Root(), n, ups)) | 198 | + | t.Run("anonymous is rejected", func(t *testing.T) { |
| 165 | - | require.NoError(t, f.pushes.Check(ctx, tokenPrincipal(domain.ScopeRepoWrite, "tools"), n, ups)) | 199 | + | f := newPushFixture(t) |
| 166 | - | assert.ErrorIs(t, f.pushes.Check(ctx, tokenPrincipal(domain.ScopeRepoRead), n, ups), domain.ErrRejected) | 200 | + | assert.ErrorIs(t, f.pushes.Check(ctx, domain.Anonymous(), n, ups), domain.ErrRejected) |
| 167 | - | assert.ErrorIs(t, f.pushes.Check(ctx, domain.Anonymous(), n, ups), domain.ErrRejected) | 201 | + | }) |
| 168 | } | 202 | } | ||
| 169 | 203 | ||||
| 170 | func TestPushesRecord(t *testing.T) { | 204 | func TestPushesRecord(t *testing.T) { | ||
| @@ -209,6 +243,23 @@ func TestPushesRecord(t *testing.T) { | |||||
| 209 | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | 243 | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | ||
| 210 | }) | 244 | }) | ||
| 211 | 245 | ||||
| 246 | + | t.Run("a repository deleted before the default branch step is not an error", func(t *testing.T) { | |||
| 247 | + | f := newPushFixture(t) | |||
| 248 | + | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |||
| 249 | + | f.git.EXPECT().DefaultBranch(ctx, n).Return("", false, domain.ErrNotFound) | |||
| 250 | + | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |||
| 251 | + | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | |||
| 252 | + | }) | |||
| 253 | + | ||||
| 254 | + | t.Run("a repository deleted before HEAD is set is not an error", func(t *testing.T) { | |||
| 255 | + | f := newPushFixture(t) | |||
| 256 | + | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{}, domain.ErrNotFound) | |||
| 257 | + | f.git.EXPECT().DefaultBranch(ctx, n).Return("trunk", false, nil) | |||
| 258 | + | f.git.EXPECT().SetDefaultBranch(ctx, n, "main").Return(domain.ErrNotFound) | |||
| 259 | + | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |||
| 260 | + | require.NoError(t, f.pushes.Record(ctx, p, n, ups)) | |||
| 261 | + | }) | |||
| 262 | + | ||||
| 212 | t.Run("every step runs when one fails", func(t *testing.T) { | 263 | t.Run("every step runs when one fails", func(t *testing.T) { | ||
| 213 | f := newPushFixture(t) | 264 | f := newPushFixture(t) | ||
| 214 | boom := errors.New("boom") | 265 | boom := errors.New("boom") | ||
| @@ -219,17 +270,6 @@ func TestPushesRecord(t *testing.T) { | |||||
| 219 | assert.ErrorIs(t, err, boom) | 270 | assert.ErrorIs(t, err, boom) | ||
| 220 | assert.Equal(t, []domain.RepoName{n}, f.sched.names, "maintenance still scheduled") | 271 | assert.Equal(t, []domain.RepoName{n}, f.sched.names, "maintenance still scheduled") | ||
| 221 | }) | 272 | }) | ||
| 222 | - | ||||
| 223 | - | t.Run("nil scheduler is allowed", func(t *testing.T) { | |||
| 224 | - | f := newPushFixture(t) | |||
| 225 | - | repos := app.NewRepos(f.meta, f.git, f.clock) | |||
| 226 | - | pushes := app.NewPushes(f.meta, repos, f.git, f.bus, f.clock, nil) | |||
| 227 | - | f.meta.EXPECT().Get(ctx, n).Return(domain.Repo{Name: n}, nil) | |||
| 228 | - | f.meta.EXPECT().Update(ctx, mock.Anything).Return(nil) | |||
| 229 | - | f.git.EXPECT().DefaultBranch(ctx, n).Return("main", true, nil) | |||
| 230 | - | f.bus.EXPECT().Publish(ctx, mock.Anything).Return() | |||
| 231 | - | require.NoError(t, pushes.Record(ctx, p, n, ups)) | |||
| 232 | - | }) | |||
| 233 | } | 273 | } | ||
| 234 | 274 | ||||
| 235 | func TestPushesDefaultBranchRule(t *testing.T) { | 275 | func TestPushesDefaultBranchRule(t *testing.T) { | ||