carlos / platform Public

edge: fix round-2 review finding — :80 fallback is a routing question

IMPORTANT: httpFallback (the :80 handler) was still gating its
redirect-vs-unknownHost decision on Allowed(), which round 1 correctly
narrowed to an exact match for HostPolicy/ACME-issuance reasons. That
narrowing silently regressed the :80 redirect too: a wildcard-alias
host like paul.woodstar.app routes fine on 443 but was landing on
unknownHost/404 over plain HTTP.

Extracted the handler into Edge.httpFallbackHandler (was an inline
closure in RunWithActivatorReady, now testable) and switched its gate
to lookupUpstream — "does this host route?" is a routing question, and
widening it here is safe: a redirect grants nothing (a 302 back to the
same host on 443), unlike Allowed's exact-only ACME order gate. The
ACME HTTP-01 challenge path is unaffected: autocert's own HTTPHandler
checks /.well-known/acme-challenge/ (and calls its own hostPolicy,
which IS Allowed) before ever falling through to this handler.

TestHTTPFallbackRedirectsWildcardAlias pins it: paul.woodstar.app and
random.woodstar.app both redirect (they route, by design, even with no
exact row), while a host matching nothing still 404s via unknownHost.

Also: the delegated-source closure's error branch now returns a copy
(append([]string(nil), lastGood...)) rather than the cached slice
itself, so a future consumer can never alias and mutate the cache.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 5909ce859dc1fcbc2b23dfa5731ebfc7ebf0a765 parent e925343

3 files changed, +93 −19

internal/edge/alias_test.go +51 −0
internal/edge/edge.go +41 −18
internal/edge/wildcard.go +1 −1

diff --git a/internal/edge/alias_test.go b/internal/edge/alias_test.go

index c872bf05e..1b342fe3c 100644

--- a/internal/edge/alias_test.go

+++ b/internal/edge/alias_test.go

@@ -432,3 +432,54 @@ func TestHostPolicyExcludesWildcardFallback(t *testing.T) {

t.Fatalf("Allowed(random.woodstar.app) = true, want false — still no exact row for it")

}

+// TestHTTPFallbackRedirectsWildcardAlias is the round-2 review fix: the :80

+// handler (httpFallbackHandler) is a THIRD Allowed() consumer the first fix

+// pass missed. It answers a routing question ("does something serve this

+// host on 443?"), not an issuance question, so it must use lookupUpstream —

+// any host that routes via the *.woodstar.app catch-all (paul.woodstar.app,

+// random.woodstar.app, any label) gets the classic 302 to https, even though

+// none of them has its own exact row and Allowed() (HostPolicy) refuses all

+// of them. A host that doesn't route at all still gets unknownHost (404 with

+// no fallback URL configured).

