From e1ab19bd074e2e6672cdd4c33bf6872d65d5de07 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 25 Sep 2026 16:48:22 +0200 Subject: [PATCH 1/4] fix(client): default-port IPv6 literal addresses and bound REST JSON decodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hostPortAddr treated every IPv6 literal as already-ported (it contains ':'), so ws://[::1]/ws dialled an address with no port. Use u.Port() and net.JoinHostPort so bracketed literals and empty-port forms get the scheme default. getJSON now decodes through a 256 MiB LimitReader like ExportSession — a broken or hostile server streaming unbounded bytes must not OOM the client. --- internal/client/client.go | 9 ++++--- internal/client/client_addr_test.go | 39 +++++++++++++++++++++++++++++ internal/client/rest.go | 9 ++++++- 3 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 internal/client/client_addr_test.go diff --git a/internal/client/client.go b/internal/client/client.go index b41cd00..f4bb731 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -13,7 +13,6 @@ import ( "net" "net/http" "net/url" - "strings" "sync" "time" @@ -323,11 +322,13 @@ func dialWS(cfg *ws.Config) (*ws.Conn, error) { // scheme's standard port when absent (odek serve always prints one, but a // hand-typed ws://host URL should still dial). func hostPortAddr(u *url.URL) string { - if u.Host != "" && !strings.Contains(u.Host, ":") { + if u.Host != "" && u.Port() == "" { + // u.Port() is empty for bare hosts AND bracketed IPv6 literals + // ("[::1]" contains ':' but no port) — both get the scheme default. if u.Scheme == "wss" || u.Scheme == "https" { - return u.Host + ":443" + return net.JoinHostPort(u.Hostname(), "443") } - return u.Host + ":80" + return net.JoinHostPort(u.Hostname(), "80") } return u.Host } diff --git a/internal/client/client_addr_test.go b/internal/client/client_addr_test.go new file mode 100644 index 0000000..1894fd7 --- /dev/null +++ b/internal/client/client_addr_test.go @@ -0,0 +1,39 @@ +package client + +import ( + "net/url" + "testing" +) + +func parseURL(t *testing.T, raw string) *url.URL { + t.Helper() + u, err := url.Parse(raw) + if err != nil { + t.Fatalf("parse %s: %v", raw, err) + } + return u +} + +// Regression: hostPortAddr treated every IPv6 literal as "already has a +// port" because the address contains ':', so ws://[::1]/ws dialled an +// address with no port and failed with "missing port in address". +func TestHostPortAddrIPv6DefaultPort(t *testing.T) { + tests := []struct { + name, raw, want string + }{ + {"ipv6 ws default", "ws://[::1]/ws", "[::1]:80"}, + {"ipv6 wss default", "wss://[2001:db8::1]/ws", "[2001:db8::1]:443"}, + {"ipv6 explicit port", "ws://[::1]:8080/ws", "[::1]:8080"}, + {"ipv4 default", "ws://127.0.0.1/ws", "127.0.0.1:80"}, + {"hostname default", "ws://localhost/ws", "localhost:80"}, + {"explicit port", "ws://127.0.0.1:9000/ws", "127.0.0.1:9000"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + u := parseURL(t, tc.raw) + if got := hostPortAddr(u); got != tc.want { + t.Fatalf("hostPortAddr(%s) = %q, want %q", tc.raw, got, tc.want) + } + }) + } +} diff --git a/internal/client/rest.go b/internal/client/rest.go index c1cbdb1..ffa86a4 100644 --- a/internal/client/rest.go +++ b/internal/client/rest.go @@ -63,6 +63,11 @@ type ModelInfo struct { Current bool `json:"current"` } +// maxJSONBytes bounds any single REST JSON decode (sessions, models, jobs…). +// Transcripts can be large, but a broken server streaming unbounded bytes +// must not OOM the client. +const maxJSONBytes = 256 << 20 + // Sessions lists recent saved sessions (auth tokens are not included). func (c *Client) Sessions() ([]Session, error) { var out []Session @@ -391,5 +396,7 @@ func (c *Client) getJSON(u, sessionToken string, dst interface{}) error { if resp.StatusCode != http.StatusOK { return fmt.Errorf("status %s", resp.Status) } - return json.NewDecoder(resp.Body).Decode(dst) + // Bound the decode: a broken or hostile server streaming unbounded + // bytes must not OOM bodek (ExportSession already does this). + return json.NewDecoder(io.LimitReader(resp.Body, maxJSONBytes)).Decode(dst) } From 79918876870e0061d7e9af28be10d7f2751d5d6a Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 25 Sep 2026 16:48:22 +0200 Subject: [PATCH 2/4] fix(server): preserve attach-URL params, guard recycled PIDs, reassemble split stderr lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit splitTokenURL cleared the whole RawQuery, silently dropping non-token parameters from pasted attach URLs — strip only the token. Stop signalled the process group without checking whether the reaper had already observed the child's death, risking a SIGINT into a recycled PID's group. appendTail recorded a stderr line straddling two pipe writes severed in half — merge the buffered partial line before splitting. --- internal/server/server.go | 18 ++++++++++++++++-- internal/server/server_split_test.go | 27 +++++++++++++++++++++++++++ internal/server/server_tail_test.go | 19 +++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 internal/server/server_split_test.go create mode 100644 internal/server/server_tail_test.go diff --git a/internal/server/server.go b/internal/server/server.go index c7cb7ac..52ddc6b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -294,6 +294,11 @@ func (c *Conn) Stop() { if !c.stopping.CompareAndSwap(false, true) { return } + // An already-reaped child must not be signalled: the PID may have been + // recycled and the group kill would hit an innocent process. + if c.reaped.Load() { + return + } // Graceful shutdown owns the exit — retire the orphan watchdog first. c.watchMu.Lock() if c.watch != nil { @@ -375,8 +380,10 @@ func splitTokenURL(raw string) (base, token string) { if err != nil { return raw, "" } - token = u.Query().Get("token") - u.RawQuery = "" + q := u.Query() + token = q.Get("token") + q.Del("token") // strip only the token; other params must survive + u.RawQuery = q.Encode() u.Fragment = "" return u.String(), token } @@ -452,6 +459,13 @@ func (s *tokenScanWriter) scan(p []byte) { // appendTail splits p into complete lines and keeps the last maxTailLines // of them in the diagnostics tail. Callers hold s.mu. func (s *tokenScanWriter) appendTail(p []byte) { + // A chunk may start with the tail of a line whose head was buffered by + // a previous partialTail call — merge before splitting, or the line + // lands severed in the diagnostics tail. + if len(s.buf) > 0 { + p = append(s.buf, p...) + s.buf = nil + } rest := p for { i := bytes.IndexByte(rest, '\n') diff --git a/internal/server/server_split_test.go b/internal/server/server_split_test.go new file mode 100644 index 0000000..97b1b5b --- /dev/null +++ b/internal/server/server_split_test.go @@ -0,0 +1,27 @@ +package server + +import ( + "testing" +) + +// Regression: splitTokenURL cleared the entire RawQuery, so an attach URL +// with extra parameters (?token=x&profile=y) silently lost them. +func TestSplitTokenURLKeepsOtherParams(t *testing.T) { + base, token := splitTokenURL("http://127.0.0.1:8080/?token=abc&profile=dev") + if token != "abc" { + t.Fatalf("token = %q, want abc", token) + } + if want := "http://127.0.0.1:8080/?profile=dev"; base != want { + t.Fatalf("base = %q, want %q", base, want) + } + // A token-only URL still strips cleanly. + base, token = splitTokenURL("http://127.0.0.1:8080/?token=abc") + if token != "abc" || base != "http://127.0.0.1:8080/" { + t.Fatalf("token-only URL: base=%q token=%q", base, token) + } + // Fragments are stripped too. + base, token = splitTokenURL("http://127.0.0.1:8080/?token=abc#frag") + if token != "abc" || base != "http://127.0.0.1:8080/" { + t.Fatalf("fragment not stripped: base=%q token=%q", base, token) + } +} diff --git a/internal/server/server_tail_test.go b/internal/server/server_tail_test.go new file mode 100644 index 0000000..56c9aaa --- /dev/null +++ b/internal/server/server_tail_test.go @@ -0,0 +1,19 @@ +package server + +import ( + "io" + "testing" +) + +// Regression: appendTail never merged the buffered partial line (s.buf) into +// the incoming chunk, so a stderr line straddling two pipe writes lost its +// head half — the tail recorded "ror: boom" instead of "error: boom". +func TestTailSplitsAcrossWritesStayWhole(t *testing.T) { + s := &tokenScanWriter{w: io.Discard, tok: "found", tail: []string{}} + s.Write([]byte("er")) // no newline: buffered + s.Write([]byte("ror: bind: address already in use\n")) + got := s.Tail(4) + if got != "error: bind: address already in use" { + t.Fatalf("split line not reassembled whole in tail: %q", got) + } +} From 4d0da831a6a3d5422ce0dec81bee445e9651005c Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 25 Sep 2026 16:48:22 +0200 Subject: [PATCH 3/4] fix(persistence): converge token stores across instances and rotate corrupt-file backups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tokens' persist rewrote the whole map from the in-memory snapshot, so concurrent bodek instances erased each other's minted tokens; adopt the on-disk map wholesale on mutation (caller's own id re-applied) the way workspace's reloadLocked already does, so peer deletions converge too. The corrupt-store quarantine used a fixed .corrupt name, so a second corrupting write destroyed the first quarantined evidence — rotate to .corrupt.1 in both stores. Workspace's failed quarantine also left the path set, letting the next Save overwrite unrecoverable bytes; mirror tokens and stop persisting with a warning. --- internal/tokens/tokens.go | 40 +++++++++- internal/tokens/tokens_merge_test.go | 78 ++++++++++++++++++++ internal/workspace/workspace.go | 21 +++++- internal/workspace/workspace_corrupt_test.go | 35 +++++++++ 4 files changed, 171 insertions(+), 3 deletions(-) create mode 100644 internal/tokens/tokens_merge_test.go create mode 100644 internal/workspace/workspace_corrupt_test.go diff --git a/internal/tokens/tokens.go b/internal/tokens/tokens.go index 21b6331..0c99b33 100644 --- a/internal/tokens/tokens.go +++ b/internal/tokens/tokens.go @@ -45,7 +45,7 @@ func openAt(path string) *Store { return s // missing store: fresh start } if err := json.Unmarshal(data, &s.m); err != nil { - if qErr := os.Rename(path, path+".corrupt"); qErr == nil { + if qErr := quarantine(path); qErr == nil { warnPersist(fmt.Errorf("corrupt store quarantined as %s.corrupt: %w", path, err)) } else { warnPersist(fmt.Errorf("corrupt store kept in place: %w", err)) @@ -80,6 +80,7 @@ func (s *Store) Set(id, token string) { if s.m[id] == token { return // no change; skip the disk write } + s.mergeLocked() s.m[id] = token s.persistLocked() } @@ -94,10 +95,34 @@ func (s *Store) Delete(id string) { if _, ok := s.m[id]; !ok { return } + s.mergeLocked() delete(s.m, id) s.persistLocked() } +// mergeLocked adopts the on-disk map wholesale so deletions by a peer +// converge (a plain add-only merge let a stale peer rewrite every token +// another instance had deleted on its next unrelated persist). The caller +// re-applies its own mutation right after — that id wins. Mirrors +// workspace's reloadLocked. +func (s *Store) mergeLocked() { + if s.path == "" { + return + } + data, err := os.ReadFile(s.path) + if err != nil { + return // missing or unreadable: keep what we have + } + var disk map[string]string + if json.Unmarshal(data, &disk) != nil { + return // corrupt: Open's quarantine owns the diagnosis + } + if disk == nil { + disk = map[string]string{} + } + s.m = disk +} + // persistLocked writes the store while the mutex is held. Snapshot-then- // persist-outside-the-lock let interleaved Set/Delete writes reorder on // disk: an older snapshot landing last resurrected deleted tokens and @@ -154,6 +179,19 @@ func persist(path string, m map[string]string) error { return nil } +// quarantine sets a corrupt store aside as .corrupt, rotating any +// earlier backup to .corrupt.1 so repeat corruption never destroys the +// previous quarantined evidence (POSIX rename replaces its destination). +func quarantine(path string) error { + dst := path + ".corrupt" + if _, err := os.Stat(dst); err == nil { + if err := os.Rename(dst, dst+".1"); err != nil { + return err + } + } + return os.Rename(path, dst) +} + // warnPersist reports a failed best-effort save without aborting the // operation: the store stays a working in-memory cache, but a silent failure // would break session resume with no diagnostic. diff --git a/internal/tokens/tokens_merge_test.go b/internal/tokens/tokens_merge_test.go new file mode 100644 index 0000000..be3b3a0 --- /dev/null +++ b/internal/tokens/tokens_merge_test.go @@ -0,0 +1,78 @@ +package tokens + +import ( + "os" + "path/filepath" + "testing" +) + +// Regression: persistLocked rewrote the whole map from the in-memory +// snapshot without re-reading disk, so two concurrent bodek instances +// erased each other's minted session tokens (B's token vanished when A's +// next Set persisted its stale snapshot). +func TestSetMergesForeignTokens(t *testing.T) { + path := filepath.Join(t.TempDir(), "sessions.json") + a := openAt(path) + b := openAt(path) + + a.Set("sess-a", "tok-a") // instance A mints and persists + b.Set("sess-b", "tok-b") // instance B, opened before A's write + + // A fresh store must see both tokens. + c := openAt(path) + if got := c.Get("sess-a"); got != "tok-a" { + t.Fatalf("foreign token lost: Get(sess-a) = %q", got) + } + if got := c.Get("sess-b"); got != "tok-b" { + t.Fatalf("own token lost: Get(sess-b) = %q", got) + } +} + +// Regression: a stale peer must not resurrect tokens another instance +// deleted. B (opened before A's delete) writes an unrelated token — the +// wholesale disk adoption must keep the deletion converged. +func TestStalePeerKeepsPeerDeletions(t *testing.T) { + path := filepath.Join(t.TempDir(), "sessions.json") + a := openAt(path) + b := openAt(path) + + a.Set("sess-x", "tok-x") // both instances now know sess-x + a.Delete("sess-x") // A deletes it (persists the deletion) + + b.Set("sess-y", "tok-y") // stale B persists an unrelated write + + c := openAt(path) + if got := c.Get("sess-x"); got != "" { + t.Fatalf("deleted token resurrected by stale peer: %q", got) + } + if got := c.Get("sess-y"); got != "tok-y" { + t.Fatalf("unrelated token lost: %q", got) + } +} + +// Regression: the .corrupt quarantine used a fixed name, so a second +// corrupting write replaced the first quarantined evidence (POSIX rename +// replaces its destination) — the earlier snapshot became undiagnosable. +func TestQuarantineRotatesBackups(t *testing.T) { + path := filepath.Join(t.TempDir(), "sessions.json") + + if err := os.WriteFile(path, []byte("{first"), 0o600); err != nil { + t.Fatal(err) + } + openAt(path) + first, err := os.ReadFile(path + ".corrupt") + if err != nil || string(first) != "{first" { + t.Fatalf("first quarantine missing: %q err=%v", first, err) + } + + if err := os.WriteFile(path, []byte("{second"), 0o600); err != nil { + t.Fatal(err) + } + openAt(path) + if got, _ := os.ReadFile(path + ".corrupt"); string(got) != "{second" { + t.Fatalf("latest quarantine wrong: %q", got) + } + if got, err := os.ReadFile(path + ".corrupt.1"); err != nil || string(got) != "{first" { + t.Fatalf("first quarantine was clobbered: %q err=%v", got, err) + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index a5caa32..d5cd349 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -61,9 +61,13 @@ func Open() *Store { if json.Unmarshal(data, &f) != nil || f.Workspaces == nil { // Corrupt on disk: quarantine instead of silently resetting, so a // torn write never destroys every draft/queue/session undiagnosably - // (mirrors tokens.go). - if qerr := os.Rename(s.path, s.path+".corrupt"); qerr == nil { + // (mirrors tokens.go, including backup rotation). + if qerr := quarantine(s.path); qerr == nil { fmt.Fprintf(os.Stderr, "bodek: warning: corrupt %s quarantined as %s.corrupt\n", s.path, s.path) + } else { + // Quarantine failed: never overwrite bytes we could not parse. + fmt.Fprintf(os.Stderr, "bodek: warning: corrupt %s kept in place: %v\n", s.path, qerr) + s.path = "" } return s } @@ -71,6 +75,19 @@ func Open() *Store { return s } +// quarantine sets a corrupt store aside as .corrupt, rotating any +// earlier backup to .corrupt.1 so repeat corruption never destroys the +// previous quarantined evidence (POSIX rename replaces its destination). +func quarantine(path string) error { + dst := path + ".corrupt" + if _, err := os.Stat(dst); err == nil { + if err := os.Rename(dst, dst+".1"); err != nil { + return err + } + } + return os.Rename(path, dst) +} + // reloadLocked re-reads the on-disk store and adopts the on-disk state for // every cwd EXCEPT `except` (the caller is about to overwrite that one — // its in-memory value is the newest). Another bodek instance may have diff --git a/internal/workspace/workspace_corrupt_test.go b/internal/workspace/workspace_corrupt_test.go new file mode 100644 index 0000000..1052536 --- /dev/null +++ b/internal/workspace/workspace_corrupt_test.go @@ -0,0 +1,35 @@ +package workspace + +import ( + "os" + "path/filepath" + "testing" +) + +// Regression: the corrupt-store quarantine used a fixed .corrupt name, so a +// second corrupting write replaced the first quarantined evidence instead of +// rotating it aside. Mirrors the tokens.go fix. +func TestQuarantineRotatesBackups(t *testing.T) { + path := filepath.Join(t.TempDir(), "workspaces.json") + t.Setenv("BODEK_WORKSPACE", path) + + if err := os.WriteFile(path, []byte("{first"), 0o600); err != nil { + t.Fatal(err) + } + Open() + first, err := os.ReadFile(path + ".corrupt") + if err != nil || string(first) != "{first" { + t.Fatalf("first quarantine missing: %q err=%v", first, err) + } + + if err := os.WriteFile(path, []byte("{second"), 0o600); err != nil { + t.Fatal(err) + } + Open() + if got, _ := os.ReadFile(path + ".corrupt"); string(got) != "{second" { + t.Fatalf("latest quarantine wrong: %q", got) + } + if got, err := os.ReadFile(path + ".corrupt.1"); err != nil || string(got) != "{first" { + t.Fatalf("first quarantine was clobbered: %q err=%v", got, err) + } +} From a5b94446cfaf3c34272274b8b72f45462612bcf0 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 25 Sep 2026 16:48:22 +0200 Subject: [PATCH 4/4] fix(tui): drop pending approvals and clarify on /clear and error teardown /clear mid-turn wiped the transcript but left approvals, deadlines, bells, and clarify armed over a dead request, keeping the keyboard gated; errMsg's teardown cleared approvals and deadlines but not the bell latch, breaking the documented lockstep invariant. Both paths now tear down the full set, matching done/disconnect. --- internal/tui/clear_approval_teardown_test.go | 50 ++++++++++++++++++++ internal/tui/model.go | 9 ++++ 2 files changed, 59 insertions(+) create mode 100644 internal/tui/clear_approval_teardown_test.go diff --git a/internal/tui/clear_approval_teardown_test.go b/internal/tui/clear_approval_teardown_test.go new file mode 100644 index 0000000..294981f --- /dev/null +++ b/internal/tui/clear_approval_teardown_test.go @@ -0,0 +1,50 @@ +package tui + +import ( + "errors" + "testing" + "time" + + "github.com/BackendStack21/bodek/internal/client" +) + +// Regression: the errMsg teardown cleared approvals and deadlines but left +// apprBells armed, breaking the documented lockstep invariant — the next +// approval popped a stale bell latch. +func TestErrMsgTeardownKeepsBellLockstep(t *testing.T) { + m := newTestModel() + feedApproval(t, m, client.Event{Type: "approval_request", ID: "apr", Risk: "shell_exec", Command: "rm x"}) + if len(m.apprBells) != len(m.apprDeadlines) { + t.Fatalf("precondition: bells/deadlines out of lockstep: %d/%d", + len(m.apprBells), len(m.apprDeadlines)) + } + + m.Update(errMsg{err: errors.New("write failed")}) + + if len(m.approvals) != 0 || len(m.apprDeadlines) != 0 { + t.Fatalf("teardown left approvals armed: %d/%d", len(m.approvals), len(m.apprDeadlines)) + } + if len(m.apprBells) != 0 { + t.Fatalf("teardown left bells armed (lockstep broken): %d", len(m.apprBells)) + } +} + +// Regression: /clear mid-turn wiped the transcript but left the approval +// queue, deadlines, bells, and clarify armed over a dead request — the +// keyboard stayed gated by a request that no longer had a turn behind it. +// Every other teardown path (done/error/disconnect) clears them explicitly. +func TestClearConversationDropsPendingApprovals(t *testing.T) { + m := newTestModel() + feedApproval(t, m, client.Event{Type: "approval_request", ID: "apr", Risk: "shell_exec", Command: "rm x"}) + m.apprDeadlines[0] = time.Now().Add(time.Minute) + + m.clearConversation() + + if len(m.approvals) != 0 || len(m.apprDeadlines) != 0 || len(m.apprBells) != 0 { + t.Fatalf("/clear left approvals armed: appr=%d dl=%d bells=%d", + len(m.approvals), len(m.apprDeadlines), len(m.apprBells)) + } + if m.clarify != nil || m.clarifyBuf != "" { + t.Fatalf("/clear left clarify armed: q=%v buf=%q", m.clarify != nil, m.clarifyBuf) + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 8608162..b1630d0 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -578,6 +578,7 @@ func (m *Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // the keyboard after busy is already false. m.approvals = nil m.apprDeadlines = nil + m.apprBells = nil // lockstep with apprDeadlines — a stale latch must not leak m.resetApprovalInput() m.clearClarify() m.relayout() // the busy status line releases its row @@ -1189,6 +1190,14 @@ func (m *Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { func (m *Model) clearConversation() tea.Cmd { m.inspect = nil m.focusIdx = -1 // stale anchor would copy/move against the regrown transcript + // Pending approvals and clarify die with the conversation — leaving them + // armed captures the keyboard over a request with no turn behind it + // (the same contract done/error/disconnect document). + m.approvals = nil + m.apprDeadlines = nil + m.apprBells = nil + m.resetApprovalInput() + m.clearClarify() captureHome(m) m.msgs = nil m.curIdx = -1