Fix a real regression from my own isolation fix: dropped wildcard black-holed exact routes

Round 8 (codex + gemini + grok + harper-domain) reviewed the prior commit's
route-isolation fix and found I'd introduced exactly the class of bug it
was meant to prevent, just inverted: RouteTable::resolve() checked
dropped_wildcard before the exact-match lookup, so any SNI matching a
dropped wildcard's suffix returned None immediately — including a
perfectly healthy, fully-built exact route at that same SNI. One tenant's
wildcard misconfiguration (e.g. `*.acme.com` missing `protocol: 'http'`)
would black-hole every co-tenant with an exact route under that suffix
(`api.acme.com`, ACME challenge routes, etc.), which is worse than the
fall-through bug being fixed.

Root cause: I checked both dropped sets before any live lookup, on the
theory that "a poison entry should always win." That's wrong — a poison
entry must only suppress a fallback *broader* than itself, never something
*more specific*. Fixed by checking exact (live, then dropped) before moving
to wildcard (live, then dropped) before default — each dropped set
immediately follows its same-specificity live counterpart, so an exact
match always wins regardless of wildcard state. Added a Rust unit test
reproducing the exact scenario (healthy exact route + dropped sibling
wildcard) that fails against the previous ordering and passes against this
one — verified by reverting and reconfirming the failure before restoring.

Also from round 8: last-good carry-forward (designed for a transient
cert-rotation race that heals on the next reconcile) was reaching the new
protocol/no-carrier rejections too, which are permanent config errors that
never heal — so a route could silently keep serving a stale, previous
version indefinitely across every later reconcile, with the operator's
`updateConfig` reporting success while their edit quietly never took
effect. Extracted the two protocol/carrier checks into a standalone
validate_route_protocol_declaration(), called before build_route in the
build_route_table loop, and given its own never-carry-forward path — a
permanent rejection now always drops the route on a hot-swap, same as on
initial build. Added a test for this too.

Also narrowed the http2 warning in parse_route_spec: it fired for both
xForwardedFor and forwardFingerprint, but xForwardedFor+http2 is always a
hard rejection (build_route), so that branch was dead and misleadingly
implied the route degrades gracefully when it's actually dropped outright.

cargo build/clippy clean, cargo test 116/116, npm test 115/115.

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

Author: kriszyp

src/proxy.rs

@@ -816,18 +816,15 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result<RouteSpec> {
 	}
 	// xForwardedFor + http2 is a hard error (build_route, router.rs) since it's unconditionally
 	// unsafe — an h2 client's XFF would be neither injected nor stripped, forwarding an arbitrary
