Fix review findings: port forwards, refresh retry, snippets, host rename, pairing lockout and more

app/src/main/java/com/secrm/HostSessions.kt

@@ -93,7 +93,8 @@ class HostSessions(private val app: SecureRemoteApp) {
 
     /** Records the presented key as the one to trust for [change]'s host. The next connect will pin to it. */
     fun trustNewKey(change: KeyChange) {
-        app.connections.save(change.conn.copy(hostKey = change.presented))
+        // On the latest record, not the copy taken at connect time, so an edit made since (such as an MCP flag turned off) is kept.
+        app.connections.update(change.conn.name) { it.copy(hostKey = change.presented) }
         keyChange.value = null
     }
 

app/src/main/java/com/secrm/SecureRemoteApp.kt

@@ -44,6 +44,8 @@ class SecureRemoteApp : Application() {
     lateinit var agents: AgentTokens; private set
     lateinit var snippets: SnippetStore; private set
     lateinit var jobs: JobStore; private set
+    /** MCP's pooled SSH sessions; closed, with their forwards, when MCP is stopped or a client is revoked. */
+    lateinit var mcpSessions: com.secrm.core.ssh.tools.McpSessionPool; private set
     /** The connected hosts, each with its own shell and file browser. */
     val sessions by lazy { HostSessions(this) }
 
@@ -95,7 +97,7 @@ class SecureRemoteApp : Application() {
                 McpServerService.noteActivity()
             }
         })
