p1: close the six review findings on the rpc-only mode

The service-side fence held; every defect was on the app side or in the
mode's headline promise.

B1 (blocking) — an rpc-only request rewrote routes against a pre-mode
service, and the log asserted the opposite. A v1 service has no `mode`
handler, so it drops the field, runs all eight steps and calls
netConfig_->Apply; the app then logged "the service is clamped. Nothing
will be connected." while the routing table had just been rewritten. The
code reasoned only about the downgrade direction. kProtocolVersion -> 2
with kFirstStartModeVersion, and BootstrapSession now refuses BEFORE
start_tunnel when it asks for rpc-only and hello reports < 2. The two
mismatch directions are handled separately: the safe one (asked tunnel,
got rpc-only) refuses; the dangerous one (asked rpc-only, got tunnel)
stops the tunnel it did not ask for and refuses.

B2 (blocking) — the app rendered "Connected" in rpc-only mode. The
previous audit was of the wrong signal: the user-visible connect status
never read TunnelState. The real chain is getConnectionStatus() ->
LiveStats::connectionStatus -> ParseConnectStatus -> ApplyConnectStatus,
and in rpc-only the DeviceLocal negotiates providers normally, so picking
a location showed "Connected", a green dot, a Disconnect button,
"Connected to N providers" and a live rate with zero packets carried --
while the tray, reading TunnelState, stayed disconnected.

Fixed at ONE chokepoint in SdkHost::ReadStats, not in the render path:
when the session is rpc-only, connectionStatus is forced to "RPC_ONLY"
and connected/providerCount/rates to zero. "RPC_ONLY" is deliberately a
value the view does not recognise -- ParseConnectStatus documents that
anything unrecognised reads as Disconnected precisely so an unknown
status cannot leave the button claiming a connection the SDK never made.
The true values are kept in rawConnectionStatus/rawConnected for the P2
developer surface. No view file is touched, so the P0 ConnectPage split
does not collide with this.

S1 — an rpc-only app could attach to a live production tunnel and then
revert it via Logout/re-registration. Reattach now requires an exact mode
match, and attaching to a live tunnel while asking rpc-only is refused.

S2 — --rpc-only did delete routes and clear DNS on startup, and consumed
the crash marker, while the banner claimed it wrote nothing. The startup
sweep is now observe-only in rpc-only: SweepOrphanedTunnel(remove=false)
reports orphans without touching them, and PeekActiveMarker reports the
marker without eating it. Banners corrected.

S3 — reply.ok now means "I did what you asked": IsSessionLive && mode
matches. The error string is left empty on a pure mode mismatch, because
ServiceClient::CallStatus overwrites state with Error whenever !ok
carries one, which would erase the mode the caller needs.

S4 — sessionMode_ defaults and resets to RpcOnly, the mode that claims
less, matching the policy Protocol.h already stated.

Also: URNETWORK_RPC_ONLY is now an explicit truthy allow-list. "off",
"no" and "0 " (trailing space) all used to evaluate to ON; unrecognised
values now mean OFF and say so. `console --rpc-only --bogus` no longer
ignores argv[3].

Verified by running, unelevated:
  * B2 by SCREENSHOT, same build and same synthetic SDK inputs, only the
    env var differing: clamp off renders "Connected" / green dot /
    Disconnect / "Connected to 7 providers" / 12.3 Mbps; clamp on renders
    "Ready to connect" / idle dot / Connect / no count / no rate.
  * B1 both ways over the real pipe: a service reporting v1 makes the app
    refuse and NO start_tunnel is ever sent; a v2 service bootstraps --
    "session bootstrapped (mode=rpc_only)", the DeviceRemote's first real
    RPC connection to the service.
  * S2 by planting a marker: an unelevated rpc-only run leaves it in
    place and says so; a normal console run consumes it and reports the
    crash.
  * S3: clamped process + mode=tunnel now replies ok=false with
    mode=rpc_only.
  * env var matrix (1/on/off/no/"0 "/banana/unset) and the argv rejection.
  * routes and DNS byte-identical before/during/after; no adapter, no
    marker, connectivity intact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PT7KcWCPKfFwQUc7SM3oZY

