diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bac5736..0d6a898 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,11 @@ concurrency: permissions: contents: write pull-requests: read + # Keyless signing: cosign exchanges this for a short-lived Fulcio certificate + # bound to the workflow identity, so there is no private key for anyone to + # lose and a verifier can ask "was this built by this repository's release + # workflow" rather than "do I recognise this key". + id-token: write jobs: changes: @@ -119,14 +124,25 @@ jobs: done echo "go mod download failed after retries" >&2 exit 1 + # -race on every platform, because the bugs it finds do not reproduce + # reliably and the suite had never been run under it. The first run found + # a real data race; a suite that only ever runs without it is a suite that + # cannot see the class of bug most likely to reach production. - name: Test - run: go test ./... + run: go test -race ./... - name: Build shell: bash run: | out=contextd [[ "${RUNNER_OS}" == "Windows" ]] && out=contextd.exe go build -o "$out" ./cmd/contextd + # Cheap and catches a category tests do not: printf verbs that do not + # match their arguments, unreachable code, lost struct tags. + - name: Vet + run: go vet ./... + - name: Vet integration-tagged code + if: runner.os == 'Linux' + run: go vet -tags integration ./... - name: Shellcheck install.sh if: runner.os == 'Linux' run: | @@ -137,6 +153,28 @@ jobs: if: runner.os == 'Linux' run: bash scripts/ci/next-version_test.sh + analysis: + needs: [changes] + if: needs.changes.outputs.tests == 'true' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: "1.25.x" + # Known vulnerabilities in what we actually call, not merely in what we + # depend on: govulncheck reports a module only when a reachable path + # exists, so it stays quiet enough to be worth reading. + - name: Vulnerability check + run: | + go install golang.org/x/vuln/cmd/govulncheck@latest + "$(go env GOPATH)/bin/govulncheck" ./... + - name: Lint + uses: golangci/golangci-lint-action@v6 + with: + version: v1.62 + args: --timeout=5m + integration: needs: [changes] if: needs.changes.outputs.tests == 'true' @@ -168,12 +206,13 @@ jobs: always() && needs.changes.outputs.product == 'true' && needs.test.result == 'success' && + needs.analysis.result == 'success' && needs.integration.result == 'success' && ( (github.event_name == 'push' && github.ref == 'refs/heads/main') || github.event_name == 'workflow_dispatch' ) - needs: [changes, test, integration] + needs: [changes, test, analysis, integration] runs-on: ubuntu-latest concurrency: group: release-main @@ -188,6 +227,11 @@ jobs: with: go-version: "1.25.x" + # Signing and the bill of materials. Both are GoReleaser steps; these put + # the tools on the runner. + - uses: sigstore/cosign-installer@v3 + - uses: anchore/sbom-action/download-syft@v0 + - name: Compute next minor version id: ver run: | diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..20a355a --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,43 @@ +# Linters that must pass, chosen so the gate means something on the day it is +# switched on. +# +# The full set was run first, and it reports 489 findings — 409 of them errcheck +# on deliberate discards like `defer f.Close()`, 43 noctx, 15 errorlint. Turning +# all of that on at once produces a gate that is red from the first commit, and a +# gate nobody can get green is a gate somebody deletes. Those three are tracked +# as their own piece of work, with the numbers, rather than being enabled and +# then permanently ignored. +# +# What is here catches defects rather than taste, and passes today — so a new +# finding is a new mistake, which is the only way a linter earns its place in CI. + +run: + timeout: 5m + tests: true + +linters: + disable-all: true + enable: + - govet + # staticcheck found the real one: an `if` with an empty body where backend + # migration was supposed to verify that what arrived matched what was sent. + - staticcheck + - ineffassign + - unused + +linters-settings: + staticcheck: + checks: ["all"] + +issues: + exclude-use-default: false + max-issues-per-linter: 0 + max-same-issues: 0 + exclude-rules: + # SA1019 on the TUI is bubbles renaming its viewport scrolling methods. Real, + # but it is a rename to follow deliberately in one change, not a reason to + # block every unrelated commit in the meantime. + - path: internal/tui/ + linters: + - staticcheck + text: "SA1019" diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 7c1c680..da79546 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -38,6 +38,46 @@ archives: checksum: name_template: checksums.txt +# Signatures and a bill of materials, because a checksum file signs nothing. +# +# Whoever can replace the binary on the release page can replace checksums.txt +# beside it, and the install script downloads both from the same place. The +# checksum proves the download was not corrupted in transit; it says nothing +# about who produced it. That matters more here than for most projects, because +# the documented way to install this is `curl … | bash`. +# +# Keyless signing: cosign gets a short-lived certificate from Fulcio bound to +# the GitHub Actions identity that ran the release, and the fact is recorded in +# the Rekor transparency log. There is no private key for anybody to lose, and +# the question a verifier asks is answerable — "was this built by the release +# workflow of this repository", rather than "do I recognise this key". +# +# Verify a download with: +# cosign verify-blob --signature checksums.txt.sig \ +# --certificate checksums.txt.pem \ +# --certificate-identity-regexp 'https://github.com/orkcom-tech/contextverse/.*' \ +# --certificate-oidc-issuer https://token.actions.githubusercontent.com \ +# checksums.txt +signs: + - cmd: cosign + certificate: "${artifact}.pem" + args: + - sign-blob + - "--output-certificate=${certificate}" + - "--output-signature=${signature}" + - "${artifact}" + - "--yes" + # The checksum file covers every artifact, so signing it signs the release. + artifacts: checksum + output: true + +# What is actually inside each archive, in a format a scanner can read. Without +# one, answering "is this release affected by CVE-x" means rebuilding it and +# hoping the dependency graph has not moved. +sboms: + - id: archive + artifacts: archive + changelog: sort: asc filters: diff --git a/internal/auth/auth_hardening_test.go b/internal/auth/auth_hardening_test.go index 5992800..6484d24 100644 --- a/internal/auth/auth_hardening_test.go +++ b/internal/auth/auth_hardening_test.go @@ -108,7 +108,9 @@ func TestLoginFailuresAreIndistinguishableAndLockOut(t *testing.T) { t.Fatalf("failure reasons must not differ: %q vs %q", missing, wrong) } - for i := 0; i < MaxLoginFailures; i++ { + // Enough failures from anywhere to reach the account's own budget, which is + // deliberately looser than the per-address one — see lockout.go. + for i := 0; i < MaxAccountFailures; i++ { _, _, _ = s.LoginUserpass("kim", "wrong-password") } if !s.LoginLocked("kim") { diff --git a/internal/auth/lockout.go b/internal/auth/lockout.go index 0fc62f4..495e13f 100644 --- a/internal/auth/lockout.go +++ b/internal/auth/lockout.go @@ -6,12 +6,40 @@ import ( "time" ) -// Lockout thresholds for password login. Single-node, in-process: a restart -// clears the counters, which is acceptable because the window is short. +// Lockout thresholds for password login. In-process: a restart clears the +// counters, which is acceptable because the window is short. +// +// # Who gets locked out +// +// Counting failures against the username alone means anyone who knows a name can +// lock its owner out by failing five times — a denial of service delivered by +// typing the wrong password, and one that a support desk cannot distinguish from +// a real attack. That is worse than the attack it prevents, because the attacker +// pays nothing and the victim loses access. +// +// So failures count twice: against the account, and against the address they +// came from. An account lock still exists, because an attacker spread across +// many addresses has to be stopped somewhere, but it is deliberately the looser +// of the two — a wider budget, so a single hostile client hits its own limit +// long before it can spend the account's. +// +// # What this does not solve +// +// The counters live in one process. A fleet behind a load balancer gives an +// attacker a fresh budget per replica, and a restart clears everything. Fixing +// that needs shared state, which the OSS server deliberately does not have — +// contextverse-server.md calls its HA "stateless, no clustering". An operator +// running more than one replica should rate-limit authentication at the router; +// the values here are a floor, not a fleet-wide guarantee. const ( + // MaxLoginFailures is the per-address budget: the one an attacker spends. MaxLoginFailures = 5 - LockoutWindow = 15 * time.Minute - LockoutDuration = 15 * time.Minute + // MaxAccountFailures is the per-account budget. Larger on purpose: reaching + // it means failures arrived from several addresses, which is the case the + // account lock exists for. + MaxAccountFailures = 25 + LockoutWindow = 15 * time.Minute + LockoutDuration = 15 * time.Minute ) // BootstrapTokenTTL bounds the first-run admin token, which is written to disk @@ -48,7 +76,7 @@ func (l *loginFailures) locked(key string, now time.Time) bool { return false } -func (l *loginFailures) fail(key string, now time.Time) { +func (l *loginFailures) fail(key string, now time.Time, max int) { l.mu.Lock() defer l.mu.Unlock() if l.by == nil { @@ -60,7 +88,7 @@ func (l *loginFailures) fail(key string, now time.Time) { return } st.count++ - if st.count >= MaxLoginFailures { + if st.count >= max { st.blocked = now.Add(LockoutDuration) } // Opportunistic eviction so a scripted attacker cannot grow the map without @@ -82,5 +110,10 @@ func (l *loginFailures) reset(key string) { // LoginLocked reports whether password login for username is currently refused. func (s *Store) LoginLocked(username string) bool { - return s.failures.locked(username, time.Now()) + return s.failures.locked(accountKey(username), time.Now()) } + +// Namespaced so a username can never collide with an address — "10.0.0.1" is a +// legal username. +func accountKey(username string) string { return "user:" + username } +func addressKey(addr string) string { return "addr:" + addr } diff --git a/internal/auth/lockout_test.go b/internal/auth/lockout_test.go new file mode 100644 index 0000000..1eeba6c --- /dev/null +++ b/internal/auth/lockout_test.go @@ -0,0 +1,159 @@ +package auth + +import ( + "errors" + "testing" +) + +// Counting failures against the username alone means anybody who knows a name +// can lock its owner out by typing the wrong password five times. That is a +// denial of service delivered for free, indistinguishable at the support desk +// from a real attack, and worse than the attack the lockout was for. +// +// Failures now count against the address as well, and the address budget is the +// tight one. + +func passwordStore(t *testing.T, users ...string) *Store { + t.Helper() + s := newStore(t) + for _, u := range users { + if err := s.AddUser(u, RoleContributor); err != nil { + t.Fatal(err) + } + if err := s.SetPassword(u, "correct-horse-battery"); err != nil { + t.Fatal(err) + } + } + return s +} + +// The attack this must not permit: somebody else's failures locking you out. +func TestAnAttackerCannotLockSomebodyElseOut(t *testing.T) { + s := passwordStore(t, "kim") + + // An attacker at one address burns their own budget against kim's account. + for i := 0; i < MaxLoginFailures*2; i++ { + _, _, _ = s.LoginUserpassFrom("kim", "wrong", "203.0.113.9") + } + + // The attacker is stopped. + if _, _, err := s.LoginUserpassFrom("kim", "correct-horse-battery", "203.0.113.9"); !errors.Is(err, ErrLockedOut) { + t.Error("the attacker's address was not locked out") + } + + // Kim, at her own address, can still log in. This is the whole point. + if _, _, err := s.LoginUserpassFrom("kim", "correct-horse-battery", "198.51.100.4"); err != nil { + t.Fatalf("the real owner was locked out by somebody else's failures: %v", err) + } +} + +// The address budget is what an attacker actually spends, so it has to bite +// quickly. +func TestOneAddressIsStoppedAfterItsBudget(t *testing.T) { + s := passwordStore(t, "kim") + const addr = "203.0.113.9" + + for i := 0; i < MaxLoginFailures; i++ { + if _, _, err := s.LoginUserpassFrom("kim", "wrong", addr); errors.Is(err, ErrLockedOut) { + t.Fatalf("locked out after only %d attempts, want %d", i, MaxLoginFailures) + } + } + if _, _, err := s.LoginUserpassFrom("kim", "wrong", addr); !errors.Is(err, ErrLockedOut) { + t.Errorf("still accepting attempts past the budget: %v", err) + } +} + +// An attacker working through a list of accounts from one machine is stopped by +// the address budget, not by locking every account they touch. +func TestGuessingManyAccountsFromOneAddressLocksTheAddress(t *testing.T) { + s := passwordStore(t, "kim", "sam", "alex") + const addr = "203.0.113.9" + + for _, u := range []string{"kim", "sam", "alex", "kim", "sam", "alex"} { + _, _, _ = s.LoginUserpassFrom(u, "wrong", addr) + } + + if _, _, err := s.LoginUserpassFrom("kim", "correct-horse-battery", addr); !errors.Is(err, ErrLockedOut) { + t.Error("the address spraying accounts was not locked") + } + // None of the three accounts should be locked: each took two failures. + for _, u := range []string{"kim", "sam", "alex"} { + if s.LoginLocked(u) { + t.Errorf("%s was locked out by an attack that never reached the account budget", u) + } + } +} + +// An account still locks when failures arrive from many addresses, which is the +// case the account budget exists for. +func TestAnAccountLocksWhenFailuresComeFromEverywhere(t *testing.T) { + s := passwordStore(t, "kim") + + for i := 0; i < MaxAccountFailures; i++ { + // A different address every time, so no address budget is ever reached. + addr := "203.0.113." + string(rune('a'+i%26)) + itoa(i) + _, _, _ = s.LoginUserpassFrom("kim", "wrong", addr) + } + if !s.LoginLocked("kim") { + t.Error("an account attacked from many addresses was never locked") + } +} + +// A correct password clears the account, but must not clear the address: one +// right answer cannot wipe the record of somebody working through a list from +// the same machine. +func TestASuccessDoesNotForgiveTheAddress(t *testing.T) { + s := passwordStore(t, "kim", "sam") + const addr = "203.0.113.9" + + for i := 0; i < MaxLoginFailures-1; i++ { + _, _, _ = s.LoginUserpassFrom("sam", "wrong", addr) + } + // A legitimate-looking success from the same machine. + if _, _, err := s.LoginUserpassFrom("kim", "correct-horse-battery", addr); err != nil { + t.Fatal(err) + } + // One more failure should still reach the address budget. + _, _, _ = s.LoginUserpassFrom("sam", "wrong", addr) + if _, _, err := s.LoginUserpassFrom("sam", "wrong", addr); !errors.Is(err, ErrLockedOut) { + t.Error("a success reset the attacker's address budget") + } +} + +// A username that looks like an address must not share its counter. +func TestUsernamesAndAddressesDoNotCollide(t *testing.T) { + s := passwordStore(t, "203.0.113.9") + + // Burn the address budget from a different machine. + for i := 0; i < MaxLoginFailures+1; i++ { + _, _, _ = s.LoginUserpassFrom("somebody", "wrong", "198.51.100.4") + } + // The user named like an address is untouched. + if s.LoginLocked("203.0.113.9") { + t.Error("a username collided with an address counter") + } +} + +// A caller with no address to offer still works, counting against the account +// alone — a local console, or a test. +func TestLoginWithoutAnAddressStillWorks(t *testing.T) { + s := passwordStore(t, "kim") + if _, _, err := s.LoginUserpass("kim", "correct-horse-battery"); err != nil { + t.Fatal(err) + } + if _, _, err := s.LoginUserpass("kim", "wrong"); err == nil { + t.Error("a wrong password was accepted") + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var b []byte + for n > 0 { + b = append([]byte{byte('0' + n%10)}, b...) + n /= 10 + } + return string(b) +} diff --git a/internal/auth/password.go b/internal/auth/password.go index 253a3b1..2af6207 100644 --- a/internal/auth/password.go +++ b/internal/auth/password.go @@ -70,12 +70,31 @@ var errInvalidCredentials = fmt.Errorf("invalid credentials") var dummyHash = []byte("$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92ldGxad68LJZdL17lhWy") // LoginUserpass verifies password and issues a new bearer token (shown once). +// +// Deprecated in favour of LoginUserpassFrom, which can also count failures +// against where they came from. Kept so callers that genuinely have no address — +// a local console, a test — do not have to invent one. func (s *Store) LoginUserpass(username, password string) (plaintext string, rec TokenRecord, err error) { + return s.LoginUserpassFrom(username, password, "") +} + +// LoginUserpassFrom verifies a password and issues a bearer token, counting a +// failure against both the account and the address it came from. +// +// addr may be empty when the caller has none. Failures then count only against +// the account, which is the old behaviour and the weaker of the two. +func (s *Store) LoginUserpassFrom(username, password, addr string) (plaintext string, rec TokenRecord, err error) { if username == "" || password == "" { return "", TokenRecord{}, fmt.Errorf("username and password required") } now := time.Now() - if s.failures.locked(username, now) { + // The address is checked first and refused hardest: it is the budget an + // attacker actually spends, and the one that cannot be used to lock out + // somebody else. + if addr != "" && s.failures.locked(addressKey(addr), now) { + return "", TokenRecord{}, ErrLockedOut + } + if s.failures.locked(accountKey(username), now) { return "", TokenRecord{}, ErrLockedOut } s.mu.RLock() @@ -99,10 +118,16 @@ func (s *Store) LoginUserpass(username, password string) (plaintext string, rec } matched := bcrypt.CompareHashAndPassword(hash, []byte(password)) == nil if !usable || !matched { - s.failures.fail(username, now) + s.failures.fail(accountKey(username), now, MaxAccountFailures) + if addr != "" { + s.failures.fail(addressKey(addr), now, MaxLoginFailures) + } return "", TokenRecord{}, errInvalidCredentials } - s.failures.reset(username) + // A success clears the account, but not the address: one correct password + // must not wipe the record of an attacker working through a list from the + // same machine. + s.failures.reset(accountKey(username)) return s.CreateToken(username, "userpass") } diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 220b027..0f79e37 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -59,6 +59,12 @@ func newRoot() *cobra.Command { PersistentPreRun: func(cmd *cobra.Command, args []string) { logx.SetDebug(flagDebug) }, + // Cobra skips PostRun when the command failed, which is the behaviour we + // want: somebody debugging a broken command does not also need to hear + // about a release. + PersistentPostRun: func(cmd *cobra.Command, args []string) { + maybePrintUpdateNotice(cmd) + }, } root.PersistentFlags().BoolVar(&flagDebug, "debug", false, "enable debug logging") root.PersistentFlags().BoolVar(&flagJSON, "json", false, "structured JSON output (where supported)") diff --git a/internal/cli/client.go b/internal/cli/client.go index 9570ccd..e94efd4 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -226,7 +226,14 @@ func newPushCmd() *cobra.Command { if expected == "" { expected = head } - res, err := client.Push(cmd.Context(), root, expected, syncCfg, check) + // The record of what this machine last sent is what makes a push + // carry the difference rather than the whole space, and what makes a + // local deletion reach the server at all. + st, err := syncclient.LoadState(root) + if err != nil { + return err + } + res, err := client.Push(cmd.Context(), root, expected, syncCfg, st, check) if err != nil { return err } @@ -242,6 +249,11 @@ func newPushCmd() *cobra.Command { if err := config.Save(cfg); err != nil { return err } + // Saved after the server accepted the batch. A record written + // earlier would make the next push skip files that never arrived. + if err := syncclient.SaveState(root, st); err != nil { + return err + } rep := SyncReport{Applied: res.Applied, Head: res.Head} return emit(cmd.OutOrStdout(), rep, func(w io.Writer) error { fmt.Fprintf(w, "pushed %d ops; head=%s\n", rep.Applied, rep.Head) diff --git a/internal/cli/exit.go b/internal/cli/exit.go index 5a42555..5b7cd7a 100644 --- a/internal/cli/exit.go +++ b/internal/cli/exit.go @@ -5,7 +5,6 @@ import ( "errors" "io" "net" - "os" "github.com/orkcom-tech/contextverse/internal/cliout" "github.com/orkcom-tech/contextverse/internal/storage" @@ -103,12 +102,3 @@ func structuredOutput() bool { } return f.Structured() } - -// stderrIfStructured returns the writer chatter should go to: stderr when stdout -// is carrying a structured document, stdout otherwise. -func stderrIfStructured(stdout io.Writer) io.Writer { - if structuredOutput() { - return os.Stderr - } - return stdout -} diff --git a/internal/cli/server.go b/internal/cli/server.go index f2a0af2..1106020 100644 --- a/internal/cli/server.go +++ b/internal/cli/server.go @@ -203,13 +203,6 @@ func loopbackHost(address string) string { return address } -func orDefaultInt(v, def int) int { - if v == 0 { - return def - } - return v -} - // UserEntry is one row of `user list`. type UserEntry struct { Name string `json:"name" yaml:"name"` diff --git a/internal/cli/update_notice.go b/internal/cli/update_notice.go new file mode 100644 index 0000000..33d232a --- /dev/null +++ b/internal/cli/update_notice.go @@ -0,0 +1,104 @@ +package cli + +import ( + "fmt" + "os" + + "github.com/spf13/cobra" + + "github.com/orkcom-tech/contextverse/internal/config" + "github.com/orkcom-tech/contextverse/internal/prompt" + "github.com/orkcom-tech/contextverse/internal/selfupdate" + "github.com/orkcom-tech/contextverse/internal/version" +) + +// Where the update notice is allowed to appear. +// +// The check itself is cheap and silent; what makes a version notice hated is +// where it lands. Ranked by damage: +// +// - It corrupts a protocol. `mcp serve` speaks JSON-RPC over stdio, and a +// stray line breaks the AI client outright. `daemon run` is a background +// process with nobody reading it. +// - It corrupts a parse. --json and --yaml exist to be piped into something. +// - It is simply not wanted. Scripts, CI, and any non-terminal output. +// +// So the rule is an allowlist of circumstances rather than a blocklist of +// commands: a terminal, a person, no structured output, and not one of the +// commands whose output belongs to a machine. + +// quietCommands never print a notice, whatever else is true. Named by their +// full path so a future `contextd mcp inspect` is not silenced by accident. +var quietCommands = map[string]bool{ + "contextd mcp serve": true, // JSON-RPC on stdio; a line here breaks the client + "contextd daemon run": true, // background process, nobody is reading + "contextd completion": true, // output is sourced by the shell + "contextd server start": true, // long-running; the notice would scroll past at boot +} + +// maybePrintUpdateNotice writes at most one line to stderr, or nothing. +// +// Called from the root command's PersistentPostRun, which cobra skips when the +// command failed — somebody debugging a broken command does not also need to +// hear about a release. +func maybePrintUpdateNotice(cmd *cobra.Command) { + if !updateNoticeAllowed(cmd) { + return + } + checker := &selfupdate.Checker{Current: version.Version} + if notice := checker.Notice(cmd.Context()); notice != "" { + // stderr, always: stdout may be in a pipe even when it looks like a + // terminal to us, and this is not part of any command's answer. + fmt.Fprintln(os.Stderr, notice) + } +} + +func updateNoticeAllowed(cmd *cobra.Command) bool { + if cmd == nil || quietCommands[cmd.CommandPath()] { + return false + } + // Structured output is somebody's input. structuredOutput already answers + // this for the whole CLI — it was written for exactly this and had no + // callers, so hand-rolling the flag check here would have been a second + // answer to the same question, free to drift from the first. + if structuredOutput() { + return false + } + // A person at a terminal, not a pipe, a cron job or a build. + if !prompt.Interactive() { + return false + } + if isCI() { + return false + } + if noUpdateCheckConfigured() { + return false + } + return true +} + +// isCI recognises the usual markers. Every one of these means the output is +// going into a log nobody reads until something breaks. +func isCI() bool { + for _, key := range []string{"CI", "CONTINUOUS_INTEGRATION", "BUILD_NUMBER", "GITHUB_ACTIONS", "GITLAB_CI"} { + if os.Getenv(key) != "" { + return true + } + } + return false +} + +// noUpdateCheckConfigured reads the persistent off switch. The environment +// variable is handled inside the checker; this is the setting somebody makes +// once instead of editing their shell profile. +func noUpdateCheckConfigured() bool { + root, err := resolveSpaceRoot() + if err != nil || !config.Exists(root) { + return false + } + cfg, err := config.Load(root) + if err != nil { + return false + } + return cfg.NoUpdateCheck +} diff --git a/internal/cli/update_notice_test.go b/internal/cli/update_notice_test.go new file mode 100644 index 0000000..6241c91 --- /dev/null +++ b/internal/cli/update_notice_test.go @@ -0,0 +1,106 @@ +package cli + +import ( + "testing" + + "github.com/spf13/cobra" +) + +// The check itself is tested in internal/selfupdate. What is tested here is the +// part that decides whether anyone hears it, because that is where a version +// notice stops being helpful and starts being the reason somebody turns it off +// — or worse, the reason an AI client's JSON-RPC stream stops parsing. + +func cmdPath(path string) *cobra.Command { + // cobra derives CommandPath from the tree, so build the tree. + root := &cobra.Command{Use: "contextd"} + cur := root + for _, part := range splitPath(path)[1:] { + child := &cobra.Command{Use: part} + cur.AddCommand(child) + cur = child + } + return cur +} + +func splitPath(s string) []string { + var out []string + start := 0 + for i := 0; i <= len(s); i++ { + if i == len(s) || s[i] == ' ' { + out = append(out, s[start:i]) + start = i + 1 + } + } + return out +} + +// The ones that must never speak, and why. +func TestCommandsWhoseOutputBelongsToAMachineAreSilent(t *testing.T) { + for _, path := range []string{ + "contextd mcp serve", // JSON-RPC over stdio; a line breaks the AI client + "contextd daemon run", // background process, nobody reading + "contextd completion", // output is sourced by the shell + "contextd server start", // long-running; it would scroll past at boot + } { + if !quietCommands[path] { + t.Errorf("%s is not in the quiet list", path) + } + if updateNoticeAllowed(cmdPath(path)) { + t.Errorf("%s would print an update notice", path) + } + } +} + +// Structured output is somebody's input; a friendly line in it is a parse error. +func TestStructuredOutputIsSilent(t *testing.T) { + defer func() { flagJSON, flagYAML = false, false }() + + flagJSON, flagYAML = true, false + if updateNoticeAllowed(cmdPath("contextd status")) { + t.Error("--json output would carry an update notice") + } + flagJSON, flagYAML = false, true + if updateNoticeAllowed(cmdPath("contextd status")) { + t.Error("--yaml output would carry an update notice") + } +} + +func TestCIIsSilent(t *testing.T) { + for _, key := range []string{"CI", "GITHUB_ACTIONS", "GITLAB_CI", "BUILD_NUMBER", "CONTINUOUS_INTEGRATION"} { + t.Run(key, func(t *testing.T) { + t.Setenv(key, "true") + if !isCI() { + t.Errorf("%s set but isCI() is false", key) + } + if updateNoticeAllowed(cmdPath("contextd status")) { + t.Errorf("a notice would be printed under %s", key) + } + }) + } +} + +// The test process is not a terminal, so an ordinary command is silent here for +// the same reason a piped one is in real use. Worth asserting rather than +// assuming: it is the condition that covers every script nobody thought of. +func TestANonTerminalIsSilent(t *testing.T) { + if updateNoticeAllowed(cmdPath("contextd status")) { + t.Error("a notice would be printed with stdout not a terminal") + } +} + +func TestNilCommandIsSilent(t *testing.T) { + if updateNoticeAllowed(nil) { + t.Error("a nil command was allowed to print") + } +} + +// A command that is not on the quiet list is only silenced by circumstance, not +// by name — otherwise the feature is off for everyone by accident. +func TestOrdinaryCommandsAreNotBlockedByName(t *testing.T) { + for _, path := range []string{"contextd status", "contextd pull", "contextd version", "contextd mcp"} { + if quietCommands[path] { + t.Errorf("%s is silenced by name; it should only be silenced by context", path) + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 9385820..8ef7e1c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,6 +7,8 @@ import ( "time" "gopkg.in/yaml.v3" + + "github.com/orkcom-tech/contextverse/internal/logx" ) // Mode is the runtime deployment mode. @@ -41,6 +43,11 @@ type Config struct { Daemon DaemonConfig `yaml:"daemon,omitempty"` // client background poller Editor string `yaml:"editor,omitempty"` // remembered TUI editor choice (binary id) + // NoUpdateCheck silences the "a newer contextd exists" line for good. + // Phrased as an opt-out so the zero value keeps the check on, and so a + // config written before this existed behaves the way a new one does. + NoUpdateCheck bool `yaml:"no_update_check,omitempty"` + // Anchors record where each project's code actually lives, learned from the // directory `contextd activate` was run in. // @@ -153,6 +160,12 @@ func Load(spaceRoot string) (*Config, error) { if err := yaml.Unmarshal(data, &cfg); err != nil { return nil, fmt.Errorf("parse config %s: %w", path, err) } + // Repair on read, not only on write: a config written by an earlier version + // is world-readable and holds credentials, and nobody is going to notice + // that on their own. Best-effort — an unwritable config is still usable. + if err := restrictSecretFile(path); err != nil { + logx.L().Debug("could not tighten config permissions", "path", path, "err", err) + } // Where we just read from is authoritative, and it is stored absolute. // @@ -193,14 +206,35 @@ func Save(cfg *Config) error { return fmt.Errorf("create space root: %w", err) } tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { + if err := os.WriteFile(tmp, data, secretFileMode); err != nil { return fmt.Errorf("write config temp: %w", err) } if err := os.Rename(tmp, path); err != nil { _ = os.Remove(tmp) return fmt.Errorf("replace config: %w", err) } - return nil + // A file written before this was tightened keeps its old mode through a + // rename, so the permissions are asserted rather than assumed. + return restrictSecretFile(path) +} + +// secretFileMode is owner-only. These files carry git_token, s3_secret_key and +// sql_dsn — a DSN with the password inline — and were world-readable at 0644, +// while the bearer token next to them was already 0600. On a shared host or in +// a container with a second user that is a credential handed out for free. +const secretFileMode os.FileMode = 0o600 + +// restrictSecretFile removes group and world access from a file that holds +// credentials, including one written by an older version. +func restrictSecretFile(path string) error { + st, err := os.Stat(path) + if err != nil { + return err + } + if st.Mode().Perm()&0o077 == 0 { + return nil + } + return os.Chmod(path, secretFileMode) } // DetectMode inspects conventional locations and returns the active mode. diff --git a/internal/config/perms_test.go b/internal/config/perms_test.go new file mode 100644 index 0000000..2fcf3c0 --- /dev/null +++ b/internal/config/perms_test.go @@ -0,0 +1,118 @@ +package config + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// These files hold git_token, s3_secret_key and sql_dsn — a DSN with the +// password inline — and were written world-readable while the bearer token +// beside them was already owner-only. On a shared host or in a container with a +// second user, that is a credential given away. + +func TestSavedConfigIsOwnerOnly(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply") + } + root := t.TempDir() + cfg := &Config{ + Mode: ModeSolo, + SpaceRoot: root, + Backend: Backend{Driver: "s3", S3SecretKey: "not-for-everyone"}, + } + if err := Save(cfg); err != nil { + t.Fatal(err) + } + assertOwnerOnly(t, Path(root)) +} + +func TestSavedServerConfigIsOwnerOnly(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply") + } + dir := t.TempDir() + cfg := &ServerConfig{ + Mode: ModeServer, + DataDir: dir, + Backend: Backend{Driver: "sql", SQLDSN: "postgres://u:secret@db/cv"}, + } + if err := SaveServer(cfg); err != nil { + t.Fatal(err) + } + assertOwnerOnly(t, ServerConfigPathIn(dir)) +} + +// A config written by an earlier version is already on disk and world-readable. +// Nobody is going to notice that on their own, so reading it repairs it. +func TestLoadingRepairsAWorldReadableConfig(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply") + } + root := t.TempDir() + cfg := &Config{Mode: ModeSolo, SpaceRoot: root, Backend: Backend{Driver: "local"}} + if err := Save(cfg); err != nil { + t.Fatal(err) + } + // Put it back the way the old version left it. + if err := os.Chmod(Path(root), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := Load(root); err != nil { + t.Fatal(err) + } + assertOwnerOnly(t, Path(root)) +} + +func TestLoadingRepairsAWorldReadableServerConfig(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply") + } + dir := t.TempDir() + if err := SaveServer(&ServerConfig{Mode: ModeServer, DataDir: dir}); err != nil { + t.Fatal(err) + } + if err := os.Chmod(ServerConfigPathIn(dir), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := LoadServer(dir); err != nil { + t.Fatal(err) + } + assertOwnerOnly(t, ServerConfigPathIn(dir)) +} + +// A config left more restrictive than we ask for stays that way. +func TestRepairDoesNotLoosenPermissions(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply") + } + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte("mode: solo\n"), 0o400); err != nil { + t.Fatal(err) + } + if err := restrictSecretFile(path); err != nil { + t.Fatal(err) + } + st, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if st.Mode().Perm() != 0o400 { + t.Errorf("mode %o, want the stricter 0400 left alone", st.Mode().Perm()) + } +} + +func assertOwnerOnly(t *testing.T, path string) { + t.Helper() + st, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := st.Mode().Perm(); perm&0o077 != 0 { + t.Errorf("%s is mode %o; group and world can read credentials", path, perm) + } +} diff --git a/internal/config/server.go b/internal/config/server.go index bb34149..d4a12a8 100644 --- a/internal/config/server.go +++ b/internal/config/server.go @@ -8,6 +8,8 @@ import ( "time" "gopkg.in/yaml.v3" + + "github.com/orkcom-tech/contextverse/internal/logx" ) const ( @@ -186,6 +188,11 @@ func LoadServer(dataDir string) (*ServerConfig, error) { if err := yaml.Unmarshal(raw, &cfg); err != nil { return nil, fmt.Errorf("parse server config: %w", err) } + // Same repair-on-read as the client config: this one holds the backend's + // credentials for every space the server serves. + if err := restrictSecretFile(path); err != nil { + logx.L().Debug("could not tighten server config permissions", "path", path, "err", err) + } if cfg.DataDir == "" { cfg.DataDir = dataDir } @@ -317,10 +324,13 @@ func SaveServer(cfg *ServerConfig) error { return err } tmp := path + ".tmp" - if err := os.WriteFile(tmp, raw, 0o644); err != nil { + if err := os.WriteFile(tmp, raw, secretFileMode); err != nil { + return err + } + if err := os.Rename(tmp, path); err != nil { return err } - return os.Rename(tmp, path) + return restrictSecretFile(path) } // ServerExists reports whether server config is present. diff --git a/internal/mcpserver/read_test.go b/internal/mcpserver/read_test.go new file mode 100644 index 0000000..9a9f541 --- /dev/null +++ b/internal/mcpserver/read_test.go @@ -0,0 +1,169 @@ +package mcpserver + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// This package had no tests at all, and it is the surface that hands files to a +// language model. The reader compared path strings and stopped there, which a +// symlink walks straight past. +// +// It is not a hypothetical for a client space: its contents arrive from a +// server, and `contextd pull` writes the paths the server names. A hostile or +// compromised server plants a link, the model reads it, and the contents leave +// the machine inside a prompt. + +func spaceWith(t *testing.T, files map[string]string) string { + t.Helper() + root := t.TempDir() + for rel, body := range files { + abs := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(abs, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + return root +} + +func TestReadingAnOrdinaryFileWorks(t *testing.T) { + root := spaceWith(t, map[string]string{"team/principles.md": "how we work"}) + + got, err := readSpaceFile(root, "team/principles.md") + if err != nil { + t.Fatal(err) + } + if got != "how we work" { + t.Errorf("got %q", got) + } +} + +func TestASymlinkOutOfTheSpaceIsRefused(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need elevation on Windows") + } + secretDir := t.TempDir() + secret := filepath.Join(secretDir, "id_rsa") + if err := os.WriteFile(secret, []byte("PRIVATE KEY"), 0o600); err != nil { + t.Fatal(err) + } + + root := spaceWith(t, map[string]string{"team/real.md": "fine"}) + link := filepath.Join(root, "team", "notes.md") + if err := os.Symlink(secret, link); err != nil { + t.Fatal(err) + } + + got, err := readSpaceFile(root, "team/notes.md") + if err == nil { + t.Fatalf("read through a symlink out of the space and returned %q", got) + } + if strings.Contains(got, "PRIVATE KEY") { + t.Fatal("the secret was returned alongside the error") + } +} + +// A directory symlink is the same escape one level up. +func TestASymlinkedDirectoryIsRefused(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need elevation on Windows") + } + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "secret.md"), []byte("elsewhere"), 0o600); err != nil { + t.Fatal(err) + } + + root := spaceWith(t, nil) + if err := os.Symlink(outside, filepath.Join(root, "escape")); err != nil { + t.Fatal(err) + } + + if got, err := readSpaceFile(root, "escape/secret.md"); err == nil { + t.Fatalf("read through a symlinked directory and returned %q", got) + } +} + +// A link that stays inside the space is legitimate and must keep working. +func TestASymlinkInsideTheSpaceIsAllowed(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need elevation on Windows") + } + root := spaceWith(t, map[string]string{"team/real.md": "inside"}) + if err := os.Symlink(filepath.Join(root, "team", "real.md"), filepath.Join(root, "alias.md")); err != nil { + t.Fatal(err) + } + + got, err := readSpaceFile(root, "alias.md") + if err != nil { + t.Fatalf("a link within the space was refused: %v", err) + } + if got != "inside" { + t.Errorf("got %q", got) + } +} + +func TestTraversalIsRefused(t *testing.T) { + root := spaceWith(t, map[string]string{"notes.md": "here"}) + for _, bad := range []string{"../outside.md", "team/../../outside.md", "/etc/passwd", ""} { + if got, err := readSpaceFile(root, bad); err == nil { + t.Errorf("%q was accepted and returned %q", bad, got) + } + } +} + +// The whole body goes into a tool result bound for a model. Nothing in a context +// space is legitimately this big, and refusing is better than sending it. +func TestAnEnormousFileIsRefused(t *testing.T) { + root := spaceWith(t, map[string]string{"big.md": strings.Repeat("x", maxToolFileBytes+1)}) + + if _, err := readSpaceFile(root, "big.md"); err == nil { + t.Fatal("a file over the limit was read into a tool result") + } + if _, err := readSpaceFile(root, "big.md"); err != nil && !strings.Contains(err.Error(), "too large") { + t.Errorf("the error does not say why: %v", err) + } +} + +func TestADirectoryIsNotAFile(t *testing.T) { + root := spaceWith(t, map[string]string{"team/principles.md": "x"}) + if _, err := readSpaceFile(root, "team"); err == nil { + t.Fatal("a directory was read as a file") + } +} + +// The listing must not advertise what the reader will refuse, or the model is +// invited to ask for something that then fails. +func TestListingSkipsLinksOutOfTheSpace(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need elevation on Windows") + } + outside := t.TempDir() + secret := filepath.Join(outside, "secret.md") + if err := os.WriteFile(secret, []byte("elsewhere"), 0o600); err != nil { + t.Fatal(err) + } + + root := spaceWith(t, map[string]string{"real.md": "inside"}) + if err := os.Symlink(secret, filepath.Join(root, "escape.md")); err != nil { + t.Fatal(err) + } + + files, err := listFiles(root, "") + if err != nil { + t.Fatal(err) + } + for _, f := range files { + if f == "escape.md" { + t.Error("the listing offers a file that leaves the space") + } + } + if len(files) == 0 || files[0] != "real.md" { + t.Errorf("the real file went missing from the listing: %v", files) + } +} diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 6c63815..347d11b 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -14,6 +14,7 @@ import ( "github.com/orkcom-tech/contextverse/internal/logx" "github.com/orkcom-tech/contextverse/internal/search" "github.com/orkcom-tech/contextverse/internal/space" + "github.com/orkcom-tech/contextverse/internal/storage" "github.com/orkcom-tech/contextverse/internal/version" ) @@ -286,6 +287,12 @@ func listFiles(root, prefix string) ([]string, error) { if rel == "config.yaml" || rel == "template.yaml" { return nil } + // The listing must not advertise anything the reader will refuse. A + // symlink out of the space is skipped here so it never appears as a + // file the model can ask for. + if _, err := storage.ResolveUnder(root, rel); err != nil { + return nil + } if prefix != "" && !strings.HasPrefix(rel, strings.TrimPrefix(prefix, "/")) { return nil } @@ -298,23 +305,35 @@ func listFiles(root, prefix string) ([]string, error) { return out, err } +// maxToolFileBytes bounds one file handed to a model. Nothing in a context +// space is legitimately this big, and the whole body goes into a tool result +// that is about to be sent to somebody's language model. +const maxToolFileBytes = 1 << 20 + +// readSpaceFile reads one file from the space, refusing anything that is not +// really inside it. +// +// This compared strings and stopped there, which a symlink walks straight past: +// a link at team/notes.md pointing at ~/.ssh/id_rsa passes every check above and +// is then read and handed to the model. That is not hypothetical for a client +// space — its contents arrive from a server, and `contextd pull` writes what the +// server names. storage.ResolveUnder exists for exactly this and resolves the +// deepest existing ancestor before comparing, so the link is followed first and +// judged afterwards. func readSpaceFile(root, rel string) (string, error) { - rel = filepath.Clean(rel) - if rel == "." || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) { - return "", fmt.Errorf("invalid path %q", rel) - } - full := filepath.Join(root, rel) - // ensure still under root - absRoot, err := filepath.Abs(root) + full, err := storage.ResolveUnder(root, rel) if err != nil { - return "", err + return "", fmt.Errorf("refusing path %q: %w", rel, err) } - absFull, err := filepath.Abs(full) + st, err := os.Stat(full) if err != nil { return "", err } - if !strings.HasPrefix(absFull, absRoot+string(os.PathSeparator)) && absFull != absRoot { - return "", fmt.Errorf("path escapes space root") + if st.IsDir() { + return "", fmt.Errorf("%s is a directory", rel) + } + if st.Size() > maxToolFileBytes { + return "", fmt.Errorf("%s is %d bytes; too large to read into a tool result (limit %d)", rel, st.Size(), maxToolFileBytes) } data, err := os.ReadFile(full) if err != nil { diff --git a/internal/selfupdate/check.go b/internal/selfupdate/check.go new file mode 100644 index 0000000..d9c1f96 --- /dev/null +++ b/internal/selfupdate/check.go @@ -0,0 +1,262 @@ +// Package selfupdate tells someone a newer contextd exists, and otherwise says +// nothing at all. +// +// # The design problem is not the check, it is the noise +// +// A version notice is easy to write and easy to make hated. Three ways it goes +// wrong, in order of how much damage they do: +// +// 1. It corrupts something. `contextd mcp serve` speaks JSON-RPC over stdio; a +// stray line there breaks the AI client's parser. `--json` output is read by +// scripts. Anything printed to stdout can end up inside a pipe. So the +// notice goes to stderr, never stdout, and whole commands are excluded +// rather than trusted to be careful. +// 2. It costs time. A network call on the hot path makes every command wait for +// someone else's server. So the check never blocks: a command uses whatever +// is already in the cache and refreshes in the background for next time. +// 3. It repeats. Being told the same thing daily is what makes people reach for +// the off switch, and then they never hear about the release that matters. +// One notice per new version, and at most one check a day. +// +// The result is that a person sees a single line the first time a release +// appears, and scripts, CI and AI integrations see nothing ever. +package selfupdate + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strconv" + "strings" + "time" +) + +const ( + // checkEvery bounds how often we ask. A day is short enough to hear about a + // release the week it happens and long enough that nobody notices us asking. + checkEvery = 24 * time.Hour + // fetchTimeout bounds the background fetch. It is not on anyone's critical + // path, but a goroutine that never ends is still a leak. + fetchTimeout = 5 * time.Second + // releasesURL is the source of truth for "what is the latest". + releasesURL = "https://api.github.com/repos/orkcom-tech/contextverse/releases/latest" + // EnvDisable turns the whole thing off. + EnvDisable = "CONTEXTD_NO_UPDATE_CHECK" +) + +// state is what we remember between runs. +type state struct { + // LastCheck is when we last asked upstream, successful or not. Failures are + // recorded too: an offline machine must not retry on every command. + LastCheck time.Time `json:"last_check"` + // LatestSeen is the newest version upstream reported. + LatestSeen string `json:"latest_seen,omitempty"` + // Announced is the version we have already told this person about, so the + // same news is delivered once rather than daily. + Announced string `json:"announced,omitempty"` +} + +// Checker decides whether to say anything, and says it at most once. +type Checker struct { + // Current is the running version. + Current string + // CacheDir holds the state file. Empty uses the user cache directory. + CacheDir string + // Now is overridable for tests. + Now func() time.Time + // Fetch returns the latest published version. Overridable for tests. + Fetch func(ctx context.Context) (string, error) + // Disabled skips everything, whatever else is true. + Disabled bool +} + +// Notice returns the line to show, or "" for silence. +// +// It never blocks on the network: the answer comes from the cache, and a stale +// cache is refreshed in the background for the next run. The first run after +// install therefore says nothing, which is correct — somebody who just +// downloaded contextd does not need to be told about a release. +func (c *Checker) Notice(ctx context.Context) string { + if c.Disabled || os.Getenv(EnvDisable) != "" { + return "" + } + cur, ok := parseRelease(c.Current) + if !ok { + // Anything that is not a plain release — 0.0.0-dev from a working tree, + // a release candidate, a build stamped by hand — has no meaningful place + // in the sequence. Telling a contributor their working tree is out of + // date is noise, and it is the version they see most often. + return "" + } + + st, path := c.load() + if c.now().Sub(st.LastCheck) > checkEvery { + go c.refresh(path, st) + } + + latest, ok := parse(st.LatestSeen) + if !ok || !latest.newerThan(cur) { + return "" + } + if st.Announced == st.LatestSeen { + return "" // already said, once is enough + } + + st.Announced = st.LatestSeen + c.save(path, st) + return fmt.Sprintf("A newer contextd is available: %s (you have %s). https://github.com/orkcom-tech/contextverse/releases/latest", + st.LatestSeen, c.Current) +} + +func (c *Checker) now() time.Time { + if c.Now != nil { + return c.Now() + } + return time.Now() +} + +// refresh asks upstream and records the answer. Runs detached from the command +// that started it, so its failure is nobody's problem. +func (c *Checker) refresh(path string, st state) { + ctx, cancel := context.WithTimeout(context.Background(), fetchTimeout) + defer cancel() + + fetch := c.Fetch + if fetch == nil { + fetch = fetchLatest + } + latest, err := fetch(ctx) + // The timestamp moves either way. Recording only successes means an offline + // machine asks again on every single command. + st.LastCheck = c.now() + if err == nil && latest != "" { + st.LatestSeen = latest + } + c.save(path, st) +} + +func (c *Checker) statePath() string { + dir := c.CacheDir + if dir == "" { + base, err := os.UserCacheDir() + if err != nil { + return "" + } + dir = filepath.Join(base, "contextverse") + } + return filepath.Join(dir, "update-check.json") +} + +func (c *Checker) load() (state, string) { + path := c.statePath() + if path == "" { + return state{}, "" + } + raw, err := os.ReadFile(path) + if err != nil { + return state{}, path + } + var st state + if err := json.Unmarshal(raw, &st); err != nil { + return state{}, path + } + return st, path +} + +// save is best-effort throughout. Nothing here is worth failing a command over, +// and the worst case of a lost write is asking again tomorrow. +func (c *Checker) save(path string, st state) { + if path == "" { + return + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return + } + raw, err := json.Marshal(st) + if err != nil { + return + } + tmp := path + ".tmp" + if err := os.WriteFile(tmp, raw, 0o600); err != nil { + return + } + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + } +} + +func fetchLatest(ctx context.Context) (string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releasesURL, nil) + if err != nil { + return "", err + } + req.Header.Set("Accept", "application/vnd.github+json") + res, err := http.DefaultClient.Do(req) + if err != nil { + return "", err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return "", fmt.Errorf("release feed: HTTP %d", res.StatusCode) + } + var body struct { + TagName string `json:"tag_name"` + } + if err := json.NewDecoder(io.LimitReader(res.Body, 1<<20)).Decode(&body); err != nil { + return "", err + } + return strings.TrimSpace(body.TagName), nil +} + +// semver is the subset needed to answer "is that one newer". +type semver struct{ major, minor, patch int } + +// parseRelease accepts only a plain release: 1.2.3 or v1.2.3, and nothing with +// a prerelease or build suffix. +// +// Kept separate from parse because the two questions differ. Ordering can +// ignore a suffix; deciding whether to speak at all cannot, or 0.0.0-dev — the +// version every contributor runs — gets told it is nine releases behind. +func parseRelease(v string) (semver, bool) { + trimmed := strings.TrimPrefix(strings.TrimSpace(v), "v") + if strings.ContainsAny(trimmed, "-+") { + return semver{}, false + } + return parse(v) +} + +func parse(v string) (semver, bool) { + v = strings.TrimSpace(v) + v = strings.TrimPrefix(v, "v") + // A prerelease or build suffix does not change the ordering question here. + if i := strings.IndexAny(v, "-+"); i >= 0 { + v = v[:i] + } + parts := strings.Split(v, ".") + if len(parts) != 3 { + return semver{}, false + } + var out semver + for i, dst := range []*int{&out.major, &out.minor, &out.patch} { + n, err := strconv.Atoi(parts[i]) + if err != nil || n < 0 { + return semver{}, false + } + *dst = n + } + return out, true +} + +func (s semver) newerThan(other semver) bool { + if s.major != other.major { + return s.major > other.major + } + if s.minor != other.minor { + return s.minor > other.minor + } + return s.patch > other.patch +} diff --git a/internal/selfupdate/check_test.go b/internal/selfupdate/check_test.go new file mode 100644 index 0000000..550e670 --- /dev/null +++ b/internal/selfupdate/check_test.go @@ -0,0 +1,334 @@ +package selfupdate + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" +) + +// A version notice is easy to write and easy to make hated. These are mostly +// about the silence: when it must say nothing, and that it says a given thing +// exactly once. + +func checker(t *testing.T, current, latest string) *Checker { + t.Helper() + return &Checker{ + Current: current, + CacheDir: t.TempDir(), + Now: func() time.Time { return time.Date(2026, 7, 30, 12, 0, 0, 0, time.UTC) }, + Fetch: func(context.Context) (string, error) { return latest, nil }, + } +} + +// warm runs one Notice and waits for the background refresh to record want, +// which is what a second run of the command would see. +// +// Waiting for a specific value rather than for "anything recorded": after the +// first warm there is already a timestamp, so a weaker condition returns before +// the second refresh has landed and the test reads the previous answer. +func warm(t *testing.T, c *Checker, want string) { + t.Helper() + _ = c.Notice(context.Background()) + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if st, _ := c.load(); st.LatestSeen == want { + return + } + time.Sleep(5 * time.Millisecond) + } + st, _ := c.load() + t.Fatalf("the background refresh recorded %q, want %q", st.LatestSeen, want) +} + +// The first run says nothing: somebody who has just installed contextd does not +// need to be told about a release, and there is no cached answer yet anyway. +func TestTheFirstRunIsSilent(t *testing.T) { + c := checker(t, "0.7.0", "0.9.0") + if notice := c.Notice(context.Background()); notice != "" { + t.Errorf("the first run spoke: %q", notice) + } +} + +func TestASecondRunReportsANewerRelease(t *testing.T) { + c := checker(t, "0.7.0", "0.9.0") + warm(t, c, "0.9.0") + + notice := c.Notice(context.Background()) + if !strings.Contains(notice, "0.9.0") || !strings.Contains(notice, "0.7.0") { + t.Fatalf("notice = %q; want both versions named", notice) + } +} + +// Being told the same thing every day is what makes people turn it off, and +// then they never hear about the release that matters. +func TestTheSameVersionIsAnnouncedOnce(t *testing.T) { + c := checker(t, "0.7.0", "0.9.0") + warm(t, c, "0.9.0") + + if first := c.Notice(context.Background()); first == "" { + t.Fatal("nothing was said at all") + } + for i := 0; i < 5; i++ { + if again := c.Notice(context.Background()); again != "" { + t.Fatalf("said it again on run %d: %q", i+2, again) + } + } +} + +// A newer release after one has already been announced is news again. +func TestANewerReleaseIsAnnouncedAgain(t *testing.T) { + c := checker(t, "0.7.0", "0.9.0") + warm(t, c, "0.9.0") + if c.Notice(context.Background()) == "" { + t.Fatal("the first release was not announced") + } + + // A day later, upstream has moved again. + c.Now = func() time.Time { return time.Date(2026, 8, 1, 12, 0, 0, 0, time.UTC) } + c.Fetch = func(context.Context) (string, error) { return "1.0.0", nil } + warm(t, c, "1.0.0") + + if notice := c.Notice(context.Background()); !strings.Contains(notice, "1.0.0") { + t.Errorf("notice = %q; want the new release", notice) + } +} + +func TestNothingIsSaidWhenUpToDateOrAhead(t *testing.T) { + for _, tc := range []struct{ current, latest string }{ + {"0.9.0", "0.9.0"}, + {"1.0.0", "0.9.0"}, // a local build ahead of the feed + } { + c := checker(t, tc.current, tc.latest) + warm(t, c, tc.latest) + if notice := c.Notice(context.Background()); notice != "" { + t.Errorf("current %s vs latest %s: said %q", tc.current, tc.latest, notice) + } + } +} + +// Telling a contributor their working tree is out of date is noise, and it is +// the version they see most often. A dev build does not even ask upstream: there +// is no answer that would change what it does. +func TestADevelopmentBuildIsSilentAndDoesNotAsk(t *testing.T) { + var asked bool + var mu sync.Mutex + c := checker(t, "0.0.0-dev", "9.9.9") + c.Fetch = func(context.Context) (string, error) { + mu.Lock() + asked = true + mu.Unlock() + return "9.9.9", nil + } + + for i := 0; i < 3; i++ { + if notice := c.Notice(context.Background()); notice != "" { + t.Fatalf("a dev build was nagged: %q", notice) + } + } + time.Sleep(50 * time.Millisecond) + + mu.Lock() + defer mu.Unlock() + if asked { + t.Error("a dev build went to the network for an answer it cannot use") + } +} + +// A release candidate is not a release either. +func TestAPrereleaseIsSilent(t *testing.T) { + for _, v := range []string{"1.0.0-rc1", "0.9.0-beta.2", "1.2.3+build.5"} { + c := checker(t, v, "9.9.9") + if notice := c.Notice(context.Background()); notice != "" { + t.Errorf("%s was nagged: %q", v, notice) + } + } +} + +func TestTheEnvironmentSwitchSilencesEverything(t *testing.T) { + c := checker(t, "0.7.0", "0.9.0") + warm(t, c, "0.9.0") + t.Setenv(EnvDisable, "1") + + if notice := c.Notice(context.Background()); notice != "" { + t.Errorf("%s was ignored: %q", EnvDisable, notice) + } +} + +// A command must never wait for somebody else's server. The check reads the +// cache and refreshes behind it. +func TestNoticeDoesNotWaitForTheNetwork(t *testing.T) { + release := make(chan struct{}) + c := checker(t, "0.7.0", "0.9.0") + c.Fetch = func(ctx context.Context) (string, error) { + <-release // never answers until the test says so + return "0.9.0", nil + } + + done := make(chan struct{}) + go func() { _ = c.Notice(context.Background()); close(done) }() + select { + case <-done: + case <-time.After(time.Second): + close(release) + t.Fatal("Notice blocked on the fetch") + } + + // Let the refresh finish before the temp directory is cleaned up, or it + // writes its state into a directory the test framework is removing. + close(release) + warmDeadline := time.Now().Add(2 * time.Second) + for time.Now().Before(warmDeadline) { + if st, _ := c.load(); !st.LastCheck.IsZero() { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("the released fetch never recorded anything") +} + +// An offline machine must not ask again on every single command. The timestamp +// moves whether or not the fetch worked. +func TestAFailedCheckStillCountsAsAsking(t *testing.T) { + var asks int + var mu sync.Mutex + c := checker(t, "0.7.0", "") + c.Fetch = func(context.Context) (string, error) { + mu.Lock() + asks++ + mu.Unlock() + return "", errors.New("no network") + } + + waitForOneAsk(t, c) + for i := 0; i < 5; i++ { + _ = c.Notice(context.Background()) + } + time.Sleep(50 * time.Millisecond) + + mu.Lock() + defer mu.Unlock() + if asks != 1 { + t.Errorf("asked %d times after a failure; an offline machine would retry on every command", asks) + } +} + +// Between checks the cached answer is used and upstream is left alone. +func TestUpstreamIsAskedAtMostOncePerDay(t *testing.T) { + var asks int + var mu sync.Mutex + c := checker(t, "0.7.0", "0.9.0") + c.Fetch = func(context.Context) (string, error) { + mu.Lock() + asks++ + mu.Unlock() + return "0.9.0", nil + } + + warm(t, c, "0.9.0") + for i := 0; i < 10; i++ { + _ = c.Notice(context.Background()) + } + time.Sleep(50 * time.Millisecond) + + mu.Lock() + got := asks + mu.Unlock() + if got != 1 { + t.Errorf("asked upstream %d times within the window, want 1", got) + } + + // A day later it is allowed to ask again. + c.Now = func() time.Time { return time.Date(2026, 8, 1, 12, 0, 0, 0, time.UTC) } + _ = c.Notice(context.Background()) + time.Sleep(50 * time.Millisecond) + + mu.Lock() + defer mu.Unlock() + if asks != 2 { + t.Errorf("asked %d times in total; the window never reopened", asks) + } +} + +// waitForOneAsk waits for a refresh that records no version, only that it tried. +func waitForOneAsk(t *testing.T, c *Checker) { + t.Helper() + _ = c.Notice(context.Background()) + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if st, _ := c.load(); !st.LastCheck.IsZero() { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("the failed check was never recorded") +} + +func TestVersionParsing(t *testing.T) { + for _, tc := range []struct { + in string + ok bool + major int + }{ + {"1.2.3", true, 1}, + {"v1.2.3", true, 1}, + {"v10.0.1", true, 10}, + {"1.2.3-rc1", true, 1}, + {"1.2.3+build.5", true, 1}, + {"0.0.0-dev", true, 0}, + {"1.2", false, 0}, + {"", false, 0}, + {"latest", false, 0}, + {"1.2.x", false, 0}, + } { + got, ok := parse(tc.in) + if ok != tc.ok { + t.Errorf("parse(%q) ok = %v, want %v", tc.in, ok, tc.ok) + continue + } + if ok && got.major != tc.major { + t.Errorf("parse(%q).major = %d, want %d", tc.in, got.major, tc.major) + } + } +} + +func TestOrdering(t *testing.T) { + for _, tc := range []struct { + newer, older string + want bool + }{ + {"1.0.0", "0.9.9", true}, + {"0.10.0", "0.9.0", true}, // not string order + {"0.9.10", "0.9.9", true}, + {"0.9.0", "0.9.0", false}, + {"0.9.0", "1.0.0", false}, + } { + a, _ := parse(tc.newer) + b, _ := parse(tc.older) + if got := a.newerThan(b); got != tc.want { + t.Errorf("%s newerThan %s = %v, want %v", tc.newer, tc.older, got, tc.want) + } + } +} + +// parse and parseRelease answer different questions: ordering may ignore a +// suffix, deciding whether to speak at all may not. +func TestOnlyPlainReleasesCount(t *testing.T) { + for _, tc := range []struct { + in string + release bool + }{ + {"1.2.3", true}, + {"v1.2.3", true}, + {"1.2.3-rc1", false}, + {"0.0.0-dev", false}, + {"1.2.3+build.5", false}, + {"nonsense", false}, + } { + if _, ok := parseRelease(tc.in); ok != tc.release { + t.Errorf("parseRelease(%q) = %v, want %v", tc.in, ok, tc.release) + } + } +} diff --git a/internal/server/audit_emit.go b/internal/server/audit_emit.go index 9d109fe..47b4c08 100644 --- a/internal/server/audit_emit.go +++ b/internal/server/audit_emit.go @@ -18,14 +18,6 @@ func (s *Server) auditDenied(r *http.Request, action, space, target, msg string) s.auditWrite(r, action, space, target, audit.ResultDenied, msg, nil) } -func (s *Server) auditError(r *http.Request, action, space, target string, err error) { - msg := "" - if err != nil { - msg = err.Error() - } - s.auditWrite(r, action, space, target, audit.ResultError, msg, nil) -} - func (s *Server) auditWrite(r *http.Request, action, space, target, result, errMsg string, diff *audit.Diff) { if s.Audit == nil { return diff --git a/internal/server/bodylimit_test.go b/internal/server/bodylimit_test.go new file mode 100644 index 0000000..5f4d144 --- /dev/null +++ b/internal/server/bodylimit_test.go @@ -0,0 +1,66 @@ +package server + +import ( + "bytes" + "errors" + "strings" + "testing" +) + +// io.LimitReader stops at the cap and io.ReadAll calls that success, so a body +// that had more to give was indistinguishable from one that ended. handlePutFile +// read through a 32 MiB limit reader and stored whatever came back: upload 40 +// MiB and the server answered 200 with a file two thirds of the way through a +// sentence. Silent truncation on a write path is the worst kind of bug — the +// client is told its document is safe. + +func TestABodyAtTheLimitIsAccepted(t *testing.T) { + body := strings.Repeat("x", 100) + got, err := readAtMost(bytes.NewBufferString(body), 100) + if err != nil { + t.Fatalf("a body exactly at the limit was refused: %v", err) + } + if string(got) != body { + t.Errorf("read %d bytes, want %d", len(got), len(body)) + } +} + +func TestABodyOverTheLimitIsRefusedNotTruncated(t *testing.T) { + body := strings.Repeat("x", 101) + got, err := readAtMost(bytes.NewBufferString(body), 100) + if !errors.Is(err, errBodyTooLarge) { + t.Fatalf("got (%d bytes, %v), want a refusal", len(got), err) + } + if got != nil { + t.Errorf("returned %d bytes alongside the error; a caller could store them", len(got)) + } +} + +// One byte over is the case that matters: it is the smallest overrun, and the +// one a naive limit reader is least likely to notice. +func TestOneByteOverIsStillRefused(t *testing.T) { + for _, limit := range []int64{1, 2, 1024, 5 << 20} { + body := strings.Repeat("y", int(limit)+1) + if _, err := readAtMost(bytes.NewBufferString(body), limit); !errors.Is(err, errBodyTooLarge) { + t.Errorf("limit %d: %d bytes accepted, want a refusal", limit, len(body)) + } + } +} + +// A limit of zero means "unset", not "accept nothing" — a space with no +// configured maximum must still be able to take an ordinary file. +func TestAnUnsetLimitFallsBackToADefault(t *testing.T) { + if _, err := readAtMost(bytes.NewBufferString("small"), 0); err != nil { + t.Fatalf("an unset limit refused a small body: %v", err) + } +} + +func TestAnEmptyBodyIsFine(t *testing.T) { + got, err := readAtMost(bytes.NewBuffer(nil), 100) + if err != nil { + t.Fatal(err) + } + if len(got) != 0 { + t.Errorf("read %d bytes from an empty body", len(got)) + } +} diff --git a/internal/server/clientip.go b/internal/server/clientip.go index 4eb1ab2..ca16d76 100644 --- a/internal/server/clientip.go +++ b/internal/server/clientip.go @@ -1,6 +1,8 @@ package server import ( + "crypto/sha256" + "encoding/hex" "net" "net/http" "strings" @@ -82,11 +84,20 @@ func (s *Server) clientIP(r *http.Request) string { return remoteIP(r.RemoteAddr) } +// rateLimitKey identifies who to count a request against. +// +// The token is hashed rather than used directly. The limiter keeps its buckets +// in a map that lives for the life of the process, and the old key put a live +// bearer token in it — a working credential sitting in memory long after the +// request that carried it, reachable by anything that can read the heap: a core +// dump, a crash reporter, a debugger. The limiter needs to tell callers apart, +// which a hash does exactly as well. func (s *Server) rateLimitKey(r *http.Request) string { if h := r.Header.Get("Authorization"); strings.HasPrefix(h, "Bearer ") { tok := strings.TrimSpace(strings.TrimPrefix(h, "Bearer ")) if tok != "" { - return "bearer:" + tok + sum := sha256.Sum256([]byte(tok)) + return "bearer:" + hex.EncodeToString(sum[:16]) } } return "ip:" + s.clientIP(r) diff --git a/internal/server/clientip_test.go b/internal/server/clientip_test.go index 7154e3b..a62d0a5 100644 --- a/internal/server/clientip_test.go +++ b/internal/server/clientip_test.go @@ -2,6 +2,7 @@ package server import ( "net/http" + "strings" "testing" ) @@ -39,15 +40,24 @@ func TestForwardedForHonouredFromTrustedProxy(t *testing.T) { } } +// A token caller is counted per token rather than per address. The key is +// derived from the token, not equal to it — this used to assert the literal +// "bearer:", which pinned a live credential into the limiter's map +// as the intended behaviour. See TestTheRateLimitKeyDoesNotCarryTheToken. func TestRateLimitKeyPrefersToken(t *testing.T) { s := &Server{} r := req("203.0.113.7:1", "") r.Header.Set("Authorization", "Bearer cv-kim-secret") - if got := s.rateLimitKey(r); got != "bearer:cv-kim-secret" { - t.Fatalf("token callers must be limited per token, got %q", got) + + tokenKey := s.rateLimitKey(r) + if !strings.HasPrefix(tokenKey, "bearer:") { + t.Fatalf("token callers must be limited per token, got %q", tokenKey) + } + if ipKey := s.rateLimitKey(req("203.0.113.7:1", "1.2.3.4")); ipKey != "ip:203.0.113.7" { + t.Fatalf("anonymous callers must be limited per real peer, got %q", ipKey) } - if got := s.rateLimitKey(req("203.0.113.7:1", "1.2.3.4")); got != "ip:203.0.113.7" { - t.Fatalf("anonymous callers must be limited per real peer, got %q", got) + if tokenKey == "ip:203.0.113.7" { + t.Fatal("a token caller fell back to their address") } } diff --git a/internal/server/requestid_test.go b/internal/server/requestid_test.go new file mode 100644 index 0000000..d1b4c58 --- /dev/null +++ b/internal/server/requestid_test.go @@ -0,0 +1,117 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The request id goes into structured logs and into the error envelope, and it +// came from a header the caller controls with nothing checked. A newline forges +// a log line; control bytes confuse whatever reads them; an unbounded string is +// an unbounded log record. + +func TestAWellFormedRequestIDIsKept(t *testing.T) { + for _, id := range []string{ + "abc123", + "trace-1234", + "a.b.c", + "span:0af7651916cd43dd", + "UPPER_case-123", + } { + if got := sanitizeRequestID(id); got != id { + t.Errorf("sanitizeRequestID(%q) = %q; a legitimate id was discarded", id, got) + } + } +} + +func TestADangerousRequestIDIsDiscarded(t *testing.T) { + for _, id := range []string{ + "line\nINFO forged=\"log entry\"", // the whole point + "tab\there", + "null\x00byte", + "esc\x1b[31m", + "spaces here", + `quote"and'`, + "unicode-Привет", + strings.Repeat("x", maxRequestIDLen+1), + } { + if got := sanitizeRequestID(id); got != "" { + t.Errorf("sanitizeRequestID(%q) kept %q", id, got) + } + } +} + +// A discarded id must be replaced, not left empty: every request needs one. +func TestAForgedIDIsReplacedNotEchoed(t *testing.T) { + s := &Server{} + var seen string + h := s.withRequestID(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen, _ = r.Context().Value(requestIDKey).(string) + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/health", nil) + req.Header.Set("X-Request-Id", "evil\nINFO level=forged") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if seen == "" { + t.Fatal("no request id was assigned") + } + if strings.ContainsAny(seen, "\n\r") { + t.Errorf("the forged id reached the handler: %q", seen) + } + if echoed := rec.Header().Get("X-Request-Id"); echoed != seen { + t.Errorf("header says %q, context says %q", echoed, seen) + } +} + +// Generated ids are random. The old ones were UnixNano: guessable, so a caller +// could predict somebody else's, and identical for two requests in the same +// tick on a platform with a coarse clock — exactly when telling them apart +// matters. +func TestGeneratedIDsAreUniqueAndNotAClock(t *testing.T) { + seen := map[string]bool{} + for i := 0; i < 1000; i++ { + id := randomRequestID() + if seen[id] { + t.Fatalf("duplicate generated id %q after %d", id, i) + } + seen[id] = true + if sanitizeRequestID(id) != id { + t.Fatalf("a generated id %q does not survive its own validation", id) + } + } +} + +// The rate limiter keeps its buckets for the life of the process. A live bearer +// token in that map is a working credential sitting in memory long after the +// request that carried it. +func TestTheRateLimitKeyDoesNotCarryTheToken(t *testing.T) { + s := &Server{} + const token = "cv-kim-supersecret" + + req := httptest.NewRequest(http.MethodGet, "/api/v1/spaces", nil) + req.Header.Set("Authorization", "Bearer "+token) + req.RemoteAddr = "203.0.113.7:4242" + + key := s.rateLimitKey(req) + if strings.Contains(key, token) { + t.Fatalf("the key holds the token: %q", key) + } + if !strings.HasPrefix(key, "bearer:") { + t.Errorf("key = %q; token callers should still be counted per token", key) + } + + // Same token, same bucket: hashing must not break the limiting. + if again := s.rateLimitKey(req); again != key { + t.Error("the same token produced two different keys") + } + + other := httptest.NewRequest(http.MethodGet, "/api/v1/spaces", nil) + other.Header.Set("Authorization", "Bearer cv-sam-different") + if s.rateLimitKey(other) == key { + t.Error("two tokens share a bucket") + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 86eadfc..3e84878 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2,7 +2,9 @@ package server import ( "context" + "crypto/rand" "encoding/base64" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -415,11 +417,25 @@ func (s *Server) auth(next http.HandlerFunc) http.Handler { }) } +// maxRequestIDLen bounds a caller-supplied correlation id. +const maxRequestIDLen = 64 + +// withRequestID gives every request an id for logs and error bodies. +// +// A caller may bring its own so a trace can be followed across services, but +// what it brings is checked. The value went into structured logs and into the +// error envelope verbatim: newlines forge log lines, control bytes confuse +// whatever reads them, and an unbounded string is an unbounded log record. +// +// The generated id is random rather than the clock. UnixNano is guessable — it +// let a caller predict and then claim somebody else's id — and on a platform +// with a coarse clock two requests in the same tick shared one, which is +// precisely when correlating them matters. func (s *Server) withRequestID(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - id := r.Header.Get("X-Request-Id") + id := sanitizeRequestID(r.Header.Get("X-Request-Id")) if id == "" { - id = fmt.Sprintf("%d", time.Now().UnixNano()) + id = randomRequestID() } w.Header().Set("X-Request-Id", id) ctx := context.WithValue(r.Context(), requestIDKey, id) @@ -427,6 +443,35 @@ func (s *Server) withRequestID(next http.Handler) http.Handler { }) } +// sanitizeRequestID keeps a caller's id only if it is plainly printable and +// short. Anything else is discarded rather than escaped: a correlation id has no +// business carrying punctuation we would have to reason about. +func sanitizeRequestID(id string) string { + if id == "" || len(id) > maxRequestIDLen { + return "" + } + for i := 0; i < len(id); i++ { + c := id[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + case c == '-', c == '_', c == '.', c == ':': + default: + return "" + } + } + return id +} + +func randomRequestID() string { + var b [12]byte + if _, err := rand.Read(b[:]); err != nil { + // Only reachable if the system entropy source is broken, at which point + // a unique-ish id is the least of anyone's problems. + return fmt.Sprintf("t%d", time.Now().UnixNano()) + } + return hex.EncodeToString(b[:]) +} + func (s *Server) withAccessLog(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/metrics" || strings.HasPrefix(r.URL.Path, "/ui/static/") { @@ -524,7 +569,7 @@ func (s *Server) handleUserpassLogin(w http.ResponseWriter, r *http.Request) { writeErr(w, r, http.StatusBadRequest, "invalid_request", "invalid json", nil) return } - tok, rec, err := s.Auth.LoginUserpass(body.Username, body.Password) + tok, rec, err := s.Auth.LoginUserpassFrom(body.Username, body.Password, s.clientIP(r)) if err != nil { s.auditWrite(r, "auth.login", "", body.Username, audit.ResultDenied, err.Error(), nil) if errors.Is(err, auth.ErrLockedOut) { @@ -839,7 +884,21 @@ func (s *Server) handlePutFile(w http.ResponseWriter, r *http.Request) { writeErr(w, r, http.StatusBadRequest, "invalid_request", err.Error(), nil) return } - data, err := io.ReadAll(io.LimitReader(r.Body, 32<<20)) + // Bounded by the space's own limit, and one byte past it is an error rather + // than a shorter file. io.LimitReader stops at the cap and reports success, + // so the previous 32 MiB reader silently stored a truncated document and + // answered 200 — a client that uploaded 40 MiB was told its file was safe. + // The cap sits above max_file_size so the quota check still produces the + // specific "file too large" answer for the ordinary case. + limit := s.Spaces.QuotasFor(name).MaxFileSize + data, err := readAtMost(r.Body, limit) + if errors.Is(err, errBodyTooLarge) { + writeErr(w, r, http.StatusRequestEntityTooLarge, "quota_exceeded", + fmt.Sprintf("file too large: limit %d bytes", limit), map[string]any{ + "quota": "max_file_size", "limit": limit, + }) + return + } if err != nil { writeErr(w, r, http.StatusBadRequest, "invalid_request", "read body", nil) return @@ -1088,6 +1147,30 @@ func (s *Server) bumpHead(ctx context.Context, name string) (storage.Version, er return next, nil } +// errBodyTooLarge marks a request body that ran past the limit, as opposed to +// one that ended there. +var errBodyTooLarge = errors.New("request body over limit") + +// readAtMost reads a body of at most limit bytes and refuses a longer one. +// +// The distinction is the whole point: io.LimitReader returns EOF at the cap and +// io.ReadAll calls that success, so reading a stream that had more to give is +// indistinguishable from reading one that did not. Asking for one extra byte is +// what tells the two apart. +func readAtMost(body io.Reader, limit int64) ([]byte, error) { + if limit <= 0 { + limit = 5 << 20 + } + data, err := io.ReadAll(io.LimitReader(body, limit+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > limit { + return nil, errBodyTooLarge + } + return data, nil +} + func parseIfMatch(h string) (storage.Version, error) { h = strings.TrimSpace(h) if h == "" { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 17af2be..43afa9a 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -163,7 +163,6 @@ func TestServerPushPullFlow(t *testing.T) { } // secret-scan blocks known patterns - head2 := headBody.Space // refresh head after push req, _ = http.NewRequest(http.MethodGet, ts.URL+"/api/v1/spaces/team/head", nil) req.Header.Set("Authorization", "Bearer "+token) @@ -173,7 +172,7 @@ func TestServerPushPullFlow(t *testing.T) { } _ = json.NewDecoder(res.Body).Decode(&headBody) res.Body.Close() - head2 = headBody.Space + head2 := headBody.Space leak, _ := json.Marshal(map[string]any{ "expected_head": head2, "ops": []map[string]string{{ diff --git a/internal/server/ui_handlers.go b/internal/server/ui_handlers.go index a4c1891..c9f57aa 100644 --- a/internal/server/ui_handlers.go +++ b/internal/server/ui_handlers.go @@ -362,7 +362,7 @@ func (s *Server) handleLoginPost(w http.ResponseWriter, r *http.Request) { err error ) if user != "" && pass != "" { - tok, _, loginErr := s.Auth.LoginUserpass(user, pass) + tok, _, loginErr := s.Auth.LoginUserpassFrom(user, pass, s.clientIP(r)) if loginErr != nil { s.auditWrite(r, "auth.login", "", user, audit.ResultDenied, loginErr.Error(), nil) msg := "invalid credentials" diff --git a/internal/spacesvc/push.go b/internal/spacesvc/push.go index 4432480..d772ea1 100644 --- a/internal/spacesvc/push.go +++ b/internal/spacesvc/push.go @@ -309,12 +309,19 @@ func livePutVersion(ctx context.Context, fl *storage.FileLog, path string) (int, return n, n > 0 } -// inventory returns the on-disk size of every file in the space and their total. +// inventory returns the size of every file in the space and their total, as the +// backend reports them. // -// A list failure is an error rather than a pass. The previous behaviour returned -// nil — "don't block on list failure" — which made an unreadable space an -// unlimited one, the exact failure QuotasFor's own comment warns about two -// screens above. +// The sizes used to come from os.Stat on the working-tree mirror. That mirror is +// written by whichever replica handled the write, so with s3 or sql every other +// replica saw the files as absent and counted them as nothing — a space read as +// far smaller than it is, and a quota that lets it grow past its limit. The +// documented stateless HA is precisely that arrangement, so the failure was not +// hypothetical for anyone running more than one process. +// +// A list failure is an error rather than a pass. The behaviour before that +// returned nil — "don't block on list failure" — which made an unreadable space +// an unlimited one, the exact failure QuotasFor's own comment warns about. func (s *Service) inventory(ctx context.Context, name string) (map[string]int64, int64, error) { entries, err := s.Tree(ctx, name) if err != nil { @@ -323,16 +330,19 @@ func (s *Service) inventory(ctx context.Context, name string) (map[string]int64, sizes := make(map[string]int64, len(entries)) var total int64 for _, e := range entries { - p, err := s.treePath(name, e.Path) - if err != nil { - continue - } - st, err := os.Stat(p) - if err != nil { - continue + size := e.Size + if size == 0 { + // The backend did not answer. Fall back to the local mirror rather + // than counting the file as nothing: an unknown size read as zero is + // free space that is not there. + if p, perr := s.treePath(name, e.Path); perr == nil { + if st, serr := os.Stat(p); serr == nil { + size = st.Size() + } + } } - sizes[e.Path] = st.Size() - total += st.Size() + sizes[e.Path] = size + total += size } return sizes, total, nil } diff --git a/internal/spacesvc/service.go b/internal/spacesvc/service.go index 33c0a15..4ea7376 100644 --- a/internal/spacesvc/service.go +++ b/internal/spacesvc/service.go @@ -433,13 +433,17 @@ func (s *Service) SpaceUsage(ctx context.Context, name string) (bytes int64, fil if err != nil { return 0, 0, err } + // Same reasoning as inventory: the backend knows what it holds, and the + // working-tree mirror only exists on whichever replica did the writing. for _, e := range entries { - p, err := s.treePath(name, e.Path) - if err != nil { + if e.Size > 0 { + bytes += e.Size continue } - if st, err := os.Stat(p); err == nil { - bytes += st.Size() + if p, perr := s.treePath(name, e.Path); perr == nil { + if st, serr := os.Stat(p); serr == nil { + bytes += st.Size() + } } } return bytes, len(entries), nil diff --git a/internal/storage/backend.go b/internal/storage/backend.go index 30fc20e..fa28128 100644 --- a/internal/storage/backend.go +++ b/internal/storage/backend.go @@ -11,17 +11,28 @@ import ( type Version string // Entry is a listed object. +// +// Size is what the object holds, in bytes, as the backend knows it. It exists +// because quota accounting used to list the backend and then stat the local +// working-tree mirror for sizes — which works on the one machine that wrote the +// file and nowhere else. With s3 or sql the mirror is per-replica, so every +// other replica read the space as smaller than it is and let it grow past its +// limit; the documented stateless HA is exactly that arrangement. +// +// A backend that genuinely cannot answer cheaply may leave it zero, and callers +// treat zero as "unknown" rather than "empty". type Entry struct { Path string Version Version + Size int64 } // Common errors. var ( - ErrNotFound = errors.New("storage: not found") - ErrConflict = errors.New("storage: version conflict") - ErrNotSupported = errors.New("storage: not supported") - ErrInvalidArgument = errors.New("storage: invalid argument") + ErrNotFound = errors.New("storage: not found") + ErrConflict = errors.New("storage: version conflict") + ErrNotSupported = errors.New("storage: not supported") + ErrInvalidArgument = errors.New("storage: invalid argument") ) // Backend is the narrow pluggable store: blobs + optimistic CAS + scope heads. diff --git a/internal/storage/filelog.go b/internal/storage/filelog.go index 8a1a83c..344361e 100644 --- a/internal/storage/filelog.go +++ b/internal/storage/filelog.go @@ -215,7 +215,18 @@ func (f *FileLog) LiveVersion(ctx context.Context, path string) (Version, error) return FormatFileVersion(1), nil } -// Get returns live content. +// Get returns the current version's content. +// +// Read from the version blob the metadata names, not from the live copy. Those +// are two objects and a write cannot update both at once, so reading one while +// labelling it from the other is how a caller ends up holding version N's bytes +// stamped N+1 — and then writing back against a token that describes content +// they never saw. +// +// The live copy stays as the mirror that makes a space enumerable: List walks +// real paths, and a tree of hashed version blobs would not be a space anybody +// could look at. It is a mirror, though, and this is the one place that used to +// treat it as the source of truth. func (f *FileLog) Get(ctx context.Context, path string) ([]byte, Version, error) { m, err := f.loadMeta(ctx, path) if err != nil { @@ -226,13 +237,22 @@ func (f *FileLog) Get(ctx context.Context, path string) ([]byte, Version, error) return nil, "", err } if m.Current == 0 { + // No version history: an unversioned blob written before the file log, + // or nothing at all. data, ver, err := f.Backend.Get(ctx, path) if err != nil { return nil, "", err } return data, ver, nil } - data, _, err := f.Backend.Get(ctx, path) + data, _, err := f.Backend.Get(ctx, verBlobPath(path, m.Current)) + if errors.Is(err, ErrNotFound) { + // The metadata names a version whose blob is gone — pruned, destroyed, + // or lost. Falling back to the live copy would hand back content that + // does not belong to the version being reported, so say what happened. + return nil, "", fmt.Errorf("%s is recorded at version %d but that version's content is missing: %w", + path, m.Current, ErrNotFound) + } if err != nil { return nil, "", err } @@ -326,25 +346,56 @@ func (f *FileLog) Put(ctx context.Context, path string, data []byte, expected Ve } } } + // Three writes, and the order decides what a half-finished one leaves + // behind. + // + // It used to run version blob, then live blob, then metadata. Stop after the + // second and the live file holds the new content while the metadata still + // says the old version is current — so the file reads at version N while + // containing N+1, every CAS token handed out is wrong, and the new content + // belongs to no version at all. Nothing detects it and nothing repairs it. + // + // The order now is: version blob, metadata, live blob. Metadata is the + // commit point, because it is the only one of the three that decides what a + // version *is* — and Get reads the version blob it names, so the pair is + // self-consistent whichever write is interrupted. + // + // Stopping before the metadata leaves an unreferenced blob: storage spent, + // nothing else. Stopping after it leaves the live mirror stale, which costs + // a listing that is briefly out of date and no reader a wrong answer. There + // is no ordering that makes three writes into one; there is an ordering + // where every interruption leaves a state somebody can explain. hash := string(contentVersion(data)) if _, err := f.Backend.Put(ctx, verBlobPath(path, next), data, ""); err != nil { return "", err } - if err := f.putLive(ctx, path, data); err != nil { - return "", err - } - m.Current = next - m.Versions[strconv.Itoa(next)] = FileVersionInfo{ + committed := *m + committed.Current = next + committed.Versions = make(map[string]FileVersionInfo, len(m.Versions)+1) + for k, v := range m.Versions { + committed.Versions[k] = v + } + committed.Versions[strconv.Itoa(next)] = FileVersionInfo{ Version: next, CreatedAt: time.Now().UTC(), Hash: hash, Size: len(data), } - f.prune(ctx, m) - if err := f.saveMeta(ctx, m); err != nil { + f.prune(ctx, &committed) + if err := f.saveMeta(ctx, &committed); err != nil { + // Nothing has been published. The version blob written above is + // unreferenced and will be overwritten by the next attempt at this + // version number. return "", err } + + if err := f.putLive(ctx, path, data); err != nil { + // The version is real — its bytes are in the version blob and the + // metadata names it — but the live copy is stale. Say so precisely + // rather than reporting a clean failure over a committed write. + return "", fmt.Errorf("version %d of %s was recorded but the live copy was not updated: %w", next, path, err) + } return FormatFileVersion(next), nil } diff --git a/internal/storage/filelog_atomic_test.go b/internal/storage/filelog_atomic_test.go new file mode 100644 index 0000000..7ccac50 --- /dev/null +++ b/internal/storage/filelog_atomic_test.go @@ -0,0 +1,196 @@ +package storage + +import ( + "context" + "errors" + "strings" + "testing" +) + +// A versioned write is three objects — the version blob, the metadata, the live +// mirror — and no backend can write three things at once. What can be arranged +// is that every way of stopping part-way leaves a state somebody can explain. +// +// It could not, before. The order was version blob, live, metadata: stop after +// the second and the live file holds the new content while the metadata still +// calls the old version current, so a read returned the new bytes labelled with +// the old version and every CAS token handed out was a lie about content the +// caller had never seen. + +// failingBackend fails the nth write to a matching path, so a test can stop a +// three-step write anywhere in the middle. +type failingBackend struct { + Backend + failOn func(path string) bool +} + +var errInjected = errors.New("injected write failure") + +func (b *failingBackend) Put(ctx context.Context, path string, data []byte, expected Version) (Version, error) { + if b.failOn != nil && b.failOn(path) { + return "", errInjected + } + return b.Backend.Put(ctx, path, data, expected) +} + +func logOver(t *testing.T, failOn func(string) bool) *FileLog { + t.Helper() + local, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + return &FileLog{Backend: &failingBackend{Backend: local, failOn: failOn}} +} + +// The read must come from the version the metadata names, whatever the mirror +// happens to hold. +func TestGetReturnsTheVersionItReports(t *testing.T) { + fl := logOver(t, nil) + ctx := context.Background() + + if _, err := fl.Put(ctx, "notes.md", []byte("first"), ""); err != nil { + t.Fatal(err) + } + if _, err := fl.Put(ctx, "notes.md", []byte("second"), FormatFileVersion(1)); err != nil { + t.Fatal(err) + } + + data, ver, err := fl.Get(ctx, "notes.md") + if err != nil { + t.Fatal(err) + } + if string(data) != "second" || ver != FormatFileVersion(2) { + t.Fatalf("got %q at %q, want \"second\" at v2", data, ver) + } +} + +// Interrupted before the metadata: nothing was published, so the previous +// version is still current and still readable. The orphaned blob costs storage +// and nothing else. +func TestAWriteInterruptedBeforeTheCommitChangesNothing(t *testing.T) { + fl := logOver(t, func(p string) bool { return strings.HasPrefix(p, FileMetaPrefix) }) + ctx := context.Background() + + // The first write needs metadata, so allow it, then arm the failure. + fb := fl.Backend.(*failingBackend) + fb.failOn = nil + if _, err := fl.Put(ctx, "notes.md", []byte("first"), ""); err != nil { + t.Fatal(err) + } + fb.failOn = func(p string) bool { return strings.HasPrefix(p, FileMetaPrefix) } + + if _, err := fl.Put(ctx, "notes.md", []byte("second"), FormatFileVersion(1)); !errors.Is(err, errInjected) { + t.Fatalf("expected the injected failure, got %v", err) + } + + data, ver, err := fl.Get(ctx, "notes.md") + if err != nil { + t.Fatal(err) + } + if string(data) != "first" || ver != FormatFileVersion(1) { + t.Errorf("after a failed write the file reads %q at %q, want \"first\" at v1", data, ver) + } +} + +// Interrupted after the metadata but before the mirror: the version is real and +// reads correctly. Only the listing mirror is briefly behind, which costs +// nobody a wrong answer. +func TestAWriteInterruptedAfterTheCommitStillReadsCorrectly(t *testing.T) { + local, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + fb := &failingBackend{Backend: local} + fl := &FileLog{Backend: fb} + ctx := context.Background() + + if _, err := fl.Put(ctx, "notes.md", []byte("first"), ""); err != nil { + t.Fatal(err) + } + + // Fail only the live mirror: not the version blob, not the metadata. + fb.failOn = func(p string) bool { + return !strings.HasPrefix(p, FileMetaPrefix) && !strings.HasPrefix(p, FileVerPrefix) + } + _, err = fl.Put(ctx, "notes.md", []byte("second"), FormatFileVersion(1)) + if err == nil { + t.Fatal("a failed mirror write reported success") + } + if !strings.Contains(err.Error(), "recorded") { + t.Errorf("the error does not say the version was committed: %v", err) + } + + // The committed version is what a reader gets — not the stale mirror. + fb.failOn = nil + data, ver, err := fl.Get(ctx, "notes.md") + if err != nil { + t.Fatal(err) + } + if string(data) != "second" || ver != FormatFileVersion(2) { + t.Errorf("read %q at %q, want the committed \"second\" at v2", data, ver) + } +} + +// A version the metadata names whose content is gone must be an error, not a +// quiet fallback to whatever the mirror holds — that would hand back content +// belonging to a different version than the one reported. +func TestAMissingVersionBlobIsAnError(t *testing.T) { + local, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + fl := &FileLog{Backend: local} + ctx := context.Background() + + if _, err := fl.Put(ctx, "notes.md", []byte("only"), ""); err != nil { + t.Fatal(err) + } + // Remove the version blob behind the log's back. + _, ver, err := local.Get(ctx, verBlobPath("notes.md", 1)) + if err != nil { + t.Fatal(err) + } + if err := local.Delete(ctx, verBlobPath("notes.md", 1), ver); err != nil { + t.Fatal(err) + } + + if _, _, err := fl.Get(ctx, "notes.md"); !errors.Is(err, ErrNotFound) { + t.Fatalf("got %v, want a not-found rather than the mirror's copy", err) + } +} + +// The ordinary sequence still has to work, or atomicity has been bought by +// breaking the feature. +func TestVersionsStillAccumulate(t *testing.T) { + fl := logOver(t, nil) + ctx := context.Background() + + expected := Version("") + for i, body := range []string{"one", "two", "three"} { + ver, err := fl.Put(ctx, "notes.md", []byte(body), expected) + if err != nil { + t.Fatalf("write %d: %v", i+1, err) + } + expected = ver + } + _, versions, err := fl.ListVersions(ctx, "notes.md") + if err != nil { + t.Fatal(err) + } + if len(versions) != 3 { + t.Fatalf("kept %d versions, want 3", len(versions)) + } + for _, want := range []struct { + n int + body string + }{{1, "one"}, {2, "two"}, {3, "three"}} { + data, _, err := fl.GetVersion(ctx, "notes.md", want.n) + if err != nil { + t.Errorf("v%d: %v", want.n, err) + continue + } + if string(data) != want.body { + t.Errorf("v%d = %q, want %q", want.n, data, want.body) + } + } +} diff --git a/internal/storage/git.go b/internal/storage/git.go index e51ced2..c3e833f 100644 --- a/internal/storage/git.go +++ b/internal/storage/git.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "github.com/go-git/go-git/v5" @@ -41,6 +42,21 @@ type Git struct { auth GitAuth autoPush bool repo *git.Repository + + // mu serializes every operation that touches the repository. + // + // This backend had no locking at all while Local took a flock for each one, + // and the interface it implements promises compare-and-swap. Two things + // break without it. The compare and the swap were separate — read the file, + // compare its hash, write — so two writers both passed and the second + // silently discarded the first. And go-git's worktree is not safe for + // concurrent use: Add and Commit share one index file, so overlapping + // commits corrupt it or lose a change that was staged and never committed. + // + // One lock rather than one per path, because the index and HEAD are the + // repository's, not any single file's. A commit per write is already the + // slow part; contending on the lock does not change that. + mu sync.Mutex } // OpenGit opens or initializes a git backend at cfg.LocalPath. @@ -182,6 +198,8 @@ func (g *Git) commit(paths []string, msg string) error { func (g *Git) Get(ctx context.Context, path string) ([]byte, Version, error) { _ = ctx + g.mu.Lock() + defer g.mu.Unlock() path, err := CleanFilePath(path) if err != nil { return nil, "", err @@ -202,6 +220,8 @@ func (g *Git) Get(ctx context.Context, path string) ([]byte, Version, error) { func (g *Git) List(ctx context.Context, prefix string) ([]Entry, error) { _ = ctx + g.mu.Lock() + defer g.mu.Unlock() prefix, err := CleanPath(prefix) if err != nil { return nil, err @@ -231,7 +251,7 @@ func (g *Git) List(ctx context.Context, prefix string) ([]Entry, error) { if err != nil { return err } - out = append(out, Entry{Path: rel, Version: contentVersion(data)}) + out = append(out, Entry{Path: rel, Version: contentVersion(data), Size: int64(len(data))}) return nil }) if err != nil && !os.IsNotExist(err) { @@ -242,6 +262,10 @@ func (g *Git) List(ctx context.Context, prefix string) ([]Entry, error) { func (g *Git) Put(ctx context.Context, path string, data []byte, expected Version) (Version, error) { _ = ctx + // The read, the compare and the write are one operation or they are a + // lost update. + g.mu.Lock() + defer g.mu.Unlock() path, err := CleanFilePath(path) if err != nil { return "", err @@ -285,6 +309,8 @@ func (g *Git) Put(ctx context.Context, path string, data []byte, expected Versio func (g *Git) Delete(ctx context.Context, path string, expected Version) error { _ = ctx + g.mu.Lock() + defer g.mu.Unlock() path, err := CleanFilePath(path) if err != nil { return err @@ -316,6 +342,8 @@ func (g *Git) Delete(ctx context.Context, path string, expected Version) error { func (g *Git) Head(ctx context.Context, scope string) (Version, error) { _ = ctx + g.mu.Lock() + defer g.mu.Unlock() scope, err := CleanPath(scope) if err != nil { return "", err @@ -332,6 +360,8 @@ func (g *Git) Head(ctx context.Context, scope string) (Version, error) { func (g *Git) SetHead(ctx context.Context, scope string, expected, next Version) error { _ = ctx + g.mu.Lock() + defer g.mu.Unlock() scope, err := CleanPath(scope) if err != nil { return err @@ -365,6 +395,15 @@ func (g *Git) SetHead(ctx context.Context, scope string, expected, next Version) // Push pushes to the configured remote (no-op if unset). func (g *Git) Push(ctx context.Context) error { + g.mu.Lock() + defer g.mu.Unlock() + return g.pushLocked(ctx) +} + +// pushLocked is Push for callers already holding the lock. Separated rather than +// made reentrant: a mutex that is sometimes held twice is a deadlock waiting for +// the day somebody adds a call. +func (g *Git) pushLocked(ctx context.Context) error { if g.remote == "" { return nil } @@ -382,15 +421,18 @@ func (g *Git) Push(ctx context.Context) error { return err } +// maybePush is only ever called from a locked method, so it must not re-lock. func (g *Git) maybePush(ctx context.Context) error { if !g.autoPush { return nil } - return g.Push(ctx) + return g.pushLocked(ctx) } // TestConnectivity verifies the local repo opens and, if remote is set, that ls-remote works. func (g *Git) TestConnectivity(ctx context.Context) error { + g.mu.Lock() + defer g.mu.Unlock() if _, err := g.repo.Head(); err != nil { return fmt.Errorf("local git head: %w", err) } diff --git a/internal/storage/git_concurrency_test.go b/internal/storage/git_concurrency_test.go new file mode 100644 index 0000000..28b4c97 --- /dev/null +++ b/internal/storage/git_concurrency_test.go @@ -0,0 +1,176 @@ +package storage + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" +) + +// Local took a flock around every operation. This backend took nothing, while +// implementing the same interface — one that promises compare-and-swap. Two +// separate failures follow from that: the compare and the swap were not one +// operation, so a lost update was possible; and go-git's worktree shares one +// index between Add and Commit, so overlapping commits corrupt it. +// +// Run with -race these would be flaky rather than reliably wrong, which is the +// worst way for a storage bug to present. What is asserted here is the outcome: +// exactly one winner per contended write, and a repository still readable +// afterwards. + +func gitBackend(t *testing.T) *Git { + t.Helper() + g, err := OpenGit(GitConfig{LocalPath: t.TempDir()}) + if err != nil { + t.Fatal(err) + } + return g +} + +// Every writer starts from the same version. CAS means one of them wins. +func TestGitConcurrentCreatesProduceOneWinner(t *testing.T) { + g := gitBackend(t) + ctx := context.Background() + + const n = 8 + errs := make([]error, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + // Empty expected version means "create": only one may. + _, errs[i] = g.Put(ctx, "notes.md", []byte(fmt.Sprintf("writer %d", i)), "") + }(i) + } + wg.Wait() + + won := 0 + for i, err := range errs { + switch { + case err == nil: + won++ + case errors.Is(err, ErrConflict): + default: + t.Errorf("writer %d failed with %v, want nil or a conflict", i, err) + } + } + if won != 1 { + t.Fatalf("%d writers created the same path, want exactly 1", won) + } +} + +// A read-modify-write cycle run concurrently must not lose an update: every +// successful Put has to have been against the version it actually replaced. +func TestGitConcurrentOverwritesDoNotLoseUpdates(t *testing.T) { + g := gitBackend(t) + ctx := context.Background() + + base, err := g.Put(ctx, "notes.md", []byte("base"), "") + if err != nil { + t.Fatal(err) + } + + const n = 8 + errs := make([]error, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + _, errs[i] = g.Put(ctx, "notes.md", []byte(fmt.Sprintf("update %d", i)), base) + }(i) + } + wg.Wait() + + won := 0 + for i, err := range errs { + switch { + case err == nil: + won++ + case errors.Is(err, ErrConflict): + default: + t.Errorf("writer %d failed with %v", i, err) + } + } + if won != 1 { + t.Fatalf("%d writers succeeded against one version, want exactly 1", won) + } + + // And the survivor is readable, which is what a corrupt index would cost. + data, ver, err := g.Get(ctx, "notes.md") + if err != nil { + t.Fatalf("the file is unreadable after concurrent writes: %v", err) + } + if ver == base { + t.Error("the version never moved; no write landed") + } + if len(data) == 0 { + t.Error("the file is empty") + } +} + +// Heads carry the same CAS promise, through a different file and the same index. +func TestGitConcurrentHeadUpdatesProduceOneWinner(t *testing.T) { + g := gitBackend(t) + ctx := context.Background() + + const n = 8 + errs := make([]error, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + errs[i] = g.SetHead(ctx, SpaceScope, "", Version(fmt.Sprintf("head-%d", i))) + }(i) + } + wg.Wait() + + won := 0 + for _, err := range errs { + if err == nil { + won++ + } + } + if won != 1 { + t.Fatalf("%d writers set the head from empty, want exactly 1", won) + } + if _, err := g.Head(ctx, SpaceScope); err != nil { + t.Fatalf("the head is unreadable afterwards: %v", err) + } +} + +// Writes to different paths still share one index and one HEAD, so they are the +// case most likely to corrupt the repository rather than merely lose a write. +func TestGitConcurrentWritesToDifferentPathsAllLand(t *testing.T) { + g := gitBackend(t) + ctx := context.Background() + + const n = 12 + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + if _, err := g.Put(ctx, fmt.Sprintf("f%d.md", i), []byte("body"), ""); err != nil { + t.Errorf("write %d: %v", i, err) + } + }(i) + } + wg.Wait() + + entries, err := g.List(ctx, "") + if err != nil { + t.Fatalf("listing after concurrent writes: %v", err) + } + if len(entries) != n { + t.Errorf("%d files present, want %d — a write was lost", len(entries), n) + } + for i := 0; i < n; i++ { + if _, _, err := g.Get(ctx, fmt.Sprintf("f%d.md", i)); err != nil { + t.Errorf("f%d.md unreadable: %v", i, err) + } + } +} diff --git a/internal/storage/history.go b/internal/storage/history.go index 7e4d51f..a9612bb 100644 --- a/internal/storage/history.go +++ b/internal/storage/history.go @@ -1,6 +1,7 @@ package storage import ( + "bytes" "context" "crypto/rand" "encoding/hex" @@ -221,7 +222,7 @@ func Migrate(ctx context.Context, src, dst Backend) (int, error) { } n := 0 for _, e := range entries { - data, ver, err := src.Get(ctx, e.Path) + data, _, err := src.Get(ctx, e.Path) if err != nil { return n, err } @@ -234,12 +235,26 @@ func Migrate(ctx context.Context, src, dst Backend) (int, error) { } else if !errors.Is(err, ErrNotFound) { return n, err } - got, err := dst.Put(ctx, e.Path, data, "") - if err != nil { + if _, err := dst.Put(ctx, e.Path, data, ""); err != nil { return n, fmt.Errorf("put dest %s: %w", e.Path, err) } - if got != ver && contentVersion(data) != got { - // versions may differ by driver encoding; content must match + // Read it back. The check was here and did nothing: an if with an empty + // body, under a comment saying content must match. A migration is the + // one operation where "it probably worked" is not good enough — the + // operator is about to point the server at the new backend and the old + // one stops being consulted, so a file that arrived wrong is a file that + // is simply wrong from then on. + // + // Compared by content rather than by version marker, because the comment + // was right about that part: two drivers derive versions differently, and + // the same bytes legitimately carry different tokens either side. + back, _, err := dst.Get(ctx, e.Path) + if err != nil { + return n, fmt.Errorf("verify %s on the destination: %w", e.Path, err) + } + if !bytes.Equal(back, data) { + return n, fmt.Errorf("verify %s: the destination holds %d bytes, the source has %d — migration stopped so nothing further is written", + e.Path, len(back), len(data)) } n++ } diff --git a/internal/storage/local.go b/internal/storage/local.go index 159af4a..f90bf86 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -60,10 +60,24 @@ func (l *Local) objectPath(path string) string { type objectRecord struct { Path string `json:"path"` Version Version `json:"version"` + Size int64 `json:"size"` Data []byte `json:"data"` Updated time.Time `json:"updated"` } +// objectHeader is objectRecord without the body. +// +// A listing wants a path and a version. The record keeps the file's bytes in the +// same JSON document, base64-encoded, so decoding into objectRecord to answer +// "what is in here" reads and decodes every byte of every file — on every tree, +// every changes and every quota check, while holding the store's exclusive lock. +// Leaving Data out of the struct means the decoder skips it. +type objectHeader struct { + Path string `json:"path"` + Version Version `json:"version"` + Size int64 `json:"size"` +} + func (l *Local) withLock(ctx context.Context, fn func() error) error { lockPath := filepath.Join(l.root, localLockFile) lock := flock.New(lockPath) @@ -126,14 +140,23 @@ func (l *Local) List(ctx context.Context, prefix string) ([]Entry, error) { if err != nil { return err } - var rec objectRecord - if err := json.Unmarshal(raw, &rec); err != nil { + var hdr objectHeader + if err := json.Unmarshal(raw, &hdr); err != nil { return err } - if prefix != "" && !strings.HasPrefix(rec.Path, prefix) { + if prefix != "" && !strings.HasPrefix(hdr.Path, prefix) { return nil } - out = append(out, Entry{Path: rec.Path, Version: rec.Version}) + // A record written before size was stored has none; fall back to + // the encoded length rather than reporting a file as empty, which a + // quota check would read as free space. + size := hdr.Size + if size == 0 { + if rec, rerr := l.readRecord(hdr.Path); rerr == nil { + size = int64(len(rec.Data)) + } + } + out = append(out, Entry{Path: hdr.Path, Version: hdr.Version, Size: size}) return nil }) }) @@ -162,6 +185,7 @@ func (l *Local) Put(ctx context.Context, path string, data []byte, expected Vers nrec := objectRecord{ Path: sanitizePath(path), Version: next, + Size: int64(len(data)), Data: append([]byte(nil), data...), Updated: time.Now().UTC(), } diff --git a/internal/storage/local_test.go b/internal/storage/local_test.go index 39006cd..996ea0b 100644 --- a/internal/storage/local_test.go +++ b/internal/storage/local_test.go @@ -1,6 +1,7 @@ package storage import ( + "bytes" "context" "errors" "path/filepath" @@ -116,3 +117,128 @@ func TestSanitizePath(t *testing.T) { t.Fatalf("got %q", got) } } + +// A listing wants a path and a version, and the record keeps the file's bytes +// in the same JSON document. Decoding into the full record read and decoded +// every byte of every file to answer a question about names — on every tree, +// every changes and every quota check, holding the store's exclusive lock. +func TestListDoesNotDecodeFileBodies(t *testing.T) { + l, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + + body := bytes.Repeat([]byte("x"), 256<<10) + for _, p := range []string{"a.md", "b.md", "team/c.md"} { + if _, err := l.Put(ctx, p, body, ""); err != nil { + t.Fatal(err) + } + } + + entries, err := l.List(ctx, "") + if err != nil { + t.Fatal(err) + } + if len(entries) != 3 { + t.Fatalf("listed %d entries, want 3", len(entries)) + } + for _, e := range entries { + if e.Path == "" { + t.Error("an entry came back with no path") + } + if e.Version == "" { + t.Errorf("%s came back with no version; callers compare against it", e.Path) + } + } + + // The version a listing reports must be the one Get reports, or the two + // disagree about what a caller is holding. + _, ver, err := l.Get(ctx, "a.md") + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Path == "a.md" && e.Version != ver { + t.Errorf("list says %q, get says %q", e.Version, ver) + } + } +} + +func TestListFiltersByPrefix(t *testing.T) { + l, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + for _, p := range []string{"team/a.md", "team/b.md", "other/c.md"} { + if _, err := l.Put(ctx, p, []byte("body"), ""); err != nil { + t.Fatal(err) + } + } + + entries, err := l.List(ctx, "team/") + if err != nil { + t.Fatal(err) + } + if len(entries) != 2 { + t.Fatalf("listed %d entries under team/, want 2", len(entries)) + } +} + +// Quota accounting used to list the backend and then stat the working-tree +// mirror for sizes. That mirror is written by whichever replica handled the +// write, so with a shared backend every other replica counted the files as +// nothing and let the space grow past its limit. The backend has to answer. +func TestListReportsTheSizeOfEachObject(t *testing.T) { + l, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + + want := map[string]int64{} + for _, tc := range []struct { + path string + size int + }{{"a.md", 10}, {"b.md", 4096}, {"team/c.md", 1}} { + body := bytes.Repeat([]byte("x"), tc.size) + if _, err := l.Put(ctx, tc.path, body, ""); err != nil { + t.Fatal(err) + } + want[tc.path] = int64(tc.size) + } + + entries, err := l.List(ctx, "") + if err != nil { + t.Fatal(err) + } + if len(entries) != len(want) { + t.Fatalf("listed %d entries, want %d", len(entries), len(want)) + } + for _, e := range entries { + if e.Size != want[e.Path] { + t.Errorf("%s reported %d bytes, want %d", e.Path, e.Size, want[e.Path]) + } + } +} + +// An empty file is legitimately zero bytes, and must not be mistaken for a size +// the backend failed to report. +func TestAnEmptyFileListsAsZero(t *testing.T) { + l, err := OpenLocal(t.TempDir()) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := l.Put(ctx, "empty.md", nil, ""); err != nil { + t.Fatal(err) + } + entries, err := l.List(ctx, "") + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Size != 0 { + t.Errorf("got %+v, want one entry of zero bytes", entries) + } +} diff --git a/internal/storage/prefixed.go b/internal/storage/prefixed.go index f8511db..027650a 100644 --- a/internal/storage/prefixed.go +++ b/internal/storage/prefixed.go @@ -60,7 +60,7 @@ func (p *Prefixed) List(ctx context.Context, prefix string) ([]Entry, error) { } out := make([]Entry, 0, len(entries)) for _, e := range entries { - out = append(out, Entry{Path: p.strip(e.Path), Version: e.Version}) + out = append(out, Entry{Path: p.strip(e.Path), Version: e.Version, Size: e.Size}) } return out, nil } diff --git a/internal/storage/s3.go b/internal/storage/s3.go index ced84a2..f0788f2 100644 --- a/internal/storage/s3.go +++ b/internal/storage/s3.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "strconv" "strings" "github.com/aws/aws-sdk-go-v2/aws" @@ -100,11 +101,6 @@ func isBucketAlreadyOwned(err error) bool { func (s *S3) Name() string { return "s3" } -func (s *S3) objectKey(path string) string { - sum := contentVersion([]byte(sanitizePath(path))) - return s.prefix + "objects/" + string(sum) + ".json" -} - func (s *S3) headKey(scope string) string { sc := sanitizePath(scope) if sc == "" || sc == "." { @@ -114,31 +110,44 @@ func (s *S3) headKey(scope string) string { return s.prefix + "heads/" + string(sum) + ".head" } -func (s *S3) getRecord(ctx context.Context, path string) (s3ObjectRecord, string, error) { +// getRecord reads a path, looking under the legacy key when the current one is +// absent so a bucket written by an older contextd keeps working. +// +// Returns the key it found the object under, because a writer needs to know +// whether it is replacing a legacy object and should clean it up. +func (s *S3) getRecord(ctx context.Context, path string) (s3ObjectRecord, string, string, error) { + key := s.objectKey(path) out, err := s.client.GetObject(ctx, &s3.GetObjectInput{ Bucket: aws.String(s.bucket), - Key: aws.String(s.objectKey(path)), + Key: aws.String(key), }) + if err != nil && isNoSuchKey(err) { + key = s.legacyObjectKey(path) + out, err = s.client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(key), + }) + } if err != nil { if isNoSuchKey(err) { - return s3ObjectRecord{}, "", ErrNotFound + return s3ObjectRecord{}, "", "", ErrNotFound } - return s3ObjectRecord{}, "", err + return s3ObjectRecord{}, "", "", err } defer out.Body.Close() raw, err := io.ReadAll(out.Body) if err != nil { - return s3ObjectRecord{}, "", err + return s3ObjectRecord{}, "", "", err } var rec s3ObjectRecord if err := json.Unmarshal(raw, &rec); err != nil { - return s3ObjectRecord{}, "", err + return s3ObjectRecord{}, "", "", err } etag := "" if out.ETag != nil { etag = strings.Trim(*out.ETag, `"`) } - return rec, etag, nil + return rec, etag, key, nil } func (s *S3) Get(ctx context.Context, path string) ([]byte, Version, error) { @@ -146,7 +155,7 @@ func (s *S3) Get(ctx context.Context, path string) ([]byte, Version, error) { if err != nil { return nil, "", err } - rec, _, err := s.getRecord(ctx, path) + rec, _, _, err := s.getRecord(ctx, path) if err != nil { return nil, "", err } @@ -158,10 +167,20 @@ func (s *S3) List(ctx context.Context, prefix string) ([]Entry, error) { if err != nil { return nil, err } + // The listing is the answer for anything written under the current scheme: + // the key carries the path, so no object needs to be fetched. Only legacy + // keys — which say nothing about their path — still cost a GET, and each one + // stops costing it the next time that file is written. + // + // This used to issue a GetObject for every object in the bucket and download + // the whole body to read the path back out of it, on every tree, every + // changes and every quota check. var out []Entry + var current []listedObject + var legacyKeys []string paginator := s3.NewListObjectsV2Paginator(s.client, &s3.ListObjectsV2Input{ Bucket: aws.String(s.bucket), - Prefix: aws.String(s.prefix + "objects/"), + Prefix: aws.String(s.prefix + s3ObjectsPrefix), }) for paginator.HasMorePages() { page, err := paginator.NextPage(ctx) @@ -172,37 +191,122 @@ func (s *S3) List(ctx context.Context, prefix string) ([]Entry, error) { if obj.Key == nil { continue } - got, err := s.client.GetObject(ctx, &s3.GetObjectInput{ - Bucket: aws.String(s.bucket), - Key: obj.Key, - }) - if err != nil { - return nil, err - } - raw, err := io.ReadAll(got.Body) - got.Body.Close() - if err != nil { - return nil, err - } - var rec s3ObjectRecord - if err := json.Unmarshal(raw, &rec); err != nil { + path, ok := s.pathFromKey(*obj.Key) + if !ok { + legacyKeys = append(legacyKeys, *obj.Key) continue } - if prefix != "" && !strings.HasPrefix(rec.Path, prefix) { + if prefix != "" && !strings.HasPrefix(path, prefix) { continue } - out = append(out, Entry{Path: rec.Path, Version: rec.Version}) + current = append(current, listedObject{path: path, key: *obj.Key}) + } + } + + // One HeadObject each: metadata, no body. The version is a few bytes of + // header rather than the whole file, which is what this used to move. + for _, obj := range current { + ver, size, err := s.versionOf(ctx, obj.key) + if err != nil { + logx.L().Warn("s3 list: skipping unreadable object", "key", obj.key, "err", err) + continue + } + out = append(out, Entry{Path: obj.path, Version: ver, Size: size}) + } + + for _, key := range legacyKeys { + rec, err := s.readRecordByKey(ctx, key) + if err != nil { + // One unreadable legacy object must not cost the whole listing. + logx.L().Warn("s3 list: skipping unreadable legacy object", "key", key, "err", err) + continue + } + if prefix != "" && !strings.HasPrefix(rec.Path, prefix) { + continue } + out = append(out, Entry{Path: rec.Path, Version: rec.Version, Size: int64(len(rec.Data))}) } return out, nil } +// User-metadata keys: the CAS token and the content's own byte length. +const ( + s3VersionMeta = "cv-version" + s3SizeMeta = "cv-size" +) + +// listedObject is one key a listing recognised as belonging to a path. +type listedObject struct { + path string + key string +} + +// versionOf reads an object's CAS token from its metadata, falling back to the +// body for an object written before the stamp existed. +func (s *S3) versionOf(ctx context.Context, key string) (Version, int64, error) { + head, err := s.client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(key), + }) + if err != nil { + return "", 0, err + } + var ver Version + var size int64 + for k, v := range head.Metadata { + // S3 lowercases metadata keys, and SDKs differ on whether they hand + // them back canonicalised. + switch { + case strings.EqualFold(k, s3VersionMeta) && v != "": + ver = Version(v) + case strings.EqualFold(k, s3SizeMeta) && v != "": + if n, perr := strconv.ParseInt(v, 10, 64); perr == nil { + size = n + } + } + } + if ver != "" { + return ver, size, nil + } + // Written before the stamps existed: read the record itself. + rec, err := s.readRecordByKey(ctx, key) + if err != nil { + return "", 0, err + } + return rec.Version, int64(len(rec.Data)), nil +} + +// readRecordByKey fetches one object by its exact key, for the legacy objects a +// listing cannot describe on its own. +func (s *S3) readRecordByKey(ctx context.Context, key string) (s3ObjectRecord, error) { + got, err := s.client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(key), + }) + if err != nil { + return s3ObjectRecord{}, err + } + defer got.Body.Close() + raw, err := io.ReadAll(got.Body) + if err != nil { + return s3ObjectRecord{}, err + } + var rec s3ObjectRecord + if err := json.Unmarshal(raw, &rec); err != nil { + return s3ObjectRecord{}, err + } + return rec, nil +} + func (s *S3) Put(ctx context.Context, path string, data []byte, expected Version) (Version, error) { path, err := CleanFilePath(path) if err != nil { return "", err } - rec, etag, err := s.getRecord(ctx, path) + if err := s.checkKeyLength(path); err != nil { + return "", err + } + rec, etag, foundKey, err := s.getRecord(ctx, path) actual := Version("") if err == nil { actual = rec.Version @@ -212,6 +316,13 @@ func (s *S3) Put(ctx context.Context, path string, data []byte, expected Version if actual != expected { return "", &ConflictError{Path: path, Expected: expected, Actual: actual} } + // A legacy object is being replaced, so the write goes to the new key and + // the old one is dropped afterwards. Migration happens as a bucket is used + // rather than in a step somebody has to remember to run. + migrating := foundKey != "" && foundKey != s.objectKey(path) + if migrating { + etag = "" // the precondition belongs to the key we are writing, not the one we read + } next := contentVersion(data) nrec := s3ObjectRecord{Path: sanitizePath(path), Version: next, Data: append([]byte(nil), data...)} raw, err := json.Marshal(nrec) @@ -223,6 +334,16 @@ func (s *S3) Put(ctx context.Context, path string, data []byte, expected Version Key: aws.String(s.objectKey(path)), Body: bytes.NewReader(raw), ContentType: aws.String("application/json"), + // The CAS token, stamped where a HeadObject can read it. Listing needs + // the version as well as the path, and the alternative is downloading + // every file to find out what version it is. + Metadata: map[string]string{ + s3VersionMeta: string(next), + // The content's own length, not the wrapper's. A listing needs it + // for quota accounting and the object's reported size is the JSON + // record around it, which is a third larger because of base64. + s3SizeMeta: strconv.FormatInt(int64(len(data)), 10), + }, } if etag != "" { input.IfMatch = aws.String(etag) @@ -236,6 +357,16 @@ func (s *S3) Put(ctx context.Context, path string, data []byte, expected Version } return "", err } + if migrating { + // Best-effort: the object is already safe under its new key, and a + // leftover legacy copy is only read when the new one is missing. + if _, derr := s.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(foundKey), + }); derr != nil { + logx.L().Warn("s3 migrate: could not remove the legacy object", "path", path, "key", foundKey, "err", derr) + } + } logx.L().Debug("s3 put", "path", path, "version", string(next)) return next, nil } @@ -245,16 +376,18 @@ func (s *S3) Delete(ctx context.Context, path string, expected Version) error { if err != nil { return err } - rec, _, err := s.getRecord(ctx, path) + rec, _, foundKey, err := s.getRecord(ctx, path) if err != nil { return err } if rec.Version != expected { return &ConflictError{Path: path, Expected: expected, Actual: rec.Version} } + // Delete the key it was actually found under: a legacy object deleted at + // the new key would stay readable. _, err = s.client.DeleteObject(ctx, &s3.DeleteObjectInput{ Bucket: aws.String(s.bucket), - Key: aws.String(s.objectKey(path)), + Key: aws.String(foundKey), }) return err } diff --git a/internal/storage/s3_keys.go b/internal/storage/s3_keys.go new file mode 100644 index 0000000..6f4c2fa --- /dev/null +++ b/internal/storage/s3_keys.go @@ -0,0 +1,181 @@ +package storage + +import ( + "fmt" + "strings" +) + +// How an object's path becomes an S3 key, and why it changed. +// +// # The old scheme was too narrow and too opaque +// +// The key was contentVersion(path) — SHA-256 truncated to eight bytes. Sixty-four +// bits for a value that decides which file you are reading. Local used the full +// digest for the same job; only S3 was cut short, and nothing recorded why. +// Sixty-four bits is inside reach: a collision needs about 2^32 candidate paths +// to find by chance, and far less to construct deliberately. Two different paths +// landing on one key means one file silently overwrites the other, with no error +// anywhere. +// +// It was also unreadable. Because the key said nothing about the path, listing +// the space meant downloading every object in the bucket to read the path back +// out of its body — one GET per file, with the whole body on the wire, on every +// tree, every changes and every quota check. That is a bill and a wait for an +// answer S3 already had. +// +// # The new scheme is the path +// +// Keys are the path itself, escaped where S3 or a human would trip over it. +// Collisions become impossible rather than unlikely, listing needs no request +// beyond the listing, and the bucket is legible to whoever has to look at it at +// three in the morning. +// +// # Compatibility +// +// Buckets written by the old scheme still exist, so a read falls back to the old +// key, and a write to a path that still lives there moves it. A bucket converges +// as it is used, and a listing pays the old cost only for the objects that have +// not been touched yet. + +const ( + s3ObjectsPrefix = "objects/" + s3ObjectSuffix = ".json" + // s3MaxKeyLen is S3's own limit on a key, in bytes. + s3MaxKeyLen = 1024 +) + +// objectKey returns the key for a path under the current scheme. +func (s *S3) objectKey(path string) string { + return s.prefix + s3ObjectsPrefix + escapeKeySegment(sanitizePath(path)) + s3ObjectSuffix +} + +// legacyObjectKey returns the key the old truncated-hash scheme would have used. +// Read-only: nothing new is ever written here. +func (s *S3) legacyObjectKey(path string) string { + sum := contentVersion([]byte(sanitizePath(path))) + return s.prefix + s3ObjectsPrefix + string(sum) + s3ObjectSuffix +} + +// pathFromKey recovers the logical path from a key written under the current +// scheme, reporting false for anything else — including a legacy hashed key, +// which carries no path to recover. +func (s *S3) pathFromKey(key string) (string, bool) { + rest, ok := strings.CutPrefix(key, s.prefix+s3ObjectsPrefix) + if !ok { + return "", false + } + rest, ok = strings.CutSuffix(rest, s3ObjectSuffix) + if !ok || rest == "" { + return "", false + } + path, err := unescapeKeySegment(rest) + if err != nil { + return "", false + } + // A legacy key is sixteen hex characters and decodes to itself. Treating one + // as a path would invent a file named after a hash. + if isLegacyHashKey(rest) { + return "", false + } + return path, true +} + +// isLegacyHashKey reports whether a key body looks like the old scheme's +// sixteen hex characters. +func isLegacyHashKey(s string) bool { + if len(s) != 16 { + return false + } + for i := 0; i < len(s); i++ { + c := s[i] + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + +// checkKeyLength refuses a path whose key would exceed what S3 accepts, rather +// than letting the store fail somewhere less obvious. +func (s *S3) checkKeyLength(path string) error { + if k := s.objectKey(path); len(k) > s3MaxKeyLen { + return fmt.Errorf("%w: path is too long for this bucket (key would be %d bytes, limit %d)", + ErrInvalidArgument, len(k), s3MaxKeyLen) + } + return nil +} + +// escapeKeySegment percent-encodes the few bytes that make an S3 key awkward, +// and nothing else. +// +// Deliberately not url.PathEscape, which also escapes characters that are +// perfectly good in a key and would leave the bucket full of %2F where a slash +// belongs. Slashes are kept: they are what make the listing look like the space +// it holds. +func escapeKeySegment(path string) string { + var b strings.Builder + b.Grow(len(path)) + for i := 0; i < len(path); i++ { + c := path[i] + if needsKeyEscape(c) { + fmt.Fprintf(&b, "%%%02X", c) + continue + } + b.WriteByte(c) + } + return b.String() +} + +func needsKeyEscape(c byte) bool { + switch { + case c == '%': // the escape character itself, or decoding is ambiguous + return true + case c < 0x20 || c == 0x7f: // control bytes + return true + case c == '\\', c == '{', c == '}', c == '^', c == '`', c == '"', c == '<', c == '>', c == '|': + // Characters S3's own guidance calls out as needing special handling. + return true + default: + return false + } +} + +func unescapeKeySegment(s string) (string, error) { + if !strings.Contains(s, "%") { + return s, nil + } + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + if s[i] != '%' { + b.WriteByte(s[i]) + continue + } + if i+2 >= len(s) { + return "", fmt.Errorf("truncated escape in key") + } + hi, err := hexVal(s[i+1]) + if err != nil { + return "", err + } + lo, err := hexVal(s[i+2]) + if err != nil { + return "", err + } + b.WriteByte(hi<<4 | lo) + i += 2 + } + return b.String(), nil +} + +func hexVal(c byte) (byte, error) { + switch { + case c >= '0' && c <= '9': + return c - '0', nil + case c >= 'A' && c <= 'F': + return c - 'A' + 10, nil + case c >= 'a' && c <= 'f': + return c - 'a' + 10, nil + } + return 0, fmt.Errorf("bad escape digit %q", c) +} diff --git a/internal/storage/s3_keys_test.go b/internal/storage/s3_keys_test.go new file mode 100644 index 0000000..ebfdb40 --- /dev/null +++ b/internal/storage/s3_keys_test.go @@ -0,0 +1,157 @@ +package storage + +import ( + "strings" + "testing" +) + +// The old key was SHA-256 cut to eight bytes — sixty-four bits deciding which +// file you are reading, where Local used the full digest for the same job. A +// collision means one file silently overwriting another with no error anywhere, +// and sixty-four bits is well inside reach: about 2^32 candidates by chance, and +// far fewer to construct on purpose. +// +// Keys are the path now, so a collision is not unlikely but impossible. These +// check that the mapping is faithful in both directions, that the old keys are +// still recognised, and that the two schemes cannot be confused for each other. + +func s3For(prefix string) *S3 { + if prefix != "" && !strings.HasSuffix(prefix, "/") { + prefix += "/" + } + return &S3{bucket: "b", prefix: prefix} +} + +func TestDistinctPathsGetDistinctKeys(t *testing.T) { + s := s3For("spaces/team") + paths := []string{ + "notes.md", + "Notes.md", + "team/notes.md", + "team/notes.md.bak", + "a/b/c.md", + "a/b%2Fc.md", + "identity/me.md", + strings.Repeat("deep/", 20) + "x.md", + } + seen := map[string]string{} + for _, p := range paths { + k := s.objectKey(p) + if prev, dup := seen[k]; dup { + t.Fatalf("%q and %q share the key %q", prev, p, k) + } + seen[k] = p + } +} + +func TestAKeyRoundTripsBackToItsPath(t *testing.T) { + s := s3For("spaces/team") + for _, p := range []string{ + "notes.md", + "team/principles.md", + "projects/example/services/README.md", + "odd name with spaces.md", + "unicode-Привет-文件.md", + "percent%sign.md", + "back\\slash.md", + "brace{}caret^.md", + } { + key := s.objectKey(p) + got, ok := s.pathFromKey(key) + if !ok { + t.Errorf("%q: key %q did not decode", p, key) + continue + } + if got != p { + t.Errorf("%q round-tripped to %q via %q", p, got, key) + } + } +} + +// A legacy key carries no path, so it must not be mistaken for one — otherwise +// the listing invents a file named after a hash. +func TestALegacyKeyIsNotReadAsAPath(t *testing.T) { + s := s3For("spaces/team") + legacy := s.legacyObjectKey("notes.md") + if _, ok := s.pathFromKey(legacy); ok { + t.Errorf("legacy key %q was decoded as a path", legacy) + } + if !strings.HasSuffix(legacy, ".json") { + t.Errorf("legacy key shape changed: %q", legacy) + } +} + +// The legacy scheme must keep producing exactly what it used to, or reads +// against an existing bucket miss. +func TestTheLegacyKeyIsUnchanged(t *testing.T) { + s := s3For("spaces/team") + want := "spaces/team/objects/" + string(contentVersion([]byte("notes.md"))) + ".json" + if got := s.legacyObjectKey("notes.md"); got != want { + t.Errorf("legacy key = %q, want %q", got, want) + } +} + +func TestKeysFromAnotherPrefixAreIgnored(t *testing.T) { + s := s3For("spaces/team") + for _, key := range []string{ + "spaces/other/objects/notes.md.json", + "spaces/team/heads/abc.head", + "spaces/team/objects/notes.md", // no suffix + "unrelated", + "", + } { + if p, ok := s.pathFromKey(key); ok { + t.Errorf("%q was decoded as path %q", key, p) + } + } +} + +// A path long enough to push the key past what S3 accepts must be refused here +// rather than failing somewhere less legible. +func TestAnOverlongPathIsRefused(t *testing.T) { + s := s3For("spaces/team") + if err := s.checkKeyLength("notes.md"); err != nil { + t.Fatalf("an ordinary path was refused: %v", err) + } + if err := s.checkKeyLength(strings.Repeat("x", s3MaxKeyLen)); err == nil { + t.Error("a path whose key exceeds S3's limit was accepted") + } +} + +func TestEscapingIsReversible(t *testing.T) { + for _, in := range []string{ + "plain", + "with/slashes/kept", + "percent%20literal", + "control\x01byte", + "quote\"brace{}", + "", + } { + esc := escapeKeySegment(in) + got, err := unescapeKeySegment(esc) + if err != nil { + t.Errorf("%q escaped to %q which will not decode: %v", in, esc, err) + continue + } + if got != in { + t.Errorf("%q -> %q -> %q", in, esc, got) + } + } +} + +// Slashes survive escaping: they are what makes a bucket listing look like the +// space it holds, which is the difference between a legible store and a wall of +// hashes. +func TestSlashesAreNotEscaped(t *testing.T) { + if got := escapeKeySegment("team/projects/notes.md"); got != "team/projects/notes.md" { + t.Errorf("escaped to %q; slashes should be kept", got) + } +} + +func TestBadEscapesAreRejected(t *testing.T) { + for _, in := range []string{"%", "%A", "%ZZ", "abc%G0"} { + if _, err := unescapeKeySegment(in); err == nil { + t.Errorf("%q decoded without complaint", in) + } + } +} diff --git a/internal/storage/sql.go b/internal/storage/sql.go index fa85ab5..77025bd 100644 --- a/internal/storage/sql.go +++ b/internal/storage/sql.go @@ -92,7 +92,10 @@ func (s *SQL) List(ctx context.Context, prefix string) ([]Entry, error) { if err != nil { return nil, err } - q := `SELECT path, version FROM cv_objects` + // octet_length rather than the data itself: the size is the only thing a + // listing needs, and shipping every blob to count its bytes is what this + // replaced everywhere else. + q := `SELECT path, version, octet_length(data) FROM cv_objects` var args []any if prefix != "" { // A prefix is data, not a pattern: escape LIKE wildcards so "a_b" or @@ -110,7 +113,7 @@ func (s *SQL) List(ctx context.Context, prefix string) ([]Entry, error) { for rows.Next() { var e Entry var ver string - if err := rows.Scan(&e.Path, &ver); err != nil { + if err := rows.Scan(&e.Path, &ver, &e.Size); err != nil { return nil, err } e.Version = Version(ver) diff --git a/internal/syncclient/client.go b/internal/syncclient/client.go index 7787053..911efc7 100644 --- a/internal/syncclient/client.go +++ b/internal/syncclient/client.go @@ -3,7 +3,9 @@ package syncclient import ( "bytes" "context" + "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "fmt" "io" @@ -86,19 +88,6 @@ func (c *Client) do(ctx context.Context, method, path string, body any) (*http.R return c.HTTP.Do(req) } -func (c *Client) doBytes(ctx context.Context, method, path string, data []byte, ifMatch string) (*http.Response, error) { - req, err := http.NewRequestWithContext(ctx, method, c.BaseURL+path, bytes.NewReader(data)) - if err != nil { - return nil, err - } - req.Header.Set("Authorization", "Bearer "+c.Token) - req.Header.Set("Content-Type", "application/octet-stream") - if ifMatch != "" { - req.Header.Set("If-Match", `"`+ifMatch+`"`) - } - return c.HTTP.Do(req) -} - // WhoAmI returns user/role. func (c *Client) WhoAmI(ctx context.Context) (user, role string, err error) { res, err := c.do(ctx, http.MethodGet, "/api/v1/auth/whoami", nil) @@ -128,6 +117,13 @@ type SpaceInfo struct { // ListSpaces returns the spaces this token can see. The server has always // exposed this; the client never asked, so joining a team meant typing a space // name you had to be told out of band. +// +// It then went on never asking successfully. The server answers +// {"spaces":[...]} and this decoded into a bare []SpaceInfo, so every call +// failed with "cannot unmarshal object into []SpaceInfo" — and the wizard's +// fallback swallowed it and told the person "the server did not return a +// listing for this token", blaming the server for a bug on this side. The space +// picker has never once run. func (c *Client) ListSpaces(ctx context.Context) ([]SpaceInfo, error) { res, err := c.do(ctx, http.MethodGet, "/api/v1/spaces", nil) if err != nil { @@ -137,11 +133,13 @@ func (c *Client) ListSpaces(ctx context.Context) ([]SpaceInfo, error) { if res.StatusCode != 200 { return nil, apiErr(res) } - var out []SpaceInfo + var out struct { + Spaces []SpaceInfo `json:"spaces"` + } if err := json.NewDecoder(res.Body).Decode(&out); err != nil { - return nil, err + return nil, fmt.Errorf("list spaces: %w", err) } - return out, nil + return out.Spaces, nil } // Head returns space head. @@ -293,16 +291,31 @@ type PushResult struct { Applied int } -// Push uploads local files that differ from last known remote inventory. -// MVP: walk local tree and push all always/init-only paths as put ops. -func (c *Client) Push(ctx context.Context, spaceRoot string, expectedHead string, sync spacesvc.SyncConfig, checkOnly bool) (*PushResult, error) { - ops, err := collectPushOps(spaceRoot, sync) +// Push sends what has changed on this machine since the last successful push. +// +// It used to walk the tree and put every file, every time, base64-encoded into +// one JSON body — so a space of a thousand documents re-uploaded a thousand +// documents to publish an edit to one. And it never sent a delete, so a file +// removed locally came back on the next pull, which is the kind of thing that +// makes people stop trusting a sync tool. +// +// state carries what this machine last sent. Passing nil falls back to sending +// everything, which is right for a caller that has no record to compare against. +func (c *Client) Push(ctx context.Context, spaceRoot string, expectedHead string, sync spacesvc.SyncConfig, state *LocalState, checkOnly bool) (*PushResult, error) { + ops, sent, err := collectPushOps(spaceRoot, sync, state) if err != nil { return nil, err } if checkOnly { return &PushResult{Head: expectedHead, Applied: len(ops)}, nil } + if len(ops) == 0 { + // Nothing to say. Sending an empty batch would move the head for no + // reason and make every other client re-check a space that did not + // change. + logx.L().Info("push: nothing changed since the last one") + return &PushResult{Head: expectedHead, Applied: 0}, nil + } req := spacesvc.PushRequest{ExpectedHead: expectedHead, Ops: ops} res, err := c.do(ctx, http.MethodPost, "/api/v1/spaces/"+c.Space+"/push", req) if err != nil { @@ -323,14 +336,28 @@ func (c *Client) Push(ctx context.Context, spaceRoot string, expectedHead string if err := json.NewDecoder(res.Body).Decode(&out); err != nil { return nil, err } + // Recorded only after the server accepted the batch. Writing it earlier + // would make a failed push look like a delivered one, and the next push + // would skip the very files that never arrived. + if state != nil { + state.Sent = sent + } logx.L().Info("push complete", "head", out.Head, "applied", out.Applied) return &out, nil } -// LocalState tracks init-only seeding. +// LocalState is what this machine remembers about the last sync. type LocalState struct { Seeded map[string]bool `json:"seeded"` Versions map[string]string `json:"versions"` + // Sent records the content of each path as this machine last put it on the + // server, so a push can carry the difference instead of the whole space. + // + // Hashes rather than the server's version markers, because the question a + // push asks is "did I change this", which is about local content. It is also + // what makes a deletion visible: a path in here with no file beside it was + // pushed once and has since been removed. + Sent map[string]string `json:"sent,omitempty"` } func statePath(spaceRoot string) string { @@ -342,7 +369,11 @@ func LoadState(spaceRoot string) (*LocalState, error) { raw, err := os.ReadFile(statePath(spaceRoot)) if err != nil { if os.IsNotExist(err) { - return &LocalState{Seeded: map[string]bool{}, Versions: map[string]string{}}, nil + return &LocalState{ + Seeded: map[string]bool{}, + Versions: map[string]string{}, + Sent: map[string]string{}, + }, nil } return nil, err } @@ -356,6 +387,9 @@ func LoadState(spaceRoot string) (*LocalState, error) { if st.Versions == nil { st.Versions = map[string]string{} } + if st.Sent == nil { + st.Sent = map[string]string{} + } return &st, nil } @@ -371,44 +405,43 @@ func SaveState(spaceRoot string, st *LocalState) error { return os.WriteFile(statePath(spaceRoot), raw, 0o644) } -// ResolveMode returns always|init-only|never for a path. +// ResolveMode decides whether a path syncs, and it decides what leaves this +// machine — so it is worth being able to read. +// +// Longest match wins, and an exact path beats a prefix of the same length. That +// is the rule people already assume from every ignore-file they have used, and +// it was not what the code did: there were two passes, the first with conditions +// that contradicted each other and could not be reasoned about, the second +// overwriting the first with a >= comparison that made the answer depend on the +// order the rules happened to be listed in. Two configurations with the same +// rules in a different order gave different answers about whether somebody's +// identity file was published to their team. +// +// A rule ending in "/" matches a subtree. Anything else matches that exact path. func ResolveMode(sync spacesvc.SyncConfig, path string) string { best := sync.Default if best == "" { best = "always" } bestLen := -1 + bestExact := false + for _, r := range sync.Rules { - prefix := strings.TrimSuffix(r.Path, "/") - if r.Path == path || strings.HasPrefix(path, strings.TrimSuffix(r.Path, "*")) { - if strings.HasSuffix(r.Path, "/") && strings.HasPrefix(path, r.Path) { - if len(r.Path) > bestLen { - best = r.Mode - bestLen = len(r.Path) - } - } else if r.Path == path { - if len(r.Path) > bestLen { - best = r.Mode - bestLen = len(r.Path) - } - } else if strings.HasSuffix(r.Path, "/") == false && strings.HasPrefix(path, prefix+"/") { - if len(prefix) > bestLen { - best = r.Mode - bestLen = len(prefix) - } - } + if r.Path == "" || r.Mode == "" { + continue } - } - // simpler second pass - for _, r := range sync.Rules { - if strings.HasSuffix(r.Path, "/") { - if strings.HasPrefix(path, r.Path) && len(r.Path) >= bestLen { - best = r.Mode - bestLen = len(r.Path) - } - } else if path == r.Path { - best = r.Mode - bestLen = len(r.Path) + exact := !strings.HasSuffix(r.Path, "/") + switch { + case exact && path == r.Path: + case !exact && strings.HasPrefix(path, r.Path): + default: + continue + } + // Ties go to the more specific rule rather than to whichever came last: + // "identity/me.md" beats "identity/" even though the strings are close + // in length, and an equal-length prefix never displaces an exact match. + if len(r.Path) > bestLen || (len(r.Path) == bestLen && exact && !bestExact) { + best, bestLen, bestExact = r.Mode, len(r.Path), exact } } return best @@ -476,12 +509,20 @@ func apiErr(res *http.Response) error { return &APIError{Status: res.StatusCode, Message: string(raw)} } -// collectPushOps gathers what this client should send upward. +// collectPushOps gathers what this client should send upward, and what the +// record of "sent" will be once the server accepts it. // // Split out of Push so the filter can be tested without a server: what does and // does not leave the machine is a question worth being able to ask directly. -func collectPushOps(spaceRoot string, sync spacesvc.SyncConfig) ([]spacesvc.PushOp, error) { +func collectPushOps(spaceRoot string, sync spacesvc.SyncConfig, state *LocalState) ([]spacesvc.PushOp, map[string]string, error) { + previous := map[string]string{} + if state != nil && state.Sent != nil { + previous = state.Sent + } + var ops []spacesvc.PushOp + sent := make(map[string]string, len(previous)) + err := filepath.WalkDir(spaceRoot, func(p string, d os.DirEntry, err error) error { if err != nil { return err @@ -518,6 +559,11 @@ func collectPushOps(spaceRoot string, sync spacesvc.SyncConfig) ([]spacesvc.Push if err != nil { return err } + sum := contentHash(data) + sent[rel] = sum + if prev, ok := previous[rel]; ok && prev == sum { + return nil // unchanged since the last accepted push + } ops = append(ops, spacesvc.PushOp{ Op: "put", Path: rel, @@ -525,5 +571,28 @@ func collectPushOps(spaceRoot string, sync spacesvc.SyncConfig) ([]spacesvc.Push }) return nil }) - return ops, err + if err != nil { + return nil, nil, err + } + + // A path we sent before and cannot find now was deleted here. Without this + // the server kept its copy and the next pull put the file back, which reads + // as the tool undoing your work. + for rel := range previous { + if _, stillHere := sent[rel]; stillHere { + continue + } + if ResolveMode(sync, rel) == "never" { + continue + } + ops = append(ops, spacesvc.PushOp{Op: "delete", Path: rel}) + } + + return ops, sent, nil +} + +// contentHash identifies a file's contents for the "did I change this" question. +func contentHash(data []byte) string { + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) } diff --git a/internal/syncclient/identity_test.go b/internal/syncclient/identity_test.go index 5cccab1..e474ba2 100644 --- a/internal/syncclient/identity_test.go +++ b/internal/syncclient/identity_test.go @@ -39,7 +39,9 @@ func TestInitOnlyPathsAreNotPushedToTheSharedSpace(t *testing.T) { write("team/principles.md", "shared, and meant to be") write("identity/me.md", "Eduard, DevOps engineer, personal preferences") - ops, err := collectPushOps(root, defaultSync()) + // nil state: nothing has been sent from this machine yet, which is the + // first-push case and the one where everything eligible travels. + ops, _, err := collectPushOps(root, defaultSync(), nil) if err != nil { t.Fatal(err) } diff --git a/internal/syncclient/sync_test.go b/internal/syncclient/sync_test.go new file mode 100644 index 0000000..e8c0578 --- /dev/null +++ b/internal/syncclient/sync_test.go @@ -0,0 +1,305 @@ +package syncclient + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/orkcom-tech/contextverse/internal/spacesvc" +) + +func writeFile(t *testing.T, root, rel, body string) { + t.Helper() + p := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +func alwaysSync() spacesvc.SyncConfig { + return spacesvc.SyncConfig{Default: "always"} +} + +func opsByPath(ops []spacesvc.PushOp) map[string]string { + out := map[string]string{} + for _, o := range ops { + out[o.Path] = o.Op + } + return out +} + +// A push used to walk the tree and put every file every time, so publishing an +// edit to one document re-uploaded the whole space. +func TestOnlyChangedFilesArePushed(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "a.md", "one") + writeFile(t, root, "b.md", "two") + + // First push from a machine that has sent nothing: everything travels. + ops, sent, err := collectPushOps(root, alwaysSync(), nil) + if err != nil { + t.Fatal(err) + } + if len(ops) != 2 { + t.Fatalf("first push carried %d ops, want 2", len(ops)) + } + + // Nothing has changed since. + state := &LocalState{Sent: sent} + ops, _, err = collectPushOps(root, alwaysSync(), state) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Fatalf("an unchanged space produced %d ops: %+v", len(ops), ops) + } + + // One edit, one op. + writeFile(t, root, "b.md", "two, revised") + ops, _, err = collectPushOps(root, alwaysSync(), state) + if err != nil { + t.Fatal(err) + } + if got := opsByPath(ops); len(got) != 1 || got["b.md"] != "put" { + t.Errorf("got %+v, want only a put for b.md", got) + } +} + +// The one that made people distrust the tool: a file deleted here came back on +// the next pull, because a push only ever said "put". +func TestADeletedFileIsPushedAsADeletion(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "keep.md", "still here") + writeFile(t, root, "gone.md", "not for long") + + _, sent, err := collectPushOps(root, alwaysSync(), nil) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(root, "gone.md")); err != nil { + t.Fatal(err) + } + + ops, nextSent, err := collectPushOps(root, alwaysSync(), &LocalState{Sent: sent}) + if err != nil { + t.Fatal(err) + } + got := opsByPath(ops) + if got["gone.md"] != "delete" { + t.Errorf("got %+v, want a delete for gone.md", got) + } + if _, ok := got["keep.md"]; ok { + t.Errorf("an unchanged file was pushed as well: %+v", got) + } + if _, ok := nextSent["gone.md"]; ok { + t.Error("the record still lists a file that is gone") + } +} + +// A file that never travelled cannot be deleted remotely by disappearing here. +func TestAFileThatWasNeverSentIsNotDeleted(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "local-only.md", "mine") + + sync := spacesvc.SyncConfig{ + Default: "always", + Rules: []spacesvc.SyncRule{{Path: "local-only.md", Mode: "never"}}, + } + ops, sent, err := collectPushOps(root, sync, nil) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Fatalf("a never-sync file was pushed: %+v", ops) + } + if err := os.Remove(filepath.Join(root, "local-only.md")); err != nil { + t.Fatal(err) + } + ops, _, err = collectPushOps(root, sync, &LocalState{Sent: sent}) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Errorf("removing a never-sync file produced %+v", ops) + } +} + +// The record is only written once the server has taken the batch. Writing it +// earlier would make the next push skip the files that never arrived. +func TestTheSentRecordIsOnlyKeptAfterTheServerAcceptsIt(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "a.md", "one") + + refuses := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"code":"internal","message":"nope"}}`)) + })) + defer refuses.Close() + + c := &Client{BaseURL: refuses.URL, Space: "team", HTTP: refuses.Client()} + st := &LocalState{Sent: map[string]string{}} + if _, err := c.Push(context.Background(), root, "head", alwaysSync(), st, false); err == nil { + t.Fatal("a refused push reported success") + } + if len(st.Sent) != 0 { + t.Errorf("a failed push recorded %v as sent", st.Sent) + } +} + +func TestASuccessfulPushRecordsWhatItSent(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "a.md", "one") + + accepts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"head": "h2", "applied": 1}) + })) + defer accepts.Close() + + c := &Client{BaseURL: accepts.URL, Space: "team", HTTP: accepts.Client()} + st := &LocalState{Sent: map[string]string{}} + if _, err := c.Push(context.Background(), root, "h1", alwaysSync(), st, false); err != nil { + t.Fatal(err) + } + if _, ok := st.Sent["a.md"]; !ok { + t.Errorf("an accepted push recorded %v", st.Sent) + } +} + +// An empty batch must not be sent: it would move the head for no reason and +// make every other client re-check a space that did not change. +func TestNothingChangedSendsNothing(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "a.md", "one") + _, sent, err := collectPushOps(root, alwaysSync(), nil) + if err != nil { + t.Fatal(err) + } + + var called bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + called = true + _ = json.NewEncoder(w).Encode(map[string]any{"head": "h2", "applied": 0}) + })) + defer server.Close() + + c := &Client{BaseURL: server.URL, Space: "team", HTTP: server.Client()} + res, err := c.Push(context.Background(), root, "h1", alwaysSync(), &LocalState{Sent: sent}, false) + if err != nil { + t.Fatal(err) + } + if called { + t.Error("an empty push was sent to the server") + } + if res.Head != "h1" { + t.Errorf("head moved to %q on an empty push", res.Head) + } +} + +// ResolveMode decides what leaves the machine. Longest match wins and an exact +// path beats a prefix, which is what every ignore-file people have used does — +// and is not what the old two-pass version did, where the answer depended on the +// order the rules were listed in. +func TestResolveModeTakesTheLongestMatch(t *testing.T) { + rules := []spacesvc.SyncRule{ + {Path: "identity/", Mode: "init-only"}, + {Path: "identity/shared.md", Mode: "always"}, + {Path: "team/", Mode: "always"}, + {Path: "team/secrets/", Mode: "never"}, + } + sync := spacesvc.SyncConfig{Default: "always", Rules: rules} + + for path, want := range map[string]string{ + "identity/me.md": "init-only", + "identity/shared.md": "always", + "team/principles.md": "always", + "team/secrets/keys.md": "never", + "projects/example/notes.md": "always", // falls through to the default + } { + if got := ResolveMode(sync, path); got != want { + t.Errorf("%s = %q, want %q", path, got, want) + } + } +} + +// The same rules in a different order must give the same answers. They did not. +func TestResolveModeDoesNotDependOnRuleOrder(t *testing.T) { + rules := []spacesvc.SyncRule{ + {Path: "identity/", Mode: "init-only"}, + {Path: "identity/shared.md", Mode: "always"}, + {Path: "team/", Mode: "always"}, + {Path: "team/secrets/", Mode: "never"}, + } + reversed := make([]spacesvc.SyncRule, len(rules)) + for i, r := range rules { + reversed[len(rules)-1-i] = r + } + + forward := spacesvc.SyncConfig{Default: "always", Rules: rules} + backward := spacesvc.SyncConfig{Default: "always", Rules: reversed} + + for _, path := range []string{ + "identity/me.md", + "identity/shared.md", + "team/principles.md", + "team/secrets/keys.md", + "stray.md", + } { + a, b := ResolveMode(forward, path), ResolveMode(backward, path) + if a != b { + t.Errorf("%s: %q listed one way, %q the other", path, a, b) + } + } +} + +func TestResolveModeFallsBackToTheDefault(t *testing.T) { + if got := ResolveMode(spacesvc.SyncConfig{}, "anything.md"); got != "always" { + t.Errorf("an empty config gave %q, want always", got) + } + sync := spacesvc.SyncConfig{Default: "never"} + if got := ResolveMode(sync, "anything.md"); got != "never" { + t.Errorf("got %q, want the configured default", got) + } +} + +// An exact rule and a prefix rule of the same length: the exact one is the more +// specific statement and must win regardless of which was listed first. +func TestAnExactRuleBeatsAPrefixOfTheSameLength(t *testing.T) { + sync := spacesvc.SyncConfig{ + Default: "always", + Rules: []spacesvc.SyncRule{ + {Path: "notes/", Mode: "never"}, + {Path: "notes1", Mode: "always"}, // same length, exact + }, + } + if got := ResolveMode(sync, "notes1"); got != "always" { + t.Errorf("notes1 = %q, want always", got) + } + if got := ResolveMode(sync, "notes/x.md"); got != "never" { + t.Errorf("notes/x.md = %q, want never", got) + } +} + +// The listing the wizard offers. Decoded into the wrong shape, this failed on +// every call and the wizard blamed the server for it. +func TestListSpacesReadsTheServersShape(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"spaces":[{"name":"team","head":"h1"},{"name":"archive"}]}`)) + })) + defer server.Close() + + c := &Client{BaseURL: server.URL, HTTP: server.Client()} + spaces, err := c.ListSpaces(context.Background()) + if err != nil { + t.Fatalf("the listing the server actually sends was refused: %v", err) + } + if len(spaces) != 2 || spaces[0].Name != "team" || spaces[0].Head != "h1" || spaces[1].Name != "archive" { + t.Errorf("decoded %+v", spaces) + } +} diff --git a/internal/tui/filever.go b/internal/tui/filever.go index 821016d..8113836 100644 --- a/internal/tui/filever.go +++ b/internal/tui/filever.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "github.com/orkcom-tech/contextverse/internal/config" "github.com/orkcom-tech/contextverse/internal/spacefiles" @@ -93,13 +92,6 @@ func listSpaceFiles(fl *storage.FileLog, spaceRoot string) ([]TrackedFile, error return out, nil } -func skipStoragePath(p string) bool { - return strings.HasPrefix(p, storage.SnapshotPrefix) || - storage.IsFileLogInternal(p) || - strings.HasPrefix(p, "_health/") || - strings.HasPrefix(p, "_heads/") -} - func listVersionRows(fl *storage.FileLog, path string) (current int, rows []FileVersionRow, err error) { meta, versions, err := fl.ListVersions(context.Background(), path) if err != nil { diff --git a/internal/tui/styles.go b/internal/tui/styles.go index 09924aa..dd29020 100644 --- a/internal/tui/styles.go +++ b/internal/tui/styles.go @@ -22,13 +22,11 @@ var ( var ( styleBrand = lipgloss.NewStyle().Bold(true).Foreground(colAccent) - styleTitle = lipgloss.NewStyle().Bold(true).Foreground(colInk) styleMuted = lipgloss.NewStyle().Foreground(colMuted) styleSel = lipgloss.NewStyle().Foreground(colTabFg).Background(colAccent).Padding(0, 1) styleItem = lipgloss.NewStyle().Foreground(colInk).Padding(0, 1) styleOk = lipgloss.NewStyle().Foreground(colOk) styleErr = lipgloss.NewStyle().Foreground(colDanger) - styleBox = lipgloss.NewStyle().Border(lipgloss.RoundedBorder()).BorderForeground(colAccent).Padding(0, 1) styleHeader = lipgloss.NewStyle(). Bold(true). diff --git a/scripts/install.sh b/scripts/install.sh index 19251c6..4c7c7db 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -26,6 +26,40 @@ info() { log "==> $*"; } warn() { log "warning: $*"; } die() { log "error: $*"; exit 1; } +# verify_signature checks the release's cosign signature over checksums.txt. +# +# Keyless: the certificate is issued to the GitHub Actions identity that ran the +# release, so what is verified is "this came out of this repository's release +# workflow" — a question with an answer, unlike "do I recognise this key". +# +# Skipped with a warning when cosign is absent, because refusing to install +# because a verification tool is missing would push people to bypass the script +# entirely, which is worse than an unverified install they were told about. +verify_signature() { + local tag="$1" sums="$2" tmp="$3" + if ! command -v cosign >/dev/null 2>&1; then + warn "cosign not found; the download is checksummed but not verified as ours." + warn "Install cosign (https://docs.sigstore.dev) for a signature check." + return 0 + fi + local sig="${tmp}/checksums.txt.sig" cert="${tmp}/checksums.txt.pem" + local base="https://github.com/${REPO}/releases/download/${tag}" + if ! http_get "${base}/checksums.txt.sig" "$sig" 2>/dev/null || + ! http_get "${base}/checksums.txt.pem" "$cert" 2>/dev/null; then + warn "no signature published for ${tag}; releases before signing was added have none" + return 0 + fi + info "Verifying signature" + if ! cosign verify-blob \ + --signature "$sig" \ + --certificate "$cert" \ + --certificate-identity-regexp "https://github.com/${REPO}/.*" \ + --certificate-oidc-issuer "https://token.actions.githubusercontent.com" \ + "$sums"; then + die "signature verification failed for ${tag} — not installing" + fi +} + usage() { cat <<'EOF' ContextVerse installer — installs contextd @@ -177,6 +211,14 @@ download_release() { if [[ "$VERIFY_CHECKSUM" == "1" ]]; then local sums="${tmp}/checksums.txt" if http_get "https://github.com/${REPO}/releases/download/${tag}/checksums.txt" "$sums" 2>/dev/null; then + # The signature first, when cosign is available. + # + # A checksum on its own answers "did this download arrive intact", not + # "who made it": whoever can replace the binary on the release page can + # replace checksums.txt beside it, and this script fetches both from the + # same place. The signature is what makes the checksum worth checking. + verify_signature "$tag" "$sums" "$tmp" + info "Verifying checksum" ( cd "$tmp"