Repository navigation
Add the loopback stream server behind the embedded terminal - #24
Merged
Merged
Conversation
Terminal I/O cannot go over the Wails bridge. A build log arrives as
thousands of small writes and every one of them would cross a JSON
marshaling boundary, so DESIGN.md §3.3 splits the transport: bindings for
RPC, a loopback WebSocket for throughput. This is that server, and the
first thing it carries is PTY I/O.
internal/stream serves 127.0.0.1 on an OS-assigned port with a per-launch
crypto/rand token. Two endpoints: /pty/{sessionID} carries the character
stream as binary frames with control messages as text, and /events carries
backend-push envelopes. The wire contract is specified in PROTOCOL.md next
to the package rather than left to be inferred from the handlers, because
the frontend is written against it.
Every refusal lands as an HTTP status before the upgrade — 401 for a
missing or wrong token, 403 for a disallowed origin, 404 for an unknown
session — so no client gets a socket it is not allowed to use, and an
unauthenticated caller cannot use the endpoint to discover which sessions
exist. The token is accepted as an Authorization header and as a
subprotocol, because the browser WebSocket API cannot set headers.
Backpressure is the property that makes the transport usable at all: a
webview that has stopped reading must not be able to stall the user's
shell. Each connection has a bounded queue that discards its oldest frame
when full, and every discarded byte is reported in a resync marker so a
renderer knows not to paint across a hole in an escape-sequence stream.
The two services do not know about each other. stream declares a Terminals
seam, pty knows nothing about transports, and internal/app holds the
adapter that joins them — so either can be replaced without touching the
other, and the import graph pin records that shape.
Also fixes a defect in #2 that this exposes: pty.Attachment had no way to
detach, so every reconnect left a consumer holding its queue on the session
until the session ended. Attachment.Detach closes that leak.
Ratchets raised, with the reason recorded next to each number:
internal/app 200 -> 260 LOC (the first service adapter), maxAppFields
2 -> 3 (one *stream.Server handle), maxAppMethods 1 -> 2 (StreamEndpoint;
terminal operations are frames, not bindings).
Closes #3
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 94.04% 94.53% +0.48%
==========================================
Files 8 14 +6
Lines 252 549 +297
==========================================
+ Hits 237 519 +282
- Misses 11 21 +10
- Partials 4 9 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3. Depends on #2 (merged). DESIGN.md §3.3.
The token-authenticated loopback WebSocket server that carries m6t's
throughput-sensitive traffic, and the first thing it carries: PTY I/O.
Why this exists
Terminal I/O cannot go over the Wails bridge. A build log arrives as thousands
of small writes and every one of them would cross a JSON marshaling boundary. So
the transport is split by payload profile (DESIGN.md §3.3): bindings for RPC,
loopback WebSocket for streams. The only thing the bridge carries for the
terminal is the endpoint that says where to connect and with what token.
What's in it
internal/stream— 838 LOC across six files, five exported names(
Server,New,Endpoint,Attachment,Terminals).127.0.0.1:0. Not configurable: a stream server reachablefrom another host is a shell server. Port 0 so two m6t instances can run side
by side.
crypto/rand.Text()— 26 base32 chars, ~130 bits. Mintedat construction, so a port learned from a previous run is useless.
GET /pty/{sessionID}— binary frames carry the character stream bothdirections, text frames carry control messages.
GET /events— backend-push envelopes. PTY exit today; git, watch and helmplug into the same envelope as they land, which is why it carries a
typerather than the endpoint implying one.
internal/stream/PROTOCOL.mdis the wirecontract. It is written down rather than left to be inferred from the
handlers, because the frontend is written against it. Read that first.
Architecture: why there is no
stream → ptyedgeThe stream server carries PTY bytes and does not import
internal/pty. Itdeclares a
Terminalsseam;internal/ptyknows nothing about transports; andinternal/app— the one layer that knows about both — holds the adapter(
internal/app/terminals.go). depguard enforces the rule andTestImportGraphIsPinnedrecords the resulting shape:Two service edges out of the binding layer, none between the services. The cost
is that
stream.Attachmentrestates the shape ofpty.Attachmentand theadapter translates. The benefit is that either service can be replaced without
touching the other, and a future
stream → ptyimport fails a test as well asthe linter.
Security
Every refusal lands as an HTTP status before the upgrade. No client ever gets
a socket it is not allowed to use, and no 101 is sent and then walked back.
401Originpresent and not allowed403404cannot use
/pty/{sessionID}to discover which session IDs exist. The attachcomes second, so an unknown session is a 404 on a plain response rather than a
socket that opens and dies. The upgrade is last, and it is where a bad origin
is refused.
crypto/subtle).Authorization: Bearer <token>for clients that canset headers, and a
m6t.token.<token>subprotocol for the browser WebSocketAPI, which cannot. The server negotiates
m6t.v1in that case because abrowser fails the connection if it offered subprotocols and the server selected
none. Same pattern the Kubernetes API server uses. Header wins when both are
present, so a stale subprotocol cannot override an explicit credential.
token is what guards that case), the Wails webview (
wails://…, andhttp://wails.localhoston Windows), or loopback on any port.null— what afile://document and a sandboxed frame report — is refused. This is whatcloses DNS rebinding: a rebound attacker page sends its own origin and gets a
403 before it can try the token it does not have.
that needs it and goes nowhere else — a token in a log file outlives the launch
it was minted for.
Backpressure
This is the property that makes the transport usable at all: a webview that has
stopped reading — mid-repaint, mid-GC, or wedged — must not be able to stall the
user's shell.
Each connection has a fixed 64-frame outbound queue. A full queue discards its
oldest frame and the producer never waits. Every discarded byte is counted, and
the next frame written is preceded by a
resyncframe carrying the total. Dropsafter the final data frame get a trailing marker before the socket closes, so the
accounting always balances:
resyncmeans the character stream has a hole in it. Escape sequences do notsurvive truncation, so a renderer must clear and redraw from a fresh attach
rather than paint across one. Oldest-first is deliberate: a terminal that kept
the start of a build log and dropped the prompt would be showing the user the
wrong thing.
Also fixes a defect in #2
pty.Attachmenthad no way to detach. Every reconnect — a webview reload, aproject-tab switch — left a consumer registered on the session holding up to
consumerQueue× 32KB of queue for a reader that was never coming back, for therest of the session's life.
Attachment.Detachcloses that leak. It is a structfield rather than a new method, so
internal/pty's exported-surface pin stays at7.
The subtlety worth reviewing: both a detach and a real exit close the
attachment's channels, and only an exit publishes a status. A detach reported as
an exit would tell the UI that a live shell had died.
session.detachandsession.finishare serialized on the same lock and each closes only what theother did not, which is what makes "closed with no value" mean detach
unambiguously.
Acceptance criteria
internal/app/stream_test.go—TestATerminalSessionEchoesOverTheStreamSocketstty sizereflects it)TestResizeControlFrameChangesWhatTheChildSeesTestCloseControlFrameEndsTheChildAndReportsItinternal/stream/auth_test.go—TestConnectionsAreRefusedBeforeTheUpgradeinternal/stream/backpressure_test.go—TestFiftyMegabytesStreamWithoutUnboundedMemoryGrowthThe end-to-end tests live in
internal/appon purpose.internal/streamistested against a fake
Terminals— correctly, since it must not know what a PTYis — which leaves the adapter between the two services untested by construction.
That adapter is exactly the kind of code that looks obviously right and gets a
channel's close semantics wrong, so the real composition is what gets exercised
against a real shell.
Gates
make verifygreen.make test-race -shuffle=on; 5 repeat runs, no flakesmake coverage-reportmake patch-coveragemake lint/make frontend-lintmake security/make semgrepmake licensesgorilla/websocketis BSD-3-Clausemake bindings-checkmake build-checkgremlins(not in verify)internal/stream24 killed / 0 lived / 100% efficacy;internal/app5 killed / 0 lived / 100%Zero suppressions. Two findings were fixed by changing code, not config:
go.gorilla.security.audit.websocket-missing-origin-checkcould not seeCheckOriginset on a struct field.upgrade()now builds theUpgraderatthe point of use, which reads better anyway: there is no path to
Upgradethatdoes not go past the origin policy, and that is now visible in one function.
constant is named
authSubprotocolPrefix, with a comment saying why.Ratchets raised
Each number carries its justification in the diff, next to the number.
structuralPins["internal/stream"]structuralPins["internal/app"]maxAppFields*stream.Serverhandle — port, token, connections and subscribers all live behind itmaxAppMethodsStreamEndpoint. Every terminal operation is a WebSocket frame and adds nothing here — a future PR adding a per-operation binding is not raising a ceiling, it is bypassing the transportlocCeilingNotealso updated:internal/pty,internal/streamandinternal/appare now measured rather than policy-seeded.Adversarial review — what I tried to break
One real bug, found and fixed.
closeinitially returned "stop reading" fromthe read loop, whose
defer c.close()raced the forwarder'sexitwrite. Theclient saw
1006 abnormal closureinstead of its exit code. The read loop nowstays up on close and the forwarder closes the socket after writing
exit, so aclose never costs the client the status it asked for.
Two limits documented rather than fixed. Both want a design decision, not a
patch, and both are in PROTOCOL.md:
with no socket attached is therefore not announced, and two sockets attached
to one session both publish — a consumer must treat
exitas idempotent. Thefix, when something other than the terminal tab needs to know, is a dedicated
attachment that watches the session.
Shutdowncan miss the close sweep.Moot in practice: the process is exiting.
Deliberately not covered (15 of 416 changed lines): write-failure paths on a
socket that is already gone, the
*net.TCPAddrtype assertion, and thequeue-full-after-making-room branch, which is unreachable for
/pty(singleproducer) and only reachable for
/events. Everything else is exercised.Out of scope
Frontend consumption is #4, so
StreamEndpointis the one bound method with nocaller yet — which is what this issue's scope line asks for. Event types beyond
PTY exit are also #4/#5.
projectIDis documented in PROTOCOL.md as reserved in the envelope but isnot a Go field yet: nothing has projects until #5, and a field that is never
set is the vaporware the leash exists to refuse. Decoders must tolerate its
absence, which they will have to do anyway.
Reviewing this
internal/stream/PROTOCOL.md— the contract.internal/stream/conn.go— the backpressure queue. The interesting questionis whether any path can block a producer.
internal/app/terminals.go— the adapter, andexitCodesin particular: itis where detach-versus-exit is preserved across the seam.
internal/pty/session.go—detachagainstfinish, and which one closes.