~bigbes/sr-ht-dolt

35875ff76df1efe232d436a3041a8e621cdab418 — Eugene Blikh 5 days ago 11c622f
web: stop printing store paths to the owner
2 files changed, 71 insertions(+), 4 deletions(-)

M web/handlers_settings.go
M web/web_test.go
M web/handlers_settings.go => web/handlers_settings.go +15 -2
@@ 2,6 2,7 @@ package web

import (
	"errors"
	"log/slog"
	"net/http"
	"net/url"
	"strconv"


@@ 9,6 10,8 @@ import (

	"github.com/go-chi/chi/v5"

	"go.bigb.es/auxilia/scribe"

	"sourcecraft.dev/bigbes/sr-ht-ecore/chrome"
	"sourcecraft.dev/bigbes/sr-ht-ecore/pages"



@@ 198,12 201,22 @@ func (a *app) settingsDelete(w http.ResponseWriter, r *http.Request, repo *core.
		return
	}
	if err := a.cfg.Stores.DeleteStore(r.Context(), a.cfg.ReposRoot, repo.Path); err != nil {
		http.Error(w, "database record removed but store deletion failed: "+err.Error(),
		// The store-layer error names the on-disk path, which nothing else on
		// this surface discloses. The reader keeps the fact that matters to
		// them — the record is gone but the store may still be on disk — and
		// the detail goes to the log against the database id.
		slog.Error("deleting a database's on-disk store failed after the record was removed",
			"component", "web", "database", repo.ID, scribe.Err(err))
		http.Error(w, "The database record was removed, but the on-disk store could not be deleted. Contact support.",
			http.StatusInternalServerError)
		return
	}
	if err := a.cfg.Stores.Evict(repo.Path); err != nil {
		http.Error(w, "store deleted but cache eviction failed: "+err.Error(),
		// Same split: the store is gone, but the cached handle may survive it
		// — a different fact from the one above, kept distinct on purpose.
		slog.Error("evicting a database's cached store handle failed after the store was deleted",
			"component", "web", "database", repo.ID, scribe.Err(err))
		http.Error(w, "The on-disk store was deleted, but the cached handle could not be evicted. Contact support.",
			http.StatusInternalServerError)
		return
	}

M web/web_test.go => web/web_test.go +56 -2
@@ 247,6 247,8 @@ func (f *fakeStore) DeleteKey(_ context.Context, id, userID int) error {

type fakeStoreManager struct {
	initErr     error
	deleteErr   error
	evictErr    error
	initCalls   []string
	deleteCalls []string
	evictCalls  []string


@@ 258,11 260,11 @@ func (m *fakeStoreManager) InitStore(_ context.Context, absPath, _, _ string) er
}
func (m *fakeStoreManager) DeleteStore(_ context.Context, _, absPath string) error {
	m.deleteCalls = append(m.deleteCalls, absPath)
	return nil
	return m.deleteErr
}
func (m *fakeStoreManager) Evict(diskPath string) error {
	m.evictCalls = append(m.evictCalls, diskPath)
	return nil
	return m.evictErr
}

type fakeSession struct {


@@ 851,6 853,58 @@ func TestSettingsUpdateAndDelete(t *testing.T) {
	}
}

// storeDetail is a store-layer failure of the shape DeleteStore/Evict really
// produce: the on-disk path underneath the store.
const storeDetail = "unlink /var/lib/dolt/~owner/db/.dolt/noms/oldgen: permission denied"

// The settings delete path used to render the store layer's own error text
// into the response, under "database record removed but store deletion
// failed: " and "store deleted but cache eviction failed: " — both carrying
// the store's path on disk, which nothing else on this surface discloses.
//
// The two failures are still told apart: one means the row is gone but the
// store may still be on disk, the other that the store is gone but a cached
// handle may survive it. The path just no longer rides along.
func TestSettingsDeleteDoesNotPrintTheStoreError(t *testing.T) {
	newDeleteHarness := func(t *testing.T) (*harness, *auth.AuthContext) {
		t.Helper()
		h := newHarness(t)
		h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner",
			Path: "/var/lib/dolt/~owner/db", Visibility: core.VisibilityPublic})
		return h, testCaller(10, "owner")
	}
	assertHidden := func(t *testing.T, body string) {
		t.Helper()
		assert.NotContains(t, body, storeDetail)
		assert.NotContains(t, body, "/var/lib/dolt", "the store's path must not reach the reader")
		assert.NotContains(t, body, "permission denied", "the underlying OS error must not reach the reader")
	}

	t.Run("store deletion fails", func(t *testing.T) {
		h, owner := newDeleteHarness(t)
		h.stores.deleteErr = errors.New(storeDetail)

		rec := h.do("POST", "/~owner/db/settings", owner, url.Values{"action": {"delete"}, "confirm_name": {"db"}})
		require.Equal(t, http.StatusInternalServerError, rec.Code)
		body := rec.Body.String()
		assertHidden(t, body)
		assert.Contains(t, body, "The database record was removed, but the on-disk store could not be deleted.")
		// Eviction must not be attempted once the store deletion itself failed.
		assert.Empty(t, h.stores.evictCalls)
	})

	t.Run("cache eviction fails", func(t *testing.T) {
		h, owner := newDeleteHarness(t)
		h.stores.evictErr = errors.New(storeDetail)

		rec := h.do("POST", "/~owner/db/settings", owner, url.Values{"action": {"delete"}, "confirm_name": {"db"}})
		require.Equal(t, http.StatusInternalServerError, rec.Code)
		body := rec.Body.String()
		assertHidden(t, body)
		assert.Contains(t, body, "The on-disk store was deleted, but the cached handle could not be evicted.")
	})
}

func TestSettingsACLAddRemove(t *testing.T) {
	h := newHarness(t)
	repo := h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", Path: "/d", Visibility: core.VisibilityPublic})