mirror of
https://github.com/storytold/photocraft.git
synced 2026-11-12 08:51:51 +00:00
Michael Higgins
9d5be5ec8e
## Summary
`photocraft-cli mcp --bridge` sent the control token in clear as its first frame, to whatever had accepted the connection: a `[::1]` listener reached through a `localhost` lookup, or any process holding `127.0.0.1:<port>` while the app was not listening. This PR fixes both cases:
- **Literal loopback only.** `BridgeClient` parses its address into a socket address and connects to it. Only `127.0.0.1:<port>` and `[::1]:<port>` are accepted. `localhost:<port>` means `127.0.0.1:<port>` (what the desktop app binds) and is never looked up.
- **Challenge authentication.** The bridge authenticates with `auth.challenge {clientNonce}` → `{serverNonce, serverProof}` → `auth {proof}`. Both proofs are HMAC-SHA256 keyed by the token over both nonces, each under its own label. The client checks the server's proof before it sends its own and never falls back to sending the token. The token never crosses the wire, so a listener that is not the app gets only a random nonce.
- **One shared server implementation.** `security::ServerHandshake` replaces `authentication_reply`, and both the desktop control server and `photocraft-cli serve` use it. The servers still accept the one-frame `auth {token}`, so existing scripts and older clients keep working. A repeat `auth.challenge` after authenticating gets "already authenticated", like a repeat `auth`.
- HMAC comes from RustCrypto's `hmac` 0.12 with `sha2` 0.10 (`sha2` was already in the lockfile). The proofs are compared without an early exit.
- `docs/control-protocol.md` documents the handshake, the exact HMAC message, and the bridge's address rules. The book's protocol summary is updated too.
Compatibility: a new bridge needs an app from the same release. Against an older app it fails with "refused challenge authentication … the control token was not sent". It does not downgrade.
Known limit: the channel is plain TCP with no TLS, so nothing binds the handshake to the connection. A party sitting between a bridge and a real app could relay the handshake without learning the token. On loopback that needs it to hold the port the bridge dials while the app listens on another, which the literal-address rule above narrows. Older clients that still send `auth {token}` keep their old exposure until they upgrade.
Closes #9777
## Tests
New in `crates/automation/tests/it/bridge_token.rs`:
- `localhost_reaches_the_ipv4_app_not_an_ipv6_listener`: a listener on `[::1]:P` gets no connection, and the app on `127.0.0.1:P` does.
- `a_listener_that_is_not_the_app_never_receives_the_token`: on both loopbacks, a listener that stays silent, refuses, or fakes the challenge receives only the challenge frame, never the token. The error says why.
- `the_bridge_authenticates_to_the_headless_server_by_challenge`: end to end against `rpc::serve_tcp`. A wrong token is refused before its proof is sent.
Unit tests: the handshake in both directions, and that wrong, missing or replayed proofs, the token after a challenge, a second challenge and malformed nonces are all refused (`security.rs`). `loopback_socket` address parsing (`bridge.rs`). The desktop server answering a challenge, then dispatching (`control_server.rs`). The existing bridge and MCP fakes now run the real `ServerHandshake`.
Run locally (Windows 11, targeted tests only):
- **Fail before.** I put upstream's `crates/automation/src/bridge.rs` (the old client) back and ran `cargo test -p photocraft-automation --test it bridge_token`: **3 failed**. The `[::1]` listener received `{"method":"auth","params":{"token":"<the token>"}}`, a `127.0.0.1` listener received the raw token, and the headless test got "authentication required" instead of the refusal. The original audit tests (`audit_nv6_bridge_sends_token_to_ipv6_impostor`, `audit_nv6_bridge_sends_token_to_any_ipv4_holder`) fail the same way on upstream/main.
- **Pass after.**
- `cargo test -p photocraft-automation --lib --test it -- bridge security tcp`: 23 + 21 passed. This covers the new tests, the existing bridge, MCP-bridge, security and serve TCP tests, and the legacy-token tests that still pass unchanged.
- `cargo test -p photocraft control_server`: 6 passed, including the new challenge test. The legacy-token tests still pass.
- `cargo clippy -p photocraft-automation --all-targets -- -D warnings`: clean.
- `cargo fmt`: clean.
- `Cargo.lock` adds only `hmac` 0.12.1, plus `subtle` under `digest` (already in the lock). Checked with `cargo metadata --locked`.
Not run here: the full gate (`cargo xtask ...`, the workspace-wide tests, wasm, clippy on the desktop app crate). That is left to CI.
API note: the workspace-internal `security::authentication_reply` is replaced by `ServerHandshake::reply`, which returns an `AuthStep`.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
<!-- coderabbit-ai-generated:v1 -->
## Summary by CodeRabbit
* **Security**
* Control connections support challenge-response authentication, verifying possession of the configured token without sending it. Token-in-frame authentication remains available for scripts.
* The MCP bridge accepts literal loopback addresses and maps `localhost` directly to IPv4 loopback. It rejects servers that refuse or cannot complete challenge authentication.
* **Bug Fixes**
* Rejected authentication exchanges close the connection; successful authentication allows requests to proceed.
* **Documentation**
* Control protocol and bridge authentication behavior are documented.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->