docs: the proxy is the open door, not the closed one (Copilot review)
Both Copilot findings on PR #21 applied. The first is the serious one and it is a defect in documentation this PR itself introduced. `improvise-ws-server` has NO authentication -- a deliberate MVP limit its own module doc states -- so anyone who can open a socket to it can rewrite every document it serves. Loopback binding is the only thing standing between that and the network, and terminating TLS in front of it REMOVES EXACTLY THAT PROTECTION. The previous text not only omitted the warning, it actively reassured the reader: the `--bind` note explained that keeping loopback means the port "is not reachable from anywhere else", which is true of the port and false of the deployment, because the proxy is public. TLS buys confidentiality, not access control, and the two read alike in a recipe. So the warning leads the section, the Caddyfile carries `basic_auth` with a comment saying it is not optional, the `--bind` note now says what it does and does not buy, and the tunnel line says cloudflared/ngrok print public URLs and authenticate nothing by default -- an unguessable URL is not a control. Second finding: the watchdog comment was ambiguous in a file whose only verification is reading, which makes an ambiguous comment a real defect. It said a drop before the first frame "leaves the watchdog armed", meaning the state on ARRIVAL; it reads as a claim about the state after the call, which is the opposite of what `setShellFailure` does. Reworded to say the call disarms, why the opened path needs that (the watchdog is armed only after `transport.ready` resolves, i.e. on `open`), and that the never-opened path makes it a no-op. No open ticket covers server auth -- improvise-j31x is the multi-document registry and is closed -- so nothing is cited here and one is proposed in the hand-back rather than an ID invented. 1184 passing, unchanged. `node --check` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -237,12 +237,33 @@ document in *a* browser, you do not need any of this — that is the standalone
|
||||
worker deployment, it is what the Pages site is for, and it needs no server
|
||||
process at all.
|
||||
|
||||
**Read this before following any of it.** `improvise-ws-server` has **no
|
||||
authentication of any kind**, and that is a deliberate MVP limit rather than an
|
||||
oversight — its own module doc says so. Anyone who can open a socket to it can
|
||||
**rewrite every document it serves**. Loopback binding is the only thing
|
||||
currently standing between that and the network, and *terminating TLS in front
|
||||
of it removes exactly that protection*: the proxy is public, so the endpoint is
|
||||
public, and `--bind 127.0.0.1` protects the port while doing nothing about the
|
||||
proxy. TLS gets you confidentiality, not access control.
|
||||
|
||||
So put the access control in the proxy, and treat it as required rather than
|
||||
optional — HTTP basic auth, mTLS, an IP allow-list, or a tunnel with its own
|
||||
authentication (`cloudflared` can enforce Cloudflare Access; a bare
|
||||
`--url` tunnel enforces nothing and is a public URL). Caddy's `basic_auth`
|
||||
directive is the smallest version. Share only documents you would be willing
|
||||
to let an anonymous visitor edit, and take the tunnel down when you are done.
|
||||
|
||||
A worked example with [Caddy](https://caddyserver.com/), which obtains and
|
||||
renews the certificate itself. Point a DNS name at the host, then:
|
||||
|
||||
```caddyfile
|
||||
# Caddyfile
|
||||
improvise.example.com {
|
||||
# NOT optional — see above. Without this the document is world-writable.
|
||||
# Generate the hash with: caddy hash-password
|
||||
basic_auth {
|
||||
editor $2a$14$...replace-with-your-own-hash...
|
||||
}
|
||||
reverse_proxy 127.0.0.1:8787
|
||||
}
|
||||
```
|
||||
@@ -265,12 +286,16 @@ answered as an ordinary request and the socket never opens.
|
||||
For a one-off share with no DNS name and no certificate of your own,
|
||||
`cloudflared tunnel --url http://127.0.0.1:8787` or `ngrok http 8787` each
|
||||
print an https hostname that works the same way — pass the hostname to `?ws=`
|
||||
without a scheme.
|
||||
without a scheme. Both print a **public** URL and neither authenticates
|
||||
anything by default, so add the tunnel's own auth (`ngrok http --basic-auth`,
|
||||
or a named Cloudflare tunnel behind Access) rather than relying on the URL
|
||||
being unguessable.
|
||||
|
||||
Note the server binds `127.0.0.1` by default and `--bind` is the only way to
|
||||
widen that. Behind a proxy on the same host you should *not* widen it: the
|
||||
proxy reaches loopback, and leaving the default means the unencrypted port is
|
||||
not reachable from anywhere else.
|
||||
proxy reaches loopback, and leaving the default keeps the **unencrypted** port
|
||||
off the network. That is worth doing and is not access control — the proxy is
|
||||
still the open door, which is why the auth above belongs there.
|
||||
|
||||
### Why not native `wss://`
|
||||
|
||||
|
||||
+7
-3
@@ -223,9 +223,13 @@ function socketTransport() {
|
||||
// disarmed because it is for silence only, whereas suppressing `close`
|
||||
// outright would hide a genuine mid-session disconnect, which is the one
|
||||
// thing this listener is really for. Hence the split rather than a guard.
|
||||
// Both arms go through `setShellFailure`: a drop before the first frame
|
||||
// leaves the watchdog armed, and "disconnected" is more precise than the
|
||||
// guess it would fire 8s later.
|
||||
// Both arms go through `setShellFailure`, which DISARMS the watchdog. That
|
||||
// matters on the opened path: the watchdog is armed after `transport.ready`
|
||||
// resolves — i.e. on `open` — so a socket that opened and then dropped
|
||||
// before its first frame reaches here with the timer still pending, and
|
||||
// "disconnected" is more precise than the guess it would otherwise fire 8s
|
||||
// later. On the never-opened path there is no timer yet and the clear is a
|
||||
// no-op. Neither arm leaves one armed.
|
||||
socket.addEventListener("close", () =>
|
||||
setShellFailure(opened ? "disconnected" : unreachable),
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user