fix: seventeen devices from the poka-yoke audit (#705–#721)

cmd/polyemesis/main.go

@@ -20,14 +20,14 @@ import (
 	"github.com/rainmanjam/polyemesis/internal/api"
 	"github.com/rainmanjam/polyemesis/internal/automod"
 	"github.com/rainmanjam/polyemesis/internal/chat"
+	"github.com/rainmanjam/polyemesis/internal/childcensus"
 	"github.com/rainmanjam/polyemesis/internal/config"
 	"github.com/rainmanjam/polyemesis/internal/db"
 	"github.com/rainmanjam/polyemesis/internal/diag"
 	"github.com/rainmanjam/polyemesis/internal/engine"
 	"github.com/rainmanjam/polyemesis/internal/events"
 	"github.com/rainmanjam/polyemesis/internal/ffmpeg"
 	"github.com/rainmanjam/polyemesis/internal/logtz"
-	"github.com/rainmanjam/polyemesis/internal/supervisor"
 	// Aliased: main.go already has a `hooks` type for the service lifecycle
 	// callbacks, and that name is the older claim on it.
 	webhooks "github.com/rainmanjam/polyemesis/internal/hooks"
@@ -116,6 +116,13 @@ func run(h *hooks) error {
 		logLevel    = flag.String("log", "info", "log level: debug, info, warn, error")
 		showVersion = flag.Bool("version", false, "print the version and exit")
 		resetPass   = flag.Bool("reset-admin", false, "set a new admin password and sign out every session, then exit")
+		// #718. A password change ends SESSIONS; API tokens carry no epoch and
+		// survive it. The command now always says which tokens survive, and
+		// this is how an operator who has decided they are compromised ends
+		// them from a shell they can reach. Opt-in rather than implied: routine
+		// rotation is the common case and destroying every integration's
+		// credential is the wrong default for it.
+		resetRevoke = flag.Bool("revoke-api-tokens", false, "with -reset-admin, also delete every API token")
 		verifyBak   = flag.String("verify-backup", "", "check that a backup directory holds a database that opens, then exit")
 	)
 	flag.Parse()
@@ -183,7 +190,7 @@ func run(h *hooks) error {
 	}
 
 	if *resetPass {
-		return resetAdmin(cfg, os.Stdin, os.Stdout)
+		return resetAdmin(cfg, os.Stdin, os.Stdout, *resetRevoke)
 	}
 	// Text overlays need a font FILE, and the image polyemesis ships has no
 	// system fonts at all -- fontconfig is installed and finds nothing. The
@@ -489,7 +496,7 @@ func run(h *hooks) error {
 	srv.DrainLifecycleWithin(shutdownCtx)
 	eng.StopWithin(shutdownCtx)
 	warnIfShutdownOverran(shutdownCtx, log)
-	reportSurvivingChildren(log, supervisor.Live())
+	reportSurvivingChildren(log, childcensus.Live())
 	log.Info("goodbye")
 	return nil
 }
@@ -1022,8 +1029,24 @@ func verifyBackup(dir string, out io.Writer) error {
 // the whole budget rather than merely being slow.
 // Takes the census rather than reading it, so the reporting can be tested
 // without spawning a process to be reported ON.
-func reportSurvivingChildren(log *slog.Logger, live []supervisor.Child) {
+func reportSurvivingChildren(log *slog.Logger, live []childcensus.Child) {
 	if len(live) == 0 {
+		// SAYS SO, RATHER THAN SAYING NOTHING. #717.
+		//
+		// Silence here was read as an all-clear, and for most of this program's
+		// life it could not have been one: the census covered supervisor
+		// children only, so a transcode or a whisper child outliving the
+		// shutdown produced exactly the silence #631 produced -- while this
+		// function said nothing and the operator concluded the teardown was
+		// clean.
+		//
+		// It now covers every spawner that can outlive a call, and
+		// TestEverySpawnSiteIsAccountedFor is what keeps that true: a package
+		// that spawns without enrolling, and without a stated reason, fails the
+		// build. The line below is the claim; that test is what backs it.
+		log.Info("no child outlived the shutdown",
+			"scope", "every OS child this process spawned and enrolled: supervisor "+
+				"children, media transcodes, and the transcription and live-caption workers")
 		return
 	}
 	// Warn rather than Error: on a SIGKILLed child the census clears when the

cmd/polyemesis/resetadmin.go

@@ -37,7 +37,7 @@ import (
 // a security operation rather than a convenience. Someone resetting a forgotten
 // password may be locking an intruder out, and leaving that intruder's existing
 // session valid would defeat the whole exercise.
-func resetAdmin(cfg config.Config, in io.Reader, out io.Writer) error {
+func resetAdmin(cfg config.Config, in io.Reader, out io.Writer, revokeTokens bool) error {
 	store, err := db.Open(cfg.DBPath())
 	if err != nil {
 		return fmt.Errorf("open database: %w", err)
@@ -68,7 +68,48 @@ func resetAdmin(cfg config.Config, in io.Reader, out io.Writer) error {
 		return fmt.Errorf("password changed, but existing sessions could not be invalidated: %w", err)
 	}
 
+	// WHAT SURVIVES, ALWAYS, AND NOT ONLY WHEN IT IS EMPTY. #718.
+	//
+	// This printed "every existing session has been signed out" and stopped
+	// there. That sentence is true and incomplete: bumping the epoch ends
+	// SESSIONS, and API tokens are resolved by hash alone and carry no epoch, so
+	// they live on. An operator reaching for this command is usually locked out,
+	// which is the compromise case -- and this is the one path that could not
+	// tell them what still has access.
+	//
+	// The HTTP handler for the same gesture reads the surviving tokens back and
+	// discloses them. Mirroring that here is the device: the operator is told,
+	// every time, rather than left to assume. Deliberately NOT a forced revoke,
+	// for the reason that handler's own comment gives -- routine rotation is the
+	// common case, and destroying every integration's credential is the wrong
+	// default for it. --revoke-api-tokens is the explicit ask.
 	fmt.Fprintf(out, "password reset for %q; every existing session has been signed out\n", user.Username)
+
+	if revokeTokens {
+		n, rerr := store.DeleteAllAPITokens()
+		if rerr != nil {
+			fmt.Fprintf(out, "WARNING: the password changed, but the API tokens could not be\n"+
+				"  revoked (%v). They still work.\n", rerr)
+		} else {
+			fmt.Fprintf(out, "%d API token(s) revoked.\n", n)
+		}
+	}
+
+	tokens, terr := store.ListAPITokens()
+	switch {
+	case terr != nil:
+		fmt.Fprintf(out, "WARNING: could not read the API token list (%v). Tokens are NOT\n"+
+			"  ended by a password change, and this command could not tell you what survives.\n", terr)
+	case len(tokens) == 0:
+		fmt.Fprintln(out, "no API tokens exist, so nothing else can reach this install.")
+	default:
+		fmt.Fprintf(out, "\n%d API TOKEN(S) STILL WORK. A password change does not end them:\n", len(tokens))
+		for _, t := range tokens {
+			fmt.Fprintf(out, "  - %s (%s, created %s)\n", t.Name, t.Scope, t.CreatedAt.Format("2006-01-02"))
+		}
+		fmt.Fprintln(out, "Re-run with --revoke-api-tokens to delete them, or revoke them\n"+
+			"individually in Settings once you can sign in.")
+	}
 	return nil
 }
 

cmd/polyemesis/resetadmin_test.go

@@ -2,15 +2,50 @@ package main
 
 import (
 	"bytes"
+	"database/sql"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/rainmanjam/polyemesis/internal/config"
 	"github.com/rainmanjam/polyemesis/internal/db"
+
+	_ "modernc.org/sqlite"
 )
 
+// breakTheTokenTable makes both halves of the token disclosure fail, through a
+// connection of its own, in a way that SURVIVES db.Open.
+//
+// Dropping the table does not work: resetAdmin opens the database itself, and
+// Open runs the schema, which puts the table straight back. So this leaves the
+// schema exactly as it is and breaks the two operations instead --
+//
+//   - a BEFORE DELETE trigger that aborts, so DeleteAllAPITokens fails; and
+//   - a row whose created_at holds text, so ListAPITokens fails on Scan.
+//     SQLite is dynamically typed and stores it happily; the Go driver cannot
+//     hand it to an int64.
+//
+// The alternative was a fake store, which would have tested the fake.
+func breakTheTokenTable(t *testing.T, path string) {
+	t.Helper()
+	raw, err := sql.Open("sqlite", path)
+	if err != nil {
+		t.Fatalf("open raw: %v", err)
+	}
+	defer raw.Close()
+	for _, stmt := range []string{
+		`CREATE TRIGGER no_token_deletes BEFORE DELETE ON api_tokens
+		 BEGIN SELECT RAISE(ABORT, 'the token table refuses deletes'); END`,
+		`INSERT INTO api_tokens (name, prefix, token_hash, created_at, last_used_at, scope)
+		 VALUES ('unreadable', 'poly_xx', 'x', 'not-a-timestamp', 0, 'admin')`,
+	} {
+		if _, err := raw.Exec(stmt); err != nil {
+			t.Fatalf("breaking the token table: %v", err)
+		}
+	}
+}
+
 func resetFixture(t *testing.T) (config.Config, *db.DB) {
 	t.Helper()
 	dir := t.TempDir()
@@ -40,7 +75,7 @@ func TestResetAdminNeverLeavesTheInstallUnowned(t *testing.T) {
 
 	var out bytes.Buffer
 	in := strings.NewReader("a-brand-new-password\na-brand-new-password\n")
-	if err := resetAdmin(cfg, in, &out); err != nil {
+	if err := resetAdmin(cfg, in, &out, false); err != nil {
 		t.Fatalf("resetAdmin: %v", err)
 	}
 
@@ -72,7 +107,7 @@ func TestResetAdminChangesThePassword(t *testing.T) {
 	store.Close()
 
 	var out bytes.Buffer
-	if err := resetAdmin(cfg, strings.NewReader("a-brand-new-password\na-brand-new-password\n"), &out); err != nil {
+	if err := resetAdmin(cfg, strings.NewReader("a-brand-new-password\na-brand-new-password\n"), &out, false); err != nil {
 		t.Fatalf("resetAdmin: %v", err)
 	}
 
@@ -121,7 +156,7 @@ func TestResetAdminRefusals(t *testing.T) {
 			store.Close()
 
 			var out bytes.Buffer
-			err := resetAdmin(cfg, strings.NewReader(tc.input), &out)
+			err := resetAdmin(cfg, strings.NewReader(tc.input), &out, false)
 			if err == nil {
 				t.Fatalf("expected a refusal, got success")
 			}
@@ -163,3 +198,154 @@ func readSource(t *testing.T, name string) (string, error) {
 	b, err := os.ReadFile(filepath.Join(".", name))
 	return string(b), err
 }
+
+// A PASSWORD CHANGE DOES NOT END API TOKENS, AND THE OPERATOR IS TOLD SO. #718.
+//
+// This command is the one an operator reaches for when they cannot sign in,
+// which is the compromise case. It printed "every existing session has been
+// signed out" and stopped there -- true, and incomplete: bumping token_epoch
+// ends SESSIONS, while API tokens are resolved by hash alone, carry no epoch,
+// and live on. Nothing listed them, so the sentence read as "access has ended"
+// when it had not.
+//
+// The HTTP handler for the same gesture already reads the surviving tokens back
+// and discloses them. This pins that the CLI does too.
+func TestResetAdminNamesTheTokensThatSurviveIt(t *testing.T) {
+	cfg, store := resetFixture(t)
+	if _, err := store.CreateUser("admin", "the-old-password"); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+	if _, _, err := store.CreateAPIToken("ci-runner", string(db.ScopeAdmin)); err != nil {
+		t.Fatalf("create token: %v", err)
+	}
+	store.Close()
+
+	var out bytes.Buffer
+	in := strings.NewReader("a-brand-new-password\na-brand-new-password\n")
+	if err := resetAdmin(cfg, in, &out, false); err != nil {
+		t.Fatalf("resetAdmin: %v", err)
+	}
+
+	got := out.String()
+	if !strings.Contains(got, "ci-runner") {
+		t.Errorf("the surviving token is not named in the output, so an operator who "+
+			"has just locked out an intruder is not told what still reaches the "+
+			"install:\n%s", got)
+	}
+	if !strings.Contains(got, "STILL WORK") {
+		t.Errorf("the output does not say the tokens still work. Naming them is only "+
+			"half of it -- the sentence above them says sessions were signed out, and "+
+			"a list under that reads as a list of things that were ended:\n%s", got)
+	}
+	if !strings.Contains(got, "--revoke-api-tokens") {
+		t.Errorf("the output does not say how to end them. An operator told there is a "+
+			"problem and not told the remedy is worse off than one told nothing:\n%s", got)
+	}
+}
+
+// And the flag actually revokes, or the sentence above is advice that does not work.
+func TestResetAdminCanRevokeTheTokensItWarnsAbout(t *testing.T) {
+	cfg, store := resetFixture(t)
+	if _, err := store.CreateUser("admin", "the-old-password"); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+	if _, _, err := store.CreateAPIToken("ci-runner", string(db.ScopeAdmin)); err != nil {
+		t.Fatalf("create token: %v", err)
+	}
+	store.Close()
+
+	var out bytes.Buffer
+	in := strings.NewReader("a-brand-new-password\na-brand-new-password\n")
+	if err := resetAdmin(cfg, in, &out, true); err != nil {
+		t.Fatalf("resetAdmin: %v", err)
+	}
+
+	reopened, err := db.Open(cfg.DBPath())
+	if err != nil {
+		t.Fatalf("reopen: %v", err)
+	}
+	defer reopened.Close()
+	tokens, err := reopened.ListAPITokens()
+	if err != nil {
+		t.Fatalf("list: %v", err)
+	}
+	if len(tokens) != 0 {
+		t.Errorf("--revoke-api-tokens left %d token(s) in the database", len(tokens))
+	}
+	if got := out.String(); !strings.Contains(got, "1 API token(s) revoked") {
+		t.Errorf("the output does not report what was revoked:\n%s", got)
+	}
+}
+
+// THE DISCLOSURE MUST SURVIVE ITS OWN FAILURE. #718.
+//
+// The device is that an operator running this command is always told what still
+// reaches the install. Its two failure arms are the ones that matter most: they
+// fire exactly when the command cannot answer the question, and an operator who
+// is not told that silence means "unknown" will read it as "nothing".
+//
+// Reached by dropping the api_tokens table out from under the command, which is
+// the only way to make both the delete and the list fail without a fake store.
+// sqlite lets a second connection do that while resetAdmin holds its own.
+func TestResetAdminSaysSoWhenItCannotReadTheSurvivingTokens(t *testing.T) {
+	cfg, store := resetFixture(t)
+	if _, err := store.CreateUser("admin", "the-old-password"); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+	if _, _, err := store.CreateAPIToken("ci-runner", string(db.ScopeAdmin)); err != nil {
+		t.Fatalf("create token: %v", err)
+	}
+	store.Close()
+	breakTheTokenTable(t, cfg.DBPath())
+
+	var out bytes.Buffer
+	in := strings.NewReader("a-brand-new-password\na-brand-new-password\n")
+	// --revoke-api-tokens as well, so BOTH failure arms run in one command: the
+	// revoke that could not revoke, and the read-back that could not read.
+	if err := resetAdmin(cfg, in, &out, true); err != nil {
+		t.Fatalf("resetAdmin returned an error rather than reporting the trouble: %v", err)
+	}
+
+	got := out.String()
+	// The password change itself must still have happened and still be reported.
+	// A token table that will not read is not a reason to leave the operator
+	// locked out, which is the situation they ran this from.
+	if !strings.Contains(got, "password reset") {
+		t.Errorf("the password reset is not reported:\n%s", got)
+	}
+	if !strings.Contains(got, "could not be") || !strings.Contains(got, "still work") {
+		t.Errorf("a revoke that failed does not say the tokens still work. An "+
+			"operator who asked for a revoke and was not told it failed believes "+
+			"the credentials are dead:\n%s", got)
+	}
+	if !strings.Contains(got, "could not read the API token list") {
+		t.Errorf("a token list that could not be read is passed over in silence, "+
+			"which an operator reads as 'no tokens exist' -- the opposite of "+
+			"what is known:\n%s", got)
+	}
+	if !strings.Contains(got, "NOT") {
+		t.Errorf("the warning does not say tokens are not ended by a password "+
+			"change, which is the fact the whole disclosure exists to carry:\n%s", got)
+	}
+}
+
+// And the ordinary quiet case still says something rather than nothing: an
+// install with no tokens gets a sentence, not silence. Silence is what the two
+// arms above must be distinguishable from.
+func TestResetAdminSaysSoWhenNoTokensExist(t *testing.T) {
+	cfg, store := resetFixture(t)
+	if _, err := store.CreateUser("admin", "the-old-password"); err != nil {
+		t.Fatalf("create: %v", err)
+	}
+	store.Close()
+
+	var out bytes.Buffer
+	in := strings.NewReader("a-brand-new-password\na-brand-new-password\n")
+	if err := resetAdmin(cfg, in, &out, false); err != nil {
+		t.Fatalf("resetAdmin: %v", err)
+	}
+	if got := out.String(); !strings.Contains(got, "no API tokens exist") {
+		t.Errorf("an install with no tokens is told nothing, so it cannot be "+
+			"told apart from one whose token list could not be read:\n%s", got)
+	}
+}