+func TestHTTPFallbackRedirectsWildcardAlias(t *testing.T) {

+ backend, _ := hostEchoBackend(t, "primary")

+ reg := openReg(t)

+ if err := reg.Upsert(registry.Route{

+ Host: "woodstar-relay.bab.oncarlos.com", Kind: "service",

+ Addr: backend.Listener.Addr().String(), Status: "ready",

+ }); err != nil {

+ t.Fatalf("upsert: %v", err)

+ }

+ upsertAlias(t, reg, "*.woodstar.app", "woodstar-relay.bab.oncarlos.com")

+ e, err := New(reg, nil)

+ if err != nil {

+ t.Fatalf("New: %v", err)

+ }

+ h := e.httpFallbackHandler()

+ for _, host := range []string{"paul.woodstar.app", "random.woodstar.app"} {

+ req := httptest.NewRequest(http.MethodGet, "http://"+host+"/", nil)

+ req.Host = host

+ rec := httptest.NewRecorder()

+ h.ServeHTTP(rec, req)

+ if rec.Code != http.StatusFound {

+ t.Fatalf("%s: status = %d, want 302 — it routes via the *.woodstar.app catch-all even with no exact row", host, rec.Code)

+ }

+ if loc := rec.Header().Get("Location"); loc != "https://"+host+"/" {

+ t.Fatalf("%s: Location = %q, want https://%s/", host, loc, host)

+ }

+ // A host that doesn't route at all (no exact route, no matching

+ // wildcard alias) still hits unknownHost — no fallback URL configured

+ // here, so a plain 404.

+ req := httptest.NewRequest(http.MethodGet, "http://nowhere.example.com/", nil)

+ req.Host = "nowhere.example.com"

+ rec := httptest.NewRecorder()

+ h.ServeHTTP(rec, req)

+ if rec.Code != http.StatusNotFound {

+ t.Fatalf("nowhere.example.com: status = %d, want 404 (unknownHost, no fallback URL)", rec.Code)

+ }

diff --git a/internal/edge/edge.go b/internal/edge/edge.go

index f173c8eed..45c369c7d 100644

--- a/internal/edge/edge.go

+++ b/internal/edge/edge.go

@@ -1035,6 +1035,44 @@ func (e *Edge) unknownHost(w http.ResponseWriter, r *http.Request) {

http.Error(w, "unknown host", http.StatusNotFound)

}

+// httpFallbackHandler is the non-ACME :80 handler passed to

+// autocert.Manager.HTTPHandler (RunWithActivatorReady): known hosts get the

+// classic redirect to https (GET/HEAD only, 302); unknown hosts go to the

+// fallback URL instead of a redirect into a doomed TLS handshake (no cert

+// will ever exist for them).

+//

+// This gate uses lookupUpstream, NOT Allowed: "does something answer this

+// host on 443?" is a ROUTING question (lookupUpstream's job, including its

+// one-label wildcard-alias fallback), not an issuance question (Allowed's

+// job, deliberately exact-only — see its own doc comment). A wildcard-alias

+// host like paul.woodstar.app resolves fine on 443 via the wildcard cert, so

+// plain http:// must redirect it rather than 404. This is safe to widen past

+// Allowed's exact match because a redirect grants nothing: it's a 302 to the

+// SAME host on 443, never a new ACME order — the attacker-supplied-label

+// hazard that keeps Allowed narrow does not apply here.

+//

+// The ACME HTTP-01 challenge path is untouched by this: m.HTTPHandler (the

+// caller) checks the /.well-known/acme-challenge/ prefix — and runs its OWN

+// hostPolicy, which IS Allowed — before ever falling through to this

+// handler, so a challenge request never reaches lookupUpstream at all.

+func (e *Edge) httpFallbackHandler() http.Handler {

+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

+ host := r.Host

+ if h, _, err := net.SplitHostPort(host); err == nil {

+ host = h

+ }

+ if _, ok := e.lookupUpstream(host); !ok {

+ e.unknownHost(w, r)

+ return

+ }

+ if r.Method != "GET" && r.Method != "HEAD" {

+ http.Error(w, "Use HTTPS", http.StatusBadRequest)

+ return

+ }

+ http.Redirect(w, r, "https://"+host+r.URL.RequestURI(), http.StatusFound)

+ })

// Watch polls the registry for writes from other processes (`carlos add`

// etc., without needing rights to signal us) every 2s and reloads on change.

// It polls a cheap fingerprint of the routes table rather than PRAGMA

@@ -1409,24 +1447,9 @@ func RunWithActivatorReady(args []string, act Activator, cache *Cache, ready fun

splitDomains(os.Getenv("CARLOS_WILDCARD_DOMAINS")), os.Getenv("PLATFORM_APPS_DOMAIN"), w.delegationZone)

}

- // The non-ACME :80 handler. Known hosts get the classic redirect to https

- // (GET/HEAD only, 302); unknown hosts go to the fallback URL instead of a

- // redirect into a doomed TLS handshake (no cert will ever exist for them).

- httpFallback := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

- host := r.Host

- if h, _, err := net.SplitHostPort(host); err == nil {

- host = h

- }

- if !e.Allowed(host) {

- e.unknownHost(w, r)

- return

- }

- if r.Method != "GET" && r.Method != "HEAD" {

- http.Error(w, "Use HTTPS", http.StatusBadRequest)

- return

- }

- http.Redirect(w, r, "https://"+host+r.URL.RequestURI(), http.StatusFound)

- })

+ // The non-ACME :80 handler. See httpFallbackHandler's doc comment for the

+ // Allowed-vs-lookupUpstream split.

+ httpFallback := e.httpFallbackHandler()

// ReadHeaderTimeout guards the public front door against Slowloris-style

// slow-header connection exhaustion. No ReadTimeout/WriteTimeout — those

diff --git a/internal/edge/wildcard.go b/internal/edge/wildcard.go

index 1d013f2b6..41d65a9f9 100644

--- a/internal/edge/wildcard.go

+++ b/internal/edge/wildcard.go

@@ -642,7 +642,7 @@ func newWildcardFromEnv(e *Edge, cache autocert.Cache) *wildcardManager {

wds, err := e.reg.ListWildcardDomains()

if err != nil {

mu.Lock()

- out := lastGood

+ out := append([]string(nil), lastGood...)

mu.Unlock()

log.Printf("edge: wildcard domains read failed: %v — delegated set unchanged this pass", err)

return out