diff --git a/server/deploy.sh b/server/deploy.sh index 9874788..1f6ef76 100755 --- a/server/deploy.sh +++ b/server/deploy.sh @@ -58,7 +58,19 @@ git rev-parse HEAD | "${SSH[@]}" "cat > $REMOTE_DIR/release/$VERSION/GIT_SHA" if [ "${DRY_RUN:-}" != "" ]; then echo "DRY_RUN: shipped to $REMOTE_DIR/release/$VERSION, nothing changed"; exit 0; fi step "3. Back up the database" -"${SSH[@]}" "docker exec behavision-db sh -c 'PGPASSWORD=\$POSTGRES_PASSWORD pg_dump -U behavision -d behavision' | gzip > $REMOTE_DIR/backups/pre-$VERSION-\$(date +%Y%m%d-%H%M%S).sql.gz && ls -la $REMOTE_DIR/backups | tail -1" +# The dump's own filename is echoed, not `ls | tail -1`, which reported the +# WRONG file: ls sorts alphabetically, so pre-...-demo-12-... sorts before +# pre-...-demo-6-... and the line printed a backup from four days earlier. A +# deploy that names the wrong safety net is worse than one that names none - +# that is the file somebody reaches for at the worst possible moment. +# +# Bare `-s` on the dump so an empty or failed one cannot be reported as a +# backup: pg_dump exiting non-zero already fails the pipeline under pipefail, +# but a zero-byte gzip would still satisfy it. +"${SSH[@]}" "set -e; f=$REMOTE_DIR/backups/pre-$VERSION-\$(date +%Y%m%d-%H%M%S).sql.gz; \ + docker exec behavision-db sh -c 'PGPASSWORD=\$POSTGRES_PASSWORD pg_dump -U behavision -d behavision' | gzip > \$f; \ + [ -s \$f ] || { echo 'backup is empty - refusing to continue' >&2; exit 1; }; \ + ls -la \$f" step "4. Image" "${SSH[@]}" "cd $REMOTE_DIR/release/$VERSION && docker build -q -t behavision-backend:$VERSION -f Dockerfile.runtime . && docker tag behavision-backend:$VERSION behavision-backend:latest" diff --git a/server/internal/api/handlers_admin_monitor.go b/server/internal/api/handlers_admin_monitor.go index 888c1a4..072293f 100644 --- a/server/internal/api/handlers_admin_monitor.go +++ b/server/internal/api/handlers_admin_monitor.go @@ -22,7 +22,7 @@ import "net/http" // precisely what an admin opens the console to look at, and hiding it would // make the one screen that can fix it the one screen that cannot see it. func (s *Server) clientForAdmin(w http.ResponseWriter, r *http.Request) (ClientDetail, bool) { - c, err := s.Store.ClientDetail(r.Context(), pathUUID(r, "id")) + c, err := s.Store.ClientDetail(r.Context(), r.PathValue("id")) if err != nil { s.serverError(w, "admin client", err) return ClientDetail{}, false @@ -179,15 +179,3 @@ func (s *Server) auditAdminRead(r *http.Request, clientID, action, entity, entit Detail: map[string]any{"path": r.URL.Path, "rows": n}, }) } - -// pathUUID reads a path segment that must be a uuid, returning "" otherwise so -// the lookup misses and the caller answers 404. Handing a malformed string to -// Postgres as a uuid is an error, not a miss, and would surface as a 500 on -// what is really just a wrong URL. -func pathUUID(r *http.Request, name string) string { - v := r.PathValue(name) - if !looksLikeUUID(v) { - return "" - } - return v -} diff --git a/server/internal/store/api_admin_monitor.go b/server/internal/store/api_admin_monitor.go index 3a33b7b..0ccfd89 100644 --- a/server/internal/store/api_admin_monitor.go +++ b/server/internal/store/api_admin_monitor.go @@ -22,6 +22,15 @@ import ( // ClientDetail is one merchant, with the owner a support conversation starts // from. ListClients cannot carry it: an owner lookup per row would be a query // per merchant on a screen that only needs the name. +// +// `c.id::text = $1`, for the same reason the two resolvers below use it, and +// this one learned it the hard way: as `c.id = $1::uuid` it answered 500 to +// /api/admin/clients/not-a-uuid/sites on the first real database, because +// casting a malformed string - or the empty one a shape check hands back - to +// uuid is an ERROR in Postgres rather than a miss. Comparing the column as +// text cannot fail: an id that is not a uuid simply matches nothing, which is +// the 404 a wrong URL should get. The sibling queries were already written +// this way and correctly 404'd; only this one was not. func (s *Store) ClientDetail(ctx context.Context, clientID string) (api.ClientDetail, error) { var c api.ClientDetail var at time.Time @@ -36,7 +45,7 @@ func (s *Store) ClientDetail(ctx context.Context, clientID string) (api.ClientDe WHERE au.client_id = c.id AND au.role = 'owner' AND au.active ORDER BY au.created_at LIMIT 1), '') FROM clients c - WHERE c.id = $1::uuid`, clientID). + WHERE c.id::text = $1`, clientID). Scan(&c.ID, &c.Slug, &c.Name, &c.Active, &at, &c.Sites, &c.Users, &c.OwnerEmail, &c.OwnerName) if errors.Is(err, pgx.ErrNoRows) { diff --git a/server/internal/store/api_admin_monitor_live_test.go b/server/internal/store/api_admin_monitor_live_test.go new file mode 100644 index 0000000..47ca749 --- /dev/null +++ b/server/internal/store/api_admin_monitor_live_test.go @@ -0,0 +1,57 @@ +package store + +import ( + "context" + "testing" +) + +// A malformed identifier must MISS, never error. +// +// This exists because the in-memory fake cannot catch it and did not. The API +// fake resolves a merchant with a map lookup, so every handler test passed +// while the real query answered 500 to /api/admin/clients/not-a-uuid/sites: +// `c.id = $1::uuid` makes Postgres cast the path segment, and casting a +// malformed string - or the empty one a shape check hands back in its place - +// is an ERROR rather than no match. Comparing the column as text cannot fail. +// +// Kept as a live test on purpose. There is no way to assert this against a +// fake: the property belongs to Postgres, and a fake that reproduced it would +// be a second implementation of the thing under test. +func TestLiveMalformedIdentifiersMissRatherThanError(t *testing.T) { + st := liveStore(t) + ctx := context.Background() + + for _, id := range []string{"", "not-a-uuid", "'; DROP TABLE clients;--", "42"} { + c, err := st.ClientDetail(ctx, id) + if err != nil { + t.Errorf("ClientDetail(%q): %v - a wrong URL must be a miss, not a 500", id, err) + } + if c.ID != "" { + t.Errorf("ClientDetail(%q) matched %q", id, c.ID) + } + } + + // The sibling resolvers take a real client id and a free-text reference, so + // the reference is the part a caller controls and the part that must not + // blow up. A malformed CLIENT id here is covered above. + const noClient = "00000000-0000-4000-8000-000000000000" + for _, ref := range []string{"", "not-a-uuid", "'; --", "42"} { + if id, err := st.AdminSiteID(ctx, noClient, ref); err != nil { + t.Errorf("AdminSiteID(%q): %v", ref, err) + } else if id != "" { + t.Errorf("AdminSiteID(%q) matched %q", ref, id) + } + if id, err := st.AdminCameraID(ctx, noClient, noClient, ref); err != nil { + t.Errorf("AdminCameraID(%q): %v", ref, err) + } else if id != "" { + t.Errorf("AdminCameraID(%q) matched %q", ref, id) + } + } + + // And a sale id is the same shape of input on the tenant side. + if s, err := st.Sale(ctx, noClient, "not-a-uuid"); err != nil { + t.Errorf("Sale(not-a-uuid): %v", err) + } else if s.ID != "" { + t.Errorf("Sale(not-a-uuid) matched %q", s.ID) + } +}