amadan
app: fix round 1 -- the gate fails instead of hanging, a whitespace origin can't slip past, run-dev.sh doesn't orphan the server
Three findings from review, all real: The gate's dev-auth regression check had no timeout: a release build that (wrongly) accepted -dev-auth would call rastrillo.Serve and block forever, so the check meant to catch that regression would hang the gate itself rather than fail it -- worse than no check, since a hang gets retried and a failure gets read. It also defaulted to docs.db in the repo root. Added `timeout 5`, a scratch -db, and an ephemeral -addr, and distinguish "timed out" (124, the vulnerability is present) from a normal refusal. Verified by reverting devauth_flag_release.go to the vulnerable version, confirming the gate now fails loudly in ~60s with its own message, and restoring it. internal/docs/app.go's empty-Origin refusal tested cfg.Origin == "", so a whitespace-only $DOCS_ORIGIN slipped past it: the app would boot looking healthy and then 403 every request forever, failing safe but silently and late. New now trims before testing for blank, and validates any non-blank value as an absolute http(s) origin with a host and nothing else (no path/query/fragment/userinfo) via a new validateOrigin, refusing at startup with an error naming what was wrong. New tests in origin_test.go and app_test.go. scripts/run-dev.sh ran the server as a plain subprocess, so its own PID and the server's differed -- killing `./scripts/run-dev.sh &` (the pattern the script itself invites) deleted the scratch dir via the EXIT trap while orphaning a server still holding the port. Now execs the binary so the wrapper's PID is the server's PID and a signal reaches it directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
5 files changed,
+213
−7
.amadan/ci.d/10-gate+25 −2internal/docs/app.go+60 −4internal/docs/app_test.go+47 −0internal/docs/origin_test.go+61 −0scripts/run-dev.sh+20 −1
CI passed — run details
diff --git a/.amadan/ci.d/10-gate b/.amadan/ci.d/10-gate| index 96728d9..2f8d83d 100755 |
| --- a/.amadan/ci.d/10-gate |
| +++ b/.amadan/ci.d/10-gate |
| @@ -93,8 +93,31 @@ echo "=== go: a release build cannot start with dev auth" |
| gate_tmp="$(mktemp -d)" |
| trap 'rm -rf "$gate_tmp"' EXIT |
| CGO_ENABLED=0 go build -tags release -o "$gate_tmp/docs-release" ./cmd/docs |
| -if "$gate_tmp/docs-release" -dev-auth >"$gate_tmp/out" 2>&1; then |
| - echo "gate: a release build accepted -dev-auth and started — see cmd/docs/devauth_flag_release.go" >&2 |
| + |
| +# 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 |
diff --git a/internal/docs/app.go b/internal/docs/app.go| index bf84fbf..2d84a23 100644 |
| --- a/internal/docs/app.go |
| +++ b/internal/docs/app.go |
| @@ -16,6 +16,8 @@ import ( |
| "fmt" |
| "log/slog" |
| "net/http" |
| + "net/url" |
| + "strings" |
| "github.com/carlosframework/rastrillo/csrf" |
| "github.com/carlosframework/rastrillo/db" |
| @@ -64,6 +66,12 @@ type Config struct { |
| // leaving $DOCS_ORIGIN unset must fail loudly rather than quietly |
| // protecting the app with the wrong origin — that is exactly what |
| // the Env-gated default below refuses to do. |
| + // |
| + // A non-empty value is trimmed and then validated (validateOrigin) |
| + // rather than trusted as-is: whitespace-only is not "empty" by a |
| + // bare == "" check, and would otherwise sail past the guard above |
| + // and boot an app that rejects every request forever rather than |
| + // refusing to start. |
| Origin string |
| // Logger defaults to slog.Default(). |
| @@ -76,6 +84,36 @@ type Config struct { |
| // does not have to duplicate the literal. |
| const DefaultOrigin = "http://localhost:8080" |
| +// validateOrigin reports whether origin (already trimmed by the |
| +// caller — New) is something csrf.Protect can actually use: an |
| +// absolute http(s) URL naming a scheme and a host and nothing else. |
| +// "Nothing else" is checked deliberately, not just "parses as a |
| +// URL" — url.Parse happily accepts a bare path, a value carrying a |
| +// query string, or one with embedded userinfo, none of which is a |
| +// same-origin check has any business being handed, and none of which |
| +// would ever equal a real browser's Origin header, so an app that |
| +// accepted one would boot looking healthy and then reject every |
| +// request rather than refusing to start. |
| +func validateOrigin(origin string) error { |
| + if origin == "" { |
| + return errors.New("origin is blank") |
| + } |
| + u, err := url.Parse(origin) |
| + if err != nil { |
| + return fmt.Errorf("origin %q does not parse as a URL: %w", origin, err) |
| + } |
| + if u.Scheme != "http" && u.Scheme != "https" { |
| + return fmt.Errorf("origin %q must have an http or https scheme", origin) |
| + } |
| + if u.Host == "" { |
| + return fmt.Errorf("origin %q has no host", origin) |
| + } |
| + if u.Path != "" || u.RawQuery != "" || u.Fragment != "" || u.User != nil { |
| + return fmt.Errorf("origin %q must be scheme://host[:port] only, with no path, query, fragment or userinfo", origin) |
| + } |
| + return nil |
| +} |
| + |
| // App is the CARLOS Docs rastrillo app. |
| type App struct { |
| cfg Config |
| @@ -119,11 +157,29 @@ func New(cfg Config) (*App, error) { |
| // fail-closed shape as guardDevAuth below: exactly one string |
| // ("development") is ever treated as "safe to default", checked |
| // before anything else opens. |
| - if cfg.Origin == "" { |
| - if cfg.Env != "development" { |
| - return nil, fmt.Errorf("docs: Config.Origin is empty and Env is %q, not %q — refusing to start with an unset origin; set $DOCS_ORIGIN to this instance's real origin", cfg.Env, "development") |
| - } |
| + // |
| + // cfg.Origin == "" alone is not the right test: a whitespace value |
| + // like $DOCS_ORIGIN=" " is non-empty, so it would sail past that |
| + // check, and csrf.Protect would then compare every request's real |
| + // Origin/Referer against literal whitespace — never matching, |
| + // which means the app boots looking healthy and 403s all traffic, |
| + // with or without a correct Origin header. That fails safe, but |
| + // silently and late (at request time), not at boot where a |
| + // malformed origin belongs. validateOrigin below rejects that and |
| + // any other value that cannot possibly be what csrf.Protect needs |
| + // (no scheme, no host, a stray path/query/fragment/userinfo) |
| + // before New ever returns a listening app. |
| + trimmedOrigin := strings.TrimSpace(cfg.Origin) |
| + switch { |
| + case trimmedOrigin == "" && cfg.Env != "development": |
| + return nil, fmt.Errorf("docs: Config.Origin is empty and Env is %q, not %q — refusing to start with an unset origin; set $DOCS_ORIGIN to this instance's real origin", cfg.Env, "development") |
| + case trimmedOrigin == "": |
| cfg.Origin = DefaultOrigin |
| + default: |
| + if err := validateOrigin(trimmedOrigin); err != nil { |
| + return nil, fmt.Errorf("docs: Config.Origin is invalid: %w", err) |
| + } |
| + cfg.Origin = trimmedOrigin |
| } |
| // Checked before anything else opens — including the database — |
diff --git a/internal/docs/app_test.go b/internal/docs/app_test.go| index 63946e5..9042c61 100644 |
| --- a/internal/docs/app_test.go |
| +++ b/internal/docs/app_test.go |
| @@ -3,6 +3,8 @@ package docs_test |
| import ( |
| "net/http" |
| "net/http/httptest" |
| + "path/filepath" |
| + "strings" |
| "testing" |
| "github.com/carlosframework/docs/internal/docs" |
| @@ -24,3 +26,48 @@ func TestRootRendersAPage(t *testing.T) { |
| t.Error("no Content-Type on the response") |
| } |
| } |
| + |
| +// TestNewRefusesWhitespaceOrigin is fix round 1's own regression test |
| +// at the New boundary (origin_test.go covers validateOrigin directly, |
| +// package-internal): $DOCS_ORIGIN=" " is non-empty, so a bare |
| +// cfg.Origin == "" check would let it through New and boot an app |
| +// that 403s every request forever instead of refusing to start. New |
| +// trims before testing for blank, so whitespace-only collapses onto |
| +// the same "empty origin" refusal an actually-empty Origin gets — |
| +// still a startup refusal either way, which is the property that |
| +// matters here, not which of the two error messages it is. |
| +func TestNewRefusesWhitespaceOrigin(t *testing.T) { |
| + _, err := docs.New(docs.Config{ |
| + DBPath: filepath.Join(t.TempDir(), "docs.db"), |
| + Env: "production", |
| + Origin: " ", |
| + }) |
| + if err == nil { |
| + t.Fatal("New must refuse a whitespace-only Origin") |
| + } |
| + if !strings.Contains(err.Error(), "empty") { |
| + t.Errorf("error should say the origin is empty: %v", err) |
| + } |
| +} |
| + |
| +// TestNewRefusesMalformedOrigin covers the other ways a non-blank |
| +// Origin can still be unusable — no scheme, no host, or a URL |
| +// carrying more than scheme://host — at the same New boundary. |
| +func TestNewRefusesMalformedOrigin(t *testing.T) { |
| + for _, origin := range []string{ |
| + "not a url at all", |
| + "localhost:8080", |
| + "http://example.com/path", |
| + } { |
| + t.Run(origin, func(t *testing.T) { |
| + _, err := docs.New(docs.Config{ |
| + DBPath: filepath.Join(t.TempDir(), "docs.db"), |
| + Env: "production", |
| + Origin: origin, |
| + }) |
| + if err == nil { |
| + t.Fatalf("New must refuse malformed origin %q", origin) |
| + } |
| + }) |
| + } |
| +} |
diff --git a/internal/docs/origin_test.go b/internal/docs/origin_test.go| new file mode 100644 |
| index 0000000..26ca15d |
| --- /dev/null |
| +++ b/internal/docs/origin_test.go |
| @@ -0,0 +1,61 @@ |
| +package docs |
| + |
| +import "testing" |
| + |
| +// TestValidateOriginRejectsWhitespace is fix round 1's own regression |
| +// test: DOCS_ORIGIN=" " (all whitespace) is non-empty, so a bare |
| +// cfg.Origin == "" check in New lets it through, and csrf.Protect then |
| +// compares every request's real Origin/Referer against literal |
| +// whitespace — never matching, so the app boots looking healthy and |
| +// 403s all traffic instead of refusing to start. validateOrigin must |
| +// catch it (New trims before calling this, but the function itself |
| +// stays correct against an untrimmed caller too). |
| +func TestValidateOriginRejectsWhitespace(t *testing.T) { |
| + for _, origin := range []string{"", " ", "\t\n "} { |
| + if err := validateOrigin(origin); err == nil { |
| + t.Errorf("validateOrigin(%q) = nil, want an error", origin) |
| + } |
| + } |
| +} |
| + |
| +// TestValidateOriginRejectsMalformed covers the other ways a |
| +// non-blank string can still be unusable as csrf.Protect's |
| +// comparison origin: no scheme, the wrong scheme, no host, and a URL |
| +// carrying more than scheme://host (a path, a query, a fragment, |
| +// embedded userinfo) — none of which a real browser's Origin header |
| +// would ever equal. |
| +func TestValidateOriginRejectsMalformed(t *testing.T) { |
| + cases := []string{ |
| + "not a url at all", |
| + "localhost:8080", // no recognized scheme |
| + "ftp://example.com", // wrong scheme |
| + "http://", // no host |
| + "http://example.com/path", // path |
| + "http://example.com?x=1", // query |
| + "http://example.com#frag", // fragment |
| + "http://user:pass@example.com", // userinfo |
| + } |
| + for _, origin := range cases { |
| + t.Run(origin, func(t *testing.T) { |
| + if err := validateOrigin(origin); err == nil { |
| + t.Errorf("validateOrigin(%q) = nil, want an error", origin) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestValidateOriginAcceptsGoodOrigins proves the check above is not |
| +// so strict it rejects the values this app actually uses: |
| +// DefaultOrigin, an -addr-derived origin, and a real platform origin |
| +// with a port. |
| +func TestValidateOriginAcceptsGoodOrigins(t *testing.T) { |
| + for _, origin := range []string{ |
| + DefaultOrigin, |
| + "http://localhost:8099", |
| + "https://docs.bds.oncarlos.com", |
| + } { |
| + if err := validateOrigin(origin); err != nil { |
| + t.Errorf("validateOrigin(%q) = %v, want nil", origin, err) |
| + } |
| + } |
| +} |
diff --git a/scripts/run-dev.sh b/scripts/run-dev.sh| index fc446ce..b560598 100755 |
| --- a/scripts/run-dev.sh |
| +++ b/scripts/run-dev.sh |
| @@ -20,6 +20,19 @@ addr="localhost:${port}" |
| # accumulates state nobody wrote down. mktemp -d rather than a fixed |
| # path under /tmp, so two concurrent runs (two people, or a person and |
| # a CI job) never collide or clobber each other's database. |
| +# |
| +# The EXIT trap below only ever fires while this is still a running |
| +# bash script — up through a failed build. It is deliberately gone the |
| +# moment exec below replaces this process with the server (see that |
| +# line's own comment): fix round 1 found that running the server as a |
| +# plain foreground subprocess instead left the wrapper's PID and the |
| +# server's PID different, so killing `./scripts/run-dev.sh &` — the |
| +# exact pattern this script invites — fired this trap and deleted the |
| +# scratch directory while orphaning a server still holding the port, |
| +# undeletable and needing `kill -9` by hand. exec fixes the orphan at |
| +# the cost of this cleanup after the server itself is running; a |
| +# scratch directory under $TMPDIR left behind by a killed dev server is |
| +# a smaller problem than a leaked listener nothing can reach again. |
| scratch="$(mktemp -d "${TMPDIR:-/tmp}/docs-dev.XXXXXX")" |
| db="${scratch}/docs.db" |
| trap 'rm -rf "$scratch"' EXIT |
| @@ -37,4 +50,10 @@ go build -o "${scratch}/docs" ./cmd/docs |
| echo "docs: dev database at ${db}" |
| echo "docs: http://${addr}/" |
| -"${scratch}/docs" -dev-auth -addr "${addr}" -db "${db}" |
| +# exec, not a plain invocation: this script's own PID becomes the |
| +# server's PID, so a signal sent to it (Ctrl-C, or `kill` on the |
| +# backgrounded `./scripts/run-dev.sh &`) reaches the server directly — |
| +# 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}" |