Skip to content

Commit 0be35c2

Browse files
claudesinelaw
authored andcommitted
fix(webui): gate the HTTP POST routes with the same-origin guard instead of removing them
CI's Playwright suite caught that removing the mutating POST routes broke the web-UI E2E tests: the harness drives the editor through `POST /action` (15+ call sites), `/widget`, and `/reset` — the latter has no WebSocket equivalent. My earlier "nothing uses them" investigation was wrong (it searched for `fetch` / uppercase `POST` and missed Playwright's `page.request.post`). Restore the routes and instead close the CSRF/finding-#3 hole the intended way: apply the SAME same-origin/Host check as the `/ws` upgrade to every state-mutating POST (in `serve_request`). A cross-origin browser page sends its own `Origin` and is rejected; non-browser callers (curl, the Playwright request API, the parity harness) send no `Origin` and pass, so the routes stay scriptable. This keeps the Content-Length crash fix (#1), the write-timeout (#2), the rebinding guard (#4), and the rest intact. Verified: the full Playwright suite passes locally (149/0), including the trust-dialog and live-grep-toolbar sections that failed in CI; webui unit tests and scene_parity pass. Docs updated to describe the gating (not removal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsqkqerfYQ4A48asmUxDQG
1 parent a2138be commit 0be35c2

3 files changed

Lines changed: 156 additions & 83 deletions

File tree

crates/fresh-editor/src/webui/mod.rs

Lines changed: 143 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@
2828
//! `"regions.panes.len"` when the pane count changes (panes carry the bulk of
2929
//! the bytes; typing resends only the changed pane). Client→server input is
3030
//! JSON text frames, tagged `{"type":"key"|"mouse"|"action"|"widget"|
31-
//! "settings"|"kbedit"|"paste"|"resize"}` — each carrying the fields its
32-
//! `apply_*` handler reads. Input arrives ONLY over this socket.
31+
//! "settings"|"kbedit"|"paste"|"resize"}` — each carrying the same fields as
32+
//! the HTTP POST bodies below.
3333
//!
3434
//! Session model: exactly ONE WebSocket client at a time. A second upgrade
3535
//! attempt while one is connected is answered with a plain HTTP
@@ -46,21 +46,21 @@
4646
//! wildcard bind like `--web 0.0.0.0:8137` work when reached as
4747
//! `127.0.0.1:8137`; non-browser tools send no Origin and are accepted.
4848
//!
49-
//! **HTTP routes** are now **read-only**. The mutating `POST` routes
50-
//! (`/key`, `/paste`, `/mouse`, `/action`, `/widget`, `/settings`, `/kbedit`,
51-
//! `/resize`, and the `/step` `/reset` parity-harness routes) predated the WS
52-
//! transport and were removed: the shipping frontend and the Playwright suite
53-
//! drive input over the WebSocket, and the Rust `scene_parity` test drives it
54-
//! via the in-process `apply_step` / `scene_value` entry points — nothing went
55-
//! through those HTTP routes. They were also the whole cross-origin hole: the
56-
//! same-origin check below guards the `/ws` upgrade but never ran on HTTP, so
57-
//! any web page could POST `/action` (or `/reset`) into the editor. What
58-
//! remains only reads state:
59-
//! - `GET /` → serves the page assembled from `web-ui/` (see `INDEX_HTML`)
49+
//! **HTTP routes** all keep working (full-scene responses); curl, the Playwright
50+
//! harness and the parity routes depend on them. A mutation made over HTTP
51+
//! reaches a connected WebSocket client as a pushed diff on the next tick.
52+
//! State-mutating `POST`s are gated by the SAME same-origin/Host check as the
53+
//! `/ws` upgrade (in `serve_request`), so a cross-origin browser page can't
54+
//! drive the editor over HTTP — but non-browser callers (curl, the parity
55+
//! harness, the Playwright request API) send no `Origin` and pass:
56+
//! - `GET /` → serves the page assembled from `web-ui/` (see `INDEX_HTML`)
6057
//! - `GET /favicon.ico` → 204
61-
//! - `GET /state` → `{ w, h, regions, theme, clipboard }` from the real
62-
//! render (the frontend's manual `refresh()` resync, `run.sh`'s readiness
63-
//! poll, curl inspection)
58+
//! - `GET /state` → `{ w, h, regions, theme, clipboard }` from the real render
59+
//! - `POST /key` → runs the real `Editor::handle_key`, returns `/state`
60+
//! - `POST /paste` → `{text}` → the editor's bracketed-paste path, returns `/state`
61+
//! - `POST /resize` → `{cols, rows}` → `Editor::resize`, returns `/state`
62+
//! - `POST /mouse` `/action` `/widget` `/settings` `/kbedit` → same pattern
63+
//! - `POST /step` `/reset` → parity-harness routes (no clipboard attach)
6464
6565
use std::collections::HashMap;
6666
use std::io::{ErrorKind, Read, Write};
@@ -85,7 +85,8 @@ use crate::config;
8585
use crate::config_io::DirectoryContext;
8686
use crate::model::filesystem::{FileSystem, StdFileSystem};
8787

