fix: harden attach, pairing, and relay for v0.7.0 - #54
Conversation
Locator success now requires Noise KK against the pinned host key, and LAN dials only private, link-local, or loopback addresses. ICE disconnected no longer ends a live session. Native attach fetches ephemeral TURN, omits third-party STUN when TURN is present, and reconnects onto the same tmux session. Pairing prints the safety number after msg2 so a QR client can compare it before msg3 is sent. A live pin-set follows disk without restart. Pair bridges are capped in count, duration, and bytes. TURN credentials last 15 minutes with an origin allowlist, never CORS *. A revocation persist failure cannot return 204 and then vanish after restart. Verified registry host_pub and signal beat the unsigned local cache. SPA deploy refuses a dirty or digest-mismatched web tree.
bb29e9e to
ce34315
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb29e9eda8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case d.recv <- buf: | ||
| case <-d.closed: | ||
| default: | ||
| } |
There was a problem hiding this comment.
When more than 64 messages arrive before the consumer drains recv, this default branch silently drops an otherwise reliable SCTP message. These messages contain sequential Noise transport ciphertexts, so dropping one advances the sender's nonce without advancing the receiver's nonce; the next decrypt then fails and terminates the terminal session. This can occur during a burst of terminal output or when the browser/client is temporarily slow, so the callback needs lossless queueing or explicit flow control rather than frame dropping.
Useful? React with 👍 / 👎.
| // The cred is fetched per-attach and only grants relay bandwidth (Noise keeps | ||
| // content E2E; coturn quotas bound abuse), so a generous TTL is acceptable. | ||
| const turnTTL = 12 * time.Hour | ||
| const turnTTL = 15 * time.Minute |
There was a problem hiding this comment.
For users whose only viable ICE path is TURN, reducing the REST credential lifetime to 15 minutes makes coturn reject later allocation refreshes or permission creation after the embedded username expires. As the comment immediately above notes, this drops long-running terminal sessions; multi-machine attach does not use the new reconnect loop, and even reconnecting clients suffer periodic interruption. The credential lifetime must outlast the supported session duration or be refreshed without interrupting the existing allocation.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-time.After(backoff): |
There was a problem hiding this comment.
Every dial error is retried indefinitely here, including deterministic local errors such as an invalid cached host_pub, an unavailable signer/keychain, or malformed signaling configuration. Before this reconnect wrapper, mir attach surfaced those errors immediately; now the command appears to hang until interrupted and never tells the user what must be fixed. Only errors classified as transient/unreachable should enter the backoff loop, while permanent dial errors should be returned.
Useful? React with 👍 / 👎.
| defaultMaxAgentSessions = 128 | ||
| defaultMaxPairRooms = 1024 | ||
| defaultMaxPairBridges = 64 | ||
| defaultPairBridgeTTL = 60 * time.Second |
There was a problem hiding this comment.
The bridge timeout starts as soon as both peers rendezvous, but the new protocol deliberately pauses after msg2 while the user compares the six-part safety number before sending msg3. If that comparison takes more than 60 seconds, the relay closes both sockets and confirmation fails, despite the agent UI advertising a five-minute pairing window. The bridge should use the ceremony's advertised lifetime or apply an idle timeout that accommodates the human confirmation step.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ce34315. Configure here.
| rt.release() | ||
| } | ||
| } | ||
| defer releaseHS() |
There was a problem hiding this comment.
Admit burns attach session IDs
High Severity
authorizeOffer records the one-shot session via acceptAttachSession before admit runs. When the handshake semaphore is full, admit fails and the offer is dropped, but the session id stays marked used. A retry of the same offer hits the replay guard and fails, so load that trips the DoS cap can permanently kill otherwise valid attaches until the client gets a brand-new relay session.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ce34315. Configure here.
| return client.Attach(ctx, m, idn, iceList, *relayOnly) | ||
| }, func(ctx context.Context, mc peer.MsgConn, sess *noise.Session) error { | ||
| return client.RunInteractive(ctx, mc, sess, m.Name) | ||
| }) |
There was a problem hiding this comment.
Reconnect reuses expired TURN
Medium Severity
ResolveICE runs once before ReconnectLoop, while TURN credentials now expire in 15 minutes. Each redial reuses that same iceList. After a long session drop, reconnect can present expired TURN creds and fail on paths that need the relay, even though the agent fetches fresh TURN per offer.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ce34315. Configure here.
There was a problem hiding this comment.
Left a non-blocking comment; not approving. Cursor Bugbot completed with 2 unresolved findings, including a high-severity attach session-id issue, so this needs human review. Reviewers were assigned.
Sent by Cursor Approval Agent: Pull Request Router and Approver
1 participant
Summary
Closes the remaining security-review P0s so attach and pairing work under a hostile relay and LAN.
disconnecteddoes not end a live session. Native attach fetches ephemeral TURN (no Google STUN when TURN is present) and reconnects onto the same tmux session.pair-devpins a base58 owner id, not X25519 hex.*). A revocation persist failure cannot return 204 and then vanish after restart. SPA deploy refuses a dirty or digest-mismatched web tree.host_puband signal beat the unsigned local cache.Test plan
cd go && go test ./...cd go && go test -race -count=1on touched packagescd web && npm test(114)v0.7.0, deploymir-signal+ SPA to Lightsail withMIR_WEB_SHA256Note
High Risk
Touches attach authorization, pairing trust, LAN dial policy, TURN/CORS, revocation durability, and relay DoS bounds—security-critical paths under a hostile-relay threat model.
Overview
Closes remaining P0s so attach and pairing hold up against a hostile relay or LAN, and ships Miranda as v0.7.0.
Attach now treats a locator as a win only after Noise KK against the pinned host key (wrong-key or stalled LAN falls through). LAN mDNS addresses must be private/loopback/link-local. ICE
disconnectedno longer tears down a live session. Native attach prefers ephemeral TURN (no Google STUN when TURN is present) and redials onto the same tmux session. Handshake admit slots are released after KK so authenticated sessions do not starve new attaches. Unpinning an owner cancels its live serve loop.Pairing splits the handshake so the safety number is shown after msg2 and msg3 is withheld until confirm (CLI + web).
pair-devpins a base58 owner id, not X25519 hex. Verified registryhost_pub/signal beat the unsigned local cache.Relay / deploy: pair bridges are capped by count, TTL, and bytes. TURN creds last 15 minutes with an origin allowlist (never CORS
*); coturn denies RFC1918/loopback peers. Revocation persist failure rolls back and cannot 204 a non-durable tombstone. SPA deploy refuses a dirty or digest-mismatched web tarball.Reviewed by Cursor Bugbot for commit ce34315. Bugbot is set up for automated code reviews on this repo. Configure here.