fix(ci): judge the pushed commits in the workflow push guard (#30)
- commit
- 7328df5a20ccf8a38e21d1366844ce6969cf0065
- author
- brice · · 2026-10-14 23:50 UTC
- committer
- bnema · · 2026-10-14 23:50 UTC
- parent
- dccd128
- signature
- SSH Unverified
SHA256:VOCA5lYARHcbKDZ7GhTV45VmcjWnJ6ZQIBazj17LivAThe key is unknown - refs
- tag: v0.12.1
7 files · +492 -50
M internal/adapters/gitcli/export_test.go +15 -0
| @@ -3,7 +3,9 @@ | |||
| 3 | 3 | package gitcli | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "bufio" | |
| 6 | 7 | "context" | |
| 8 | + | "io" | |
| 7 | 9 | "sort" | |
| 8 | 10 | ||
| 9 | 11 | "bnema.dev/ermite/internal/domain" | |
| @@ -107,3 +109,16 @@ func (g *Git) CatFileWaiting() int { | |||
| 107 | 109 | } | |
| 108 | 110 | return n | |
| 109 | 111 | } | |
| 112 | + | ||
| 113 | + | // MaxPushedCommits is the bound of the commits of a push, for the tests. | |
| 114 | + | const MaxPushedCommits = maxPushedCommits | |
| 115 | + | ||
| 116 | + | // ParseRevList reads rev-list output with the bounds of the guard replaced, and | |
| 117 | + | // returns the number of commits kept. | |
| 118 | + | func ParseRevList(r io.Reader, maxCommits, maxIDs int) (int, error) { | |
| 119 | + | c, err := parseRevList(bufio.NewReaderSize(r, 64*1024), maxCommits, maxIDs) | |
| 120 | + | return len(c), err | |
| 121 | + | } | |
| 122 | + | ||
| 123 | + | // MaxPushedObjects is the bound of the ids of the commits of a push. | |
| 124 | + | const MaxPushedObjects = maxPushedObjects | |
| @@ -3,7 +3,9 @@ | |||||
| 3 | package gitcli | 3 | package gitcli | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "bufio" | |||
| 6 | "context" | 7 | "context" | ||
| 8 | + | "io" | |||
| 7 | "sort" | 9 | "sort" | ||
| 8 | 10 | ||||
| 9 | "bnema.dev/ermite/internal/domain" | 11 | "bnema.dev/ermite/internal/domain" | ||
| @@ -107,3 +109,16 @@ func (g *Git) CatFileWaiting() int { | |||||
| 107 | } | 109 | } | ||
| 108 | return n | 110 | return n | ||
| 109 | } | 111 | } | ||
| 112 | + | ||||
| 113 | + | // MaxPushedCommits is the bound of the commits of a push, for the tests. | |||
| 114 | + | const MaxPushedCommits = maxPushedCommits | |||
| 115 | + | ||||
| 116 | + | // ParseRevList reads rev-list output with the bounds of the guard replaced, and | |||
| 117 | + | // returns the number of commits kept. | |||
| 118 | + | func ParseRevList(r io.Reader, maxCommits, maxIDs int) (int, error) { | |||
| 119 | + | c, err := parseRevList(bufio.NewReaderSize(r, 64*1024), maxCommits, maxIDs) | |||
| 120 | + | return len(c), err | |||
| 121 | + | } | |||
| 122 | + | ||||
| 123 | + | // MaxPushedObjects is the bound of the ids of the commits of a push. | |||
| 124 | + | const MaxPushedObjects = maxPushedObjects | |||
M internal/adapters/gitcli/workflow_guard.go +209 -31
| @@ -7,7 +7,9 @@ import ( | |||
| 7 | 7 | "context" | |
| 8 | 8 | "errors" | |
| 9 | 9 | "fmt" | |
| 10 | + | "io" | |
| 10 | 11 | "path/filepath" | |
| 12 | + | "strconv" | |
| 11 | 13 | "strings" | |
| 12 | 14 | ||
| 13 | 15 | "bnema.dev/ermite/internal/domain" | |
| @@ -23,36 +25,60 @@ const workflowsDir = ".github/workflows/" | |||
| 23 | 25 | // fails closed (the push is refused), whatever the trees hold. | |
| 24 | 26 | const maxWorkflowPath = 4096 | |
| 25 | 27 | ||
| 26 | - | // ChangedWorkflow tells whether a push creates or changes a file under | |
| 27 | - | // .github/workflows/. For each update that does not delete its ref, the | |
| 28 | - | // directory .github/workflows of the old commit (none for a new ref) is | |
| 29 | - | // compared with the one of the new commit: the same tree id is the same | |
| 30 | - | // content, whatever it holds, so nothing is read; a different one, or one that | |
| 31 | - | // appears, is a change, and the first top-level entry that differs names it. A | |
| 32 | - | // new ref is thus compared with nothing: any workflow it carries counts, as on | |
| 33 | - | // GitHub. A directory that disappears is not a change (as a deleted ref is | |
| 34 | - | // not). | |
| 28 | + | // Bounds of the walk over the commits a push brings: more than that, or a | |
| 29 | + | // commit with more parents than a line of rev-list holds, fails closed (the | |
| 30 | + | // push is refused), whatever the commits hold. maxPushedObjects counts every | |
| 31 | + | // commit and every parent listed, repeats included (so it bounds the distinct | |
| 32 | + | // ids too), and is enforced as the lines are read, before anything more is | |
| 33 | + | // kept: a few thousand commits with two hundred parents each are refused after | |
| 34 | + | // the first lines, not after they have been held. | |
| 35 | + | const ( | |
| 36 | + | maxPushedCommits = 10000 | |
| 37 | + | maxPushedObjects = 4 * maxPushedCommits | |
| 38 | + | maxRevListLine = 8192 | |
| 39 | + | ) | |
| 40 | + | ||
| 41 | + | // ChangedWorkflow tells whether a push changes the workflows of a repository. | |
| 42 | + | // It returns the path of the first change, "" for none. Deleting a ref changes | |
| 43 | + | // nothing, and a workflows directory that disappears is not a change. | |
| 44 | + | // | |
| 45 | + | // Two things are judged, as GitHub does for the token of a job (it refuses a | |
| 46 | + | // push when the commits pushed change workflow files, not when a new branch is | |
| 47 | + | // cut from a commit that has them): | |
| 48 | + | // | |
| 49 | + | // 1. Every commit the push brings (`rev-list <new> --not --all`, so the commits | |
| 50 | + | // no ref has yet) must leave .github/workflows as it found it: it is | |
| 51 | + | // compared with each of its parents (a root commit with nothing) and is a | |
| 52 | + | // change when it is present and differs from every one of them. A merge of | |
| 53 | + | // existing commits is thus no change; a side that brings a change is itself | |
| 54 | + | // a new commit, and judged as one. | |
| 55 | + | // 2. The directory a ref ends with must not be one it did not have: for an | |
| 56 | + | // update the final .github/workflows must be the old one or absent, for a | |
| 57 | + | // new ref the one of the default branch (HEAD) or absent. This keeps a job | |
| 58 | + | // token from moving a ref onto existing commits that carry other workflows | |
| 59 | + | // (an older commit, a commit of a branch with different ones). | |
| 35 | 60 | // | |
| 36 | - | // What counts as the directory is exactly what the workflow loader | |
| 37 | - | // (CI.workflowFiles, on both git readers) lists: .github and workflows both | |
| 38 | - | // entries of mode 040000 and type tree. A path through a gitlink (160000), a | |
| 39 | - | // symbolic link (120000, which the loader does not follow) or a file is no | |
| 40 | - | // workflows, so the same tree id that is no workflows as a gitlink and becomes | |
| 41 | - | // workflows as a directory is a change, which the object id alone would not | |
| 42 | - | // tell (`cat-file` mode and type of "<commit>:.github/workflows"). | |
| 61 | + | // Presence is the directory the workflow loader lists (CI.workflowFiles, on both | |
| 62 | + | // git readers): .github and workflows both entries of mode 040000 and type | |
| 63 | + | // tree. A path through a gitlink (160000), a symbolic link (120000, which the | |
| 64 | + | // loader does not follow) or a file is no workflows, so the same tree id that is | |
| 65 | + | // no workflows as a gitlink and becomes workflows as a directory is a change, | |
| 66 | + | // which the object id alone would not tell. Mode and id of | |
| 67 | + | // "<commit>:.github/workflows" are what is compared: equal means the same | |
| 68 | + | // content, whatever it holds, so nothing is read. | |
| 43 | 69 | // | |
| 44 | - | // The comparison never walks the trees recursively: the trees of a push are the | |
| 45 | - | // pusher's, and a recursive diff of trees that share subtrees fans out into | |
| 46 | - | // more paths than the push has bytes (git builds the whole diff before it | |
| 47 | - | // prints a line, so reading it as a stream would not help). The shallow diff | |
| 48 | - | // that names the path reads one directory, as large as its tree object, as a | |
| 49 | - | // stream, up to its first path, and git is stopped and reaped then; a path | |
| 50 | - | // longer than maxWorkflowPath fails closed. | |
| 70 | + | // Nothing is walked recursively: the trees of a push are the pusher's, and a | |
| 71 | + | // recursive diff of trees that share subtrees fans out into more paths than the | |
| 72 | + | // push has bytes (git builds the whole diff before it prints a line, so reading | |
| 73 | + | // it as a stream would not help). The shallow diff that names the path reads one | |
| 74 | + | // directory, as large as its tree object, as a stream, up to its first name, and | |
| 75 | + | // git is stopped and reaped then; a name longer than maxWorkflowPath fails | |
| 76 | + | // closed. | |
| 51 | 77 | // | |
| 52 | - | // Both ends are peeled first (`cat-file --batch-check` on `<oid>^{}`): a | |
| 53 | - | // commit or a tree is compared, a blob (a tag may point to one) holds no | |
| 54 | - | // workflows; a missing or malformed object fails closed. Git reads the objects | |
| 55 | - | // of a push still in quarantine through the environment of the hook | |
| 78 | + | // The ends of each update are peeled first (`cat-file --batch-check` on | |
| 79 | + | // `<oid>^{}`): a commit or a tree is compared, a blob (a tag may point to one) | |
| 80 | + | // holds no workflows; a missing or malformed object fails closed. Git reads the | |
| 81 | + | // objects of a push still in quarantine through the environment of the hook | |
| 56 | 82 | // (ports.PushObjects), accepted only inside the repository's objects directory. | |
| 57 | 83 | // | |
| 58 | 84 | //hexcheck:ignore suspicious-business-logic-in-adapter reads the object ids and the path git prints for the compared trees; the rule that refuses is the use case's | |
| @@ -78,23 +104,175 @@ func (g *Git) ChangedWorkflow(ctx context.Context, repo domain.RepoName, updates | |||
| 78 | 104 | if err != nil { | |
| 79 | 105 | return "", err | |
| 80 | 106 | } | |
| 107 | + | if p, err := g.changedByNewCommits(ctx, dir, extra, kept, types); p != "" || err != nil { | |
| 108 | + | return p, err | |
| 109 | + | } | |
| 81 | 110 | trees, err := g.batchCheck(ctx, dir, extra, kept, ":"+strings.TrimSuffix(workflowsDir, "/"), "%(objectmode) %(objectname) %(objecttype)") | |
| 82 | 111 | if err != nil { | |
| 83 | 112 | return "", err | |
| 84 | 113 | } | |
| 85 | - | for i := range kept { | |
| 114 | + | headW, headRead := "", false | |
| 115 | + | for i, up := range kept { | |
| 86 | 116 | if types[2*i+1] == "blob" { | |
| 87 | 117 | continue // a blob holds no workflow, whatever it replaces | |
| 88 | 118 | } | |
| 89 | - | oldW, newW := workflowsOf(types[2*i], trees[2*i]), workflowsOf(types[2*i+1], trees[2*i+1]) | |
| 90 | - | if oldW == newW || newW == "" { | |
| 119 | + | base := workflowsOf(types[2*i], trees[2*i]) | |
| 120 | + | if up.IsCreate() { | |
| 121 | + | if !headRead { | |
| 122 | + | w, err := g.workflowsAt(ctx, dir, extra, []string{"HEAD:" + strings.TrimSuffix(workflowsDir, "/")}) | |
| 123 | + | if err != nil { | |
| 124 | + | return "", err | |
| 125 | + | } | |
| 126 | + | headW, headRead = w["HEAD:"+strings.TrimSuffix(workflowsDir, "/")], true | |
| 127 | + | } | |
| 128 | + | base = headW // a new ref may end with the workflows of the default branch | |
| 129 | + | } | |
| 130 | + | newW := workflowsOf(types[2*i+1], trees[2*i+1]) | |
| 131 | + | if base == newW || newW == "" { | |
| 91 | 132 | continue // the same directory, or none left | |
| 92 | 133 | } | |
| 93 | - | return g.firstChanged(ctx, dir, extra, oldW, newW) | |
| 134 | + | return g.firstChanged(ctx, dir, extra, base, newW) | |
| 135 | + | } | |
| 136 | + | return "", nil | |
| 137 | + | } | |
| 138 | + | ||
| 139 | + | // changedByNewCommits judges the commits the updates bring (rule 1 of | |
| 140 | + | // ChangedWorkflow). | |
| 141 | + | func (g *Git) changedByNewCommits(ctx context.Context, dir string, extra []string, ups []domain.RefUpdate, types []string) (string, error) { | |
| 142 | + | var tips []string | |
| 143 | + | seen := map[string]bool{} | |
| 144 | + | for i, up := range ups { | |
| 145 | + | if types[2*i+1] == "commit" && !seen[up.New] { | |
| 146 | + | seen[up.New] = true | |
| 147 | + | tips = append(tips, up.New) | |
| 148 | + | } | |
| 149 | + | } | |
| 150 | + | if len(tips) == 0 { | |
| 151 | + | return "", nil | |
| 152 | + | } | |
| 153 | + | commits, err := g.newCommits(ctx, dir, extra, tips) | |
| 154 | + | if err != nil { | |
| 155 | + | return "", err | |
| 156 | + | } | |
| 157 | + | const suffix = ":.github/workflows" | |
| 158 | + | var specs []string | |
| 159 | + | asked := map[string]bool{} | |
| 160 | + | for _, c := range commits { | |
| 161 | + | for _, id := range append([]string{c.id}, c.parents...) { | |
| 162 | + | if !asked[id] { | |
| 163 | + | asked[id] = true | |
| 164 | + | specs = append(specs, id+suffix) | |
| 165 | + | } | |
| 166 | + | } | |
| 167 | + | } | |
| 168 | + | ws, err := g.workflowsAt(ctx, dir, extra, specs) | |
| 169 | + | if err != nil { | |
| 170 | + | return "", err | |
| 171 | + | } | |
| 172 | + | for _, c := range commits { | |
| 173 | + | cur := ws[c.id+suffix] | |
| 174 | + | if cur == "" { | |
| 175 | + | continue // none: nothing new is active | |
| 176 | + | } | |
| 177 | + | first, same := "", false | |
| 178 | + | for i, p := range c.parents { | |
| 179 | + | if i == 0 { | |
| 180 | + | first = ws[p+suffix] | |
| 181 | + | } | |
| 182 | + | if ws[p+suffix] == cur { | |
| 183 | + | same = true | |
| 184 | + | } | |
| 185 | + | } | |
| 186 | + | if !same { | |
| 187 | + | return g.firstChanged(ctx, dir, extra, first, cur) | |
| 188 | + | } | |
| 94 | 189 | } | |
| 95 | 190 | return "", nil | |
| 96 | 191 | } | |
| 97 | 192 | ||
| 193 | + | type pushedCommit struct { | |
| 194 | + | id string | |
| 195 | + | parents []string | |
| 196 | + | } | |
| 197 | + | ||
| 198 | + | // newCommits lists the commits reachable from the tips and from no ref | |
| 199 | + | // (`rev-list --parents <tips> --not --all`), at most maxPushedCommits: more | |
| 200 | + | // fails closed. | |
| 201 | + | func (g *Git) newCommits(ctx context.Context, dir string, extra []string, tips []string) ([]pushedCommit, error) { | |
| 202 | + | args := readArgs(dir, append(append([]string{"rev-list", "--parents", "--max-count=" + strconv.Itoa(maxPushedCommits+1)}, tips...), "--not", "--all")...) | |
| 203 | + | var out []pushedCommit | |
| 204 | + | err := g.pipeWith(ctx, extra, nil, args, func(r *bufio.Reader) error { | |
| 205 | + | var err error | |
| 206 | + | out, err = parseRevList(r, maxPushedCommits, maxPushedObjects) | |
| 207 | + | return err | |
| 208 | + | }) | |
| 209 | + | if err != nil { | |
| 210 | + | return nil, fmt.Errorf("commits of a push: %w", err) | |
| 211 | + | } | |
| 212 | + | return out, nil | |
| 213 | + | } | |
| 214 | + | ||
| 215 | + | // parseRevList reads the lines of `rev-list --parents` ("<commit> <parent>..."): | |
| 216 | + | // at most maxCommits lines and maxIDs ids in all (a commit and each of its | |
| 217 | + | // parents count, repeats too). A line over maxRevListLine, a limit crossed or an | |
| 218 | + | // id that is not one is an error, and it is raised as soon as the line that | |
| 219 | + | // crosses it is read: nothing past it is read or kept. | |
| 220 | + | func parseRevList(r *bufio.Reader, maxCommits, maxIDs int) ([]pushedCommit, error) { | |
| 221 | + | var out []pushedCommit | |
| 222 | + | ids := 0 | |
| 223 | + | for { | |
| 224 | + | line, err := r.ReadSlice('\n') | |
| 225 | + | switch { | |
| 226 | + | case errors.Is(err, bufio.ErrBufferFull) || len(line) > maxRevListLine: | |
| 227 | + | return nil, fmt.Errorf("%w: a pushed commit has too many parents", domain.ErrInvalid) | |
| 228 | + | case errors.Is(err, io.EOF) && len(line) == 0: | |
| 229 | + | return out, nil | |
| 230 | + | case err != nil && !errors.Is(err, io.EOF): | |
| 231 | + | return nil, err | |
| 232 | + | } | |
| 233 | + | f := strings.Fields(string(line)) | |
| 234 | + | if len(f) == 0 { | |
| 235 | + | continue | |
| 236 | + | } | |
| 237 | + | if len(out) == maxCommits { | |
| 238 | + | return nil, fmt.Errorf("%w: the push brings more than %d commits", domain.ErrInvalid, maxCommits) | |
| 239 | + | } | |
| 240 | + | if ids += len(f); ids > maxIDs { | |
| 241 | + | return nil, fmt.Errorf("%w: the push brings commits with more than %d ids (commits and parents) in all", domain.ErrInvalid, maxIDs) | |
| 242 | + | } | |
| 243 | + | for _, id := range f { | |
| 244 | + | if _, err := domain.ParseObjectID(id); err != nil { | |
| 245 | + | return nil, fmt.Errorf("%w: unexpected rev-list output", domain.ErrInvalid) | |
| 246 | + | } | |
| 247 | + | } | |
| 248 | + | out = append(out, pushedCommit{id: f[0], parents: f[1:]}) | |
| 249 | + | } | |
| 250 | + | } | |
| 251 | + | ||
| 252 | + | // workflowsAt returns, for each spec ("<rev>:.github/workflows"), the id of the | |
| 253 | + | // directory it names, or "" when there is none (see workflowsOf). | |
| 254 | + | func (g *Git) workflowsAt(ctx context.Context, dir string, extra []string, specs []string) (map[string]string, error) { | |
| 255 | + | out := make(map[string]string, len(specs)) | |
| 256 | + | if len(specs) == 0 { | |
| 257 | + | return out, nil | |
| 258 | + | } | |
| 259 | + | args := readArgs(dir, "cat-file", "--batch-check=%(objectmode) %(objectname) %(objecttype)") | |
| 260 | + | err := g.pipeWith(ctx, extra, strings.NewReader(strings.Join(specs, "\n")+"\n"), args, func(r *bufio.Reader) error { | |
| 261 | + | for _, spec := range specs { | |
| 262 | + | line, err := r.ReadSlice('\n') | |
| 263 | + | if err != nil { | |
| 264 | + | return fmt.Errorf("read object answers: %w", err) | |
| 265 | + | } | |
| 266 | + | out[spec] = workflowsOf("", strings.TrimSuffix(string(line), "\n")) | |
| 267 | + | } | |
| 268 | + | return nil | |
| 269 | + | }) | |
| 270 | + | if err != nil { | |
| 271 | + | return nil, fmt.Errorf("workflows of a push: %w", err) | |
| 272 | + | } | |
| 273 | + | return out, nil | |
| 274 | + | } | |
| 275 | + | ||
| 98 | 276 | // workflowsOf is the id of the tree that is the .github/workflows of a commit | |
| 99 | 277 | // or a tree, "" when there is none: the answer for "<it>:.github/workflows" must | |
| 100 | 278 | // be a tree of mode 040000, and the object peeled to a tree-ish, not a blob. | |
| @@ -7,7 +7,9 @@ import ( | |||||
| 7 | "context" | 7 | "context" | ||
| 8 | "errors" | 8 | "errors" | ||
| 9 | "fmt" | 9 | "fmt" | ||
| 10 | + | "io" | |||
| 10 | "path/filepath" | 11 | "path/filepath" | ||
| 12 | + | "strconv" | |||
| 11 | "strings" | 13 | "strings" | ||
| 12 | 14 | ||||
| 13 | "bnema.dev/ermite/internal/domain" | 15 | "bnema.dev/ermite/internal/domain" | ||
| @@ -23,36 +25,60 @@ const workflowsDir = ".github/workflows/" | |||||
| 23 | // fails closed (the push is refused), whatever the trees hold. | 25 | // fails closed (the push is refused), whatever the trees hold. | ||
| 24 | const maxWorkflowPath = 4096 | 26 | const maxWorkflowPath = 4096 | ||
| 25 | 27 | ||||
| 26 | - | // ChangedWorkflow tells whether a push creates or changes a file under | 28 | + | // Bounds of the walk over the commits a push brings: more than that, or a |
| 27 | - | // .github/workflows/. For each update that does not delete its ref, the | 29 | + | // commit with more parents than a line of rev-list holds, fails closed (the |
| 28 | - | // directory .github/workflows of the old commit (none for a new ref) is | 30 | + | // push is refused), whatever the commits hold. maxPushedObjects counts every |
| 29 | - | // compared with the one of the new commit: the same tree id is the same | 31 | + | // commit and every parent listed, repeats included (so it bounds the distinct |
| 30 | - | // content, whatever it holds, so nothing is read; a different one, or one that | 32 | + | // ids too), and is enforced as the lines are read, before anything more is |
| 31 | - | // appears, is a change, and the first top-level entry that differs names it. A | 33 | + | // kept: a few thousand commits with two hundred parents each are refused after |
| 32 | - | // new ref is thus compared with nothing: any workflow it carries counts, as on | 34 | + | // the first lines, not after they have been held. |
| 33 | - | // GitHub. A directory that disappears is not a change (as a deleted ref is | 35 | + | const ( |
| 34 | - | // not). | 36 | + | maxPushedCommits = 10000 |
| 37 | + | maxPushedObjects = 4 * maxPushedCommits | |||
| 38 | + | maxRevListLine = 8192 | |||
| 39 | + | ) | |||
| 40 | + | ||||
| 41 | + | // ChangedWorkflow tells whether a push changes the workflows of a repository. | |||
| 42 | + | // It returns the path of the first change, "" for none. Deleting a ref changes | |||
| 43 | + | // nothing, and a workflows directory that disappears is not a change. | |||
| 44 | + | // | |||
| 45 | + | // Two things are judged, as GitHub does for the token of a job (it refuses a | |||
| 46 | + | // push when the commits pushed change workflow files, not when a new branch is | |||
| 47 | + | // cut from a commit that has them): | |||
| 48 | + | // | |||
| 49 | + | // 1. Every commit the push brings (`rev-list <new> --not --all`, so the commits | |||
| 50 | + | // no ref has yet) must leave .github/workflows as it found it: it is | |||
| 51 | + | // compared with each of its parents (a root commit with nothing) and is a | |||
| 52 | + | // change when it is present and differs from every one of them. A merge of | |||
| 53 | + | // existing commits is thus no change; a side that brings a change is itself | |||
| 54 | + | // a new commit, and judged as one. | |||
| 55 | + | // 2. The directory a ref ends with must not be one it did not have: for an | |||
| 56 | + | // update the final .github/workflows must be the old one or absent, for a | |||
| 57 | + | // new ref the one of the default branch (HEAD) or absent. This keeps a job | |||
| 58 | + | // token from moving a ref onto existing commits that carry other workflows | |||
| 59 | + | // (an older commit, a commit of a branch with different ones). | |||
| 35 | // | 60 | // | ||
| 36 | - | // What counts as the directory is exactly what the workflow loader | 61 | + | // Presence is the directory the workflow loader lists (CI.workflowFiles, on both |
| 37 | - | // (CI.workflowFiles, on both git readers) lists: .github and workflows both | 62 | + | // git readers): .github and workflows both entries of mode 040000 and type |
| 38 | - | // entries of mode 040000 and type tree. A path through a gitlink (160000), a | 63 | + | // tree. A path through a gitlink (160000), a symbolic link (120000, which the |
| 39 | - | // symbolic link (120000, which the loader does not follow) or a file is no | 64 | + | // loader does not follow) or a file is no workflows, so the same tree id that is |
| 40 | - | // workflows, so the same tree id that is no workflows as a gitlink and becomes | 65 | + | // no workflows as a gitlink and becomes workflows as a directory is a change, |
| 41 | - | // workflows as a directory is a change, which the object id alone would not | 66 | + | // which the object id alone would not tell. Mode and id of |
| 42 | - | // tell (`cat-file` mode and type of "<commit>:.github/workflows"). | 67 | + | // "<commit>:.github/workflows" are what is compared: equal means the same |
| 68 | + | // content, whatever it holds, so nothing is read. | |||
| 43 | // | 69 | // | ||
| 44 | - | // The comparison never walks the trees recursively: the trees of a push are the | 70 | + | // Nothing is walked recursively: the trees of a push are the pusher's, and a |
| 45 | - | // pusher's, and a recursive diff of trees that share subtrees fans out into | 71 | + | // recursive diff of trees that share subtrees fans out into more paths than the |
| 46 | - | // more paths than the push has bytes (git builds the whole diff before it | 72 | + | // push has bytes (git builds the whole diff before it prints a line, so reading |
| 47 | - | // prints a line, so reading it as a stream would not help). The shallow diff | 73 | + | // it as a stream would not help). The shallow diff that names the path reads one |
| 48 | - | // that names the path reads one directory, as large as its tree object, as a | 74 | + | // directory, as large as its tree object, as a stream, up to its first name, and |
| 49 | - | // stream, up to its first path, and git is stopped and reaped then; a path | 75 | + | // git is stopped and reaped then; a name longer than maxWorkflowPath fails |
| 50 | - | // longer than maxWorkflowPath fails closed. | 76 | + | // closed. |
| 51 | // | 77 | // | ||
| 52 | - | // Both ends are peeled first (`cat-file --batch-check` on `<oid>^{}`): a | 78 | + | // The ends of each update are peeled first (`cat-file --batch-check` on |
| 53 | - | // commit or a tree is compared, a blob (a tag may point to one) holds no | 79 | + | // `<oid>^{}`): a commit or a tree is compared, a blob (a tag may point to one) |
| 54 | - | // workflows; a missing or malformed object fails closed. Git reads the objects | 80 | + | // holds no workflows; a missing or malformed object fails closed. Git reads the |
| 55 | - | // of a push still in quarantine through the environment of the hook | 81 | + | // objects of a push still in quarantine through the environment of the hook |
| 56 | // (ports.PushObjects), accepted only inside the repository's objects directory. | 82 | // (ports.PushObjects), accepted only inside the repository's objects directory. | ||
| 57 | // | 83 | // | ||
| 58 | //hexcheck:ignore suspicious-business-logic-in-adapter reads the object ids and the path git prints for the compared trees; the rule that refuses is the use case's | 84 | //hexcheck:ignore suspicious-business-logic-in-adapter reads the object ids and the path git prints for the compared trees; the rule that refuses is the use case's | ||
| @@ -78,23 +104,175 @@ func (g *Git) ChangedWorkflow(ctx context.Context, repo domain.RepoName, updates | |||||
| 78 | if err != nil { | 104 | if err != nil { | ||
| 79 | return "", err | 105 | return "", err | ||
| 80 | } | 106 | } | ||
| 107 | + | if p, err := g.changedByNewCommits(ctx, dir, extra, kept, types); p != "" || err != nil { | |||
| 108 | + | return p, err | |||
| 109 | + | } | |||
| 81 | trees, err := g.batchCheck(ctx, dir, extra, kept, ":"+strings.TrimSuffix(workflowsDir, "/"), "%(objectmode) %(objectname) %(objecttype)") | 110 | trees, err := g.batchCheck(ctx, dir, extra, kept, ":"+strings.TrimSuffix(workflowsDir, "/"), "%(objectmode) %(objectname) %(objecttype)") | ||
| 82 | if err != nil { | 111 | if err != nil { | ||
| 83 | return "", err | 112 | return "", err | ||
| 84 | } | 113 | } | ||
| 85 | - | for i := range kept { | 114 | + | headW, headRead := "", false |
| 115 | + | for i, up := range kept { | |||
| 86 | if types[2*i+1] == "blob" { | 116 | if types[2*i+1] == "blob" { | ||
| 87 | continue // a blob holds no workflow, whatever it replaces | 117 | continue // a blob holds no workflow, whatever it replaces | ||
| 88 | } | 118 | } | ||
| 89 | - | oldW, newW := workflowsOf(types[2*i], trees[2*i]), workflowsOf(types[2*i+1], trees[2*i+1]) | 119 | + | base := workflowsOf(types[2*i], trees[2*i]) |
| 90 | - | if oldW == newW || newW == "" { | 120 | + | if up.IsCreate() { |
| 121 | + | if !headRead { | |||
| 122 | + | w, err := g.workflowsAt(ctx, dir, extra, []string{"HEAD:" + strings.TrimSuffix(workflowsDir, "/")}) | |||
| 123 | + | if err != nil { | |||
| 124 | + | return "", err | |||
| 125 | + | } | |||
| 126 | + | headW, headRead = w["HEAD:"+strings.TrimSuffix(workflowsDir, "/")], true | |||
| 127 | + | } | |||
| 128 | + | base = headW // a new ref may end with the workflows of the default branch | |||
| 129 | + | } | |||
| 130 | + | newW := workflowsOf(types[2*i+1], trees[2*i+1]) | |||
| 131 | + | if base == newW || newW == "" { | |||
| 91 | continue // the same directory, or none left | 132 | continue // the same directory, or none left | ||
| 92 | } | 133 | } | ||
| 93 | - | return g.firstChanged(ctx, dir, extra, oldW, newW) | 134 | + | return g.firstChanged(ctx, dir, extra, base, newW) |
| 135 | + | } | |||
| 136 | + | return "", nil | |||
| 137 | + | } | |||
| 138 | + | ||||
| 139 | + | // changedByNewCommits judges the commits the updates bring (rule 1 of | |||
| 140 | + | // ChangedWorkflow). | |||
| 141 | + | func (g *Git) changedByNewCommits(ctx context.Context, dir string, extra []string, ups []domain.RefUpdate, types []string) (string, error) { | |||
| 142 | + | var tips []string | |||
| 143 | + | seen := map[string]bool{} | |||
| 144 | + | for i, up := range ups { | |||
| 145 | + | if types[2*i+1] == "commit" && !seen[up.New] { | |||
| 146 | + | seen[up.New] = true | |||
| 147 | + | tips = append(tips, up.New) | |||
| 148 | + | } | |||
| 149 | + | } | |||
| 150 | + | if len(tips) == 0 { | |||
| 151 | + | return "", nil | |||
| 152 | + | } | |||
| 153 | + | commits, err := g.newCommits(ctx, dir, extra, tips) | |||
| 154 | + | if err != nil { | |||
| 155 | + | return "", err | |||
| 156 | + | } | |||
| 157 | + | const suffix = ":.github/workflows" | |||
| 158 | + | var specs []string | |||
| 159 | + | asked := map[string]bool{} | |||
| 160 | + | for _, c := range commits { | |||
| 161 | + | for _, id := range append([]string{c.id}, c.parents...) { | |||
| 162 | + | if !asked[id] { | |||
| 163 | + | asked[id] = true | |||
| 164 | + | specs = append(specs, id+suffix) | |||
| 165 | + | } | |||
| 166 | + | } | |||
| 167 | + | } | |||
| 168 | + | ws, err := g.workflowsAt(ctx, dir, extra, specs) | |||
| 169 | + | if err != nil { | |||
| 170 | + | return "", err | |||
| 171 | + | } | |||
| 172 | + | for _, c := range commits { | |||
| 173 | + | cur := ws[c.id+suffix] | |||
| 174 | + | if cur == "" { | |||
| 175 | + | continue // none: nothing new is active | |||
| 176 | + | } | |||
| 177 | + | first, same := "", false | |||
| 178 | + | for i, p := range c.parents { | |||
| 179 | + | if i == 0 { | |||
| 180 | + | first = ws[p+suffix] | |||
| 181 | + | } | |||
| 182 | + | if ws[p+suffix] == cur { | |||
| 183 | + | same = true | |||
| 184 | + | } | |||
| 185 | + | } | |||
| 186 | + | if !same { | |||
| 187 | + | return g.firstChanged(ctx, dir, extra, first, cur) | |||
| 188 | + | } | |||
| 94 | } | 189 | } | ||
| 95 | return "", nil | 190 | return "", nil | ||
| 96 | } | 191 | } | ||
| 97 | 192 | ||||
| 193 | + | type pushedCommit struct { | |||
| 194 | + | id string | |||
| 195 | + | parents []string | |||
| 196 | + | } | |||
| 197 | + | ||||
| 198 | + | // newCommits lists the commits reachable from the tips and from no ref | |||
| 199 | + | // (`rev-list --parents <tips> --not --all`), at most maxPushedCommits: more | |||
| 200 | + | // fails closed. | |||
| 201 | + | func (g *Git) newCommits(ctx context.Context, dir string, extra []string, tips []string) ([]pushedCommit, error) { | |||
| 202 | + | args := readArgs(dir, append(append([]string{"rev-list", "--parents", "--max-count=" + strconv.Itoa(maxPushedCommits+1)}, tips...), "--not", "--all")...) | |||
| 203 | + | var out []pushedCommit | |||
| 204 | + | err := g.pipeWith(ctx, extra, nil, args, func(r *bufio.Reader) error { | |||
| 205 | + | var err error | |||
| 206 | + | out, err = parseRevList(r, maxPushedCommits, maxPushedObjects) | |||
| 207 | + | return err | |||
| 208 | + | }) | |||
| 209 | + | if err != nil { | |||
| 210 | + | return nil, fmt.Errorf("commits of a push: %w", err) | |||
| 211 | + | } | |||
| 212 | + | return out, nil | |||
| 213 | + | } | |||
| 214 | + | ||||
| 215 | + | // parseRevList reads the lines of `rev-list --parents` ("<commit> <parent>..."): | |||
| 216 | + | // at most maxCommits lines and maxIDs ids in all (a commit and each of its | |||
| 217 | + | // parents count, repeats too). A line over maxRevListLine, a limit crossed or an | |||
| 218 | + | // id that is not one is an error, and it is raised as soon as the line that | |||
| 219 | + | // crosses it is read: nothing past it is read or kept. | |||
| 220 | + | func parseRevList(r *bufio.Reader, maxCommits, maxIDs int) ([]pushedCommit, error) { | |||
| 221 | + | var out []pushedCommit | |||
| 222 | + | ids := 0 | |||
| 223 | + | for { | |||
| 224 | + | line, err := r.ReadSlice('\n') | |||
| 225 | + | switch { | |||
| 226 | + | case errors.Is(err, bufio.ErrBufferFull) || len(line) > maxRevListLine: | |||
| 227 | + | return nil, fmt.Errorf("%w: a pushed commit has too many parents", domain.ErrInvalid) | |||
| 228 | + | case errors.Is(err, io.EOF) && len(line) == 0: | |||
| 229 | + | return out, nil | |||
| 230 | + | case err != nil && !errors.Is(err, io.EOF): | |||
| 231 | + | return nil, err | |||
| 232 | + | } | |||
| 233 | + | f := strings.Fields(string(line)) | |||
| 234 | + | if len(f) == 0 { | |||
| 235 | + | continue | |||
| 236 | + | } | |||
| 237 | + | if len(out) == maxCommits { | |||
| 238 | + | return nil, fmt.Errorf("%w: the push brings more than %d commits", domain.ErrInvalid, maxCommits) | |||
| 239 | + | } | |||
| 240 | + | if ids += len(f); ids > maxIDs { | |||
| 241 | + | return nil, fmt.Errorf("%w: the push brings commits with more than %d ids (commits and parents) in all", domain.ErrInvalid, maxIDs) | |||
| 242 | + | } | |||
| 243 | + | for _, id := range f { | |||
| 244 | + | if _, err := domain.ParseObjectID(id); err != nil { | |||
| 245 | + | return nil, fmt.Errorf("%w: unexpected rev-list output", domain.ErrInvalid) | |||
| 246 | + | } | |||
| 247 | + | } | |||
| 248 | + | out = append(out, pushedCommit{id: f[0], parents: f[1:]}) | |||
| 249 | + | } | |||
| 250 | + | } | |||
| 251 | + | ||||
| 252 | + | // workflowsAt returns, for each spec ("<rev>:.github/workflows"), the id of the | |||
| 253 | + | // directory it names, or "" when there is none (see workflowsOf). | |||
| 254 | + | func (g *Git) workflowsAt(ctx context.Context, dir string, extra []string, specs []string) (map[string]string, error) { | |||
| 255 | + | out := make(map[string]string, len(specs)) | |||
| 256 | + | if len(specs) == 0 { | |||
| 257 | + | return out, nil | |||
| 258 | + | } | |||
| 259 | + | args := readArgs(dir, "cat-file", "--batch-check=%(objectmode) %(objectname) %(objecttype)") | |||
| 260 | + | err := g.pipeWith(ctx, extra, strings.NewReader(strings.Join(specs, "\n")+"\n"), args, func(r *bufio.Reader) error { | |||
| 261 | + | for _, spec := range specs { | |||
| 262 | + | line, err := r.ReadSlice('\n') | |||
| 263 | + | if err != nil { | |||
| 264 | + | return fmt.Errorf("read object answers: %w", err) | |||
| 265 | + | } | |||
| 266 | + | out[spec] = workflowsOf("", strings.TrimSuffix(string(line), "\n")) | |||
| 267 | + | } | |||
| 268 | + | return nil | |||
| 269 | + | }) | |||
| 270 | + | if err != nil { | |||
| 271 | + | return nil, fmt.Errorf("workflows of a push: %w", err) | |||
| 272 | + | } | |||
| 273 | + | return out, nil | |||
| 274 | + | } | |||
| 275 | + | ||||
| 98 | // workflowsOf is the id of the tree that is the .github/workflows of a commit | 276 | // workflowsOf is the id of the tree that is the .github/workflows of a commit | ||
| 99 | // or a tree, "" when there is none: the answer for "<it>:.github/workflows" must | 277 | // or a tree, "" when there is none: the answer for "<it>:.github/workflows" must | ||
| 100 | // be a tree of mode 040000, and the object peeled to a tree-ish, not a blob. | 278 | // be a tree of mode 040000, and the object peeled to a tree-ish, not a blob. | ||
M internal/adapters/gitcli/workflow_guard_test.go +217 -0
| @@ -6,6 +6,7 @@ import ( | |||
| 6 | 6 | "context" | |
| 7 | 7 | "encoding/hex" | |
| 8 | 8 | "fmt" | |
| 9 | + | "io" | |
| 9 | 10 | "os" | |
| 10 | 11 | "os/exec" | |
| 11 | 12 | "path/filepath" | |
| @@ -246,6 +247,8 @@ func TestChangedWorkflowPeelsAndClassifiesBothEnds(t *testing.T) { | |||
| 246 | 247 | plain := f.Commit("plain") | |
| 247 | 248 | f.Write(".github/workflows/ci.yml", "on: push\n") | |
| 248 | 249 | wf := f.Commit("workflow") | |
| 250 | + | f.Write(".github/workflows/ci.yml", "on: [push]\n") | |
| 251 | + | f.Commit("the default branch moves on to other workflows") // HEAD is not wf | |
| 249 | 252 | blob := f.WriteObject("blob", "hello\n") | |
| 250 | 253 | annotatedBlob := f.WriteObject("tag", "object "+blob+"\ntype blob\ntag b\ntagger a <a@b> 1 +0000\n\nm\n") | |
| 251 | 254 | annotatedWF := f.WriteObject("tag", "object "+oidOf(t, wf).String()+"\ntype commit\ntag c\ntagger a <a@b> 1 +0000\n\nm\n") | |
| @@ -373,3 +376,217 @@ func TestChangedWorkflowComparesTheModeOfTheEntries(t *testing.T) { | |||
| 373 | 376 | require.NoError(t, err) | |
| 374 | 377 | assert.Equal(t, ".github/workflows/ci.yml", p) | |
| 375 | 378 | } | |
| 379 | + | ||
| 380 | + | // A history for the rules on the commits a push brings: main holds workflows A, | |
| 381 | + | // old (a branch) holds B, and the commits made here are in no ref yet, like | |
| 382 | + | // those of a push in quarantine. | |
| 383 | + | type guardHistory struct { | |
| 384 | + | t *testing.T | |
| 385 | + | f *readertest.Fixture | |
| 386 | + | trees map[string]string // workflows name -> tree id | |
| 387 | + | m, o string // main (A) and old (B) | |
| 388 | + | extraN int | |
| 389 | + | readme string | |
| 390 | + | } | |
| 391 | + | ||
| 392 | + | func newGuardHistory(t *testing.T, g *gitcli.Git, data string) *guardHistory { | |
| 393 | + | t.Helper() | |
| 394 | + | f := readertest.NewFixture(t, data, "proj") | |
| 395 | + | h := &guardHistory{t: t, f: f, trees: map[string]string{}} | |
| 396 | + | h.readme = f.WriteObject("blob", "r\n") | |
| 397 | + | for _, n := range []string{"A", "B", "C", "D"} { | |
| 398 | + | h.trees[n] = rawTree(t, f, [3]string{"100644", "ci.yml", f.WriteObject("blob", "on: push # "+n+"\n")}) | |
| 399 | + | } | |
| 400 | + | h.m = h.commit("A", "") | |
| 401 | + | h.o = h.commit("B", "") | |
| 402 | + | f.Git("update-ref", "refs/heads/main", h.m) | |
| 403 | + | f.Git("update-ref", "refs/heads/old", h.o) | |
| 404 | + | return h | |
| 405 | + | } | |
| 406 | + | ||
| 407 | + | // commit makes a commit with the workflows named ("" for none), one more file | |
| 408 | + | // each time (so that no two commits share a tree by accident), and the parents. | |
| 409 | + | func (h *guardHistory) commit(workflows, _ string, parents ...string) string { | |
| 410 | + | h.t.Helper() | |
| 411 | + | h.extraN++ | |
| 412 | + | extra := h.f.WriteObject("blob", fmt.Sprintf("file %d\n", h.extraN)) | |
| 413 | + | entries := [][3]string{{"100644", "README", h.readme}, {"100644", fmt.Sprintf("x%d", h.extraN), extra}} | |
| 414 | + | if workflows != "" { | |
| 415 | + | gh := rawTree(h.t, h.f, [3]string{"040000", "workflows", h.trees[workflows]}) | |
| 416 | + | entries = append([][3]string{{"040000", ".github", gh}}, entries...) | |
| 417 | + | } | |
| 418 | + | return commitOf(h.t, h.f, rawTree(h.t, h.f, entries...), parents...) | |
| 419 | + | } | |
| 420 | + | ||
| 421 | + | func TestChangedWorkflowJudgesThePushedCommits(t *testing.T) { | |
| 422 | + | g, data := newGit(t) | |
| 423 | + | ctx := context.Background() | |
| 424 | + | h := newGuardHistory(t, g, data) | |
| 425 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |
| 426 | + | f := h.f | |
| 427 | + | run := func(ups ...domain.RefUpdate) (string, error) { | |
| 428 | + | return g.ChangedWorkflow(ctx, f.Name, ups, ports.PushObjects{}) | |
| 429 | + | } | |
| 430 | + | create := func(ref, to string) domain.RefUpdate { return domain.RefUpdate{Old: domain.ZeroOID, New: to, Ref: ref} } | |
| 431 | + | move := func(ref, from, to string) domain.RefUpdate { return domain.RefUpdate{Old: from, New: to, Ref: ref} } | |
| 432 | + | const ci = ".github/workflows/ci.yml" | |
| 433 | + | ||
| 434 | + | allowed := func(name string, ups ...domain.RefUpdate) { | |
| 435 | + | t.Helper() | |
| 436 | + | p, err := run(ups...) | |
| 437 | + | require.NoError(t, err, name) | |
| 438 | + | assert.Empty(t, p, name) | |
| 439 | + | } | |
| 440 | + | refused := func(name string, ups ...domain.RefUpdate) { | |
| 441 | + | t.Helper() | |
| 442 | + | p, err := run(ups...) | |
| 443 | + | require.NoError(t, err, name) | |
| 444 | + | assert.Equal(t, ci, p, name) | |
| 445 | + | } | |
| 446 | + | ||
| 447 | + | // The staging repro: a new branch off main with a commit that touches no workflow. | |
| 448 | + | allowed("new branch off main with a plain commit", create("refs/heads/smoke", h.commit("A", "", h.m))) | |
| 449 | + | allowed("a new branch at the head of main", create("refs/heads/same", h.m)) | |
| 450 | + | allowed("a new tag at the head of main", create("refs/tags/v1", h.m)) | |
| 451 | + | allowed("a new branch whose commits carry no workflows at all", create("refs/heads/bare", h.commit("", ""))) | |
| 452 | + | allowed("a fast-forward with plain commits", move("refs/heads/main", h.m, h.commit("A", "", h.commit("A", "", h.m)))) | |
| 453 | + | allowed("a commit that removes the workflows", move("refs/heads/main", h.m, h.commit("", "", h.m))) | |
| 454 | + | ||
| 455 | + | // A pushed commit that changes them is refused wherever it goes. | |
| 456 | + | refused("new branch with a commit that changes workflows", create("refs/heads/w", h.commit("C", "", h.m))) | |
| 457 | + | refused("a root commit that carries workflows", create("refs/heads/root", h.commit("C", ""))) | |
| 458 | + | refused("an update with a commit that changes workflows", move("refs/heads/main", h.m, h.commit("C", "", h.m))) | |
| 459 | + | n4 := h.commit("C", "", h.m) | |
| 460 | + | n5 := h.commit("A", "", n4) // reverts the change of the commit before | |
| 461 | + | refused("a fast-forward through a commit that changes them and a later one that reverts it", move("refs/heads/main", h.m, n5)) | |
| 462 | + | refused("a new branch through the same", create("refs/heads/through", n5)) | |
| 463 | + | n6 := h.commit("A", "", h.commit("C", "", h.o)) | |
| 464 | + | refused("a commit that changes them below plain ones, on an old base", create("refs/heads/below", h.commit("C", "", n6))) | |
| 465 | + | ||
| 466 | + | // A ref cannot be moved onto existing commits with other workflows. | |
| 467 | + | refused("a new branch at an old commit whose workflows differ from main", create("refs/heads/b", h.o)) | |
| 468 | + | refused("a new tag at that commit", create("refs/tags/old", h.o)) | |
| 469 | + | refused("an update moving back to an older commit with other workflows", move("refs/heads/main", h.m, h.o)) | |
| 470 | + | allowed("an update to a commit with the same workflows", move("refs/heads/old", h.o, h.commit("B", "", h.o))) | |
| 471 | + | allowed("an update that moves a ref to a commit without workflows (removes them)", move("refs/heads/old", h.o, h.commit("", "", h.o))) | |
| 472 | + | ||
| 473 | + | // Merges: a change only if it differs from every parent. | |
| 474 | + | allowed("a merge of existing commits that keeps main's workflows", move("refs/heads/main", h.m, h.commit("A", "", h.m, h.o))) | |
| 475 | + | allowed("a new branch with a merge keeping one parent's workflows", create("refs/heads/merge", h.commit("A", "", h.m, h.o))) | |
| 476 | + | refused("a merge that differs from both parents", create("refs/heads/merge2", h.commit("D", "", h.m, h.o))) | |
| 477 | + | refused("a merge of a pushed side that changes them (the side commit is itself new)", create("refs/heads/merge3", h.commit("A", "", h.m, h.commit("C", "", h.o)))) | |
| 478 | + | // One that takes the old side's workflows is a change of the ref, not of a commit. | |
| 479 | + | refused("a merge taking the other parent's workflows, onto main", move("refs/heads/main", h.m, h.commit("B", "", h.m, h.o))) | |
| 480 | + | ||
| 481 | + | // Several updates: the first refused refuses the push. | |
| 482 | + | refused("a plain ref and a refused one", create("refs/heads/ok", h.commit("A", "", h.m)), create("refs/heads/w2", h.commit("C", "", h.m))) | |
| 483 | + | allowed("deletes", move("refs/heads/old", h.o, domain.ZeroOID)) | |
| 484 | + | } | |
| 485 | + | ||
| 486 | + | // With no default branch head, a new ref is compared with no workflows at all. | |
| 487 | + | func TestChangedWorkflowOfANewRefWithoutADefaultBranchHead(t *testing.T) { | |
| 488 | + | g, data := newGit(t) | |
| 489 | + | ctx := context.Background() | |
| 490 | + | h := newGuardHistory(t, g, data) | |
| 491 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |
| 492 | + | h.f.Git("symbolic-ref", "HEAD", "refs/heads/nothing") // the default branch does not exist | |
| 493 | + | run := func(to string) string { | |
| 494 | + | p, err := g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: domain.ZeroOID, New: to, Ref: "refs/tags/t"}}, ports.PushObjects{}) | |
| 495 | + | require.NoError(t, err) | |
| 496 | + | return p | |
| 497 | + | } | |
| 498 | + | assert.Equal(t, ".github/workflows/ci.yml", run(h.m), "an existing commit with workflows") | |
| 499 | + | plain := h.commit("", "") | |
| 500 | + | h.f.Git("update-ref", "refs/heads/plain", plain) | |
| 501 | + | assert.Empty(t, run(plain), "an existing commit without") | |
| 502 | + | } | |
| 503 | + | ||
| 504 | + | // The commits of a push are bounded: more than the bound fails closed. | |
| 505 | + | func TestChangedWorkflowBoundsThePushedCommits(t *testing.T) { | |
| 506 | + | g, data := newGit(t) | |
| 507 | + | ctx := context.Background() | |
| 508 | + | h := newGuardHistory(t, g, data) | |
| 509 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |
| 510 | + | // A chain of plain commits on main, made with fast-import in a ref that is | |
| 511 | + | // removed afterwards: the objects stay, in no ref, like a push in quarantine. | |
| 512 | + | chain := func(n int) string { | |
| 513 | + | var b strings.Builder | |
| 514 | + | fmt.Fprintf(&b, "reset refs/tmp/chain\nfrom %s\n", h.m) | |
| 515 | + | for i := range n { | |
| 516 | + | fmt.Fprintf(&b, "commit refs/tmp/chain\ncommitter a <a@b> %d +0000\ndata 1\nc\nM 100644 inline f.txt\ndata 2\n%d\n\n", 1000+i, i%10) | |
| 517 | + | } | |
| 518 | + | cmd := exec.Command("git", "--git-dir="+h.f.Bare, "fast-import", "--quiet") | |
| 519 | + | cmd.Stdin = strings.NewReader(b.String()) | |
| 520 | + | out, err := cmd.CombinedOutput() | |
| 521 | + | require.NoError(t, err, string(out)) | |
| 522 | + | tip := h.f.Git("rev-parse", "refs/tmp/chain") | |
| 523 | + | h.f.Git("update-ref", "-d", "refs/tmp/chain") | |
| 524 | + | return tip | |
| 525 | + | } | |
| 526 | + | // Plain commits carry the workflows of main (the fast-import tree has none, | |
| 527 | + | // so the chain is "removing" them): allowed, whatever their number, up to the bound. | |
| 528 | + | p, err := g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: h.m, New: chain(gitcli.MaxPushedCommits), Ref: "refs/heads/main"}}, ports.PushObjects{}) | |
| 529 | + | require.NoError(t, err) | |
| 530 | + | assert.Empty(t, p) | |
| 531 | + | _, err = g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: h.m, New: chain(gitcli.MaxPushedCommits + 1), Ref: "refs/heads/main"}}, ports.PushObjects{}) | |
| 532 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "more commits than the bound refuses the push") | |
| 533 | + | } | |
| 534 | + | ||
| 535 | + | // countingReader serves synthetic rev-list output and counts what was read. | |
| 536 | + | type countingReader struct { | |
| 537 | + | lines, read int | |
| 538 | + | line []byte | |
| 539 | + | pending []byte | |
| 540 | + | made int | |
| 541 | + | } | |
| 542 | + | ||
| 543 | + | func (c *countingReader) Read(p []byte) (int, error) { | |
| 544 | + | if len(c.pending) == 0 { | |
| 545 | + | if c.made == c.lines { | |
| 546 | + | return 0, io.EOF | |
| 547 | + | } | |
| 548 | + | c.made++ | |
| 549 | + | c.pending = c.line | |
| 550 | + | } | |
| 551 | + | n := copy(p, c.pending) | |
| 552 | + | c.pending = c.pending[n:] | |
| 553 | + | c.read += n | |
| 554 | + | return n, nil | |
| 555 | + | } | |
| 556 | + | ||
| 557 | + | // A hostile push: ten thousand commits of two hundred parents each is two | |
| 558 | + | // million ids. The bound is met after the first lines, and nothing more is read | |
| 559 | + | // or kept. | |
| 560 | + | func TestParseRevListRejectsManyParentsEarly(t *testing.T) { | |
| 561 | + | id := func(n int) string { return fmt.Sprintf("%040x", n+1) } | |
| 562 | + | var b strings.Builder | |
| 563 | + | b.WriteString(id(0)) | |
| 564 | + | for i := range 198 { | |
| 565 | + | b.WriteString(" " + id(1+i%3)) // few distinct parents, repeated: the distinct count alone would stay small | |
| 566 | + | } | |
| 567 | + | b.WriteString("\n") | |
| 568 | + | r := &countingReader{lines: gitcli.MaxPushedCommits, line: []byte(b.String())} | |
| 569 | + | kept, err := gitcli.ParseRevList(r, gitcli.MaxPushedCommits, gitcli.MaxPushedObjects) | |
| 570 | + | require.ErrorIs(t, err, domain.ErrInvalid) | |
| 571 | + | assert.Contains(t, err.Error(), "ids (commits and parents)", "refused by the bound on ids, not by the line length") | |
| 572 | + | assert.Zero(t, kept, "nothing is returned with the error") | |
| 573 | + | total := gitcli.MaxPushedCommits * len(b.String()) | |
| 574 | + | assert.Less(t, r.read, 2<<20, "read only the head of %d bytes", total) | |
| 575 | + | assert.Less(t, r.read*20, total) | |
| 576 | + | ||
| 577 | + | // Within the bounds the same lines are kept: 100 commits of 198 parents. | |
| 578 | + | r = &countingReader{lines: 100, line: []byte(b.String())} | |
| 579 | + | kept, err = gitcli.ParseRevList(r, gitcli.MaxPushedCommits, gitcli.MaxPushedObjects) | |
| 580 | + | require.NoError(t, err) | |
| 581 | + | assert.Equal(t, 100, kept) | |
| 582 | + | ||
| 583 | + | // The other bounds and malformed output. | |
| 584 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+"\n"+id(1)+"\n"), 1, 100) | |
| 585 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "commits") | |
| 586 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+" "+id(1)+"\n"+id(2)+"\n"), 10, 2) | |
| 587 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "ids") | |
| 588 | + | _, err = gitcli.ParseRevList(strings.NewReader("nonsense\n"), 10, 10) | |
| 589 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "an id that is none") | |
| 590 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+strings.Repeat(" "+id(1), 300)+"\n"), 10, 1000) | |
| 591 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "a line over the bound") | |
| 592 | + | } | |
| @@ -6,6 +6,7 @@ import ( | |||||
| 6 | "context" | 6 | "context" | ||
| 7 | "encoding/hex" | 7 | "encoding/hex" | ||
| 8 | "fmt" | 8 | "fmt" | ||
| 9 | + | "io" | |||
| 9 | "os" | 10 | "os" | ||
| 10 | "os/exec" | 11 | "os/exec" | ||
| 11 | "path/filepath" | 12 | "path/filepath" | ||
| @@ -246,6 +247,8 @@ func TestChangedWorkflowPeelsAndClassifiesBothEnds(t *testing.T) { | |||||
| 246 | plain := f.Commit("plain") | 247 | plain := f.Commit("plain") | ||
| 247 | f.Write(".github/workflows/ci.yml", "on: push\n") | 248 | f.Write(".github/workflows/ci.yml", "on: push\n") | ||
| 248 | wf := f.Commit("workflow") | 249 | wf := f.Commit("workflow") | ||
| 250 | + | f.Write(".github/workflows/ci.yml", "on: [push]\n") | |||
| 251 | + | f.Commit("the default branch moves on to other workflows") // HEAD is not wf | |||
| 249 | blob := f.WriteObject("blob", "hello\n") | 252 | blob := f.WriteObject("blob", "hello\n") | ||
| 250 | annotatedBlob := f.WriteObject("tag", "object "+blob+"\ntype blob\ntag b\ntagger a <a@b> 1 +0000\n\nm\n") | 253 | annotatedBlob := f.WriteObject("tag", "object "+blob+"\ntype blob\ntag b\ntagger a <a@b> 1 +0000\n\nm\n") | ||
| 251 | annotatedWF := f.WriteObject("tag", "object "+oidOf(t, wf).String()+"\ntype commit\ntag c\ntagger a <a@b> 1 +0000\n\nm\n") | 254 | annotatedWF := f.WriteObject("tag", "object "+oidOf(t, wf).String()+"\ntype commit\ntag c\ntagger a <a@b> 1 +0000\n\nm\n") | ||
| @@ -373,3 +376,217 @@ func TestChangedWorkflowComparesTheModeOfTheEntries(t *testing.T) { | |||||
| 373 | require.NoError(t, err) | 376 | require.NoError(t, err) | ||
| 374 | assert.Equal(t, ".github/workflows/ci.yml", p) | 377 | assert.Equal(t, ".github/workflows/ci.yml", p) | ||
| 375 | } | 378 | } | ||
| 379 | + | ||||
| 380 | + | // A history for the rules on the commits a push brings: main holds workflows A, | |||
| 381 | + | // old (a branch) holds B, and the commits made here are in no ref yet, like | |||
| 382 | + | // those of a push in quarantine. | |||
| 383 | + | type guardHistory struct { | |||
| 384 | + | t *testing.T | |||
| 385 | + | f *readertest.Fixture | |||
| 386 | + | trees map[string]string // workflows name -> tree id | |||
| 387 | + | m, o string // main (A) and old (B) | |||
| 388 | + | extraN int | |||
| 389 | + | readme string | |||
| 390 | + | } | |||
| 391 | + | ||||
| 392 | + | func newGuardHistory(t *testing.T, g *gitcli.Git, data string) *guardHistory { | |||
| 393 | + | t.Helper() | |||
| 394 | + | f := readertest.NewFixture(t, data, "proj") | |||
| 395 | + | h := &guardHistory{t: t, f: f, trees: map[string]string{}} | |||
| 396 | + | h.readme = f.WriteObject("blob", "r\n") | |||
| 397 | + | for _, n := range []string{"A", "B", "C", "D"} { | |||
| 398 | + | h.trees[n] = rawTree(t, f, [3]string{"100644", "ci.yml", f.WriteObject("blob", "on: push # "+n+"\n")}) | |||
| 399 | + | } | |||
| 400 | + | h.m = h.commit("A", "") | |||
| 401 | + | h.o = h.commit("B", "") | |||
| 402 | + | f.Git("update-ref", "refs/heads/main", h.m) | |||
| 403 | + | f.Git("update-ref", "refs/heads/old", h.o) | |||
| 404 | + | return h | |||
| 405 | + | } | |||
| 406 | + | ||||
| 407 | + | // commit makes a commit with the workflows named ("" for none), one more file | |||
| 408 | + | // each time (so that no two commits share a tree by accident), and the parents. | |||
| 409 | + | func (h *guardHistory) commit(workflows, _ string, parents ...string) string { | |||
| 410 | + | h.t.Helper() | |||
| 411 | + | h.extraN++ | |||
| 412 | + | extra := h.f.WriteObject("blob", fmt.Sprintf("file %d\n", h.extraN)) | |||
| 413 | + | entries := [][3]string{{"100644", "README", h.readme}, {"100644", fmt.Sprintf("x%d", h.extraN), extra}} | |||
| 414 | + | if workflows != "" { | |||
| 415 | + | gh := rawTree(h.t, h.f, [3]string{"040000", "workflows", h.trees[workflows]}) | |||
| 416 | + | entries = append([][3]string{{"040000", ".github", gh}}, entries...) | |||
| 417 | + | } | |||
| 418 | + | return commitOf(h.t, h.f, rawTree(h.t, h.f, entries...), parents...) | |||
| 419 | + | } | |||
| 420 | + | ||||
| 421 | + | func TestChangedWorkflowJudgesThePushedCommits(t *testing.T) { | |||
| 422 | + | g, data := newGit(t) | |||
| 423 | + | ctx := context.Background() | |||
| 424 | + | h := newGuardHistory(t, g, data) | |||
| 425 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |||
| 426 | + | f := h.f | |||
| 427 | + | run := func(ups ...domain.RefUpdate) (string, error) { | |||
| 428 | + | return g.ChangedWorkflow(ctx, f.Name, ups, ports.PushObjects{}) | |||
| 429 | + | } | |||
| 430 | + | create := func(ref, to string) domain.RefUpdate { return domain.RefUpdate{Old: domain.ZeroOID, New: to, Ref: ref} } | |||
| 431 | + | move := func(ref, from, to string) domain.RefUpdate { return domain.RefUpdate{Old: from, New: to, Ref: ref} } | |||
| 432 | + | const ci = ".github/workflows/ci.yml" | |||
| 433 | + | ||||
| 434 | + | allowed := func(name string, ups ...domain.RefUpdate) { | |||
| 435 | + | t.Helper() | |||
| 436 | + | p, err := run(ups...) | |||
| 437 | + | require.NoError(t, err, name) | |||
| 438 | + | assert.Empty(t, p, name) | |||
| 439 | + | } | |||
| 440 | + | refused := func(name string, ups ...domain.RefUpdate) { | |||
| 441 | + | t.Helper() | |||
| 442 | + | p, err := run(ups...) | |||
| 443 | + | require.NoError(t, err, name) | |||
| 444 | + | assert.Equal(t, ci, p, name) | |||
| 445 | + | } | |||
| 446 | + | ||||
| 447 | + | // The staging repro: a new branch off main with a commit that touches no workflow. | |||
| 448 | + | allowed("new branch off main with a plain commit", create("refs/heads/smoke", h.commit("A", "", h.m))) | |||
| 449 | + | allowed("a new branch at the head of main", create("refs/heads/same", h.m)) | |||
| 450 | + | allowed("a new tag at the head of main", create("refs/tags/v1", h.m)) | |||
| 451 | + | allowed("a new branch whose commits carry no workflows at all", create("refs/heads/bare", h.commit("", ""))) | |||
| 452 | + | allowed("a fast-forward with plain commits", move("refs/heads/main", h.m, h.commit("A", "", h.commit("A", "", h.m)))) | |||
| 453 | + | allowed("a commit that removes the workflows", move("refs/heads/main", h.m, h.commit("", "", h.m))) | |||
| 454 | + | ||||
| 455 | + | // A pushed commit that changes them is refused wherever it goes. | |||
| 456 | + | refused("new branch with a commit that changes workflows", create("refs/heads/w", h.commit("C", "", h.m))) | |||
| 457 | + | refused("a root commit that carries workflows", create("refs/heads/root", h.commit("C", ""))) | |||
| 458 | + | refused("an update with a commit that changes workflows", move("refs/heads/main", h.m, h.commit("C", "", h.m))) | |||
| 459 | + | n4 := h.commit("C", "", h.m) | |||
| 460 | + | n5 := h.commit("A", "", n4) // reverts the change of the commit before | |||
| 461 | + | refused("a fast-forward through a commit that changes them and a later one that reverts it", move("refs/heads/main", h.m, n5)) | |||
| 462 | + | refused("a new branch through the same", create("refs/heads/through", n5)) | |||
| 463 | + | n6 := h.commit("A", "", h.commit("C", "", h.o)) | |||
| 464 | + | refused("a commit that changes them below plain ones, on an old base", create("refs/heads/below", h.commit("C", "", n6))) | |||
| 465 | + | ||||
| 466 | + | // A ref cannot be moved onto existing commits with other workflows. | |||
| 467 | + | refused("a new branch at an old commit whose workflows differ from main", create("refs/heads/b", h.o)) | |||
| 468 | + | refused("a new tag at that commit", create("refs/tags/old", h.o)) | |||
| 469 | + | refused("an update moving back to an older commit with other workflows", move("refs/heads/main", h.m, h.o)) | |||
| 470 | + | allowed("an update to a commit with the same workflows", move("refs/heads/old", h.o, h.commit("B", "", h.o))) | |||
| 471 | + | allowed("an update that moves a ref to a commit without workflows (removes them)", move("refs/heads/old", h.o, h.commit("", "", h.o))) | |||
| 472 | + | ||||
| 473 | + | // Merges: a change only if it differs from every parent. | |||
| 474 | + | allowed("a merge of existing commits that keeps main's workflows", move("refs/heads/main", h.m, h.commit("A", "", h.m, h.o))) | |||
| 475 | + | allowed("a new branch with a merge keeping one parent's workflows", create("refs/heads/merge", h.commit("A", "", h.m, h.o))) | |||
| 476 | + | refused("a merge that differs from both parents", create("refs/heads/merge2", h.commit("D", "", h.m, h.o))) | |||
| 477 | + | refused("a merge of a pushed side that changes them (the side commit is itself new)", create("refs/heads/merge3", h.commit("A", "", h.m, h.commit("C", "", h.o)))) | |||
| 478 | + | // One that takes the old side's workflows is a change of the ref, not of a commit. | |||
| 479 | + | refused("a merge taking the other parent's workflows, onto main", move("refs/heads/main", h.m, h.commit("B", "", h.m, h.o))) | |||
| 480 | + | ||||
| 481 | + | // Several updates: the first refused refuses the push. | |||
| 482 | + | refused("a plain ref and a refused one", create("refs/heads/ok", h.commit("A", "", h.m)), create("refs/heads/w2", h.commit("C", "", h.m))) | |||
| 483 | + | allowed("deletes", move("refs/heads/old", h.o, domain.ZeroOID)) | |||
| 484 | + | } | |||
| 485 | + | ||||
| 486 | + | // With no default branch head, a new ref is compared with no workflows at all. | |||
| 487 | + | func TestChangedWorkflowOfANewRefWithoutADefaultBranchHead(t *testing.T) { | |||
| 488 | + | g, data := newGit(t) | |||
| 489 | + | ctx := context.Background() | |||
| 490 | + | h := newGuardHistory(t, g, data) | |||
| 491 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |||
| 492 | + | h.f.Git("symbolic-ref", "HEAD", "refs/heads/nothing") // the default branch does not exist | |||
| 493 | + | run := func(to string) string { | |||
| 494 | + | p, err := g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: domain.ZeroOID, New: to, Ref: "refs/tags/t"}}, ports.PushObjects{}) | |||
| 495 | + | require.NoError(t, err) | |||
| 496 | + | return p | |||
| 497 | + | } | |||
| 498 | + | assert.Equal(t, ".github/workflows/ci.yml", run(h.m), "an existing commit with workflows") | |||
| 499 | + | plain := h.commit("", "") | |||
| 500 | + | h.f.Git("update-ref", "refs/heads/plain", plain) | |||
| 501 | + | assert.Empty(t, run(plain), "an existing commit without") | |||
| 502 | + | } | |||
| 503 | + | ||||
| 504 | + | // The commits of a push are bounded: more than the bound fails closed. | |||
| 505 | + | func TestChangedWorkflowBoundsThePushedCommits(t *testing.T) { | |||
| 506 | + | g, data := newGit(t) | |||
| 507 | + | ctx := context.Background() | |||
| 508 | + | h := newGuardHistory(t, g, data) | |||
| 509 | + | t.Cleanup(func() { _ = g.Close(ctx) }) | |||
| 510 | + | // A chain of plain commits on main, made with fast-import in a ref that is | |||
| 511 | + | // removed afterwards: the objects stay, in no ref, like a push in quarantine. | |||
| 512 | + | chain := func(n int) string { | |||
| 513 | + | var b strings.Builder | |||
| 514 | + | fmt.Fprintf(&b, "reset refs/tmp/chain\nfrom %s\n", h.m) | |||
| 515 | + | for i := range n { | |||
| 516 | + | fmt.Fprintf(&b, "commit refs/tmp/chain\ncommitter a <a@b> %d +0000\ndata 1\nc\nM 100644 inline f.txt\ndata 2\n%d\n\n", 1000+i, i%10) | |||
| 517 | + | } | |||
| 518 | + | cmd := exec.Command("git", "--git-dir="+h.f.Bare, "fast-import", "--quiet") | |||
| 519 | + | cmd.Stdin = strings.NewReader(b.String()) | |||
| 520 | + | out, err := cmd.CombinedOutput() | |||
| 521 | + | require.NoError(t, err, string(out)) | |||
| 522 | + | tip := h.f.Git("rev-parse", "refs/tmp/chain") | |||
| 523 | + | h.f.Git("update-ref", "-d", "refs/tmp/chain") | |||
| 524 | + | return tip | |||
| 525 | + | } | |||
| 526 | + | // Plain commits carry the workflows of main (the fast-import tree has none, | |||
| 527 | + | // so the chain is "removing" them): allowed, whatever their number, up to the bound. | |||
| 528 | + | p, err := g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: h.m, New: chain(gitcli.MaxPushedCommits), Ref: "refs/heads/main"}}, ports.PushObjects{}) | |||
| 529 | + | require.NoError(t, err) | |||
| 530 | + | assert.Empty(t, p) | |||
| 531 | + | _, err = g.ChangedWorkflow(ctx, h.f.Name, []domain.RefUpdate{{Old: h.m, New: chain(gitcli.MaxPushedCommits + 1), Ref: "refs/heads/main"}}, ports.PushObjects{}) | |||
| 532 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "more commits than the bound refuses the push") | |||
| 533 | + | } | |||
| 534 | + | ||||
| 535 | + | // countingReader serves synthetic rev-list output and counts what was read. | |||
| 536 | + | type countingReader struct { | |||
| 537 | + | lines, read int | |||
| 538 | + | line []byte | |||
| 539 | + | pending []byte | |||
| 540 | + | made int | |||
| 541 | + | } | |||
| 542 | + | ||||
| 543 | + | func (c *countingReader) Read(p []byte) (int, error) { | |||
| 544 | + | if len(c.pending) == 0 { | |||
| 545 | + | if c.made == c.lines { | |||
| 546 | + | return 0, io.EOF | |||
| 547 | + | } | |||
| 548 | + | c.made++ | |||
| 549 | + | c.pending = c.line | |||
| 550 | + | } | |||
| 551 | + | n := copy(p, c.pending) | |||
| 552 | + | c.pending = c.pending[n:] | |||
| 553 | + | c.read += n | |||
| 554 | + | return n, nil | |||
| 555 | + | } | |||
| 556 | + | ||||
| 557 | + | // A hostile push: ten thousand commits of two hundred parents each is two | |||
| 558 | + | // million ids. The bound is met after the first lines, and nothing more is read | |||
| 559 | + | // or kept. | |||
| 560 | + | func TestParseRevListRejectsManyParentsEarly(t *testing.T) { | |||
| 561 | + | id := func(n int) string { return fmt.Sprintf("%040x", n+1) } | |||
| 562 | + | var b strings.Builder | |||
| 563 | + | b.WriteString(id(0)) | |||
| 564 | + | for i := range 198 { | |||
| 565 | + | b.WriteString(" " + id(1+i%3)) // few distinct parents, repeated: the distinct count alone would stay small | |||
| 566 | + | } | |||
| 567 | + | b.WriteString("\n") | |||
| 568 | + | r := &countingReader{lines: gitcli.MaxPushedCommits, line: []byte(b.String())} | |||
| 569 | + | kept, err := gitcli.ParseRevList(r, gitcli.MaxPushedCommits, gitcli.MaxPushedObjects) | |||
| 570 | + | require.ErrorIs(t, err, domain.ErrInvalid) | |||
| 571 | + | assert.Contains(t, err.Error(), "ids (commits and parents)", "refused by the bound on ids, not by the line length") | |||
| 572 | + | assert.Zero(t, kept, "nothing is returned with the error") | |||
| 573 | + | total := gitcli.MaxPushedCommits * len(b.String()) | |||
| 574 | + | assert.Less(t, r.read, 2<<20, "read only the head of %d bytes", total) | |||
| 575 | + | assert.Less(t, r.read*20, total) | |||
| 576 | + | ||||
| 577 | + | // Within the bounds the same lines are kept: 100 commits of 198 parents. | |||
| 578 | + | r = &countingReader{lines: 100, line: []byte(b.String())} | |||
| 579 | + | kept, err = gitcli.ParseRevList(r, gitcli.MaxPushedCommits, gitcli.MaxPushedObjects) | |||
| 580 | + | require.NoError(t, err) | |||
| 581 | + | assert.Equal(t, 100, kept) | |||
| 582 | + | ||||
| 583 | + | // The other bounds and malformed output. | |||
| 584 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+"\n"+id(1)+"\n"), 1, 100) | |||
| 585 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "commits") | |||
| 586 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+" "+id(1)+"\n"+id(2)+"\n"), 10, 2) | |||
| 587 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "ids") | |||
| 588 | + | _, err = gitcli.ParseRevList(strings.NewReader("nonsense\n"), 10, 10) | |||
| 589 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "an id that is none") | |||
| 590 | + | _, err = gitcli.ParseRevList(strings.NewReader(id(0)+strings.Repeat(" "+id(1), 300)+"\n"), 10, 1000) | |||
| 591 | + | assert.ErrorIs(t, err, domain.ErrInvalid, "a line over the bound") | |||
| 592 | + | } | |||
M internal/app/ci_permissions_test.go +21 -9
| @@ -196,19 +196,25 @@ func TestJobTokenWithContentsWritePushesButNotWorkflows(t *testing.T) { | |||
| 196 | 196 | require.Error(t, err, "a rename into .github/workflows") | |
| 197 | 197 | assert.Contains(t, out, refusal(".github/workflows/moved.yml")) | |
| 198 | 198 | ||
| 199 | - | // A tag that points to a commit holding workflows (even an existing one). | |
| 199 | + | // A ref cut from the head of main carries main's workflows and changes none: | |
| 200 | + | // a tag, an annotated tag and a branch there are allowed, as are new branches | |
| 201 | + | // with commits that touch no workflow (staging run 855). | |
| 200 | 202 | out, err = c.tokenPush(c.wt, tok, "main:refs/tags/v1") | |
| 201 | - | require.Error(t, err, "a tag on a commit with workflows: "+out) | |
| 202 | - | assert.Contains(t, out, "without `workflows` permission") | |
| 203 | - | // An annotated tag is judged by the commit it peels to. | |
| 203 | + | require.NoError(t, err, "a tag on main's head: "+out) | |
| 204 | 204 | c.mustGit(c.wt, "tag", "-a", "-m", "release", "va", "main") | |
| 205 | 205 | out, err = c.tokenPush(c.wt, tok, "refs/tags/va:refs/tags/va") | |
| 206 | - | require.Error(t, err, "an annotated tag on a commit with workflows: "+out) | |
| 207 | - | assert.Contains(t, out, "without `workflows` permission") | |
| 208 | - | // A new branch off main carries the workflows too. | |
| 206 | + | require.NoError(t, err, "an annotated tag on main's head: "+out) | |
| 209 | 207 | out, err = c.tokenPush(c.wt, tok, "main:refs/heads/copy") | |
| 210 | - | require.Error(t, err) | |
| 211 | - | assert.Contains(t, out, "without `workflows` permission") | |
| 208 | + | require.NoError(t, err, "a branch at main's head: "+out) | |
| 209 | + | c.mustGit(c.wt, "checkout", "-q", "-B", "smoke/perms-ok", "main") | |
| 210 | + | c.commitIn(c.wt, "smoke.txt", "ok") | |
| 211 | + | out, err = c.tokenPush(c.wt, tok, "smoke/perms-ok:refs/heads/smoke/perms-ok") | |
| 212 | + | require.NoError(t, err, "a new branch with a commit that touches no workflow: "+out) | |
| 213 | + | c.mustGit(c.wt, "checkout", "-q", "-B", "smoke/wf", "main") | |
| 214 | + | c.commitIn(c.wt, ".github/workflows/smoke.yml", wfYAML) | |
| 215 | + | out, err = c.tokenPush(c.wt, tok, "smoke/wf:refs/heads/smoke/wf") | |
| 216 | + | require.Error(t, err, "a new branch with a commit that adds a workflow: "+out) | |
| 217 | + | assert.Contains(t, out, refusal(".github/workflows/smoke.yml")) | |
| 212 | 218 | ||
| 213 | 219 | // One ref that touches a workflow refuses a push of several. | |
| 214 | 220 | c.mustGit(c.wt, "checkout", "-q", "-B", "ok", "main") | |
| @@ -223,6 +229,12 @@ func TestJobTokenWithContentsWritePushesButNotWorkflows(t *testing.T) { | |||
| 223 | 229 | out, err = c.git(c.wt, "push", c.urlAuth(adminToken, "/proj.git"), "r:refs/heads/rworkflow") | |
| 224 | 230 | require.NoError(t, err, out) | |
| 225 | 231 | ||
| 232 | + | // A job token cannot put a ref on that existing commit, whose workflows | |
| 233 | + | // differ from the default branch's, nor move main back onto it. | |
| 234 | + | out, err = c.tokenPush(c.wt, tok, "r:refs/heads/cut") | |
| 235 | + | require.Error(t, err, "a new branch at an existing commit with other workflows: "+out) | |
| 236 | + | assert.Contains(t, out, "without `workflows` permission") | |
| 237 | + | ||
| 226 | 238 | // None of the pushes of the job token started a run; a push of the owner | |
| 227 | 239 | // does, and queues after them. | |
| 228 | 240 | c.push("sentinel", map[string]string{"s.txt": "s"}, "") | |
| @@ -196,19 +196,25 @@ func TestJobTokenWithContentsWritePushesButNotWorkflows(t *testing.T) { | |||||
| 196 | require.Error(t, err, "a rename into .github/workflows") | 196 | require.Error(t, err, "a rename into .github/workflows") | ||
| 197 | assert.Contains(t, out, refusal(".github/workflows/moved.yml")) | 197 | assert.Contains(t, out, refusal(".github/workflows/moved.yml")) | ||
| 198 | 198 | ||||
| 199 | - | // A tag that points to a commit holding workflows (even an existing one). | 199 | + | // A ref cut from the head of main carries main's workflows and changes none: |
| 200 | + | // a tag, an annotated tag and a branch there are allowed, as are new branches | |||
| 201 | + | // with commits that touch no workflow (staging run 855). | |||
| 200 | out, err = c.tokenPush(c.wt, tok, "main:refs/tags/v1") | 202 | out, err = c.tokenPush(c.wt, tok, "main:refs/tags/v1") | ||
| 201 | - | require.Error(t, err, "a tag on a commit with workflows: "+out) | 203 | + | require.NoError(t, err, "a tag on main's head: "+out) |
| 202 | - | assert.Contains(t, out, "without `workflows` permission") | |||
| 203 | - | // An annotated tag is judged by the commit it peels to. | |||
| 204 | c.mustGit(c.wt, "tag", "-a", "-m", "release", "va", "main") | 204 | c.mustGit(c.wt, "tag", "-a", "-m", "release", "va", "main") | ||
| 205 | out, err = c.tokenPush(c.wt, tok, "refs/tags/va:refs/tags/va") | 205 | out, err = c.tokenPush(c.wt, tok, "refs/tags/va:refs/tags/va") | ||
| 206 | - | require.Error(t, err, "an annotated tag on a commit with workflows: "+out) | 206 | + | require.NoError(t, err, "an annotated tag on main's head: "+out) |
| 207 | - | assert.Contains(t, out, "without `workflows` permission") | |||
| 208 | - | // A new branch off main carries the workflows too. | |||
| 209 | out, err = c.tokenPush(c.wt, tok, "main:refs/heads/copy") | 207 | out, err = c.tokenPush(c.wt, tok, "main:refs/heads/copy") | ||
| 210 | - | require.Error(t, err) | 208 | + | require.NoError(t, err, "a branch at main's head: "+out) |
| 211 | - | assert.Contains(t, out, "without `workflows` permission") | 209 | + | c.mustGit(c.wt, "checkout", "-q", "-B", "smoke/perms-ok", "main") |
| 210 | + | c.commitIn(c.wt, "smoke.txt", "ok") | |||
| 211 | + | out, err = c.tokenPush(c.wt, tok, "smoke/perms-ok:refs/heads/smoke/perms-ok") | |||
| 212 | + | require.NoError(t, err, "a new branch with a commit that touches no workflow: "+out) | |||
| 213 | + | c.mustGit(c.wt, "checkout", "-q", "-B", "smoke/wf", "main") | |||
| 214 | + | c.commitIn(c.wt, ".github/workflows/smoke.yml", wfYAML) | |||
| 215 | + | out, err = c.tokenPush(c.wt, tok, "smoke/wf:refs/heads/smoke/wf") | |||
| 216 | + | require.Error(t, err, "a new branch with a commit that adds a workflow: "+out) | |||
| 217 | + | assert.Contains(t, out, refusal(".github/workflows/smoke.yml")) | |||
| 212 | 218 | ||||
| 213 | // One ref that touches a workflow refuses a push of several. | 219 | // One ref that touches a workflow refuses a push of several. | ||
| 214 | c.mustGit(c.wt, "checkout", "-q", "-B", "ok", "main") | 220 | c.mustGit(c.wt, "checkout", "-q", "-B", "ok", "main") | ||
| @@ -223,6 +229,12 @@ func TestJobTokenWithContentsWritePushesButNotWorkflows(t *testing.T) { | |||||
| 223 | out, err = c.git(c.wt, "push", c.urlAuth(adminToken, "/proj.git"), "r:refs/heads/rworkflow") | 229 | out, err = c.git(c.wt, "push", c.urlAuth(adminToken, "/proj.git"), "r:refs/heads/rworkflow") | ||
| 224 | require.NoError(t, err, out) | 230 | require.NoError(t, err, out) | ||
| 225 | 231 | ||||
| 232 | + | // A job token cannot put a ref on that existing commit, whose workflows | |||
| 233 | + | // differ from the default branch's, nor move main back onto it. | |||
| 234 | + | out, err = c.tokenPush(c.wt, tok, "r:refs/heads/cut") | |||
| 235 | + | require.Error(t, err, "a new branch at an existing commit with other workflows: "+out) | |||
| 236 | + | assert.Contains(t, out, "without `workflows` permission") | |||
| 237 | + | ||||
| 226 | // None of the pushes of the job token started a run; a push of the owner | 238 | // None of the pushes of the job token started a run; a push of the owner | ||
| 227 | // does, and queues after them. | 239 | // does, and queues after them. | ||
| 228 | c.push("sentinel", map[string]string{"s.txt": "s"}, "") | 240 | c.push("sentinel", map[string]string{"s.txt": "s"}, "") | ||
M internal/ports/ci.go +11 -5
| @@ -273,11 +273,17 @@ type PushObjects struct { | |||
| 273 | 273 | // WorkflowGuard finds the changes of a push to the workflow files of a | |
| 274 | 274 | // repository. | |
| 275 | 275 | type WorkflowGuard interface { | |
| 276 | - | // ChangedWorkflow returns the first path under .github/workflows/ that | |
| 277 | - | // the updates create or change, from the old tree of each ref (the empty tree | |
| 278 | - | // for a new ref) to its new one. A deleted ref changes nothing. "" means | |
| 279 | - | // none. objects are those of the push in progress; a directory that is not | |
| 280 | - | // inside the repository is refused. | |
| 276 | + | // ChangedWorkflow returns the first path under .github/workflows/ that the | |
| 277 | + | // updates change, "" for none. Two checks, as GitHub judges the token of a | |
| 278 | + | // job: (1) every commit the push brings (those no ref has yet) is compared | |
| 279 | + | // with each of its parents (a root commit with nothing) and is a change when | |
| 280 | + | // it has workflows that differ from every parent's; (2) the workflows a ref | |
| 281 | + | // ends with must be its old ones or none for an update, and the default | |
| 282 | + | // branch head's or none for a new ref (none when there is no head). A | |
| 283 | + | // workflows directory that disappears is no change, and a deleted ref changes | |
| 284 | + | // nothing. objects are those of the push in progress; a directory that is not | |
| 285 | + | // inside the repository is refused. A push beyond the bounds of the check | |
| 286 | + | // (commits, parents) is an error that matches domain.ErrInvalid. | |
| 281 | 287 | ChangedWorkflow(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, objects PushObjects) (path string, err error) | |
| 282 | 288 | } | |
| 283 | 289 | ||
| @@ -273,11 +273,17 @@ type PushObjects struct { | |||||
| 273 | // WorkflowGuard finds the changes of a push to the workflow files of a | 273 | // WorkflowGuard finds the changes of a push to the workflow files of a | ||
| 274 | // repository. | 274 | // repository. | ||
| 275 | type WorkflowGuard interface { | 275 | type WorkflowGuard interface { | ||
| 276 | - | // ChangedWorkflow returns the first path under .github/workflows/ that | 276 | + | // ChangedWorkflow returns the first path under .github/workflows/ that the |
| 277 | - | // the updates create or change, from the old tree of each ref (the empty tree | 277 | + | // updates change, "" for none. Two checks, as GitHub judges the token of a |
| 278 | - | // for a new ref) to its new one. A deleted ref changes nothing. "" means | 278 | + | // job: (1) every commit the push brings (those no ref has yet) is compared |
| 279 | - | // none. objects are those of the push in progress; a directory that is not | 279 | + | // with each of its parents (a root commit with nothing) and is a change when |
| 280 | - | // inside the repository is refused. | 280 | + | // it has workflows that differ from every parent's; (2) the workflows a ref |
| 281 | + | // ends with must be its old ones or none for an update, and the default | |||
| 282 | + | // branch head's or none for a new ref (none when there is no head). A | |||
| 283 | + | // workflows directory that disappears is no change, and a deleted ref changes | |||
| 284 | + | // nothing. objects are those of the push in progress; a directory that is not | |||
| 285 | + | // inside the repository is refused. A push beyond the bounds of the check | |||
| 286 | + | // (commits, parents) is an error that matches domain.ErrInvalid. | |||
| 281 | ChangedWorkflow(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, objects PushObjects) (path string, err error) | 287 | ChangedWorkflow(ctx context.Context, repo domain.RepoName, updates []domain.RefUpdate, objects PushObjects) (path string, err error) | ||
| 282 | } | 288 | } | ||
| 283 | 289 | ||||
M internal/usecase/pushes.go +11 -5
| @@ -131,11 +131,14 @@ func openExisting(p domain.Principal, repo domain.Repo) error { | |||
| 131 | 131 | // stops the push before Git applies it. A failure to tell refuses it too. | |
| 132 | 132 | // | |
| 133 | 133 | // The token of a CI job (GITHUB_TOKEN with contents: write) may not change | |
| 134 | - | // workflows: a push that creates or changes a file under .github/workflows/ on | |
| 135 | - | // any ref, branch or tag, is refused as a whole (CI-3, D3). A new ref counts | |
| 136 | - | // every workflow its commit holds, as on GitHub; deleting a ref is not a | |
| 137 | - | // change. objects locate the objects of the push, which are not in the | |
| 138 | - | // repository yet. | |
| 134 | + | // workflows (CI-3, D3). The push is refused as a whole, on any ref, branch or | |
| 135 | + | // tag, when a commit it brings changes .github/workflows (it has workflows that | |
| 136 | + | // differ from those of every parent; removing them is allowed), or when a ref | |
| 137 | + | // would end with workflows other than it had: an update with other than the old | |
| 138 | + | // ones, a new ref with other than those of the default branch head (or any, when | |
| 139 | + | // there is no head). A new branch cut from existing commits that change nothing | |
| 140 | + | // is therefore allowed; deleting a ref is not a change. objects locate the | |
| 141 | + | // objects of the push, which are not in the repository yet. | |
| 139 | 142 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate, objects ports.PushObjects) error { | |
| 140 | 143 | if err := u.auth.Revalidate(ctx, p); err != nil { | |
| 141 | 144 | if errors.Is(err, domain.ErrUnauthenticated) { | |
| @@ -173,6 +176,9 @@ func (u *Pushes) checkWorkflows(ctx context.Context, name domain.RepoName, updat | |||
| 173 | 176 | } | |
| 174 | 177 | path, err := u.wf.ChangedWorkflow(ctx, name, updates, objects) | |
| 175 | 178 | switch { | |
| 179 | + | case errors.Is(err, domain.ErrInvalid): | |
| 180 | + | // A push the guard cannot judge within its bounds: refused, with the reason. | |
| 181 | + | return fmt.Errorf("%w: %v", domain.ErrRejected, err) | |
| 176 | 182 | case err != nil: | |
| 177 | 183 | return fmt.Errorf("find the workflows changed by a push to %s: %w", name, err) | |
| 178 | 184 | case path != "": | |
| @@ -131,11 +131,14 @@ func openExisting(p domain.Principal, repo domain.Repo) error { | |||||
| 131 | // stops the push before Git applies it. A failure to tell refuses it too. | 131 | // stops the push before Git applies it. A failure to tell refuses it too. | ||
| 132 | // | 132 | // | ||
| 133 | // The token of a CI job (GITHUB_TOKEN with contents: write) may not change | 133 | // The token of a CI job (GITHUB_TOKEN with contents: write) may not change | ||
| 134 | - | // workflows: a push that creates or changes a file under .github/workflows/ on | 134 | + | // workflows (CI-3, D3). The push is refused as a whole, on any ref, branch or |
| 135 | - | // any ref, branch or tag, is refused as a whole (CI-3, D3). A new ref counts | 135 | + | // tag, when a commit it brings changes .github/workflows (it has workflows that |
| 136 | - | // every workflow its commit holds, as on GitHub; deleting a ref is not a | 136 | + | // differ from those of every parent; removing them is allowed), or when a ref |
| 137 | - | // change. objects locate the objects of the push, which are not in the | 137 | + | // would end with workflows other than it had: an update with other than the old |
| 138 | - | // repository yet. | 138 | + | // ones, a new ref with other than those of the default branch head (or any, when |
| 139 | + | // there is no head). A new branch cut from existing commits that change nothing | |||
| 140 | + | // is therefore allowed; deleting a ref is not a change. objects locate the | |||
| 141 | + | // objects of the push, which are not in the repository yet. | |||
| 139 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate, objects ports.PushObjects) error { | 142 | func (u *Pushes) Check(ctx context.Context, p domain.Principal, name domain.RepoName, updates []domain.RefUpdate, objects ports.PushObjects) error { | ||
| 140 | if err := u.auth.Revalidate(ctx, p); err != nil { | 143 | if err := u.auth.Revalidate(ctx, p); err != nil { | ||
| 141 | if errors.Is(err, domain.ErrUnauthenticated) { | 144 | if errors.Is(err, domain.ErrUnauthenticated) { | ||
| @@ -173,6 +176,9 @@ func (u *Pushes) checkWorkflows(ctx context.Context, name domain.RepoName, updat | |||||
| 173 | } | 176 | } | ||
| 174 | path, err := u.wf.ChangedWorkflow(ctx, name, updates, objects) | 177 | path, err := u.wf.ChangedWorkflow(ctx, name, updates, objects) | ||
| 175 | switch { | 178 | switch { | ||
| 179 | + | case errors.Is(err, domain.ErrInvalid): | |||
| 180 | + | // A push the guard cannot judge within its bounds: refused, with the reason. | |||
| 181 | + | return fmt.Errorf("%w: %v", domain.ErrRejected, err) | |||
| 176 | case err != nil: | 182 | case err != nil: | ||
| 177 | return fmt.Errorf("find the workflows changed by a push to %s: %w", name, err) | 183 | return fmt.Errorf("find the workflows changed by a push to %s: %w", name, err) | ||
| 178 | case path != "": | 184 | case path != "": | ||
M internal/usecase/pushes_test.go +8 -0
| @@ -5,6 +5,7 @@ package usecase_test | |||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | 7 | "errors" | |
| 8 | + | "fmt" | |
| 8 | 9 | "testing" | |
| 9 | 10 | "time" | |
| 10 | 11 | ||
| @@ -499,6 +500,13 @@ func TestPushesCheckRefusesWorkflowChangesOfJobTokens(t *testing.T) { | |||
| 499 | 500 | require.Error(t, err) | |
| 500 | 501 | assert.NotErrorIs(t, err, domain.ErrRejected) | |
| 501 | 502 | }) | |
| 503 | + | t.Run("a push beyond the guard's bounds is refused as a policy, with the reason", func(t *testing.T) { | |
| 504 | + | t.Parallel() | |
| 505 | + | f := build(t, &fakeGuard{err: fmt.Errorf("%w: the push brings more than 10000 commits", domain.ErrInvalid)}) | |
| 506 | + | err := f.pushes.Check(ctx, job, n, ups, objs) | |
| 507 | + | require.ErrorIs(t, err, domain.ErrRejected) | |
| 508 | + | assert.Contains(t, err.Error(), "more than 10000 commits") | |
| 509 | + | }) | |
| 502 | 510 | t.Run("without a guard a job token cannot be judged: refused", func(t *testing.T) { | |
| 503 | 511 | t.Parallel() | |
| 504 | 512 | f := build(t, nil) | |
| @@ -5,6 +5,7 @@ package usecase_test | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | "errors" | 7 | "errors" | ||
| 8 | + | "fmt" | |||
| 8 | "testing" | 9 | "testing" | ||
| 9 | "time" | 10 | "time" | ||
| 10 | 11 | ||||
| @@ -499,6 +500,13 @@ func TestPushesCheckRefusesWorkflowChangesOfJobTokens(t *testing.T) { | |||||
| 499 | require.Error(t, err) | 500 | require.Error(t, err) | ||
| 500 | assert.NotErrorIs(t, err, domain.ErrRejected) | 501 | assert.NotErrorIs(t, err, domain.ErrRejected) | ||
| 501 | }) | 502 | }) | ||
| 503 | + | t.Run("a push beyond the guard's bounds is refused as a policy, with the reason", func(t *testing.T) { | |||
| 504 | + | t.Parallel() | |||
| 505 | + | f := build(t, &fakeGuard{err: fmt.Errorf("%w: the push brings more than 10000 commits", domain.ErrInvalid)}) | |||
| 506 | + | err := f.pushes.Check(ctx, job, n, ups, objs) | |||
| 507 | + | require.ErrorIs(t, err, domain.ErrRejected) | |||
| 508 | + | assert.Contains(t, err.Error(), "more than 10000 commits") | |||
| 509 | + | }) | |||
| 502 | t.Run("without a guard a job token cannot be judged: refused", func(t *testing.T) { | 510 | t.Run("without a guard a job token cannot be judged: refused", func(t *testing.T) { | ||
| 503 | t.Parallel() | 511 | t.Parallel() | ||
| 504 | f := build(t, nil) | 512 | f := build(t, nil) | ||