diff --git a/web/handlers_settings.go b/web/handlers_settings.go index 846d90c441a7c6e5cfe3070c85ed3a970b913ae2..02576edc2c07696053b36d3fc1eccbb71e9f2334 100644 --- a/web/handlers_settings.go +++ b/web/handlers_settings.go @@ -21,6 +21,10 @@ // gate for settings. Login is required (anonymous → login redirect). A missing // repo, or a hidden PRIVATE repo the caller cannot even browse, is reported as // not found; a visible repo the caller does not own is a plain 403. On any of // these it writes the response and returns ok=false. +// +// The lookup is classified by repoLookupFailed, exactly as the browse path is: a +// settings URL that answered "no such database" while the metadata store was +// unreachable would be the same lie told on a different page. func (a *app) loadRepoForAdmin(w http.ResponseWriter, r *http.Request) (repo *core.Repo, ac *authContext, ok bool) { ac = a.requireLogin(w, r) if ac == nil { @@ -32,7 +36,7 @@ owner := chi.URLParam(r, "user") name := chi.URLParam(r, "db") repo, err := a.cfg.Repos.GetRepoByOwnerAndName(r.Context(), owner, name) if err != nil { - a.notFound(w, r) + a.repoLookupFailed(w, r, err) return nil, nil, false } diff --git a/web/router.go b/web/router.go index 179139ea9dae45c6649a436276797147dcd50798..74403c177d59203ef694009e72c1b8f53f9d7c1a 100644 --- a/web/router.go +++ b/web/router.go @@ -1,13 +1,17 @@ package web import ( + "errors" "fmt" "html/template" "io/fs" + "log/slog" "net/http" "os" "github.com/go-chi/chi/v5" + + "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-ecore/assets" "sourcecraft.dev/bigbes/sr-ht-ecore/chimw" @@ -20,6 +24,7 @@ "sourcecraft.dev/bigbes/sr-ht-dolt/authn" "sourcecraft.dev/bigbes/sr-ht-dolt/beads" "sourcecraft.dev/bigbes/sr-ht-dolt/core" + "sourcecraft.dev/bigbes/sr-ht-dolt/db" ) // serviceName is our own service key: the config section, the JWT audience and @@ -308,6 +313,18 @@ func (a *app) forbidden(w http.ResponseWriter, r *http.Request, msg string) { a.fail(w, r, http.StatusForbidden, msg) } +// internalError renders the shared 500 page and logs the cause. The split is the +// whole point: the reason a request could not be answered names hosts, queries +// and on-disk paths, so it goes to the operator's log, and the reader gets the +// same fixed sentence every other bug on this surface produces. +// +// scribe.Err rather than %v, as the newer handlers here do: it expands a culpa +// chain into err.msg, err.code and err.hint instead of flattening it. +func (a *app) internalError(w http.ResponseWriter, r *http.Request, what string, err error) { + slog.Error(what, "component", "web", "path", r.URL.Path, scribe.Err(err)) + a.fail(w, r, http.StatusInternalServerError, "") +} + // redirectLogin sends an unauthenticated caller to meta's login, returning them // to the current URL afterwards. // @@ -323,6 +340,9 @@ // enforces read (OpBrowse) authorization. On any denial it writes the response // (404 for hidden PRIVATE repos, 403 otherwise) and returns ok=false. On // success it returns the repo, the (possibly nil) caller and the caller's ACL // grant for reuse by the handler. +// +// "Not there" and "could not be looked up" are two answers and not one; see +// repoLookupFailed. func (a *app) loadRepoForBrowse(w http.ResponseWriter, r *http.Request) (repo *core.Repo, caller *core.Caller, aclMode *core.AccessMode, ok bool) { owner := chi.URLParam(r, "user") name := chi.URLParam(r, "db") @@ -331,8 +351,7 @@ _, caller = callerOf(r.Context()) repo, err := a.cfg.Repos.GetRepoByOwnerAndName(r.Context(), owner, name) if err != nil { - // A missing repo is reported as not found regardless of the caller. - a.notFound(w, r) + a.repoLookupFailed(w, r, err) return nil, nil, nil, false } @@ -346,6 +365,32 @@ } return nil, nil, nil, false } return repo, caller, aclMode, true +} + +// repoLookupFailed answers a failed {user}/{db} lookup, and is the one place +// this surface decides which of the two failures it was. +// +// - db.ErrNotFound is an answer about the data: there is no such row. It is a +// 404, and it has to stay the *same* 404 the visibility rule renders for a +// PRIVATE database the caller may not see (core.NotFoundForPrivate, in the +// callers below) — a database somebody may not look at must be +// indistinguishable from one that is not there, which is the whole reason +// the status was chosen. +// - Anything else is not an answer at all: the metadata store could not be +// read. Reporting that as a 404 tells a reader their database does not +// exist when the truth is that we cannot say right now, and it does it on +// every page of every database on the instance for as long as Postgres is +// down. It is a 500, with the cause logged and never rendered. +// +// The default arm is the 500 on purpose: an unmapped error is a bug in a layer +// below, and rendering it as "not found" would report that bug to the reader as +// a fact about their data. +func (a *app) repoLookupFailed(w http.ResponseWriter, r *http.Request, err error) { + if errors.Is(err, db.ErrNotFound) { + a.notFound(w, r) + return + } + a.internalError(w, r, "looking up a database failed", err) } // effectiveACL resolves the caller's ACL grant on repo, or nil for an anonymous diff --git a/web/web_test.go b/web/web_test.go index 08e4bd739856925cd6f83087854f7448acc6c197..6fa962c72c1f64851f3831506164f70834dd7fed 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -49,7 +49,12 @@ createErr error // listErr, when set, makes ListReposForViewer fail — the metadata store // unreachable, which the pages that enumerate databases have to survive. - listErr error + listErr error + // getErr, when set, makes GetRepoByOwnerAndName fail with it instead of + // answering — the metadata store unreachable rather than the row missing, + // which the pages that resolve one database must not confuse with a + // database that does not exist. + getErr error createdCalls []*core.Repo deletedRepos []int } @@ -87,6 +92,9 @@ return out, nil } func (f *fakeStore) GetRepoByOwnerAndName(_ context.Context, owner, name string) (*core.Repo, error) { + if f.getErr != nil { + return nil, f.getErr + } r, ok := f.repos[owner+"/"+name] if !ok { return nil, db.ErrNotFound @@ -471,6 +479,69 @@ rec := h.do("GET", "/~alice/sec", testCaller(7, "alice"), nil) if rec.Code != http.StatusOK { t.Fatalf("owner private overview: got %d, want 200", rec.Code) } +} + +// storeOutage is the shape of a metadata store that cannot answer: not a miss, +// and carrying a host and a port the reader has no business seeing. +var storeOutage = errors.New("dial tcp 10.0.0.5:5432: connect: connection refused") + +// A database whose metadata row cannot be *read* is not a database that does not +// exist. Reporting the outage as a 404 tells every reader on the instance that +// their database is gone for as long as Postgres is down — and, being a 404 with +// the shared sentence, tells them so in the voice reserved for "there is nothing +// here". +func TestAStoreOutageIsNotAMissingDatabase(t *testing.T) { + for _, tc := range []struct { + name, target string + caller *auth.AuthContext + }{ + {"overview", "/~alice/db", nil}, + {"log", "/~alice/db/log", nil}, + {"tree", "/~alice/db/tree/main", nil}, + {"table", "/~alice/db/table/main/things", nil}, + {"view", "/~alice/db/view/beads", nil}, + // The admin path resolves the same row through loadRepoForAdmin and must + // classify it the same way. + {"settings", "/~alice/db/settings", testCaller(1, "alice")}, + } { + t.Run(tc.name, func(t *testing.T) { + h := newHarness(t) + h.store.add(&core.Repo{Name: "db", OwnerID: 1, OwnerName: "alice", Path: "/d", Visibility: core.VisibilityPublic}) + h.store.getErr = storeOutage + + rec := h.do("GET", tc.target, tc.caller, nil) + require.Equal(t, http.StatusInternalServerError, rec.Code) + + body := rec.Body.String() + assert.Contains(t, body, pages.InternalMessage) + assert.NotContains(t, body, "connection refused", "the cause belongs in the log") + assert.NotContains(t, body, "10.0.0.5:5432") + assert.NotContains(t, body, pages.NotFoundMessage) + }) + } +} + +// The other arm, and the one the masking rule rests on: the store's own "no such +// row" — including a wrapped one, which is how db/ hands it up — is still the +// 404, and still the *same* 404 a PRIVATE database the caller may not see gets. +func TestAMissingDatabaseIsStillNotFound(t *testing.T) { + h := newHarness(t) + h.store.add(&core.Repo{Name: "sec", OwnerID: 1, OwnerName: "alice", Path: "/s", Visibility: core.VisibilityPrivate}) + + // The masked PRIVATE database, and the same URL with no row behind it at + // all. One URL for both so the two responses are comparable byte for byte: + // the shared error page carries the request's own path in the login link. + hidden := h.do("GET", "/~alice/sec", nil, nil) + require.Equal(t, http.StatusNotFound, hidden.Code) + assert.Contains(t, hidden.Body.String(), pages.NotFoundMessage) + + // A miss the store wrapped on its way up, which errors.Is must still see + // through — db/ wraps its misses with the owner and name it looked up. + h.store.getErr = fmt.Errorf("db: get repo ~alice/sec: %w", db.ErrNotFound) + missing := h.do("GET", "/~alice/sec", nil, nil) + require.Equal(t, http.StatusNotFound, missing.Code) + assert.Equal(t, hidden.Body.String(), missing.Body.String(), + "a database somebody may not see and one that is not there must render the same page") } func TestDashboardLists(t *testing.T) {