Author: Ryanmello07

app/src/App/SdkHost.cpp

@@ -6,6 +6,7 @@
 #include "SdkHost.h"
 
 #include <algorithm>
+#include <cwctype>
 #include <fstream>
 #include <random>
 #include <thread>
@@ -31,19 +32,37 @@ struct RpcSession {
 };
 
 // URNETWORK_RPC_ONLY: ask the service for a session that stops before it would
-// touch the machine's routes or DNS (spec P1). Empty, "0" and "false" mean off;
-// anything else means on, so `set URNETWORK_RPC_ONLY=1` is enough.
+// touch the machine's routes or DNS (spec P1).
+//
+// Parsed as an explicit allow-list of truthy values rather than "anything that
+// is not falsy". The earlier version accepted anything unrecognised as ON, so
+// `URNETWORK_RPC_ONLY=off`, `=no` and `=0 ` (trailing space) all turned the
+// mode ON — a stray `setx` giving a client that silently refuses to connect.
+// Unrecognised now means OFF *and says so*, because the failure of guessing
+// wrong is a developer confused about why nothing connects.
 proto::StartMode StartModeFromEnvironment() {
   constexpr DWORD kMax = 64;
   wchar_t buf[kMax] = {0};
   const DWORD n = ::GetEnvironmentVariableW(L"URNETWORK_RPC_ONLY", buf, kMax);
-  // n == 0: unset. n >= kMax: a value too long to be one of ours; treat it as
-  // unset rather than truncating into a comparison.
+  // n == 0: unset. n >= kMax: too long to be one of ours; treat as unset rather
+  // than truncating into a comparison.
   if (n == 0 || n >= kMax) return proto::StartMode::Tunnel;
+
   std::wstring v(buf, n);
-  if (v == L"0" || v == L"false" || v == L"FALSE" || v == L"False")
+  const size_t first = v.find_first_not_of(L" \t\r\n");
+  const size_t last = v.find_last_not_of(L" \t\r\n");
+  v = (first == std::wstring::npos) ? L"" : v.substr(first, last - first + 1);
+  std::transform(v.begin(), v.end(), v.begin(),
+                 [](wchar_t c) { return static_cast<wchar_t>(::towlower(c)); });
+
+  if (v == L"1" || v == L"true" || v == L"yes" || v == L"on")
+    return proto::StartMode::RpcOnly;
+  if (v.empty() || v == L"0" || v == L"false" || v == L"no" || v == L"off")
     return proto::StartMode::Tunnel;
-  return proto::StartMode::RpcOnly;
+  LogWarn("sdkhost: URNETWORK_RPC_ONLY is set to an unrecognised value; "
+          "ignoring it and using the normal tunnel mode. Use 1/true/yes/on to "
+          "enable rpc-only.");
+  return proto::StartMode::Tunnel;
 }
 
 void SaveRpcSession(const RpcSession& s) {
@@ -771,18 +790,52 @@ bool SdkHost::BootstrapSession() {
   try {
     std::string clientPem, serverCertPem, hostPort;
 
-    // Reattach to a live session if the service reports one and we have the key
-    // material for it. A live session is reusable when it is at LEAST as
-    // capable as what we asked for: a tunnel serves an rpc-only request fine,
-    // but an rpc-only session does not serve a tunnel request — and tearing
-    // down a running tunnel because this process happens to be in rpc-only mode
-    // would disconnect the user to make a developer's life easier.
     proto::TunnelStatus hello = service_.Hello();
+
+    if (requestedMode_ == proto::StartMode::RpcOnly) {
+      // A service older than kFirstStartModeVersion has no `mode` handler: it
+      // drops the field, runs all eight steps and rewrites this machine's
+      // routes and DNS. Refuse BEFORE start_tunnel — after it the damage is
+      // done and all we could do is revert. This check is the ONLY thing that
+      // distinguishes "honours mode" from "ignores mode"; without it the
+      // safest-looking configuration in the tree is the one that silently
+      // builds a real tunnel.
+      if (hello.protocol_version < proto::kFirstStartModeVersion) {
+        LogError("sdkhost: REFUSING to start a session. URNETWORK_RPC_ONLY is "
+                 "set, but the running service speaks control protocol v{} and "
+                 "only v{}+ understands the start mode — it would ignore the "
+                 "field, build a REAL TUNNEL and rewrite this machine's routes "
+                 "and dns. Update the installed service, or run `urnetworkd "
+                 "console --rpc-only` from this build.",
+                 hello.protocol_version, proto::kFirstStartModeVersion);
+        return false;
+      }
+      // A live TUNNEL when we asked for rpc-only. Attaching would be honestly
+      // REPORTED, but it also hands this process the authority to tear that
+      // tunnel down: TeardownSessionLocked -> StopTunnel -> NetworkConfig::
+      // Revert, reachable from Logout and from re-registration. Somebody who
+      // set the env var to guarantee "this run cannot touch my network" must
+      // not find that Log out reverted the tunnel they were using.
+      if (proto::IsSessionLive(hello.state) &&
+          hello.mode == proto::StartMode::Tunnel) {
+        LogError("sdkhost: REFUSING to attach. URNETWORK_RPC_ONLY is set, but "
+                 "the service is running a REAL TUNNEL (state={} "
+                 "routes_installed={}). This process would be able to stop it — "
+                 "a log out or a re-registration reverts its routes. Stop the "
+                 "tunnel first, or unset URNETWORK_RPC_ONLY.",
+                 proto::ToString(hello.state),
+                 hello.routes_installed ? "yes" : "no");
+        return false;
+      }
+    }
+
+    // Reattach only when the live session's mode is EXACTLY the one we asked
+    // for. "At least as capable" was wrong in the rpc-only direction: a tunnel
+    // does carry rpc-only traffic, but attaching to it also confers the power
+    // to revert it — refused above.
     auto saved = LoadRpcSession();
     const bool liveIsSufficient =
-        proto::IsSessionLive(hello.state) &&
-        !(requestedMode_ == proto::StartMode::Tunnel &&
-          hello.mode == proto::StartMode::RpcOnly);
+        proto::IsSessionLive(hello.state) && hello.mode == requestedMode_;
     if (liveIsSufficient && saved && hello.rpc_listen_hostport == saved->host_port) {
       clientPem = saved->client_pem;
       serverCertPem = saved->server_cert_pem;
@@ -827,10 +880,34 @@ bool SdkHost::BootstrapSession() {
         return false;
       }
       sessionMode_.store(st.mode);
+      // A mode mismatch is never benign, and the two directions are NOT the
+      // same event. The old code logged the harmless one's message for both.
       if (st.mode != cfg.mode) {
-        LogWarn("sdkhost: asked the service for a {} session and got {} — the "
-                "service is clamped. Nothing will be connected.",
-                proto::ToString(cfg.mode), proto::ToString(st.mode));
+        if (cfg.mode == proto::StartMode::RpcOnly) {
+          // The dangerous direction. We asked for no network changes and the
+          // service built a tunnel: routes and DNS have ALREADY been rewritten.
+          // The version gate above should make this unreachable, so reaching it
+          // means a peer is misreporting its version — give the routes back
+          // rather than keep a tunnel nobody asked for.
+          LogError("sdkhost: the service returned a TUNNEL for an rpc-only "
+                   "request (routes_installed={}). This machine's routes and "
+                   "dns have already been rewritten by a request that asked for "
+                   "the opposite. Stopping it and refusing the session.",
+                   st.routes_installed ? "yes" : "no");
+          service_.StopTunnel();
+          return false;
+        }
+        // The safe direction: we asked for a tunnel and the service is clamped
+        // to rpc-only, so nothing was written. Refuse rather than silently run
+        // a session the caller did not ask for.
+        LogError("sdkhost: asked the service for a {} session and it served {} "
+                 "— the service is clamped (`urnetworkd console --rpc-only`). "
+                 "No tunnel was created and no routes were touched. Set "
+                 "URNETWORK_RPC_ONLY=1 to ask for rpc-only explicitly, or run "
+                 "an unclamped service.",
+                 proto::ToString(cfg.mode), proto::ToString(st.mode));
+        service_.StopTunnel();
+        return false;
       }
       if (st.mode == proto::StartMode::RpcOnly) {
         LogWarn("sdkhost: RPC-ONLY session at {} — the DeviceRemote is live and "
@@ -979,6 +1056,44 @@ LiveStats SdkHost::ReadStats() {
       if (loc->country) s.countryName = *loc->country;
     }
   }
+
+  // ---- rpc-only: clamp the RENDERED connection state ----------------------
+  //
+  // LAST, so it covers every field the window renders, including the throughput
+  // rates filled in above. It belongs here rather than in the window for two
+  // reasons: this is where the fields the UI renders are produced, and it is
+  // the only place a fix can reach them without touching the UI layer.
+  //
+  // Clamping TunnelState was NOT enough, because the user-visible connect
+  // status never read TunnelState. The chain that reaches the pixels is
+  // getConnectionStatus() -> LiveStats::connectionStatus ->
+  // MainWindow::ParseConnectStatus -> ApplyConnectStatus. In rpc-only the
+  // DeviceLocal is live and negotiates provider transports normally — that is
+  // the POINT of the mode, and the spec puts connect controls inside it — so
+  // the moment a location is picked getConnectionStatus() returns CONNECTED and
+  // the window shows "Connected", a green dot, a Disconnect button, "Connected
+  // to N providers" and a live rate, with zero packets carried. The tray
+  // meanwhile reads TunnelState and stays disconnected, so the app contradicts
+  // itself.
+  //
+  // "RPC_ONLY" is deliberately a value the window does not recognise:
+  // ParseConnectStatus documents that anything unrecognised reads as
+  // Disconnected, precisely so an unknown status cannot leave the button
+  // claiming a connection the SDK never made. This uses that existing fail-safe
+  // rather than adding a parallel one.
+  s.rpcOnly = sessionMode_.load() == proto::StartMode::RpcOnly;
+  if (s.rpcOnly) {
+    // The true SDK values are kept, not discarded: the developer surface (P2)
+    // is the one place that SHOULD see them. Everything the connect page
+    // renders is clamped.
+    s.rawConnectionStatus = s.connectionStatus;
+    s.rawConnected = s.connected;
+    s.connectionStatus = "RPC_ONLY";
+    s.connected = false;  // gates "Connected to N providers" and the rate line
+    s.providerCount = 0;
+    s.downBitsPerSecond = 0;
+    s.upBitsPerSecond = 0;
+  }
   return s;
 }
 
@@ -1712,9 +1827,10 @@ void SdkHost::TeardownSessionLocked() {
   if (service_.IsConnected()) {
     service_.StopTunnel();
   }
-  // No session, so no session mode. Back to the default rather than leaving the
-  // last one to be read by a status built before the next bootstrap sets it.
-  sessionMode_.store(proto::StartMode::Tunnel);
+  // No session, so no tunnel — reset to the mode that claims less, not to
+  // Tunnel. A status built between this teardown and the next bootstrap must
+  // not be able to render "connected".
+  sessionMode_.store(proto::StartMode::RpcOnly);
   ClearRpcSession();
 }
 

app/src/App/SdkHost.h

@@ -66,11 +66,24 @@ struct CreateNetworkParams {
 // Snapshot of live connection / throughput / provide stats. Pushed to the UI on
 // SDK listener callbacks (macOS parity: listener-push, not polling).
 struct LiveStats {
-  std::string connectionStatus;   // getConnectionStatus() (CONNECTED/CONNECTING/...)
-  bool connected = false;
+  // getConnectionStatus() (CONNECTED/CONNECTING/DESTINATION_SET/DISCONNECTED)
+  // -- EXCEPT in an rpc-only session, where it is forced to the deliberately
+  // unrecognised "RPC_ONLY" so the connect page renders as disconnected. There
+  // is no tunnel in that mode and nothing may claim otherwise. See the clamp at
+  // the end of SdkHost::ReadStats.
+  std::string connectionStatus;
+  bool connected = false;         // forced false in an rpc-only session
   int64_t providerCount = 0;      // grid window current size (providers in window)
   int64_t downBitsPerSecond = 0;  // remote (tunneled) ingress bit rate
   int64_t upBitsPerSecond = 0;    // remote (tunneled) egress bit rate
+  // This snapshot came from an rpc-only session: no tunnel exists, nothing is
+  // carried, and the four fields above have been clamped to say so.
+  bool rpcOnly = false;
+  // What the SDK actually reported before the clamp. For the developer surface
+  // (P2), which is the one place that should see through it. Empty/false unless
+  // rpcOnly.
+  std::string rawConnectionStatus;
+  bool rawConnected = false;
   bool insufficientBalance = false;
   bool provideEnabled = false;
   bool providePaused = false;
@@ -538,8 +551,12 @@ class SdkHost {
   ServiceClient service_;
   // Set once in Initialize() from URNETWORK_RPC_ONLY; never changes after.
   proto::StartMode requestedMode_ = proto::StartMode::Tunnel;
-  // Set from the service's reply/hello whenever a session is established.
-  std::atomic<proto::StartMode> sessionMode_{proto::StartMode::Tunnel};
+  // Set from the service's reply/hello whenever a session is established, and
+  // reset here when one is torn down. Defaults to RpcOnly — the mode that
+  // CLAIMS LESS — matching the policy TunnelStatus::from_json states for an
+  // unreadable mode. With no session there is certainly no tunnel, so a stray
+  // read before or after one must not be able to render "connected".
+  std::atomic<proto::StartMode> sessionMode_{proto::StartMode::RpcOnly};
   std::string appVersion_ = "0.0.1";
 
   WalletConnect wallet_;

app/src/Common/Protocol.h

@@ -25,7 +25,22 @@
 namespace urnw::proto {
 
 // bump when the wire format changes incompatibly; hello negotiates it
-inline constexpr int kProtocolVersion = 1;
+//
+// 2: StartTunnel::mode / TunnelStatus::mode + TunnelState::RpcOnly.
+//    This bump is load-bearing, not bookkeeping. A version-1 service has no
+//    `mode` handler in its from_json, so it SILENTLY DROPS the field: an
+//    rpc-only request arrives as a plain start_tunnel, all eight steps run, and
+//    the machine's routes and DNS are rewritten by a request that asked for
+//    exactly the opposite. `mode` cannot be made safe by its own absence — the
+//    only thing that distinguishes "this peer honours mode" from "this peer
+//    ignores mode" is the version. Anything requesting RpcOnly MUST refuse to
+//    proceed against a peer reporting < kFirstStartModeVersion.
+//    See SdkHost::BootstrapSession.
+inline constexpr int kProtocolVersion = 2;
+
+// The first version that understands StartTunnel::mode. Below this, an absent
+// `mode` on the wire means "ignored", not "defaulted".
+inline constexpr int kFirstStartModeVersion = 2;
 
 // ---- message type tags ----------------------------------------------------
 
@@ -164,6 +179,11 @@ struct TunnelStatus {
   // `state` so it survives Starting/Stopping/Error, where `state` says nothing
   // about which kind of session was asked for. In a live session the two agree
   // by construction: the controller derives both from one stored mode.
+  //
+  // Defaults to Tunnel, and that is right even for an absent field: a peer old
+  // enough not to send `mode` is a peer that only ever built real tunnels, so
+  // reading its silence as "Tunnel" is honest. What is NOT safe is asking such
+  // a peer for RpcOnly — see kFirstStartModeVersion.
   StartMode mode = StartMode::Tunnel;
   // True only when routes and DNS are actually installed right now. This is the
   // field to trust for "is my traffic going through the tunnel"; it is false for

app/src/Service/ControlServer.cpp

@@ -26,12 +26,18 @@ nlohmann::json ControlServer::Handle(const nlohmann::json& request) {
     } else if (type == proto::msg::kStartTunnel) {
       proto::StartTunnel cfg = request.get<proto::StartTunnel>();
       proto::TunnelStatus st = tunnel_.Start(cfg);
-      // "ok" means "the session you asked for is live", which for an rpc-only
-      // request is state rpc_only, not up. The caller still has to read
-      // st.state / st.mode to know what it got — ok alone never implies a
-      // tunnel. (An unknown mode string throws out of the get<> above and is
-      // answered as a failed reply, so a garbled mode never starts anything.)
-      reply.ok = proto::IsSessionLive(st.state);
+      // "ok" means "I did what you asked" — live AND in the mode requested.
+      // A clamped process serving a tunnel request produces a live session, but
+      // not the one the caller asked for, and reporting ok for that is how a
+      // caller ends up believing it has a tunnel. The status carries the mode
+      // actually served, so the caller can see which way it differed.
+      // (An unknown mode string throws out of the get<> above and is answered
+      // as a failed reply, so a garbled mode never starts anything.)
+      reply.ok = proto::IsSessionLive(st.state) && st.mode == cfg.mode;
+      // Left EMPTY on a pure mode mismatch, deliberately: the status already
+      // says what happened, and ServiceClient::CallStatus overwrites state with
+      // Error whenever !ok carries an error string — which would erase the very
+      // mode the caller needs in order to react correctly.
       reply.error = st.error;
       reply.status = st;
       PushState();

app/src/Service/NetworkConfig.cpp

@@ -268,7 +268,7 @@ void NetworkConfig::CrashRevert() {
 }
 
 int NetworkConfig::SweepOrphanedTunnel(const GUID& tunGuid,
-                                       const wchar_t* adapterName) {
+                                       const wchar_t* adapterName, bool remove) {
   // Collect candidate LUIDs first, then sweep, so a rename or a GUID fallback
   // cannot make us miss the interface that is holding the machine's traffic.
   uint64_t candidates[8] = {0};
@@ -326,6 +326,15 @@ int NetworkConfig::SweepOrphanedTunnel(const GUID& tunGuid,
     row.InterfaceLuid = luid;
     std::string alias = (::GetIfEntry2(&row) == NO_ERROR) ? Narrow(row.Alias)
                                                           : std::string("<gone>");
+    if (!remove) {
+      LogWarn("netcfg: ORPHANED tun interface \"{}\" (luid {:#x}) present at "
+              "startup — a previous run did not revert. NOT cleaning it: this "
+              "process is observe-only (rpc-only mode) and removing routes "
+              "needs elevation. Run `urnetworkd revert` from an elevated prompt "
+              "to take the routes back.",
+              alias, luid.Value);
+      continue;
+    }
     int removed = DeleteTunnelRoutes(luid);
     ClearTunnelDns(luid);
     LogWarn(

app/src/Service/NetworkConfig.h

@@ -91,7 +91,15 @@ class NetworkConfig {
   // exists when the service starts, a previous run died without unwinding (or
   // the adapter outlived it). Delete our route set and DNS from each and say so
   // loudly. Returns how many orphaned interfaces were found.
-  static int SweepOrphanedTunnel(const GUID& tunGuid, const wchar_t* adapterName);
+  //
+  // remove=false makes it OBSERVE ONLY: it still finds and reports orphans but
+  // deletes no route and clears no DNS. That is what the unelevated rpc-only
+  // mode uses — the removal needs privilege it does not have, and a mode whose
+  // entire promise is "this will not touch your network" must not open by
+  // rewriting the route table. Reporting an orphan it cannot clean is still
+  // worth doing: it tells the owner to run `urnetworkd revert` elevated.
+  static int SweepOrphanedTunnel(const GUID& tunGuid, const wchar_t* adapterName,
+                                 bool remove = true);
 
   // Find the best default-route interface indices that are NOT the tun. Used to
   // set the SDK egress binding. Recomputed on every network change.

app/src/Service/TunnelController.cpp

@@ -450,6 +450,11 @@ bool TunnelController::TakeActiveMarker() {
   return true;
 }
 
+bool TunnelController::PeekActiveMarker() {
+  std::error_code ec;
+  return std::filesystem::exists(ActiveMarkerPath(), ec);
+}
+
 void TunnelController::PushPhysicalAddressesLocked(const EgressInterfaces& egress) {
   uint8_t addr4[4] = {0};
   uint8_t addr6[16] = {0};