amadan
instances enable/create: close the review's three confirmed holes
Opus review findings on the #66 door, all built:
- Sibling-apex capture: a platform-pinned manifest made every sibling
app's apex a valid host shape for the wrong app, and a record there
would mint a real route the domains pass defers to forever. The
create door now refuses a label naming another claimed app in the
account (user and app tokens both); custom-domain manifests are
untouched.
- TOCTOU on enable: read-then-PutManifest (an upsert that re-reads
inside) could silently re-home an operator's store-mode manifest
written between check and write. The door now writes with
PutManifestIfMatch against the etag it read ("" = create-only) and
answers a lost race with a 409.
- Unvalidated pinned domain: PLATFORM_APPS_DOMAIN is refused at console
boot when malformed (the secretsPub precedent) and the composed
<sqid>.<domain> is shape-checked in the handler, so a wildcard-DNS
copy-paste can never be stamped into a manifest every future host is
validated against.
Also: store-mode -channel "" explicit stays the fatal it always was
(parity fix), README's PLATFORM_APPS_DOMAIN entry names the console as
a reader, and the spec addendum records the built outcomes plus the
recorded-not-built ones (record caps, additive-field drop on RMW,
non-apex instances minting the apex onto one customer's socket).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8 files changed,
+242
−15
README.md+6 −3cmd/console/main.go+22 −1docs/superpowers/specs/2026-08-04-instance-provisioning-design.md+28 −0instances_cli.go+6 −2internal/console/apiinstances.go+57 −4internal/console/apiinstancesenable_test.go+52 −0internal/provision/provision.go+31 −5internal/provision/provision_test.go+40 −0
diff --git a/README.md b/README.md| index d02050170..f9c453f57 100644 |
| --- a/README.md |
| +++ b/README.md |
| @@ -292,9 +292,12 @@ publish an inflated instance count. |
| **Config surface:** |
| - `PLATFORM_APPS_DOMAIN` — the deployment's apps domain (flagship: |
| - `oncarlos.com`), read by `carlos agent`. **Unset = the whole feature is |
| - inert**: no minting, no manifests read, zero behavior change — the same |
| - litestream convention as everything else here. |
| + `oncarlos.com`), read by `carlos agent`, the edge, and the console (the |
| + member-facing `carlos instances enable` pins manifests to |
| + `<sqid>.<this domain>`; the console refuses a malformed value at boot). |
| + **Unset = the whole feature is inert**: no minting, no manifests read, |
| + the enable door answers 503 — the same litestream convention as |
| + everything else here. |
| - `PLATFORM_EDGE_IPS` — comma-separated edge IPs the DNS health checks |
| compare against, read by both the agent (checks) and the console |
| (guidance text), so the two can never name different addresses. Default: |
diff --git a/cmd/console/main.go b/cmd/console/main.go| index d509f3cad..c91aabf00 100644 |
| --- a/cmd/console/main.go |
| +++ b/cmd/console/main.go |
| @@ -52,6 +52,7 @@ import ( |
| "github.com/carlosframework/platform/internal/appconfig" |
| "github.com/carlosframework/platform/internal/console" |
| + "github.com/carlosframework/platform/internal/provision" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -230,7 +231,7 @@ func serve() { |
| Key: key, |
| EdgeIPs: edgeIPs(), |
| SecretsPub: secretsPub(), |
| - AppsDomain: strings.ToLower(strings.TrimSpace(os.Getenv("PLATFORM_APPS_DOMAIN"))), |
| + AppsDomain: appsDomain(), |
| }, st) |
| if err != nil { |
| log.Fatal(err) |
| @@ -306,6 +307,26 @@ func secretsPub() string { |
| return raw |
| } |
| +// appsDomain reads PLATFORM_APPS_DOMAIN — the same variable the edge and |
| +// the host agent read — for the CLI's member-facing `carlos instances |
| +// enable` door, which pins manifests to <sqid>.<this domain>. Garbage is |
| +// refused HERE, at boot, rather than stamped into a manifest every future |
| +// instance host gets validated against (the secretsPub precedent): the |
| +// suffix rides into bucket keys verbatim, so "*.oncarlos.com" copied off a |
| +// DNS record would produce records the reconcile pass skips — and protects |
| +// — forever. Unset is a legal deployment: the door answers 503 naming the |
| +// store-mode operator act. |
| +func appsDomain() string { |
| + d := strings.ToLower(strings.TrimSpace(os.Getenv("PLATFORM_APPS_DOMAIN"))) |
| + if d == "" { |
| + return "" |
| + } |
| + if !provision.ValidHostShape(d) { |
| + log.Fatalf("console: PLATFORM_APPS_DOMAIN %q is not a valid domain (dot-joined DNS labels, no wildcard, no leading dot)", d) |
| + } |
| + return d |
| +} |
| + |
| // edgeIPs parses PLATFORM_EDGE_IPS (comma-separated) — the SAME variable the |
| // agent's domains reconcile loop reads, so the console's point-your-DNS-here |
| // guidance and the agent's DNS health checks can never name different |
diff --git a/docs/superpowers/specs/2026-08-04-instance-provisioning-design.md b/docs/superpowers/specs/2026-08-04-instance-provisioning-design.md| index 94a1fd8cb..d412fb808 100644 |
| --- a/docs/superpowers/specs/2026-08-04-instance-provisioning-design.md |
| +++ b/docs/superpowers/specs/2026-08-04-instance-provisioning-design.md |
| @@ -394,6 +394,34 @@ minting idempotently, and `wildcardParent` covers the host at exactly the |
| per-account wildcard depth. No console route write anywhere: §11's "the |
| agent's reconcile pass is the only route writer" stands untouched. |
| +Review outcomes (Opus, 2026-08-06), built: |
| + |
| +- **Sibling-apex refusal.** The platform subdomain is a namespace every |
| + app's apex shares, so a platform-pinned manifest makes sibling apexes |
| + valid host shapes for the wrong app — and a record there would mint a |
| + real route the domains pass defers to forever. The create door refuses a |
| + label naming another claimed app in the account (user and app tokens |
| + both); custom-domain manifests never enter apex space and are untouched. |
| +- **Guarded manifest write.** The enable door writes with |
| + `PutManifestIfMatch` against the etag it READ ("" = create-only), so an |
| + operator's store-mode manifest landing between check and write surfaces |
| + as a 409, never a silent re-home. `PutManifest`'s upsert stays the |
| + store-mode (operator-authority) semantics. |
| +- **Domain validated at both ends.** The console refuses a malformed |
| + `PLATFORM_APPS_DOMAIN` at boot (the secretsPub precedent) and the door |
| + re-checks the composed `<sqid>.<domain>` shape before stamping it. |
| + |
| +Recorded, not built: no per-app/per-account cap on records beyond the rate |
| +limiter (the operator opt-in used to be the admission control — a ceiling |
| +wants its own ruling); the enable door's read-modify-write would drop |
| +schema-1 fields added by a NEWER binary (upgrade the console before |
| +shipping such a field — same property the store-mode upsert has); a |
| +non-apex instance (e.g. `alice.<app>.<sqid>.<domain>`-style multi-tenant |
| +use under the account pin) still causes candidateApps/firstReadyUpstream to |
| +mint the app's apex onto that one customer's socket — pre-existing |
| +behaviour, now reachable by members, worth its own slice if platform-pinned |
| +multi-tenant apps become a real shape. |
| + |
| ## 13. Testing |
| - `internal/provision`: record/manifest validation matrices, key parsing |
diff --git a/instances_cli.go b/instances_cli.go| index 1da564fe8..79e3f3bac 100644 |
| --- a/instances_cli.go |
| +++ b/instances_cli.go |
| @@ -129,11 +129,15 @@ func runInstancesEnable(args []string) { |
| return |
| } |
| - // Store mode — the operator path, unchanged. |
| + // Store mode — the operator path, unchanged: an ABSENT -channel takes |
| + // the documented "stable" default, while an explicitly empty one stays |
| + // the "not a valid channel" refusal it has always been (the flag's |
| + // default moved from "stable" to "" for the signed-in mode's |
| + // preserve-vs-replace split; set[] is what keeps the two cases apart). |
| if *domain == "" { |
| log.Fatal(instancesEnableUsage) |
| } |
| - if *channel == "" { |
| + if *channel == "" && !set["channel"] { |
| *channel = "stable" |
| } |
| // A domain is dot-joined labels with no suffix of its own to check |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index aa2fc647a..31c3a208c 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -240,6 +240,32 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| "invalid host — an instance host is one DNS label followed by ."+m.Domain) |
| return |
| } |
| + // The account's platform subdomain is a SHARED namespace: every app's |
| + // apex lives at <app>.<sqid>.<AppsDomain>, and a manifest pinned there |
| + // (the member-facing enable door) makes every sibling's apex a valid |
| + // host shape for THIS app. A record at a sibling's apex would mint a |
| + // real route there first, and the domains pass would then skip the |
| + // sibling's own apex mint forever (byHost) — one app quietly serving |
| + // another's public URL (review 2026-08-06 #1). Within-account only, but |
| + // reachable by an app token, so it is refused rather than trusted to |
| + // membership: a label naming another claimed app in the account belongs |
| + // to that app. Custom-domain manifests never enter apex space (ValidHost |
| + // pinned the suffix above), so this check cannot touch them. |
| + if s.cfg.AppsDomain != "" && m.Domain == acct.Sqid+"."+s.cfg.AppsDomain { |
| + if label := strings.TrimSuffix(host, "."+m.Domain); label != app { |
| + claimed, err := s.store.HasAppIn(acct.ID, label) |
| + if err != nil { |
| + log.Printf("console: api instances create sibling check %s/%s: %v", acct.Sqid, label, err) |
| + jsonError(w, http.StatusInternalServerError, "could not check the host — try again") |
| + return |
| + } |
| + if claimed { |
| + jsonError(w, http.StatusConflict, |
| + host+" is app "+label+"'s platform apex — create it as an instance of "+label+" instead") |
| + return |
| + } |
| + } |
| + } |
| region := strings.TrimSpace(req.Region) |
| if !provision.ValidRegion(region) || !instanceRegionAllowed(m.Regions, region) { |
| msg := "invalid region" |
| @@ -461,12 +487,30 @@ func (s *Server) handleAPIInstanceEnable(w http.ResponseWriter, r *http.Request) |
| } |
| } |
| domain := acct.Sqid + "." + s.cfg.AppsDomain |
| + // The domain is about to be stamped into a manifest every future host |
| + // gets checked against, and ValidHost only regex-checks the leading |
| + // label — the suffix rides into bucket keys verbatim. Store mode |
| + // validates its -domain for exactly this reason (instances_cli.go); a |
| + // misconfigured PLATFORM_APPS_DOMAIN ("*.oncarlos.com" copied off a DNS |
| + // record) must refuse here, not surface as records reconcile skips |
| + // forever. The console also refuses garbage at boot (cmd/console); this |
| + // guard covers embedders that build Config directly. |
| + if !provision.ValidHostShape(domain) { |
| + jsonError(w, http.StatusServiceUnavailable, |
| + "this console's PLATFORM_APPS_DOMAIN is not a valid domain — fix the console configuration") |
| + return |
| + } |
| ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| - m, err := provision.GetManifest(ctx, s.cfg.Releases, acct.Sqid, app) |
| + // Read WITH the etag and write guarded by it below: the domain-conflict |
| + // refusal is only honest if nothing can land between this read and the |
| + // write. PutManifest's upsert re-reads inside, so an operator's |
| + // store-mode manifest written into that window would be silently |
| + // re-homed — the exact overwrite the 409 exists to prevent. |
| + m, etag, err := provision.GetManifestETag(ctx, s.cfg.Releases, acct.Sqid, app) |
| switch { |
| case errors.Is(err, os.ErrNotExist): |
| - m = &provision.Manifest{Schema: 1, App: app, Domain: domain} |
| + m, etag = &provision.Manifest{Schema: 1, App: app, Domain: domain}, "" |
| case err != nil: |
| log.Printf("console: api instances enable read %s/%s: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not read instance settings") |
| @@ -483,14 +527,23 @@ func (s *Server) handleAPIInstanceEnable(w http.ResponseWriter, r *http.Request) |
| return |
| } |
| // Pool and Regions survive untouched on an update: they are operator |
| - // policy this door never speaks for, in either direction. |
| + // policy this door never speaks for, in either direction. (Recorded |
| + // debt: fields ADDED to schema 1 after this binary would be dropped by |
| + // this read-modify-write, the same property the store-mode upsert has — |
| + // an additive manifest field needs the console upgraded before members |
| + // enable through it.) |
| if req.Channel != nil { |
| m.Channel = *req.Channel |
| } |
| if req.Seed != nil { |
| m.Seed = *req.Seed |
| } |
| - if err := provision.PutManifest(ctx, s.cfg.Releases, acct.Sqid, app, *m); err != nil { |
| + if err := provision.PutManifestIfMatch(ctx, s.cfg.Releases, acct.Sqid, app, *m, etag); err != nil { |
| + if errors.Is(err, release.ErrCASConflict) { |
| + jsonError(w, http.StatusConflict, |
| + "instance settings changed while this request was in flight — try again") |
| + return |
| + } |
| log.Printf("console: api instances enable %s/%s: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not enable instances — try again") |
| return |
diff --git a/internal/console/apiinstancesenable_test.go b/internal/console/apiinstancesenable_test.go| index b1ca85fbc..82896501b 100644 |
| --- a/internal/console/apiinstancesenable_test.go |
| +++ b/internal/console/apiinstancesenable_test.go |
| @@ -167,6 +167,58 @@ func TestAPIInstanceEnableUpdatePreserves(t *testing.T) { |
| } |
| } |
| +// TestAPIInstanceCreateSiblingApexRefused: the account's platform subdomain |
| +// is a namespace every app's apex shares, so a manifest pinned there |
| +// (exactly what the enable door writes) makes sibling apexes valid host |
| +// SHAPES for the wrong app. A record at a sibling's apex would mint a real |
| +// route the domains pass then defers to forever — one app serving another's |
| +// public URL — so the create door refuses a label that names another |
| +// claimed app in the account, for user and app tokens both. Unclaimed |
| +// labels and the app's own apex stay open. |
| +func TestAPIInstanceCreateSiblingApexRefused(t *testing.T) { |
| + fx, member := instanceFixture(t, "code-inst-sibling") |
| + fx.srv.cfg.AppsDomain = "oncarlos.test" |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "marketing", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if status, _, out := rawReq(t, http.MethodPut, enableURL(fx, "bab", "titogo"), member, `{}`); status != http.StatusOK { |
| + t.Fatalf("enable = %d %s", status, out) |
| + } |
| + domain := fx.sqid + ".oncarlos.test" |
| + |
| + status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"marketing.`+domain+`"}`) |
| + if status != http.StatusConflict { |
| + t.Fatalf("sibling apex create = %d %s, want 409", status, out) |
| + } |
| + if !strings.Contains(out, "marketing") || !strings.Contains(out, "platform apex") { |
| + t.Fatalf("sibling refusal body = %s, want it to name the app and the apex", out) |
| + } |
| + |
| + // The app token meets the same wall: it authorizes titogo's records, |
| + // and a sibling's apex is not one of them. |
| + appTok, err := fx.st.NewAppToken(OpsAccountID, "titogo", "alice@keymail.dev", "signup", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), appTok, |
| + `{"host":"marketing.`+domain+`"}`); status != http.StatusConflict { |
| + t.Fatalf("app-token sibling apex create = %d %s, want 409", status, out) |
| + } |
| + |
| + // The app's own apex and an unclaimed label are exactly what the door |
| + // is for. |
| + if status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"titogo.`+domain+`"}`); status != http.StatusCreated { |
| + t.Fatalf("own apex create = %d %s, want 201", status, out) |
| + } |
| + if status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"acme.`+domain+`"}`); status != http.StatusCreated { |
| + t.Fatalf("unclaimed label create = %d %s, want 201", status, out) |
| + } |
| +} |
| + |
| // TestAPIInstanceEnableRefusals: every way of not being allowed to enable, |
| // and the order the answers keep secrets in. Before membership is proven the |
| // answer is the family's byte-identical 404; an app token meets cliUser's |
diff --git a/internal/provision/provision.go b/internal/provision/provision.go| index 2c3cb0838..6b149476f 100644 |
| --- a/internal/provision/provision.go |
| +++ b/internal/provision/provision.go |
| @@ -155,18 +155,26 @@ func ValidPool(pool string) bool { return regionPoolRe.MatchString(pool) } |
| // truncated by json.Unmarshal (appconfig/store.go's precedent) — acting on |
| // a newer manifest would silently discard whatever the unknown fields meant. |
| func GetManifest(ctx context.Context, s release.Store, acct, app string) (*Manifest, error) { |
| - raw, _, err := s.Get(ctx, ManifestKey(acct, app)) |
| + m, _, err := GetManifestETag(ctx, s, acct, app) |
| + return m, err |
| +} |
| + |
| +// GetManifestETag is GetManifest carrying the etag its bytes were read |
| +// under — for a caller that must write back GUARDED against what it read |
| +// (PutManifestIfMatch), not against whatever is there at write time. |
| +func GetManifestETag(ctx context.Context, s release.Store, acct, app string) (*Manifest, string, error) { |
| + raw, etag, err := s.Get(ctx, ManifestKey(acct, app)) |
| if err != nil { |
| - return nil, err |
| + return nil, "", err |
| } |
| var m Manifest |
| if err := json.Unmarshal(raw, &m); err != nil { |
| - return nil, fmt.Errorf("provision: manifest %s/%s: %w", acct, app, err) |
| + return nil, "", fmt.Errorf("provision: manifest %s/%s: %w", acct, app, err) |
| } |
| if m.Schema > 1 { |
| - return nil, fmt.Errorf("provision: manifest %s/%s: schema %d is newer than this binary understands — upgrade carlos", acct, app, m.Schema) |
| + return nil, "", fmt.Errorf("provision: manifest %s/%s: schema %d is newer than this binary understands — upgrade carlos", acct, app, m.Schema) |
| } |
| - return &m, nil |
| + return &m, etag, nil |
| } |
| // PutManifest publishes app's instances manifest — a CAS upsert (read |
| @@ -197,6 +205,24 @@ func PutManifest(ctx context.Context, s release.Store, acct, app string, m Manif |
| return nil |
| } |
| +// PutManifestIfMatch writes app's instances manifest guarded by the etag |
| +// the caller READ it under — "" meaning it read nothing, which makes the |
| +// write create-only. Unlike PutManifest's upsert (which re-reads inside and |
| +// retries, so the last writer always wins) a lost race here surfaces as |
| +// release.ErrCASConflict for the caller to answer: the console's |
| +// member-facing enable door uses this because its domain-conflict check is |
| +// only honest if nothing can slip in between the check and the write |
| +// (review 2026-08-06 — an operator's store-mode manifest landing in that |
| +// window must never be silently re-homed). |
| +func PutManifestIfMatch(ctx context.Context, s release.Store, acct, app string, m Manifest, etag string) error { |
| + body, err := json.Marshal(m) |
| + if err != nil { |
| + return err |
| + } |
| + _, err = s.Put(ctx, ManifestKey(acct, app), body, etag) |
| + return err |
| +} |
| + |
| // CreateRecord writes a new instance record, create-only: a lost race |
| // against a second signup for the same host comes back wrapping |
| // release.ErrCASConflict, never a silent overwrite of the winner's record. |
diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go| index 30f94c15b..a42cbc7c4 100644 |
| --- a/internal/provision/provision_test.go |
| +++ b/internal/provision/provision_test.go |
| @@ -335,3 +335,43 @@ func TestDefaults(t *testing.T) { |
| t.Errorf("DefaultDBPath(%q, %q) = %q, want /data/h.db", "/data/", "h", got) |
| } |
| } |
| + |
| +// TestPutManifestIfMatch pins the guarded write's two properties: etag "" |
| +// is CREATE-ONLY (a manifest landing between a caller's read-nothing and |
| +// its write surfaces as the conflict it is, never an overwrite), and a |
| +// stale etag — the caller's read racing somebody else's update — is refused |
| +// the same way. This is what makes the console enable door's domain check |
| +// honest: PutManifest's upsert re-reads inside and the last writer wins, |
| +// which is exactly the wrong property when the write was authorized by |
| +// what the FIRST read said. |
| +func TestPutManifestIfMatch(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + m := Manifest{Schema: 1, App: "tito", Domain: "go.tito.io"} |
| + |
| + if err := PutManifestIfMatch(ctx, s, "bad", "tito", m, ""); err != nil { |
| + t.Fatalf("create-only put: %v", err) |
| + } |
| + if err := PutManifestIfMatch(ctx, s, "bad", "tito", m, ""); !errors.Is(err, release.ErrCASConflict) { |
| + t.Fatalf("second create-only put: err = %v, want ErrCASConflict", err) |
| + } |
| + |
| + got, etag, err := GetManifestETag(ctx, s, "bad", "tito") |
| + if err != nil || etag == "" { |
| + t.Fatalf("GetManifestETag: %+v %q %v, want a manifest with an etag", got, etag, err) |
| + } |
| + got.Channel = "beta" |
| + if err := PutManifestIfMatch(ctx, s, "bad", "tito", *got, etag); err != nil { |
| + t.Fatalf("guarded update: %v", err) |
| + } |
| + // The first read's etag is now stale; a write still guarded by it must |
| + // lose, not overwrite the update that beat it. |
| + got.Channel = "edge" |
| + if err := PutManifestIfMatch(ctx, s, "bad", "tito", *got, etag); !errors.Is(err, release.ErrCASConflict) { |
| + t.Fatalf("stale-etag put: err = %v, want ErrCASConflict", err) |
| + } |
| + final, err := GetManifest(ctx, s, "bad", "tito") |
| + if err != nil || final.Channel != "beta" { |
| + t.Fatalf("manifest after stale write = %+v %v, want the guarded update's beta", final, err) |
| + } |
| +} |