fix(gateway): do not treat a failed registry load as no HTTP clients (SBS-900) - #775
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour. 📝 WalkthroughWalkthroughThe gateway now preserves registry-load success state. HTTP startup and request authorization use this state to reject insecure loopback access after failed loads while allowing it for successfully loaded empty registries. Tests cover startup, scope, caller, and integration behavior. ChangesRegistry Load Authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change makes failed registry loads fail closed for unauthenticated insecure-loopback binding while preserving successful empty-load behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RegistryLoader
participant HTTPStartup
participant HTTPServingLoop
participant CallerResolution
RegistryLoader->>HTTPStartup: registry and registry_loaded
HTTPStartup->>HTTPServingLoop: registry_loaded
HTTPServingLoop->>CallerResolution: registry_loaded and request scope
CallerResolution->>CallerResolution: authorize insecure fallback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…loopback-registry-load # Conflicts: # CHANGELOG.md
…stener (SBS-900) The first pass closed the `load_resolved` Err path. `Ok` is not "we read the registry" either, and the verdict was a boot constant while the registry it described is live. - `load_from` now reports a `LoadSource`: the file itself, a genuine first run, a recovery from a backup, or a default standing in for contents that could not be read. `read_registry_file` used to fold every read failure into "absent", so a locked or unreadable registry came back looking like a first run; it now separates a not-found from a read that failed. A recovery is state N-1 (save_to snapshots the pre-write content), so the save that registered the FIRST http client is exactly the one whose backup has none. Only `File` and `FirstRun` count as authoritative, which is what the `--insecure-loopback` open branch now requires. A missing registry file is still a first run and still opens. - That verdict is now live. The watcher swaps the registry on any `Ok`, so a registry corrupted while the gateway runs could hand the request path an empty client list and silently re-open a listener that auth had closed. The watcher republishes the verdict with every swap, under the registry lock, and the request path reads it while holding that lock instead of using the boot value. - `serve_http` recovers a poisoned registry lock like every other lock in the file. `unwrap_or(false)` read a poisoned lock as "no clients registered", which is the input that turns auth off. - Tests drive the real load path: a corrupt registry recovered from a backup is not authoritative, a missing file is, an unreadable file is not (unix), a watcher tick on a recovered registry publishes the untrusted verdict, and a live listener that loses trust starts rejecting unauthenticated requests.
…loopback-registry-load # Conflicts: # CHANGELOG.md
…try we read SECURITY.md describes the `--insecure-loopback` precondition as "no token and an empty `http_clients`". That list is also empty when the registry was recovered from a backup or could not be read, which no longer opens a listener (SBS-900).
…loopback-registry-load # Conflicts: # CHANGELOG.md
…loopback-registry-load # Conflicts: # CHANGELOG.md
…e SBS-900 merge The SBS-866 collision test landed on main while SBS-900 was adding the registry_loaded parameter to resolve_http_caller. Git merged both cleanly but the call site still passed four arguments. The registry loads fine in this test, so it passes true; the test covers scope resolution, not a failed load.
…loopback-registry-load # Conflicts: # CHANGELOG.md # src-tauri/src/bin/toolport-gateway.rs
Fixes SBS-900.
User-visible result
A user who starts the HTTP gateway with
--insecure-loopbackafter a failed registry load (load_resolvedErr), and withoutTOOLPORT_HTTP_TOKEN/CONDUIT_HTTP_TOKEN, now sees the gateway refuse to bind instead of opening an unauthenticated loopback listener.A failed load plus an env token still binds (the token is the auth). A successful empty load — including a missing registry file, which is already
Ok(default)— plus the flag still opens.The bug
resolve_http_caller's open branch usedreg.http_clients.is_empty(). That is also true when bootload_resolvedreturns Err and falls back toRegistry::default(). So a corrupt or unreadable registry silently satisfied the "no clients configured" precondition for--insecure-loopback.Nothing reaches a tool today (the fallback has zero servers). The auth posture of an explicitly-insecure mode still should not flip on a read failure.
Fix
Thread
registry_loaded(Ok=true,Err=false) intoresolve_http_caller,resolve_http_scope, and startupallow_insecure_open/http_bind_is_authorized. The open branch now requiresregistry_loaded && http_clients.is_empty().Watcher reload-fail already keeps the previous registry. That path was not changed.
Existing tests that use
Registry::default()as a successful empty registry passregistry_loaded=true.Sweep
rg -n 'http_clients\.is_empty|allow_insecure_open'(excludingdocs/audit/):resolve_http_calleropen branchregistry_loadedhttp_allows_insecure_open/http_bind_is_authorizedregistry_loadedserve_http!reg.http_clients.is_empty()--insecure-loopback; the hatch is what this PR closes.serve_http_loop/handle_connectionregistry_loadedthroughload_fromErrFail-without-fix
Reverted only the two production gates (
&& registry_loadedinresolve_http_callerand inhttp_allows_insecure_open). Signatures stayed so the tests compiled. Then:Restored the gates. The same test, plus
successful_empty_registry_load_still_opens_insecure_loopback, then passed.Tests
In
src-tauri/src/bin/toolport-gateway.rs, both citing SBS-900:failed_registry_load_does_not_open_insecure_loopback— failed load must not open; failed load + no env token must not authorize the open bind; failed load + env token still bindssuccessful_empty_registry_load_still_opens_insecure_loopback— successful empty load + flag still opensCI (this worktree)
npm run format:checknpm run lintnpm run buildnpm run testcargo clippy --manifest-path src-tauri/Cargo.toml --no-default-features --lib --binscargo test --manifest-path src-tauri/Cargo.toml --no-default-features --lib --bins --teststest:rust, default features)npm run test:rustWhat this makes more likely
An operator who used
--insecure-loopbackas a recovery hatch after a corrupt registry, with no env token, now gets a refuse-to-bind error instead of an open listener. They must setTOOLPORT_HTTP_TOKENor repair the registry. That is the intended fail-closed direction.A later successful watcher reload after a failed boot does not flip
registry_loadedto true. Startup policy stays immutable. Env-token and registered-client auth still work on that path if the process bound.Gaps / not done
smoke:headless.--no-default-features)./home/box/projects/toolportor other worktrees.docs/audit/.Note
Fix gateway to reject insecure loopback binding when registry load fails
LoadSourceenum in registry.rs to track whether a registry load was authoritative (File) or a fallback (FirstRun,Backup,Unreadable).registry_trusted: Arc<AtomicBool>toGatewayStateand propagates it throughwatch_registry/watch_tickso auth decisions reflect live reload outcomes.resolve_http_caller,http_bind_is_authorized, andhttp_allows_insecure_opennow requireregistry_loaded == truebefore treating an emptyhttp_clientslist as "no clients configured" for the--insecure-loopbackescape hatch.🖇️ Linked Issues
Fixes SBS-900 — the gateway incorrectly treated a failed registry load as having no HTTP clients, potentially opening an insecure loopback listener.
Macroscope summarized 320c0a7.