diff --git a/API Server/Dockerfile b/API Server/Dockerfile index fd41196..affd3ec 100644 --- a/API Server/Dockerfile +++ b/API Server/Dockerfile @@ -23,7 +23,8 @@ RUN CGO_ENABLED=0 GOOS=linux go build -trimpath -ldflags="-s -w" -o /out/api-ser FROM alpine:3.24 # HTTPS calls to PocketBase need CA certificates; tzdata for correct timestamps. -RUN apk add --no-cache ca-certificates tzdata +# su-exec lets the entrypoint fix /data ownership as root and then drop to app. +RUN apk add --no-cache ca-certificates tzdata su-exec # Run as an unprivileged user. RUN addgroup -S app && adduser -S -G app app @@ -34,11 +35,34 @@ COPY --from=build /out/api-server /usr/local/bin/api-server # (plugin enable-state + config) and .env, which the panel rewrites when a # superadmin retargets the PocketBase connection. Both must therefore live on a # writable, persistent path — hence /data, owned by the unprivileged user and -# declared as a volume. A named volume mounted here inherits this ownership. +# declared as a volume. A fresh named volume inherits this ownership. RUN mkdir -p /data && chown app:app /data WORKDIR /data VOLUME /data +# A fresh named volume inherits /data's ownership, but two common cases do not: +# a host bind mount (API_DATA=/srv/... in docker-compose.prod.yml) arrives owned +# by root, and so does a volume created by an image from before /data existed, +# when the server ran with a root-owned working directory. In both cases the +# unprivileged process cannot write plugins.json — which shows up as plugins +# that enable fine in the panel and come back disabled after the next redeploy. +# So the entrypoint starts as root purely to fix ownership, then drops to app. +RUN cat > /entrypoint.sh <<'ENTRY' +#!/bin/sh +set -e +if [ "$(id -u)" = "0" ]; then + mkdir -p /data + if [ "$(stat -c %U /data 2>/dev/null)" != "app" ]; then + echo "entrypoint: taking ownership of /data" + chown -R app:app /data + fi + exec su-exec app "$@" +fi +# Already unprivileged (docker run --user ...): nothing to drop, just run. +exec "$@" +ENTRY +RUN chmod +x /entrypoint.sh + # Config comes entirely from environment variables (see .env.example). # POCKETBASE_ADMIN_EMAIL / _PASSWORD are optional at startup: without them the # server still runs and a superadmin can configure the connection from the panel. @@ -46,12 +70,12 @@ ENV API_ADDR=:8080 \ PLUGINS_FILE=/data/plugins.json EXPOSE 8080 -USER app - # Liveness only: /healthz answers 200 as soon as the process is serving, and # does not depend on PocketBase, so a database outage does not mark the # container unhealthy. Lets compose gate dependants on condition: service_healthy. HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \ CMD wget -qO- http://127.0.0.1:8080/healthz >/dev/null 2>&1 || exit 1 -ENTRYPOINT ["/usr/local/bin/api-server"] +# The entrypoint drops to the unprivileged app user after fixing /data. +ENTRYPOINT ["/entrypoint.sh"] +CMD ["/usr/local/bin/api-server"] diff --git a/API Server/internal/api/plugins.go b/API Server/internal/api/plugins.go index 800b5e6..19e005d 100644 --- a/API Server/internal/api/plugins.go +++ b/API Server/internal/api/plugins.go @@ -46,16 +46,20 @@ func (s *Server) handleUpdatePlugin(w http.ResponseWriter, r *http.Request) { } v, err := s.plugins.Upsert(r.Context(), name, enabled, body.Config) - if err != nil { - if plugins.IsUnknown(err) { - writeError(w, http.StatusNotFound, "unknown plugin") - return - } + switch { + case err == nil: + writeJSON(w, http.StatusOK, map[string]any{"plugin": v}) + case plugins.IsUnknown(err): + writeError(w, http.StatusNotFound, "unknown plugin") + case plugins.IsPersist(err): + // The change never reached plugins.json and has been rolled back. + // Reporting this as a 200-with-warning is what let a plugin look + // enabled in the panel and come back disabled after a redeploy. + writeError(w, http.StatusInternalServerError, err.Error()) + default: // A failed init (e.g. bad credentials) is reported but the state was saved. writeJSON(w, http.StatusOK, map[string]any{"plugin": v, "warning": err.Error()}) - return } - writeJSON(w, http.StatusOK, map[string]any{"plugin": v}) } // POST /api/admin/plugins — register an external (remote HTTP) plugin. Body: @@ -76,6 +80,10 @@ func (s *Server) handleRegisterPlugin(w http.ResponseWriter, r *http.Request) { return } if err := s.plugins.RegisterExternal(body.Name, body.BaseURL, body.Provider); err != nil { + if plugins.IsPersist(err) { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } writeError(w, http.StatusConflict, err.Error()) return } @@ -87,6 +95,10 @@ func (s *Server) handleRegisterPlugin(w http.ResponseWriter, r *http.Request) { // only be disabled). func (s *Server) handleDeletePlugin(w http.ResponseWriter, r *http.Request) { if err := s.plugins.Remove(r.Context(), r.PathValue("name")); err != nil { + if plugins.IsPersist(err) { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } writeError(w, http.StatusBadRequest, err.Error()) return } diff --git a/API Server/internal/api/server.go b/API Server/internal/api/server.go index 73ada05..77b0574 100644 --- a/API Server/internal/api/server.go +++ b/API Server/internal/api/server.go @@ -185,6 +185,13 @@ func New(cfg config.Config, client *pb.Client) *Server { // non-blocking; if PocketBase is not yet configured it no-ops and the lazy path // rebuilds on first connect. func (s *Server) StartPlugins() error { + // Surface an unwritable state directory at boot. Without this the first + // symptom is a superadmin enabling plugins, seeing them work, and finding + // them all disabled after the next redeploy — because every save failed. + if err := s.plugins.CheckWritable(); err != nil { + log.Printf("WARNING: %v", err) + log.Printf("WARNING: plugin changes will NOT survive a restart — make the directory holding PLUGINS_FILE writable by the container user") + } err := s.plugins.Load() go func() { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) diff --git a/API Server/internal/plugins/manager.go b/API Server/internal/plugins/manager.go index 0e1397c..49e6331 100644 --- a/API Server/internal/plugins/manager.go +++ b/API Server/internal/plugins/manager.go @@ -1,12 +1,15 @@ package plugins import ( + "bytes" "context" "encoding/json" "errors" + "fmt" "log" "net/http" "os" + "path/filepath" "sort" "strings" "sync" @@ -65,13 +68,15 @@ func (m *Manager) Load() error { m.mu.Lock() defer m.mu.Unlock() - if data, err := os.ReadFile(m.path); err == nil { - var recs map[string]*record - if err := json.Unmarshal(data, &recs); err != nil { + data, err := os.ReadFile(m.path) + switch { + case err == nil: + if err := m.decodeLocked(data); err != nil { return err } - m.records = recs - } else if !errors.Is(err, os.ErrNotExist) { + case errors.Is(err, os.ErrNotExist): + // No state file yet — first boot, nothing configured. + default: return err } @@ -94,6 +99,58 @@ func (m *Manager) Load() error { return nil } +// decodeLocked parses the state file into m.records. It is deliberately strict +// about two shapes that would otherwise take the server down or quietly destroy +// state: +// +// - An empty file, or a literal "null", decodes to a nil map. Assigning that +// to m.records makes the next save panic with "assignment to entry in nil +// map", so both are treated as "nothing configured" instead. +// - A null entry ({"toyota": null}) leaves a nil *record that the enable loop +// in Load would dereference. Those entries are dropped. +// +// Content that does not parse at all is moved aside rather than left in place: +// the server carries on with no plugins configured, and the next save would +// otherwise overwrite the unreadable file and take every setting in it along. +func (m *Manager) decodeLocked(data []byte) error { + if len(bytes.TrimSpace(data)) == 0 { + return nil + } + var recs map[string]*record + if err := json.Unmarshal(data, &recs); err != nil { + backup := m.path + ".corrupt" + if renameErr := os.Rename(m.path, backup); renameErr != nil { + return fmt.Errorf("plugin state file %s is unreadable (%v) and could not be set aside: %v", m.path, err, renameErr) + } + return fmt.Errorf("plugin state file %s was unreadable (%v); moved it to %s and started with no plugins configured", m.path, err, backup) + } + for name, rec := range recs { + if rec == nil { + delete(recs, name) + } + } + if recs != nil { + m.records = recs + } + return nil +} + +// CheckWritable reports whether the state file can actually be written, by +// creating and removing a temporary file beside it. Worth calling at startup: +// an unwritable state directory (a root-owned bind mount under an unprivileged +// process, or a volume left over from an image that ran as root) otherwise +// stays invisible until a restart brings every plugin back disabled. +func (m *Manager) CheckWritable() error { + f, err := os.CreateTemp(filepath.Dir(m.path), ".plugins-writecheck-*") + if err != nil { + return fmt.Errorf("%w: %v", errPersist, err) + } + name := f.Name() + _ = f.Close() + _ = os.Remove(name) + return nil +} + // construct builds a plugin instance from a builtin factory or an external record. func construct(name string, f Factory, rec *record) Plugin { if f != nil { @@ -185,7 +242,8 @@ func (m *Manager) Upsert(ctx context.Context, name string, enabled bool, incomin m.mu.Unlock() return View{}, errUnknown } - if rec == nil { + isNew := rec == nil + if isNew { rec = &record{} m.records[name] = rec } @@ -212,15 +270,28 @@ func (m *Manager) Upsert(ctx context.Context, name string, enabled bool, incomin if enabled { for _, f := range d.ConfigFields { if f.Required && merged[f.Key] == "" { + if isNew { + delete(m.records, name) // don't leave a blank record behind + } m.mu.Unlock() return View{}, errors.New("missing required setting: " + f.Label) } } } + prevEnabled, prevConfig := rec.Enabled, rec.Config rec.Enabled = enabled rec.Config = merged if err := m.persistLocked(); err != nil { + // Roll back, so the panel shows what is actually on disk. Keeping the + // change in memory is what made an unwritable state file look like a + // successful save — right up until the next restart brought it back + // disabled. + if isNew { + delete(m.records, name) + } else { + rec.Enabled, rec.Config = prevEnabled, prevConfig + } m.mu.Unlock() return View{}, err } @@ -268,7 +339,11 @@ func (m *Manager) RegisterExternal(name, baseURL, provider string) error { return errors.New("a plugin with that name already exists") } m.records[name] = &record{Kind: KindExternal, BaseURL: baseURL, Provider: provider} - return m.persistLocked() + if err := m.persistLocked(); err != nil { + delete(m.records, name) + return err + } + return nil } // Remove deletes an external plugin registration. Builtins can only be disabled. @@ -279,13 +354,19 @@ func (m *Manager) Remove(ctx context.Context, name string) error { if rec == nil || rec.Kind != KindExternal { return errors.New("only external plugins can be removed") } + // Persist before tearing the instance down, so a failed write leaves a + // still-registered plugin still running rather than a half-removed one. + delete(m.records, name) + if err := m.persistLocked(); err != nil { + m.records[name] = rec + return err + } if p := m.live[name]; p != nil { _ = p.Shutdown(ctx) delete(m.live, name) } - delete(m.records, name) delete(m.health, name) - return m.persistLocked() + return nil } // HealthCheck probes a plugin now, building a transient instance if it is not @@ -449,15 +530,58 @@ func (m *Manager) Shutdown(ctx context.Context) { } // persistLocked writes the state file. Caller must hold m.mu. +// +// The write goes to a temporary file in the same directory, is flushed, and is +// then renamed over the target. A truncating write in place can be interrupted +// (crash, container stop, full disk) and leave a half-written plugins.json that +// fails to parse on the next boot — which surfaces as every plugin coming back +// disabled. Every failure is wrapped in errPersist so callers can tell "your +// change was not saved" apart from "saved, but the plugin failed to start". func (m *Manager) persistLocked() error { data, err := json.MarshalIndent(m.records, "", " ") if err != nil { - return err + return fmt.Errorf("%w: %v", errPersist, err) } - return os.WriteFile(m.path, append(data, '\n'), 0o600) + data = append(data, '\n') + + tmp, err := os.CreateTemp(filepath.Dir(m.path), ".plugins-*.json") + if err != nil { + return fmt.Errorf("%w: %v", errPersist, err) + } + tmpName := tmp.Name() + defer func() { + if tmpName != "" { + _ = os.Remove(tmpName) + } + }() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return fmt.Errorf("%w: %v", errPersist, err) + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return fmt.Errorf("%w: %v", errPersist, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("%w: %v", errPersist, err) + } + if err := os.Rename(tmpName, m.path); err != nil { + return fmt.Errorf("%w: %v", errPersist, err) + } + tmpName = "" // renamed into place; nothing left to clean up + return nil } -var errUnknown = errors.New("unknown plugin") +var ( + errUnknown = errors.New("unknown plugin") + errPersist = errors.New("plugin state could not be saved") +) // IsUnknown reports whether err came from addressing a plugin that doesn't exist. func IsUnknown(err error) bool { return errors.Is(err, errUnknown) } + +// IsPersist reports whether err means the change never reached the state file. +// Such a change has been rolled back in memory: it must be reported as a +// failure, or the caller sees a save that silently vanishes on the next restart. +func IsPersist(err error) bool { return errors.Is(err, errPersist) } diff --git a/API Server/internal/plugins/manager_persist_test.go b/API Server/internal/plugins/manager_persist_test.go new file mode 100644 index 0000000..3f59993 --- /dev/null +++ b/API Server/internal/plugins/manager_persist_test.go @@ -0,0 +1,209 @@ +package plugins + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// testPlugin is a no-op plugin used to exercise the manager's persistence +// without reaching for a real connector (the builtins register themselves from +// a package that imports this one, so they are not available here). +type testPlugin struct{ name string } + +func (t *testPlugin) Descriptor() Descriptor { return Descriptor{Name: t.name} } +func (t *testPlugin) Init(context.Context, map[string]string) error { return nil } +func (t *testPlugin) Shutdown(context.Context) error { return nil } +func (t *testPlugin) HealthCheck(context.Context) Health { return Health{Status: "ok"} } +func (t *testPlugin) Invoke(context.Context, string, json.RawMessage) (json.RawMessage, error) { + return nil, nil +} + +func init() { + for _, n := range []string{"test-a", "test-b"} { + Register(n, func() Plugin { return &testPlugin{name: n} }) + } +} + +// A state file holding "null" (or nothing at all) used to decode to a nil map, +// which made the next save panic with "assignment to entry in nil map". +func TestLoadNullStateFileDoesNotPanic(t *testing.T) { + for _, content := range []string{"null", "", " \n"} { + path := filepath.Join(t.TempDir(), "plugins.json") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + m := NewManager(path) + if err := m.Load(); err != nil { + t.Fatalf("Load(%q): %v", content, err) + } + if _, err := m.Upsert(context.Background(), "test-a", true, nil); err != nil { + t.Fatalf("Upsert after %q state file: %v", content, err) + } + if v, _ := m.Get("test-a"); !v.Enabled { + t.Fatalf("plugin not enabled after save (state file was %q)", content) + } + } +} + +// A null entry for a single plugin left a nil *record that Load dereferenced. +func TestLoadNullRecordIsDropped(t *testing.T) { + path := filepath.Join(t.TempDir(), "plugins.json") + if err := os.WriteFile(path, []byte(`{"test-a":null,"test-b":{"enabled":true}}`), 0o600); err != nil { + t.Fatal(err) + } + m := NewManager(path) + if err := m.Load(); err != nil { + t.Fatalf("Load: %v", err) + } + if v, _ := m.Get("test-b"); !v.Enabled { + t.Fatal("test-b should still be enabled") + } +} + +// An unwritable state directory must fail loudly and leave the in-memory state +// matching the disk, rather than reporting success and reverting on restart. +func TestUpsertRollsBackWhenStateCannotBeSaved(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing-dir", "plugins.json") + m := NewManager(path) + if err := m.Load(); err != nil { + t.Fatalf("Load: %v", err) + } + + _, err := m.Upsert(context.Background(), "test-a", true, map[string]string{"k": "v"}) + if err == nil { + t.Fatal("expected an error when the state file cannot be written") + } + if !IsPersist(err) { + t.Fatalf("error should be classified as a persist failure, got %v", err) + } + if v, _ := m.Get("test-a"); v.Enabled { + t.Fatal("plugin reported as enabled although the save never reached disk") + } + if _, statErr := os.Stat(path); statErr == nil { + t.Fatal("state file unexpectedly exists") + } +} + +// A rolled-back save must not clobber the value that was already stored. +func TestUpsertRollbackKeepsPreviousConfig(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "plugins.json") + m := NewManager(path) + if err := m.Load(); err != nil { + t.Fatal(err) + } + if _, err := m.Upsert(context.Background(), "test-a", true, map[string]string{"user": "first"}); err != nil { + t.Fatal(err) + } + + // Make the next write fail by pointing the manager at an unusable directory. + m.path = filepath.Join(dir, "missing-dir", "plugins.json") + if _, err := m.Upsert(context.Background(), "test-a", false, map[string]string{"user": "second"}); !IsPersist(err) { + t.Fatalf("expected persist failure, got %v", err) + } + + v, _ := m.Get("test-a") + if !v.Enabled || v.Config["user"] != "first" { + t.Fatalf("failed save leaked into memory: enabled=%v user=%q", v.Enabled, v.Config["user"]) + } +} + +// A state file that does not parse must be preserved, not silently replaced by +// the next save — which used to take every other plugin's settings with it. +func TestCorruptStateFileIsPreservedNotOverwritten(t *testing.T) { + path := filepath.Join(t.TempDir(), "plugins.json") + good := `{"test-a":{"enabled":true,"config":{"user":"me"}},"test-b":{"enabled":true}}` + if err := os.WriteFile(path, []byte(good+"garbage"), 0o600); err != nil { + t.Fatal(err) + } + + m := NewManager(path) + err := m.Load() + if err == nil { + t.Fatal("Load should report an unreadable state file") + } + if !strings.Contains(err.Error(), ".corrupt") { + t.Fatalf("error should name the backup it made, got: %v", err) + } + + backup, readErr := os.ReadFile(path + ".corrupt") + if readErr != nil { + t.Fatalf("original state was not preserved: %v", readErr) + } + if !strings.Contains(string(backup), `"user":"me"`) { + t.Fatal("backup does not hold the original content") + } + + // The server keeps running; a later save must not touch the backup. + if _, err := m.Upsert(context.Background(), "test-a", true, nil); err != nil { + t.Fatal(err) + } + if again, _ := os.ReadFile(path + ".corrupt"); string(again) != string(backup) { + t.Fatal("backup was modified by a later save") + } +} + +// Enable/disable state must survive a restart — the whole point of the file. +func TestStateSurvivesReload(t *testing.T) { + path := filepath.Join(t.TempDir(), "plugins.json") + + m := NewManager(path) + if err := m.Load(); err != nil { + t.Fatal(err) + } + if _, err := m.Upsert(context.Background(), "test-a", true, map[string]string{"user": "me"}); err != nil { + t.Fatal(err) + } + + // Restart. + m2 := NewManager(path) + if err := m2.Load(); err != nil { + t.Fatalf("reload: %v", err) + } + v, ok := m2.Get("test-a") + if !ok || !v.Enabled || v.Config["user"] != "me" { + t.Fatalf("state lost across restart: ok=%v enabled=%v config=%v", ok, v.Enabled, v.Config) + } +} + +// persistLocked writes via a temp file + rename; no strays may be left behind. +func TestPersistLeavesNoTempFiles(t *testing.T) { + dir := t.TempDir() + m := NewManager(filepath.Join(dir, "plugins.json")) + if err := m.Load(); err != nil { + t.Fatal(err) + } + for i := 0; i < 3; i++ { + if _, err := m.Upsert(context.Background(), "test-a", true, nil); err != nil { + t.Fatal(err) + } + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Name() != "plugins.json" { + t.Fatalf("unexpected leftover file: %s", e.Name()) + } + } +} + +// CheckWritable is the boot-time probe that makes an unwritable volume visible. +func TestCheckWritable(t *testing.T) { + dir := t.TempDir() + if err := NewManager(filepath.Join(dir, "plugins.json")).CheckWritable(); err != nil { + t.Fatalf("writable directory reported as unwritable: %v", err) + } + err := NewManager(filepath.Join(dir, "missing-dir", "plugins.json")).CheckWritable() + if err == nil { + t.Fatal("missing directory should not report as writable") + } + if !IsPersist(err) { + t.Fatalf("expected a persist-classified error, got %v", err) + } +}