88-
/// Default terminal size the bridge boots to (cols, rows).
88+
/// Default terminal size the bridge boots / resets to (cols, rows). One source
89+
/// so `run()` and the `/reset` route can't drift apart.
8990
const DEFAULT_SIZE: (u16, u16) = (140, 44);
9091

9192
/// The web-UI frontend served at `GET /`, embedded at compile time from the
@@ -156,7 +157,8 @@ impl ClipboardSync {
156157

157158
/// Construct a fresh editor exactly as the web bridge does: real plugin runtime
158159
/// enabled, init.ts loaded, chrome drawn as a semantic model (not cells). Shared
159-
/// by `run()` and the parity test runner so both drive an identical editor.
160+
/// by `run()`, the `/reset` route (scenario isolation) and the parity test
161+
/// runner so all three drive an identical editor.
160162
pub fn build_editor(cols: u16, rows: u16, files: &[PathBuf]) -> Result<Editor> {
161163
let dir_context = DirectoryContext::from_system()?;
162164
let working_dir = std::env::current_dir().unwrap_or_default();
@@ -199,8 +201,8 @@ pub fn build_editor(cols: u16, rows: u16, files: &[PathBuf]) -> Result<Editor> {
199201
}
200202

201203
/// Apply one parity-scenario step to the editor: a key, a mouse event at a cell,
202-
/// an action by name, a literal string to type, or a tick. Used by the Rust
203-
/// parity runner (`scene_parity`) to drive scenario input in-process.
204+
/// an action by name, a literal string to type, or a tick. Shared by the web
205+
/// `/step` route and the Rust parity runner so both drive identical input.
204206
pub fn apply_step(editor: &mut Editor, step: &Value) {
205207
if let Some(s) = step.get("type").and_then(|t| t.as_str()) {
206208
for ch in s.chars() {
@@ -345,9 +347,11 @@ pub fn run(addr: &str, files: &[PathBuf]) -> Result<()> {
345347

346348
// 3) Pump pending connections; serve the complete ones. A `/ws`
347349
// upgrade becomes THE client (or gets 409/403); anything else runs
348-
// through the read-only HTTP routes, blocking only for its short
349-
// localhost response write. HTTP no longer mutates the editor (input
350-
// is WS-only), so a served request never needs to force a push.
350+
// through the HTTP routes, blocking only for its short localhost
351+
// response write. An HTTP route that mutated the editor (input
352+
// routes, /step, /reset) counts as input so the connected WS client
353+
// gets the resulting diff pushed this pass.
354+
let mut http_mutated = false;
351355
let mut i = 0;
352356
while i < pending.len() {
353357
match pump_pending(&mut pending[i]) {
@@ -361,14 +365,15 @@ pub fn run(addr: &str, files: &[PathBuf]) -> Result<()> {
361365
conn.stream,
362366
&req,
363367
&mut editor,
364-
cols,
365-
rows,
368+
&mut cols,
369+
&mut rows,
370+
files,
366371
&mut clip,
367372
ws.is_some(),
368373
bind_host,
369374
) {
370375
Ok(Served::WsClient(session)) => ws = Some(session),
371-
Ok(Served::Http) => {}
376+
Ok(Served::Http { mutated }) => http_mutated |= mutated,
372377
Err(e) => eprintln!("conn error: {e}"),
373378
}
374379
}
@@ -394,7 +399,7 @@ pub fn run(addr: &str, files: &[PathBuf]) -> Result<()> {
394399
// the diff cache belongs to the connected session, and a reconnect
395400
// starts over with a fresh hello anyway.
396401
let now = Instant::now();
397-
let had_input = applied_input;
402+
let had_input = applied_input || http_mutated;
398403
if had_input || now >= next_tick {
399404
let needs_render = tick_only(&mut editor);
400405
let active_hint = poll_active(&editor);
@@ -447,14 +452,12 @@ pub fn run(addr: &str, files: &[PathBuf]) -> Result<()> {
447452
// HTTP request assembly (nonblocking) + routing
448453
// ---------------------------------------------------------------------------
449454

450-
/// A parsed HTTP request. Header names are lowercased. The body is *consumed
451-
/// for framing* (a declared `Content-Length` must fully arrive before the
452-
/// request is considered complete) but not retained: the routes are read-only
453-
/// GETs that never inspect a body.
455+
/// A parsed HTTP request (head + full body). Header names are lowercased.
454456
struct HttpRequest {
455457
method: String,
456458
path: String,
457459
headers: Vec<(String, String)>,
460+
body: Vec<u8>,
458461
}
459462

460463
impl HttpRequest {
@@ -537,29 +540,37 @@ fn try_parse_request(buf: &[u8]) -> Option<HttpRequest> {
537540
if buf.len() < end {
538541
return None; // body still arriving (or never will — dropped on deadline)
539542
}
543+
let body = buf[head_end..end].to_vec();
540544
Some(HttpRequest {
541545
method,
542546
path,
543547
headers,
548+
body,
544549
})
545550
}
546551

547552
/// Outcome of serving one complete request.
548553
enum Served {
549554
/// The request was a successful `/ws` upgrade — this is the new client.
550555
WsClient(WsSession),
551-
/// A plain, read-only HTTP exchange — the page, favicon, or `/state`.
552-
Http,
556+
/// A plain HTTP exchange; `mutated` = the route may have changed editor
557+
/// state (input routes, /step, /reset), so a connected WS client should
558+
/// get a diff pushed without waiting for the tick deadline.
559+
Http { mutated: bool },
553560
}
554561

555562
/// Serve one complete request: WS upgrades become THE client; everything else
556-
/// goes through the read-only HTTP routes with `Connection: close`.
563+
/// goes through the HTTP routes with `Connection: close`. State-mutating POSTs
564+
/// are gated by the same-origin/Host guard so a foreign page can't drive the
565+
/// editor over HTTP (see below).
566+
#[allow(clippy::too_many_arguments)]
557567
fn serve_request(
558568
mut stream: TcpStream,
559569
req: &HttpRequest,
560570
editor: &mut Editor,
561-
cols: u16,
562-
rows: u16,
571+
cols: &mut u16,
572+
rows: &mut u16,
573+
files: &[PathBuf],
563574
clip: &mut ClipboardSync,
564575
ws_busy: bool,
565576
bind_host: &str,
@@ -568,7 +579,7 @@ fn serve_request(
568579
// returns the whole embedded page (~200 KB), so a client that stops reading
569580
// (or advertises a tiny/zero window) would otherwise wedge `write_all` on
570581
// this one editor thread forever. A write deadline drops such a peer instead
571-
// (the WS handshake's own small write below is well under it, and `/ws` then
582+
// (the WS handshake's own small write is well under it, and `/ws` then
572583
// switches back to nonblocking). The read side is already bounded by the
573584
// pending-pool `HTTP_READ_DEADLINE`.
574585
stream.set_nonblocking(false)?;
@@ -579,54 +590,116 @@ fn serve_request(
579590
.header("upgrade")
580591
.is_some_and(|u| u.to_ascii_lowercase().contains("websocket"));
581592
if wants_ws {
582-
return match upgrade_ws(stream, req, editor, cols, rows, clip, ws_busy, bind_host)? {
593+
return match upgrade_ws(stream, req, editor, *cols, *rows, clip, ws_busy, bind_host)? {
583594
Some(session) => Ok(Served::WsClient(session)),
584-
None => Ok(Served::Http),
595+
None => Ok(Served::Http { mutated: false }),
585596
};
586597
}
587-
handle_http(&mut stream, req, editor, cols, rows, clip)?;
588-
Ok(Served::Http)
598+
// CSRF / DNS-rebinding guard for state-mutating requests. `upgrade_ws` checks
599+
// the `/ws` upgrade; the POST routes mutate the editor too, so a cross-origin
600+
// browser page (which sends its own `Origin`) must be rejected here as well —
601+
// otherwise the same-origin guard is trivially bypassed by choosing HTTP over
602+
// WS. It fires only when an `Origin` is present and mismatched/untrusted, so
603+
// non-browser callers (curl, the Playwright request API, the parity harness)
604+
// send no `Origin` and pass — the routes stay scriptable. GET stays open: the
605+
// same-origin policy already stops a foreign page from *reading* a response.
606+
if req.method == "POST" {
607+
if let Some(origin) = req.header("origin") {
608+
if !origin_host_matches(origin, req.header("host"), bind_host) {
609+
respond(&mut stream, "403 Forbidden", "text/plain", b"origin not allowed")?;
610+
return Ok(Served::Http { mutated: false });
611+
}
612+
}
613+
}
614+
let mutated = handle_http(&mut stream, req, editor, cols, rows, files, clip)?;
615+
Ok(Served::Http { mutated })
589616
}
590617

591-
/// The HTTP routes — now **read-only**. Input arrives exclusively over the
592-
/// WebSocket (`apply_message`); the mutating `POST` routes that predated the WS
593-
/// transport were removed (see the module docs): nothing depended on them and
594-
/// leaving them unguarded made every editor mutation reachable cross-origin
595-
/// over HTTP, since the same-origin check guards only the `/ws` upgrade.
618+
/// The HTTP routes (full-scene responses). The mutating `POST` routes are
619+
/// reachable only after `serve_request`'s same-origin guard, so a cross-origin
620+
/// page can't drive them; non-browser callers (curl, the Playwright request
621+
/// API, the parity harness `/step` `/reset`) send no `Origin` and pass. Returns
622+
/// whether the route may have mutated editor state.
623+
#[allow(clippy::too_many_arguments)]
596624
fn handle_http(
597625
stream: &mut TcpStream,
598626
req: &HttpRequest,
599627
editor: &mut Editor,
600-
cols: u16,
601-
rows: u16,
628+
cols: &mut u16,
629+
rows: &mut u16,
630+
files: &[PathBuf],
602631
clip: &mut ClipboardSync,
603-
) -> Result<()> {
632+
) -> Result<bool> {
633+
let body_json = || serde_json::from_slice::<Value>(&req.body).unwrap_or_else(|_| json!({}));
604634
match (req.method.as_str(), req.path.as_str()) {
605-
("GET", "/") => respond(
606-
stream,
607-
"200 OK",
608-
"text/html; charset=utf-8",
609-
INDEX_HTML.as_bytes(),
610-
),
611-
("GET", "/favicon.ico") => respond(stream, "204 No Content", "image/x-icon", b""),
612-
// Read-only scene snapshot: the frontend's manual `refresh()` resync and
613-
// `web-ui/test/run.sh`'s readiness poll use it, and it stays curl-able.
635+
("GET", "/") => {
636+
respond(
637+
stream,
638+
"200 OK",
639+
"text/html; charset=utf-8",
640+
INDEX_HTML.as_bytes(),
641+
)?;
642+
Ok(false)
643+
}
644+
("GET", "/favicon.ico") => {
645+
respond(stream, "204 No Content", "image/x-icon", b"")?;
646+
Ok(false)
647+
}
614648
("GET", "/state") => {
615-
let s = tick_scene(editor, cols, rows, clip).to_string();
616-
respond(stream, "200 OK", "application/json", s.as_bytes())
649+
let s = tick_scene(editor, *cols, *rows, clip).to_string();
650+
respond(stream, "200 OK", "application/json", s.as_bytes())?;
651+
Ok(false)
652+
}
653+
// Input routes: the route name IS the message kind, and `apply_message`
654+
// is the same dispatch the WebSocket transport uses — the two transports
655+
// cannot drift. Each returns the full post-tick scene, exactly as
656+
// before; a connected WS client gets the mutation pushed as a diff in
657+
// the same loop pass (the `true` below counts as input).
658+
(
659+
"POST",
660+
p @ ("/key" | "/paste" | "/mouse" | "/action" | "/widget" | "/settings" | "/kbedit"
661+
| "/resize"),
662+
) => {
663+
apply_message(editor, &p[1..], &body_json(), cols, rows);
664+
let s = tick_scene(editor, *cols, *rows, clip).to_string();
665+
respond(stream, "200 OK", "application/json", s.as_bytes())?;
666+
Ok(true)
667+
}
668+
// Parity-harness routes: apply one scenario step, and reset to a fresh
669+
// editor so each scenario runs in isolation (mirrors the Rust runner,
670+
// which builds a fresh editor per scenario).
671+
("POST", "/step") => {
672+
let v = body_json();
673+
apply_step(editor, &v);
674+
let s = scene_json(editor, *cols, *rows).to_string();
675+
respond(stream, "200 OK", "application/json", s.as_bytes())?;
676+
Ok(true)
677+
}
678+
("POST", "/reset") => {
679+
(*cols, *rows) = DEFAULT_SIZE;
680+
match build_editor(*cols, *rows, files) {
681+
Ok(e) => *editor = e,
682+
Err(err) => eprintln!("reset failed: {err}"),
683+
}
684+
let s = scene_json(editor, *cols, *rows).to_string();
685+
respond(stream, "200 OK", "application/json", s.as_bytes())?;
686+
Ok(true)
687+
}
688+
_ => {
689+
respond(stream, "404 Not Found", "text/plain", b"not found")?;
690+
Ok(false)
617691
}
618-
_ => respond(stream, "404 Not Found", "text/plain", b"not found"),
619692
}
620693
}
621694

622695
// ---------------------------------------------------------------------------
623-
// WebSocket input dispatch
696+
// Shared input dispatch (HTTP routes + WebSocket messages)
624697
// ---------------------------------------------------------------------------
625698

626-
/// Apply one input message by kind — the dispatch behind the WS
627-
/// `{"type": kind, ...}` messages (input arrives only over the WebSocket).
628-
/// Returns false for unknown kinds. Does NOT render: the caller builds one
629-
/// scene per WS input batch.
699+
/// Apply one input message by kind — the single dispatch behind both the HTTP
700+
/// POST routes (kind = route name) and the WS `{"type": kind, ...}` messages.
701+
/// Returns false for unknown kinds. Does NOT render: callers decide when a
702+
/// scene is built (per HTTP request, or once per WS input batch).
630703
fn apply_message(
631704
editor: &mut Editor,
632705
kind: &str,
@@ -1838,12 +1911,13 @@ mod tests {
18381911
fn well_formed_request_with_body_still_parses() {
18391912
// A request whose declared body has fully arrived parses (the fix must
18401913
// reject only absurd/oversize lengths, not normal ones).
1841-
let raw = b"POST /state HTTP/1.1\r\nContent-Length: 5\r\n\r\nhello";
1914+
let raw = b"POST /action HTTP/1.1\r\nContent-Length: 5\r\n\r\nhello";
18421915
let req = try_parse_request(raw).expect("complete request parses");
18431916
assert_eq!(req.method, "POST");
1844-
assert_eq!(req.path, "/state");
1917+
assert_eq!(req.path, "/action");
1918+
assert_eq!(req.body, b"hello");
18451919
// A request whose body hasn't fully arrived yet is "incomplete", not an error.
1846-
let partial = b"POST /state HTTP/1.1\r\nContent-Length: 5\r\n\r\nhel";
1920+
let partial = b"POST /action HTTP/1.1\r\nContent-Length: 5\r\n\r\nhel";
18471921
assert!(try_parse_request(partial).is_none());
18481922
}
18491923

0 commit comments

Comments
 (0)