-	// client-supplied value as if authoritative. A header-carried forwardFingerprint in the same
-	// spot is intentionally only a warning here, not a hard error, even though the risk is the
-	// same in kind (an h2 client can forge X-JA3/X-JA4, not merely go unforwarded) — see
-	// README.md's "Forwarding the fingerprint downstream" section for the tradeoff this leaves
-	// open and why it isn't a hard error like XFF. Name whichever mode actually triggered this so
-	// the message doesn't blame X-Forwarded-For when the route never configured it.
-	if requires_http && spec.http2 {
-		let mode_desc = if source_address_mode == SourceAddressMode::XForwardedFor {
-			"xForwardedFor"
-		} else {
-			"a header-carried forwardFingerprint (X-JA3/X-JA4)"
-		};
+	// client-supplied value as if authoritative — so it's excluded here: this warning would always
+	// be immediately followed by that hard rejection, misleadingly implying the route still works
+	// (minus the header) when the whole route is in fact dropped. A header-carried
+	// forwardFingerprint in the same spot is intentionally only a warning, not a hard error, even
+	// though the risk is the same in kind (an h2 client can forge X-JA3/X-JA4, not merely go
+	// unforwarded) — see README.md's "Forwarding the fingerprint downstream" section for the
+	// tradeoff this leaves open and why it isn't a hard error like XFF.
+	if requires_http && spec.http2 && source_address_mode != SourceAddressMode::XForwardedFor {
+		let mode_desc = "a header-carried forwardFingerprint (X-JA3/X-JA4)";
 		eprintln!(
 			"symphony: route '{}': {mode_desc} has no effect for any client that negotiates h2 (http2=true) — injection and client-supplied-header stripping are both skipped, so an h2 client's own value reaches the upstream unmodified; consider sourceAddressHeader='proxyProtocolV2'",
 			spec.sni

src/router.rs

@@ -236,25 +236,32 @@ impl RouteTable {
 		};
 
 		// A route rejected outright (no last-good to carry forward) must fail closed, not fall
-		// through to a broader wildcard or the default route: it was configured for this SNI, just
-		// rejected, which is not the same as "no route was ever configured here." Checked before
-		// any match below, exact or wildcard, so a poisoned entry always wins over a fallback.
-		if self.dropped_exact.contains(sni) || self.dropped_wildcard.iter().any(|suffix| wildcard_suffix_matches(sni, suffix)) {
-			return None;
-		}
-
-		// Exact match first
+		// through to a broader fallback: it was configured for this SNI, just rejected, which is
+		// not the same as "no route was ever configured here." But a poison entry must only ever
+		// suppress a fallback *broader* than itself — it must never shadow a route that is more
+		// specific than the poison. So each dropped set is checked immediately after its
+		// same-specificity live counterpart, not before it: a healthy exact route always wins over
+		// a dropped wildcard at the same suffix (checking dropped_wildcard first would incorrectly
+		// black-hole every exact route under a wildcard that happened to fail validation).
+
+		// Exact, live or dropped — most specific, decided first.
 		if let Some(r) = self.exact.get(sni) {
 			return Some(r);
 		}
+		if self.dropped_exact.contains(sni) {
+			return None;
+		}
 
-		// Wildcard: match exactly one left-most label against stored suffixes.
+		// Wildcard, live or dropped — match exactly one left-most label against stored suffixes.
 		// e.g. "foo.example.com" matches "*.example.com" but "a.b.example.com" does not.
 		for (suffix, route) in &self.wildcard {
 			if wildcard_suffix_matches(sni, suffix) {
 				return Some(route);
 			}
 		}
+		if self.dropped_wildcard.iter().any(|suffix| wildcard_suffix_matches(sni, suffix)) {
+			return None;
+		}
 
 		self.default.as_ref()
 	}
@@ -359,6 +366,29 @@ pub fn build_route_table(
 	let mut dropped_wildcard: Vec<Arc<str>> = Vec::new();
 
 	for spec in specs {
+		// The protocol/no-carrier declaration checks are validated before any cert/TLS work, and
+		// deliberately never carried forward on a hot-swap even if a last-good route exists: unlike
+		// a cert failure (a transient race between two non-atomic file writes that heals on its own
+		// next reconcile), a missing declaration or a passthrough-with-no-carrier config is a
+		// permanent config error — it never heals without a human editing the config. Applying
+		// cert-style carry-forward to it would silently keep serving the *previous* route
+		// indefinitely, including ignoring any other change (a new upstream, a cert rotation) the
+		// same reconcile made to that route — the operator's `updateConfig` reports success while
+		// their edit quietly never took effect.
+		if let Err(e) = validate_route_protocol_declaration(spec) {
+			let newly_failing = previous.is_none_or(|p| !p.failing_snis.contains(spec.sni.as_str()));
+			failing_snis.insert(Arc::from(spec.sni.as_str()));
+			if newly_failing {
+				eprintln!("symphony: skipping route '{}': {}", spec.sni, e);
+			}
+			if let Some(suffix) = spec.sni.strip_prefix("*.") {
+				dropped_wildcard.push(Arc::from(suffix));
+			} else {
+				dropped_exact.insert(Arc::from(spec.sni.as_str()));
+			}
+			continue;
+		}
+
 		// Isolate per-route failures: a single route whose cert can't be built (e.g. a
 		// rotated key no longer matching an inlined chain → rustls KeyMismatch) must not
 		// abort the whole table and take every other tenant on the port down with it.
@@ -429,19 +459,16 @@ pub fn build_route_table(
 	Ok(RouteTable { exact, wildcard, default: None, monitored_balancers, failing_snis, dropped_exact, dropped_wildcard })
 }
 
-fn build_route(
-	spec: &RouteSpec,
-	listener_tls: &ListenerTlsSpec,
-	cache: &mut TlsConfigCache,
-) -> crate::error::Result<Route> {
+/// The "no carrier" and "declare protocol: 'http'" requirements — checked in `build_route_table`
+/// before `build_route` runs, and deliberately never carried forward on a hot-swap (see the call
+/// site): unlike a cert-build failure, this is a permanent config error, not a transient race.
+fn validate_route_protocol_declaration(spec: &RouteSpec) -> crate::error::Result<()> {
 	let requires_http = requires_http_protocol(spec.source_address_mode, spec.forward_fingerprint);
 
 	// Passthrough (terminateTls=false) never decrypts the stream, so a header-carried mode has no
 	// carrier at all regardless of `protocol` — declaring 'http' wouldn't help. Checked before (and
 	// distinct from) the declaration requirement below: it's not that the route mislabeled its
-	// protocol, it's that no protocol declaration could make this work. Isolated here (rather than
-	// at parse time in proxy.rs) so one route's misconfiguration is dropped/last-good-carried like
-	// a bad cert, instead of aborting construction or updateConfig for every other route.
+	// protocol, it's that no protocol declaration could make this work.
 	if requires_http && !spec.terminate_tls {
 		return Err(crate::error::SymphonyError::Config(format!(
 			"route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint",
@@ -456,6 +483,14 @@ fn build_route(
 		)));
 	}
 
+	Ok(())
+}
+
+fn build_route(
+	spec: &RouteSpec,
+	listener_tls: &ListenerTlsSpec,
+	cache: &mut TlsConfigCache,
+) -> crate::error::Result<Route> {
 	let tls_config = if spec.terminate_tls {
 		let cert_pem = spec
 			.cert_pem
@@ -811,6 +846,30 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA==
 		);
 	}
 
+	// The inverse and more dangerous case: a dropped wildcard must NOT shadow a healthy, more
+	// specific exact route at the same suffix. A prior version of the fail-closed check tested
+	// `dropped_wildcard` before `exact`, which black-holed every exact route under any wildcard
+	// that happened to fail validation — turning one tenant's wildcard typo into an outage for
+	// every co-tenant sharing that parent domain.
+	#[test]
+	fn healthy_exact_route_survives_a_dropped_sibling_wildcard() {
+		let specs = vec![
+			tls_route("*.acme.com", CERT_A, KEY_B),  // KeyMismatch — dropped
+			tls_route("api.acme.com", CERT_A, KEY_A), // valid, more specific
+		];
+
+		let table = build_route_table(&specs, &ListenerTlsSpec::empty(), None).expect("build");
+
+		assert!(
+			table.resolve(Some("api.acme.com")).is_some(),
+			"a healthy exact route must resolve normally even when a sibling wildcard at the same suffix was dropped"
+		);
+		assert!(
+			table.resolve(Some("other.acme.com")).is_none(),
+			"a subdomain with no exact route of its own must still fail closed against the dropped wildcard"
+		);
+	}
+
 	// On a hot-swap, a route whose cert transiently fails to rebuild (the normal cert+key
 	// non-atomic write window) must retain its last-good route from the live table rather
 	// than dropping the SNI.
@@ -842,6 +901,31 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA==
 		);
 	}
 
+	// Unlike a cert-build failure (a transient race that heals on the next reconcile), a
+	// protocol-declaration/no-carrier rejection is a permanent config error — it never heals
+	// without a human editing the config. It must therefore never get cert-style last-good
+	// carry-forward: an operator who accidentally drops `protocol: 'http'` in the same edit that
+	// repoints a route to a new upstream must see that route stop serving, not have `updateConfig`
+	// silently keep the old route (old upstream included) running indefinitely.
+	#[test]
+	fn permanent_protocol_rejection_never_carries_forward_on_hot_swap() {
+		let mut good = tls_route("tenant.example.com", CERT_A, KEY_A);
+		good.source_address_mode = SourceAddressMode::XForwardedFor;
+		good.protocol = RouteProtocol::Http;
+		let live = build_route_table(&[good], &ListenerTlsSpec::empty(), None).expect("initial build");
+		assert!(live.resolve(Some("tenant.example.com")).is_some());
+
+		// Hot-swap drops the protocol declaration — a permanent rejection, not a transient one.
+		let mut broken = tls_route("tenant.example.com", CERT_A, KEY_A);
+		broken.source_address_mode = SourceAddressMode::XForwardedFor;
+		// protocol left at its default (Opaque) — undeclared.
+		let swapped = build_route_table(&[broken], &ListenerTlsSpec::empty(), Some(&live)).expect("hot-swap must not fail");
+		assert!(
+			swapped.resolve(Some("tenant.example.com")).is_none(),
+			"a permanent protocol-declaration rejection must drop the route, not silently carry the previous one forward"
+		);
+	}
+
 	#[test]
 	fn header_injection_detection() {
 		// xForwardedFor always needs protocol: 'http', regardless of fingerprint.