Commit fb6e9dcfa4
Verified · cmc ci/build: success ci/test: success
Layout: unified · split
docs/plans/2026-09-28-followups.md added +2628
| @@ -0,0 +1,2628 @@ | ||
| 1 | # Review follow-ups implementation plan | |
| 2 | ||
| 3 | > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | |
| 4 | ||
| 5 | **Goal:** Close #287, #284, #298, #285 and #297: the runner drop-in's | |
| 6 | comment names the repositories the runner is really attached to; | |
| 7 | `webhook add` takes its signing secret on stdin only; `repo import | |
| 8 | --from` fetches only over http(s) from a resolved, checked and pinned | |
| 9 | address; an LFS transfer token dies with the SSH key that obtained it; | |
| 10 | and a browser session mints credentials only within 15 minutes of | |
| 11 | signing in. | |
| 12 | ||
| 13 | **Architecture:** No migrations. Four MRs on independent branches off | |
| 14 | `main`. The resolve-check-pin logic mirror sync gained in #279 moves | |
| 15 | out of `internal/mirror` into a new `internal/gitpin` package, which | |
| 16 | mirror sync and `repo import` both call (`internal/control` cannot | |
| 17 | import `internal/mirror`: mirror imports control). LFS tokens gain a | |
| 18 | key id in their signed payload, and `httpd.lfsAuth` checks it with | |
| 19 | `store.LiveSSHKeys`, the liveness query #256 added. For #297, | |
| 20 | `web_sessions.created_at` is the sign-in time (only `/login?token=` | |
| 21 | creates a session, and the idle renewal from #276 never writes | |
| 22 | `created_at`), so `store.WebSessionUser` returns it on the user, | |
| 23 | every web dispatch copies it into a new `Ctx.SignedInAt`, and | |
| 24 | `control.runChecked` refuses a `MintsCredential` command when it is | |
| 25 | older than `control.ReauthWindow`, next to the #257 `Expires` check. | |
| 26 | SSH and the API never set `SignedInAt`, so they are untouched. | |
| 27 | ||
| 28 | **Tech stack:** Go 1.27, SQLite (modernc), `golang.org/x/crypto/ssh`, | |
| 29 | cobra, git ≥ 2.37 (`http.curloptResolve`), git-lfs (e2e only). | |
| 30 | ||
| 31 | **Spec:** none — the issue texts of #284, #285, #287, #297 and #298 on | |
| 32 | krz/gitbay, and the decisions below, are the spec. | |
| 33 | ||
| 34 | ## Global constraints | |
| 35 | ||
| 36 | - Four MRs, each on its own branch off `main`, in a fresh worktree | |
| 37 | (`git worktree add ../gitbay-<branch> -b <branch> main`); the | |
| 38 | checkout may hold another session's edits. | |
| 39 | - Commits are signed (the repository refuses unsigned ones); messages | |
| 40 | reference the issue they touch (`Ref #N`), and the commit that | |
| 41 | finishes an issue says `Closes #N`. No attribution to any assistant, | |
| 42 | model or AI anywhere: commits, MR bodies, comments. | |
| 43 | - MR: `gitbay mr create --source <branch> --target main --title "..."`; | |
| 44 | merge with `gitbay mr merge <n> --strategy ff` once CI is green | |
| 45 | (this repository requires signed commits, so `squash`/`merge` are | |
| 46 | refused), then delete the branch locally and on the remote. If the | |
| 47 | merge reports the branch is behind, rebase onto `main`, force-push, | |
| 48 | merge again. | |
| 49 | - Locally: `go build ./...`, `go vet ./...`, the unit tests of every | |
| 50 | touched package, and at most the one e2e test named in the task | |
| 51 | (`go test ./e2e -run TestName -count=1`). CI on bay1 runs the full | |
| 52 | suite. | |
| 53 | - No new migrations; the next free number stays 0067. | |
| 54 | - Registries that fail CI when a new thing lacks its row: a top-level | |
| 55 | route word in `internal/policy/names.go`; a new page template in the | |
| 56 | width map of `TestMainWidthClass` (`internal/web/web_test.go`); a new | |
| 57 | `ReadOnly` command in `readArgs` in `e2e/readonly_test.go`; a new | |
| 58 | control command needs a `pass()` entry in `cmd/gitbay/main.go` | |
| 59 | (`cmd/gitbay/coverage_test.go`); a command that reads stdin needs | |
| 60 | `ReadsStdin: true`; a changed `Summary` needs | |
| 61 | `go test ./cmd/gitbay -run TestSummariesAreCurrent -update` | |
| 62 | (`cmd/gitbay/summaries_gen.go`). This plan adds no route, no | |
| 63 | template, no command and changes no summary; it sets `ReadsStdin` on | |
| 64 | `webhook add` (Task 1.2). | |
| 65 | - Secrets travel on stdin, never argv; never logged or echoed. | |
| 66 | - Wiki pages live in `.gitbay/wiki/`. Update the page in the MR that | |
| 67 | changes the behaviour it describes; close the matching row in | |
| 68 | `Architecture/10-Known-Gaps.org` and update the row in | |
| 69 | `Architecture/09-Controls.org`. Parity is not affected by any of the | |
| 70 | four MRs (webhooks have no web form; no capability changes surface). | |
| 71 | - `CHANGELOG.org`: each MR's last task appends its entries at the end | |
| 72 | of the `* Unreleased` bullet list, directly above | |
| 73 | `* v1.36.0 — 2026-09-23`. | |
| 74 | - Writing style: plain, direct, no hype; code comments match the | |
| 75 | surrounding density; no before/after narration in comments or docs. | |
| 76 | ||
| 77 | ## Decisions (binding, from the brief) | |
| 78 | ||
| 79 | - #287: correct the comment in `deploy/gitbay-runner.override.conf`: | |
| 80 | `cmc/ci-smoke` no longer exists; the runner is attached to krz/gitbay, | |
| 81 | krz/hutch, krz/keycask, krz/orgo, krz/skunky-art and cmc/cleberg.net; | |
| 82 | a scratch repository is created and attached for runner validation. | |
| 83 | - #284: `webhook add` takes the secret from stdin only (`--secret -`, | |
| 84 | `ReadsStdin: true`); a literal `--secret <value>` is refused with a | |
| 85 | message saying to pipe it, as `repo secret set` does. | |
| 86 | - #298: `repo import --from` accepts http and https only; `git://` and | |
| 87 | anything else is refused with a message to use the https URL. Import | |
| 88 | gets the same resolve, check and pin as mirror sync, through one | |
| 89 | shared helper. | |
| 90 | - #285: LFS transfer tokens carry the SSH key id (deploy keys | |
| 91 | included); each LFS request that presents a token checks the key | |
| 92 | still exists, is unexpired, and its account is not disabled. Tokens | |
| 93 | minted before the upgrade (no key id) are refused. | |
| 94 | - #297: a `MintsCredential` command dispatched from a web session is | |
| 95 | refused unless the session signed in within the last 15 minutes; the | |
| 96 | web shows the form again with the message and a "Sign in again" link | |
| 97 | that returns to the form. The check lives in `control.runChecked`. | |
| 98 | ||
| 99 | ## Findings from the code that shape this plan | |
| 100 | ||
| 101 | - `repo secret set` takes no value argument at all | |
| 102 | (`internal/control/build.go:426-451`): a third argument is a usage | |
| 103 | error, and an empty stdin is refused with | |
| 104 | `no value on stdin (pipe it: printf %s TOKEN | ...)`. `webhook add` | |
| 105 | mirrors both refusals, as exit 2. | |
| 106 | - No web handler builds `webhook add` (Parity: webhooks, web `no`), so | |
| 107 | #284 changes only the command, the CLI passthrough, one e2e call | |
| 108 | (`e2e/webhook_test.go:115-116`) and `API.org:153`. | |
| 109 | - The CLI forwards stdin for a `stdinOK` command only when `usesStdin` | |
| 110 | (`cmd/gitbay/main.go:358-368`) sees `--file -`, `--key -` or | |
| 111 | `--token-stdin`; it must learn `--secret -`. | |
| 112 | - Deploy keys are `ssh_keys` rows with scope `deploy:<repo>:ro|rw` | |
| 113 | (`internal/policy/access.go:60-79`). `git-lfs-authenticate` is | |
| 114 | reached from `sshd.Exec` (`internal/sshd/sshd.go:492-499`), which | |
| 115 | holds the authenticating `store.SSHKey`, so its `ID` identifies user | |
| 116 | keys and deploy keys alike, and `store.LiveSSHKeys` | |
| 117 | (`internal/store/revoke.go:36-62`) already answers "registered, | |
| 118 | unexpired, account not disabled" for both. | |
| 119 | - `lfsBatch` (`internal/httpd/lfs.go:130`) mints transfer tokens for | |
| 120 | anonymous batch requests on public repositories too. Those carry key | |
| 121 | id 0, and `lfsAuth` honours a key-0 token only as an anonymous | |
| 122 | download of a public repository. | |
| 123 | - A pre-upgrade LFS token's payload has three fields | |
| 124 | (`repo:op:exp`, `internal/lfs/lfs.go:130`); the new payload has four, | |
| 125 | so `Verify` refuses the old shape by field count. The server returns | |
| 126 | `expires_in` with every token; nothing in `internal/lfs` makes a | |
| 127 | client retry after a refusal. git-lfs obtains a token per process | |
| 128 | from `git-lfs-authenticate`, so only a transfer in flight across the | |
| 129 | deploy fails; the upgrade note says so. | |
| 130 | - `web_sessions.created_at` defaults to insert time | |
| 131 | (`0001_init.up.sql:192-197`), `CreateWebSession` is called only from | |
| 132 | `/login?token=` (`internal/httpd/accounts.go:153`), and | |
| 133 | `WebSessionUser`'s renewal updates `last_used_at` and `expires_at` | |
| 134 | only (`internal/store/sessions.go:98-100`). `created_at` is the | |
| 135 | sign-in time; no column is needed. | |
| 136 | - Web dispatch builds a `control.Ctx` in five places | |
| 137 | (`internal/httpd/control.go:35, 59, 105, 147, 188`), each from a | |
| 138 | `store.User` and nothing else. Carrying the sign-in time on the user | |
| 139 | that `viewer` returns reaches all five without changing a handler | |
| 140 | signature; one `webCtx` helper replaces the five literals so none can | |
| 141 | miss the field. | |
| 142 | - Web forms that dispatch a `MintsCredential` command: | |
| 143 | `keys add`, `email verify`, `token create` (`/settings`, | |
| 144 | `internal/httpd/account.go:224, 277, 309`) and `repo runner add` | |
| 145 | (repository settings, `internal/httpd/settings.go:193`). The account | |
| 146 | page reports a refusal through the flash cookie and a redirect; the | |
| 147 | repository settings page re-renders the form with the notice. | |
| 148 | - `setNext` (`internal/httpd/flash.go:52`) sets the `gitbay_next` | |
| 149 | cookie that `/login?token=` follows after creating a session | |
| 150 | (`accounts.go:158`); the login page names the destination. | |
| 151 | - `repo import`'s unit-level harness: `newQueueTestRepo` and | |
| 152 | `pruneCtx` (`internal/control/build_test.go:49`, | |
| 153 | `mrprune_test.go:68`). `pruneCtx` leaves `Limits.CloneTimeoutSec` | |
| 154 | at 0, which is an already-expired context; the import tests set it. | |
| 155 | - `TestRepoImport` (`e2e/import_test.go`) imports from its own | |
| 156 | instance at `127.0.0.1` on a default instance and imports once over | |
| 157 | `git://`. Both stop working: it moves to `allow_local = true`, and | |
| 158 | the `git://` import becomes a refusal case. | |
| 159 | - `deploy/gitbay-runner.override.conf:35-39` and `Admin.org:923-937, | |
| 160 | 953-954` both describe `cmc/ci-smoke` and a `-repos krz/gitbay` on | |
| 161 | `ExecStart` that the unit no longer has (`override.conf:87`); the | |
| 162 | Admin procedure's `sed` matches nothing. | |
| 163 | ||
| 164 | ## Order and dependencies | |
| 165 | ||
| 166 | | # | Branch | Closes | Migration | Depends on | | |
| 167 | |---|---|---|---|---| | |
| 168 | | 1 | `webhook-secret-stdin` | #287, #284 | — | — | | |
| 169 | | 2 | `import-pin-address` | #298 | — | — | | |
| 170 | | 3 | `lfs-token-key` | #285 | — | — | | |
| 171 | | 4 | `web-mint-reauth` | #297 | — | — | | |
| 172 | ||
| 173 | #287 and #284 share MR 1: #287 is a comment and a wiki procedure, too | |
| 174 | small for its own review, and neither touches a file the other does. | |
| 175 | No MR changes code another MR changes; none is stacked. Overlaps are | |
| 176 | textual only and resolve on rebase by keeping both sides: | |
| 177 | ||
| 178 | - Every MR appends to `* Unreleased` in `CHANGELOG.org`. | |
| 179 | - MRs 2, 3 and 4 edit `Threat-Model.org` and | |
| 180 | `Architecture/09-Controls.org` (different paragraphs and rows); MRs | |
| 181 | 2 and 4 each delete their own row from `Architecture/10-Known-Gaps.org`; | |
| 182 | MRs 3 and 4 edit different rows of `Architecture/05-Identity-and-Access.org`. | |
| 183 | ||
| 184 | Land in the table's order; any order works. | |
| 185 | ||
| 186 | ## File map | |
| 187 | ||
| 188 | | File | MR | Responsibility | | |
| 189 | |---|---|---| | |
| 190 | | `deploy/gitbay-runner.override.conf` | 1 | comment names the real attachments | | |
| 191 | | `.gitbay/wiki/Admin.org` | 1, 2 | scratch-repository procedure; import in `[webhooks]`/`[limits]` | | |
| 192 | | `internal/control/webhook.go`, `webhook_test.go` | 1 | `--secret -` from stdin, literal refused | | |
| 193 | | `cmd/gitbay/main.go`, `stdinpayload_test.go` | 1 | forward stdin for `--secret -` | | |
| 194 | | `e2e/webhook_test.go` | 1 | pipe the secret | | |
| 195 | | `.gitbay/wiki/API.org` | 1 | webhook add usage | | |
| 196 | | `internal/gitpin/gitpin.go`, `gitpin_test.go` | 2 | resolve, check, pin args, clean env, git version | | |
| 197 | | `internal/mirror/mirror.go`, `mirror_test.go` | 2 | sync through `gitpin` | | |
| 198 | | `internal/gitutil/gitutil.go` | 2 | `FetchMirror`, `RemoteDefaultBranch` take pin args and a whole env | | |
| 199 | | `internal/control/import.go`, `import_test.go` | 2 | http(s) only, resolve, check, pin | | |
| 200 | | `e2e/import_test.go` | 2 | `allow_local`, `git://` refused | | |
| 201 | | `.gitbay/wiki/Users.org`, `Architecture/02-Components.org` | 2 | import rules; `internal/gitpin` row | | |
| 202 | | `internal/lfs/lfs.go`, `lfs_test.go` | 3 | key id in the token, `Grant` | | |
| 203 | | `internal/sshd/lfs.go`, `internal/sshd/sshd.go` | 3 | sign with the authenticating key | | |
| 204 | | `internal/httpd/lfs.go`, `lfsauth_test.go` | 3 | liveness check, key-0 rule | | |
| 205 | | `e2e/lfs_test.go` | 3 | `TestLFSTokenEndsWithItsKey` | | |
| 206 | | `.gitbay/wiki/Architecture/04-Trust-Boundaries.org` | 3 | LFS flow | | |
| 207 | | `internal/store/users.go`, `sessions.go`, `sessions_test.go` | 4 | `User.SignedInAt` | | |
| 208 | | `internal/control/control.go`, `reauth_test.go` | 4 | `SignedInAt`, `ReauthWindow`, `ReauthRefusal`, the check | | |
| 209 | | `internal/httpd/control.go`, `flash.go`, `account.go`, `settings.go`, `account_test.go`, `reauth_test.go` | 4 | `webCtx`, the sign-in link | | |
| 210 | | `internal/web/templates/account.html`, `settings.html` | 4 | the link | | |
| 211 | | `.gitbay/wiki/Threat-Model.org`, `Architecture/05-Identity-and-Access.org`, `09-Controls.org`, `10-Known-Gaps.org` | 2, 3, 4 | docs per MR | | |
| 212 | | `CHANGELOG.org` | 1–4 | entries under `* Unreleased` | | |
| 213 | ||
| 214 | --- | |
| 215 | ||
| 216 | # MR 1: runner comment and webhook secret on stdin (branch `webhook-secret-stdin`, closes #287 and #284) | |
| 217 | ||
| 218 | ### Task 1.1: the runner drop-in and the Admin procedure name what exists | |
| 219 | ||
| 220 | **Files:** | |
| 221 | - Modify: `deploy/gitbay-runner.override.conf:35-39` | |
| 222 | - Modify: `.gitbay/wiki/Admin.org:923-937`, `:953-954` | |
| 223 | ||
| 224 | **Interfaces:** none. | |
| 225 | ||
| 226 | - [ ] **Step 1: Correct the drop-in's comment** | |
| 227 | ||
| 228 | Replace lines 35-39: | |
| 229 | ||
| 230 | ``` | |
| 231 | # The runner polls as a non-admin account with a runner-scoped key, and | |
| 232 | # claims only the repositories that key is attached to (`repo runner | |
| 233 | # add`): krz/gitbay and cmc/ci-smoke. The attachments are the boundary, | |
| 234 | # so ExecStart names no -repos. cmc/ci-smoke is the nightly isolation | |
| 235 | # canary; keep it attached or its scheduled build waits forever. | |
| 236 | ``` | |
| 237 | ||
| 238 | with: | |
| 239 | ||
| 240 | ``` | |
| 241 | # The runner polls as a non-admin account with a runner-scoped key, and | |
| 242 | # claims only the repositories that key is attached to (`repo runner | |
| 243 | # add`): krz/gitbay, krz/hutch, krz/keycask, krz/orgo, krz/skunky-art | |
| 244 | # and cmc/cleberg.net. The attachments are the boundary, so ExecStart | |
| 245 | # names no -repos. To validate a runner change, create a scratch | |
| 246 | # repository, attach this key to it, and run with -repos naming only | |
| 247 | # that repository until the change is proven (Admin wiki, CI runner). | |
| 248 | ``` | |
| 249 | ||
| 250 | - [ ] **Step 2: Correct the Admin procedure** | |
| 251 | ||
| 252 | In `.gitbay/wiki/Admin.org`, replace lines 923-937 (from | |
| 253 | `*Validate podman mode on a scratch repository` through `#+end_src`) | |
| 254 | with: | |
| 255 | ||
| 256 | ```org | |
| 257 | *Validate podman mode on a scratch repository before pointing the runner | |
| 258 | at real ones.* Every deploy that switched the whole instance to | |
| 259 | containers and failed took CI down with it. Instead: create a throwaway | |
| 260 | repository the runner account can read (public, or granted read — a | |
| 261 | private one is "not found" to the runner and the build stays pending), | |
| 262 | give it one job that names the CI image, attach the runner's key to it, | |
| 263 | and deploy the runner with =-repos= naming only that repository. The | |
| 264 | production unit, with its real hardening, then claims nothing else; | |
| 265 | other repositories' builds queue until =-repos= is removed again, which | |
| 266 | is a pause, not an outage. | |
| 267 | ||
| 268 | #+begin_src sh | |
| 269 | gitbay repo create cmc/runner-scratch # then push a .gitbay/ci.yml naming the image | |
| 270 | # on the host: | |
| 271 | gitbay repo runner add cmc/runner-scratch < /var/lib/gitbay-runner/.ssh/id_ed25519.pub | |
| 272 | sed -i 's#^ExecStart=/usr/local/bin/gitbay-runner #&-repos cmc/runner-scratch #' /etc/systemd/system/gitbay-runner.service.d/override.conf | |
| 273 | systemctl daemon-reload && systemctl restart gitbay-runner | |
| 274 | gitbay build log cmc/runner-scratch 1 # green: remove -repos, redeploy, delete the scratch repository | |
| 275 | #+end_src | |
| 276 | ``` | |
| 277 | ||
| 278 | Delete lines 953-954: | |
| 279 | ||
| 280 | ```org | |
| 281 | The nightly canary on =cmc/ci-smoke= only runs if the runner's =-repos= | |
| 282 | names that repository too; a scoped runner claims nothing else. | |
| 283 | ``` | |
| 284 | ||
| 285 | - [ ] **Step 3: Check nothing else names the old repository** | |
| 286 | ||
| 287 | Run: `grep -rn "ci-smoke" --exclude-dir=.git . | grep -v "docs/plans\|docs/specs\|CHANGELOG"` | |
| 288 | Expected: no output. | |
| 289 | ||
| 290 | - [ ] **Step 4: Commit** | |
| 291 | ||
| 292 | ```bash | |
| 293 | git add deploy/gitbay-runner.override.conf .gitbay/wiki/Admin.org | |
| 294 | git commit -S -m "deploy: runner comment and Admin procedure name the real attachments | |
| 295 | ||
| 296 | cmc/ci-smoke no longer exists and ExecStart carries no -repos; the | |
| 297 | scratch-repository check attaches the key and adds -repos for the run. | |
| 298 | ||
| 299 | Closes #287" | |
| 300 | ``` | |
| 301 | ||
| 302 | ### Task 1.2: `webhook add` reads the secret from stdin | |
| 303 | ||
| 304 | **Files:** | |
| 305 | - Modify: `internal/control/webhook.go:1-13` (imports), `:16-24` (registration), `:47-81` (`runWebhookAdd`) | |
| 306 | - Test: `internal/control/webhook_test.go` | |
| 307 | ||
| 308 | **Interfaces:** | |
| 309 | - Produces: `webhook add <owner/name> <url> [--secret -] [--events ...]`, `ReadsStdin: true`. Refusal texts, exit 2: | |
| 310 | `the secret is read from stdin, never argv: pipe it and pass --secret - (printf %s SECRET | ... --secret -)` and | |
| 311 | `no secret on stdin (pipe it: printf %s SECRET | ... --secret -)`. | |
| 312 | ||
| 313 | - [ ] **Step 1: Write the failing test** | |
| 314 | ||
| 315 | Append to `internal/control/webhook_test.go`: | |
| 316 | ||
| 317 | ```go | |
| 318 | // The signing secret arrives on stdin with --secret -, like a build | |
| 319 | // secret: a value on the command line is refused before anything is | |
| 320 | // stored, since argv shows in /proc and in shell history (#284). | |
| 321 | func TestWebhookAddSecretFromStdin(t *testing.T) { | |
| 322 | st, repo, uid := newQueueTestRepo(t) | |
| 323 | run := func(stdin string, argv ...string) (string, int) { | |
| 324 | c, errOut := pruneCtx(st, t.TempDir(), store.User{ID: uid, Username: "alice"}) | |
| 325 | c.Cfg.Limits.WriteRate = -1 | |
| 326 | c.Cfg.Webhooks.AllowLocal = true | |
| 327 | c.Stdin = strings.NewReader(stdin) | |
| 328 | code := Dispatch(c, argv) | |
| 329 | return errOut.String(), code | |
| 330 | } | |
| 331 | msg, code := run("", "webhook", "add", repo.Path(), "http://127.0.0.1/hook", "--secret", "s3cret") | |
| 332 | if code != protocol.ExitUsage || !strings.Contains(msg, "--secret -") { | |
| 333 | t.Fatalf("literal secret: exit %d, %q", code, msg) | |
| 334 | } | |
| 335 | if msg, code := run("", "webhook", "add", repo.Path(), "http://127.0.0.1/hook", "--secret", "-"); code != protocol.ExitUsage || !strings.Contains(msg, "no secret on stdin") { | |
| 336 | t.Fatalf("empty stdin: exit %d, %q", code, msg) | |
| 337 | } | |
| 338 | if hooks, err := st.ListWebhooks(repo.ID); err != nil || len(hooks) != 0 { | |
| 339 | t.Fatalf("a refused add stored %+v (%v)", hooks, err) | |
| 340 | } | |
| 341 | if msg, code := run("s3cret\n", "webhook", "add", repo.Path(), "http://127.0.0.1/hook", "--secret", "-"); code != protocol.ExitOK { | |
| 342 | t.Fatalf("piped secret: exit %d, %q", code, msg) | |
| 343 | } | |
| 344 | // Without --secret nothing reads stdin and the hook is unsigned. | |
| 345 | if msg, code := run("not a secret\n", "webhook", "add", repo.Path(), "http://127.0.0.1/other"); code != protocol.ExitOK { | |
| 346 | t.Fatalf("no secret: exit %d, %q", code, msg) | |
| 347 | } | |
| 348 | hooks, err := st.ListWebhooks(repo.ID) | |
| 349 | if err != nil || len(hooks) != 2 { | |
| 350 | t.Fatalf("hooks: %+v %v", hooks, err) | |
| 351 | } | |
| 352 | if hooks[0].Secret != "s3cret" || hooks[1].Secret != "" { | |
| 353 | t.Fatalf("secrets: %q, %q", hooks[0].Secret, hooks[1].Secret) | |
| 354 | } | |
| 355 | } | |
| 356 | ``` | |
| 357 | ||
| 358 | - [ ] **Step 2: Run it and see it fail** | |
| 359 | ||
| 360 | Run: `go test ./internal/control -run TestWebhookAddSecretFromStdin -count=1` | |
| 361 | Expected: FAIL at `literal secret: exit 0` (the literal is accepted today). | |
| 362 | ||
| 363 | - [ ] **Step 3: Implement** | |
| 364 | ||
| 365 | Imports in `internal/control/webhook.go` gain `"strings"`: | |
| 366 | ||
| 367 | ```go | |
| 368 | import ( | |
| 369 | "errors" | |
| 370 | "fmt" | |
| 371 | "io" | |
| 372 | "strconv" | |
| 373 | "strings" | |
| 374 | ||
| 375 | "gitbay.org/gitbay/internal/policy" | |
| 376 | "gitbay.org/gitbay/internal/protocol" | |
| 377 | "gitbay.org/gitbay/internal/store" | |
| 378 | "gitbay.org/gitbay/internal/webhook" | |
| 379 | ) | |
| 380 | ``` | |
| 381 | ||
| 382 | The registration: | |
| 383 | ||
| 384 | ```go | |
| 385 | register(Command{Path: []string{"webhook", "add"}, | |
| 386 | Summary: "add a webhook", | |
| 387 | Usage: "webhook add <owner/name> <url> [--secret -] [--events push,issue.created|*]", | |
| 388 | Flags: []Flag{ | |
| 389 | {"--secret", "-", "read the secret that signs deliveries from stdin", ""}, | |
| 390 | {"--events", "push,issue.created|*", "which events to send", "*"}, | |
| 391 | }, | |
| 392 | Examples: []string{ | |
| 393 | "webhook add krz/gitbay https://ci.example.org/hook --events push", | |
| 394 | "webhook add krz/gitbay https://ci.example.org/hook --secret - < secret.txt", | |
| 395 | }, | |
| 396 | ReadsStdin: true, | |
| 397 | Run: runWebhookAdd}) | |
| 398 | ``` | |
| 399 | ||
| 400 | `runWebhookAdd`, whole function: | |
| 401 | ||
| 402 | ```go | |
| 403 | func runWebhookAdd(c *Ctx, args []string) int { | |
| 404 | f, err := c.parseArgs(args, flagSpec{Values: []string{"--secret", "--events"}, MaxPos: 2, Usage: "webhook add <owner/name> <url> [--secret -] [--events push,issue.created|*]"}) | |
| 405 | if err != nil { | |
| 406 | return c.fail(protocol.ExitUsage, "%v", err) | |
| 407 | } | |
| 408 | path, url, events := f.pos(0), f.pos(1), "*" | |
| 409 | if f.Has("--events") { | |
| 410 | events = f.Value("--events") | |
| 411 | } | |
| 412 | if path == "" || url == "" { | |
| 413 | return c.usage() | |
| 414 | } | |
| 415 | // Secrets travel on stdin: argv shows in /proc and in shell history. | |
| 416 | if f.Has("--secret") && f.Value("--secret") != "-" { | |
| 417 | return c.fail(protocol.ExitUsage, "the secret is read from stdin, never argv: pipe it and pass --secret - (printf %%s SECRET | ... --secret -)") | |
| 418 | } | |
| 419 | repo, code := resolveRepo(c, path, policy.CanAdmin) | |
| 420 | if code >= 0 { | |
| 421 | return code | |
| 422 | } | |
| 423 | // A name that is not an event is a subscription that never fires, and | |
| 424 | // nothing would ever say so. Checked before the URL, which resolves | |
| 425 | // DNS: a typo here should not need a reachable host to report. | |
| 426 | if code := checkEventNames(c, events); code >= 0 { | |
| 427 | return code | |
| 428 | } | |
| 429 | if err := webhook.ValidateURL(url, c.Cfg.Webhooks.AllowLocal); err != nil { | |
| 430 | // The command line parsed; the value is what the server refuses. | |
| 431 | // Exit 1 carries the reason to every client verbatim (#187). | |
| 432 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 433 | } | |
| 434 | secret := "" | |
| 435 | if f.Has("--secret") { | |
| 436 | raw, err := io.ReadAll(io.LimitReader(c.Stdin, 64<<10)) | |
| 437 | if err != nil { | |
| 438 | return c.fail(protocol.ExitFailure, "reading secret: %v", err) | |
| 439 | } | |
| 440 | secret = strings.TrimRight(string(raw), "\n") | |
| 441 | if secret == "" { | |
| 442 | return c.fail(protocol.ExitUsage, "no secret on stdin (pipe it: printf %%s SECRET | ... --secret -)") | |
| 443 | } | |
| 444 | } | |
| 445 | id, err := c.Store.AddWebhook(repo.ID, url, secret, events) | |
| 446 | if err != nil { | |
| 447 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 448 | } | |
| 449 | return c.emit(map[string]any{"id": id, "url": url, "events": events}, func(w io.Writer) { | |
| 450 | fmt.Fprintf(w, "webhook %d added for %s (%s)\n", id, repo.Path(), events) | |
| 451 | }) | |
| 452 | } | |
| 453 | ``` | |
| 454 | ||
| 455 | - [ ] **Step 4: Run the package** | |
| 456 | ||
| 457 | Run: `go test ./internal/control -count=1` | |
| 458 | Expected: PASS, including `TestWebhookAddRefusedURLIsAFailure`, | |
| 459 | `TestStdinCommandsReadStdin` and the help tests (the usage names | |
| 460 | `--secret`, the flag is described, both examples resolve to | |
| 461 | `webhook add`). | |
| 462 | ||
| 463 | - [ ] **Step 5: Commit** | |
| 464 | ||
| 465 | ```bash | |
| 466 | git add internal/control/webhook.go internal/control/webhook_test.go | |
| 467 | git commit -S -m "webhook: add reads the signing secret from stdin | |
| 468 | ||
| 469 | --secret - reads it; a value on the command line is refused. | |
| 470 | ||
| 471 | Ref #284" | |
| 472 | ``` | |
| 473 | ||
| 474 | ### Task 1.3: the CLI forwards stdin for `--secret -` | |
| 475 | ||
| 476 | **Files:** | |
| 477 | - Modify: `cmd/gitbay/main.go:358-368` (`usesStdin`), `:753` (`webhook add` pass) | |
| 478 | - Test: `cmd/gitbay/stdinpayload_test.go` | |
| 479 | ||
| 480 | **Interfaces:** | |
| 481 | - Consumes: `webhook add ... --secret -` (Task 1.2). | |
| 482 | ||
| 483 | - [ ] **Step 1: Write the failing test** | |
| 484 | ||
| 485 | Append to `cmd/gitbay/stdinpayload_test.go`: | |
| 486 | ||
| 487 | ```go | |
| 488 | // webhook add --secret - reads the secret on the server, so the CLI must | |
| 489 | // forward stdin for it the way it does for --file - (#284). | |
| 490 | func TestUsesStdinForSecretDash(t *testing.T) { | |
| 491 | if !usesStdin([]string{"alice/app", "https://ci.example/hook", "--secret", "-"}) { | |
| 492 | t.Error("--secret - does not forward stdin") | |
| 493 | } | |
| 494 | if usesStdin([]string{"alice/app", "https://ci.example/hook", "--events", "push"}) { | |
| 495 | t.Error("forwarded stdin with no flag asking for it") | |
| 496 | } | |
| 497 | } | |
| 498 | ``` | |
| 499 | ||
| 500 | - [ ] **Step 2: Run it and see it fail** | |
| 501 | ||
| 502 | Run: `go test ./cmd/gitbay -run TestUsesStdinForSecretDash -count=1` | |
| 503 | Expected: FAIL, `--secret - does not forward stdin`. | |
| 504 | ||
| 505 | - [ ] **Step 3: Implement** | |
| 506 | ||
| 507 | `usesStdin`: | |
| 508 | ||
| 509 | ```go | |
| 510 | // usesStdin reports whether the arguments request stdin content. | |
| 511 | func usesStdin(args []string) bool { | |
| 512 | for i, a := range args { | |
| 513 | if (a == "--file" || a == "--key" || a == "--secret") && i+1 < len(args) && args[i+1] == "-" { | |
| 514 | return true | |
| 515 | } | |
| 516 | if a == "--token-stdin" { | |
| 517 | return true | |
| 518 | } | |
| 519 | } | |
| 520 | return false | |
| 521 | } | |
| 522 | ``` | |
| 523 | ||
| 524 | `cmd/gitbay/main.go:753`: | |
| 525 | ||
| 526 | ```go | |
| 527 | pass("add", passOpts{server: []string{"webhook", "add"}, needsRepo: true, stdinOK: true, stdinWhat: "the webhook secret", stdinSecret: true}), | |
| 528 | ``` | |
| 529 | ||
| 530 | - [ ] **Step 4: Run the package** | |
| 531 | ||
| 532 | Run: `go test ./cmd/gitbay -count=1 && go vet ./cmd/gitbay` | |
| 533 | Expected: PASS (coverage, summaries and stdin-payload tests included). | |
| 534 | ||
| 535 | - [ ] **Step 5: Commit** | |
| 536 | ||
| 537 | ```bash | |
| 538 | git add cmd/gitbay/main.go cmd/gitbay/stdinpayload_test.go | |
| 539 | git commit -S -m "cli: webhook add forwards stdin for --secret - | |
| 540 | ||
| 541 | Ref #284" | |
| 542 | ``` | |
| 543 | ||
| 544 | ### Task 1.4: e2e, API wiki and changelog | |
| 545 | ||
| 546 | **Files:** | |
| 547 | - Modify: `e2e/webhook_test.go:115-116` | |
| 548 | - Modify: `.gitbay/wiki/API.org:153` and the paragraph after its `#+end_src` | |
| 549 | - Modify: `CHANGELOG.org` (`* Unreleased`) | |
| 550 | ||
| 551 | **Interfaces:** none. | |
| 552 | ||
| 553 | - [ ] **Step 1: Pipe the secret in the e2e test** | |
| 554 | ||
| 555 | `e2e/webhook_test.go:115-116`: | |
| 556 | ||
| 557 | ```go | |
| 558 | if _, errOut, code := inst.ssh(t, aliceKey, "s3cret\n", | |
| 559 | "webhook", "add", "alice/proj", hookURL, "--secret", "-"); code != 0 { | |
| 560 | ``` | |
| 561 | ||
| 562 | The HMAC check below it (`hmac.New(sha256.New, []byte("s3cret"))`) | |
| 563 | stays: the server trims the trailing newline. | |
| 564 | ||
| 565 | - [ ] **Step 2: Run the e2e test** | |
| 566 | ||
| 567 | Run: `go build ./... && go test ./e2e -run 'TestWebhooks$' -count=1` | |
| 568 | Expected: PASS. | |
| 569 | ||
| 570 | - [ ] **Step 3: API wiki** | |
| 571 | ||
| 572 | `.gitbay/wiki/API.org:153` becomes: | |
| 573 | ||
| 574 | ```org | |
| 575 | printf %s "$SECRET" | gitbay webhook add <url> --secret - [--events push,issue.created] # default * | |
| 576 | ``` | |
| 577 | ||
| 578 | After that block's `#+end_src` and before `** Events`, add: | |
| 579 | ||
| 580 | ```org | |
| 581 | ||
| 582 | The signing secret is read from stdin with =--secret -=; a value on the | |
| 583 | command line is refused, since argv shows in process listings and shell | |
| 584 | history. Over the JSON API it goes in the request's =stdin= field. | |
| 585 | ``` | |
| 586 | ||
| 587 | - [ ] **Step 4: Changelog** | |
| 588 | ||
| 589 | Append to the `* Unreleased` list, directly above `* v1.36.0 — 2026-09-23`: | |
| 590 | ||
| 591 | ```org | |
| 592 | - =webhook add= reads the signing secret from stdin with =--secret -=; | |
| 593 | a value on the command line is refused, since argv shows in process | |
| 594 | listings and shell history. A script that passed the value must pipe | |
| 595 | it: =printf %s "$SECRET" | gitbay webhook add <repo> <url> --secret -= | |
| 596 | (#284). | |
| 597 | ``` | |
| 598 | ||
| 599 | - [ ] **Step 5: Commit, MR, merge** | |
| 600 | ||
| 601 | ```bash | |
| 602 | git add e2e/webhook_test.go .gitbay/wiki/API.org CHANGELOG.org | |
| 603 | git commit -S -m "webhook: document --secret -, pipe it in the e2e test | |
| 604 | ||
| 605 | Closes #284" | |
| 606 | git push -u origin webhook-secret-stdin | |
| 607 | gitbay mr create --source webhook-secret-stdin --target main --title "webhook add reads its secret from stdin; runner comment names the real attachments" | |
| 608 | ``` | |
| 609 | ||
| 610 | Merge `--strategy ff` after CI, delete the branch both places. | |
| 611 | ||
| 612 | --- | |
| 613 | ||
| 614 | # MR 2: import checks and pins its address (branch `import-pin-address`, closes #298) | |
| 615 | ||
| 616 | ### Task 2.1: `internal/gitpin` | |
| 617 | ||
| 618 | **Files:** | |
| 619 | - Create: `internal/gitpin/gitpin.go` | |
| 620 | - Create: `internal/gitpin/gitpin_test.go` | |
| 621 | ||
| 622 | **Interfaces:** | |
| 623 | - Produces: | |
| 624 | - `type Lookup func(ctx context.Context, host string) ([]net.IP, error)` | |
| 625 | - `func LookupIP(ctx context.Context, host string) ([]net.IP, error)` | |
| 626 | - `type Remote struct { URL *url.URL; IPs []net.IP }` | |
| 627 | - `func Resolve(ctx context.Context, lookup Lookup, raw string, allowLocal bool) (Remote, error)` | |
| 628 | - `func (r Remote) Args() []string` | |
| 629 | - `func Env(home string) []string` | |
| 630 | - `func VersionOK(out string) error` | |
| 631 | - `func CheckGit(ctx context.Context) error` | |
| 632 | ||
| 633 | - [ ] **Step 1: Write the failing tests** | |
| 634 | ||
| 635 | `internal/gitpin/gitpin_test.go`: | |
| 636 | ||
| 637 | ```go | |
| 638 | package gitpin | |
| 639 | ||
| 640 | import ( | |
| 641 | "context" | |
| 642 | "net" | |
| 643 | "net/url" | |
| 644 | "slices" | |
| 645 | "strings" | |
| 646 | "testing" | |
| 647 | ) | |
| 648 | ||
| 649 | func answer(ips ...string) Lookup { | |
| 650 | return func(context.Context, string) ([]net.IP, error) { | |
| 651 | var out []net.IP | |
| 652 | for _, s := range ips { | |
| 653 | out = append(out, net.ParseIP(s)) | |
| 654 | } | |
| 655 | return out, nil | |
| 656 | } | |
| 657 | } | |
| 658 | ||
| 659 | func TestResolve(t *testing.T) { | |
| 660 | ctx := context.Background() | |
| 661 | r, err := Resolve(ctx, answer("203.0.113.5"), "https://git.example/x.git", false) | |
| 662 | if err != nil || r.URL.Hostname() != "git.example" || len(r.IPs) != 1 { | |
| 663 | t.Fatalf("public: %+v %v", r, err) | |
| 664 | } | |
| 665 | if _, err := Resolve(ctx, answer("203.0.113.5", "10.0.0.7"), "https://git.example/x.git", false); err == nil || !strings.Contains(err.Error(), "10.0.0.7") { | |
| 666 | t.Fatalf("private: %v", err) | |
| 667 | } | |
| 668 | if _, err := Resolve(ctx, answer("10.0.0.7"), "https://git.example/x.git", true); err != nil { | |
| 669 | t.Fatalf("allow_local: %v", err) | |
| 670 | } | |
| 671 | // An empty resolve list would leave curl to resolve the host itself. | |
| 672 | if _, err := Resolve(ctx, answer(), "https://git.example/x.git", true); err == nil || !strings.Contains(err.Error(), "no address") { | |
| 673 | t.Fatalf("empty answer: %v", err) | |
| 674 | } | |
| 675 | for _, raw := range []string{"git://git.example/x.git", "ssh://git.example/x.git", "file:///etc"} { | |
| 676 | _, err := Resolve(ctx, func(context.Context, string) ([]net.IP, error) { | |
| 677 | t.Fatalf("looked up a host for %s", raw) | |
| 678 | return nil, nil | |
| 679 | }, raw, true) | |
| 680 | if err == nil || !strings.Contains(err.Error(), "not http or https") { | |
| 681 | t.Errorf("%s: %v", raw, err) | |
| 682 | } | |
| 683 | } | |
| 684 | } | |
| 685 | ||
| 686 | func TestArgs(t *testing.T) { | |
| 687 | u, _ := url.Parse("https://git.example/x.git") | |
| 688 | got := Remote{u, []net.IP{net.ParseIP("203.0.113.5"), net.ParseIP("2001:db8::1")}}.Args() | |
| 689 | want := []string{"-c", "http.followRedirects=false", | |
| 690 | "-c", "http.curloptResolve=git.example:443:203.0.113.5,[2001:db8::1]"} | |
| 691 | if !slices.Equal(got, want) { | |
| 692 | t.Fatalf("https: %q", got) | |
| 693 | } | |
| 694 | u, _ = url.Parse("http://git.example:8080/x.git") | |
| 695 | if got := (Remote{u, []net.IP{net.ParseIP("203.0.113.5")}}).Args(); got[3] != "http.curloptResolve=git.example:8080:203.0.113.5" { | |
| 696 | t.Fatalf("http with port: %q", got) | |
| 697 | } | |
| 698 | // An address literal is its own resolution; there is nothing to pin. | |
| 699 | u, _ = url.Parse("https://203.0.113.5/x.git") | |
| 700 | if got := (Remote{u, []net.IP{net.ParseIP("203.0.113.5")}}).Args(); !slices.Equal(got, []string{"-c", "http.followRedirects=false"}) { | |
| 701 | t.Fatalf("literal: %q", got) | |
| 702 | } | |
| 703 | } | |
| 704 | ||
| 705 | func TestEnv(t *testing.T) { | |
| 706 | want := []string{"GIT_TERMINAL_PROMPT=0", "HOME=/srv/gitbay", | |
| 707 | "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=/dev/null"} | |
| 708 | if got := Env("/srv/gitbay"); !slices.Equal(got, want) { | |
| 709 | t.Fatalf("Env = %q", got) | |
| 710 | } | |
| 711 | } | |
| 712 | ||
| 713 | func TestVersionOK(t *testing.T) { | |
| 714 | for _, s := range []string{"git version 2.37.0", "git version 2.47.3", "git version 2.39.5 (Apple Git-154)", | |
| 715 | "git version 2.45.2.windows.1", "git version 3.0.0\n"} { | |
| 716 | if err := VersionOK(s); err != nil { | |
| 717 | t.Errorf("%q: %v", s, err) | |
| 718 | } | |
| 719 | } | |
| 720 | for _, s := range []string{"git version 2.36.9", "git version 1.99.0", "git version 2", "nonsense", ""} { | |
| 721 | if err := VersionOK(s); err == nil { | |
| 722 | t.Errorf("%q accepted", s) | |
| 723 | } | |
| 724 | } | |
| 725 | } | |
| 726 | ``` | |
| 727 | ||
| 728 | - [ ] **Step 2: Run them and see them fail** | |
| 729 | ||
| 730 | Run: `go test ./internal/gitpin -count=1` | |
| 731 | Expected: build failure, `undefined: Lookup` / `undefined: Resolve`. | |
| 732 | ||
| 733 | - [ ] **Step 3: Implement** | |
| 734 | ||
| 735 | `internal/gitpin/gitpin.go`: | |
| 736 | ||
| 737 | ```go | |
| 738 | // Package gitpin runs git against a user-supplied http or https remote | |
| 739 | // only at addresses resolved and checked immediately before: mirror | |
| 740 | // sync (#279) and repo import (#298). | |
| 741 | package gitpin | |
| 742 | ||
| 743 | import ( | |
| 744 | "context" | |
| 745 | "fmt" | |
| 746 | "net" | |
| 747 | "net/url" | |
| 748 | "os/exec" | |
| 749 | "strconv" | |
| 750 | "strings" | |
| 751 | ||
| 752 | "gitbay.org/gitbay/internal/toolpath" | |
| 753 | "gitbay.org/gitbay/internal/webhook" | |
| 754 | ) | |
| 755 | ||
| 756 | // Lookup resolves a host to its addresses. | |
| 757 | type Lookup func(ctx context.Context, host string) ([]net.IP, error) | |
| 758 | ||
| 759 | // LookupIP is the system resolver. | |
| 760 | func LookupIP(ctx context.Context, host string) ([]net.IP, error) { | |
| 761 | return net.DefaultResolver.LookupIP(ctx, "ip", host) | |
| 762 | } | |
| 763 | ||
| 764 | // Remote is a URL whose host resolved to IPs, every one of which passed | |
| 765 | // the address check. | |
| 766 | type Remote struct { | |
| 767 | URL *url.URL | |
| 768 | IPs []net.IP | |
| 769 | } | |
| 770 | ||
| 771 | // Resolve parses raw, requires http or https, resolves the host with | |
| 772 | // lookup, and refuses it when it resolves to nothing or, unless | |
| 773 | // allowLocal, to any private or local address. | |
| 774 | func Resolve(ctx context.Context, lookup Lookup, raw string, allowLocal bool) (Remote, error) { | |
| 775 | u, err := url.Parse(raw) | |
| 776 | if err != nil { | |
| 777 | return Remote{}, err | |
| 778 | } | |
| 779 | if u.Scheme != "https" && u.Scheme != "http" { | |
| 780 | return Remote{}, fmt.Errorf("URL scheme %q is not http or https", u.Scheme) | |
| 781 | } | |
| 782 | host := u.Hostname() | |
| 783 | if host == "" { | |
| 784 | return Remote{}, fmt.Errorf("URL has no host") | |
| 785 | } | |
| 786 | ips, err := lookup(ctx, host) | |
| 787 | if err != nil { | |
| 788 | return Remote{}, fmt.Errorf("resolving %s: %w", host, err) | |
| 789 | } | |
| 790 | if len(ips) == 0 { | |
| 791 | // An empty resolve list would leave curl to resolve the host itself. | |
| 792 | return Remote{}, fmt.Errorf("%s resolves to no address", host) | |
| 793 | } | |
| 794 | if err := webhook.CheckAddrs(host, ips, allowLocal); err != nil { | |
| 795 | return Remote{}, err | |
| 796 | } | |
| 797 | return Remote{URL: u, IPs: ips}, nil | |
| 798 | } | |
| 799 | ||
| 800 | // Args are git's leading -c options for r: curl's resolve list pins | |
| 801 | // the host to the checked addresses, and with redirects off a server | |
| 802 | // cannot send git on to a host nobody checked. An address literal | |
| 803 | // needs no pin. | |
| 804 | func (r Remote) Args() []string { | |
| 805 | args := []string{"-c", "http.followRedirects=false"} | |
| 806 | host := r.URL.Hostname() | |
| 807 | if net.ParseIP(host) != nil { | |
| 808 | return args | |
| 809 | } | |
| 810 | port := r.URL.Port() | |
| 811 | if port == "" { | |
| 812 | port = "443" | |
| 813 | if r.URL.Scheme == "http" { | |
| 814 | port = "80" | |
| 815 | } | |
| 816 | } | |
| 817 | addrs := make([]string, len(r.IPs)) | |
| 818 | for i, ip := range r.IPs { | |
| 819 | if ip.To4() == nil { | |
| 820 | addrs[i] = "[" + ip.String() + "]" | |
| 821 | } else { | |
| 822 | addrs[i] = ip.String() | |
| 823 | } | |
| 824 | } | |
| 825 | return append(args, "-c", "http.curloptResolve="+host+":"+port+":"+strings.Join(addrs, ",")) | |
| 826 | } | |
| 827 | ||
| 828 | // Env is git's whole environment for a pinned remote. No system or | |
| 829 | // global gitconfig: a proxy, URL rewrite or redirect setting there | |
| 830 | // would take git around the pin. | |
| 831 | func Env(home string) []string { | |
| 832 | return []string{"GIT_TERMINAL_PROMPT=0", "HOME=" + home, | |
| 833 | "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL=/dev/null"} | |
| 834 | } | |
| 835 | ||
| 836 | // VersionOK accepts the output of `git version` for git 2.37 or later, | |
| 837 | // the first release with http.curloptResolve. An older git ignores the | |
| 838 | // setting and would resolve the host itself. | |
| 839 | func VersionOK(out string) error { | |
| 840 | fields := strings.Fields(out) | |
| 841 | if len(fields) >= 3 && fields[0] == "git" && fields[1] == "version" { | |
| 842 | parts := strings.Split(fields[2], ".") | |
| 843 | if len(parts) >= 2 { | |
| 844 | major, err1 := strconv.Atoi(parts[0]) | |
| 845 | minor, err2 := strconv.Atoi(parts[1]) | |
| 846 | if err1 == nil && err2 == nil { | |
| 847 | if major > 2 || major == 2 && minor >= 37 { | |
| 848 | return nil | |
| 849 | } | |
| 850 | return fmt.Errorf("git %s is older than 2.37 and cannot pin remote addresses", fields[2]) | |
| 851 | } | |
| 852 | } | |
| 853 | } | |
| 854 | return fmt.Errorf("cannot read git version from %q", strings.TrimSpace(out)) | |
| 855 | } | |
| 856 | ||
| 857 | // CheckGit runs the server's git and refuses one that cannot pin. | |
| 858 | func CheckGit(ctx context.Context) error { | |
| 859 | out, err := exec.CommandContext(ctx, toolpath.Look("git"), "version").Output() | |
| 860 | if err != nil { | |
| 861 | return fmt.Errorf("running git version: %v", err) | |
| 862 | } | |
| 863 | return VersionOK(string(out)) | |
| 864 | } | |
| 865 | ``` | |
| 866 | ||
| 867 | `webhook` imports only `store`, and `store` imports neither `gitpin` | |
| 868 | nor `webhook`: no cycle. | |
| 869 | ||
| 870 | - [ ] **Step 4: Run the package** | |
| 871 | ||
| 872 | Run: `go test ./internal/gitpin -count=1 && go vet ./internal/gitpin` | |
| 873 | Expected: PASS. | |
| 874 | ||
| 875 | - [ ] **Step 5: Commit** | |
| 876 | ||
| 877 | ```bash | |
| 878 | git add internal/gitpin | |
| 879 | git commit -S -m "gitpin: resolve, check and pin a git remote | |
| 880 | ||
| 881 | The mirror worker's resolve-check-pin, as a package import can share. | |
| 882 | ||
| 883 | Ref #298" | |
| 884 | ``` | |
| 885 | ||
| 886 | ### Task 2.2: mirror sync goes through `gitpin` | |
| 887 | ||
| 888 | **Files:** | |
| 889 | - Modify: `internal/mirror/mirror.go:8-26` (imports), `:46-79` (`New`, `Run`), `:102-167` (`sync`); delete `:169-215` (`pinArgs`, `gitVersionOK`) | |
| 890 | - Modify: `internal/mirror/mirror_test.go:3-19` (imports), `:141-160` (`TestSweepRefusesWithAnOldGit`); delete `:162-176` (`TestGitVersionOK`) and `:242-260` (`TestPinArgs`) | |
| 891 | ||
| 892 | **Interfaces:** | |
| 893 | - Consumes: `gitpin.LookupIP`, `gitpin.Resolve`, `gitpin.Remote.Args`, `gitpin.Env`, `gitpin.VersionOK`, `gitpin.CheckGit` (Task 2.1). | |
| 894 | - Produces: no new names; `Worker.Lookup` keeps its type. | |
| 895 | ||
| 896 | - [ ] **Step 1: Point the old-git test at the moved function** | |
| 897 | ||
| 898 | In `TestSweepRefusesWithAnOldGit`, replace | |
| 899 | ||
| 900 | ```go | |
| 901 | w.gitErr = gitVersionOK("git version 2.36.1") | |
| 902 | ``` | |
| 903 | ||
| 904 | with | |
| 905 | ||
| 906 | ```go | |
| 907 | w.gitErr = gitpin.VersionOK("git version 2.36.1") | |
| 908 | ``` | |
| 909 | ||
| 910 | Delete `TestGitVersionOK` and `TestPinArgs` (now `gitpin`'s | |
| 911 | `TestVersionOK` and `TestArgs`). Add | |
| 912 | `"gitbay.org/gitbay/internal/gitpin"` to the test imports. | |
| 913 | ||
| 914 | - [ ] **Step 2: Baseline** | |
| 915 | ||
| 916 | This task is a refactor: the mirror tests are the check, green before | |
| 917 | and after. | |
| 918 | ||
| 919 | Run: `go test ./internal/mirror -count=1` | |
| 920 | Expected: PASS (`gitpin.VersionOK` exists from Task 2.1; the package's | |
| 921 | own `pinArgs` and `gitVersionOK` still exist until Step 3). | |
| 922 | ||
| 923 | - [ ] **Step 3: Implement** | |
| 924 | ||
| 925 | Imports: | |
| 926 | ||
| 927 | ```go | |
| 928 | import ( | |
| 929 | "context" | |
| 930 | "fmt" | |
| 931 | "log/slog" | |
| 932 | "net" | |
| 933 | "os" | |
| 934 | "os/exec" | |
| 935 | "path/filepath" | |
| 936 | "time" | |
| 937 | ||
| 938 | "gitbay.org/gitbay/internal/config" | |
| 939 | "gitbay.org/gitbay/internal/control" | |
| 940 | "gitbay.org/gitbay/internal/gitpin" | |
| 941 | "gitbay.org/gitbay/internal/store" | |
| 942 | "gitbay.org/gitbay/internal/toolpath" | |
| 943 | ) | |
| 944 | ``` | |
| 945 | ||
| 946 | `New` sets `Lookup: gitpin.LookupIP` in place of the inline closure: | |
| 947 | ||
| 948 | ```go | |
| 949 | return &Worker{St: st, Cfg: cfg, Tick: tick, Lookup: gitpin.LookupIP} | |
| 950 | ``` | |
| 951 | ||
| 952 | `Run`, the first lines through the `slog.Error`: | |
| 953 | ||
| 954 | ```go | |
| 955 | func (w *Worker) Run(ctx context.Context) { | |
| 956 | if err := gitpin.CheckGit(ctx); err != nil { | |
| 957 | w.gitErr = fmt.Errorf("mirrors disabled: %w", err) | |
| 958 | slog.Error("mirror: not syncing", "err", w.gitErr) | |
| 959 | } | |
| 960 | t := time.NewTicker(w.Tick) | |
| 961 | ``` | |
| 962 | ||
| 963 | `sync`, whole function: | |
| 964 | ||
| 965 | ```go | |
| 966 | func (w *Worker) sync(m store.Mirror) error { | |
| 967 | repo, err := w.St.RepoByID(m.RepoID) | |
| 968 | if err != nil { | |
| 969 | return err | |
| 970 | } | |
| 971 | dir := control.RepoDir(w.Cfg.Server.Root, repo.OwnerName, repo.Name) | |
| 972 | ||
| 973 | ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) | |
| 974 | defer cancel() | |
| 975 | // The URL was checked when saved, but DNS can answer differently | |
| 976 | // now. Check what it resolves to at sync time, then let git connect | |
| 977 | // to exactly those addresses. | |
| 978 | remote, err := gitpin.Resolve(ctx, w.Lookup, m.URL, w.Cfg.Webhooks.AllowLocal) | |
| 979 | if err != nil { | |
| 980 | return err | |
| 981 | } | |
| 982 | ||
| 983 | env := gitpin.Env(w.Cfg.Server.Root) | |
| 984 | if m.Token != "" { | |
| 985 | askpass := filepath.Join(w.Cfg.Server.Root, "mirror-askpass.sh") | |
| 986 | if err := os.WriteFile(askpass, []byte(askpassScript), 0o700); err != nil { | |
| 987 | return err | |
| 988 | } | |
| 989 | user := m.Username | |
| 990 | if user == "" { | |
| 991 | user = "x-access-token" | |
| 992 | } | |
| 993 | env = append(env, | |
| 994 | "GIT_ASKPASS="+askpass, | |
| 995 | "GITBAY_MIRROR_USER="+user, | |
| 996 | "GITBAY_MIRROR_TOKEN="+m.Token) | |
| 997 | } | |
| 998 | ||
| 999 | args := append(remote.Args(), "-C", dir) | |
| 1000 | if m.Direction == "push" { | |
| 1001 | // Branches and tags only: internal refs (merge-requests) stay home. | |
| 1002 | args = append(args, "push", "--prune", m.URL, | |
| 1003 | "+refs/heads/*:refs/heads/*", "+refs/tags/*:refs/tags/*") | |
| 1004 | } else { | |
| 1005 | args = append(args, "fetch", "--prune", m.URL, | |
| 1006 | "+refs/heads/*:refs/heads/*", "+refs/tags/*:refs/tags/*") | |
| 1007 | } | |
| 1008 | cmd := exec.CommandContext(ctx, toolpath.Look("git"), args...) | |
| 1009 | cmd.Env = env | |
| 1010 | if out, err := cmd.CombinedOutput(); err != nil { | |
| 1011 | return fmt.Errorf("git %s: %v: %.300s", m.Direction, err, out) | |
| 1012 | } | |
| 1013 | return nil | |
| 1014 | } | |
| 1015 | ``` | |
| 1016 | ||
| 1017 | Delete `pinArgs` and `gitVersionOK`. | |
| 1018 | ||
| 1019 | - [ ] **Step 4: Run the mirror tests and the mirror e2e** | |
| 1020 | ||
| 1021 | Run: `go test ./internal/mirror ./internal/gitpin -count=1 && go vet ./internal/mirror` | |
| 1022 | Expected: PASS. `TestSyncRefusesANonHTTPScheme` matches | |
| 1023 | `not http or https`, `TestSyncRefusesAnEmptyAnswer` matches | |
| 1024 | `no address`, `TestSweepRefusesWithAnOldGit` matches `2.37`. | |
| 1025 | ||
| 1026 | Run: `go build ./... && go test ./e2e -run TestMirrors -count=1` | |
| 1027 | Expected: PASS. | |
| 1028 | ||
| 1029 | - [ ] **Step 5: Commit** | |
| 1030 | ||
| 1031 | ```bash | |
| 1032 | git add internal/mirror | |
| 1033 | git commit -S -m "mirror: sync through gitpin | |
| 1034 | ||
| 1035 | Ref #298" | |
| 1036 | ``` | |
| 1037 | ||
| 1038 | ### Task 2.3: `repo import` accepts http(s) only and pins the address | |
| 1039 | ||
| 1040 | **Files:** | |
| 1041 | - Modify: `internal/gitutil/gitutil.go:218-233` (`FetchMirror`), `:256-273` (`RemoteDefaultBranch`) | |
| 1042 | - Modify: `internal/control/import.go:3-16` (imports), `:80-116`, `:146-158` | |
| 1043 | - Create: `internal/control/import_test.go` | |
| 1044 | ||
| 1045 | **Interfaces:** | |
| 1046 | - Consumes: `gitpin.CheckGit`, `gitpin.Resolve`, `gitpin.Remote.Args`, `gitpin.Env`, `gitpin.Lookup`, `gitpin.LookupIP` (Task 2.1). | |
| 1047 | - Produces: | |
| 1048 | - `func FetchMirror(ctx context.Context, dir, url string, errW io.Writer, pin, env []string) error` | |
| 1049 | - `func RemoteDefaultBranch(ctx context.Context, url string, pin, env []string) (string, error)` | |
| 1050 | - `var importLookup gitpin.Lookup = gitpin.LookupIP` (package `control`; tests replace it) | |
| 1051 | - Refusal: `import fetches over http:// and https:// only; use the repository's https:// URL` (exit 2). | |
| 1052 | ||
| 1053 | - [ ] **Step 1: Write the failing tests** | |
| 1054 | ||
| 1055 | `internal/control/import_test.go`: | |
| 1056 | ||
| 1057 | ```go | |
| 1058 | package control | |
| 1059 | ||
| 1060 | import ( | |
| 1061 | "bytes" | |
| 1062 | "context" | |
| 1063 | "net" | |
| 1064 | "net/http/cgi" | |
| 1065 | "net/http/httptest" | |
| 1066 | "net/url" | |
| 1067 | "os" | |
| 1068 | "path/filepath" | |
| 1069 | "slices" | |
| 1070 | "strings" | |
| 1071 | "testing" | |
| 1072 | ||
| 1073 | "gitbay.org/gitbay/internal/protocol" | |
| 1074 | "gitbay.org/gitbay/internal/store" | |
| 1075 | ) | |
| 1076 | ||
| 1077 | func importCtx(t *testing.T, allowLocal bool) (*Ctx, *bytes.Buffer, *store.Store, string) { | |
| 1078 | t.Helper() | |
| 1079 | st, _, uid := newQueueTestRepo(t) | |
| 1080 | root := t.TempDir() | |
| 1081 | c, errOut := pruneCtx(st, root, store.User{ID: uid, Username: "alice"}) | |
| 1082 | c.Cfg.Limits.WriteRate = -1 | |
| 1083 | c.Cfg.Limits.CloneTimeoutSec = 60 | |
| 1084 | c.Cfg.Webhooks.AllowLocal = allowLocal | |
| 1085 | c.Stdin = strings.NewReader("") | |
| 1086 | return c, errOut, st, root | |
| 1087 | } | |
| 1088 | ||
| 1089 | // importUpstream serves a bare repository with one commit on main over | |
| 1090 | // smart HTTP and returns its URL and that commit. | |
| 1091 | func importUpstream(t *testing.T) (string, string) { | |
| 1092 | t.Helper() | |
| 1093 | git := gitRunner(t) | |
| 1094 | parent := t.TempDir() | |
| 1095 | bare := filepath.Join(parent, "remote.git") | |
| 1096 | work := filepath.Join(parent, "work") | |
| 1097 | git(parent, "init", "-q", "--bare", "--initial-branch=main", bare) | |
| 1098 | git(parent, "init", "-q", "--initial-branch=main", work) | |
| 1099 | git(work, "commit", "-q", "--allow-empty", "-m", "one") | |
| 1100 | git(work, "push", "-q", bare, "main") | |
| 1101 | sha := strings.TrimSpace(git(work, "rev-parse", "HEAD")) | |
| 1102 | execPath := strings.TrimSpace(git(parent, "--exec-path")) | |
| 1103 | srv := httptest.NewServer(&cgi.Handler{ | |
| 1104 | Path: filepath.Join(execPath, "git-http-backend"), | |
| 1105 | Env: []string{"GIT_PROJECT_ROOT=" + parent, "GIT_HTTP_EXPORT_ALL=1"}, | |
| 1106 | }) | |
| 1107 | t.Cleanup(srv.Close) | |
| 1108 | return srv.URL + "/remote.git", sha | |
| 1109 | } | |
| 1110 | ||
| 1111 | // git:// cannot be held to a checked address, so import refuses it | |
| 1112 | // before creating anything (#298). | |
| 1113 | func TestRepoImportRefusesGitScheme(t *testing.T) { | |
| 1114 | c, errOut, st, _ := importCtx(t, true) | |
| 1115 | code := Dispatch(c, []string{"repo", "import", "alice/x", "--from", "git://example.org/x.git"}) | |
| 1116 | if code != protocol.ExitUsage || !strings.Contains(errOut.String(), "use the repository's https:// URL") { | |
| 1117 | t.Fatalf("exit %d, %q", code, errOut.String()) | |
| 1118 | } | |
| 1119 | if _, err := st.RepoByPath("alice/x"); err == nil { | |
| 1120 | t.Fatal("a refused import created a repository") | |
| 1121 | } | |
| 1122 | } | |
| 1123 | ||
| 1124 | // A source on loopback is refused on a default instance, and nothing is | |
| 1125 | // left behind. | |
| 1126 | func TestRepoImportRefusesALocalAddress(t *testing.T) { | |
| 1127 | c, errOut, st, root := importCtx(t, false) | |
| 1128 | code := Dispatch(c, []string{"repo", "import", "alice/x", "--from", "http://127.0.0.1:9/x.git"}) | |
| 1129 | if code != protocol.ExitFailure || !strings.Contains(errOut.String(), "127.0.0.1") { | |
| 1130 | t.Fatalf("exit %d, %q", code, errOut.String()) | |
| 1131 | } | |
| 1132 | if _, err := st.RepoByPath("alice/x"); err == nil { | |
| 1133 | t.Fatal("a refused import created a repository") | |
| 1134 | } | |
| 1135 | if _, err := os.Stat(RepoDir(root, "alice", "x")); !os.IsNotExist(err) { | |
| 1136 | t.Fatalf("a refused import left a directory: %v", err) | |
| 1137 | } | |
| 1138 | } | |
| 1139 | ||
| 1140 | // import.test does not resolve; the import works only because git was | |
| 1141 | // pinned to the address import looked up and checked. | |
| 1142 | func TestRepoImportConnectsToTheCheckedAddress(t *testing.T) { | |
| 1143 | remote, sha := importUpstream(t) | |
| 1144 | u, _ := url.Parse(remote) | |
| 1145 | c, errOut, _, root := importCtx(t, true) | |
| 1146 | var asked []string | |
| 1147 | prev := importLookup | |
| 1148 | importLookup = func(ctx context.Context, host string) ([]net.IP, error) { | |
| 1149 | asked = append(asked, host) | |
| 1150 | return []net.IP{net.ParseIP("127.0.0.1")}, nil | |
| 1151 | } | |
| 1152 | t.Cleanup(func() { importLookup = prev }) | |
| 1153 | ||
| 1154 | code := Dispatch(c, []string{"repo", "import", "alice/copy", "--from", "http://import.test:" + u.Port() + "/remote.git"}) | |
| 1155 | if code != protocol.ExitOK { | |
| 1156 | t.Fatalf("exit %d: %s", code, errOut.String()) | |
| 1157 | } | |
| 1158 | if out := c.Stdout.(*bytes.Buffer).String(); !strings.Contains(out, "default main") { | |
| 1159 | t.Fatalf("output %q: the default branch was not read through the pin", out) | |
| 1160 | } | |
| 1161 | got := strings.TrimSpace(gitRunner(t)(RepoDir(root, "alice", "copy"), "rev-parse", "refs/heads/main")) | |
| 1162 | if got != sha { | |
| 1163 | t.Fatalf("main = %s, want %s", got, sha) | |
| 1164 | } | |
| 1165 | if !slices.Equal(asked, []string{"import.test"}) { | |
| 1166 | t.Fatalf("looked up %v", asked) | |
| 1167 | } | |
| 1168 | } | |
| 1169 | ``` | |
| 1170 | ||
| 1171 | - [ ] **Step 2: Run them and see them fail** | |
| 1172 | ||
| 1173 | Run: `go test ./internal/control -run 'TestRepoImport' -count=1` | |
| 1174 | Expected: build failure, `undefined: importLookup`. | |
| 1175 | ||
| 1176 | - [ ] **Step 3: Implement the gitutil half** | |
| 1177 | ||
| 1178 | `internal/gitutil/gitutil.go`, `FetchMirror` with its comment: | |
| 1179 | ||
| 1180 | ```go | |
| 1181 | // FetchMirror pulls all branches, tags, and notes from a foreign URL into | |
| 1182 | // the bare repository at dir, forcing updates. Progress streams to errW so | |
| 1183 | // an interactive caller can watch. pin is git's leading -c options | |
| 1184 | // (gitpin.Remote.Args); env is git's whole environment and carries | |
| 1185 | // credentials via GIT_ASKPASS: the URL itself must never contain them. | |
| 1186 | func FetchMirror(ctx context.Context, dir, url string, errW io.Writer, pin, env []string) error { | |
| 1187 | args := append(append([]string{}, pin...), "-C", dir, "fetch", "--progress", "--no-write-fetch-head", url, | |
| 1188 | "+refs/heads/*:refs/heads/*", | |
| 1189 | "+refs/tags/*:refs/tags/*", | |
| 1190 | "+refs/notes/*:refs/notes/*") | |
| 1191 | cmd := exec.CommandContext(ctx, toolpath.Look("git"), args...) | |
| 1192 | cmd.Env = env | |
| 1193 | cmd.Stderr = errW | |
| 1194 | if err := cmd.Run(); err != nil { | |
| 1195 | return fmt.Errorf("fetch from %s: %w", url, err) | |
| 1196 | } | |
| 1197 | return nil | |
| 1198 | } | |
| 1199 | ``` | |
| 1200 | ||
| 1201 | `RemoteDefaultBranch`, the comment and the first three lines: | |
| 1202 | ||
| 1203 | ```go | |
| 1204 | // RemoteDefaultBranch asks the remote which branch HEAD points at. pin | |
| 1205 | // and env are as for FetchMirror. | |
| 1206 | func RemoteDefaultBranch(ctx context.Context, url string, pin, env []string) (string, error) { | |
| 1207 | args := append(append([]string{}, pin...), "ls-remote", "--symref", url, "HEAD") | |
| 1208 | cmd := exec.CommandContext(ctx, toolpath.Look("git"), args...) | |
| 1209 | cmd.Env = env | |
| 1210 | out, err := cmd.Output() | |
| 1211 | ``` | |
| 1212 | ||
| 1213 | The rest of `RemoteDefaultBranch` is unchanged. | |
| 1214 | ||
| 1215 | - [ ] **Step 4: Implement the import half** | |
| 1216 | ||
| 1217 | Imports in `internal/control/import.go`: | |
| 1218 | ||
| 1219 | ```go | |
| 1220 | import ( | |
| 1221 | "bufio" | |
| 1222 | "context" | |
| 1223 | "fmt" | |
| 1224 | "io" | |
| 1225 | "os" | |
| 1226 | "path/filepath" | |
| 1227 | "strings" | |
| 1228 | "time" | |
| 1229 | ||
| 1230 | "gitbay.org/gitbay/internal/gitpin" | |
| 1231 | "gitbay.org/gitbay/internal/gitutil" | |
| 1232 | "gitbay.org/gitbay/internal/policy" | |
| 1233 | "gitbay.org/gitbay/internal/protocol" | |
| 1234 | ) | |
| 1235 | ``` | |
| 1236 | ||
| 1237 | After `askpassScript` (line 39), add: | |
| 1238 | ||
| 1239 | ```go | |
| 1240 | // importLookup resolves an import's host; tests replace it. | |
| 1241 | var importLookup gitpin.Lookup = gitpin.LookupIP | |
| 1242 | ``` | |
| 1243 | ||
| 1244 | Replace lines 80-116 (from `// Scheme allowlist.` through the closing | |
| 1245 | brace of the token `if`/`else`) with: | |
| 1246 | ||
| 1247 | ```go | |
| 1248 | // http and https only. git:// has no equivalent of curl's resolve | |
| 1249 | // list, so its connection cannot be held to a checked address; | |
| 1250 | // file:// would read the server's filesystem, and ssh:// would use | |
| 1251 | // the server's own keys. | |
| 1252 | if !strings.HasPrefix(from, "https://") && !strings.HasPrefix(from, "http://") { | |
| 1253 | return c.fail(protocol.ExitUsage, "import fetches over http:// and https:// only; use the repository's https:// URL") | |
| 1254 | } | |
| 1255 | if strings.ContainsAny(from, "@") { | |
| 1256 | // Credentials belong on stdin, not in the URL where they would | |
| 1257 | // land in process listings and logs. | |
| 1258 | return c.fail(protocol.ExitUsage, "do not embed credentials in the URL; use --token-stdin") | |
| 1259 | } | |
| 1260 | ||
| 1261 | // Resolve and check the host now and hold git to those addresses, | |
| 1262 | // as mirror sync does (#298). | |
| 1263 | timeout := time.Duration(c.Cfg.Limits.CloneTimeoutSec) * time.Second | |
| 1264 | ctx, cancel := context.WithTimeout(context.Background(), timeout) | |
| 1265 | defer cancel() | |
| 1266 | if err := gitpin.CheckGit(ctx); err != nil { | |
| 1267 | return c.fail(protocol.ExitFailure, "import unavailable: %v", err) | |
| 1268 | } | |
| 1269 | remote, err := gitpin.Resolve(ctx, importLookup, from, c.Cfg.Webhooks.AllowLocal) | |
| 1270 | if err != nil { | |
| 1271 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 1272 | } | |
| 1273 | ||
| 1274 | // The token is read from stdin and handed to git via GIT_ASKPASS and | |
| 1275 | // the environment — never argv, never the database, never a log line. | |
| 1276 | env := gitpin.Env(c.Cfg.Server.Root) | |
| 1277 | if tokenStdin { | |
| 1278 | token, err := bufio.NewReader(io.LimitReader(c.Stdin, 4096)).ReadString('\n') | |
| 1279 | if err != nil && err != io.EOF { | |
| 1280 | return c.fail(protocol.ExitFailure, "reading token: %v", err) | |
| 1281 | } | |
| 1282 | token = strings.TrimSpace(token) | |
| 1283 | if token == "" { | |
| 1284 | return c.fail(protocol.ExitUsage, "--token-stdin given but stdin held no token") | |
| 1285 | } | |
| 1286 | askpass := filepath.Join(c.Cfg.Server.Root, "askpass.sh") | |
| 1287 | if err := os.WriteFile(askpass, []byte(askpassScript), 0o700); err != nil { | |
| 1288 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 1289 | } | |
| 1290 | env = append(env, "GIT_ASKPASS="+askpass, "GITBAY_IMPORT_TOKEN="+token) | |
| 1291 | } | |
| 1292 | ``` | |
| 1293 | ||
| 1294 | Replace lines 146-158 (the old `timeout`/`ctx`/`cancel`, the fetch | |
| 1295 | and the default-branch lookup) with: | |
| 1296 | ||
| 1297 | ```go | |
| 1298 | fmt.Fprintf(c.Stderr, "importing %s into %s ...\n", from, path) | |
| 1299 | if err := gitutil.FetchMirror(ctx, dir, from, c.Stderr, remote.Args(), env); err != nil { | |
| 1300 | cleanup() | |
| 1301 | return c.fail(protocol.ExitFailure, "import failed: %v", err) | |
| 1302 | } | |
| 1303 | ||
| 1304 | branch, err := gitutil.RemoteDefaultBranch(ctx, from, remote.Args(), env) | |
| 1305 | if err != nil { | |
| 1306 | branch = "main" // remote gone quiet after the fetch; keep the default | |
| 1307 | } | |
| 1308 | ``` | |
| 1309 | ||
| 1310 | - [ ] **Step 5: Run the packages** | |
| 1311 | ||
| 1312 | Run: `go build ./... && go vet ./internal/control ./internal/gitutil && go test ./internal/control ./internal/gitutil -count=1` | |
| 1313 | Expected: PASS, including the three `TestRepoImport*` tests. | |
| 1314 | ||
| 1315 | - [ ] **Step 6: Commit** | |
| 1316 | ||
| 1317 | ```bash | |
| 1318 | git add internal/gitutil/gitutil.go internal/control/import.go internal/control/import_test.go | |
| 1319 | git commit -S -m "import: http(s) only, address checked and pinned like a mirror sync | |
| 1320 | ||
| 1321 | git:// is refused: it cannot be held to a checked address. | |
| 1322 | ||
| 1323 | Ref #298" | |
| 1324 | ``` | |
| 1325 | ||
| 1326 | ### Task 2.4: e2e, wiki and changelog | |
| 1327 | ||
| 1328 | **Files:** | |
| 1329 | - Modify: `e2e/import_test.go:14`, `:85-92`, `:105-114` | |
| 1330 | - Modify: `.gitbay/wiki/Threat-Model.org:91-103`, `Architecture/09-Controls.org:78`, `Architecture/10-Known-Gaps.org:17`, `Architecture/02-Components.org` (packages table), `Admin.org:196-198`, `:201`, `Users.org:257-259` | |
| 1331 | - Modify: `CHANGELOG.org` | |
| 1332 | ||
| 1333 | **Interfaces:** none. | |
| 1334 | ||
| 1335 | - [ ] **Step 1: Update the e2e test** | |
| 1336 | ||
| 1337 | `e2e/import_test.go:14`: the test imports from its own instance on | |
| 1338 | `127.0.0.1`, which a default instance now refuses: | |
| 1339 | ||
| 1340 | ```go | |
| 1341 | inst := startInstanceWith(t, "[webhooks]\nallow_local = true\n") | |
| 1342 | ``` | |
| 1343 | ||
| 1344 | Delete lines 85-92 (`// Import over git:// too.` through the closing | |
| 1345 | brace of the `git://` import). | |
| 1346 | ||
| 1347 | The refusal table, lines 105-114: | |
| 1348 | ||
| 1349 | ```go | |
| 1350 | // Refusals: bad scheme, git://, credentials in URL, existing name, foreign owner. | |
| 1351 | cases := []struct { | |
| 1352 | args []string | |
| 1353 | want string | |
| 1354 | }{ | |
| 1355 | {[]string{"repo", "import", "alice/x", "--from", "file:///etc"}, "http:// and https:// only"}, | |
| 1356 | {[]string{"repo", "import", "alice/x", "--from", "git://127.0.0.1:1/alice/src.git"}, "use the repository's https:// URL"}, | |
| 1357 | {[]string{"repo", "import", "alice/x", "--from", "https://token@github.com/a/b"}, "--token-stdin"}, | |
| 1358 | {[]string{"repo", "import", "alice/mirror", "--from", httpURL}, "already exists"}, | |
| 1359 | {[]string{"repo", "import", "bob/x", "--from", httpURL}, "not you and not an organization"}, | |
| 1360 | } | |
| 1361 | ``` | |
| 1362 | ||
| 1363 | - [ ] **Step 2: Run it** | |
| 1364 | ||
| 1365 | Run: `go build ./... && go test ./e2e -run 'TestRepoImport$' -count=1` | |
| 1366 | Expected: PASS. | |
| 1367 | ||
| 1368 | - [ ] **Step 3: Wiki** | |
| 1369 | ||
| 1370 | `Threat-Model.org`, the paragraph under `* Network-facing request | |
| 1371 | forgery` (lines 91-103) becomes: | |
| 1372 | ||
| 1373 | ```org | |
| 1374 | Webhook delivery, GitHub-history import =--api-base=, mirror remotes | |
| 1375 | and =repo import --from=, which make the *server* open an outbound | |
| 1376 | connection to a user-supplied address, pass the same SSRF guard: the | |
| 1377 | scheme must be http/https and, unless =webhooks.allow_local= is set, | |
| 1378 | the resolved address must not be loopback, private, shared | |
| 1379 | (100.64.0.0/10), link-local, or multicast. The webhook dialer re-checks | |
| 1380 | at connect time, and mirror sync and =repo import= resolve and check | |
| 1381 | immediately before running git and pin it to the checked addresses | |
| 1382 | (=internal/gitpin=), so a DNS answer that changes after validation | |
| 1383 | still cannot reach private space. Redirects are never followed. | |
| 1384 | =repo import= refuses =git://=, which cannot be pinned. | |
| 1385 | ``` | |
| 1386 | ||
| 1387 | `Architecture/09-Controls.org:78`: | |
| 1388 | ||
| 1389 | ```org | |
| 1390 | | SSRF protection on user-supplied URLs | in place | webhooks at save and connect; mirrors at save and sync, =repo import= before its fetch, git pinned to the checked address (=internal/gitpin=) | | |
| 1391 | ``` | |
| 1392 | ||
| 1393 | `Architecture/10-Known-Gaps.org`: delete the `#298` row (line 17). | |
| 1394 | ||
| 1395 | `Architecture/02-Components.org`, after the `=internal/mirror=` row: | |
| 1396 | ||
| 1397 | ```org | |
| 1398 | | =internal/gitpin= | Resolves and checks a user-supplied http(s) remote and pins git to the checked addresses; mirror sync and =repo import=. | | |
| 1399 | ``` | |
| 1400 | ||
| 1401 | `Admin.org:196-198` (`[webhooks]`): | |
| 1402 | ||
| 1403 | ```org | |
| 1404 | - =allow_local= (false) — permit webhook, mirror and import targets on | |
| 1405 | loopback, private, shared (100.64.0.0/10), link-local or multicast | |
| 1406 | addresses. Leave off unless you know why you need it (SSRF). | |
| 1407 | ``` | |
| 1408 | ||
| 1409 | `Admin.org:201` (`[limits]`): | |
| 1410 | ||
| 1411 | ```org | |
| 1412 | - =clone_timeout= (3600s) — cap on =repo import= fetches. An import | |
| 1413 | takes http and https URLs only, passes the same address check as a | |
| 1414 | mirror sync and is pinned the same way, so it needs git 2.37 too. | |
| 1415 | ``` | |
| 1416 | ||
| 1417 | `Users.org`, after the `#+end_src` at line 257 and before | |
| 1418 | `Sourcehut has no API of that shape`: | |
| 1419 | ||
| 1420 | ```org | |
| 1421 | =repo import= fetches over http and https only, from an address that | |
| 1422 | passes the same check as a webhook target; a =git://= URL is refused, | |
| 1423 | so use the repository's https URL. | |
| 1424 | ||
| 1425 | ``` | |
| 1426 | ||
| 1427 | - [ ] **Step 4: Changelog** | |
| 1428 | ||
| 1429 | Append to the `* Unreleased` list, directly above `* v1.36.0 — 2026-09-23`: | |
| 1430 | ||
| 1431 | ```org | |
| 1432 | - =repo import --from= takes http and https URLs only; =git://= is | |
| 1433 | refused, since its connection cannot be held to a checked address. | |
| 1434 | The host is resolved and checked like a mirror's, git connects only | |
| 1435 | to the checked addresses with redirects off, and the system and | |
| 1436 | global gitconfig are ignored. A source that redirects (a renamed | |
| 1437 | repository) fails; import from the URL it redirects to. Needs git | |
| 1438 | 2.37 or later on the server (#298). | |
| 1439 | ``` | |
| 1440 | ||
| 1441 | - [ ] **Step 5: Commit, MR, merge** | |
| 1442 | ||
| 1443 | ```bash | |
| 1444 | git add e2e/import_test.go .gitbay/wiki/Threat-Model.org .gitbay/wiki/Architecture/09-Controls.org \ | |
| 1445 | .gitbay/wiki/Architecture/10-Known-Gaps.org .gitbay/wiki/Architecture/02-Components.org \ | |
| 1446 | .gitbay/wiki/Admin.org .gitbay/wiki/Users.org CHANGELOG.org | |
| 1447 | git commit -S -m "import: document the address check; e2e imports with allow_local | |
| 1448 | ||
| 1449 | Closes #298" | |
| 1450 | git push -u origin import-pin-address | |
| 1451 | gitbay mr create --source import-pin-address --target main --title "repo import: http(s) only, address checked and pinned" | |
| 1452 | ``` | |
| 1453 | ||
| 1454 | Merge `--strategy ff` after CI, delete the branch both places. | |
| 1455 | ||
| 1456 | --- | |
| 1457 | ||
| 1458 | # MR 3: LFS tokens die with their key (branch `lfs-token-key`, closes #285) | |
| 1459 | ||
| 1460 | ### Task 3.1: the token names the key that obtained it | |
| 1461 | ||
| 1462 | **Files:** | |
| 1463 | - Modify: `internal/lfs/lfs.go:122-169` (`Sign`, `Verify`) | |
| 1464 | - Create: `internal/lfs/lfs_test.go` | |
| 1465 | - Modify: `internal/sshd/lfs.go:16-78` (`runLFSAuthenticate`) | |
| 1466 | - Modify: `internal/sshd/sshd.go:499` (call site) | |
| 1467 | - Modify: `internal/httpd/lfs.go:38-58` (`lfsAuth`), `:99`, `:130` (`lfsBatch`), `:180-185` (`lfsDownload`), `:199-204` (`lfsUpload`) | |
| 1468 | ||
| 1469 | **Interfaces:** | |
| 1470 | - Produces: | |
| 1471 | - `func Sign(secret []byte, repoID, keyID int64, op string, now time.Time) string` | |
| 1472 | - `type Grant struct { RepoID, KeyID int64; Op string }` | |
| 1473 | - `func Verify(secret []byte, token string, now time.Time) (Grant, bool)` | |
| 1474 | - `func runLFSAuthenticate(cfg config.Config, st *store.Store, user store.User, key store.SSHKey, argv []string, stdout, stderr io.Writer) int` | |
| 1475 | - `func (s *Server) lfsAuth(r *http.Request, repo store.Repo) (op string, keyID int64)` | |
| 1476 | - Consumed by Task 3.2: `lfsAuth`'s signature, `Grant.KeyID`. | |
| 1477 | ||
| 1478 | - [ ] **Step 1: Write the failing tests** | |
| 1479 | ||
| 1480 | `internal/lfs/lfs_test.go`: | |
| 1481 | ||
| 1482 | ```go | |
| 1483 | package lfs | |
| 1484 | ||
| 1485 | import ( | |
| 1486 | "crypto/hmac" | |
| 1487 | "crypto/sha256" | |
| 1488 | "encoding/base64" | |
| 1489 | "fmt" | |
| 1490 | "testing" | |
| 1491 | "time" | |
| 1492 | ) | |
| 1493 | ||
| 1494 | func TestTokenCarriesTheKey(t *testing.T) { | |
| 1495 | secret := []byte("secret") | |
| 1496 | now := time.Now() | |
| 1497 | tok := Sign(secret, 7, 42, "upload", now) | |
| 1498 | g, ok := Verify(secret, tok, now) | |
| 1499 | if !ok || g != (Grant{RepoID: 7, KeyID: 42, Op: "upload"}) { | |
| 1500 | t.Fatalf("Verify = %+v, %v", g, ok) | |
| 1501 | } | |
| 1502 | if _, ok := Verify(secret, tok, now.Add(TokenTTL+time.Second)); ok { | |
| 1503 | t.Error("an expired token verified") | |
| 1504 | } | |
| 1505 | if _, ok := Verify([]byte("other"), tok, now); ok { | |
| 1506 | t.Error("a token verified under another secret") | |
| 1507 | } | |
| 1508 | if g, ok := Verify(secret, Sign(secret, 7, 0, "download", now), now); !ok || g.KeyID != 0 { | |
| 1509 | t.Errorf("anonymous grant = %+v, %v", g, ok) | |
| 1510 | } | |
| 1511 | } | |
| 1512 | ||
| 1513 | // A token minted before tokens named their key has three fields. It is | |
| 1514 | // refused, not read as a grant bound to no key (#285). | |
| 1515 | func TestUnboundTokenRefused(t *testing.T) { | |
| 1516 | secret := []byte("secret") | |
| 1517 | payload := fmt.Sprintf("%d:%s:%d", 7, "upload", time.Now().Add(TokenTTL).Unix()) | |
| 1518 | mac := hmac.New(sha256.New, secret) | |
| 1519 | mac.Write([]byte(payload)) | |
| 1520 | tok := base64.RawURLEncoding.EncodeToString([]byte(payload)) + "." + | |
| 1521 | base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) | |
| 1522 | if g, ok := Verify(secret, tok, time.Now()); ok { | |
| 1523 | t.Fatalf("a pre-upgrade token verified: %+v", g) | |
| 1524 | } | |
| 1525 | } | |
| 1526 | ``` | |
| 1527 | ||
| 1528 | - [ ] **Step 2: Run them and see them fail** | |
| 1529 | ||
| 1530 | Run: `go test ./internal/lfs -count=1` | |
| 1531 | Expected: build failure, `too many arguments in call to Sign` / | |
| 1532 | `undefined: Grant`. | |
| 1533 | ||
| 1534 | - [ ] **Step 3: Implement the lfs package** | |
| 1535 | ||
| 1536 | Replace lines 122-169 of `internal/lfs/lfs.go`: | |
| 1537 | ||
| 1538 | ```go | |
| 1539 | // Tokens bridge SSH authentication to the HTTP endpoints: stateless, | |
| 1540 | // HMAC-signed, scoped to one repo and one operation, short-lived, and | |
| 1541 | // bound to the SSH key that obtained them, which must still be live | |
| 1542 | // when the token is used (#285). The secret persists in the settings | |
| 1543 | // table so tokens survive restarts. | |
| 1544 | ||
| 1545 | const TokenTTL = time.Hour | |
| 1546 | ||
| 1547 | // Sign mints a token for op ("download" or "upload") on repoID, bound | |
| 1548 | // to keyID: the SSH key, user or deploy, that asked for it, or 0 for an | |
| 1549 | // anonymous download of a public repository. | |
| 1550 | func Sign(secret []byte, repoID, keyID int64, op string, now time.Time) string { | |
| 1551 | payload := fmt.Sprintf("%d:%d:%s:%d", repoID, keyID, op, now.Add(TokenTTL).Unix()) | |
| 1552 | mac := hmac.New(sha256.New, secret) | |
| 1553 | mac.Write([]byte(payload)) | |
| 1554 | return base64.RawURLEncoding.EncodeToString([]byte(payload)) + "." + | |
| 1555 | base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) | |
| 1556 | } | |
| 1557 | ||
| 1558 | // Grant is what a verified token authorizes. | |
| 1559 | type Grant struct { | |
| 1560 | RepoID int64 | |
| 1561 | KeyID int64 // 0: an anonymous download of a public repository | |
| 1562 | Op string | |
| 1563 | } | |
| 1564 | ||
| 1565 | // Verify checks a token's MAC, shape and expiry. A token from before | |
| 1566 | // tokens named their key does not verify. | |
| 1567 | func Verify(secret []byte, token string, now time.Time) (Grant, bool) { | |
| 1568 | payloadB64, macB64, found := strings.Cut(token, ".") | |
| 1569 | if !found { | |
| 1570 | return Grant{}, false | |
| 1571 | } | |
| 1572 | payload, err := base64.RawURLEncoding.DecodeString(payloadB64) | |
| 1573 | if err != nil { | |
| 1574 | return Grant{}, false | |
| 1575 | } | |
| 1576 | gotMAC, err := base64.RawURLEncoding.DecodeString(macB64) | |
| 1577 | if err != nil { | |
| 1578 | return Grant{}, false | |
| 1579 | } | |
| 1580 | mac := hmac.New(sha256.New, secret) | |
| 1581 | mac.Write(payload) | |
| 1582 | if !hmac.Equal(mac.Sum(nil), gotMAC) { | |
| 1583 | return Grant{}, false | |
| 1584 | } | |
| 1585 | parts := strings.Split(string(payload), ":") | |
| 1586 | if len(parts) != 4 { | |
| 1587 | return Grant{}, false | |
| 1588 | } | |
| 1589 | repoID, err1 := strconv.ParseInt(parts[0], 10, 64) | |
| 1590 | keyID, err2 := strconv.ParseInt(parts[1], 10, 64) | |
| 1591 | exp, err3 := strconv.ParseInt(parts[3], 10, 64) | |
| 1592 | if err1 != nil || err2 != nil || err3 != nil || keyID < 0 || now.Unix() > exp { | |
| 1593 | return Grant{}, false | |
| 1594 | } | |
| 1595 | if parts[2] != "download" && parts[2] != "upload" { | |
| 1596 | return Grant{}, false | |
| 1597 | } | |
| 1598 | return Grant{RepoID: repoID, KeyID: keyID, Op: parts[2]}, true | |
| 1599 | } | |
| 1600 | ``` | |
| 1601 | ||
| 1602 | - [ ] **Step 4: Update the callers** | |
| 1603 | ||
| 1604 | `internal/sshd/lfs.go`, lines 16-29 (doc comment, signature and the | |
| 1605 | usage check) become: | |
| 1606 | ||
| 1607 | ```go | |
| 1608 | // runLFSAuthenticate answers the git-lfs client's SSH probe: | |
| 1609 | // | |
| 1610 | // git-lfs-authenticate <path> download|upload | |
| 1611 | // | |
| 1612 | // with the HTTP endpoint and a short-lived repo- and operation-scoped | |
| 1613 | // token. Access rules mirror the git transports: download needs read, | |
| 1614 | // upload needs write; deploy keys authorize by their binding alone, and | |
| 1615 | // every denial on an invisible repo reads as nonexistence. The token | |
| 1616 | // names key, so it stops working when the key does (#285). | |
| 1617 | func runLFSAuthenticate(cfg config.Config, st *store.Store, user store.User, key store.SSHKey, | |
| 1618 | argv []string, stdout, stderr io.Writer) int { | |
| 1619 | scope := key.Scope | |
| 1620 | if len(argv) != 3 || (argv[2] != "download" && argv[2] != "upload") { | |
| 1621 | fmt.Fprintln(stderr, "usage: git-lfs-authenticate <path> download|upload") | |
| 1622 | return protocol.ExitUsage | |
| 1623 | } | |
| 1624 | ``` | |
| 1625 | ||
| 1626 | Lines 30-69 stay as they are (they read `scope`). Line 70 becomes: | |
| 1627 | ||
| 1628 | ```go | |
| 1629 | token := lfs.Sign([]byte(secret), repo.ID, key.ID, op, time.Now()) | |
| 1630 | ``` | |
| 1631 | ||
| 1632 | `internal/sshd/sshd.go:499`: | |
| 1633 | ||
| 1634 | ```go | |
| 1635 | return runLFSAuthenticate(cfg, st, user, key, argv, stdout, stderr) | |
| 1636 | ``` | |
| 1637 | ||
| 1638 | `internal/httpd/lfs.go`, `lfsAuth` for now carries the key id through | |
| 1639 | without checking it (Task 3.2 adds the check): | |
| 1640 | ||
| 1641 | ```go | |
| 1642 | // lfsAuth resolves what the request may do to the repo: "upload", | |
| 1643 | // "download", or "" for no access, and the key the grant rests on (0 | |
| 1644 | // for none). Tokens are repo-scoped; without one, public repos allow | |
| 1645 | // anonymous download only. | |
| 1646 | func (s *Server) lfsAuth(r *http.Request, repo store.Repo) (string, int64) { | |
| 1647 | auth := r.Header.Get("Authorization") | |
| 1648 | if tok, ok := strings.CutPrefix(auth, "Bearer "); ok { | |
| 1649 | secret, err := s.lfsSecret() | |
| 1650 | if err != nil { | |
| 1651 | return "", 0 | |
| 1652 | } | |
| 1653 | g, ok := lfs.Verify(secret, tok, time.Now()) | |
| 1654 | if !ok || g.RepoID != repo.ID { | |
| 1655 | return "", 0 | |
| 1656 | } | |
| 1657 | return g.Op, g.KeyID | |
| 1658 | } | |
| 1659 | if repo.Visibility == "public" { | |
| 1660 | return "download", 0 | |
| 1661 | } | |
| 1662 | return "", 0 | |
| 1663 | } | |
| 1664 | ``` | |
| 1665 | ||
| 1666 | `lfsBatch` line 99 and line 130: | |
| 1667 | ||
| 1668 | ```go | |
| 1669 | granted, keyID := s.lfsAuth(r, repo) | |
| 1670 | ``` | |
| 1671 | ||
| 1672 | ```go | |
| 1673 | transferToken := lfs.Sign(secret, repo.ID, keyID, req.Operation, time.Now()) | |
| 1674 | ``` | |
| 1675 | ||
| 1676 | `lfsDownload`, lines 181-185: | |
| 1677 | ||
| 1678 | ```go | |
| 1679 | repo, err := s.st.RepoByPath(r.PathValue("owner") + "/" + r.PathValue("repo")) | |
| 1680 | if err != nil { | |
| 1681 | lfsError(w, http.StatusNotFound, "not found") | |
| 1682 | return | |
| 1683 | } | |
| 1684 | if op, _ := s.lfsAuth(r, repo); op == "" { | |
| 1685 | lfsError(w, http.StatusNotFound, "not found") | |
| 1686 | return | |
| 1687 | } | |
| 1688 | ``` | |
| 1689 | ||
| 1690 | `lfsUpload`, lines 200-204: | |
| 1691 | ||
| 1692 | ```go | |
| 1693 | repo, err := s.st.RepoByPath(r.PathValue("owner") + "/" + r.PathValue("repo")) | |
| 1694 | if err != nil { | |
| 1695 | lfsError(w, http.StatusNotFound, "not found") | |
| 1696 | return | |
| 1697 | } | |
| 1698 | if op, _ := s.lfsAuth(r, repo); op != "upload" { | |
| 1699 | lfsError(w, http.StatusNotFound, "not found") | |
| 1700 | return | |
| 1701 | } | |
| 1702 | ``` | |
| 1703 | ||
| 1704 | - [ ] **Step 5: Run the packages** | |
| 1705 | ||
| 1706 | Run: `go build ./... && go vet ./internal/lfs ./internal/sshd ./internal/httpd && go test ./internal/lfs ./internal/sshd ./internal/httpd -count=1` | |
| 1707 | Expected: PASS. | |
| 1708 | ||
| 1709 | - [ ] **Step 6: Commit** | |
| 1710 | ||
| 1711 | ```bash | |
| 1712 | git add internal/lfs internal/sshd/lfs.go internal/sshd/sshd.go internal/httpd/lfs.go | |
| 1713 | git commit -S -m "lfs: a transfer token names the key that obtained it | |
| 1714 | ||
| 1715 | A pre-upgrade token, which names none, no longer verifies. | |
| 1716 | ||
| 1717 | Ref #285" | |
| 1718 | ``` | |
| 1719 | ||
| 1720 | ### Task 3.2: an LFS request checks the token's key is live | |
| 1721 | ||
| 1722 | **Files:** | |
| 1723 | - Modify: `internal/httpd/lfs.go` (`lfsAuth`) | |
| 1724 | - Create: `internal/httpd/lfsauth_test.go` | |
| 1725 | ||
| 1726 | **Interfaces:** | |
| 1727 | - Consumes: `lfs.Sign`, `lfs.Verify`, `Grant` (Task 3.1); `store.LiveSSHKeys(ids []int64) (map[int64]bool, error)`. | |
| 1728 | ||
| 1729 | - [ ] **Step 1: Write the failing tests** | |
| 1730 | ||
| 1731 | `internal/httpd/lfsauth_test.go`: | |
| 1732 | ||
| 1733 | ```go | |
| 1734 | package httpd | |
| 1735 | ||
| 1736 | import ( | |
| 1737 | "net/http" | |
| 1738 | "net/http/httptest" | |
| 1739 | "testing" | |
| 1740 | "time" | |
| 1741 | ||
| 1742 | "gitbay.org/gitbay/internal/lfs" | |
| 1743 | "gitbay.org/gitbay/internal/store" | |
| 1744 | ) | |
| 1745 | ||
| 1746 | func lfsRequest(tok string) *http.Request { | |
| 1747 | r := httptest.NewRequest("GET", "/alice/app.git/info/lfs/objects/x", nil) | |
| 1748 | if tok != "" { | |
| 1749 | r.Header.Set("Authorization", "Bearer "+tok) | |
| 1750 | } | |
| 1751 | return r | |
| 1752 | } | |
| 1753 | ||
| 1754 | func lfsTestRepo(t *testing.T, st *store.Store, uid int64, name, visibility string) store.Repo { | |
| 1755 | t.Helper() | |
| 1756 | id, err := st.CreateRepo("user", uid, name, visibility) | |
| 1757 | if err != nil { | |
| 1758 | t.Fatal(err) | |
| 1759 | } | |
| 1760 | repo, err := st.RepoByID(id) | |
| 1761 | if err != nil { | |
| 1762 | t.Fatal(err) | |
| 1763 | } | |
| 1764 | return repo | |
| 1765 | } | |
| 1766 | ||
| 1767 | // A token works only while its key does: removed, expired or on a | |
| 1768 | // disabled account, the key takes its tokens with it (#285). | |
| 1769 | func TestLFSTokenNeedsALiveKey(t *testing.T) { | |
| 1770 | s, st, u := newTokenTestServer(t) | |
| 1771 | repo := lfsTestRepo(t, st, u.ID, "app", "private") | |
| 1772 | secret, err := s.lfsSecret() | |
| 1773 | if err != nil { | |
| 1774 | t.Fatal(err) | |
| 1775 | } | |
| 1776 | addKey := func(fp string, exp *time.Time) int64 { | |
| 1777 | t.Helper() | |
| 1778 | if err := st.AddSSHKeyFrom(u.ID, fp, "ssh-ed25519", []byte(fp), "full", "", store.KeyOrigin{ExpiresAt: exp}); err != nil { | |
| 1779 | t.Fatal(err) | |
| 1780 | } | |
| 1781 | k, err := st.SSHKeyByFingerprint(fp) | |
| 1782 | if err != nil { | |
| 1783 | t.Fatal(err) | |
| 1784 | } | |
| 1785 | return k.ID | |
| 1786 | } | |
| 1787 | ||
| 1788 | live := addKey("SHA256:live", nil) | |
| 1789 | tok := lfs.Sign(secret, repo.ID, live, "upload", time.Now()) | |
| 1790 | if op, key := s.lfsAuth(lfsRequest(tok), repo); op != "upload" || key != live { | |
| 1791 | t.Fatalf("live key: %q, %d", op, key) | |
| 1792 | } | |
| 1793 | ||
| 1794 | past := time.Now().Add(-time.Minute) | |
| 1795 | expired := addKey("SHA256:expired", &past) | |
| 1796 | if op, _ := s.lfsAuth(lfsRequest(lfs.Sign(secret, repo.ID, expired, "upload", time.Now())), repo); op != "" { | |
| 1797 | t.Errorf("expired key: %q", op) | |
| 1798 | } | |
| 1799 | ||
| 1800 | if err := st.RemoveSSHKey(u.ID, "SHA256:live"); err != nil { | |
| 1801 | t.Fatal(err) | |
| 1802 | } | |
| 1803 | if op, _ := s.lfsAuth(lfsRequest(tok), repo); op != "" { | |
| 1804 | t.Errorf("removed key: %q", op) | |
| 1805 | } | |
| 1806 | ||
| 1807 | other := addKey("SHA256:other", nil) | |
| 1808 | otherTok := lfs.Sign(secret, repo.ID, other, "download", time.Now()) | |
| 1809 | if op, _ := s.lfsAuth(lfsRequest(otherTok), repo); op != "download" { | |
| 1810 | t.Fatalf("second key before disable: %q", op) | |
| 1811 | } | |
| 1812 | if err := st.SetUserDisabled(u.ID, true); err != nil { | |
| 1813 | t.Fatal(err) | |
| 1814 | } | |
| 1815 | if op, _ := s.lfsAuth(lfsRequest(otherTok), repo); op != "" { | |
| 1816 | t.Errorf("disabled account: %q", op) | |
| 1817 | } | |
| 1818 | } | |
| 1819 | ||
| 1820 | // A token with no key comes from an anonymous batch on a public | |
| 1821 | // repository and is worth exactly what anonymous is: a download, while | |
| 1822 | // the repository is public. | |
| 1823 | func TestLFSAnonymousTokenOnlyDownloadsPublic(t *testing.T) { | |
| 1824 | s, st, u := newTokenTestServer(t) | |
| 1825 | pub := lfsTestRepo(t, st, u.ID, "big", "public") | |
| 1826 | priv := lfsTestRepo(t, st, u.ID, "vault", "private") | |
| 1827 | secret, err := s.lfsSecret() | |
| 1828 | if err != nil { | |
| 1829 | t.Fatal(err) | |
| 1830 | } | |
| 1831 | now := time.Now() | |
| 1832 | if op, _ := s.lfsAuth(lfsRequest(lfs.Sign(secret, pub.ID, 0, "download", now)), pub); op != "download" { | |
| 1833 | t.Errorf("public download: %q", op) | |
| 1834 | } | |
| 1835 | if op, _ := s.lfsAuth(lfsRequest(lfs.Sign(secret, pub.ID, 0, "upload", now)), pub); op != "" { | |
| 1836 | t.Errorf("anonymous upload: %q", op) | |
| 1837 | } | |
| 1838 | if op, _ := s.lfsAuth(lfsRequest(lfs.Sign(secret, priv.ID, 0, "download", now)), priv); op != "" { | |
| 1839 | t.Errorf("private download: %q", op) | |
| 1840 | } | |
| 1841 | } | |
| 1842 | ``` | |
| 1843 | ||
| 1844 | - [ ] **Step 2: Run them and see them fail** | |
| 1845 | ||
| 1846 | Run: `go test ./internal/httpd -run 'TestLFS' -count=1` | |
| 1847 | Expected: FAIL: `expired key: "upload"`, `removed key: "upload"`, | |
| 1848 | `disabled account: "download"`, `anonymous upload: "upload"`, | |
| 1849 | `private download: "download"`. | |
| 1850 | ||
| 1851 | - [ ] **Step 3: Implement** | |
| 1852 | ||
| 1853 | `lfsAuth`: | |
| 1854 | ||
| 1855 | ```go | |
| 1856 | // lfsAuth resolves what the request may do to the repo: "upload", | |
| 1857 | // "download", or "" for no access, and the key the grant rests on (0 | |
| 1858 | // for none). A token is bound to the SSH key that obtained it and | |
| 1859 | // works only while that key is registered, unexpired and on an enabled | |
| 1860 | // account (#285). Without one, public repos allow anonymous download | |
| 1861 | // only. | |
| 1862 | func (s *Server) lfsAuth(r *http.Request, repo store.Repo) (string, int64) { | |
| 1863 | auth := r.Header.Get("Authorization") | |
| 1864 | if tok, ok := strings.CutPrefix(auth, "Bearer "); ok { | |
| 1865 | secret, err := s.lfsSecret() | |
| 1866 | if err != nil { | |
| 1867 | return "", 0 | |
| 1868 | } | |
| 1869 | g, ok := lfs.Verify(secret, tok, time.Now()) | |
| 1870 | if !ok || g.RepoID != repo.ID { | |
| 1871 | return "", 0 | |
| 1872 | } | |
| 1873 | if g.KeyID == 0 { | |
| 1874 | // Minted by an anonymous batch: worth what anonymous is. | |
| 1875 | if g.Op == "download" && repo.Visibility == "public" { | |
| 1876 | return "download", 0 | |
| 1877 | } | |
| 1878 | return "", 0 | |
| 1879 | } | |
| 1880 | live, err := s.st.LiveSSHKeys([]int64{g.KeyID}) | |
| 1881 | if err != nil || !live[g.KeyID] { | |
| 1882 | return "", 0 | |
| 1883 | } | |
| 1884 | return g.Op, g.KeyID | |
| 1885 | } | |
| 1886 | if repo.Visibility == "public" { | |
| 1887 | return "download", 0 | |
| 1888 | } | |
| 1889 | return "", 0 | |
| 1890 | } | |
| 1891 | ``` | |
| 1892 | ||
| 1893 | - [ ] **Step 4: Run the package** | |
| 1894 | ||
| 1895 | Run: `go test ./internal/httpd -count=1 && go vet ./internal/httpd` | |
| 1896 | Expected: PASS. | |
| 1897 | ||
| 1898 | - [ ] **Step 5: Commit** | |
| 1899 | ||
| 1900 | ```bash | |
| 1901 | git add internal/httpd/lfs.go internal/httpd/lfsauth_test.go | |
| 1902 | git commit -S -m "lfs: refuse a token whose key was removed, expired or disabled | |
| 1903 | ||
| 1904 | Ref #285" | |
| 1905 | ``` | |
| 1906 | ||
| 1907 | ### Task 3.3: e2e, wiki and changelog | |
| 1908 | ||
| 1909 | **Files:** | |
| 1910 | - Modify: `e2e/lfs_test.go` (new test after `TestLFS`) | |
| 1911 | - Modify: `.gitbay/wiki/Architecture/05-Identity-and-Access.org:25`, `Architecture/04-Trust-Boundaries.org:109-116`, `Architecture/09-Controls.org:28`, `Threat-Model.org:44-53` | |
| 1912 | - Modify: `CHANGELOG.org` | |
| 1913 | ||
| 1914 | **Interfaces:** | |
| 1915 | - Consumes: e2e helpers `startInstance`, `inst.newKey`, `inst.admin`, `inst.ssh`, `fingerprint` (`e2e/revoke_test.go:38`). | |
| 1916 | ||
| 1917 | - [ ] **Step 1: Write the e2e test** | |
| 1918 | ||
| 1919 | Append to `e2e/lfs_test.go`: | |
| 1920 | ||
| 1921 | ```go | |
| 1922 | // A transfer token works only while the key that obtained it does: | |
| 1923 | // removing the key ends it before its hour is up (#285). No git-lfs | |
| 1924 | // client needed: the token comes from git-lfs-authenticate over SSH. | |
| 1925 | func TestLFSTokenEndsWithItsKey(t *testing.T) { | |
| 1926 | t.Parallel() | |
| 1927 | inst := startInstance(t) | |
| 1928 | aliceKey := inst.newKey(t, "alice") | |
| 1929 | inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub") | |
| 1930 | if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/vault", "--private"); code != 0 { | |
| 1931 | t.Fatalf("repo create: %s", errOut) | |
| 1932 | } | |
| 1933 | spare := inst.newKey(t, "spare") | |
| 1934 | pub, err := os.ReadFile(spare + ".pub") | |
| 1935 | if err != nil { | |
| 1936 | t.Fatal(err) | |
| 1937 | } | |
| 1938 | if _, errOut, code := inst.ssh(t, aliceKey, string(pub), "keys", "add"); code != 0 { | |
| 1939 | t.Fatalf("keys add: %s", errOut) | |
| 1940 | } | |
| 1941 | out, errOut, code := inst.ssh(t, spare, "", "git-lfs-authenticate", "alice/vault", "download") | |
| 1942 | if code != 0 { | |
| 1943 | t.Fatalf("authenticate: %s", errOut) | |
| 1944 | } | |
| 1945 | var grant struct { | |
| 1946 | Header map[string]string `json:"header"` | |
| 1947 | } | |
| 1948 | if err := json.Unmarshal([]byte(out), &grant); err != nil { | |
| 1949 | t.Fatalf("authenticate JSON: %v\n%s", err, out) | |
| 1950 | } | |
| 1951 | batch := func() int { | |
| 1952 | body := fmt.Sprintf(`{"operation":"download","transfers":["basic"],"objects":[{"oid":%q,"size":4}]}`, strings.Repeat("ab", 32)) | |
| 1953 | req, _ := http.NewRequest("POST", | |
| 1954 | fmt.Sprintf("http://127.0.0.1:%d/alice/vault.git/info/lfs/objects/batch", inst.httpPort), | |
| 1955 | strings.NewReader(body)) | |
| 1956 | req.Header.Set("Content-Type", "application/vnd.git-lfs+json") | |
| 1957 | req.Header.Set("Authorization", grant.Header["Authorization"]) | |
| 1958 | resp, err := http.DefaultClient.Do(req) | |
| 1959 | if err != nil { | |
| 1960 | t.Fatal(err) | |
| 1961 | } | |
| 1962 | resp.Body.Close() | |
| 1963 | return resp.StatusCode | |
| 1964 | } | |
| 1965 | if code := batch(); code != 200 { | |
| 1966 | t.Fatalf("batch with a live key: %d", code) | |
| 1967 | } | |
| 1968 | if _, errOut, code := inst.ssh(t, aliceKey, "", "keys", "remove", fingerprint(t, spare+".pub")); code != 0 { | |
| 1969 | t.Fatalf("keys remove: %s", errOut) | |
| 1970 | } | |
| 1971 | if code := batch(); code != 404 { | |
| 1972 | t.Fatalf("batch after the key was removed: %d, want 404", code) | |
| 1973 | } | |
| 1974 | } | |
| 1975 | ``` | |
| 1976 | ||
| 1977 | The file already imports `encoding/json`, `fmt`, `net/http`, `os` and | |
| 1978 | `strings`. | |
| 1979 | ||
| 1980 | - [ ] **Step 2: Run it** | |
| 1981 | ||
| 1982 | Run: `go build ./... && go test ./e2e -run 'TestLFSTokenEndsWithItsKey$' -count=1` | |
| 1983 | Expected: PASS. | |
| 1984 | ||
| 1985 | - [ ] **Step 3: Wiki** | |
| 1986 | ||
| 1987 | `Architecture/05-Identity-and-Access.org:25`, the LFS row: | |
| 1988 | ||
| 1989 | ```org | |
| 1990 | | LFS transfer token | HMAC-SHA256 over repo, SSH key, operation, expiry | not stored (stateless) | one repository, upload or download | 1 h | with its key: refused once the key is removed or expires, or its account is disabled | | |
| 1991 | ``` | |
| 1992 | ||
| 1993 | `Architecture/04-Trust-Boundaries.org`, section `** F. LFS`, the | |
| 1994 | paragraph becomes: | |
| 1995 | ||
| 1996 | ```org | |
| 1997 | =git-lfs-authenticate= over SSH applies the same repository checks as | |
| 1998 | git transport and returns a one-hour HMAC token scoped to repository, | |
| 1999 | operation and the SSH key that asked for it, deploy keys included | |
| 2000 | (=internal/sshd/lfs.go=, =internal/lfs/lfs.go=). The HTTP batch, upload | |
| 2001 | and download endpoints verify that token and that its key is still | |
| 2002 | registered, unexpired and on an enabled account (=store.LiveSSHKeys=); | |
| 2003 | public repositories allow anonymous download. Objects are verified | |
| 2004 | against their SHA-256 id on upload. | |
| 2005 | ``` | |
| 2006 | ||
| 2007 | `Architecture/09-Controls.org:28`: | |
| 2008 | ||
| 2009 | ```org | |
| 2010 | | Revocation takes effect immediately | in place | removing a key or disabling an account closes its connections; every exec re-reads its key (=internal/sshd/sshd.go=); LFS transfer tokens are refused with their key (=internal/httpd/lfs.go=) | | |
| 2011 | ``` | |
| 2012 | ||
| 2013 | `Threat-Model.org`, the `*Revocation is immediate.*` bullet: after | |
| 2014 | "a push killed before its pre-receive hook answers moves no ref." | |
| 2015 | insert: | |
| 2016 | ||
| 2017 | ```org | |
| 2018 | An LFS transfer token names the key that obtained it and is refused | |
| 2019 | from the moment that key is. | |
| 2020 | ``` | |
| 2021 | ||
| 2022 | - [ ] **Step 4: Changelog** | |
| 2023 | ||
| 2024 | Append to the `* Unreleased` list, directly above `* v1.36.0 — 2026-09-23`: | |
| 2025 | ||
| 2026 | ```org | |
| 2027 | - LFS transfer tokens name the SSH key that obtained them, deploy keys | |
| 2028 | included, and every LFS request checks that the key is still | |
| 2029 | registered, unexpired and on an enabled account: removing a key or | |
| 2030 | disabling an account ends its tokens at once instead of within the | |
| 2031 | hour (#285). *Operators:* tokens minted before the upgrade are | |
| 2032 | refused. git-lfs asks =git-lfs-authenticate= for a token each time | |
| 2033 | it runs, so only a transfer running across the restart fails, with | |
| 2034 | "repository not found"; running the command again fixes it. | |
| 2035 | ``` | |
| 2036 | ||
| 2037 | - [ ] **Step 5: Commit, MR, merge** | |
| 2038 | ||
| 2039 | ```bash | |
| 2040 | git add e2e/lfs_test.go .gitbay/wiki/Architecture/05-Identity-and-Access.org \ | |
| 2041 | .gitbay/wiki/Architecture/04-Trust-Boundaries.org .gitbay/wiki/Architecture/09-Controls.org \ | |
| 2042 | .gitbay/wiki/Threat-Model.org CHANGELOG.org | |
| 2043 | git commit -S -m "lfs: e2e for a token outliving its key; document the binding | |
| 2044 | ||
| 2045 | Closes #285" | |
| 2046 | git push -u origin lfs-token-key | |
| 2047 | gitbay mr create --source lfs-token-key --target main --title "lfs: transfer tokens die with the key that obtained them" | |
| 2048 | ``` | |
| 2049 | ||
| 2050 | Merge `--strategy ff` after CI, delete the branch both places. | |
| 2051 | ||
| 2052 | --- | |
| 2053 | ||
| 2054 | # MR 4: minting from the web needs a recent sign-in (branch `web-mint-reauth`, closes #297) | |
| 2055 | ||
| 2056 | ### Task 4.1: the session's sign-in time reaches the user | |
| 2057 | ||
| 2058 | **Files:** | |
| 2059 | - Modify: `internal/store/users.go:11-17` (`User`) | |
| 2060 | - Modify: `internal/store/sessions.go:83-102` (`WebSessionUser`) | |
| 2061 | - Test: `internal/store/sessions_test.go` | |
| 2062 | ||
| 2063 | **Interfaces:** | |
| 2064 | - Produces: `store.User.SignedInAt time.Time` — set by `WebSessionUser` from `web_sessions.created_at`; zero on every other path. | |
| 2065 | ||
| 2066 | - [ ] **Step 1: Write the failing test** | |
| 2067 | ||
| 2068 | Append to `internal/store/sessions_test.go`: | |
| 2069 | ||
| 2070 | ```go | |
| 2071 | // A session's sign-in time is its creation; using the session renews | |
| 2072 | // its idle expiry and leaves the sign-in time alone (#297). | |
| 2073 | func TestWebSessionUserSignedInAt(t *testing.T) { | |
| 2074 | s, uid := sessionFixture(t) | |
| 2075 | _, hash, err := NewToken() | |
| 2076 | if err != nil { | |
| 2077 | t.Fatal(err) | |
| 2078 | } | |
| 2079 | if err := s.CreateWebSession(hash, uid, 7*24*time.Hour); err != nil { | |
| 2080 | t.Fatal(err) | |
| 2081 | } | |
| 2082 | u, err := s.WebSessionUser(hash) | |
| 2083 | if err != nil { | |
| 2084 | t.Fatal(err) | |
| 2085 | } | |
| 2086 | if age := time.Since(u.SignedInAt); age < 0 || age > time.Minute { | |
| 2087 | t.Fatalf("fresh session signed in %v ago", age) | |
| 2088 | } | |
| 2089 | ||
| 2090 | signedIn := time.Now().Add(-2 * time.Hour) | |
| 2091 | if _, err := s.DB.Exec("UPDATE web_sessions SET created_at = ?, last_used_at = ? WHERE token_hash = ?", | |
| 2092 | fmtTime(signedIn), fmtTime(signedIn), hash); err != nil { | |
| 2093 | t.Fatal(err) | |
| 2094 | } | |
| 2095 | u, err = s.WebSessionUser(hash) | |
| 2096 | if err != nil { | |
| 2097 | t.Fatal(err) | |
| 2098 | } | |
| 2099 | var last string | |
| 2100 | if err := s.DB.QueryRow("SELECT last_used_at FROM web_sessions WHERE token_hash = ?", hash).Scan(&last); err != nil { | |
| 2101 | t.Fatal(err) | |
| 2102 | } | |
| 2103 | if last == fmtTime(signedIn) { | |
| 2104 | t.Fatal("using the session did not renew it") | |
| 2105 | } | |
| 2106 | if want := signedIn.UTC().Truncate(time.Millisecond); !u.SignedInAt.Equal(want) { | |
| 2107 | t.Fatalf("SignedInAt = %v, want %v", u.SignedInAt, want) | |
| 2108 | } | |
| 2109 | } | |
| 2110 | ``` | |
| 2111 | ||
| 2112 | - [ ] **Step 2: Run it and see it fail** | |
| 2113 | ||
| 2114 | Run: `go test ./internal/store -run TestWebSessionUserSignedInAt -count=1` | |
| 2115 | Expected: build failure, `u.SignedInAt undefined`. | |
| 2116 | ||
| 2117 | - [ ] **Step 3: Implement** | |
| 2118 | ||
| 2119 | `internal/store/users.go`, `User`: | |
| 2120 | ||
| 2121 | ```go | |
| 2122 | type User struct { | |
| 2123 | ID int64 | |
| 2124 | Username string | |
| 2125 | IsAdmin bool | |
| 2126 | Pending bool // self-registered, email not yet verified | |
| 2127 | Disabled bool // administratively suspended | |
| 2128 | // SignedInAt is when the browser session this user came from was | |
| 2129 | // created by a login. Set by WebSessionUser only; zero elsewhere. | |
| 2130 | SignedInAt time.Time | |
| 2131 | } | |
| 2132 | ``` | |
| 2133 | ||
| 2134 | `internal/store/sessions.go`, `WebSessionUser`: | |
| 2135 | ||
| 2136 | ```go | |
| 2137 | // WebSessionUser resolves a session cookie hash to its user, with the | |
| 2138 | // session's sign-in time, and renews the session's idle expiry. A | |
| 2139 | // session is written at most once a minute, so a burst of requests | |
| 2140 | // costs one UPDATE. Renewal never moves created_at: only a login | |
| 2141 | // creates a session, so created_at is when it signed in. | |
| 2142 | func (s *Store) WebSessionUser(hash string) (User, error) { | |
| 2143 | now := time.Now() | |
| 2144 | var userID int64 | |
| 2145 | var created string | |
| 2146 | err := s.DB.QueryRow( | |
| 2147 | "SELECT user_id, created_at FROM web_sessions WHERE token_hash = ? AND expires_at > ?", | |
| 2148 | hash, fmtTime(now)).Scan(&userID, &created) | |
| 2149 | if errors.Is(err, sql.ErrNoRows) { | |
| 2150 | return User{}, ErrNotFound | |
| 2151 | } | |
| 2152 | if err != nil { | |
| 2153 | return User{}, err | |
| 2154 | } | |
| 2155 | s.DB.Exec(`UPDATE web_sessions SET last_used_at = ?, expires_at = min(absolute_expires_at, ?) | |
| 2156 | WHERE token_hash = ? AND last_used_at < ?`, | |
| 2157 | fmtTime(now), fmtTime(now.Add(WebSessionIdle)), hash, fmtTime(now.Add(-time.Minute))) | |
| 2158 | u, err := s.UserByID(userID) | |
| 2159 | if err != nil { | |
| 2160 | return User{}, err | |
| 2161 | } | |
| 2162 | if t := parseTime(sql.NullString{String: created, Valid: true}); t != nil { | |
| 2163 | u.SignedInAt = *t | |
| 2164 | } | |
| 2165 | return u, nil | |
| 2166 | } | |
| 2167 | ``` | |
| 2168 | ||
| 2169 | - [ ] **Step 4: Run the package** | |
| 2170 | ||
| 2171 | Run: `go test ./internal/store -count=1 && go vet ./internal/store` | |
| 2172 | Expected: PASS. | |
| 2173 | ||
| 2174 | - [ ] **Step 5: Commit** | |
| 2175 | ||
| 2176 | ```bash | |
| 2177 | git add internal/store/users.go internal/store/sessions.go internal/store/sessions_test.go | |
| 2178 | git commit -S -m "store: a web session's user carries its sign-in time | |
| 2179 | ||
| 2180 | Ref #297" | |
| 2181 | ``` | |
| 2182 | ||
| 2183 | ### Task 4.2: Dispatch refuses a stale session's credential mint | |
| 2184 | ||
| 2185 | **Files:** | |
| 2186 | - Modify: `internal/control/control.go:21-70` (`Ctx`), `:205-209` (`runChecked`, the `Expires` block) | |
| 2187 | - Create: `internal/control/reauth_test.go` | |
| 2188 | ||
| 2189 | **Interfaces:** | |
| 2190 | - Consumes: none from Task 4.1 (the field is filled by httpd in Task 4.3). | |
| 2191 | - Produces: | |
| 2192 | - `const ReauthWindow = 15 * time.Minute` | |
| 2193 | - `const ReauthRefusal = "creating a credential from the web needs a sign-in from the last 15 minutes; sign in again, then submit the form again"` | |
| 2194 | - `Ctx.SignedInAt *time.Time` | |
| 2195 | ||
| 2196 | - [ ] **Step 1: Write the failing test** | |
| 2197 | ||
| 2198 | `internal/control/reauth_test.go`: | |
| 2199 | ||
| 2200 | ```go | |
| 2201 | package control | |
| 2202 | ||
| 2203 | import ( | |
| 2204 | "strings" | |
| 2205 | "testing" | |
| 2206 | "time" | |
| 2207 | ||
| 2208 | "gitbay.org/gitbay/internal/protocol" | |
| 2209 | "gitbay.org/gitbay/internal/store" | |
| 2210 | ) | |
| 2211 | ||
| 2212 | // A browser session mints credentials only within ReauthWindow of | |
| 2213 | // signing in; SSH and the API carry no session and are not affected | |
| 2214 | // (#297). | |
| 2215 | func TestMintNeedsRecentWebSignIn(t *testing.T) { | |
| 2216 | st, _, uid := newQueueTestRepo(t) | |
| 2217 | user := store.User{ID: uid, Username: "alice"} | |
| 2218 | run := func(signedIn *time.Time, stdin string, argv ...string) (string, int) { | |
| 2219 | c, errOut := pruneCtx(st, t.TempDir(), user) | |
| 2220 | c.Cfg.Limits.WriteRate = -1 | |
| 2221 | c.SignedInAt = signedIn | |
| 2222 | c.Stdin = strings.NewReader(stdin) | |
| 2223 | code := Dispatch(c, argv) | |
| 2224 | return strings.TrimSpace(errOut.String()), code | |
| 2225 | } | |
| 2226 | fresh := time.Now().Add(-time.Minute) | |
| 2227 | stale := time.Now().Add(-ReauthWindow - time.Minute) | |
| 2228 | ||
| 2229 | if msg, code := run(&stale, authorizedKey(t, "stale"), "keys", "add"); code != protocol.ExitDenied || msg != ReauthRefusal { | |
| 2230 | t.Fatalf("stale session: exit %d, %q", code, msg) | |
| 2231 | } | |
| 2232 | if msg, code := run(&fresh, authorizedKey(t, "fresh"), "keys", "add"); code != protocol.ExitOK { | |
| 2233 | t.Fatalf("fresh session: exit %d, %q", code, msg) | |
| 2234 | } | |
| 2235 | // SSH and the API set no SignedInAt. | |
| 2236 | if msg, code := run(nil, authorizedKey(t, "ssh"), "keys", "add"); code != protocol.ExitOK { | |
| 2237 | t.Fatalf("no session: exit %d, %q", code, msg) | |
| 2238 | } | |
| 2239 | // A command that mints nothing is not held back. | |
| 2240 | if msg, code := run(&stale, "", "keys", "list"); code != protocol.ExitOK { | |
| 2241 | t.Fatalf("keys list on a stale session: exit %d, %q", code, msg) | |
| 2242 | } | |
| 2243 | keys, err := st.ListSSHKeys(uid) | |
| 2244 | if err != nil || len(keys) != 2 { | |
| 2245 | t.Fatalf("keys: %d %v, want the fresh and the ssh one", len(keys), err) | |
| 2246 | } | |
| 2247 | } | |
| 2248 | ``` | |
| 2249 | ||
| 2250 | - [ ] **Step 2: Run it and see it fail** | |
| 2251 | ||
| 2252 | Run: `go test ./internal/control -run TestMintNeedsRecentWebSignIn -count=1` | |
| 2253 | Expected: build failure, `c.SignedInAt undefined`, `undefined: ReauthWindow`. | |
| 2254 | ||
| 2255 | - [ ] **Step 3: Implement** | |
| 2256 | ||
| 2257 | `Ctx`, after `Expires`: | |
| 2258 | ||
| 2259 | ```go | |
| 2260 | // SignedInAt is when the browser session behind this request signed | |
| 2261 | // in; nil off the web. Dispatch refuses MintsCredential commands when | |
| 2262 | // it is older than ReauthWindow. | |
| 2263 | SignedInAt *time.Time | |
| 2264 | ``` | |
| 2265 | ||
| 2266 | After the `Ctx` type: | |
| 2267 | ||
| 2268 | ```go | |
| 2269 | // ReauthWindow is how long after signing in a browser session may run a | |
| 2270 | // MintsCredential command. A session lasts days and its cookie is a | |
| 2271 | // bearer credential; what it creates must come from a recent sign-in | |
| 2272 | // (#297). | |
| 2273 | const ReauthWindow = 15 * time.Minute | |
| 2274 | ||
| 2275 | // ReauthRefusal is what a web session signed in longer ago than | |
| 2276 | // ReauthWindow gets; the web shows a sign-in link beside it. | |
| 2277 | const ReauthRefusal = "creating a credential from the web needs a sign-in from the last 15 minutes; sign in again, then submit the form again" | |
| 2278 | ``` | |
| 2279 | ||
| 2280 | `runChecked`, directly after the `Expires` block: | |
| 2281 | ||
| 2282 | ```go | |
| 2283 | if cmd.MintsCredential && c.SignedInAt != nil && time.Since(*c.SignedInAt) > ReauthWindow { | |
| 2284 | return c.fail(protocol.ExitDenied, "%s", ReauthRefusal) | |
| 2285 | } | |
| 2286 | ``` | |
| 2287 | ||
| 2288 | - [ ] **Step 4: Run the package** | |
| 2289 | ||
| 2290 | Run: `go test ./internal/control -count=1 && go vet ./internal/control` | |
| 2291 | Expected: PASS. | |
| 2292 | ||
| 2293 | - [ ] **Step 5: Commit** | |
| 2294 | ||
| 2295 | ```bash | |
| 2296 | git add internal/control/control.go internal/control/reauth_test.go | |
| 2297 | git commit -S -m "control: a web session mints credentials only soon after signing in | |
| 2298 | ||
| 2299 | Ref #297" | |
| 2300 | ``` | |
| 2301 | ||
| 2302 | ### Task 4.3: every web dispatch carries the sign-in time; the refusal links to sign-in | |
| 2303 | ||
| 2304 | **Files:** | |
| 2305 | - Modify: `internal/httpd/control.go:25-200` (the five `control.Ctx` literals → `webCtx`) | |
| 2306 | - Modify: `internal/httpd/flash.go` (add `reauthNotice`; import `control`) | |
| 2307 | - Modify: `internal/httpd/account.go:131-154` (`renderAccount`) | |
| 2308 | - Modify: `internal/httpd/settings.go:20-30` (`settingsPage`), `:67-74` (`settingsFormWith`) | |
| 2309 | - Modify: `internal/web/templates/account.html:5`, `settings.html:6` | |
| 2310 | - Modify: `internal/httpd/account_test.go:289-303` (`newTokenTestServer`) | |
| 2311 | - Create: `internal/httpd/reauth_test.go` | |
| 2312 | ||
| 2313 | **Interfaces:** | |
| 2314 | - Consumes: `store.User.SignedInAt` (Task 4.1); `control.Ctx.SignedInAt`, `control.ReauthWindow`, `control.ReauthRefusal` (Task 4.2). | |
| 2315 | - Produces: | |
| 2316 | - `func (s *Server) webCtx(u store.User, stdin string, stdout, stderr io.Writer) *control.Ctx` | |
| 2317 | - `func (s *Server) reauthNotice(w http.ResponseWriter, notice, path string) bool` | |
| 2318 | - page fields `Reauth bool` on the account page struct and `settingsPage`. | |
| 2319 | ||
| 2320 | - [ ] **Step 1: Write the failing tests** | |
| 2321 | ||
| 2322 | `internal/httpd/reauth_test.go`: | |
| 2323 | ||
| 2324 | ```go | |
| 2325 | package httpd | |
| 2326 | ||
| 2327 | import ( | |
| 2328 | "net/http" | |
| 2329 | "net/http/httptest" | |
| 2330 | "net/url" | |
| 2331 | "strings" | |
| 2332 | "testing" | |
| 2333 | "time" | |
| 2334 | ||
| 2335 | "gitbay.org/gitbay/internal/control" | |
| 2336 | "gitbay.org/gitbay/internal/store" | |
| 2337 | ) | |
| 2338 | ||
| 2339 | // A session signed in longer ago than ReauthWindow cannot mint from the | |
| 2340 | // settings page: the form comes back with the refusal and a sign-in | |
| 2341 | // link, and the sign-in returns to /settings (#297). | |
| 2342 | func TestWebMintNeedsRecentSignIn(t *testing.T) { | |
| 2343 | s, st, u := newTokenTestServer(t) | |
| 2344 | stale := u | |
| 2345 | stale.SignedInAt = time.Now().Add(-control.ReauthWindow - time.Minute) | |
| 2346 | rr := submitAccountForm(t, s, stale, url.Values{"field": {"token-create"}, "name": {"laptop"}, "scope": {"full"}}) | |
| 2347 | if rr.Code != http.StatusSeeOther { | |
| 2348 | t.Fatalf("status %d, body %s", rr.Code, rr.Body.String()) | |
| 2349 | } | |
| 2350 | if list, err := st.ListAPITokens(u.ID); err != nil || len(list) != 0 { | |
| 2351 | t.Fatalf("a stale session minted %+v (%v)", list, err) | |
| 2352 | } | |
| 2353 | ||
| 2354 | req := httptest.NewRequest("GET", "/settings", nil) | |
| 2355 | for _, c := range rr.Result().Cookies() { | |
| 2356 | req.AddCookie(c) | |
| 2357 | } | |
| 2358 | page := httptest.NewRecorder() | |
| 2359 | s.accountPage(page, req, stale) | |
| 2360 | body := page.Body.String() | |
| 2361 | if !strings.Contains(body, control.ReauthRefusal) { | |
| 2362 | t.Fatalf("refusal not shown: %s", body) | |
| 2363 | } | |
| 2364 | if !strings.Contains(body, `<a href="/login">Sign in again</a>`) { | |
| 2365 | t.Fatalf("no sign-in link: %s", body) | |
| 2366 | } | |
| 2367 | var next string | |
| 2368 | for _, c := range page.Result().Cookies() { | |
| 2369 | if c.Name == nextCookie { | |
| 2370 | next = c.Value | |
| 2371 | } | |
| 2372 | } | |
| 2373 | if next != url.QueryEscape("/settings") { | |
| 2374 | t.Fatalf("gitbay_next = %q, want /settings", next) | |
| 2375 | } | |
| 2376 | } | |
| 2377 | ||
| 2378 | // An API token has no browser session: minting through the API is not | |
| 2379 | // held to the sign-in window. | |
| 2380 | func TestAPIMintIgnoresTheSignInWindow(t *testing.T) { | |
| 2381 | s, st, u := newTokenTestServer(t) | |
| 2382 | if err := st.CreateAPIToken(u.ID, "ci", store.HashToken("gb_reauthtest"), "full", nil, 0); err != nil { | |
| 2383 | t.Fatal(err) | |
| 2384 | } | |
| 2385 | req := httptest.NewRequest("POST", "/api/v1/cmd", | |
| 2386 | strings.NewReader(`{"argv":["token","create","--name","second","--scope","read"]}`)) | |
| 2387 | req.Header.Set("Authorization", "Bearer gb_reauthtest") | |
| 2388 | rr := httptest.NewRecorder() | |
| 2389 | s.apiCmd(rr, req) | |
| 2390 | if rr.Code != http.StatusOK { | |
| 2391 | t.Fatalf("status %d: %s", rr.Code, rr.Body.String()) | |
| 2392 | } | |
| 2393 | } | |
| 2394 | ``` | |
| 2395 | ||
| 2396 | `internal/httpd/account_test.go`, `newTokenTestServer`'s return: the | |
| 2397 | session behind the tests' forms signed in just now, so the minting | |
| 2398 | tests exercise a fresh session: | |
| 2399 | ||
| 2400 | ```go | |
| 2401 | return New(config.Default(), st, nil), st, store.User{ID: uid, Username: "alice", SignedInAt: time.Now()} | |
| 2402 | ``` | |
| 2403 | ||
| 2404 | (`account_test.go` gains the `"time"` import if it lacks it.) | |
| 2405 | ||
| 2406 | - [ ] **Step 2: Run them and see them fail** | |
| 2407 | ||
| 2408 | Run: `go test ./internal/httpd -run 'TestWebMintNeedsRecentSignIn|TestAPIMintIgnoresTheSignInWindow' -count=1` | |
| 2409 | Expected: `TestWebMintNeedsRecentSignIn` FAILs with `status 200` | |
| 2410 | (the stale session minted); `TestAPIMintIgnoresTheSignInWindow` PASSes. | |
| 2411 | ||
| 2412 | - [ ] **Step 3: One Ctx for every web dispatch** | |
| 2413 | ||
| 2414 | `internal/httpd/control.go`, after `runControl`: | |
| 2415 | ||
| 2416 | ```go | |
| 2417 | // webCtx is the Ctx every web dispatch runs under: the session's user | |
| 2418 | // with full scope, and when that session signed in, which Dispatch | |
| 2419 | // checks before a command that mints a credential (#297). | |
| 2420 | func (s *Server) webCtx(u store.User, stdin string, stdout, stderr io.Writer) *control.Ctx { | |
| 2421 | signedIn := u.SignedInAt | |
| 2422 | return &control.Ctx{ | |
| 2423 | User: u, | |
| 2424 | Source: "web", | |
| 2425 | Scope: "full", | |
| 2426 | Store: s.st, | |
| 2427 | Cfg: s.cfg, | |
| 2428 | Stdin: strings.NewReader(stdin), | |
| 2429 | Stdout: stdout, | |
| 2430 | Stderr: stderr, | |
| 2431 | ViaAPI: true, | |
| 2432 | SignedInAt: &signedIn, | |
| 2433 | } | |
| 2434 | } | |
| 2435 | ``` | |
| 2436 | ||
| 2437 | Replace the five literals: | |
| 2438 | ||
| 2439 | - `runControlCode`: `ctx := s.webCtx(u, "", &stdout, &stderr)` | |
| 2440 | - `runControlStream`: | |
| 2441 | ```go | |
| 2442 | ctx := s.webCtx(u, "", out, &stderr) | |
| 2443 | ctx.Done = done | |
| 2444 | ctx.Stopping = s.stopping | |
| 2445 | ``` | |
| 2446 | - `runControlStdinCode`: `ctx := s.webCtx(u, stdin, &stdout, &stderr)` | |
| 2447 | - `dispatchIntoStdin`: | |
| 2448 | ```go | |
| 2449 | ctx := s.webCtx(u, stdin, &stdout, &stderr) | |
| 2450 | ctx.JSON = true | |
| 2451 | ``` | |
| 2452 | - `dispatchJSON`: | |
| 2453 | ```go | |
| 2454 | ctx := s.webCtx(u, stdin, &stdout, &stderr) | |
| 2455 | ctx.JSON = true | |
| 2456 | ``` | |
| 2457 | ||
| 2458 | `internal/httpd/flash.go`, import `"gitbay.org/gitbay/internal/control"` | |
| 2459 | and add after `peekNext`: | |
| 2460 | ||
| 2461 | ```go | |
| 2462 | // reauthNotice reports whether notice is Dispatch's refusal for a | |
| 2463 | // session that signed in too long ago to mint a credential and, when it | |
| 2464 | // is, remembers path so the sign-in the page links to returns there | |
| 2465 | // (#297). | |
| 2466 | func (s *Server) reauthNotice(w http.ResponseWriter, notice, path string) bool { | |
| 2467 | if notice != control.ReauthRefusal { | |
| 2468 | return false | |
| 2469 | } | |
| 2470 | s.setNext(w, path) | |
| 2471 | return true | |
| 2472 | } | |
| 2473 | ``` | |
| 2474 | ||
| 2475 | - [ ] **Step 4: The pages show the link** | |
| 2476 | ||
| 2477 | `internal/httpd/account.go`, `renderAccount`: before `s.render(...)`: | |
| 2478 | ||
| 2479 | ```go | |
| 2480 | notice := s.takeFlash(w, r) | |
| 2481 | reauth := s.reauthNotice(w, notice, "/settings") | |
| 2482 | ``` | |
| 2483 | ||
| 2484 | In the page struct, after `TokenShown`: | |
| 2485 | ||
| 2486 | ```go | |
| 2487 | Reauth bool // Notice is the stale-session refusal: link to sign in | |
| 2488 | ``` | |
| 2489 | ||
| 2490 | and the value list: | |
| 2491 | ||
| 2492 | ```go | |
| 2493 | }{s.baseFor(u), "account", keys, pgp, emails, profile, profileLinksText(profile.Links), | |
| 2494 | aboutRepo, aboutEdit, s.cfg.SiteHost(), | |
| 2495 | notice, r.URL.Query().Get("m"), mailOn, watchOn, pushOn, devices, theme, | |
| 2496 | tokens, tokenShown, reauth}) | |
| 2497 | ``` | |
| 2498 | ||
| 2499 | `internal/web/templates/account.html:5`: | |
| 2500 | ||
| 2501 | ```html | |
| 2502 | {{if .Notice}}<p class="error" role="alert">{{.Notice}}{{if .Reauth}} <a href="/login">Sign in again</a>{{end}}</p>{{end}} | |
| 2503 | ``` | |
| 2504 | ||
| 2505 | `internal/httpd/settings.go`, `settingsPage` gains after `Saved`: | |
| 2506 | ||
| 2507 | ```go | |
| 2508 | Reauth bool // Notice is the stale-session refusal: link to sign in | |
| 2509 | ``` | |
| 2510 | ||
| 2511 | and `settingsFormWith`'s render: | |
| 2512 | ||
| 2513 | ```go | |
| 2514 | s.render(w, "settings.html", settingsPage{ | |
| 2515 | repoPage: p, Topics: topics, Branches: branches, | |
| 2516 | DepsEnabled: deps.Enabled, Deps: deps, | |
| 2517 | Runners: runners, | |
| 2518 | Notice: notice, | |
| 2519 | Saved: strings.HasPrefix(notice, "Saved "), | |
| 2520 | Reauth: s.reauthNotice(w, notice, r.URL.Path), | |
| 2521 | Submitted: subm, | |
| 2522 | }) | |
| 2523 | ``` | |
| 2524 | ||
| 2525 | `internal/web/templates/settings.html:6`: | |
| 2526 | ||
| 2527 | ```html | |
| 2528 | {{if .Notice}}{{if .Saved}}<p class="notice" role="status">{{.Notice}}</p>{{else}}<p class="error" role="alert">{{.Notice}}{{if .Reauth}} <a href="/login">Sign in again</a>{{end}}</p>{{end}}{{end}} | |
| 2529 | ``` | |
| 2530 | ||
| 2531 | - [ ] **Step 5: Run the packages** | |
| 2532 | ||
| 2533 | Run: `go build ./... && go vet ./internal/httpd ./internal/web && go test ./internal/httpd ./internal/web -count=1` | |
| 2534 | Expected: PASS, including the existing token tests on the fresh | |
| 2535 | session from `newTokenTestServer`. | |
| 2536 | ||
| 2537 | Run: `go test ./e2e -run 'TestAccountSettingsWeb$' -count=1` | |
| 2538 | Expected: PASS (its session is created by a login moments earlier). | |
| 2539 | ||
| 2540 | - [ ] **Step 6: Commit** | |
| 2541 | ||
| 2542 | ```bash | |
| 2543 | git add internal/httpd internal/web/templates/account.html internal/web/templates/settings.html | |
| 2544 | git commit -S -m "web: dispatch carries the session's sign-in time; a stale one gets a sign-in link | |
| 2545 | ||
| 2546 | Ref #297" | |
| 2547 | ``` | |
| 2548 | ||
| 2549 | ### Task 4.4: wiki and changelog | |
| 2550 | ||
| 2551 | **Files:** | |
| 2552 | - Modify: `.gitbay/wiki/Threat-Model.org:57-63`, `Architecture/09-Controls.org:29`, `Architecture/10-Known-Gaps.org:16`, `Architecture/05-Identity-and-Access.org:21`, `:106-120` | |
| 2553 | - Modify: `CHANGELOG.org` | |
| 2554 | ||
| 2555 | **Interfaces:** none. | |
| 2556 | ||
| 2557 | - [ ] **Step 1: Wiki** | |
| 2558 | ||
| 2559 | `Threat-Model.org`, in the `*The control plane is one command | |
| 2560 | registry*` bullet, replace `Browser sessions are not covered yet | |
| 2561 | (#297).` with: | |
| 2562 | ||
| 2563 | ```org | |
| 2564 | A browser session can create one only within 15 minutes of signing | |
| 2565 | in (=control.ReauthWindow=, #297). | |
| 2566 | ``` | |
| 2567 | ||
| 2568 | `Architecture/09-Controls.org:29`: | |
| 2569 | ||
| 2570 | ```org | |
| 2571 | | Delegation bounded by the delegating credential | in place | expiring tokens refused on =MintsCredential= commands; credentials record their creating token; a browser session mints only within 15 minutes of signing in (=internal/control/control.go=) | | |
| 2572 | ``` | |
| 2573 | ||
| 2574 | `Architecture/10-Known-Gaps.org`: delete the `#297` row (line 16). | |
| 2575 | ||
| 2576 | `Architecture/05-Identity-and-Access.org:21`, the web session row: | |
| 2577 | ||
| 2578 | ```org | |
| 2579 | | Web session | 32 random bytes hex, cookie =gitbay_session= | SHA-256 hash | full account; credential-minting forms only within 15 minutes of sign-in | 12 h idle, 7 days absolute | logout, =web sessions revoke= | | |
| 2580 | ``` | |
| 2581 | ||
| 2582 | and in `* Session security on the web`, after the destructive-actions | |
| 2583 | bullet: | |
| 2584 | ||
| 2585 | ```org | |
| 2586 | - Forms that create a credential (SSH and runner keys, API tokens, | |
| 2587 | email verification) need a sign-in from the last 15 minutes; an | |
| 2588 | older session gets the form back with a sign-in link that returns to | |
| 2589 | it (=control.ReauthWindow=, =internal/httpd/flash.go=). | |
| 2590 | ``` | |
| 2591 | ||
| 2592 | - [ ] **Step 2: Changelog** | |
| 2593 | ||
| 2594 | Append to the `* Unreleased` list, directly above `* v1.36.0 — 2026-09-23`: | |
| 2595 | ||
| 2596 | ```org | |
| 2597 | - A browser session creates credentials (SSH and runner keys, API | |
| 2598 | tokens, verified addresses) only within 15 minutes of signing in. An | |
| 2599 | older session gets the form back with a "Sign in again" link, and the | |
| 2600 | login link returns to the form. SSH and API tokens are unaffected | |
| 2601 | (#297). | |
| 2602 | ``` | |
| 2603 | ||
| 2604 | - [ ] **Step 3: Commit, MR, merge** | |
| 2605 | ||
| 2606 | ```bash | |
| 2607 | git add .gitbay/wiki/Threat-Model.org .gitbay/wiki/Architecture/09-Controls.org \ | |
| 2608 | .gitbay/wiki/Architecture/10-Known-Gaps.org .gitbay/wiki/Architecture/05-Identity-and-Access.org \ | |
| 2609 | CHANGELOG.org | |
| 2610 | git commit -S -m "wiki: web credential minting needs a recent sign-in | |
| 2611 | ||
| 2612 | Closes #297" | |
| 2613 | git push -u origin web-mint-reauth | |
| 2614 | gitbay mr create --source web-mint-reauth --target main --title "web: minting a credential needs a sign-in from the last 15 minutes" | |
| 2615 | ``` | |
| 2616 | ||
| 2617 | Merge `--strategy ff` after CI, delete the branch both places. | |
| 2618 | ||
| 2619 | --- | |
| 2620 | ||
| 2621 | ## Out of scope, noted | |
| 2622 | ||
| 2623 | - `repo import-issues` validates `--api-base` once | |
| 2624 | (`internal/control/ghimport.go:162`) and then fetches pull heads with | |
| 2625 | `gitutil.FetchPullHeads` (`ghimport.go:364-379`) and calls the API | |
| 2626 | with a plain `http.Client`, neither pinned. It is the same class of | |
| 2627 | gap as #298 and is not in any of the five issues; filed as #301 | |
| 2628 | rather than widening MR 2. | |