-        SshTools.register(dispatcher, connections, newClient()) {
+        mcpSessions = SshTools.register(dispatcher, connections, newClient()) {
             snippets.list().map { NamedCommand(it.name, it.command) }
         }
         watchNetwork()

app/src/main/java/com/secrm/data/JobStore.kt

@@ -36,6 +36,10 @@ class JobStore(private val store: EncryptedConfigStore) {
     @Synchronized
     fun delete(id: String) = write(list().filterNot { it.id == id })
 
+    /** Jobs name their host, so a renamed host takes its jobs with it. */
+    @Synchronized
+    fun renameConnection(from: String, to: String) = write(list().map { if (it.connection == from) it.copy(connection = to) else it })
+
     @Synchronized
     fun recordRun(id: String, at: Long, result: String) = write(list().map {
         if (it.id == id) it.copy(lastRun = at, lastResult = result.take(300)) else it

app/src/main/java/com/secrm/data/SnippetStore.kt

@@ -9,7 +9,10 @@ import java.util.UUID
 @Serializable
 data class Snippet(val id: String, val name: String, val command: String)
 
-/** Reusable commands for the terminal, kept in the encrypted config. Inserted, never run: the owner presses Enter. */
+/**
+ * Reusable commands, kept in the encrypted config. In the terminal they are inserted, not run: the owner presses
+ * Enter. MCP agents with exec access to a host can also run them there by name (ssh.run_snippet).
+ */
 class SnippetStore(private val store: EncryptedConfigStore) {
     @Synchronized
     fun list(): List<Snippet> = store.get(KEY)?.let {

app/src/main/java/com/secrm/plugin/RunCommandService.kt

@@ -25,6 +25,7 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withTimeout
 import java.util.UUID
 
 /**
@@ -102,7 +103,10 @@ class RunCommandService : Service() {
             val auth = authFor(conn) ?: return failure("No saved key for ${conn.name}")
             secrets = Redact.secretsOf(auth)
             if (!ssh.connectSaved(conn, auth, app.connections)) return failure("Could not connect to ${conn.host}")
-            val r = ssh.execResult(s.command, live?.sink(red = false), live?.sink(red = true))
+            // Capped at the hour the macro host is asked to wait, so a command that never exits doesn't run the service forever.
+            val r = withTimeout(PluginProtocol.REQUESTED_TIMEOUT_NEVER.toLong()) {
+                ssh.execResult(s.command, live?.sink(red = false), live?.sink(red = true))
+            }
             PluginProtocol.RESULT_OK to Bundle().apply {
                 putString(PluginProtocol.VAR_STDOUT, r.stdout)
                 putString(PluginProtocol.VAR_STDERR, r.stderr)

app/src/main/java/com/secrm/service/McpServerService.kt

@@ -164,6 +164,8 @@ class McpServerService : Service() {
         if (screenReceiverOn) { runCatching { unregisterReceiver(screenOff) }; screenReceiverOn = false }
         server?.stop(); server = null
         _running.value = false
+        // Otherwise an agent's port forwards would stay open after the owner turned MCP off.
+        (application as SecureRemoteApp).mcpSessions.closeAll()
     }
 
     override fun onDestroy() { stopServer(); scope.cancel(); super.onDestroy() }

app/src/main/java/com/secrm/ui/dashboard/DashboardScreen.kt

@@ -205,7 +205,17 @@ fun DashboardScreen(
             onDismiss = { showForm = false; editing = null },
             onSave = { conn ->
                 // A corrupt store throws from upsert; report it instead of crashing the app.
-                val saved = runCatching { app.connections.upsert(conn) }
+                val old = editing
+                val saved = runCatching {
+                    // Saved by name: a taken name would silently replace that host and its trusted key.
+                    require(old?.name == conn.name || connections.none { it.name == conn.name }) { "a host named ${conn.name} already exists" }
+                    app.connections.upsert(conn)
+                    if (old != null && old.name != conn.name) {
+                        app.connections.delete(old.name)
+                        app.sessions.close(old.name)
+                        app.jobs.renameConnection(old.name, conn.name)
+                    }
+                }
                     .onFailure { Toast.makeText(activity, "Couldn't save host: ${it.message}", Toast.LENGTH_LONG).show() }
                     .isSuccess
                 if (saved) {

app/src/main/java/com/secrm/ui/server/ServerControlScreen.kt

@@ -334,7 +334,7 @@ private fun McpSection(app: SecureRemoteApp, onOpenAudit: () -> Unit) {
             Spacer(Modifier.height(Space.md))
             Row(horizontalArrangement = Arrangement.spacedBy(Space.sm)) {
                 PillButton("Pair Claude", PillStyle.Primary, Modifier.weight(1f)) { pairing = app.oauth.newPairingCode() }
-                PillButton("Disconnect Claude", PillStyle.Ghost, Modifier.weight(1f)) { app.oauth.revokeAll(); pairing = null }
+                PillButton("Disconnect Claude", PillStyle.Ghost, Modifier.weight(1f)) { app.oauth.revokeAll(); app.mcpSessions.closeAll(); pairing = null }
             }
             pairing?.let {
                 Text("Pairing code (10 min, single use)", style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant)
@@ -523,6 +523,7 @@ private fun AgentTokensSection(app: SecureRemoteApp) {
             }
             if (!a.revoked) TextButton(onClick = {
                 app.agents.revoke(a.id)
+                app.mcpSessions.closeAll() // sessions are shared, so this also ends other clients' forwards
                 agents = app.agents.list()
             }) { Text("Revoke", color = MaterialTheme.colorScheme.error) }
         }

app/src/main/java/com/secrm/ui/sftp/SftpController.kt

@@ -302,10 +302,15 @@ class SftpController(
         }
     }
 
-    /** Remote name for an upload's partial file: the same source and size always map to the same name. */
+    /**
+     * Remote name for an upload's partial file: the same source, size and modified time always map to the same name,
+     * so a file edited since the last attempt starts over instead of resuming onto old bytes. A provider that
+     * reports no modified time gives 0, and then only source and size are matched, as before.
+     */
     private fun partialName(u: Uri, size: Long): String {
+        val modified = runCatching { DocumentFile.fromSingleUri(app, u)?.lastModified() }.getOrNull() ?: 0L
         val key = java.security.MessageDigest.getInstance("SHA-256")
-            .digest("$u|$size".toByteArray())
+            .digest("$u|$size|$modified".toByteArray())
             .take(8).joinToString("") { "%02x".format(it) }
         return ".secrm-$key.part"
     }
@@ -338,7 +343,12 @@ class SftpController(
             for (c in src.listFiles()) { if (cancelRequested) return; putTree(c, remote, safeName(c.name ?: continue)) }
         } else {
             _state.update { it.copy(transfer = it.transfer?.copy(name = name, done = 0, total = src.length())) }
-            app.contentResolver.openInputStream(src.uri)!!.use { client.upload(it, remote, { cancelRequested }) { n -> progress(n) } }
+            try {
+                app.contentResolver.openInputStream(src.uri)!!.use { client.upload(it, remote, { cancelRequested }) { n -> progress(n) } }
+            } catch (t: Throwable) {
+                client.removeQuietly(remote) // no partial file left on the host
+                throw t
+            }
         }
     }
 

app/src/main/java/com/secrm/ui/terminal/PortForwardSheet.kt

@@ -76,7 +76,8 @@ fun PortForwardSheet(ssh: SshClientManager, onDismiss: () -> Unit) {
                             lp == null || lp !in 1024..65535 -> error = "Local port must be 1024–65535 (no root on Android)."
                             rp == null || rp !in 1..65535 -> error = "Remote port must be 1–65535."
                             else -> scope.launch {
-                                runCatching { ssh.addLocalForward(lp, remoteHost, rp) }
+                                // The owner's forward is for any app on this phone (a browser, a database client), which cannot send a token.
+                                runCatching { ssh.addLocalForward(lp, remoteHost, rp, requireToken = false) }
                                     .onSuccess { error = null; localPort = ""; remotePort = "" }
                                     .onFailure { error = "Could not open: ${it.message ?: "port in use"}" }
                             }

core/server-http/src/main/kotlin/com/secrm/core/server/http/McpOAuth.kt

@@ -44,13 +44,16 @@ class McpOAuth(
 
     private var pairing: String? = null
     private var pairingExpires = 0L
-    private var failures = 0
 
-    /** A new single-use pairing code, valid for 10 minutes or 5 wrong tries. */
+    /**
+     * A new single-use pairing code, valid for 10 minutes. Guessing is bounded by the route's rate limit
+     * (about 100 tries in that time against 31^8 codes), not a wrong-try lockout, which anyone who has
+     * the public URL could trip to cancel the owner's code.
+     */
     @Synchronized
     fun newPairingCode(): String =
         (1..8).map { ALPHABET[random.nextInt(ALPHABET.length)] }.joinToString("")
-            .also { pairing = it; pairingExpires = now() + 10 * MINUTE; failures = 0 }
+            .also { pairing = it; pairingExpires = now() + 10 * MINUTE }
 
     @Synchronized
     fun isValid(token: String) = tokens[sha(token)]?.let { it.kind == 'a' && it.expires > now() } == true
@@ -64,10 +67,7 @@ class McpOAuth(
     fun authorize(code: String, challenge: String, redirect: String): String? {
         codes.values.removeAll { it.expires < now() }
         val p = pairing
-        if (p == null || now() > pairingExpires || !MessageDigest.isEqual(code.trim().uppercase().toByteArray(), p.toByteArray())) {
-            if (++failures >= 5) pairing = null
-            return null
-        }
+        if (p == null || now() > pairingExpires || !MessageDigest.isEqual(code.trim().uppercase().toByteArray(), p.toByteArray())) return null
         pairing = null
         return token().also { codes[it] = Code(challenge, redirect, now() + MINUTE) }
     }
@@ -76,6 +76,7 @@ class McpOAuth(
     @Synchronized
     fun token(params: Map<String, String?>): Pair<String, String>? {
         var family = token()
+        var refreshedFrom: String? = null
         when (params["grant_type"]) {
             "authorization_code" -> {
                 val c = params["code"]?.let(codes::remove) ?: return null
@@ -99,28 +100,41 @@ class McpOAuth(
                         persist()
                         return null
                     }
+                    // Only one pair may stay live: the one this retry gets. The first pair's access token goes, and its
+                    // refresh token is marked used, so if both copies were real (one stolen) the other's next refresh
+                    // revokes the family.
+                    reissued.remove(key)?.let { (a, rf) ->
+                        tokens.remove(a)
+                        tokens[rf]?.let { tokens[rf] = it.copy(kind = 'c'); consumedAt.remove(rf) }
+                    }
                 } else {
                     if (r.kind != 'r' || r.expires < now()) return null
                     tokens[key] = r.copy(kind = 'c')
                     consumedAt[key] = now()
                 }
                 family = r.family
+                refreshedFrom = key
             }
             else -> return null
         }
         val access = token()
         val refresh = token()
         tokens.values.removeAll { it.expires < now() }
         consumedAt.values.removeAll { now() - it > REUSE_GRACE_MS }
+        reissued.keys.retainAll(consumedAt.keys)
         tokens[sha(access)] = Entry('a', now() + HOUR, family)
         tokens[sha(refresh)] = Entry('r', now() + 30 * DAY, family)
+        refreshedFrom?.let { reissued[it] = sha(access) to sha(refresh) }
         persist()
         return access to refresh
     }
 
     /** When each consumed refresh token was used. Not persisted: after a restart a reuse is treated as theft. */
     private val consumedAt = HashMap<String, Long>()
 
+    /** Consumed refresh token hash to the (access, refresh) hashes its use issued, while a retry is still allowed. */
+    private val reissued = HashMap<String, Pair<String, String>>()
+
     private fun persist() = save(tokens.entries.joinToString("\n") { "${it.key} ${it.value.kind} ${it.value.expires} ${it.value.family}" })
     private fun token() = b64(ByteArray(32).also(random::nextBytes))
     private fun sha(s: String) = sha256(s.toByteArray()).joinToString("") { "%02x".format(it) }

core/server-http/src/main/kotlin/com/secrm/core/server/http/OAuthRoutes.kt

@@ -81,6 +81,7 @@ internal fun Route.oauthRoutes(oauth: McpOAuth, mcpPath: String) {
     val registerLimit = RateLimit(max = 20)
     // Per client_id, so one client's burst cannot slow the others' refreshes.
     val tokenLimit = ClientRateLimits(max = 60)
+    val pairingLimit = RateLimit(max = 10) // the only bound on guessing a pairing code (see McpOAuth.newPairingCode)
     get("/.well-known/oauth-protected-resource/{rest...}") {
         call.json {
             put("resource", call.base() + mcpPath)
@@ -129,6 +130,7 @@ internal fun Route.oauthRoutes(oauth: McpOAuth, mcpPath: String) {
     }
     post("/authorize") {
         val p = call.boundedText()?.let { parseQueryString(it) } ?: return@post call.respond(HttpStatusCode.PayloadTooLarge)
+        if (!pairingLimit.allow()) return@post call.respondText("Too many tries. Wait a minute and try again.", status = HttpStatusCode.TooManyRequests)
         val redirect = p["redirect_uri"]
         if (!McpOAuth.allowedRedirect(redirect)) return@post call.respondText("Only Claude can connect", status = HttpStatusCode.BadRequest)
         // PKCE is mandatory: without a challenge the code could never be redeemed.

core/server-http/src/test/kotlin/com/secrm/core/server/http/McpOAuthTest.kt

@@ -41,11 +41,11 @@ class McpOAuthTest {
         assertNull(pair(oauth, pairing))
     }
 
-    @Test fun pairingLocksAfterFiveWrongGuesses() {
+    @Test fun wrongGuessesDoNotCancelThePairingCode() {
         val oauth = newOAuth()
         val pairing = oauth.newPairingCode()
         repeat(5) { assertNull(pair(oauth, "WRONG")) }
-        assertNull("correct code must fail once locked", pair(oauth, pairing))
+        requireNotNull(pair(oauth, pairing))
     }
 
     @Test fun pairingCodeIsCaseAndWhitespaceTolerant() {
@@ -105,6 +105,17 @@ class McpOAuthTest {
         assertTrue("a benign retry keeps the session", oauth.isValid(access3))
     }
 
+    @Test fun retryWithinGraceWindowLeavesOnlyOnePairLive() {
+        val oauth = newOAuth(clock = Clock())
+        val (_, refresh1) = requireNotNull(exchange(oauth, requireNotNull(pair(oauth, oauth.newPairingCode()))))
+        val (access2, refresh2) = requireNotNull(oauth.token(mapOf("grant_type" to "refresh_token", "refresh_token" to refresh1)))
+        // A stolen copy of refresh1 is replayed within the grace window.
+        val (access3, _) = requireNotNull(oauth.token(mapOf("grant_type" to "refresh_token", "refresh_token" to refresh1)))
+        assertFalse("the first pair's access token is gone", oauth.isValid(access2))
+        assertNull("its refresh token no longer works", oauth.token(mapOf("grant_type" to "refresh_token", "refresh_token" to refresh2)))
+        assertFalse("using it is treated as theft and revokes the family", oauth.isValid(access3))
+    }
+
     @Test fun omittedRedirectUriIsRejectedWhenAuthorizeSentOne() {
         val oauth = newOAuth()
         val code = requireNotNull(pair(oauth, oauth.newPairingCode()))

core/ssh-tools/src/main/kotlin/com/secrm/core/ssh/tools/McpSessionPool.kt

@@ -93,18 +93,26 @@ class McpSessionPool(
         return client
     }
 
-    /** Caller holds [lock]. A session with port forwards open is kept: closing it would close the forwards. */
+    /**
+     * Caller holds [lock]. A session with port forwards open is kept: closing it would close the forwards.
+     * It is still closed once the owner turns off MCP read access (which forwards need) or deletes the connection.
+     */
     private fun scheduleReap(e: Entry) {
         e.reaper?.cancel()
         e.reaper = scope.launch {
             delay(idleMs)
             lock.withLock {
                 if (e.users != 0 || entries[e.conn.name] !== e) return@withLock
-                if (e.client.forwards.value.isEmpty()) evict(e) else scheduleReap(e)
+                if (e.client.forwards.value.isEmpty() || saved.find(e.conn.name)?.allowMcpRead != true) evict(e) else scheduleReap(e)
             }
         }
     }
 
+    /** Closes every shared session and the forwards on them: for when MCP is stopped or a client is revoked. */
+    fun closeAll() {
+        scope.launch { lock.withLock { entries.values.toList().forEach(::evict) } }
+    }
+
     /** Caller holds [lock]. */
     private fun evict(e: Entry) {
         if (entries[e.conn.name] === e) entries.remove(e.conn.name)

core/ssh-tools/src/main/kotlin/com/secrm/core/ssh/tools/Snippets.kt

@@ -9,9 +9,44 @@ private val PLACEHOLDER = Regex("(?<!\\$)\\{([A-Za-z_][A-Za-z0-9_]*)\\}")
 /** The variable names in [command], in order of first use. */
 fun placeholdersOf(command: String): List<String> = PLACEHOLDER.findAll(command).map { it.groupValues[1] }.distinct().toList()
 
-/** [command] with each placeholder replaced by its value, single-quoted so a value cannot add commands. */
-fun fillSnippet(command: String, values: Map<String, String>): String =
-    PLACEHOLDER.replace(command) { m -> shellQuote(values[m.groupValues[1]].orEmpty()) }
+private val NAME = Regex("[A-Za-z_][A-Za-z0-9_]*")
+
+/**
+ * [command] with each placeholder replaced by its value, quoted for where the placeholder stands, so a value
+ * cannot add commands: single-quoted outside quotes, escaped inside '…' or "…". Plain single-quoting would
+ * end an enclosing '…' (leaving the value unquoted) or leave `$(…)` live inside "…".
+ */
+fun fillSnippet(command: String, values: Map<String, String>): String {
+    val out = StringBuilder()
+    var quote: Char? = null // the quote the text is inside, if any
+    var i = 0
+    while (i < command.length) {
+        val c = command[i]
+        if (c == '{' && (i == 0 || command[i - 1] != '$')) {
+            val end = command.indexOf('}', i)
+            val name = if (end > i) command.substring(i + 1, end) else ""
+            if (NAME.matches(name)) {
+                val v = values[name].orEmpty()
+                out.append(when (quote) {
+                    '\'' -> v.replace("'", "'\\''")
+                    '"' -> v.replace(Regex("[\\\\\"$`]")) { "\\" + it.value }
+                    else -> shellQuote(v)
+                })
+                i = end + 1
+                continue
+            }
+        }
+        when {
+            // A backslash outside '…' escapes the next character, which then opens or closes nothing.
+            c == '\\' && quote != '\'' && i + 1 < command.length -> { out.append(c).append(command[i + 1]); i += 2; continue }
+            c == '\'' && quote != '"' -> quote = if (quote == '\'') null else '\''
+            c == '"' && quote != '\'' -> quote = if (quote == '"') null else '"'
+        }
+        out.append(c)
+        i++
+    }
+    return out.toString()
+}
 
 /** Single-quotes [s] for a POSIX shell. */
 fun shellQuote(s: String): String = "'" + s.replace("'", "'\\''") + "'"