Reject a non-bare-host node identity and fix IPv6 replication URLs

[dawsontoth]

Enforce that this node's identity is a bare hostname or IP literal — not a URL, host:port, or non-string — at every configured source, and make IPv6 identities work end-to-end (canonical form, valid replication URLs, correct certificate SAN typing).

node.hostname (and the replication.hostname / certificate / listening address it falls back to) is this node's identity: it becomes the certificate common name (security/keys.ts) and the host replication advertises and dials (hostnameToUrl). A scheme, port, or non-string silently corrupts both — hostnameToUrl composes ws://http://host:9926:9925, whose parsed hostname is http, so the node advertises/dials a host literally named http with no error (#2218). Replaces the original warn-only PR after review by @kriszyp (replication/TLS domain expert) on this PR and #2219.

What it does

• utility/nodeIdentity.ts (new) — bareHostViolation(value), the single predicate. Accepts a bare hostname or IPv4/IPv6 literal; rejects a scheme, port, path, credentials, non-string, bracketed IPv6 ([::1] — the identity is stored unbracketed), a scoped-zone IPv6 (fe80::1%eth0), and anything the ws:// consumer would reject. It parses under a special scheme (https://) so its host grammar matches the actual replication-URL consumer (a non-special scheme is too lenient — it would pass node%20, 0x7f.1, etc.).
• validation/configValidator.ts — rejects a URL/port/numeric node.hostname and replication.hostname at the config boundary (there was no node schema; replication.hostname previously accepted string|number). Config validation throws, so a bad identity fails boot with a clear message rather than starting known-broken. This is the loud enforcement.
• server/nodeName.ts getThisNodeName() — resolves to the first source that is a valid bare host and skips an empty, unusable, or malformed derived source (replication.url host, certificate CN, listening address). It never throws and never caches an empty identity; 127.0.0.1 is the always-valid floor. Identity is normalized to the unbracketed canonical form across sources (getHostFromListeningPort and urlToNodeName strip IPv6 brackets), so net.isIP types it as an IP certificate SAN and the same node's identity string is stable.
• validation/installValidator.ts — the same check runs at install/upgrade time on NODE_HOSTNAME. It sits right after the v5 migration copies replication.hostname into node.hostname, so an upgrade carrying a URL-ish value is reported during install instead of only failing at first boot.
• Skipped sources now warn. When the resolver ignores a malformed derived source it logs once, naming the source, value, and reason — a typo'd replication.url silently resolving identity to 127.0.0.1 was a diagnosis trap on a live cluster. A valid identity resolves silently.
• hostnameToUrl() — brackets a bare IPv6 literal for URL construction (::1 → ws://[::1]:port); the unbracketed ws://::1:port is rejected by the URL parser.
• security/keys.ts — getHost() / urlToNodeName / getCommonNameFromCert are made fail-soft so a malformed replication.url or an unreadable cert file falls back instead of throwing during cert review.

Rejecting an invalid configured identity (rather than rewriting it) preserves the certificate common name — kriszyp's stated reason to prefer rejection.

⚠️ Breaking: an existing install whose node.hostname/replication.hostname is a URL, host:port, or non-string now fails to boot until corrected to a bare host — needs a release note. The v5 install migration (installer.ts copies replication.hostname → node.hostname) can carry such a value into node.hostname on upgrade, so per @ldt1996's review that case is now caught at install time with the same clear reason, rather than surfacing only at first boot. (My own local dev config had exactly this and was correctly rejected.)

For the human reviewer

The change grew from a warning into a node-identity validation subsystem after kriszyp's review, and it touches TLS cert generation, so it went through several cross-model review rounds. The judgment calls:

1. Loud at the config boundary, soft in the resolver. An invalid node.hostname/replication.hostname fails boot (kriszyp's ask). getThisNodeName deliberately does not throw — an earlier revision that threw could crash a request-time caller on a descriptive cert CN, so the resolver skips-and-falls-through while the boundary rejects. Per @ldt1996's review the resolver now also warns whenever it skips a source, so the fall-through is visible without being fatal.
2. IPv6 identity is stored unbracketed (::1), bracketed only for URL construction, so net.isIP types the cert SAN as IP and sources agree. ::1:9925 is accepted as a distinct valid IPv6 (not ::1 + port) — inherent to IPv6 syntax, no clean way to disambiguate intent.
3. Breaking-change scope (⚠️ above) — deliberate per "prevent a known-broken startup"; the release-note call is yours.
4. Deferred to follow-ups (surfaced by review, not done here):
   • Bare-IPv6-literal identity has a cert secureContexts lookup gap — filed as Replication cert lookup misses an IPv6-literal node identity (compact vs OpenSSL-expanded SAN key) #2261 (P2, under epic [Epic] Certificate & key-management hardening #1674): contexts are keyed by OpenSSL's expanded SAN text (2001:DB8:0:0:0:0:0:1) while getThisNodeName returns the compact form, so secureContexts.get() misses. Pre-existing cert infrastructure this change exposes by making IPv6 identities viable.
   • A malformed replication.url double-URL (ws://http://host… → host http) still slips (ambiguous; replication.url is a URL by design); the packaged JSON schema isn't yet synced with the runtime schema.

Rebased onto merged #2219

This now sits on top of #2219, and the two compose well: identity stays the unbracketed canonical form (so net.isIP types the cert SAN as an IP), while #2219's nodeNameToDisplayHost brackets it for display — so the startup banner renders a parseable http://[::1]:9926/. That closes the IPv6-banner gap earlier review rounds flagged against this PR, and no longer needs a follow-up.

I did have to update two of #2219's tests, and one of its assertions no longer describes reachable behavior: they set node.hostname to a URL and expected the banner to show the normalized host (localhost). Such a value is now rejected at the config boundary and skipped during resolution, so it never reaches the banner — the banner shows the fallback identity instead, with a warning explaining why. #2219's real regression guard (no http://http:// double-wrap) is kept and strengthened: the banner's URLs must now all actually parse, and a bare IPv6 node.hostname is asserted to render bracketed. Flagging it because those tests are two days old — @kriszyp / @ldt1996, tell me if you'd rather keep the old expectation and soften the resolver instead.

Addresses kriszyp's inline comments on nodeName.ts:32 (validate every source; falsy non-strings; the fallback) and :85 (net.isIP + bracket IPv6; reject parse failures; test the constructed URL).

Verification

• Route (c) — behavior via unit + boundary tests (all run locally on Node 24):
  • unitTests/utility/nodeIdentity.test.js (34): the predicate accepts every valid host shape (hostname, IPv4, IPv6) and rejects scheme/port/path/creds/non-string/bracketed/scoped-IPv6/node%20/alt-IPv4-forms — no false-positive on a valid identity (a false-positive would break boot).
  • unitTests/server/nodeName.test.js (34, including fix(run): stop double-wrapping startup URLs when node.hostname is a URL #2219's display tests): getThisNodeName returns a valid bare identity, falls through, never caches empty, skips an unusable/malformed derived source without throwing, and yields the unbracketed IPv6 identity from both a bracketed listen address and a replication.url IPv6 host; hostnameToUrl('::1') composes the parseable ws://[::1]:9933.
  • unitTests/validation/configValidator.test.js (+2) and installValidator.test.js (+2): a URL/port/numeric node.hostname and replication.hostname are rejected with a clear message; bare host and ::1 accepted.
• unitTests/bin/startupBanner.test.js (4): no double-wrapped scheme, every banner URL parses, and a bare IPv6 node.hostname renders bracketed.
• tsc build, oxlint/lint:required, and prettier clean. The bin, validation, utility, and security suites were run after the rebase; bin is now 220 passing / 0 failing.
• Full test:unit:main cannot run locally — it dies at load on the RocksDB LOCK held by my running local Harper instance (known limitation; run targeted files). CI runs the full matrix. Two unrelated pre-existing failures noted: a path-length-dependent domain-socket test-isolation flake in the validation suite, and two scopedImport/compartment tests in the security suite — both fail identically on the base commit.

Review coverage

Authored by Claude Opus 4.8/5. Reviewed across ten cross-model rounds as the design was reworked from @kriszyp's feedback and each round's findings were fixed. The graded opposite-family leg was codex (gpt-5.6-sol), joined on the rounds their tooling was available by gemini via agy (default model), Cursor Grok (cursor-grok-4.5-high) and Composer (composer-2.5). Blockers fell 3 → 1 → 0 and stayed at 0; the final round's verdict names no blocker or major, and confirms the earlier over-reach fixes (the ws+unix: finding is dropped, and the node.url / scheme-less-URL / post-unbracket concerns are fixed or non-reproducible). Honest gaps: on the last few rounds only codex ran — Gemini hit its quota and both Cursor legs failed on a local SSH-agent signing error — and the Harper domain-adjudication leg crashed on every round, so outside findings were author-adjudicated rather than machine-filtered. Two human reviews on the current code: @ldt1996 approved and traced the predicate's edge cases (her three asks and nit are implemented here), and @kriszyp's empty-hostname finding is fixed. The last review ran on b2c02c6850e2; the pushed commit adds only test changes on top of it (production code is byte-identical — verified with git diff), which is why the footer's receipt SHA is newer than the reviewed one. Grade 4 is the deterministic floor for a high-risk security/keys.ts surface with unadjudicated findings, not a mandatory-finding grade.

_{Review-Coverage: authored=unknown; ran=none; rounds=1 @ 0692c49}

_{Human-Review-Need: 4 @ 0692c49}

[gemini-code-assist]

Code Review

This pull request introduces a warning mechanism to ensure node.hostname is configured as a bare hostname, preventing silent replication and certificate issues when schemes, ports, or non-string values are provided. It also adds comprehensive unit tests to validate this behavior. The review feedback suggests improving type safety and runtime accuracy by typing the name parameter as unknown instead of string, and replacing the array filtering pattern with conditional pushes to a typed array to avoid potential type-narrowing issues and unnecessary allocations.

[claude]

Reviewed; no blockers found.

[heskew]

Reviewed; no blockers found. Clean, non-intrusive warning implementation that guards against silent replication/TLS misconfigurations.

---

🤖 Posted by Antigravity on behalf of @heskew

[kriszyp]

Sounds helpful
_{🤖 Reviewed with Codex}

[ldt1996]

LGTM! nice work

[kriszyp]

Great! (a suggestion for handling empty hostname too)

_{🤖 Reviewed with Codex}

[dawsontoth]

Thanks — and thanks for actually tracing the predicate rather than taking the description's word for it. All three asks are in, plus the nit:

1. Install-time validation. installValidator now runs bareHostViolation on NODE_HOSTNAME, which lands immediately after installer.ts:142 copies REPLICATION_HOSTNAME into it. So a v5 upgrade carrying a URL-ish value now fails during install with the same reason the config boundary gives, instead of only at first boot. You were right that a release note alone leaves the operator discovering it the hard way.

2. Warn on skip. Taken. asBareHost now carries a source label and warns once per skipped source with the source, value, and reason; a replicationUrlHost() helper does the same when replication.url has no usable host. urlToNodeName itself stays silent, since keys.ts also uses it for peer URLs rather than only this node's identity. Verified both directions: a malformed/hostless replication.url or a bad listening host each warn exactly once and fall to 127.0.0.1, and a valid identity resolves with zero warnings.

3. Issue filed. #2261 — P2, typed Bug, parented to epic #1674, with the mechanism (compact getThisNodeName vs OpenSSL-expanded SAN key), the impact (cert regenerated every boot, possible getReplicationCertAuth null deref), and a suggested fix. Agreed it would have evaporated without a number.

Nit. Fixed — myhost:443 now reports must not include a port rather than the vaguer is not a bare hostname. A trailing : after the parsed hostname is checked before the round-trip comparison, so a default port for the parse scheme no longer masks the real reason. Covered by a test.

🤖 Addressed by Claude Code