amadan
app: fix round 2 — $DOCS_ENV can no longer grant development
Three Important findings: 1. Fix round 1's own $DOCS_ENV fallback (the coordinator's instruction) reinstated the exact trap the deleted -dev-auth flag existed to close: devauth_flag.go's own words were "no config default, no env var, nothing a systemd unit's boilerplate could set without someone typing this literal flag." Platform instance config IS environment variables (DOCS_ORIGIN, DOCS_INSTANCE_KEY, DOCS_SMTP_*), so a bare DOCS_ENV=development one line away from its neighbours was a production-reachable path to the public dev InstanceKey constant and to every magic link, token included, landing in the instance's own log. cmd/docs/main.go gains chooseEnv(flagEnv, osEnv): only the literal -env flag may ever select "development"; $DOCS_ENV is still read for any other value (nothing in docs.New treats those specially). Verified live: DOCS_ENV=development with a real origin and no -env flag now refuses exactly like plain production; explicit -env development still works. 2. cmd/docs had no tests at all, so extractEnvFlag and chooseEnv -- the parser and policy that alone decide the security-relevant Env -- were covered by neither go test nor the gate. Added cmd/docs/main_test.go: extractEnvFlag (flag+value, absent, both equals forms, a trailing bare -env, and -env swallowing the next flag as its value), chooseEnv (every combination of flag/env-var, including the DOCS_ENV=development regression case), and a composed test proving the two argv edge cases fail toward production rather than merely happening to. 3. The plan's own Task 4 Step 2 still told the deployer to build with -tags release and confirm dev auth is absent -- both meaningless since fix round 1 deleted the release tag's only user. Rewritten to verify what actually matters now: unset DOCS_ORIGIN refuses, unset mail config refuses, and DOCS_ENV=development with no flag does not produce a development app. Step 3's real ship command also dropped -tags release, since it built nothing different from a plain build. Full go test ./... (now including cmd/docs) and the complete .amadan/ci.d/10-gate both pass.
5 files changed,
+256
−23
AGENTS.md+10 −2cmd/docs/main.go+45 −14cmd/docs/main_test.go+182 −0docs/superpowers/plans/2026-08-31-signin-and-samples.md+11 −3internal/docs/app.go+8 −4
diff --git a/AGENTS.md b/AGENTS.md| index 833cf67..dd5280f 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -52,8 +52,16 @@ empty `$DOCS_INSTANCE_KEY` or unconfigured mail is fine only in |
| development (a fixed dev key, and magic links logged instead of mailed, |
| clearly marked); anywhere else it is a startup refusal, the same |
| fail-closed shape `Origin` already used. `cmd/docs/main.go`'s `-env` flag |
| -(or `$DOCS_ENV`) sets it; the default, with neither set, is `"production"` |
| -— never a silent development fallback. |
| +sets it; `$DOCS_ENV` is consulted too but can never select |
| +`"development"` on its own (fix round 2 on Task 1 — platform instance |
| +config *is* environment variables, so a bare `$DOCS_ENV=development` |
| +sitting beside `$DOCS_ORIGIN`/`$DOCS_INSTANCE_KEY`/`$DOCS_SMTP_*` would |
| +be a production-reachable path to the public dev `InstanceKey` and to |
| +every magic link, token included, landing in the instance's own log — |
| +see `chooseEnv`'s own doc comment). The default, with no flag and no |
| +env var (or with `$DOCS_ENV=development` and no flag), is |
| +`"production"` — `"development"` always takes an explicit `-env` |
| +flag, never a silent fallback. |
| ## Running it locally |
diff --git a/cmd/docs/main.go b/cmd/docs/main.go| index 6af97eb..7d85196 100644 |
| --- a/cmd/docs/main.go |
| +++ b/cmd/docs/main.go |
| @@ -45,10 +45,46 @@ func extractEnvFlag(args []string) (env string, rest []string) { |
| return env, rest |
| } |
| +// chooseEnv decides docs.Config.Env from -env's own extracted value |
| +// (flagEnv, "" if the flag was absent or given with no value) and |
| +// $DOCS_ENV (osEnv, "" if unset). "development" is honoured ONLY from |
| +// flagEnv — fix round 2 on Task 1: the coordinator's own original |
| +// instruction added a $DOCS_ENV fallback for this, and that reinstated |
| +// exactly the trap the deleted -dev-auth flag existed to close. |
| +// devauth_flag.go's own words, before it was deleted: "no config |
| +// default, no env var, nothing a systemd unit's boilerplate could set |
| +// without someone typing this literal flag." Platform instance |
| +// configuration IS environment variables — $DOCS_ORIGIN, |
| +// $DOCS_INSTANCE_KEY, $DOCS_SMTP_* — so a bare $DOCS_ENV=development |
| +// sitting one line away from its neighbours in the same block is a |
| +// production-reachable path to the public constant |
| +// docs-development-instance-key-not-a-secret (internal/docs/auth.go) |
| +// standing in for the HMAC key that seals auth's pending blob, and to |
| +// every magic link — token included — landing in the instance's own |
| +// log instead of being mailed. Verified live by the reviewer: |
| +// $DOCS_ENV=development with a real origin booted a development app |
| +// under the round-1 code. |
| +// |
| +// osEnv is still consulted for any OTHER value (a future "staging", |
| +// say): every fail-closed check in docs.New reads as "== development" |
| +// or "!= development", so any non-"development" string already |
| +// behaves exactly like production, and there is nothing for $DOCS_ENV |
| +// to unlock there. Only "development" itself is special-cased out. |
| +func chooseEnv(flagEnv, osEnv string) string { |
| + if flagEnv != "" { |
| + return flagEnv |
| + } |
| + if osEnv != "" && osEnv != "development" { |
| + return osEnv |
| + } |
| + return "production" |
| +} |
| + |
| func main() { |
| logger := slog.Default() |
| - // -env (or $DOCS_ENV, checked below once flags are parsed) |
| + // -env (chooseEnv above; $DOCS_ENV is consulted too, but never for |
| + // "development" — see chooseEnv's own doc comment, fix round 2) |
| // decides docs.Config.Env — deliberately the ONLY thing that |
| // does, independent of any identity flag. It used to be |
| // entangled with -dev-auth (whichever call set Env also turned |
| @@ -124,19 +160,14 @@ func main() { |
| } |
| } |
| - // Safe default: production, unless told otherwise. -env wins over |
| - // $DOCS_ENV wins over the default — the same precedence |
| - // $DOCS_ORIGIN's own override has (opts.Addr only fills in when |
| - // $DOCS_ORIGIN is unset above). Nothing here ever defaults to |
| - // "development" on its own; that always takes an explicit flag or |
| - // env var, on this binary or in run-dev.sh's own invocation. |
| - env := envFlag |
| - if env == "" { |
| - env = os.Getenv("DOCS_ENV") |
| - } |
| - if env == "" { |
| - env = "production" |
| - } |
| + // chooseEnv: -env wins; $DOCS_ENV wins next EXCEPT for |
| + // "development" itself, which chooseEnv never honours from an env |
| + // var (fix round 2 — see its own doc comment); the default with |
| + // neither set, or with $DOCS_ENV=development and no flag, is |
| + // "production". Nothing here ever defaults to "development" on its |
| + // own; that always takes an explicit -env flag, on this binary or |
| + // in run-dev.sh's own invocation. |
| + env := chooseEnv(envFlag, os.Getenv("DOCS_ENV")) |
| // New opens its own database handle (SKILL.md §1's "app that opens |
| // its own database"), so opts.DBPath is blanked below before Serve |
diff --git a/cmd/docs/main_test.go b/cmd/docs/main_test.go| new file mode 100644 |
| index 0000000..1befd55 |
| --- /dev/null |
| +++ b/cmd/docs/main_test.go |
| @@ -0,0 +1,182 @@ |
| +package main |
| + |
| +import ( |
| + "reflect" |
| + "testing" |
| +) |
| + |
| +// TestExtractEnvFlag covers the argv parser that alone decides the |
| +// security-relevant Env value (fix round 2, finding 2: cmd/docs had |
| +// no tests at all, so this parser was covered by neither go test nor |
| +// the gate). rastrillo.Resolve owns os.Args and errors on anything it |
| +// does not define, so getting the strip-before-Resolve behavior right |
| +// matters as much as getting the value right. |
| +func TestExtractEnvFlag(t *testing.T) { |
| + cases := []struct { |
| + name string |
| + args []string |
| + wantEnv string |
| + wantRest []string |
| + }{ |
| + { |
| + name: "flag present, set to development", |
| + args: []string{"-env", "development", "-addr", ":8080"}, |
| + wantEnv: "development", |
| + wantRest: []string{"-addr", ":8080"}, |
| + }, |
| + { |
| + name: "flag absent", |
| + args: []string{"-addr", ":8080", "-db", "docs.db"}, |
| + wantEnv: "", |
| + wantRest: []string{"-addr", ":8080", "-db", "docs.db"}, |
| + }, |
| + { |
| + name: "equals form", |
| + args: []string{"-env=development"}, |
| + wantEnv: "development", |
| + wantRest: nil, |
| + }, |
| + { |
| + name: "double-dash equals form", |
| + args: []string{"--env=production"}, |
| + wantEnv: "production", |
| + wantRest: nil, |
| + }, |
| + { |
| + // A trailing bare -env with no value: extractEnvFlag must |
| + // not treat the flag as if it had been given a value (and |
| + // must not panic reading past the end of args). The |
| + // reviewer confirmed this lands on non-development |
| + // downstream (via chooseEnv, tested below) — this case |
| + // pins down why: an empty env here, not a crash or a |
| + // swallowed neighbor. |
| + name: "trailing bare -env, no value", |
| + args: []string{"-addr", ":8080", "-env"}, |
| + wantEnv: "", |
| + wantRest: []string{"-addr", ":8080"}, |
| + }, |
| + { |
| + // -env greedily takes the very next token as its value, |
| + // so "-env -addr :8080" swallows "-addr" as the env |
| + // string rather than leaving it for rastrillo.Resolve. |
| + // This is the other case the reviewer flagged as |
| + // "currently lands on non-development... but untested": |
| + // pinned down here at the parser level (env == "-addr", |
| + // definitely not "development"), and again below via |
| + // chooseEnv, which is what actually decides |
| + // docs.Config.Env from it. |
| + name: "-env with no value, next token is another flag", |
| + args: []string{"-env", "-addr", ":8080"}, |
| + wantEnv: "-addr", |
| + wantRest: []string{":8080"}, |
| + }, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + env, rest := extractEnvFlag(c.args) |
| + if env != c.wantEnv { |
| + t.Errorf("extractEnvFlag(%v) env = %q, want %q", c.args, env, c.wantEnv) |
| + } |
| + if !reflect.DeepEqual(rest, c.wantRest) { |
| + t.Errorf("extractEnvFlag(%v) rest = %v, want %v", c.args, rest, c.wantRest) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestChooseEnvHonoursDevelopmentOnlyFromTheFlag is fix round 2's own |
| +// regression test for finding 1: the coordinator's original |
| +// instruction added a $DOCS_ENV fallback that could set |
| +// Env=development, reinstating exactly the trap the deleted |
| +// -dev-auth flag existed to close (no env var may ever grant |
| +// development — only the literal flag). $DOCS_ENV=development with |
| +// no flag is the case that must NOT produce a development app. |
| +func TestChooseEnvHonoursDevelopmentOnlyFromTheFlag(t *testing.T) { |
| + cases := []struct { |
| + name string |
| + flagEnv string |
| + osEnv string |
| + want string |
| + }{ |
| + { |
| + name: "flag present and set to development", |
| + flagEnv: "development", |
| + osEnv: "", |
| + want: "development", |
| + }, |
| + { |
| + name: "absent: no flag, no env var", |
| + flagEnv: "", |
| + osEnv: "", |
| + want: "production", |
| + }, |
| + { |
| + // The regression this whole fix round exists for: |
| + // $DOCS_ENV=development with no -env flag must not select |
| + // development. Platform instance config IS environment |
| + // variables (DOCS_ORIGIN, DOCS_INSTANCE_KEY, DOCS_SMTP_*), |
| + // so this is the one value $DOCS_ENV must never be |
| + // trusted with. |
| + name: "$DOCS_ENV=development with no flag", |
| + flagEnv: "", |
| + osEnv: "development", |
| + want: "production", |
| + }, |
| + { |
| + // $DOCS_ENV is still useful for any OTHER value: nothing |
| + // in docs.New treats a non-"development" Env specially, |
| + // so there is no risk here to guard against. |
| + name: "$DOCS_ENV set to something other than development", |
| + flagEnv: "", |
| + osEnv: "staging", |
| + want: "staging", |
| + }, |
| + { |
| + name: "flag always wins over $DOCS_ENV, even the other way", |
| + flagEnv: "development", |
| + osEnv: "staging", |
| + want: "development", |
| + }, |
| + { |
| + name: "explicit -env production overrides a stray $DOCS_ENV=development", |
| + flagEnv: "production", |
| + osEnv: "development", |
| + want: "production", |
| + }, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + got := chooseEnv(c.flagEnv, c.osEnv) |
| + if got != c.want { |
| + t.Errorf("chooseEnv(%q, %q) = %q, want %q", c.flagEnv, c.osEnv, got, c.want) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestArgvEdgeCasesFailTowardProduction composes extractEnvFlag and |
| +// chooseEnv exactly as main() does, for the two argv shapes the |
| +// reviewer named as "currently land on non-development... but |
| +// untested": a trailing bare -env, and -env immediately followed by |
| +// another flag (which extractEnvFlag swallows as -env's own value). |
| +// Both must resolve to something other than "development" — "it |
| +// happens to fail safe" is not a property until something asserts |
| +// it. |
| +func TestArgvEdgeCasesFailTowardProduction(t *testing.T) { |
| + cases := []struct { |
| + name string |
| + args []string |
| + }{ |
| + {name: "trailing bare -env", args: []string{"-addr", ":8080", "-env"}}, |
| + {name: "-env swallows the next flag as its value", args: []string{"-env", "-addr", ":8080"}}, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + flagEnv, _ := extractEnvFlag(c.args) |
| + got := chooseEnv(flagEnv, "") |
| + if got == "development" { |
| + t.Errorf("args %v resolved to Env=development, want anything else (fails toward production)", c.args) |
| + } |
| + }) |
| + } |
| +} |
diff --git a/docs/superpowers/plans/2026-08-31-signin-and-samples.md b/docs/superpowers/plans/2026-08-31-signin-and-samples.md| index 7841b81..4ac7c19 100644 |
| --- a/docs/superpowers/plans/2026-08-31-signin-and-samples.md |
| +++ b/docs/superpowers/plans/2026-08-31-signin-and-samples.md |
| @@ -298,17 +298,25 @@ Docs can now go on a public URL, because every route requires a signed-in person |
| `carlos email enable` for account `bds`, since magic links need outbound mail. **If email is not available, stop and report** — a sign-in that cannot send its link is not sign-in, and a preview with mail stubbed out is a public app with extra steps. |
| -- [ ] **Step 2: Verify the guards hold in a release build** |
| +- [ ] **Step 2: Verify the guards hold** |
| -Build with `-tags release` and confirm: dev auth is absent; an unset `DOCS_ORIGIN` refuses; an unset mail configuration refuses. Paste the refusals. |
| +Corrected in a fix round on Task 1: there is no `-tags release` build and no dev-auth flag to exclude any more — the development-only stand-in identity that check used to verify the *absence* of was deleted outright once real sign-in made it dead code (`internal/docs/devauth.go` and both `cmd/docs/devauth_flag*.go` are gone; `release` is now an undefined build tag, so building with it would silently compile the ordinary binary and verify nothing). Verify instead, against a plain build (`CGO_ENABLED=0 go build ./cmd/docs`): |
| + |
| +- an unset `$DOCS_ORIGIN` refuses to start; |
| +- unset mail configuration (`$DOCS_SMTP_HOST`/`$DOCS_SMTP_FROM`) refuses to start; |
| +- `$DOCS_ENV=development` with **no** `-env` flag does **not** produce a development app — it must still refuse on the two points above, since `chooseEnv` (`cmd/docs/main.go`) never honours `"development"` from an environment variable, only from the literal `-env` flag. This is the property fix round 2 added: platform instance configuration is environment variables, so `$DOCS_ENV=development` sitting beside `$DOCS_ORIGIN`/`$DOCS_INSTANCE_KEY`/`$DOCS_SMTP_*` would otherwise be a production-reachable path to the public dev `InstanceKey` and to every magic link, token included, landing in the instance's own log. |
| + |
| +Paste the refusals — all three. |
| - [ ] **Step 3: Ship** |
| ```bash |
| -CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -tags release -ldflags "-s -w" -o docs-linux-arm64 ./cmd/docs |
| +CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -ldflags "-s -w" -o docs-linux-arm64 ./cmd/docs |
| carlos ship --account bds --app docs --kind binary --target linux-arm64 --label "first live" docs-linux-arm64 |
| ``` |
| +No `-tags release`: that tag has no meaning left in this codebase (see Step 2). |
| + |
| Then promote and create an instance per the platform's own flow — read `carlos deploy -h` and `carlos instances -h` rather than guessing, and **report each command's real output**. |
| - [ ] **Step 4: Prove it from outside** |
diff --git a/internal/docs/app.go b/internal/docs/app.go| index d999b8d..b9a0c67 100644 |
| --- a/internal/docs/app.go |
| +++ b/internal/docs/app.go |
| @@ -43,10 +43,14 @@ type Config struct { |
| // route, and it was deleted rather than left as an unreachable |
| // trap for whoever next read "DevAuth" and assumed it still did |
| // something). cmd/docs/main.go sources this from an explicit |
| - // -env flag / $DOCS_ENV, deliberately independent of any identity |
| - // flag — Env choosing "log mail instead of sending it" is not the |
| - // same decision as "who can sign in", and conflating the two is |
| - // exactly what fix round 1 un-did. |
| + // -env flag, deliberately independent of any identity flag — Env |
| + // choosing "log mail instead of sending it" is not the same |
| + // decision as "who can sign in", and conflating the two is |
| + // exactly what fix round 1 un-did. $DOCS_ENV is consulted too, |
| + // but — fix round 2, after that round's own $DOCS_ENV fallback |
| + // turned out to reopen the same hole — never for "development" |
| + // itself: only the literal flag may ever select it |
| + // (cmd/docs/main.go's chooseEnv has the full reasoning). |
| Env string |
| // Origin is this app's own origin (scheme://host[:port], no path), |