From 34653f02238f438c6699848b15a488eda209a1ee Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:13:40 +0300 Subject: [PATCH 01/12] Stop losing a file, a secret and a boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places where something quietly succeeded when it should have refused. A PUT over the cap was stored truncated. handlePutFile read through a 32 MiB io.LimitReader, which stops at the cap and reports success, so io.ReadAll cannot tell a body that ended from one that had more to give. Upload 40 MiB and the server answered 200 with a document two thirds of the way through a sentence. Silent truncation on a write path is the worst kind of bug, because the client is told its file is safe. readAtMost asks for one byte past the limit, which is what tells the two apart, and the limit now comes from the space's own max_file_size rather than a constant that had nothing to do with it — a 413 with the actual number, not a shorter file. config.yaml and the server's config.yaml were written 0644 while holding git_token, s3_secret_key and sql_dsn with the password inline. 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 given away for nothing. They are written owner-only now and repaired on read, because a config written by an earlier version is already on disk and nobody is going to go looking. A file left stricter than we ask for is left alone. The MCP reader compared path strings, which a symlink walks straight past: a link at team/notes.md pointing to ~/.ssh/id_rsa passed every check and was 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 the paths the server names, so a hostile or compromised server plants the link and the contents leave the machine inside a prompt. storage.ResolveUnder existed for exactly this and is used everywhere else; it resolves the deepest existing ancestor before comparing, so the link is followed first and judged afterwards. The listing skips them too, so the model is never offered a file the reader will refuse, and a single file is capped at 1 MiB because the whole body goes into a tool result. This package had no tests at all, which is unfortunate for the one surface that hands files to a language model. It has them now, and the escape was verified by restoring the old check and watching the test return the private key. Co-Authored-By: Claude Opus 5 --- internal/config/config.go | 33 +++++- internal/config/perms_test.go | 118 +++++++++++++++++++++ internal/config/server.go | 14 ++- internal/mcpserver/read_test.go | 169 ++++++++++++++++++++++++++++++ internal/mcpserver/server.go | 41 ++++++-- internal/server/bodylimit_test.go | 66 ++++++++++++ internal/server/server.go | 40 ++++++- 7 files changed, 465 insertions(+), 16 deletions(-) create mode 100644 internal/config/perms_test.go create mode 100644 internal/mcpserver/read_test.go create mode 100644 internal/server/bodylimit_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 9385820..37be756 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. @@ -153,6 +155,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 +201,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/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/server.go b/internal/server/server.go index 86eadfc..2d177fe 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -839,7 +839,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 +1102,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 == "" { From 00417159133efe38754ec0793523e59540b8d0d2 Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:19:23 +0300 Subject: [PATCH 02/12] Say a new release exists, once, and only to a person MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing told anyone a newer contextd had shipped. The hard part is not the check, it is not becoming the thing people turn off — and a version notice has three ways to go wrong, in order of damage. It corrupts something. `contextd mcp serve` speaks JSON-RPC over stdio, so a stray line there breaks the AI client outright; --json and --yaml exist to be piped into a parser; anything on stdout can end up inside a pipe. So the notice goes to stderr and never stdout, and the commands whose output belongs to a machine — mcp serve, daemon run, completion, server start — are excluded by name rather than trusted to be careful. It costs time. So the check never blocks: a command prints whatever is already cached and refreshes in the background for next time. The first run after an install is therefore silent, which is right — somebody who just downloaded this does not need to hear about a release. 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, at most one check a day, and a failed check still counts as having asked so an offline machine does not retry on every command. What is left is a single line the first time a release appears, to a person at a terminal, with a permanent opt-out in config (no_update_check) and an environment one (CONTEXTD_NO_UPDATE_CHECK). Scripts, CI and AI integrations see nothing, ever. Two things the tests found rather than confirmed. A development build was going to be nagged: the comment said 0.0.0-dev had no place in the sequence, but the parser stripped the suffix and compared the numbers, so every contributor would have been told they were nine releases behind. parseRelease now answers the "should we speak" question separately from the "which is newer" one, and a dev build does not even reach for the network, because no answer would change what it does. And the first version of the concurrency test waited on the wrong condition, so it read the previous answer and passed for the wrong reason. Co-Authored-By: Claude Opus 5 --- internal/cli/cli.go | 6 + internal/cli/update_notice.go | 101 +++++++++ internal/cli/update_notice_test.go | 106 +++++++++ internal/config/config.go | 5 + internal/selfupdate/check.go | 262 ++++++++++++++++++++++ internal/selfupdate/check_test.go | 334 +++++++++++++++++++++++++++++ 6 files changed, 814 insertions(+) create mode 100644 internal/cli/update_notice.go create mode 100644 internal/cli/update_notice_test.go create mode 100644 internal/selfupdate/check.go create mode 100644 internal/selfupdate/check_test.go 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/update_notice.go b/internal/cli/update_notice.go new file mode 100644 index 0000000..d95f10a --- /dev/null +++ b/internal/cli/update_notice.go @@ -0,0 +1,101 @@ +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. + if flagJSON || flagYAML { + 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 37be756..8ef7e1c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -43,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. // 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) + } + } +} From 2666183f7a846bb5681610eb3d2ffd20642b00ff Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:21:52 +0300 Subject: [PATCH 03/12] Serialize the git backend, which had no locking at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local takes a flock around every operation. This backend, implementing the same interface — one that promises compare-and-swap — took nothing. Two failures follow. The compare and the swap were separate steps: read the file, hash it, compare, write. Two writers both passed the comparison and the second silently discarded the first. And go-git's worktree is not safe for concurrent use, because Add and Commit share one index file; overlapping commits corrupt it outright. One mutex over the whole backend rather than one per path, because the index and HEAD belong to the repository and not to any single file. A commit per write is already the slow part, so contending on a lock costs nothing that was not already being paid. Push is split into a locked entry point and an unlocked pushLocked for the callers already holding it — separated rather than made reentrant, because a mutex that is sometimes held twice is a deadlock waiting for the day somebody adds a call. The tests assert outcomes rather than timing, since a race reproduces unreliably: exactly one winner per contended create, per contended overwrite and per contended head update, and a repository still readable afterwards. Removing the locks fails all four, and the fourth fails with "unknown extension" and "EOF" from go-git's index parser — which is what a corrupted index reads like, and is worse than any lost write. Co-Authored-By: Claude Opus 5 --- internal/storage/git.go | 44 +++++- internal/storage/git_concurrency_test.go | 176 +++++++++++++++++++++++ 2 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 internal/storage/git_concurrency_test.go diff --git a/internal/storage/git.go b/internal/storage/git.go index e51ced2..d059d2d 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 @@ -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) + } + } +} From ae95e1f5f11c0f43865ea31618117aa7cd26f99d Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:27:20 +0300 Subject: [PATCH 04/12] Make the S3 key the path, not sixty-four bits of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects with one cause: the key said nothing about the path. It was SHA-256 truncated to eight bytes. Sixty-four bits deciding which file you are reading, where Local used the full digest for the same job and nothing recorded why S3 was cut short. A collision means one file silently overwriting another with no error anywhere, and sixty-four bits is inside reach — about 2^32 candidates to hit by chance, far fewer to construct deliberately. And because the key was opaque, listing a space meant downloading every object in the bucket to read the path back out of its body. One GET with the whole file on the wire, per file, on every tree, every changes and every quota check. That is a bill and a wait for an answer S3 already had. Keys are now the path, escaped only where S3 or a person would trip over it — slashes kept, so a bucket listing looks like the space it holds. Collisions become impossible rather than unlikely, and a listing recovers every path from the keys alone. The version still has to be right. It is the CAS token callers compare against, so inventing one from the ETag — which is a hash of the stored record, not of the file — would have made S3 disagree with every other backend about what version a file is. It is stamped into the object's user metadata instead and read back with a HeadObject: headers, no body. N round trips still, but nothing is downloaded, which was the actual cost. A single-request listing needs an index object, and that is a design change with its own concurrency problems, not something to smuggle in here. Existing buckets keep working. A read falls back to the old key, a write to a path still living there moves it and removes the old copy, and a delete removes whichever key actually held the object. A bucket converges as it is used, and a listing pays the old price only for the objects nobody has touched yet. Legacy keys are recognised by shape so the listing cannot mistake one for a file named after a hash. Co-Authored-By: Claude Opus 5 --- internal/storage/s3.go | 178 ++++++++++++++++++++++++------ internal/storage/s3_keys.go | 181 +++++++++++++++++++++++++++++++ internal/storage/s3_keys_test.go | 157 +++++++++++++++++++++++++++ 3 files changed, 483 insertions(+), 33 deletions(-) create mode 100644 internal/storage/s3_keys.go create mode 100644 internal/storage/s3_keys_test.go diff --git a/internal/storage/s3.go b/internal/storage/s3.go index ced84a2..d8cb9b7 100644 --- a/internal/storage/s3.go +++ b/internal/storage/s3.go @@ -100,11 +100,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 +109,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 +154,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 +166,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 +190,108 @@ 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, 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}) + } + + 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}) } return out, nil } +// s3VersionMeta is the user-metadata key holding the CAS token. +const s3VersionMeta = "cv-version" + +// 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, error) { + head, err := s.client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(key), + }) + if err != nil { + return "", err + } + for k, v := range head.Metadata { + // S3 lowercases metadata keys, and SDKs differ on whether they hand + // them back canonicalised. + if strings.EqualFold(k, s3VersionMeta) && v != "" { + return Version(v), nil + } + } + rec, err := s.readRecordByKey(ctx, key) + if err != nil { + return "", err + } + return rec.Version, 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 +301,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 +319,10 @@ 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)}, } if etag != "" { input.IfMatch = aws.String(etag) @@ -236,6 +336,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 +355,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) + } + } +} From 5370be841135fa4275cf08b587ccb355ed1e8021 Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:28:59 +0300 Subject: [PATCH 05/12] Stop decoding every file to list their names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local record keeps a file's bytes in the same JSON document as its path and version, base64-encoded. List decoded into that record, so answering "what is in here" read and base64-decoded every byte of every file — on every tree, every changes and every quota check, while holding the store's single exclusive lock, which meant nothing else could touch the space until it finished. Decoding into a struct without the Data field is enough: encoding/json skips what it has nowhere to put. A hundred-megabyte space now costs a hundred megabytes of reads to list instead of a hundred megabytes of reads and decodes, and the lock is held for a fraction of the time. The tests check the part that would make this a bug rather than a speedup: the version a listing reports has to be the one Get reports, or the two disagree about what a caller is holding. Co-Authored-By: Claude Opus 5 --- internal/storage/local.go | 20 ++++++++-- internal/storage/local_test.go | 69 ++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/internal/storage/local.go b/internal/storage/local.go index 159af4a..bcdcb7e 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -64,6 +64,18 @@ type objectRecord struct { 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"` +} + func (l *Local) withLock(ctx context.Context, fn func() error) error { lockPath := filepath.Join(l.root, localLockFile) lock := flock.New(lockPath) @@ -126,14 +138,14 @@ 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}) + out = append(out, Entry{Path: hdr.Path, Version: hdr.Version}) return nil }) }) diff --git a/internal/storage/local_test.go b/internal/storage/local_test.go index 39006cd..b0736ac 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,71 @@ 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)) + } +} From b31f2d657c106b9e105e3fb93cef8e329a7b1e83 Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:37:50 +0300 Subject: [PATCH 06/12] Keep a credential out of the limiter and a forged line out of the log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small things on the request path, both of the same kind: a value nobody checked, kept somewhere it did not belong. The rate-limit key was "bearer:" plus the token. The limiter holds its buckets in a map that lives as long as the process, so every token presented to the server stayed in memory as a working credential long after the request that carried it — reachable by a core dump, a crash reporter, a debugger. It is hashed now. The limiter only ever needed to tell callers apart, which a hash does exactly as well. The existing test asserted the literal "bearer:cv-kim-secret", which pinned the defect as the intended behaviour; it now asserts the property instead. The request id came from a caller-controlled header and went verbatim into structured logs and the error envelope. A newline forges a log line, control bytes confuse whatever reads them, and an unbounded string is an unbounded log record. A caller may still bring its own id so a trace can be followed across services, but it has to be short and plainly printable; anything else is replaced rather than escaped, because a correlation id has no business carrying punctuation somebody has to reason about. The generated id is random rather than UnixNano. The clock is guessable, which 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 — precisely when telling them apart is the point. Co-Authored-By: Claude Opus 5 --- internal/server/clientip.go | 13 +++- internal/server/clientip_test.go | 18 ++++- internal/server/requestid_test.go | 117 ++++++++++++++++++++++++++++++ internal/server/server.go | 49 ++++++++++++- 4 files changed, 190 insertions(+), 7 deletions(-) create mode 100644 internal/server/requestid_test.go 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 2d177fe..03a71b7 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/") { From d8fb32ab2e051e26deb275fc8b2fc2e3e315bcce Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 11:57:21 +0300 Subject: [PATCH 07/12] Make a push carry the difference, including what was deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things in the sync path that a person would notice. A push walked the tree and put every file, every time, base64-encoded into one JSON body. Publishing an edit to one document re-uploaded the whole space. LocalState now records the content this machine last sent, so a push carries what changed — hashes rather than the server's version markers, because the question is "did I change this", which is about local content. And a push never said "delete". A file removed here stayed on the server and came back on the next pull, which reads as the tool undoing your work. A path in the record with no file beside it is a deletion, and now travels as one. A file that never left the machine cannot delete anything by disappearing, and a never-sync path is left alone either way. The record is written only after the server accepts the batch. Written earlier, a failed push would look delivered and the next one would skip exactly the files that never arrived. An empty batch is not sent at all: it would move the head for no reason and make every other client re-check a space that did not change. ResolveMode decides what leaves the machine, and could not be reasoned about. Two passes: the first with conditions that contradicted each other, the second overwriting it with a >= comparison that made the answer depend on the order the rules happened to be listed in — so the same rules in a different order could differ on whether somebody's identity file was published to their team. It is one pass now, longest match wins, an exact path beats a prefix of the same length. That is what every ignore-file anyone has used already does. ListSpaces could never succeed. The server answers {"spaces":[...]} and this decoded into a bare []SpaceInfo, so every call failed — and the wizard's fallback swallowed the error 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. Co-Authored-By: Claude Opus 5 --- internal/cli/client.go | 14 +- internal/syncclient/client.go | 166 +++++++++++---- internal/syncclient/identity_test.go | 4 +- internal/syncclient/sync_test.go | 305 +++++++++++++++++++++++++++ 4 files changed, 445 insertions(+), 44 deletions(-) create mode 100644 internal/syncclient/sync_test.go 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/syncclient/client.go b/internal/syncclient/client.go index 7787053..f498b26 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" @@ -128,6 +130,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 +146,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 +304,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 +349,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 +382,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 +400,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 +418,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 +522,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 +572,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 +584,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) + } +} From f302111525c93706398d766d43401b81921033bc Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 12:04:24 +0300 Subject: [PATCH 08/12] Put a gate in CI that means something, and clear what it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI ran go test and nothing else, so everything the last three sprints found would have passed it. -race on every platform, because the first run under the detector found a real data race and the class of bug it catches is the one least likely to reproduce on demand. go vet, including the integration-tagged files nothing ever compiled in CI. govulncheck, which reports a dependency only when a reachable call path exists and so stays quiet enough to be read. And golangci-lint. The linter set is deliberately narrow. The full one reports 489 findings — 409 errcheck on deliberate discards like `defer f.Close()`, 43 noctx, 15 errorlint. Switching all of that on produces a gate that is red from its first commit, and a gate nobody can get green is a gate somebody deletes. Those three are recorded as their own piece of work, with the numbers, instead of being enabled and then permanently ignored. What is enabled passes today, so a new finding is a new mistake — the only way a linter earns a place in CI. It found one real defect. Backend migration verified nothing: an `if` with an empty body, under a comment saying the content must match. A migration is the one operation where "it probably worked" will not do, because the operator is about to point the server at the new backend and stop consulting the old one, so a file that arrived wrong is wrong from then on. It reads back and compares bytes now, and stops on the first mismatch rather than carrying on. Content rather than version markers, because the comment was right about that part: two drivers derive versions differently from the same bytes. The rest was dead code, now gone: doBytes, skipStoragePath, orDefaultInt, auditError, two unused lipgloss styles, and an assignment overwritten before it was ever read. One was not dead but unused — structuredOutput, written to keep progress lines off a JSON stream and never called. The update notice added last sprint had hand-rolled the same check against the flags directly; it uses the helper now, because two answers to one question are free to drift apart. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 38 ++++++++++++++++++++++++++++-- .golangci.yml | 43 ++++++++++++++++++++++++++++++++++ internal/cli/exit.go | 10 -------- internal/cli/server.go | 7 ------ internal/cli/update_notice.go | 7 ++++-- internal/server/audit_emit.go | 8 ------- internal/server/server_test.go | 3 +-- internal/storage/history.go | 25 ++++++++++++++++---- internal/syncclient/client.go | 13 ---------- internal/tui/filever.go | 8 ------- internal/tui/styles.go | 2 -- 11 files changed, 105 insertions(+), 59 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bac5736..b34654a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -119,14 +119,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 +148,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 +201,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 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/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 index d95f10a..33d232a 100644 --- a/internal/cli/update_notice.go +++ b/internal/cli/update_notice.go @@ -57,8 +57,11 @@ func updateNoticeAllowed(cmd *cobra.Command) bool { if cmd == nil || quietCommands[cmd.CommandPath()] { return false } - // Structured output is somebody's input. - if flagJSON || flagYAML { + // 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. 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/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/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/syncclient/client.go b/internal/syncclient/client.go index f498b26..911efc7 100644 --- a/internal/syncclient/client.go +++ b/internal/syncclient/client.go @@ -88,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) 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). From 129b1300cd2b104cf58454107235fa69a2bf92df Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 12:09:12 +0300 Subject: [PATCH 09/12] Read the version the metadata names, not the copy beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A versioned write is three objects: the version blob, the metadata, the live mirror. No backend writes three things at once, so what can be arranged is that every way of stopping part-way leaves a state somebody can explain. It did not. 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. A read then returned the new bytes labelled with the old version, so every CAS token handed out described content the caller had never seen, and the next write compared against it and passed. Nothing detected it; nothing repaired it. Reordering alone would only move the inconsistency, so the read moved with it. Get now reads the version blob the metadata names rather than the live copy. Those were always two objects, and labelling one from the other was the bug underneath the ordering. With that, metadata is a real commit point. Stopping before it leaves an unreferenced blob — storage spent, nothing else, and the next attempt at that version number overwrites it. Stopping after it leaves the live mirror stale, which costs a listing that is briefly behind and no reader a wrong answer. The error says which of the two happened rather than reporting a clean failure over a committed write. The mirror stays, because it is what makes a space enumerable: List walks real paths, and a tree of hashed version blobs is not something anybody can look at. It is a mirror, and this was the one place treating it as the truth. A version the metadata names whose blob is missing is now an error rather than a silent fall back to the mirror, which would hand back content belonging to a different version than the one being reported. Verified by restoring the old read: two of the tests fail, one of them with the exact symptom — "read \"first\" at \"2\"". Co-Authored-By: Claude Opus 5 --- internal/storage/filelog.go | 69 +++++++-- internal/storage/filelog_atomic_test.go | 196 ++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 9 deletions(-) create mode 100644 internal/storage/filelog_atomic_test.go 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) + } + } +} From 0d60f517d5eac7e87183ccbd093d5d76add3c91f Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 12:14:36 +0300 Subject: [PATCH 10/12] Lock out the attacker, not the account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Login failures counted against the username and nothing else, so anybody who knew a name could lock its owner out by typing the wrong password five times. A denial of service that costs the attacker nothing, that the victim cannot distinguish from a real attack, and that is worse than the attack the lockout was protecting against. Failures now count twice: against the account and against the address they came from. The address budget is the tight one, because it is the budget an attacker actually spends and the only one that cannot be aimed at somebody else. The account budget stays, larger, for the case it is really for — failures arriving from many addresses, where no single address ever reaches its own limit. A success clears the account but not the address. One correct password from a machine working through a list of names must not wipe the record of the failures around it. The two counters are namespaced, because "203.0.113.9" is a legal username and sharing a map with addresses would let one lock the other. What this still does not solve is written down rather than implied: the counters live in one process, so a fleet behind a load balancer hands an attacker a fresh budget per replica and a restart clears everything. The OSS server has no shared state by design — its own docs call the HA "stateless, no clustering" — so an operator running replicas has to rate-limit authentication at the router. These values are a floor, not a fleet-wide guarantee. The existing test asserted lockout after MaxLoginFailures against the account, which pinned the old model; it now uses the account budget. Verified by putting the old counting back: the test that matters fails with "the real owner was locked out by somebody else's failures". Co-Authored-By: Claude Opus 5 --- internal/auth/auth_hardening_test.go | 4 +- internal/auth/lockout.go | 47 ++++++-- internal/auth/lockout_test.go | 159 +++++++++++++++++++++++++++ internal/auth/password.go | 31 +++++- internal/server/server.go | 2 +- internal/server/ui_handlers.go | 2 +- 6 files changed, 232 insertions(+), 13 deletions(-) create mode 100644 internal/auth/lockout_test.go 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/server/server.go b/internal/server/server.go index 03a71b7..3e84878 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -569,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) { 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" From 8182c478f05d955a66fdaea05b0ce3a992789d1a Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 12:18:12 +0300 Subject: [PATCH 11/12] Count a space by what the backend holds, not by what this replica cached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quota accounting listed the backend and then called os.Stat on the working-tree mirror to learn each file's size. The mirror is written by whichever replica handled the write, so with the s3 or sql driver every other replica found no file, counted it as nothing, and read the space as far smaller than it is — then let it grow past its limit. The server's own documentation calls its HA "stateless, no clustering", which is exactly the arrangement where this is wrong, so it was not a hypothetical for anyone running more than one process. storage.Entry carries a Size now and every backend fills it from what it actually knows. Local records the length when it writes the record, so a listing reports it without touching the body. SQL asks the database with octet_length rather than shipping every blob to count its bytes. S3 stamps the content's own length into object metadata beside the version — the object's reported size is the JSON wrapper, a third larger because of base64, so it is the wrong number. Git measures the file. Prefixed passes it through. Zero means "the backend did not say", not "the file is empty", so the working tree is still consulted as a fallback rather than a file of unknown size being counted as free space. An actually-empty file is zero either way, and a quota check treating it as zero is correct. Co-Authored-By: Claude Opus 5 --- internal/spacesvc/push.go | 38 ++++++++++++++--------- internal/spacesvc/service.go | 12 ++++--- internal/storage/backend.go | 19 +++++++++--- internal/storage/git.go | 2 +- internal/storage/local.go | 14 ++++++++- internal/storage/local_test.go | 57 ++++++++++++++++++++++++++++++++++ internal/storage/prefixed.go | 2 +- internal/storage/s3.go | 45 ++++++++++++++++++++------- internal/storage/sql.go | 7 +++-- 9 files changed, 157 insertions(+), 39 deletions(-) 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/git.go b/internal/storage/git.go index d059d2d..c3e833f 100644 --- a/internal/storage/git.go +++ b/internal/storage/git.go @@ -251,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) { diff --git a/internal/storage/local.go b/internal/storage/local.go index bcdcb7e..f90bf86 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -60,6 +60,7 @@ 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"` } @@ -74,6 +75,7 @@ type objectRecord struct { 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 { @@ -145,7 +147,16 @@ func (l *Local) List(ctx context.Context, prefix string) ([]Entry, error) { if prefix != "" && !strings.HasPrefix(hdr.Path, prefix) { return nil } - out = append(out, Entry{Path: hdr.Path, Version: hdr.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 }) }) @@ -174,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 b0736ac..996ea0b 100644 --- a/internal/storage/local_test.go +++ b/internal/storage/local_test.go @@ -185,3 +185,60 @@ func TestListFiltersByPrefix(t *testing.T) { 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 d8cb9b7..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" @@ -205,12 +206,12 @@ func (s *S3) List(ctx context.Context, prefix string) ([]Entry, error) { // 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, err := s.versionOf(ctx, obj.key) + 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}) + out = append(out, Entry{Path: obj.path, Version: ver, Size: size}) } for _, key := range legacyKeys { @@ -223,13 +224,16 @@ func (s *S3) List(ctx context.Context, prefix string) ([]Entry, error) { if prefix != "" && !strings.HasPrefix(rec.Path, prefix) { continue } - out = append(out, Entry{Path: rec.Path, Version: rec.Version}) + out = append(out, Entry{Path: rec.Path, Version: rec.Version, Size: int64(len(rec.Data))}) } return out, nil } -// s3VersionMeta is the user-metadata key holding the CAS token. -const s3VersionMeta = "cv-version" +// 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 { @@ -239,26 +243,37 @@ type listedObject struct { // 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, error) { +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 "", err + 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. - if strings.EqualFold(k, s3VersionMeta) && v != "" { - return Version(v), nil + 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 "", err + return "", 0, err } - return rec.Version, nil + return rec.Version, int64(len(rec.Data)), nil } // readRecordByKey fetches one object by its exact key, for the legacy objects a @@ -322,7 +337,13 @@ func (s *S3) Put(ctx context.Context, path string, data []byte, expected Version // 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)}, + 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) 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) From ca7e35e89e4c3590d756e58325ba69134c456947 Mon Sep 17 00:00:00 2001 From: edward lugovtsov Date: Fri, 31 Jul 2026 12:20:09 +0300 Subject: [PATCH 12/12] Sign releases, and let the installer check the signature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A checksum file signs nothing. Whoever can replace the binary on the release page can replace checksums.txt beside it, and the install script fetches both from the same URL — so the check answered "did this arrive intact", never "is this ours". That matters more here than for most projects, because the documented way to install this is `curl … | bash`. Releases are signed with cosign, keylessly: the certificate is issued to the GitHub Actions identity that ran the release and the fact is recorded in the transparency log. There is no private key for anybody to lose or rotate, and the question a verifier can answer is the useful one — "was this built by this repository's release workflow" rather than "do I recognise this key". The checksum file covers every artifact, so signing it signs the release. An SBOM per archive, because otherwise answering "is this release affected by CVE-x" means rebuilding it and hoping the dependency graph has not moved since. The installer verifies before it checksums, and is deliberately forgiving about one thing: a missing cosign is a warning, not a refusal. Refusing to install because a verification tool is absent pushes people to bypass the script entirely, which leaves them worse off than an unverified install they were told about. A signature that exists and does not verify is fatal. Releases published before this have no signature, and the installer says so rather than failing on them. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 10 ++++++++++ .goreleaser.yaml | 40 ++++++++++++++++++++++++++++++++++++++ scripts/install.sh | 42 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b34654a..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: @@ -222,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/.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/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"