oficina / docs Public

app: fix round 1 on sign-in — POST regression coverage, -env, sign-out UI

Three Important findings from review:

1. TestSignInRequiredForEverything only checked GET / and GET
   /d/anything; POST /documents (the one route whose regression would
   be catastrophic) had no assertion. Added, with Sec-Fetch-Site:
   same-origin so csrf.Protect lets it through and RequireSession's
   own 403 is what's actually being proven. Verified by hand that the
   new assertion fails (a signed-out POST creates a document and
   redirects 303) when the route is moved out of the RequireSession
   group, then passes once restored.

2. -dev-auth was the only thing that ever set Env=development, and
   Env=development is what lets InstanceKey/Mailer default instead of
   refusing to start (auth.go) — an identity flag was secretly
   load-bearing for something that has nothing to do with identity.
   Decoupled: cmd/docs/main.go gains an explicit -env flag / $DOCS_ENV
   fallback, defaulting to "production". scripts/run-dev.sh now passes
   -env development explicitly.

   With that decoupling in place, the development-only stand-in
   identity itself was fully deletable, not just fixable:
   DevIdentity, CurrentDevIdentity and devAuthMiddleware had zero
   readers anywhere in the app once RequireSession started guarding
   every route unconditionally, and RequireSession never read or
   trusted the context value it stamped. Deleted internal/docs/devauth.go,
   internal/docs/devauth_test.go, cmd/docs/devauth_flag.go and
   cmd/docs/devauth_flag_release.go outright, along with
   Config.DevAuth, guardDevAuth, NewWithDevAuth and the -dev-auth flag
   itself — the `release` build tag they were the only user of goes
   with them. .amadan/ci.d/10-gate's release-build/-dev-auth step is
   gone too; the fail-closed properties it guarded are now covered by
   go test (TestNewRefusesMissingInstanceKeyOutsideDevelopment,
   TestNewRefusesMissingMailOutsideDevelopment,
   TestSignInRequiredForEverything). AGENTS.md and .amadan/README.md
   updated to match reality instead of describing deleted code.

3. POST /signout existed and worked but nothing linked to it. Wired a
   catalog-keyed, JavaScript-free sign-out form into both list.html's
   and show.html's page headers (list.html's shares a
   [docs-header-actions] wrapper with the create form, so
   [rst-page-header]'s two-child space-between layout still holds).

Not fixed, filed instead: the docs.signin.* strings still await a
single copy-review pass before deploy, deliberately batched rather
than done piecemeal. rastrillo/auth logging a recipient address on an
SMTP delivery failure (framework-originated, error path only, no
token) is on the record for Paul to raise upstream.

Full go test ./... and the complete .amadan/ci.d/10-gate both pass.
Paul Campbell pushed by paul@keymail.dev d55d79fdcb4dc4d15ad8cccc03014ee6a2f863ae parent 34013e4
17 files changed, +273 −435
  • .amadan/README.md +20 −9
  • .amadan/ci.d/10-gate +21 −47
  • AGENTS.md +40 −29
  • cmd/docs/devauth_flag.go +0 −32
  • cmd/docs/devauth_flag_release.go +0 −55
  • cmd/docs/main.go +60 −20
  • internal/docs/app.go +16 −41
  • internal/docs/auth.go +2 −2
  • internal/docs/auth_test.go +23 −4
  • internal/docs/devauth.go +0 −81
  • internal/docs/devauth_test.go +0 −80
  • internal/docs/handlers.go +15 −3
  • internal/docs/render.go +5 −0
  • internal/docs/testing.go +9 −8
  • internal/docs/views/list.html +36 −14
  • internal/docs/views/show.html +9 −0
  • scripts/run-dev.sh +17 −10
