From 7a5584365231492d3ea93f43faee11fdb25abcdd Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 29 Oct 2026 11:12:31 -0400 Subject: [PATCH 1/4] test(conformance): wire MCP fixtures, reverse-attach negatives, parity checklist (refs #24) Adds the open test items from #24. No Sources/ or production Rust changes. New files - conformance/mcp_wire_vectors.json: 12 JSON-RPC cases (initialize, ping, tools/list shape, tools/call errors, notification, parse and batch errors). Expectations are the Swift MCPServer result. 7 Rust divergences are listed in known_drift with the output Rust gives today. - Tests/InstanceMCPCoreTests/WireConformanceTests.swift: runs the fixture against MCPServer.parse and handle. - poc/reverse-attach-linux/src/wire_conformance.rs: runs the fixture against mcp::answer; fails if a known_drift output changes or a drift is fixed. - Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift: a plain TCP runtime answers 401; the client must dial once, send "Bearer right" and end in handshakeRejected(401). Also a canary check: the attach secret is absent from POST, GET list and GET one bodies. - poc/reverse-attach-linux/src/attach/client_tests.rs: 401 on wrong, same-length wrong, empty and missing bearer (one dial each); right bearer sends the expected headers; wss:// to a plaintext listener does not succeed and sends no HTTP. - poc/reverse-attach-linux/src/attach/grant_secrecy_tests.rs: grant_json never contains the secret. - docs/parity-checklist.md: the Phase 3 gate, with wire, HTTP, reverse-attach and per-tool rows. Profile rows are a placeholder for #59. Changed - conformance/auth_vectors.json: 3 cases appended (same-length byte change, case change, trailing space). The old last object gained a trailing comma. - conformance/README.md: rows and a link to the checklist. - main.rs, attach/mod.rs, attach/client.rs: one cfg(test) mod line each. Verification (commands run on macmini; nothing built on the laptop) - Baseline on untouched origin/main de5e211 (git archive): swift test 136 passed, 0 failed; cargo test --release 18 passed. - swift test on this commit: 139 passed, 0 failed (136 + 3 new suites). Includes ConformanceVectorTests (auth vectors, 19 cases). - cargo test --release --locked in rust:latest (rustc 1.99.1, Linux aarch64, container on macmini via OrbStack): 25 passed, 0 failed (18 + 7 new). - cargo fmt --check: pass (after rustfmt on the three new Rust files). - cargo clippy --release --locked -- -D warnings: pass (CI command). - cargo test --release on native macOS: FAILS AT LINK (not a test failure). The crate is Linux-only (seat.rs calls prctl). A test that calls answer() with tools/call reaches tool_key and so links seat.rs. The baseline never reached that path from tests, so it linked by dead-stripping. CI runs the Rust job on Linux and is the authoritative Rust run. - json.tool: both fixtures valid. - Not run: bash smoke.sh and markdownlint (see follow-ups). Finding (pre-existing, not fixed here) - On Linux, tungstenite connect() with a wss:// URL panics: rustls 0.23 has no process-level CryptoProvider in this build. A wss:// runtime would panic the dial thread and leave the grant in "dialing". The wss test accepts that panic as "dial did not succeed". Fix in a follow-up. Still open for #24 after this PR - 1: 7 known_drift Rust fixes (wire layer). HTTP-layer gaps (session 404, 415, DELETE semantics, 400 vs 202) are in the checklist. - 2a: successful wss:// handshake (needs TLS fixture); Swift and Rust. - 2b: sha256 verifier (runtime side, openab-pty; not in this repo). - 2c: pod stores only the verifier (runtime side). - 2d: per-connection profile enforcement at dispatch (#59). - 2e: origin policy, certificate verification. - 3 and 4 are done by this PR. --- .../ReverseAttachNegativeTests.swift | 120 ++++++++++ .../WireConformanceTests.swift | 73 ++++++ conformance/README.md | 9 + conformance/auth_vectors.json | 5 +- conformance/mcp_wire_vectors.json | 100 ++++++++ docs/parity-checklist.md | 102 +++++++++ poc/reverse-attach-linux/src/attach/client.rs | 4 + .../src/attach/client_tests.rs | 216 ++++++++++++++++++ .../src/attach/grant_secrecy_tests.rs | 35 +++ poc/reverse-attach-linux/src/attach/mod.rs | 2 + poc/reverse-attach-linux/src/main.rs | 2 + .../src/wire_conformance.rs | 108 +++++++++ 12 files changed, 775 insertions(+), 1 deletion(-) create mode 100644 Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift create mode 100644 Tests/InstanceMCPCoreTests/WireConformanceTests.swift create mode 100644 conformance/mcp_wire_vectors.json create mode 100644 docs/parity-checklist.md create mode 100644 poc/reverse-attach-linux/src/attach/client_tests.rs create mode 100644 poc/reverse-attach-linux/src/attach/grant_secrecy_tests.rs create mode 100644 poc/reverse-attach-linux/src/wire_conformance.rs diff --git a/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift b/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift new file mode 100644 index 0000000..edcbd77 --- /dev/null +++ b/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift @@ -0,0 +1,120 @@ +import Foundation +import Network +import XCTest +@testable import InstanceMCPCore + +/// A plain TCP listener that answers every WebSocket upgrade with 401, the way a +/// runtime does when the bearer is wrong. It speaks no WebSocket at all, so the +/// only thing the client can learn is the HTTP status. +final class PlainRefusingRuntime: @unchecked Sendable { + private let lock = NSLock() + private var dialCount = 0 + private var seenAuthorization: [String] = [] + private let listener: NWListener + private let queue = DispatchQueue(label: "plain-refusing-runtime") + private(set) var port: UInt16 = 0 + + init() throws { + listener = try NWListener(using: .tcp, on: .any) + let sem = DispatchSemaphore(value: 0) + listener.stateUpdateHandler = { [weak self] st in + if case .ready = st { self?.port = self?.listener.port?.rawValue ?? 0; sem.signal() } + if case .failed = st { sem.signal() } + } + listener.newConnectionHandler = { [weak self] conn in self?.accept(conn) } + listener.start(queue: queue) + sem.wait() + } + + func stop() { listener.cancel() } + + var dials: Int { lock.lock(); defer { lock.unlock() }; return dialCount } + var authorizations: [String] { lock.lock(); defer { lock.unlock() }; return seenAuthorization } + + private func accept(_ conn: NWConnection) { + lock.lock(); dialCount += 1; lock.unlock() + conn.start(queue: queue) + readHead(conn, buffer: Data()) + } + + private func readHead(_ conn: NWConnection, buffer: Data) { + conn.receive(minimumIncompleteLength: 1, maximumLength: 4096) { [weak self] data, _, isComplete, error in + var buf = buffer + if let data { buf.append(data) } + if let end = buf.range(of: Data("\r\n\r\n".utf8)) { + let head = String(decoding: buf[.. HTTPRequest { + let h = ["content-type": "application/json", "tailscale-user-login": "a@b", "x-forwarded-for": "100.1.1.1"] + let data = body.map { try! JSONCoding.encoder.encode($0) } ?? Data() + return HTTPRequest(method: method, path: path, query: nil, headers: h, body: data) + } + + /// The attach secret goes in on POST and must not come back out on any + /// read route. The grant body is what Connect and Remote display. + func testGrantBodiesNeverCarryTheAttachSecret() async throws { + let canary = "CANARY-SECRET-7f3a" + let mgr = AttachManager(server: fullServer()) + let ep = MCPHTTPEndpoint(server: fullServer(), auth: AuthPolicy(allowedLogins: ["a@b"]), attach: mgr) + + let post = await ep.handle(attachReq("POST", "/attach", + body: ["runtime": "ws://127.0.0.1:1", "session": "s", "secret": .string(canary)]), + remoteIsLoopback: true) + XCTAssertTrue((200..<300).contains(post.status), "POST /attach: \(post.status)") + let id = try XCTUnwrap(decodeJSON(post.body)["id"]?.stringValue) + + let list = await ep.handle(attachReq("GET", "/attach"), remoteIsLoopback: true) + let one = await ep.handle(attachReq("GET", "/attach/\(id)"), remoteIsLoopback: true) + for r in [post, list, one] { + XCTAssertFalse(String(decoding: r.body, as: UTF8.self).contains(canary), + "secret leaked in \(r.status) body") + } + + let del = await ep.handle(attachReq("DELETE", "/attach/\(id)"), remoteIsLoopback: true) + XCTAssertTrue((200..<300).contains(del.status), "DELETE /attach/: \(del.status)") + } +} diff --git a/Tests/InstanceMCPCoreTests/WireConformanceTests.swift b/Tests/InstanceMCPCoreTests/WireConformanceTests.swift new file mode 100644 index 0000000..95344e1 --- /dev/null +++ b/Tests/InstanceMCPCoreTests/WireConformanceTests.swift @@ -0,0 +1,73 @@ +import Foundation +import XCTest +@testable import InstanceMCPCore + +/// Runs `conformance/mcp_wire_vectors.json` through `MCPServer.parse` and +/// `MCPServer.handle`. The Swift server is the oracle, so every `expect` must +/// hold here. The Rust runner (`poc/reverse-attach-linux/src/wire_conformance.rs`) +/// reads the same file and pins its own drift through `known_drift`. +final class WireConformanceTests: XCTestCase { + private func load() throws -> JSONValue { + let root = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() // InstanceMCPCoreTests + .deletingLastPathComponent() // Tests + .deletingLastPathComponent() // repo root + let data = try Data(contentsOf: root.appendingPathComponent("conformance/mcp_wire_vectors.json")) + return try JSONCoding.decoder.decode(JSONValue.self, from: data) + } + + /// What a client sees for one raw request, in the vocabulary `expect` uses. + private func observe(_ server: MCPServer, _ raw: String) async -> JSONValue { + let response: JSONRPCResponse? + switch MCPServer.parse(Data(raw.utf8)) { + case .failure(let e): + response = JSONRPCResponse(id: .null, error: e) + case .success(let req): + response = await server.handle(req) + } + guard let r = response else { return .object(["response": .bool(false)]) } + var o: [String: JSONValue] = ["response": .bool(true)] + if let e = r.error { o["error"] = .number(Double(e.code)) } + if let result = r.result { o["result"] = result } + return .object(o) + } + + /// Subset match: every key in `want` must hold in `obs`. Unknown keys fail, + /// so a typo in the fixture cannot pass silently. + static func matches(_ obs: JSONValue, _ want: JSONValue) -> Bool { + guard let w = want.objectValue else { return false } + for (key, value) in w { + switch key { + case "response", "error": + if obs[key] != value { return false } + case "result_subset": + guard let sub = value.objectValue, let res = obs["result"]?.objectValue else { return false } + for (k, v) in sub where res[k] != v { return false } + case "result_tools_shape": + guard let tools = obs["result"]?["tools"]?.arrayValue, !tools.isEmpty else { return false } + let ok = tools.allSatisfy { + $0["name"]?.stringValue != nil && $0["description"]?.stringValue != nil + && $0["inputSchema"]?.objectValue != nil + } + if !ok { return false } + default: + return false + } + } + return true + } + + func testEveryWireCaseMatchesTheSwiftOracle() async throws { + let doc = try load() + let cases = try XCTUnwrap(doc["cases"]?.arrayValue) + XCTAssertGreaterThanOrEqual(cases.count, 12) + let server = MCPServer(name: "t", version: "0", tools: [EchoTool()]) + for c in cases { + let name = c["name"]?.stringValue ?? "?" + let request = try XCTUnwrap(c["request"]?.stringValue, name) + let expect = try XCTUnwrap(c["expect"], name) + let got = await observe(server, request) + XCTAssertTrue(Self.matches(got, expect), "\(name): got \(got), want \(expect)") + } + } +} diff --git a/conformance/README.md b/conformance/README.md index fe1f5e4..32f8734 100644 --- a/conformance/README.md +++ b/conformance/README.md @@ -8,6 +8,15 @@ while the Swift daemon (macOS) and the Rust port (`poc/reverse-attach-linux`, Li |---|---|---|---| | `auth_vectors.json` | `AuthPolicy`: validate + allow/deny decision and exact reason strings | `ConformanceVectorTests.testAuthPolicyVectors` | `auth::conformance` | | `reverse_attach_vectors.json` | §9.2 close-code / handshake-status disposition, `attachURL` | `ConformanceVectorTests.testReverseAttachVectors` | `attach::conformance` | +| `mcp_wire_vectors.json` | JSON-RPC layer: `initialize`, `ping`, `tools/list`, `tools/call` errors, notifications, parse and envelope errors. Rust drift is pinned in `known_drift` | `WireConformanceTests.testEveryWireCaseMatchesTheSwiftOracle` | `wire_conformance::wire_fixtures_match_the_swift_oracle_and_pin_known_drift` | Both run in CI on every PR (`swift test`, `cargo test`). Change a vector only for an intended behaviour change, and make both implementations pass in the same PR. + +Dial-side and credential negatives that need code rather than a vector file live next to the +code: `ReverseAttachNegativeTests.swift` and `attach/client_tests.rs` (refused 401, wrong or +missing bearer, no plaintext fallback), `attach/grant_secrecy_tests.rs` and +`AttachCredentialSecrecyTests` (the attach secret never appears in a grant body). + +Open items, the parity gate, and the known drift list are in +[`docs/parity-checklist.md`](../docs/parity-checklist.md). diff --git a/conformance/auth_vectors.json b/conformance/auth_vectors.json index 444dd0c..eb722ef 100644 --- a/conformance/auth_vectors.json +++ b/conformance/auth_vectors.json @@ -17,6 +17,9 @@ {"name": "allowlist + no identity + loopback + insecure-local -> allow local", "config": {"allowedLogins": ["you@example.com"], "allowLocalUnauthenticated": true}, "headers": {}, "remoteIsLoopback": true, "expect": {"allow": "local"}}, {"name": "insecure-local but has x-forwarded-for -> deny (not truly local)", "config": {"allowedLogins": ["you@example.com"], "allowLocalUnauthenticated": true}, "headers": {"x-forwarded-for": "1.2.3.4"}, "remoteIsLoopback": true, "expect": {"deny": "no Tailscale identity on request"}}, {"name": "insecure-local only, loopback, no headers -> allow local", "config": {"allowLocalUnauthenticated": true}, "headers": {}, "remoteIsLoopback": true, "expect": {"allow": "local"}}, - {"name": "insecure-local only but not loopback -> deny", "config": {"allowLocalUnauthenticated": true}, "headers": {}, "remoteIsLoopback": false, "expect": {"deny": "unauthenticated"}} + {"name": "insecure-local only but not loopback -> deny", "config": {"allowLocalUnauthenticated": true}, "headers": {}, "remoteIsLoopback": false, "expect": {"deny": "unauthenticated"}}, + {"name": "token same length, one byte differs -> deny bad token", "config": {"bearerToken": "s3cret"}, "headers": {"authorization": "Bearer s3crXt"}, "remoteIsLoopback": true, "expect": {"deny": "bad token"}}, + {"name": "token case changed -> deny bad token (byte compare, not case-folded)", "config": {"bearerToken": "s3cret"}, "headers": {"authorization": "Bearer S3CRET"}, "remoteIsLoopback": true, "expect": {"deny": "bad token"}}, + {"name": "token with trailing space -> deny bad token (no trimming)", "config": {"bearerToken": "s3cret"}, "headers": {"authorization": "Bearer s3cret "}, "remoteIsLoopback": true, "expect": {"deny": "bad token"}} ] } diff --git a/conformance/mcp_wire_vectors.json b/conformance/mcp_wire_vectors.json new file mode 100644 index 0000000..4ba4365 --- /dev/null +++ b/conformance/mcp_wire_vectors.json @@ -0,0 +1,100 @@ +{ + "version": 1, + "description": "Wire-level JSON-RPC vectors for the MCP layer: initialize, ping, tools/list, tools/call error shapes, notifications, parse and envelope errors. Oracle: Swift MCPServer.swift (parse + handle). Every case's expect is the Swift result. The Rust port (poc/reverse-attach-linux, mcp::answer) runs the same cases. Each case it still gets wrong is listed in known_drift with the subset it returns today (rust_today), so a drift fix or a new drift fails the Rust test until this file changes in the same PR. request is raw JSON text so malformed and batch inputs can be expressed. expect keys: response (bool), error (JSON-RPC code), result_subset (keys that must match result), result_tools_shape (result.tools is a non-empty array of objects with name, description and inputSchema). Subset matching: unlisted keys are not checked.", + "cases": [ + { + "name": "initialize negotiates 2024-11-05", + "request": "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"initialize\",\"params\":{\"protocolVersion\":\"2024-11-05\",\"capabilities\":{},\"clientInfo\":{\"name\":\"t\",\"version\":\"0\"}}}", + "expect": {"response": true, "result_subset": {"protocolVersion": "2024-11-05"}} + }, + { + "name": "initialize negotiates 2025-06-18", + "request": "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"initialize\",\"params\":{\"protocolVersion\":\"2025-06-18\",\"capabilities\":{},\"clientInfo\":{\"name\":\"t\",\"version\":\"0\"}}}", + "expect": {"response": true, "result_subset": {"protocolVersion": "2025-06-18"}} + }, + { + "name": "initialize with unknown version falls back to 2025-06-18", + "request": "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"initialize\",\"params\":{\"protocolVersion\":\"1999-01-01\",\"capabilities\":{},\"clientInfo\":{\"name\":\"t\",\"version\":\"0\"}}}", + "expect": {"response": true, "result_subset": {"protocolVersion": "2025-06-18"}} + }, + { + "name": "ping returns an empty result", + "request": "{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"ping\"}", + "expect": {"response": true, "result_subset": {}} + }, + { + "name": "tools/list returns tools with name, description and inputSchema", + "request": "{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"tools/list\"}", + "profile": "owner", + "expect": {"response": true, "result_tools_shape": true} + }, + { + "name": "tools/call unknown tool is invalid params", + "request": "{\"jsonrpc\":\"2.0\",\"id\":4,\"method\":\"tools/call\",\"params\":{\"name\":\"no_such_tool\",\"arguments\":{}}}", + "profile": "owner", + "expect": {"response": true, "error": -32602} + }, + { + "name": "tools/call missing name is invalid params", + "request": "{\"jsonrpc\":\"2.0\",\"id\":5,\"method\":\"tools/call\",\"params\":{\"arguments\":{}}}", + "profile": "owner", + "expect": {"response": true, "error": -32602} + }, + { + "name": "unknown method is method not found", + "request": "{\"jsonrpc\":\"2.0\",\"id\":6,\"method\":\"no/such/method\"}", + "expect": {"response": true, "error": -32601} + }, + { + "name": "notification gets no response", + "request": "{\"jsonrpc\":\"2.0\",\"method\":\"notifications/initialized\"}", + "expect": {"response": false} + }, + { + "name": "malformed JSON is a parse error", + "request": "{nope", + "expect": {"response": true, "error": -32700} + }, + { + "name": "jsonrpc other than 2.0 is an invalid request", + "request": "{\"jsonrpc\":\"1.0\",\"id\":7,\"method\":\"tools/list\"}", + "profile": "owner", + "expect": {"response": true, "error": -32600} + }, + { + "name": "batch array is an invalid request (batches removed in 2025-06-18)", + "request": "[{\"jsonrpc\":\"2.0\",\"id\":8,\"method\":\"tools/list\"}]", + "expect": {"response": true, "error": -32600} + } + ], + "known_drift": [ + { + "name": "initialize negotiates 2025-06-18", + "rust_today": {"response": true, "result_subset": {"protocolVersion": "2024-11-05"}} + }, + { + "name": "initialize with unknown version falls back to 2025-06-18", + "rust_today": {"response": true, "result_subset": {"protocolVersion": "2024-11-05"}} + }, + { + "name": "ping returns an empty result", + "rust_today": {"response": true, "error": -32601} + }, + { + "name": "tools/call unknown tool is invalid params", + "rust_today": {"response": true, "error": -32601} + }, + { + "name": "malformed JSON is a parse error", + "rust_today": {"response": false} + }, + { + "name": "jsonrpc other than 2.0 is an invalid request", + "rust_today": {"response": true, "result_tools_shape": true} + }, + { + "name": "batch array is an invalid request (batches removed in 2025-06-18)", + "rust_today": {"response": false} + } + ] +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md new file mode 100644 index 0000000..0145264 --- /dev/null +++ b/docs/parity-checklist.md @@ -0,0 +1,102 @@ +# Parity checklist (Phase 3 gate) + +This checklist is the gate for retiring the Swift daemon (Phase 3) in +`docs/adr/linux-rust-port.md`. The executable part is the suite in +[`conformance/`](../conformance/README.md), which runs in `swift test` (oracle) +and in `cargo test --release` (Rust port). This file lists what the suite does +not cover yet, and the status of each row. Status values: **pass** (both +backends agree and a test pins it), **drift** (the backends differ, pinned in +`known_drift`), **open** (not yet tested). + +Refs #24. + +## 1. Gate conditions + +Every row must be green on both backends before Phase 3 starts: + +- every file in `conformance/` passes in `swift test` and in `cargo test --release`; +- `conformance/mcp_wire_vectors.json` has an empty `known_drift` list; +- the reverse-attach negatives pass in both suites; +- the per-tool table in section 5 has no open row; +- the profile rows in section 6 are signed off after issue #59 lands. + +## 2. Wire layer (JSON-RPC) + +One row per case in `conformance/mcp_wire_vectors.json`. The Swift result is the +oracle. The Rust column reflects the `known_drift` list. + +| Case | Swift | Rust | Test | +|------|-------|------|------| +| initialize negotiates 2024-11-05 | protocolVersion 2024-11-05 | pass | `WireConformanceTests`, `wire_conformance` | +| initialize negotiates 2025-06-18 | protocolVersion 2025-06-18 | drift: returns 2024-11-05 | same | +| initialize with unknown version | falls back to 2025-06-18 | drift: returns 2024-11-05 | same | +| ping | result `{}` | drift: error -32601 | same | +| tools/list shape | tools with name, description, inputSchema | pass | same | +| tools/call unknown tool | error -32602 | drift: error -32601 | same | +| tools/call missing name | error -32602 | pass | same | +| unknown method | error -32601 | pass | same | +| notification | no response | pass | same | +| malformed JSON | error -32700 | drift: no response | same | +| jsonrpc "1.0" | error -32600 | drift: result returned | same | +| batch array | error -32600 | drift: no response | same | + +## 3. HTTP layer (not in the fixtures) + +The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. + +| Behaviour | Swift | Rust | Status | +|-----------|-------|------|--------| +| `Mcp-Session-Id` unknown on POST | 404 | not enforced (no session state) | open | +| `DELETE /mcp` | ends the session, 204 | bare 204, no session | open | +| non-JSON Content-Type on POST | 415 | no check | open | +| malformed JSON body on POST | 400 with JSON-RPC error -32700 | 202, no body | open (follows from the wire drift above) | +| `GET /mcp` | 405 | 405 | pass (not tested) | + +## 4. Reverse attach + +| Row | Swift | Rust | Status | +|-----|-------|------|--------| +| close-code and handshake disposition | `ConformanceVectorTests` | `attach::conformance` | pass | +| attach URL | vectors | vectors | pass | +| dial with bearer, expected upgrade headers | `ReverseAttachTests` (WS to fake runtime) | `attach::client::tests` | pass (ws:// only) | +| refused upgrade (401) stops after one dial | `ReverseAttachRefusedUpgradeTests` | `attach::client::tests` | pass | +| constant-time bearer compare (same length, case, trailing space) | `auth_vectors.json` | `auth::conformance` | pass | +| sha256 verifier (runtime stores the hash only) | not in this repo | not in this repo | open (runtime side, openab-pty) | +| credential never in shell output (grant body) | `AttachCredentialSecrecyTests` | `attach::grant_secrecy_tests` | pass (GET bodies, POST response) | +| pod stores only the verifier | not in this repo | not in this repo | open (runtime side) | +| successful wss:// handshake | not tested | not tested | open (needs a TLS fixture) | +| wss:// never falls back to plaintext | covered by switchboard tests | `attach::client::tests` | pass (Rust) | +| TLS certificate verification | not tested | not tested | open (needs a cert fixture) | +| origin policy on the upgrade | undefined | undefined | open (decision needed) | + +## 5. Per-tool table + +Cells come from the source files on each side. The Swift names are from +`Sources/InstanceMCPCore/Tools/` and `UpstreamMCP.swift`. The Rust names are +from `poc/reverse-attach-linux/src/mcp.rs` (`LOCAL_TOOL_NAMES`) and +`src/tools/`. The classification column is owned by #59 (see section 6), so it is +not filled here. + +| Tool | Swift present | Rust present | Classification | Conformance test | +|------|---------------|--------------|----------------|------------------| +| `sys_info` | `SysInfoTool` | yes (`tools/sysinfo.rs`) | see #59 | none beyond tools/list shape | +| `screenshot` | `ScreenshotTool` | yes (`tools/screen.rs`) | see #59 | none beyond tools/list shape | +| shell (`exec` in Swift, `bash` in Rust) | `ExecTool` | yes (`tools/bash.rs`) | see #59 | none. Name differs between backends | +| `mouse` | `MouseTool` | yes (`tools/input.rs`) | see #59 | none | +| `key` | `KeyTool` | yes (`tools/input.rs`) | see #59 | none | +| `osascript` | `OsascriptTool` | no (macOS only) | see #59 | none | +| `exec_start`, `exec_poll`, `exec_list`, `exec_cancel` | `AsyncExecTools` | no | see #59 | none | +| `browser_*` (upstream Playwright) | `UpstreamMCP` | no | see #59 | none | + +## 6. Profile rows + +Owned by issue #59 (profile collapse). Placeholder only. This PR does not +change profile semantics or profile tests. The rows to fill in after #59 are: +per-connection enforcement at dispatch (owner, desktop, observe), sandbox denies +`exec*`, and the shell-equivalence rule on Linux. + +## 7. Open decisions + +- Origin policy for the WebSocket upgrade and for `/mcp`. Neither backend defines one. +- Certificate pinning or verification for `wss://` runtimes. +- Whether the Rust `ping` gap (section 2) blocks Phase 3, or is fixed first. diff --git a/poc/reverse-attach-linux/src/attach/client.rs b/poc/reverse-attach-linux/src/attach/client.rs index 0d0551d..8e53603 100644 --- a/poc/reverse-attach-linux/src/attach/client.rs +++ b/poc/reverse-attach-linux/src/attach/client.rs @@ -330,4 +330,8 @@ fn handshake_status(err: &tungstenite::Error) -> Option { } } +#[cfg(test)] +#[path = "client_tests.rs"] +mod tests; + // --------------------------------------------------------------------------- diff --git a/poc/reverse-attach-linux/src/attach/client_tests.rs b/poc/reverse-attach-linux/src/attach/client_tests.rs new file mode 100644 index 0000000..0649aa6 --- /dev/null +++ b/poc/reverse-attach-linux/src/attach/client_tests.rs @@ -0,0 +1,216 @@ +//! Dial-side negatives for reverse attach, run against a plain TCP listener +//! standing in for the runtime. They pin what the Rust dialer sends on the +//! upgrade and how it reads a refused upgrade (the Swift oracle is +//! `ReverseAttachNegativeTests`). + +use std::net::TcpListener; +use std::panic::AssertUnwindSafe; +use std::sync::{Arc, Mutex}; +use std::thread; +use std::time::Duration; + +use tungstenite::handshake::server::{ + ErrorResponse, Request as ServerRequest, Response as ServerResponse, +}; +use tungstenite::http::{Request, Response}; + +use super::*; + +/// What the one-shot runtime saw on the upgrade request. +#[derive(Clone, Debug, Default)] +struct Seen { + connections: usize, + authorization: Option, + upgrade: Option, + connection: Option, +} + +/// Accept one upgrade on 127.0.0.1:0. A request whose `Authorization` is not +/// exactly `Bearer ` gets 401. Returns the port and a join handle that +/// yields what was seen, after a short wait for any second connection. +fn one_shot_runtime(expected: &'static str) -> (u16, thread::JoinHandle) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let handle = thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let seen = Arc::new(Mutex::new(Seen { + connections: 1, + ..Seen::default() + })); + let cb_seen = seen.clone(); + let want = format!("Bearer {expected}"); + #[allow(clippy::result_large_err)] // ErrorResponse is fixed by tungstenite's callback type + let callback = move |req: &ServerRequest, + resp: ServerResponse| + -> Result { + let header = |name: &str| { + req.headers() + .get(name) + .and_then(|v| v.to_str().ok()) + .map(str::to_owned) + }; + let mut s = cb_seen.lock().unwrap(); + s.authorization = header("authorization"); + s.upgrade = header("upgrade"); + s.connection = header("connection"); + if s.authorization.as_deref() == Some(want.as_str()) { + Ok(resp) + } else { + Err(Response::builder().status(401).body(None).unwrap()) + } + }; + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .unwrap(); + let _ = tungstenite::accept_hdr(stream, callback); + + // A dialer that retried on its own would show up here as a second connection. + thread::sleep(Duration::from_millis(300)); + listener.set_nonblocking(true).unwrap(); + let extra = listener.accept().is_ok() as usize; + let mut out = seen.lock().unwrap().clone(); + out.connections += extra; + out + }); + (port, handle) +} + +/// Accept one connection and read what arrives first. Used to see whether a +/// `wss://` dial sends anything that looks like HTTP to a plaintext listener. +fn plaintext_listener() -> (u16, thread::JoinHandle>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let handle = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .unwrap(); + let mut buf = vec![0u8; 512]; + let n = std::io::Read::read(&mut stream, &mut buf).unwrap_or(0); + buf.truncate(n); + buf + }); + (port, handle) +} + +fn upgrade_url(port: u16) -> String { + format!("ws://127.0.0.1:{port}/tools/attach/s1") +} + +/// The same request `build_ws_request` builds, minus the `Authorization` header. +fn upgrade_without_authorization(url: &str) -> Request<()> { + Request::builder() + .method("GET") + .uri(url) + .header( + "Host", + url.trim_start_matches("ws://").split('/').next().unwrap(), + ) + .header("Connection", "Upgrade") + .header("Upgrade", "websocket") + .header("Sec-WebSocket-Version", "13") + .header( + "Sec-WebSocket-Key", + tungstenite::handshake::client::generate_key(), + ) + .body(()) + .unwrap() +} + +#[test] +fn right_bearer_upgrades_with_the_expected_headers() { + let (port, server) = one_shot_runtime("s3cret"); + let req = build_ws_request(&upgrade_url(port), "s3cret").unwrap(); + let (_socket, resp) = match tungstenite::connect(req) { + Ok(ok) => ok, + Err(e) => panic!("upgrade with the right bearer was refused: {e}"), + }; + assert_eq!(resp.status().as_u16(), 101); + let seen = server.join().unwrap(); + assert_eq!(seen.authorization.as_deref(), Some("Bearer s3cret")); + assert_eq!( + seen.upgrade + .as_deref() + .map(str::to_ascii_lowercase) + .as_deref(), + Some("websocket") + ); + assert!(seen + .connection + .as_deref() + .unwrap_or("") + .to_ascii_lowercase() + .contains("upgrade")); + assert_eq!(seen.connections, 1); +} + +#[test] +fn same_length_wrong_bearer_is_refused_once_and_stops() { + let (port, server) = one_shot_runtime("s3cret"); + let req = build_ws_request(&upgrade_url(port), "s3crXt").unwrap(); + let err = match tungstenite::connect(req) { + Err(e) => e, + Ok(_) => panic!("a wrong bearer must not upgrade"), + }; + assert_eq!(handshake_status(&err), Some(401)); + assert!(matches!( + disposition_handshake(401), + Disposition::Stop(ref r) if r == "handshake_rejected_401" + )); + assert_eq!(server.join().unwrap().connections, 1); +} + +#[test] +fn empty_bearer_is_refused_once_and_stops() { + let (port, server) = one_shot_runtime("s3cret"); + let req = build_ws_request(&upgrade_url(port), "").unwrap(); + let err = match tungstenite::connect(req) { + Err(e) => e, + Ok(_) => panic!("an empty bearer must not upgrade"), + }; + assert_eq!(handshake_status(&err), Some(401)); + assert_eq!(server.join().unwrap().connections, 1); +} + +#[test] +fn missing_authorization_header_is_refused_once_and_stops() { + let (port, server) = one_shot_runtime("s3cret"); + let req = upgrade_without_authorization(&upgrade_url(port)); + let err = match tungstenite::connect(req) { + Err(e) => e, + Ok(_) => panic!("an upgrade without Authorization must not succeed"), + }; + assert_eq!(handshake_status(&err), Some(401)); + assert!(matches!( + disposition_handshake(401), + Disposition::Stop(ref r) if r == "handshake_rejected_401" + )); + let seen = server.join().unwrap(); + assert_eq!(seen.authorization, None); + assert_eq!(seen.connections, 1); +} + +#[test] +fn wss_to_a_plaintext_listener_fails_without_falling_back() { + let (port, server) = plaintext_listener(); + let url = format!("wss://127.0.0.1:{port}/tools/attach/s1"); + let req = build_ws_request(&url, "s3cret").unwrap(); + // The Linux build has no process-level rustls CryptoProvider, so tungstenite can + // panic while it sets up TLS. That is a separate, pre-existing gap (a wss:// runtime + // would panic the dial thread), reported rather than fixed here. For this test both + // a TLS error and that panic mean the dial did not succeed. + let dialed = std::panic::catch_unwind(AssertUnwindSafe(|| tungstenite::connect(req).is_ok())); + assert!( + !matches!(dialed, Ok(true)), + "wss to a plaintext listener upgraded" + ); + let bytes = server.join().unwrap(); + assert!( + !bytes.starts_with(b"GET "), + "dialer fell back to plaintext HTTP" + ); + assert!( + !bytes.windows(4).any(|w| w == b"HTTP"), + "dialer sent an HTTP request" + ); +} diff --git a/poc/reverse-attach-linux/src/attach/grant_secrecy_tests.rs b/poc/reverse-attach-linux/src/attach/grant_secrecy_tests.rs new file mode 100644 index 0000000..c9f1909 --- /dev/null +++ b/poc/reverse-attach-linux/src/attach/grant_secrecy_tests.rs @@ -0,0 +1,35 @@ +//! The attach secret is kept on the node so a grant can resume after a restart +//! (#12). It must never appear in the grant body that `GET /attach` and +//! `GET /attach/{id}` return. The Swift oracle is +//! `AttachCredentialSecrecyTests`. + +use super::*; + +const CANARY: &str = "CANARY-SECRET-7f3a"; + +#[test] +fn grant_json_never_carries_the_attach_secret() { + for ended in [None, Some("revoked".to_string())] { + let g = GrantInfo { + id: "grant-1-1".to_string(), + runtime: "ws://10.0.0.2:8090".to_string(), + session: "s1".to_string(), + profile: "owner".to_string(), + principal: "a@b".to_string(), + state: "attached".to_string(), + ended, + expires_at_epoch_secs: now_epoch_secs() + 600, + cancelled: Arc::new(AtomicBool::new(false)), + secret: CANARY.to_string(), + }; + let body = grant_json(&g); + assert!( + !body.to_string().contains(CANARY), + "attach secret leaked into the grant body: {body}" + ); + assert!( + body.get("secret").is_none(), + "grant body has a secret key: {body}" + ); + } +} diff --git a/poc/reverse-attach-linux/src/attach/mod.rs b/poc/reverse-attach-linux/src/attach/mod.rs index 4a6ca60..3f48399 100644 --- a/poc/reverse-attach-linux/src/attach/mod.rs +++ b/poc/reverse-attach-linux/src/attach/mod.rs @@ -8,6 +8,8 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::{json, Value}; pub mod client; +#[cfg(test)] +mod grant_secrecy_tests; pub mod store; // Shared grant registry diff --git a/poc/reverse-attach-linux/src/main.rs b/poc/reverse-attach-linux/src/main.rs index e8dae80..f0d0e05 100644 --- a/poc/reverse-attach-linux/src/main.rs +++ b/poc/reverse-attach-linux/src/main.rs @@ -12,6 +12,8 @@ mod http; mod mcp; mod platform; mod tools; +#[cfg(test)] +mod wire_conformance; fn main() { // Input devices first, so clients see pointer/keyboard capabilities before any tool diff --git a/poc/reverse-attach-linux/src/wire_conformance.rs b/poc/reverse-attach-linux/src/wire_conformance.rs new file mode 100644 index 0000000..ff34f29 --- /dev/null +++ b/poc/reverse-attach-linux/src/wire_conformance.rs @@ -0,0 +1,108 @@ +//! `conformance/mcp_wire_vectors.json` against `mcp::answer`. The Swift +//! `MCPServer` is the oracle. Each case the Rust port gets wrong is pinned in +//! `known_drift` with the output it returns today, so the drift stays visible +//! and any change to it (fixed or worse) fails this test. + +use serde_json::Value; + +use crate::mcp::answer; + +const VECTORS: &str = include_str!("../../../conformance/mcp_wire_vectors.json"); + +/// What a client sees for one raw request, in the vocabulary the fixture uses. +fn observe(request: &str, profile: &str) -> Value { + match answer(request, profile) { + None => serde_json::json!({ "response": false }), + Some(text) => { + let v: Value = serde_json::from_str(&text).expect("answer() returns JSON"); + let mut o = serde_json::json!({ "response": true }); + if let Some(code) = v.get("error").and_then(|e| e.get("code")) { + o["error"] = code.clone(); + } + if let Some(result) = v.get("result") { + o["result"] = result.clone(); + } + o + } + } +} + +/// Subset match, same rules as the Swift runner. Unknown keys fail. +fn matches(obs: &Value, want: &Value) -> bool { + let Some(w) = want.as_object() else { + return false; + }; + w.iter().all(|(key, value)| match key.as_str() { + "response" | "error" => obs.get(key) == Some(value), + "result_subset" => match ( + obs.get("result").and_then(Value::as_object), + value.as_object(), + ) { + (Some(res), Some(sub)) => sub.iter().all(|(k, v)| res.get(k) == Some(v)), + _ => false, + }, + "result_tools_shape" => obs["result"]["tools"].as_array().is_some_and(|tools| { + !tools.is_empty() + && tools.iter().all(|t| { + t["name"].is_string() + && t["description"].is_string() + && t["inputSchema"].is_object() + }) + }), + _ => false, + }) +} + +#[test] +fn wire_fixtures_match_the_swift_oracle_and_pin_known_drift() { + let doc: Value = serde_json::from_str(VECTORS).unwrap(); + let cases = doc["cases"].as_array().unwrap(); + let drift = doc["known_drift"].as_array().unwrap(); + assert!( + cases.len() >= 12, + "wire vector file shrank: {}", + cases.len() + ); + + let mut failures = Vec::new(); + for c in cases { + let name = c["name"].as_str().unwrap(); + let request = c["request"].as_str().unwrap(); + let profile = c["profile"].as_str().unwrap_or("owner"); + let obs = observe(request, profile); + let expect = &c["expect"]; + match drift.iter().find(|d| d["name"] == c["name"]) { + None => { + if !matches(&obs, expect) { + failures.push(format!("{name}: got {obs}, oracle wants {expect}")); + } + } + Some(d) => { + let pinned = &d["rust_today"]; + if !matches(&obs, pinned) { + failures.push(format!( + "{name}: known drift changed: got {obs}, pinned {pinned}" + )); + } + if matches(&obs, expect) { + failures.push(format!( + "{name}: now matches the oracle; remove it from known_drift" + )); + } + } + } + } + for d in drift { + if !cases.iter().any(|c| c["name"] == d["name"]) { + failures.push(format!( + "known_drift names a case that does not exist: {}", + d["name"] + )); + } + } + assert!( + failures.is_empty(), + "wire conformance:\n{}", + failures.join("\n") + ); +} From c083da68bff2471eb8530a1a6a31114c79062ebb Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 29 Oct 2026 11:25:46 -0400 Subject: [PATCH 2/4] test(conformance): address review of wire fixtures and negatives (refs #24) Review (CHANGES_REQUESTED) on 7a55843 asked for two fixes. B1: the Rust refused-upgrade tests called tungstenite::connect directly, so they did not exercise the production dialer. They now run dial_loop against a runtime that answers 401 and keeps accepting for 2.5 s. The grant deadline is 3 s away, so a redial would land inside the window and be counted. Tests: - same_length_wrong_bearer_is_refused_once_and_stops - empty_bearer_is_refused_once_and_stops Both assert state "ended", reason "handshake_rejected_401", and exactly one connection. The missing-Authorization test is documented as a runtime-side refusal check. build_ws_request always sends Authorization, so it does not test the dialer. The wss test is documented as tungstenite-level. Mutation check (macmini copy only, not committed): changing dial_loop to redial on 401 fails both dial_loop tests above. B2: the verification the plan requires is now recorded below. Also changed: - right_bearer_upgrade_sends_websocket_headers is split out of the right-bearer test, so the header check has its own test. - docs/parity-checklist.md: the "refused upgrade" row names the dial_loop test. The "GET /mcp 405" row is "open (not tested)", matching the legend. The Swift wss cell says "not tested" instead of "covered by switchboard tests", because SwitchboardTests only checks URL validation. Verification (macmini, arm64 macOS; nothing built on the laptop) - swift test: 139 passed, 0 failed, 0 unexpected. - cargo test --release, native macOS: fails at link with undefined _prctl. The crate is Linux-only (seat.rs calls prctl). This is an environment limit, not a test result. - cargo test --release --locked in rust:latest on macmini (rustc 1.99.1, linux aarch64, OrbStack docker): 26 passed, 0 failed. Baseline was 18 on de5e211; this branch adds 8 (7 in 7a55843, plus the split header test). - cargo test --release --locked wire_conformance: 1 passed. - cargo test --release --locked attach::client::tests: 6 passed. - cargo test --release --locked grant_secrecy: 1 passed. - cargo fmt --check: pass (after rustfmt on client_tests.rs). - cargo clippy --release --locked -- -D warnings: pass (CI command). - cargo build --release --locked: pass. - bash smoke.sh in the same Linux container (python3, procps, curl installed there): RESULT 48 passed, 0 failed. - markdownlint-cli2@0.17.2 "docs/**/*.md" (the version the pinned CI action uses): 0 errors in 11 files. Newer markdownlint versions report MD060 on existing docs too; CI does not use them. - json.tool on conformance/mcp_wire_vectors.json and auth_vectors.json: valid. Still open for #24 after this PR (unchanged from 7a55843) - 1: 7 known_drift Rust fixes in mcp.rs (wire layer). HTTP-layer gaps are in docs/parity-checklist.md section 3. - 2a: successful wss:// handshake (needs a TLS fixture). Also: wss on Linux panics without a process-level rustls CryptoProvider (pre-existing). - 2b: sha256 verifier (runtime side, openab-pty, not in this repo). - 2c: pod stores only the verifier (runtime side). - 2d: per-connection profile enforcement at dispatch (#59). - 2e: origin policy and certificate verification. - 3 and 4 are done by this PR. --- docs/parity-checklist.md | 6 +- .../src/attach/client_tests.rs | 239 +++++++++++++----- 2 files changed, 174 insertions(+), 71 deletions(-) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 0145264..5366437 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -50,7 +50,7 @@ The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. | `DELETE /mcp` | ends the session, 204 | bare 204, no session | open | | non-JSON Content-Type on POST | 415 | no check | open | | malformed JSON body on POST | 400 with JSON-RPC error -32700 | 202, no body | open (follows from the wire drift above) | -| `GET /mcp` | 405 | 405 | pass (not tested) | +| `GET /mcp` | 405 | 405 | open (not tested) | ## 4. Reverse attach @@ -59,13 +59,13 @@ The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. | close-code and handshake disposition | `ConformanceVectorTests` | `attach::conformance` | pass | | attach URL | vectors | vectors | pass | | dial with bearer, expected upgrade headers | `ReverseAttachTests` (WS to fake runtime) | `attach::client::tests` | pass (ws:// only) | -| refused upgrade (401) stops after one dial | `ReverseAttachRefusedUpgradeTests` | `attach::client::tests` | pass | +| refused upgrade (401) stops after one dial | `ReverseAttachRefusedUpgradeTests` | `attach::client::tests`, `dial_loop` runs against the refusing runtime | pass | | constant-time bearer compare (same length, case, trailing space) | `auth_vectors.json` | `auth::conformance` | pass | | sha256 verifier (runtime stores the hash only) | not in this repo | not in this repo | open (runtime side, openab-pty) | | credential never in shell output (grant body) | `AttachCredentialSecrecyTests` | `attach::grant_secrecy_tests` | pass (GET bodies, POST response) | | pod stores only the verifier | not in this repo | not in this repo | open (runtime side) | | successful wss:// handshake | not tested | not tested | open (needs a TLS fixture) | -| wss:// never falls back to plaintext | covered by switchboard tests | `attach::client::tests` | pass (Rust) | +| wss:// never falls back to plaintext | not tested (`SwitchboardTests` checks URL validation only) | `attach::client::tests` (tungstenite connect, not `dial_loop`) | pass (Rust) | | TLS certificate verification | not tested | not tested | open (needs a cert fixture) | | origin policy on the upgrade | undefined | undefined | open (decision needed) | diff --git a/poc/reverse-attach-linux/src/attach/client_tests.rs b/poc/reverse-attach-linux/src/attach/client_tests.rs index 0649aa6..afcfdb6 100644 --- a/poc/reverse-attach-linux/src/attach/client_tests.rs +++ b/poc/reverse-attach-linux/src/attach/client_tests.rs @@ -1,13 +1,18 @@ //! Dial-side negatives for reverse attach, run against a plain TCP listener //! standing in for the runtime. They pin what the Rust dialer sends on the -//! upgrade and how it reads a refused upgrade (the Swift oracle is +//! upgrade and how it reacts to a refused upgrade (the Swift oracle is //! `ReverseAttachNegativeTests`). +//! +//! The refused-upgrade cases run the production `dial_loop`, so a regression that +//! redials after a 401 fails here. The wss case and the missing-header case call +//! `tungstenite::connect` directly and do not exercise `dial_loop`. +use std::collections::HashMap; use std::net::TcpListener; use std::panic::AssertUnwindSafe; use std::sync::{Arc, Mutex}; use std::thread; -use std::time::Duration; +use std::time::{Duration, Instant}; use tungstenite::handshake::server::{ ErrorResponse, Request as ServerRequest, Response as ServerResponse, @@ -16,44 +21,29 @@ use tungstenite::http::{Request, Response}; use super::*; -/// What the one-shot runtime saw on the upgrade request. -#[derive(Clone, Debug, Default)] -struct Seen { - connections: usize, - authorization: Option, - upgrade: Option, - connection: Option, -} - /// Accept one upgrade on 127.0.0.1:0. A request whose `Authorization` is not /// exactly `Bearer ` gets 401. Returns the port and a join handle that -/// yields what was seen, after a short wait for any second connection. -fn one_shot_runtime(expected: &'static str) -> (u16, thread::JoinHandle) { +/// yields the Authorization value of each request seen. +fn one_shot_runtime(expected: &'static str) -> (u16, thread::JoinHandle>>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let port = listener.local_addr().unwrap().port(); let handle = thread::spawn(move || { let (stream, _) = listener.accept().unwrap(); - let seen = Arc::new(Mutex::new(Seen { - connections: 1, - ..Seen::default() - })); + let seen = Arc::new(Mutex::new(None)); let cb_seen = seen.clone(); let want = format!("Bearer {expected}"); #[allow(clippy::result_large_err)] // ErrorResponse is fixed by tungstenite's callback type let callback = move |req: &ServerRequest, resp: ServerResponse| -> Result { - let header = |name: &str| { - req.headers() - .get(name) - .and_then(|v| v.to_str().ok()) - .map(str::to_owned) - }; - let mut s = cb_seen.lock().unwrap(); - s.authorization = header("authorization"); - s.upgrade = header("upgrade"); - s.connection = header("connection"); - if s.authorization.as_deref() == Some(want.as_str()) { + let header = req + .headers() + .get("authorization") + .and_then(|v| v.to_str().ok()) + .map(str::to_owned); + let ok = header.as_deref() == Some(want.as_str()); + *cb_seen.lock().unwrap() = Some(header); + if ok { Ok(resp) } else { Err(Response::builder().status(401).body(None).unwrap()) @@ -63,14 +53,63 @@ fn one_shot_runtime(expected: &'static str) -> (u16, thread::JoinHandle) { .set_read_timeout(Some(Duration::from_secs(5))) .unwrap(); let _ = tungstenite::accept_hdr(stream, callback); + let authorization = seen.lock().unwrap().clone().flatten(); + vec![authorization] + }); + (port, handle) +} - // A dialer that retried on its own would show up here as a second connection. - thread::sleep(Duration::from_millis(300)); - listener.set_nonblocking(true).unwrap(); - let extra = listener.accept().is_ok() as usize; - let mut out = seen.lock().unwrap().clone(); - out.connections += extra; - out +/// Accept TCP connections for `linger` and refuse every upgrade whose +/// `Authorization` is not `Bearer `. Returns the Authorization value of +/// each connection, so a redial shows up as a second entry. +#[allow(clippy::type_complexity)] +fn refusing_runtime( + expected: &'static str, + linger: Duration, +) -> (u16, thread::JoinHandle>>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + listener.set_nonblocking(true).unwrap(); + let handle = thread::spawn(move || { + let mut seen = Vec::new(); + let until = Instant::now() + linger; + while Instant::now() < until { + let stream = match listener.accept() { + Ok((stream, _)) => stream, + Err(_) => { + thread::sleep(Duration::from_millis(20)); + continue; + } + }; + stream.set_nonblocking(false).unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .unwrap(); + let want = format!("Bearer {expected}"); + let authorization = Arc::new(Mutex::new(None)); + let cb_seen = authorization.clone(); + #[allow(clippy::result_large_err)] + // ErrorResponse is fixed by tungstenite's callback type + let callback = move |req: &ServerRequest, + resp: ServerResponse| + -> Result { + let header = req + .headers() + .get("authorization") + .and_then(|v| v.to_str().ok()) + .map(str::to_owned); + let ok = header.as_deref() == Some(want.as_str()); + *cb_seen.lock().unwrap() = Some(header); + if ok { + Ok(resp) + } else { + Err(Response::builder().status(401).body(None).unwrap()) + } + }; + let _ = tungstenite::accept_hdr(stream, callback); + seen.push(authorization.lock().unwrap().clone().flatten()); + } + seen }); (port, handle) } @@ -117,6 +156,54 @@ fn upgrade_without_authorization(url: &str) -> Request<()> { .unwrap() } +/// Run the production `dial_loop` once against a runtime that expects `s3cret`, +/// with `secret` as the grant's bearer. Returns the grant state and end reason +/// after the loop returns, and the Authorization value of every connection. +/// +/// The runtime lingers 2.5 s. The grant deadline is 3 s away. A redial would start +/// after the 1 s first backoff, so it would land inside the linger window and +/// show up as a second entry. Without a redial the loop returns at once with a +/// handshake end reason. +fn dial_loop_against_refusing_runtime( + secret: &str, +) -> (Option, Option, Vec>) { + let (port, server) = refusing_runtime("s3cret", Duration::from_millis(2500)); + let runtime = format!("ws://127.0.0.1:{port}"); + let registry: Registry = Arc::new(Mutex::new(HashMap::new())); + let grant_id = "grant-dial-test".to_string(); + registry.lock().unwrap().insert( + grant_id.clone(), + GrantInfo { + id: grant_id.clone(), + runtime: runtime.clone(), + session: "s1".to_string(), + profile: "owner".to_string(), + principal: "a@b".to_string(), + state: "dialing".to_string(), + ended: None, + expires_at_epoch_secs: now_epoch_secs() + 600, + cancelled: Arc::new(AtomicBool::new(false)), + secret: secret.to_string(), + }, + ); + dial_loop(DialGrant { + runtime, + session: "s1".to_string(), + secret: secret.to_string(), + profile: "owner".to_string(), + deadline_epoch_secs: now_epoch_secs() + 3, + registry: registry.clone(), + grant_id: grant_id.clone(), + cancelled: Arc::new(AtomicBool::new(false)), + }); + let (state, ended) = { + let map = registry.lock().unwrap(); + let g = map.get(&grant_id).unwrap(); + (Some(g.state.clone()), g.ended.clone()) + }; + (state, ended, server.join().unwrap()) +} + #[test] fn right_bearer_upgrades_with_the_expected_headers() { let (port, server) = one_shot_runtime("s3cret"); @@ -127,69 +214,85 @@ fn right_bearer_upgrades_with_the_expected_headers() { }; assert_eq!(resp.status().as_u16(), 101); let seen = server.join().unwrap(); - assert_eq!(seen.authorization.as_deref(), Some("Bearer s3cret")); + assert_eq!(seen, vec![Some("Bearer s3cret".to_string())]); +} + +#[test] +fn right_bearer_upgrade_sends_websocket_headers() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let server = thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let headers = Arc::new(Mutex::new((None, None))); + let cb = headers.clone(); + #[allow(clippy::result_large_err)] // ErrorResponse is fixed by tungstenite's callback type + let callback = move |req: &ServerRequest, + resp: ServerResponse| + -> Result { + let get = |name: &str| { + req.headers() + .get(name) + .and_then(|v| v.to_str().ok()) + .map(str::to_owned) + }; + *cb.lock().unwrap() = (get("upgrade"), get("connection")); + Ok(resp) + }; + let _ = tungstenite::accept_hdr(stream, callback); + let h = headers.lock().unwrap().clone(); + h + }); + let req = build_ws_request(&upgrade_url(port), "s3cret").unwrap(); + tungstenite::connect(req).expect("upgrade should succeed"); + let (upgrade, connection) = server.join().unwrap(); assert_eq!( - seen.upgrade - .as_deref() - .map(str::to_ascii_lowercase) - .as_deref(), + upgrade.as_deref().map(str::to_ascii_lowercase).as_deref(), Some("websocket") ); - assert!(seen - .connection + assert!(connection .as_deref() .unwrap_or("") .to_ascii_lowercase() .contains("upgrade")); - assert_eq!(seen.connections, 1); } #[test] fn same_length_wrong_bearer_is_refused_once_and_stops() { - let (port, server) = one_shot_runtime("s3cret"); - let req = build_ws_request(&upgrade_url(port), "s3crXt").unwrap(); - let err = match tungstenite::connect(req) { - Err(e) => e, - Ok(_) => panic!("a wrong bearer must not upgrade"), - }; - assert_eq!(handshake_status(&err), Some(401)); + let (state, ended, seen) = dial_loop_against_refusing_runtime("s3crXt"); + assert_eq!(state.as_deref(), Some("ended")); + assert_eq!(ended.as_deref(), Some("handshake_rejected_401")); + assert_eq!(seen, vec![Some("Bearer s3crXt".to_string())]); assert!(matches!( disposition_handshake(401), Disposition::Stop(ref r) if r == "handshake_rejected_401" )); - assert_eq!(server.join().unwrap().connections, 1); } #[test] fn empty_bearer_is_refused_once_and_stops() { - let (port, server) = one_shot_runtime("s3cret"); - let req = build_ws_request(&upgrade_url(port), "").unwrap(); - let err = match tungstenite::connect(req) { - Err(e) => e, - Ok(_) => panic!("an empty bearer must not upgrade"), - }; - assert_eq!(handshake_status(&err), Some(401)); - assert_eq!(server.join().unwrap().connections, 1); + let (state, ended, seen) = dial_loop_against_refusing_runtime(""); + assert_eq!(state.as_deref(), Some("ended")); + assert_eq!(ended.as_deref(), Some("handshake_rejected_401")); + assert_eq!(seen.len(), 1, "a redial happened: {seen:?}"); } +/// Runtime-side refusal only: `build_ws_request` always sends Authorization, so +/// the dialer never produces this request. The test checks that a runtime which +/// gets an upgrade with no Authorization refuses it, and that one connection was +/// made. #[test] -fn missing_authorization_header_is_refused_once_and_stops() { - let (port, server) = one_shot_runtime("s3cret"); +fn upgrade_without_authorization_is_refused_with_one_connection() { + let (port, server) = refusing_runtime("s3cret", Duration::from_millis(300)); let req = upgrade_without_authorization(&upgrade_url(port)); let err = match tungstenite::connect(req) { Err(e) => e, Ok(_) => panic!("an upgrade without Authorization must not succeed"), }; assert_eq!(handshake_status(&err), Some(401)); - assert!(matches!( - disposition_handshake(401), - Disposition::Stop(ref r) if r == "handshake_rejected_401" - )); - let seen = server.join().unwrap(); - assert_eq!(seen.authorization, None); - assert_eq!(seen.connections, 1); + assert_eq!(server.join().unwrap(), vec![None]); } +/// Calls `tungstenite::connect` directly, not `dial_loop`. #[test] fn wss_to_a_plaintext_listener_fails_without_falling_back() { let (port, server) = plaintext_listener(); From 371804962dd285a8ba8c06fcfac55516c093dd14 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 29 Oct 2026 11:30:54 -0400 Subject: [PATCH 3/4] docs(conformance): fix Swift cell for dial-with-bearer row in parity checklist (refs #24) Review finding 1: the checklist claimed a Swift pass for the bearer and upgrade-header check on the WS dial. FakeRuntime accepts any bearer and never reads Authorization, Upgrade, or Connection, so no Swift test asserts them. The bearer is only seen on the refused-upgrade (401) path, covered by ReverseAttachRefusedUpgradeTests. The Swift cell now says "not asserted" and the status is "pass (Rust); Swift open". Verification on macmini, run on the synced tree at this commit: - swift test: 139 executed, 0 failures (WireConformanceTests 1/1 pass, ReverseAttachNegativeTests pass, ConformanceVectorTests pass). - cargo test: native macOS does not link (undefined _prctl from platform/linux/seat.rs, which this PR does not touch; pre-existing). Ran in rust:latest Linux container via OrbStack on macmini with --locked: cargo fmt --check OK, clippy -D warnings OK, cargo build --release OK, cargo test --release: 26 passed, 0 failed. - python3 -m json.tool on conformance/mcp_wire_vectors.json and conformance/auth_vectors.json: OK. - markdownlint-cli2@0.18.1 "docs/**/*.md" (matches the markdownlint-cli2-action v19 pin): 0 errors. Newer markdownlint reports MD060 on existing docs, which is not from this PR. - smoke.sh: not run in this pass. --- docs/parity-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 5366437..59ce9ce 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -58,7 +58,7 @@ The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. |-----|-------|------|--------| | close-code and handshake disposition | `ConformanceVectorTests` | `attach::conformance` | pass | | attach URL | vectors | vectors | pass | -| dial with bearer, expected upgrade headers | `ReverseAttachTests` (WS to fake runtime) | `attach::client::tests` | pass (ws:// only) | +| dial with bearer, expected upgrade headers | not asserted (FakeRuntime accepts any bearer); bearer seen only on the 401 path, see `ReverseAttachRefusedUpgradeTests` | `attach::client::tests` | pass (Rust); Swift open | | refused upgrade (401) stops after one dial | `ReverseAttachRefusedUpgradeTests` | `attach::client::tests`, `dial_loop` runs against the refusing runtime | pass | | constant-time bearer compare (same length, case, trailing space) | `auth_vectors.json` | `auth::conformance` | pass | | sha256 verifier (runtime stores the hash only) | not in this repo | not in this repo | open (runtime side, openab-pty) | From 56d20ad96c55bc6c6ef26a3cc27c7feacc12d546 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:05:57 -0400 Subject: [PATCH 4/4] test(conformance): address round-1 review of #62 (refs #24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - parity-checklist §5: Rust does serve browser_* tools (mcp.rs Upstream via MCP_UPSTREAM, DESKTOP_BROWSER_TOOLS, upstream_tool_allowed); fix the cell and name that code among the section sources. - parity-checklist §4: rename the "constant-time" row to byte-exact compare (vectors check outcomes, not timing); note on the wss row that the Linux dial panics (no rustls CryptoProvider) and the test accepts it; link follow-up #66. - AttachCredentialSecrecyTests: assert 200 on GET /attach and GET /attach/, and that the grant id appears in the list body. - client_tests.rs: rename right_bearer_upgrades_with_the_expected_headers to right_bearer_is_sent_and_upgrades; widen the dial_loop grant deadline to now+4 s so it clearly outlasts the 2.5 s linger. - conformance/README.md: the no-Authorization case is a runtime-side check the dialer never sends, not a dial-side "missing bearer" negative. Correction to 3718049's message: the pinned markdownlint-cli2-action sha (v19.1.0) bundles markdownlint-cli2 0.17.2, not 0.18.1 (as c083da6 said). Verified on macmini: swift test 139 pass, 0 fail. In rust:1 container (rustc 1.99.1): cargo fmt --check ok; cargo clippy --release --locked --all-targets -D warnings ok; cargo test --release --locked 26 pass, 0 fail. markdownlint-cli2@0.17.2 on docs/** and conformance/*.md: 0 errors. --- .../ReverseAttachNegativeTests.swift | 3 +++ conformance/README.md | 5 +++-- docs/parity-checklist.md | 11 ++++++----- poc/reverse-attach-linux/src/attach/client_tests.rs | 11 ++++++----- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift b/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift index edcbd77..8124be7 100644 --- a/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift +++ b/Tests/InstanceMCPCoreTests/ReverseAttachNegativeTests.swift @@ -109,6 +109,9 @@ final class AttachCredentialSecrecyTests: XCTestCase { let list = await ep.handle(attachReq("GET", "/attach"), remoteIsLoopback: true) let one = await ep.handle(attachReq("GET", "/attach/\(id)"), remoteIsLoopback: true) + XCTAssertEqual(list.status, 200, "GET /attach: \(list.status)") + XCTAssertEqual(one.status, 200, "GET /attach/: \(one.status)") + XCTAssertTrue(String(decoding: list.body, as: UTF8.self).contains(id), "grant id missing from GET /attach") for r in [post, list, one] { XCTAssertFalse(String(decoding: r.body, as: UTF8.self).contains(canary), "secret leaked in \(r.status) body") diff --git a/conformance/README.md b/conformance/README.md index 32f8734..8e0a789 100644 --- a/conformance/README.md +++ b/conformance/README.md @@ -14,8 +14,9 @@ Both run in CI on every PR (`swift test`, `cargo test`). Change a vector only fo behaviour change, and make both implementations pass in the same PR. Dial-side and credential negatives that need code rather than a vector file live next to the -code: `ReverseAttachNegativeTests.swift` and `attach/client_tests.rs` (refused 401, wrong or -missing bearer, no plaintext fallback), `attach/grant_secrecy_tests.rs` and +code: `ReverseAttachNegativeTests.swift` and `attach/client_tests.rs` (refused 401, wrong, +same-length or empty bearer, no plaintext fallback; plus a runtime-side check that an upgrade +with no Authorization is refused, which the dialer never sends), `attach/grant_secrecy_tests.rs` and `AttachCredentialSecrecyTests` (the attach secret never appears in a grant body). Open items, the parity gate, and the known drift list are in diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 59ce9ce..8aaa880 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -60,12 +60,12 @@ The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. | attach URL | vectors | vectors | pass | | dial with bearer, expected upgrade headers | not asserted (FakeRuntime accepts any bearer); bearer seen only on the 401 path, see `ReverseAttachRefusedUpgradeTests` | `attach::client::tests` | pass (Rust); Swift open | | refused upgrade (401) stops after one dial | `ReverseAttachRefusedUpgradeTests` | `attach::client::tests`, `dial_loop` runs against the refusing runtime | pass | -| constant-time bearer compare (same length, case, trailing space) | `auth_vectors.json` | `auth::conformance` | pass | +| bearer compare is byte-exact (same length, case, trailing space); vectors check outcomes, not timing | `auth_vectors.json` | `auth::conformance` | pass | | sha256 verifier (runtime stores the hash only) | not in this repo | not in this repo | open (runtime side, openab-pty) | | credential never in shell output (grant body) | `AttachCredentialSecrecyTests` | `attach::grant_secrecy_tests` | pass (GET bodies, POST response) | | pod stores only the verifier | not in this repo | not in this repo | open (runtime side) | | successful wss:// handshake | not tested | not tested | open (needs a TLS fixture) | -| wss:// never falls back to plaintext | not tested (`SwitchboardTests` checks URL validation only) | `attach::client::tests` (tungstenite connect, not `dial_loop`) | pass (Rust) | +| wss:// never falls back to plaintext | not tested (`SwitchboardTests` checks URL validation only) | `attach::client::tests` (tungstenite connect, not `dial_loop`) | pass (Rust), with a caveat: on Linux the dial panics (no process-level rustls CryptoProvider) and the test accepts the panic, so it shows only that no plaintext HTTP is sent. Follow-up #66 | | TLS certificate verification | not tested | not tested | open (needs a cert fixture) | | origin policy on the upgrade | undefined | undefined | open (decision needed) | @@ -73,8 +73,9 @@ The fixtures stop at the JSON-RPC layer. These HTTP behaviours are open. Cells come from the source files on each side. The Swift names are from `Sources/InstanceMCPCore/Tools/` and `UpstreamMCP.swift`. The Rust names are -from `poc/reverse-attach-linux/src/mcp.rs` (`LOCAL_TOOL_NAMES`) and -`src/tools/`. The classification column is owned by #59 (see section 6), so it is +from `poc/reverse-attach-linux/src/mcp.rs` (`LOCAL_TOOL_NAMES`, and the +`Upstream` re-serving code with `DESKTOP_BROWSER_TOOLS` and +`upstream_tool_allowed`) and `src/tools/`. The classification column is owned by #59 (see section 6), so it is not filled here. | Tool | Swift present | Rust present | Classification | Conformance test | @@ -86,7 +87,7 @@ not filled here. | `key` | `KeyTool` | yes (`tools/input.rs`) | see #59 | none | | `osascript` | `OsascriptTool` | no (macOS only) | see #59 | none | | `exec_start`, `exec_poll`, `exec_list`, `exec_cancel` | `AsyncExecTools` | no | see #59 | none | -| `browser_*` (upstream Playwright) | `UpstreamMCP` | no | see #59 | none | +| `browser_*` (upstream Playwright) | `UpstreamMCP` | yes (`mcp.rs` `Upstream`, `MCP_UPSTREAM`); desktop allowlist `DESKTOP_BROWSER_TOOLS` matches Swift `ToolProfile.desktopBrowserTools` | see #59 | none | ## 6. Profile rows diff --git a/poc/reverse-attach-linux/src/attach/client_tests.rs b/poc/reverse-attach-linux/src/attach/client_tests.rs index afcfdb6..7f8447d 100644 --- a/poc/reverse-attach-linux/src/attach/client_tests.rs +++ b/poc/reverse-attach-linux/src/attach/client_tests.rs @@ -160,9 +160,10 @@ fn upgrade_without_authorization(url: &str) -> Request<()> { /// with `secret` as the grant's bearer. Returns the grant state and end reason /// after the loop returns, and the Authorization value of every connection. /// -/// The runtime lingers 2.5 s. The grant deadline is 3 s away. A redial would start -/// after the 1 s first backoff, so it would land inside the linger window and -/// show up as a second entry. Without a redial the loop returns at once with a +/// The runtime lingers 2.5 s. The grant deadline is 4 whole seconds away (more +/// than 3 s), so it outlasts the linger window. A redial would start after the +/// 1 s first backoff, so it would land inside the linger window and show up as a +/// second entry. Without a redial the loop returns at once with a /// handshake end reason. fn dial_loop_against_refusing_runtime( secret: &str, @@ -191,7 +192,7 @@ fn dial_loop_against_refusing_runtime( session: "s1".to_string(), secret: secret.to_string(), profile: "owner".to_string(), - deadline_epoch_secs: now_epoch_secs() + 3, + deadline_epoch_secs: now_epoch_secs() + 4, registry: registry.clone(), grant_id: grant_id.clone(), cancelled: Arc::new(AtomicBool::new(false)), @@ -205,7 +206,7 @@ fn dial_loop_against_refusing_runtime( } #[test] -fn right_bearer_upgrades_with_the_expected_headers() { +fn right_bearer_is_sent_and_upgrades() { let (port, server) = one_shot_runtime("s3cret"); let req = build_ws_request(&upgrade_url(port), "s3cret").unwrap(); let (_socket, resp) = match tungstenite::connect(req) {