amadan

carlos / platform Public

fix(cli): headless trash validates its target and proves its gate order

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell a3ebe38efde2dd517eccfc40defe5d85dcf20e9c parent d23c4ab
2 files changed, +249 −38
  • apps.go +96 −38
  • apps_test.go +153 −0
diff --git a/apps.go b/apps.go
index e963fb353..a2a1a37e4 100644
--- a/apps.go
+++ b/apps.go
@@ -17,6 +17,7 @@ package main
import (
"context"
+ "crypto/ed25519"
"errors"
"flag"
"fmt"
@@ -28,16 +29,17 @@ import (
"github.com/carlosframework/platform/internal/release"
)
-const appsUsage = `usage: carlos apps <delete|restore> <app> [flags]
+const appsUsage = `usage: carlos apps <delete|restore> [flags] <app>
- delete <app> move an app to the Trash: every route drops (custom
- domains included), its name stays reserved, and it is
- restorable for 30 days. Refused while the app is flagged
- production — turn production off first, then delete — or
- when that flag cannot be read at all: an unreadable flag
- is treated as production, same direction "bake" fails.
+ delete [flags] <app> move an app to the Trash: every route drops
+ (custom domains included), its name stays reserved, and it
+ is restorable for 30 days. Refused while the app is
+ flagged production — turn production off first, then
+ delete — or when that flag cannot be read at all: an
+ unreadable flag is treated as production, same direction
+ "bake" fails.
- restore <app> undo a delete within the 30-day window. Releases,
+ restore [flags] <app> undo a delete within the 30-day window. Releases,
channels and the production flag are exactly as they were
— but NO routes come back, not even the platform apex:
run "carlos add <host>" afterwards to serve it again.
@@ -83,6 +85,27 @@ func RunApps(args []string) {
}
}
+// validAppName is release.ValidApp's CLI-facing gate, checked BEFORE mode
+// selection in both verbs (review finding: the -account-id "ops" phantom
+// trap, one level down). Neither TrashApp nor RestoreApp validates their app
+// argument — they trust the caller — and the signed-in path's 404
+// (apiAccountApp's HasAppIn) only catches a bad name because the CONSOLE
+// happens to keep a row per real app. Headless mode has no such row: nothing
+// stops `carlos apps delete -account-id bab MyApp` from writing
+// apps/bab/MyApp/deleted.json — a path release.ValidApp accepts as a bucket
+// key but that no real app can ever have (Ship/SetProduction both require
+// ValidApp, so a real app's own name always passes) — printing "trashed"
+// while the actual "myapp" keeps serving, untouched, forever. Refusing here,
+// before either mode runs, keeps both paths refusing the same typo instead
+// of only the lucky one.
+func validAppName(app string) error {
+ if release.ValidApp(app) {
+ return nil
+ }
+ return fmt.Errorf("%q is not a usable app name — 1-32 characters, lowercase a-z, 0-9 and hyphen, "+
+ "starting with a letter", app)
+}
+
// appsMode is the pure decision behind both apps verbs' mode selection,
// extracted so it can be table-tested without a credentials file or a
// bucket on disk. loggedIn and bucketConfigured come straight off
@@ -124,9 +147,12 @@ func runAppsDelete(args []string) {
consoleFlag := fs.String("console", "", "console origin to delete through (signed-in mode only; default: the only logged-in one)")
fs.Parse(args)
if fs.NArg() != 1 {
- log.Fatal("usage: carlos apps delete <app> [-account <sqid|name>] [-account-id <sqid>] [-console <origin>]")
+ log.Fatal("usage: carlos apps delete [-account <sqid|name>] [-account-id <sqid>] [-console <origin>] <app>")
}
app := fs.Arg(0)
+ if err := validAppName(app); err != nil {
+ log.Fatalf("apps delete: %v", err)
+ }
acctID := checkAccountID(*acctIDRaw)
sess, err := resolveMode(*consoleFlag)
@@ -150,47 +176,76 @@ func runAppsDelete(args []string) {
}
// Headless: this box has no console to enforce the production gate, so
- // this verb enforces it itself, the same direction and the same
- // wording as handleAPITrashApp — an unreadable flag is treated as
- // production and refused, never as "probably fine".
+ // headlessTrash enforces it itself — pulled out into its own function
+ // (review finding: the destructive gate order needs direct test
+ // coverage, not only exercise-through-the-CLI).
store := openStore()
- pubRaw := strings.TrimSpace(os.Getenv("CARLOS_RELEASE_PUBKEY"))
- if pubRaw == "" {
- log.Fatal("apps delete (headless): CARLOS_RELEASE_PUBKEY is not set — deleting must be able to prove the " +
- "app is not flagged production, and a flag that cannot be proven is treated as production and refused")
+ var pub ed25519.PublicKey
+ if pubRaw := strings.TrimSpace(os.Getenv("CARLOS_RELEASE_PUBKEY")); pubRaw != "" {
+ p, perr := release.ParsePublicKey(pubRaw, "CARLOS_RELEASE_PUBKEY")
+ if perr != nil {
+ log.Fatalf("apps delete: %v", perr)
+ }
+ pub = p
}
- pub, err := release.ParsePublicKey(pubRaw, "CARLOS_RELEASE_PUBKEY")
+ restorableUntil, err := headlessTrash(context.Background(), store, pub, acctID, app, time.Now())
if err != nil {
log.Fatalf("apps delete: %v", err)
}
- ctx := context.Background()
- prod, err := release.GetProduction(ctx, store, pub, acctID, app)
+ printTrashed(app, restorableUntil.UTC().Format(time.RFC3339))
+}
+
+// headlessTrash is the destructive headless-mode gate: no console in the
+// loop to enforce anything, so this is the whole check, pulled out of
+// runAppsDelete so it can be driven directly against a real store in tests
+// rather than only through the CLI's os.Exit-on-refusal plumbing (review
+// finding — this gate had zero test coverage). Gate order matches
+// handleAPITrashApp's (internal/console/apicli.go) exactly:
+//
+// - a malshaped app name refuses before anything else (validAppName) —
+// defense in depth, since runAppsDelete already checks this before mode
+// selection, but this function must not trust its caller either.
+// - no usable public key refuses before the store is even asked anything:
+// with nothing to verify a signature against, an unreadable flag and an
+// absent one are indistinguishable, so this is the same "unreadable
+// fails toward refusal" rule one step earlier.
+// - the production flag ON, or genuinely UNREADABLE, refuses — NOTHING is
+// written on this path.
+// - only then TrashApp, followed by the SAME re-read handleAPITrashApp
+// does: TrashApp is create-only, so an app already in the trash (a
+// racing headless caller, or a previous run) keeps its ORIGINAL clock,
+// and printing this call's own `now` would over-promise a deadline
+// nothing will honour.
+func headlessTrash(ctx context.Context, store release.Store, pub ed25519.PublicKey, acct, app string, now time.Time) (time.Time, error) {
+ if err := validAppName(app); err != nil {
+ return time.Time{}, err
+ }
+ if len(pub) != ed25519.PublicKeySize {
+ return time.Time{}, errors.New("CARLOS_RELEASE_PUBKEY is not set — deleting must be able to prove the app " +
+ "is not flagged production, and a flag that cannot be proven is treated as production and refused")
+ }
+ prod, err := release.GetProduction(ctx, store, pub, acct, app)
if err != nil {
- log.Fatalf("apps delete: could not read the production flag for %s/%s — an unreadable flag is treated as "+
- "production, refusing: %v", acctID, app, err)
+ return time.Time{}, fmt.Errorf("could not read the production flag for %s/%s — an unreadable flag is "+
+ "treated as production, refusing: %w", acct, app, err)
}
if prod != nil && prod.Production {
- log.Fatalf("apps delete: %s/%s is flagged production, so deleting is refused — turn production off first, "+
- "then try again", acctID, app)
+ return time.Time{}, fmt.Errorf("%s/%s is flagged production, so deleting is refused — turn production off "+
+ "first, then try again", acct, app)
}
- if err := release.TrashApp(ctx, store, acctID, app, defaultLabel(), time.Now()); err != nil {
- log.Fatalf("apps delete: %v", err)
+ if err := release.TrashApp(ctx, store, acct, app, defaultLabel(), now); err != nil {
+ return time.Time{}, err
}
- // Re-read for the SAME reason handleAPITrashApp does: TrashApp is
- // create-only, so an app already in the trash (trashed by a racing
- // headless caller, or previously) keeps its ORIGINAL clock — the marker
- // is the only clock this feature has, and printing this call's own
- // time would over-promise a deadline nothing will honour.
- tomb, terr := release.ReadAppTombstone(ctx, store, acctID, app)
- if terr != nil {
- log.Fatalf("apps delete: %s/%s is trashed, but the restore window could not be confirmed — retry is safe "+
- "(the marker is create-only): %v", acctID, app, terr)
+ tomb, err := release.ReadAppTombstone(ctx, store, acct, app)
+ if err != nil {
+ return time.Time{}, fmt.Errorf("%s/%s is trashed, but the restore window could not be confirmed — retry is "+
+ "safe (the marker is create-only): %w", acct, app, err)
}
if tomb == nil {
- log.Fatalf("apps delete: %s/%s is trashed, but its marker vanished before the window could be confirmed "+
- "(a restore may have raced this) — retry is safe", acctID, app)
+ return time.Time{}, fmt.Errorf("%s/%s is trashed, but its marker vanished before the window could be "+
+ "confirmed (a restore may have raced this) — retry is safe", acct, app)
}
- printTrashed(app, tomb.DeletedAt.Add(release.TrashCooloff).UTC().Format(time.RFC3339))
+ return tomb.DeletedAt.Add(release.TrashCooloff), nil
}
// printTrashed is the one shape both delete paths print in, so signed-in and
@@ -211,9 +266,12 @@ func runAppsRestore(args []string) {
consoleFlag := fs.String("console", "", "console origin to restore through (signed-in mode only; default: the only logged-in one)")
fs.Parse(args)
if fs.NArg() != 1 {
- log.Fatal("usage: carlos apps restore <app> [-account <sqid|name>] [-account-id <sqid>] [-console <origin>]")
+ log.Fatal("usage: carlos apps restore [-account <sqid|name>] [-account-id <sqid>] [-console <origin>] <app>")
}
app := fs.Arg(0)
+ if err := validAppName(app); err != nil {
+ log.Fatalf("apps restore: %v", err)
+ }
acctID := checkAccountID(*acctIDRaw)
sess, err := resolveMode(*consoleFlag)
diff --git a/apps_test.go b/apps_test.go
index e9ba50318..16dabce61 100644
--- a/apps_test.go
+++ b/apps_test.go
@@ -7,11 +7,16 @@ package main
import (
"context"
+ "crypto/ed25519"
"encoding/json"
+ "errors"
"net/http"
"net/http/httptest"
"strings"
"testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/release"
)
// TestAppsMode is Step 1's failing-test-first table: signed-in wins
@@ -215,3 +220,151 @@ func TestAPISessionRestoreAppServerError(t *testing.T) {
t.Fatalf("restoreApp against a 502 = %v, want the server's own text", err)
}
}
+
+// ---- review round: app-name validation and the headless trash gate --------
+
+// TestValidAppNameRejectsWhatReleaseWouldAccidentallyMint pins the review
+// finding: MyApp is not release.ValidApp (uppercase), but nothing about the
+// bucket key shape itself would stop `apps/<sqid>/MyApp/deleted.json` from
+// being written — the real app "myapp" would keep serving, untouched, while
+// the CLI printed success for a name that can never be a real app.
+func TestValidAppNameRejectsWhatReleaseWouldAccidentallyMint(t *testing.T) {
+ for _, bad := range []string{"MyApp", "", "-leadinghyphen", "has spaces", strings.Repeat("a", 33)} {
+ if err := validAppName(bad); err == nil {
+ t.Errorf("validAppName(%q) = nil, want an error", bad)
+ }
+ }
+ for _, good := range []string{"myapp", "a", "my-app-2", strings.Repeat("a", 32)} {
+ if err := validAppName(good); err != nil {
+ t.Errorf("validAppName(%q) = %v, want nil", good, err)
+ }
+ }
+}
+
+// newHeadlessTestStore returns a fresh DirStore plus a parsed keypair
+// (release.GenerateKey base64-encoded, parsed the same way the CLI parses
+// CARLOS_RELEASE_PUBKEY/CARLOS_RELEASE_KEY) for headlessTrash's direct tests.
+func newHeadlessTestStore(t *testing.T) (release.Store, ed25519.PrivateKey, ed25519.PublicKey) {
+ t.Helper()
+ seed, pubB64, err := release.GenerateKey()
+ if err != nil {
+ t.Fatal(err)
+ }
+ priv, err := release.ParseSigningKey(seed, "CARLOS_RELEASE_KEY")
+ if err != nil {
+ t.Fatal(err)
+ }
+ pub, err := release.ParsePublicKey(pubB64, "CARLOS_RELEASE_PUBKEY")
+ if err != nil {
+ t.Fatal(err)
+ }
+ return release.NewDirStore(t.TempDir()), priv, pub
+}
+
+// TestHeadlessTrashNoKeyRefusesBeforeAnyWrite pins gate order: an absent (or
+// too-short) public key refuses before the store is asked anything at all,
+// and nothing is written.
+func TestHeadlessTrashNoKeyRefusesBeforeAnyWrite(t *testing.T) {
+ store, _, _ := newHeadlessTestStore(t)
+ _, err := headlessTrash(context.Background(), store, nil, "bab", "myapp", time.Now())
+ if err == nil || !strings.Contains(err.Error(), "CARLOS_RELEASE_PUBKEY") {
+ t.Fatalf("headlessTrash with no key = %v, want it to name CARLOS_RELEASE_PUBKEY", err)
+ }
+ if tomb, terr := release.ReadAppTombstone(context.Background(), store, "bab", "myapp"); terr != nil || tomb != nil {
+ t.Fatalf("no key must not write a marker: tomb=%+v err=%v", tomb, terr)
+ }
+}
+
+// TestHeadlessTrashInvalidAppRefuses ties this gate to the app-name-shape
+// finding: an unusable app name refuses even in headless mode, before the
+// store is touched.
+func TestHeadlessTrashInvalidAppRefuses(t *testing.T) {
+ store, _, pub := newHeadlessTestStore(t)
+ _, err := headlessTrash(context.Background(), store, pub, "bab", "MyApp", time.Now())
+ if err == nil || !strings.Contains(err.Error(), "not a usable app name") {
+ t.Fatalf("headlessTrash(%q) = %v, want the app-name refusal", "MyApp", err)
+ }
+}
+
+// TestHeadlessTrashProductionOnRefusesNoWrite pins the production-ON gate:
+// refuses, and writes NOTHING — the whole point of checking before TrashApp.
+func TestHeadlessTrashProductionOnRefusesNoWrite(t *testing.T) {
+ store, priv, pub := newHeadlessTestStore(t)
+ ctx := context.Background()
+ if err := release.SetProduction(ctx, store, priv, "bab", "myapp", true, "paul@keymail.dev"); err != nil {
+ t.Fatal(err)
+ }
+ _, err := headlessTrash(ctx, store, pub, "bab", "myapp", time.Now())
+ if err == nil || !strings.Contains(err.Error(), "flagged production") {
+ t.Fatalf("headlessTrash against a production app = %v, want the production refusal", err)
+ }
+ if tomb, terr := release.ReadAppTombstone(ctx, store, "bab", "myapp"); terr != nil || tomb != nil {
+ t.Fatalf("production-ON must not write a marker: tomb=%+v err=%v", tomb, terr)
+ }
+}
+
+// unreadableProductionStore wraps a release.Store and fails every Get for
+// one app's production.json — the "flag exists but cannot be verified" case
+// production.go's GetProduction turns into an error, which headlessTrash
+// must treat exactly like flag-ON (refuse, write nothing), never like
+// flag-absent.
+type unreadableProductionStore struct {
+ release.Store
+ acct, app string
+}
+
+func (u unreadableProductionStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if key == release.ProductionKey(u.acct, u.app) {
+ return nil, "", errUnreadableProduction
+ }
+ return u.Store.Get(ctx, key)
+}
+
+var errUnreadableProduction = errors.New("simulated unreadable production flag")
+
+// TestHeadlessTrashProductionUnreadableRefusesNoWrite pins the "unreadable
+// fails toward refusal" rule (spec §5): a production flag this call cannot
+// read is treated as production, not as absent, and nothing is written.
+func TestHeadlessTrashProductionUnreadableRefusesNoWrite(t *testing.T) {
+ store, _, pub := newHeadlessTestStore(t)
+ wrapped := unreadableProductionStore{Store: store, acct: "bab", app: "myapp"}
+ ctx := context.Background()
+ _, err := headlessTrash(ctx, wrapped, pub, "bab", "myapp", time.Now())
+ if err == nil || !strings.Contains(err.Error(), "unreadable") {
+ t.Fatalf("headlessTrash with an unreadable flag = %v, want the unreadable-flag refusal", err)
+ }
+ if tomb, terr := release.ReadAppTombstone(ctx, store, "bab", "myapp"); terr != nil || tomb != nil {
+ t.Fatalf("an unreadable flag must not write a marker: tomb=%+v err=%v", tomb, terr)
+ }
+}
+
+// TestHeadlessTrashHappyPathWritesMarkerAndReturnsItsWindow pins the success
+// path end to end: no production flag at all (absent is fine, per
+// GetProduction's (nil,nil) contract), TrashApp writes the marker, and the
+// returned window is the MARKER's own DeletedAt + TrashCooloff — not just
+// `now` + TrashCooloff, so a re-trash of an already-trashed app (create-only)
+// would still report the original clock.
+func TestHeadlessTrashHappyPathWritesMarkerAndReturnsItsWindow(t *testing.T) {
+ store, _, pub := newHeadlessTestStore(t)
+ ctx := context.Background()
+ now := time.Date(2026, 8, 4, 12, 0, 0, 0, time.UTC)
+
+ restorableUntil, err := headlessTrash(ctx, store, pub, "bab", "myapp", now)
+ if err != nil {
+ t.Fatalf("headlessTrash: %v", err)
+ }
+ tomb, terr := release.ReadAppTombstone(ctx, store, "bab", "myapp")
+ if terr != nil {
+ t.Fatalf("ReadAppTombstone: %v", terr)
+ }
+ if tomb == nil {
+ t.Fatal("headlessTrash succeeded but wrote no marker")
+ }
+ want := tomb.DeletedAt.Add(release.TrashCooloff)
+ if !restorableUntil.Equal(want) {
+ t.Fatalf("restorableUntil = %v, want the marker's own DeletedAt+TrashCooloff = %v", restorableUntil, want)
+ }
+ if !tomb.DeletedAt.Equal(now) {
+ t.Fatalf("marker DeletedAt = %v, want %v", tomb.DeletedAt, now)
+ }
+}