diff --git a/.amadan/README.md b/.amadan/README.md
index d63de43..4e8ba82 100644
--- a/.amadan/README.md
+++ b/.amadan/README.md
@@ -53,12 +53,23 @@ in the same commit as the change.
`go vet ./...` and `go test ./...` already cover `cmd/docs` and
`internal/docs` — the rastrillo app built on top of `internal/core` — but
neither one proves the app actually links into a runnable binary. The gate
-also runs `CGO_ENABLED=0 go build ./...` for that, and separately builds
-`cmd/docs` with `-tags release` and runs the result with `-dev-auth` on its
-command line, asserting it refuses to start: `devauth_flag_release.go`
-replaces the flag-parsing function with one that can never return true, so
-a release binary has no code path that can turn the development-only
-identity (`internal/docs/devauth.go`) on at all — a build-time closure of
-that hole, not just `guardDevAuth`'s runtime check. `scripts/run-dev.sh`
-builds and runs the ordinary (non-release) binary with `-dev-auth`, a fixed
-port, and a scratch database — one command to see the app in a browser.
+also runs `CGO_ENABLED=0 go build ./...` for that.
+
+There used to be a further step here: a separate `-tags release` build,
+run with `-dev-auth` on its command line, asserting it refused to start —
+proving a release binary had no code path that could turn a
+development-only stand-in identity on. That identity
+(`internal/docs/devauth.go`) had zero readers once rastrillo/auth's
+`RequireSession` started guarding every document route unconditionally
+(a fix round on Task 1, once real per-person sign-in existed), so it was
+deleted rather than kept as dead code — and the `release` build tag it
+was the only user of went with it. There is no release/non-release
+distinction left for this gate to hold apart; `go test` covers the
+fail-closed properties that matter now
+(`TestNewRefusesMissingInstanceKeyOutsideDevelopment`,
+`TestNewRefusesMissingMailOutsideDevelopment`,
+`TestSignInRequiredForEverything` — all `internal/docs/auth_test.go`).
+`scripts/run-dev.sh` builds and runs the ordinary binary with `-env
+development`, a fixed port, and a scratch database — one command to see
+the app in a browser, signed in for real via a magic link logged to the
+terminal instead of mailed.
diff --git a/.amadan/ci.d/10-gate b/.amadan/ci.d/10-gate
index 2f8d83d..220c1ec 100755
--- a/.amadan/ci.d/10-gate
+++ b/.amadan/ci.d/10-gate
@@ -79,52 +79,26 @@ echo "=== go: build ./... (the app, not just the packages tests import)"
# correctly on its own.
CGO_ENABLED=0 go build ./...
-echo "=== go: a release build cannot start with dev auth"
-# cmd/docs/devauth_flag_release.go (-tags release) replaces
-# extractDevAuthFlag with a version that never returns true — the
-# strongest closure available for "a -dev-auth flag left in a deploy
-# script": the release binary has no code path that can turn it on,
-# not merely a runtime check that refuses once it is. This step
-# proves both halves: the tagged build compiles at all (a file that
-# only exists under a tag most local runs never pass is otherwise
-# untested for as long as nobody happens to build it), and running
-# that binary with -dev-auth on its command line actually refuses to
-# start rather than silently doing nothing.
-gate_tmp="$(mktemp -d)"
-trap 'rm -rf "$gate_tmp"' EXIT
-CGO_ENABLED=0 go build -tags release -o "$gate_tmp/docs-release" ./cmd/docs
-
-# A release binary that (wrongly) accepted -dev-auth would go on to call
-# rastrillo.Serve and block forever, listening — a hang, not a failure.
-# "if cmd; then" alone cannot tell "refused in 5ms" apart from "still
-# running after 5 seconds": both only reach a branch once something
-# finally kills the process, and a bare invocation with no timeout never
-# would. This check exists specifically to catch that regression, so it
-# must fail loudly the day it fires rather than hang the gate itself — a
-# hang gets retried, a failure gets read. `timeout` turns "still running"
-# into its own distinguishable exit code (124) instead. -db and -addr
-# point at a scratch path and an ephemeral port so a wrongly-accepted
-# -dev-auth run cannot litter the repo with a stray docs.db (it did,
-# once, from the default relative DBPath) or fight over a real port.
-set +e
-timeout 5 "$gate_tmp/docs-release" -dev-auth -db "$gate_tmp/scratch.db" -addr 127.0.0.1:0 \
- >"$gate_tmp/out" 2>&1
-status=$?
-set -e
-if [ "$status" -eq 124 ]; then
- echo "gate: a release build accepted -dev-auth and kept running until killed by timeout — see cmd/docs/devauth_flag_release.go" >&2
- cat "$gate_tmp/out" >&2
- exit 1
-fi
-if [ "$status" -eq 0 ]; then
- echo "gate: a release build accepted -dev-auth and exited 0 — see cmd/docs/devauth_flag_release.go" >&2
- cat "$gate_tmp/out" >&2
- exit 1
-fi
-grep -qi 'dev-auth' "$gate_tmp/out" || {
- echo "gate: release build refused to start, but not because of -dev-auth — investigate:" >&2
- cat "$gate_tmp/out" >&2
- exit 1
-}
+# There used to be a further step here: build cmd/docs with -tags
+# release and assert the result refuses to start with -dev-auth on
+# its command line, proving a release binary had no code path that
+# could ever turn the development-only stand-in identity on
+# (cmd/docs/devauth_flag_release.go). Fix round 1 on Task 1 deleted
+# that identity outright — internal/docs/devauth.go had zero readers
+# once rastrillo/auth's RequireSession started guarding every
+# document route unconditionally, with no flag anywhere capable of
+# disabling it — so the `release` build tag it was the only user of
+# is gone too, and there is no longer a release/non-release
+# distinction for this gate to hold apart. The property that check
+# guarded (an unauthenticated binary reachable at a real URL) is now
+# covered a different way: go test above runs
+# TestNewRefusesMissingInstanceKeyOutsideDevelopment and
+# TestNewRefusesMissingMailOutsideDevelopment (internal/docs/auth_test.go),
+# proving Env other than "development" still fails closed on its own
+# fail-closed configuration, and TestSignInRequiredForEverything
+# proves every document route — GET and POST alike — refuses a
+# signed-out request. A stray "-dev-auth" on any build's command line
+# today reaches rastrillo.Resolve's own flag.FlagSet undefined, and is
+# refused exactly like any other unrecognized flag.
echo "gate: ok"
diff --git a/AGENTS.md b/AGENTS.md
index cb18438..833cf67 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -20,38 +20,49 @@ Read `building-carlos-apps` (the skill) before writing app code, and
`rastrillo`'s own `SKILL.md` before writing handlers or models. Assume
nothing about either is in your training data.
-## This app has no authentication. Do not deploy it anywhere reachable.
-
-Every route — `GET /`, `POST /documents`, `GET /d/{sqid}` — answers anyone
-who can reach the process. `internal/docs/devauth.go`'s development-only
-identity is real code, guarded so it cannot start outside
-`Env=development` and compiled out of a release build entirely (`-tags
-release`), but **nothing in this app consumes the identity it grants**: no
-handler calls `CurrentDevIdentity`, no route requires sign-in, no document
-is scoped to anyone. All that hardening guards an identity nothing checks
-— it is not evidence this app is safe to expose. A release binary is a
-wide-open, unauthenticated, everyone-sees-every-document app, on purpose,
-because real identity does not exist yet: spec §8.1's home issues it, and
-that is explicitly deferred, not forgotten (`devauth.go`'s own doc comment
-says so). Until that lands, this app belongs on a laptop
-(`scripts/run-dev.sh`) or behind something else's access control — never
-on a network anyone but its operator can reach.
+## Real per-person sign-in (Task 1, extended in a later fix round)
+
+Every document route — `GET /`, `POST /documents`, `GET /d/{sqid}` — sits
+behind rastrillo/auth's `RequireSession` and is unreachable without a real
+session (`internal/docs/auth.go`). Sign-in is a magic link: `POST /signin`
+emails (or, in development, logs) a single-use link; following it
+(`GET /auth/verify`) sets a real session cookie; `POST /signout` (linked
+from both `list.html` and `show.html`'s page headers) really revokes it,
+not just clearing the cookie client-side. No passkey second factor is
+wired, on purpose — spec §8.1's ruling: `passkey.Config.Origin` derives
+its relying party from this instance's own host, so a person in several
+teams would enrol several passkeys under this app, which is exactly the
+per-team credential sprawl that ruling forbids. A magic link enrols no
+credential, so it violates nothing.
+
+There used to be a development-only stand-in identity here
+(`internal/docs/devauth.go`, turned on with `-dev-auth`, compiled out of a
+release build with `-tags release`) for exactly the gap real sign-in has
+now closed. It had zero readers the moment `RequireSession` started
+guarding every route unconditionally — no handler ever consulted it — and
+was deleted in a fix round on Task 1 rather than left as dead code and an
+unreachable trap for whoever next assumed `-dev-auth` still did something.
+The `release` build tag it was the only user of is gone with it; there is
+no longer a release/non-release distinction anywhere in this app.
+
+`Config.Env` still exists (`"development"` or `"production"`), but it now
+governs exactly one thing, independent of any identity flag: whether
+`InstanceKey` and `Mailer` (`internal/docs/auth.go`) may default — an
+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.
## Running it locally
-`./scripts/run-dev.sh` builds and runs the app in one command: the
-development-only identity on (`-dev-auth`), a fixed port
-(`localhost:8099`, override with `$DOCS_DEV_PORT`), and a scratch SQLite
-database that starts empty every run. It prints the URL to open.
-
-`-dev-auth` only exists in an ordinary build. `cmd/docs/devauth_flag_release.go`
-(`-tags release`) replaces the flag-parsing function with one that can never
-observe it — a release binary has no code path that can turn the
-development-only identity on at all, not just a runtime check that refuses
-once it is (`internal/docs/devauth.go`'s `guardDevAuth`, which still runs
-either way, for anything else that might one day set `Config.DevAuth`).
-`.amadan/ci.d/10-gate` builds both configurations and asserts the release
-one actually refuses `-dev-auth`.
+`./scripts/run-dev.sh` builds and runs the app in one command: `-env
+development`, a fixed port (`localhost:8099`, override with
+`$DOCS_DEV_PORT`), and a scratch SQLite database that starts empty every
+run. It prints the URL to open, and signing in there gets a real session
+via a magic link logged to the terminal instead of mailed — the same flow
+a real deployment uses, just not actually delivered anywhere.
**Origin is not optional in anything but development — but know what it
actually guards.** rastrillo v0.21.0's `csrf.Protect` checks
diff --git a/cmd/docs/devauth_flag.go b/cmd/docs/devauth_flag.go
deleted file mode 100644
index 16ea4ca..0000000
--- a/cmd/docs/devauth_flag.go
+++ /dev/null
@@ -1,32 +0,0 @@
-//go:build !release
-
-// This file exists so a normal build (`go build ./...`, `go run`,
-// scripts/run-dev.sh — no build tags) can still reach the
-// development-only identity by spelling out -dev-auth on the command
-// line. devauth_flag_release.go is this file's mirror image, built
-// only with -tags release, where the function below does not exist at
-// all — see that file's doc comment for why a build tag is the
-// strongest closure available for this particular hole.
-package main
-
-// extractDevAuthFlag pulls a bare "-dev-auth"/"--dev-auth" token out
-// of args, returning whether it was present and everything else
-// unchanged (order preserved). rastrillo.Resolve parses os.Args[1:]
-// itself, with its own flag.FlagSet that knows only -socket/-addr/-db
-// and errors on anything else — so -dev-auth must be stripped out of
-// os.Args before Resolve ever sees it, or Resolve fails to start
-// outright rather than merely refusing dev auth. This is presence-only
-// (no "-dev-auth=false"): the deliberately narrow, explicit shape the
-// brief asks for — no config default, no env var, nothing a systemd
-// unit's boilerplate could set without someone typing this literal
-// flag.
-func extractDevAuthFlag(args []string) (enabled bool, rest []string) {
- for _, a := range args {
- if a == "-dev-auth" || a == "--dev-auth" {
- enabled = true
- continue
- }
- rest = append(rest, a)
- }
- return enabled, rest
-}
diff --git a/cmd/docs/devauth_flag_release.go b/cmd/docs/devauth_flag_release.go
deleted file mode 100644
index eb1912f..0000000
--- a/cmd/docs/devauth_flag_release.go
+++ /dev/null
@@ -1,55 +0,0 @@
-//go:build release
-
-// A `-dev-auth` flag left in a deploy script or a systemd unit is a
-// residual security hole no in-process check can fully close — by the
-// time guardDevAuth (internal/docs/devauth.go) runs, the flag has
-// already been read and turned into Config.DevAuth true; a Env check
-// at that point catches "development" spelled wrong, not "someone
-// pasted the wrong invocation into a unit file that happens to set
-// Env=production correctly." Moving the closure to build time removes
-// the capability from the binary itself: a release build has no
-// extractDevAuthFlag that can ever observe -dev-auth and return true,
-// so there is no argv, env var, or config file anyone could hand this
-// specific binary that turns dev auth on. `go build -tags release
-// ./cmd/docs` is what the gate (.amadan/ci.d/10-gate) builds and
-// exercises; any real deploy pipeline must build the same way.
-//
-// What this still misses, honestly:
-//
-// 1. It only closes the argv path in *this* binary. Nothing enforces
-// that a deploy actually passes -tags release — a build run
-// without it still gets devauth_flag.go's real implementation.
-// The gate asserts the tagged build behaves correctly; it cannot
-// force a deploy script elsewhere to use it. That is a process
-// control, not a code one.
-// 2. internal/docs' devauth.go (NewWithDevAuth, guardDevAuth,
-// devAuthMiddleware, DevIdentity) is still compiled into the
-// binary either way — this file only removes the one path in
-// main() that could ever set Config.DevAuth true from outside the
-// process. guardDevAuth's runtime Env check (internal/docs/devauth.go)
-// is what still catches any *other* future path into DevAuth true
-// (a new flag added carelessly, a config field set by hand) — the
-// two guards are complementary, not substitutes for each other.
-// 3. The failure mode when someone does pass -dev-auth to a release
-// binary is a generic rastrillo.Resolve flag-parse error ("flag
-// provided but not defined: -dev-auth"), not a custom, friendlier
-// security message — deliberately: keeping this function's shape
-// identical to the non-release build (same signature, same
-// "always return false" contract when there is nothing to enable)
-// is what makes this file safe to review as "does nothing" rather
-// than as new logic that could itself have a bug. The refusal is
-// still loud (nonzero exit, a message naming the flag) — see
-// main.go's own os.Exit(1) on a Resolve error.
-package main
-
-// extractDevAuthFlag is the release build's version: it does not
-// scan args for -dev-auth at all, so `enabled` can never be anything
-// but false and `rest` is always the untouched input — a release
-// binary given "-dev-auth" on its command line has no code path that
-// turns it into Config.DevAuth true; the token simply reaches
-// rastrillo.Resolve's flag.FlagSet, which does not define it, and
-// Resolve fails to start (main.go's os.Exit(1)) exactly like any
-// other unrecognized flag.
-func extractDevAuthFlag(args []string) (enabled bool, rest []string) {
- return false, args
-}
diff --git a/cmd/docs/main.go b/cmd/docs/main.go
index fbf0001..6af97eb 100644
--- a/cmd/docs/main.go
+++ b/cmd/docs/main.go
@@ -12,10 +12,53 @@ import (
"github.com/carlosframework/docs/internal/docs"
)
+// extractEnvFlag pulls "-env value"/"-env=value" (or the "--env"
+// spelling) out of args, returning the value (empty if absent) and
+// everything else unchanged, order preserved. rastrillo.Resolve
+// parses os.Args[1:] itself with its own flag.FlagSet, which knows
+// only -socket/-addr/-db and errors on anything else, so any flag
+// this binary defines on top of that has to be stripped out first —
+// the same reason the now-deleted extractDevAuthFlag existed (fix
+// round 1 on Task 1: -dev-auth's own identity had zero readers once
+// real sign-in landed, and was removed rather than left as a trap;
+// this flag replaces the one thing it was still secretly load-bearing
+// for — see Env's own doc comment on docs.Config).
+func extractEnvFlag(args []string) (env string, rest []string) {
+ for i := 0; i < len(args); i++ {
+ a := args[i]
+ switch {
+ case a == "-env" || a == "--env":
+ if i+1 < len(args) {
+ env = args[i+1]
+ i++
+ }
+ continue
+ case strings.HasPrefix(a, "-env="):
+ env = strings.TrimPrefix(a, "-env=")
+ continue
+ case strings.HasPrefix(a, "--env="):
+ env = strings.TrimPrefix(a, "--env=")
+ continue
+ }
+ rest = append(rest, a)
+ }
+ return env, rest
+}
+
func main() {
logger := slog.Default()
- devAuth, rest := extractDevAuthFlag(os.Args[1:])
+ // -env (or $DOCS_ENV, checked below once flags are parsed)
+ // 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
+ // the now-deleted development-only identity on); fix round 1 on
+ // Task 1 split them apart, because "log mail instead of sending
+ // it" and "who can sign in" were never the same decision, and a
+ // future deletion of an identity flag should never silently take
+ // the mail-logging fallback (and scripts/run-dev.sh) down with
+ // it.
+ envFlag, rest := extractEnvFlag(os.Args[1:])
os.Args = append(os.Args[:1], rest...)
// Resolve applies the platform's activation argv (-socket/-addr/-db,
@@ -81,36 +124,33 @@ 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"
+ }
+
// New opens its own database handle (SKILL.md §1's "app that opens
// its own database"), so opts.DBPath is blanked below before Serve
// — otherwise Serve would open a second connection to the file New
// already owns.
- //
- // Env is "production" unless -dev-auth was given on the command
- // line, in which case NewWithDevAuth both sets Env to
- // "development" and asks for the development-only identity — the
- // only way any of this ever turns on is someone spelling that flag
- // out explicitly; running this binary with no arguments always
- // gets "production", DevAuth false.
cfg := docs.Config{
DBPath: opts.DBPath,
- Env: "production",
+ Env: env,
Origin: origin,
InstanceKey: instanceKey,
Mailer: mailer,
Logger: logger,
}
- var app *docs.App
- if devAuth {
- // NewWithDevAuth itself forces DevAuth true; Env must be set
- // to "development" here too — its guard checks both, and
- // leaving Env at "production" would make -dev-auth refuse to
- // start rather than silently doing nothing.
- cfg.Env = "development"
- app, err = docs.NewWithDevAuth(cfg)
- } else {
- app, err = docs.New(cfg)
- }
+ app, err := docs.New(cfg)
if err != nil {
logger.Error("docs: new", "err", err)
os.Exit(1)
diff --git a/internal/docs/app.go b/internal/docs/app.go
index 9765769..d999b8d 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -35,19 +35,20 @@ type Config struct {
DBPath string
// Env is "development" or "production" ("" defaults to
- // "production"). devauth.go's development-only identity refuses to
- // start unless Env is exactly "development" — see DevAuth.
+ // "production"). It gates two fail-closed defaults below — Origin
+ // and (auth.go) InstanceKey/Mailer — and nothing else: the
+ // development-only stand-in identity that used to live in
+ // devauth.go is gone (fix round 1 on Task 1 — real sign-in made
+ // it dead code the moment RequireSession started guarding every
+ // 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 string
- // DevAuth enables devauth.go's fixed development identity —
- // New's guardDevAuth refuses to start unless Env is exactly
- // "development" too. Prefer NewWithDevAuth (devauth.go) over
- // setting this by hand; it exists as its own Config field, rather
- // than being threaded through only that constructor, so
- // NewForTest — which four later tasks' tests depend on — never
- // needed a signature change once devauth.go landed.
- DevAuth bool
-
// Origin is this app's own origin (scheme://host[:port], no path),
// what csrf.Protect (rastrillo SKILL.md §5) uses as its fallback
// same-origin evidence. Fix round 3 corrected a wrong model of
@@ -199,8 +200,6 @@ var BootSchema = migrate.Merge(sessions.Schema, authSchema, Schema)
// *http.ServeMux so httptest exercises the real router, csrf.Protect
// mounted app-wide (SKILL.md §5), rastrillo/auth (auth.go) guarding
// every document route behind a real, per-person magic-link sign-in.
-// devauth.go's guardDevAuth runs first, so a misconfigured
-// Config.DevAuth refuses before any of the above opens.
func New(cfg Config) (*App, error) {
logger := cfg.Logger
if logger == nil {
@@ -218,9 +217,9 @@ func New(cfg Config) (*App, error) {
// effect is on csrf.Protect's legacy Origin/Referer fallback path
// (Config.Origin's own doc comment explains why that path, not
// "every request", is what a wrong string actually touches). This
- // is the same fail-closed shape as guardDevAuth below: exactly one
- // string ("development") is ever treated as "safe to default",
- // checked before anything else opens.
+ // is the same fail-closed shape InstanceKey and Mailer use below
+ // (auth.go): exactly one string ("development") is ever treated
+ // as "safe to default", checked before anything else opens.
//
// cfg.Origin == "" alone is not the right test: a whitespace value
// like $DOCS_ORIGIN=" " is non-empty, so it would sail past that
@@ -248,24 +247,8 @@ func New(cfg Config) (*App, error) {
cfg.Origin = trimmedOrigin
}
- // Checked before anything else opens — including the database —
- // so a misconfigured DevAuth refuses before New has any side
- // effect to unwind. See devauth.go: this is the single choke
- // point every path to the development-only identity passes
- // through, NewWithDevAuth and NewForTest included.
- if err := guardDevAuth(cfg); err != nil {
- return nil, err
- }
- if cfg.DevAuth {
- // Loud on purpose: an operator watching boot logs should
- // never be able to miss this. It does not make the guard
- // above any less necessary — logging is a tripwire, not a
- // control.
- logger.Warn("docs: development-only identity ENABLED — refuses to start outside Env=development", "env", cfg.Env)
- }
-
// Resolved before anything else opens — same fail-closed shape as
- // Origin and DevAuth above: a deployment that cannot seal a
+ // Origin above: a deployment that cannot seal a
// pending link, or cannot send one, refuses to start rather than
// booting into a broken or (worse) silently-logging-credentials
// sign-in. See auth.go.
@@ -325,14 +308,6 @@ func New(cfg Config) (*App, error) {
// nothing to "/" or "/d/{sqid}" and everything to "/documents",
// "/signin" and "/signout".
r.Use(csrf.Protect(cfg.Origin))
- if cfg.DevAuth {
- // Guarded above: this only ever mounts when guardDevAuth has
- // already confirmed Env == "development". devAuthMiddleware
- // stamps a fixed dev identity on the request context; it is
- // not a session, and RequireSession below neither reads nor
- // trusts it — see devauth.go and auth.go's own doc comments.
- r.Use(devAuthMiddleware)
- }
// Unauthenticated: the sign-in page and its four handlers, and the
// static assets that page (and the shell around it) needs to
// render at all.
diff --git a/internal/docs/auth.go b/internal/docs/auth.go
index ecb0416..8aa9a37 100644
--- a/internal/docs/auth.go
+++ b/internal/docs/auth.go
@@ -54,7 +54,7 @@ const devInstanceKey = "docs-development-instance-key-not-a-secret"
// rule (see its doc comment on Config): non-empty always wins; empty
// is fine only in development, where it defaults to devInstanceKey;
// anywhere else it is a startup refusal, the same shape New already
-// uses for Origin and DevAuth.
+// uses for Origin.
func resolveInstanceKey(cfg Config) (string, error) {
key := strings.TrimSpace(cfg.InstanceKey)
if key != "" {
@@ -71,7 +71,7 @@ func resolveInstanceKey(cfg Config) (string, error) {
// development, where it falls back to rastrillo/mail's logging
// sender; anywhere else nil is a startup refusal — a sign-in that
// cannot send its link is not sign-in, so this is the same fail-closed
-// shape New already uses for Origin and DevAuth, not merely a warning.
+// shape New already uses for Origin, not merely a warning.
//
// In development the chosen sender is always wrapped in a
// *recordingMailer, whether or not Config.Mailer was already set:
diff --git a/internal/docs/auth_test.go b/internal/docs/auth_test.go
index 54eacfa..c4a84d6 100644
--- a/internal/docs/auth_test.go
+++ b/internal/docs/auth_test.go
@@ -3,15 +3,24 @@ package docs_test
import (
"net/http"
"net/http/httptest"
+ "strings"
"testing"
"github.com/carlosframework/docs/internal/docs"
)
-// TestSignInRequiredForEverything is the brief's own literal test:
-// every document route now sits behind rastrillo/auth's
-// RequireSession, so a signed-out request must redirect to sign in
-// rather than reach the handler.
+// TestSignInRequiredForEverything is the brief's own literal test,
+// extended per fix round 1: a create endpoint reachable without a
+// session is the entire problem this task exists to solve, and a
+// "does the list require sign-in?" GET-only check reads straight
+// past it — a route moved out of the RequireSession group by a
+// future refactor would go undetected here otherwise. POST
+// /documents is sent with Sec-Fetch-Site: same-origin so csrf.Protect
+// (mounted above RequireSession in the middleware chain) lets it
+// through, isolating what actually refuses it: RequireSession itself,
+// not the CSRF check. A signed-out GET/HEAD redirects to sign-in;
+// anything else — POST included — 403s (rastrillo/auth's own
+// refuseSession, docs/site's own contract).
func TestSignInRequiredForEverything(t *testing.T) {
app, err := docs.NewForTest(t)
if err != nil {
@@ -25,6 +34,16 @@ func TestSignInRequiredForEverything(t *testing.T) {
t.Errorf("GET %s signed out: status %d, want a redirect to sign-in", path, rec.Code)
}
}
+
+ form := strings.NewReader("title=Should+not+be+created")
+ req := httptest.NewRequest(http.MethodPost, "/documents", form)
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.Header.Set("Sec-Fetch-Site", "same-origin")
+ rec := httptest.NewRecorder()
+ app.Handler().ServeHTTP(rec, req)
+ if rec.Code != http.StatusForbidden {
+ t.Errorf("POST /documents signed out: status %d, want %d (RequireSession's own refusal, not a redirect)", rec.Code, http.StatusForbidden)
+ }
}
// TestMagicLinkSignsSomebodyIn is the brief's own literal test:
diff --git a/internal/docs/devauth.go b/internal/docs/devauth.go
deleted file mode 100644
index 65a6694..0000000
--- a/internal/docs/devauth.go
+++ /dev/null
@@ -1,81 +0,0 @@
-// This file exists only because spec §8.1's home — the shared
-// credential origin that issues a signed identity assertion — does
-// not exist yet; it hands every request one fixed development
-// identity purely so the app is reachable in a browser, and it is
-// deleted the day this app verifies a real signed assertion instead.
-package docs
-
-import (
- "context"
- "fmt"
- "net/http"
-)
-
-// devSubject is the one identity devauth ever grants. Fixed rather
-// than configurable: a real identity is exactly what this stands in
-// for, so giving it knobs would just be building a worse version of
-// the thing it replaces.
-const devSubject = "dev"
-
-// DevIdentity is what devAuthMiddleware attaches to a request's
-// context — the shape spec §8.1's verified assertion will eventually
-// fill in for real.
-type DevIdentity struct {
- Subject string
-}
-
-type devIdentityKey struct{}
-
-// CurrentDevIdentity returns the identity devAuthMiddleware attached
-// to r, if any. Nothing in this app requires a signed-in viewer yet,
-// so no handler calls this today — it exists so a later task can ask
-// "is anyone signed in" without reaching into this file's internals,
-// and it goes away with the rest of this file once §8.1 lands.
-func CurrentDevIdentity(r *http.Request) (DevIdentity, bool) {
- id, ok := r.Context().Value(devIdentityKey{}).(DevIdentity)
- return id, ok
-}
-
-// devAuthMiddleware stamps every request with the fixed development
-// identity. New wires this in only after guardDevAuth has approved
-// cfg — see New and NewWithDevAuth below.
-func devAuthMiddleware(next http.Handler) http.Handler {
- return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- ctx := context.WithValue(r.Context(), devIdentityKey{}, DevIdentity{Subject: devSubject})
- next.ServeHTTP(w, r.WithContext(ctx))
- })
-}
-
-// guardDevAuth is the single choke point every path to the
-// development identity must pass through — New calls it directly, so
-// it applies whether an app is built via NewWithDevAuth, via New
-// itself with DevAuth set by hand, or via NewForTest.
-//
-// Fail closed: cfg.DevAuth false is always fine (there is nothing to
-// guard against), but cfg.DevAuth true demands cfg.Env be exactly
-// "development" — no allowlist of "development-like" strings to fall
-// out of, so an unrecognised value, a typo, or empty (New defaults ""
-// to "production" before this runs) all refuse identically to a
-// deliberate "production". There is exactly one string that is ever
-// permitted.
-func guardDevAuth(cfg Config) error {
- if !cfg.DevAuth {
- return nil
- }
- if cfg.Env != "development" {
- return fmt.Errorf("docs: DevAuth requires Env %q to start development-only, insecure identity; got Env %q — refusing to start", "development", cfg.Env)
- }
- return nil
-}
-
-// NewWithDevAuth is New, named for the one call site that means to
-// opt into the development-only identity: cmd/docs/main.go's explicit
-// -dev-auth flag. It is not a second code path — it forces
-// cfg.DevAuth true and calls New unchanged, so the guard that
-// actually enforces "development only" (guardDevAuth, run inside New)
-// lives in exactly one place no matter which name a caller reaches it
-// through.
-func NewWithDevAuth(cfg Config) (*App, error) {
- cfg.DevAuth = true
- return New(cfg)
-}
diff --git a/internal/docs/devauth_test.go b/internal/docs/devauth_test.go
deleted file mode 100644
index 0d30ca4..0000000
--- a/internal/docs/devauth_test.go
+++ /dev/null
@@ -1,80 +0,0 @@
-package docs_test
-
-import (
- "net/http"
- "net/http/httptest"
- "strings"
- "testing"
-
- "github.com/carlosframework/docs/internal/docs"
-)
-
-// TestDevAuthRefusesOutsideDevelopment is the plan's own literal test:
-// the guard is the point of this task — a build without the dev flag
-// must refuse to start rather than silently authenticating everyone.
-func TestDevAuthRefusesOutsideDevelopment(t *testing.T) {
- _, err := docs.NewWithDevAuth(docs.Config{DevAuth: true, Env: "production"})
- if err == nil {
- t.Fatal("dev auth must refuse to start outside development")
- }
- if !strings.Contains(err.Error(), "development") {
- t.Errorf("error should say why: %v", err)
- }
-}
-
-// TestDevAuthGivesAnIdentityInDevelopment is the plan's own literal
-// test: NewForTest fixes Env at "development" with DevAuth enabled, so
-// a request must not come back unauthorized.
-func TestDevAuthGivesAnIdentityInDevelopment(t *testing.T) {
- app, err := docs.NewForTest(t) // NewForTest sets Env: development
- if err != nil {
- t.Fatal(err)
- }
- req := httptest.NewRequest(http.MethodGet, "/", nil)
- rec := httptest.NewRecorder()
- app.Handler().ServeHTTP(rec, req)
- if rec.Code == http.StatusUnauthorized {
- t.Error("dev auth should provide an identity in development")
- }
-}
-
-// TestDevAuthRefusesEmptyEnv proves the fail-closed default: an empty
-// Env must not be treated as "development-like". New defaults "" to
-// "production" before the guard runs, so this exercises that the
-// default itself is refused, not just an explicit "production".
-func TestDevAuthRefusesEmptyEnv(t *testing.T) {
- _, err := docs.NewWithDevAuth(docs.Config{DevAuth: true, Env: ""})
- if err == nil {
- t.Fatal("dev auth must refuse to start with an empty Env")
- }
- if !strings.Contains(err.Error(), "development") {
- t.Errorf("error should say why: %v", err)
- }
-}
-
-// TestDevAuthRefusesTypoedEnv proves there is no allowlist to fall out
-// of loosely — only the exact string "development" is ever permitted,
-// so a near-miss (a plausible env-var typo) refuses exactly like a
-// deliberate "production" rather than being treated as close enough.
-func TestDevAuthRefusesTypoedEnv(t *testing.T) {
- _, err := docs.NewWithDevAuth(docs.Config{DevAuth: true, Env: "developement"})
- if err == nil {
- t.Fatal("dev auth must refuse to start with a typo'd Env")
- }
- if !strings.Contains(err.Error(), "development") {
- t.Errorf("error should say why: %v", err)
- }
-}
-
-// TestDevAuthRefusesViaNewDirectly proves the guard is a single choke
-// point: calling New itself with DevAuth set by hand, bypassing
-// NewWithDevAuth entirely, refuses exactly the same way.
-func TestDevAuthRefusesViaNewDirectly(t *testing.T) {
- _, err := docs.New(docs.Config{DevAuth: true, Env: "production"})
- if err == nil {
- t.Fatal("New itself must refuse DevAuth outside development, not just NewWithDevAuth")
- }
- if !strings.Contains(err.Error(), "development") {
- t.Errorf("error should say why: %v", err)
- }
-}
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go
index 839be23..69fdfd0 100644
--- a/internal/docs/handlers.go
+++ b/internal/docs/handlers.go
@@ -32,6 +32,15 @@ type handlers struct {
// reaches a person with no documents yet exactly as it reaches
// everyone else (fix round 1: the original version of this screen
// had no path to POST /documents at all outside a hand-typed curl).
+//
+// SignOutLabel backs the sign-out control list.html's header also
+// carries — added in the fix round that deleted the development-only
+// stand-in identity: POST /signout (auth.go) always existed and
+// worked, but nothing on any page linked to it, which matters once
+// this is reachable at a real URL rather than only ever behind a
+// laptop's own access. A session lasts 30 days
+// (rastrillo/auth.DefaultSessionTTL); a page with no way to end one
+// on purpose is a real gap on something public, not a cosmetic one.
type listPage struct {
Title string
Groups []docGroup
@@ -39,6 +48,7 @@ type listPage struct {
EmptyBody string
CreateLabel string
CreatePlaceholder string
+ SignOutLabel string
}
// docGroup is one recency bucket ("Today", "This week", ...) and the
@@ -85,6 +95,7 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) {
EmptyBody: rastrillo.T(r, "docs.list.empty.body"),
CreateLabel: rastrillo.T(r, "docs.list.create.label"),
CreatePlaceholder: rastrillo.T(r, "docs.list.create.placeholder"),
+ SignOutLabel: rastrillo.T(r, "docs.signout.label"),
}
if err := h.render.page(w, r, "list", data); err != nil {
h.logger.Error("docs: render list", "err", err)
@@ -124,8 +135,9 @@ func (h *handlers) create(w http.ResponseWriter, r *http.Request) {
// replays to "", which views/show.html must render as an ordinary
// empty canvas rather than erroring.
type showPage struct {
- Title string
- Text string
+ Title string
+ Text string
+ SignOutLabel string // see listPage.SignOutLabel's own doc comment
}
// show serves GET /d/{sqid}: the document read-only, its title and
@@ -157,7 +169,7 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) {
return
}
- data := showPage{Title: doc.Title, Text: text}
+ data := showPage{Title: doc.Title, Text: text, SignOutLabel: rastrillo.T(r, "docs.signout.label")}
if err := h.render.page(w, r, "show", data); err != nil {
h.logger.Error("docs: render show", "err", err)
http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
diff --git a/internal/docs/render.go b/internal/docs/render.go
index 8d67a75..67c481d 100644
--- a/internal/docs/render.go
+++ b/internal/docs/render.go
@@ -80,6 +80,11 @@ var Catalog = rastrillo.Catalog{
"docs.signin.error.rate": "Too many attempts. Wait a few minutes and try again.",
"docs.signin.error.address": "That doesn't look like a valid email address.",
"docs.signin.error.expired": "That link has expired or was already used. Enter your email below for a new one.",
+
+ // docs.signout.label is the sign-out control's text (list.html's
+ // and show.html's page headers) — both its button label and its
+ // accessible name.
+ "docs.signout.label": "Sign out",
}
// Locales and DefaultLocale declare this app's locale set for
diff --git a/internal/docs/testing.go b/internal/docs/testing.go
index c6e052f..6fa4c4e 100644
--- a/internal/docs/testing.go
+++ b/internal/docs/testing.go
@@ -16,18 +16,19 @@ import (
// _test.go file, so packages other than this one's own tests can reach
// it too.
//
-// Env is fixed at "development" with DevAuth enabled: a later task
-// (devauth.go) adds a development-only identity that refuses to start
-// outside development, and every test built on NewForTest — this
-// task's own and four later tasks' — assumes that identity is already
-// reachable rather than needing its own opt-in.
+// Env is fixed at "development": that is what lets InstanceKey and
+// Mailer (auth.go) default rather than refuse to start, and what
+// installs the *recordingMailer TestSignIn (below) reads a minted
+// link back from. It used to also enable a development-only stand-in
+// identity (devauth.go); that identity had zero readers once real
+// sign-in landed and was deleted in fix round 1 on Task 1 — every
+// test built on NewForTest signs in for real via TestSignIn now.
func NewForTest(t *testing.T) (*App, error) {
t.Helper()
app, err := New(Config{
- DBPath: filepath.Join(t.TempDir(), "docs.db"),
- Env: "development",
- DevAuth: true,
+ DBPath: filepath.Join(t.TempDir(), "docs.db"),
+ Env: "development",
})
if err != nil {
return nil, err
diff --git a/internal/docs/views/list.html b/internal/docs/views/list.html
index fd1167f..d1565f5 100644
--- a/internal/docs/views/list.html
+++ b/internal/docs/views/list.html
@@ -3,16 +3,25 @@
through rastrillo.T, EmptyTitle/EmptyBody the same; a row's Title
and When are the document's own data and are printed as-is.
- The header's <form> is the app's one create control (fix round
- 1): a plain POST to /documents, no JavaScript, sitting beside the
- titles the same way page-header's own single-link ActionHref
- would — [rst-page-header]'s flex/space-between rule lays it out
- as the second flex child without any markup of its own. It is
- outside the {{if .Groups}} branch below on purpose: the header
+ The header's create <form> is the app's one create control (fix
+ round 1 on Task 1): a plain POST to /documents, no JavaScript. It
+ is outside the {{if .Groups}} branch below on purpose: the header
renders unconditionally, so the control reaches a person with no
documents yet exactly the same way it reaches everyone else,
rather than only appearing once there is something to list.
+ [docs-header-actions] wraps that create form and the sign-out
+ form (POST /signout, auth.go) as one group — added in a later fix
+ round once real sign-in existed and nothing on the page could end
+ one: [rst-page-header]'s flex/space-between rule wants at most two
+ top-level children (titles, one action area), so both controls
+ live inside this one wrapper rather than becoming a third
+ top-level child of the header itself. Sign-out is a second,
+ separate <form>, not a second button in the create form, because
+ they are two unrelated actions with two different targets
+ (/documents vs /signout) and must never be merged into one
+ submit.
+
Writes the bare rst-* attribute grammar throughout — the rst-
vocabulary always as a bare attribute, never as a class — which is
this app's whole-screen markup convention (Task 1's home page set
@@ -55,11 +64,19 @@
color: var(--rst-text-muted);
font-size: var(--rst-fs-sm);
}
+ /* [docs-header-actions]: the header's one flex child opposite the
+ titles — [rst-page-header]'s space-between rule places it there
+ with no layout rule of its own beyond this; everything it holds
+ (the create form, the sign-out form) lays out as one inline
+ group inside it. */
+ [docs-header-actions] {
+ display: flex;
+ flex-wrap: wrap;
+ align-items: center;
+ gap: var(--rst-sp-3);
+ }
/* [docs-create-form]: the title field and its submit button as one
- inline row, matching page-header's own single-action slot — it is
- the header's second flex child, so [rst-page-header]'s
- space-between rule places it opposite the titles with no layout
- rule of its own beyond this. */
+ inline row. */
[docs-create-form] {
display: flex;
flex-wrap: wrap;
@@ -74,10 +91,15 @@
<div rst-page-header-titles>
<h1>{{.Title}}</h1>
</div>
- <form docs-create-form method="post" action="/documents">
- <input rst-input type="text" name="title" placeholder="{{.CreatePlaceholder}}" aria-label="{{.CreateLabel}}" autocomplete="off">
- <button rst-btn="primary" type="submit">{{.CreateLabel}}</button>
- </form>
+ <div docs-header-actions>
+ <form docs-create-form method="post" action="/documents">
+ <input rst-input type="text" name="title" placeholder="{{.CreatePlaceholder}}" aria-label="{{.CreateLabel}}" autocomplete="off">
+ <button rst-btn="primary" type="submit">{{.CreateLabel}}</button>
+ </form>
+ <form method="post" action="/signout">
+ <button rst-btn="ghost" type="submit">{{.SignOutLabel}}</button>
+ </form>
+ </div>
</header>
{{if .Groups}}
<div rst-list>
diff --git a/internal/docs/views/show.html b/internal/docs/views/show.html
index d7584e5..443710e 100644
--- a/internal/docs/views/show.html
+++ b/internal/docs/views/show.html
@@ -75,6 +75,15 @@
<div rst-page-header-titles>
<h1>{{.Title}}</h1>
</div>
+ {{/* Sign-out control (POST /signout, auth.go) — same reasoning as
+ list.html's own [docs-header-actions] comment: a session lasts
+ 30 days and this page needs a working way to end one on
+ purpose, not just list.html. A single form here is its own
+ header's one flex child, so no wrapper is needed the way
+ list.html's create form forced one. */}}
+ <form method="post" action="/signout">
+ <button rst-btn="ghost" type="submit">{{.SignOutLabel}}</button>
+ </form>
</header>
<div docs-canvas>
<div docs-page>
diff --git a/scripts/run-dev.sh b/scripts/run-dev.sh
index b560598..9d399a6 100755
--- a/scripts/run-dev.sh
+++ b/scripts/run-dev.sh
@@ -1,14 +1,21 @@
#!/usr/bin/env bash
-# One command to see the app in a browser: build it with the
-# development-only identity (internal/docs/devauth.go) turned on,
-# against a scratch database that starts empty every run, on a fixed
-# local port so the URL below is always the same one to open.
+# One command to see the app in a browser: build it with -env
+# development (cmd/docs/main.go's -env flag — see docs.Config.Env's
+# own doc comment for why this is a flag of its own rather than
+# riding on some other one), against a scratch database that starts
+# empty every run, on a fixed local port so the URL below is always
+# the same one to open.
#
-# Never for anything but a laptop. -dev-auth only exists in a normal
-# build — cmd/docs/devauth_flag_release.go compiles it out entirely
-# with `-tags release`, which is what an actual deploy must build
-# instead; see that file for what a build tag closes and what it does
-# not.
+# Never for anything but a laptop: -env development is what lets an
+# empty $DOCS_INSTANCE_KEY and unconfigured mail default instead of
+# refusing to start (internal/docs/auth.go), and what makes a magic
+# link land in this process's own log instead of a real mailbox —
+# fine for a throwaway local database, never for anything real. There
+# used to also be a development-only stand-in identity gated the same
+# way (internal/docs/devauth.go, turned on with -dev-auth); it had
+# zero readers once real sign-in landed and was deleted in fix round
+# 1 on Task 1 — this script now signs in for real, the same magic
+# link a real deployment would use, just logged instead of mailed.
set -euo pipefail
cd "$(dirname "$0")/.."
@@ -56,4 +63,4 @@ echo "docs: http://${addr}/"
# rastrillo.Serve's own SIGTERM/SIGINT drain runs instead of orphaning
# a listener nothing can reach or clean up afterward. See the comment
# above the trap for what this costs.
-exec "${scratch}/docs" -dev-auth -addr "${addr}" -db "${db}"
+exec "${scratch}/docs" -env development -addr "${addr}" -db "${db}"