[WIP] Websocket authentication for preview #1093
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| //! Document preview tool for Typst | ||
|
|
||
| mod auth; | ||
|
|
||
| use std::num::NonZeroUsize; | ||
| use std::{collections::HashMap, net::SocketAddr, path::Path, sync::Arc}; | ||
|
|
||
| use futures::{SinkExt, StreamExt, TryStreamExt}; | ||
| use hyper::service::service_fn; | ||
| use hyper_tungstenite::{tungstenite::Message, HyperWebsocketStream}; | ||
| use hyper_util::rt::TokioIo; | ||
| use hyper_util::server::graceful::GracefulShutdown; | ||
| use lsp_types::notification::Notification; | ||
|
|
@@ -22,9 +24,9 @@ use typst::syntax::{LinkedNode, Source, Span, SyntaxKind, VirtualPath}; | |
| use typst::World; | ||
| pub use typst_preview::CompileStatus; | ||
| use typst_preview::{ | ||
| CompileHost, ControlPlaneMessage, ControlPlaneResponse, ControlPlaneRx, ControlPlaneTx, | ||
| DocToSrcJumpInfo, EditorServer, Location, MemoryFiles, MemoryFilesShort, PreviewArgs, | ||
| PreviewBuilder, PreviewMode, Previewer, SourceFileServer, WsMessage, | ||
| }; | ||
| use typst_shim::syntax::LinkedNodeExt; | ||
|
|
||
|
|
@@ -228,6 +230,12 @@ pub struct PreviewCliArgs { | |
| /// Don't open the preview in the browser after compilation. | ||
| #[clap(long = "no-open")] | ||
| pub dont_open_in_browser: bool, | ||
|
|
||
| /// Use this to disable websocket authentication for the control plane server. Careful: Among other things, this allows any website you visit to use the control plane server. | ||
| /// | ||
| /// This option is only meant to ease the transition to authentication for downstream packages. It will be removed in a future version of tinymist. | ||
| #[clap(long, default_value = "false")] | ||
| pub disable_control_plane_auth: bool, | ||
| } | ||
|
|
||
| /// The global state of the preview tool. | ||
|
|
@@ -263,6 +271,7 @@ pub struct StartPreviewResponse { | |
| static_server_port: Option<u16>, | ||
| static_server_addr: Option<String>, | ||
| data_plane_port: Option<u16>, | ||
| secret: String, | ||
| is_primary: bool, | ||
| } | ||
|
|
||
|
|
@@ -340,17 +349,23 @@ impl PreviewState { | |
| // The fence must be put after the previewer is initialized. | ||
| compile_handler.flush_compile(); | ||
|
|
||
| let secret = auth::generate_token(); | ||
|
|
||
| let srv = make_http_server( | ||
| true, | ||
| args.data_plane_host, | ||
| Some(secret.clone()), | ||
| websocket_tx, | ||
| ) | ||
| .await; | ||
| let addr = srv.addr; | ||
| log::info!("PreviewTask({task_id}): preview server listening on: {addr}"); | ||
|
|
||
| let resp = StartPreviewResponse { | ||
| static_server_port: Some(addr.port()), | ||
| static_server_addr: Some(addr.to_string()), | ||
| data_plane_port: Some(addr.port()), | ||
| secret, | ||
| is_primary, | ||
| }; | ||
|
|
||
|
|
@@ -399,21 +414,21 @@ pub struct HttpServer { | |
|
|
||
| /// Create a http server for the previewer. | ||
| pub async fn make_http_server( | ||
| serve_frontend_html: bool, | ||
| static_file_addr: String, | ||
| secret: Option<String>, | ||
| websocket_tx: mpsc::UnboundedSender<HyperWebsocketStream>, | ||
| ) -> HttpServer { | ||
| use http_body_util::Full; | ||
| use hyper::body::{Bytes, Incoming}; | ||
| type Server = hyper_util::server::conn::auto::Builder<hyper_util::rt::TokioExecutor>; | ||
|
|
||
| let make_service = move || { | ||
| let websocket_tx = websocket_tx.clone(); | ||
| let secret = secret.clone(); | ||
| service_fn(move |mut req: hyper::Request<Incoming>| { | ||
| let websocket_tx = websocket_tx.clone(); | ||
| let secret = secret.clone(); | ||
| async move { | ||
| // Check if the request is a websocket upgrade request. | ||
| if hyper_tungstenite::is_upgrade_request(&req) { | ||
|
|
@@ -424,17 +439,52 @@ pub async fn make_http_server( | |
| }) | ||
| .unwrap(); | ||
|
|
||
| tokio::spawn(async move { | ||
| let websocket = websocket.await.unwrap(); | ||
|
|
||
| // Authenticate the client before we talk to it. | ||
| // Important even if we run on localhost because | ||
| // 1) browsers allow any website to connect to http servers/websockets on localhost | ||
| // 2) on multi-user systems another (potentially untrusted) user can connect to localhost. | ||
|
Comment on lines
+447
to
+448
There was a problem hiding this comment. Is it possible to mitigate (1) by setting relevant cors header or checking referer/origin in request header? Currently I think current auth workflow is somehow way to complex and "homemade". I wonder if there is any better solution for this. I also seek for other people's solution. But it seems that they don't do authN for websocket. For example, vite relies on websocket to hot reload dev server. And it's a plain websocket connection and there is not bi direction auth. From what I understand, if we didn't set There was a problem hiding this comment. I think CORS etc. do not work for WebSockets, so I think But here's an idea to make the authentication much less complex (so it's less of a problem that it's "homegrown"):
That's super simple and does "bi-direction auth". It has worse security properties than a challenge response authentication with nonces, but for our case that's probably okay. Edit: Actually, I don't like that. It doesn't work well in the case of multi-user systems. Consider this scenario: We start a websocket server on Such problems also don't arise if we decide that we just don't support multi-user systems. In that case we can just give the client a token that we check on the sever. No bi-directional authentication needed. Makes things much simpler. I would prefer not to do this, but it's probably mostly okay in practice since multi-user systems with adversarial users are likely rare. What do you think? There was a problem hiding this comment.
but websocket connection is upgraded from a http request? if the http request is blocked, i'd assume it would be impossible to establish ws connection There was a problem hiding this comment. I'm not an expert but google says that CORS only applies to http responses, not requests. The Websocket connection only involves an upgrade http request, not a response. There was a problem hiding this comment. I just did a quick test and cors indeed doesn't apply to websocket. Would it make sense if we check |
||
| // | ||
| // Note: We use authentication only for the websocket. The static HTML file server (see below) | ||
| // only serves a not secret static template, so we don't bother with authentication there. | ||
| match secret { | ||
| Some(secret) => { | ||
| if let Ok(websocket) = | ||
| auth::try_auth_websocket_client(websocket, &secret).await | ||
| { | ||
| let _ = websocket_tx.send(websocket); | ||
| } else { | ||
| log::error!("Websocket client authentication failed"); | ||
| } | ||
| } | ||
| None => { | ||
| // We optionally allow to skip authentication upon explicit request to ease the transition to | ||
| // authentication for downstream packages. | ||
| // FIXME: Remove this is in a future version. | ||
| let _ = websocket_tx.send(websocket); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Return the response so the spawned future can continue. | ||
| Ok(response) | ||
| } else if req.uri().path() == "/" { | ||
| // log::debug!("Serve frontend: {mode:?}"); | ||
| let res = hyper::Response::builder() | ||
| .header(hyper::header::CONTENT_TYPE, "text/html") | ||
| // It's important that we serve a static template that only contains information that is public anyway. | ||
| // Otherwise, we need authentication here (see comment for websocket case above). | ||
| // In particular, the websocket port, the secret etc. must not be in the HTML we serve. These information | ||
| // are in the # part of the URL. | ||
| .body(Full::<Bytes>::from(if serve_frontend_html { | ||
| TYPST_PREVIEW_HTML | ||
| } else { | ||
| "" | ||
| })) | ||
| .unwrap(); | ||
| Ok::<_, anyhow::Error>(res) | ||
| } else { | ||
| // jump to / | ||
| let res = hyper::Response::builder() | ||
|
|
@@ -570,17 +620,33 @@ pub async fn preview_main(args: PreviewCliArgs) -> anyhow::Result<()> { | |
| (service, handle) | ||
| }; | ||
|
|
||
| let secret = auth::generate_token(); | ||
| log::info!("Secret for websocket authentication: {secret}"); | ||
|
|
||
| let (lsp_tx, mut lsp_rx) = ControlPlaneTx::new(true); | ||
|
|
||
| let secret_for_control_plane = if args.disable_control_plane_auth { | ||
| log::warn!( | ||
| "Disabling authentication for the control plane server. This is not recommended." | ||
| ); | ||
| None | ||
| } else { | ||
| Some(secret.clone()) | ||
| }; | ||
| let control_plane_server_handle = tokio::spawn(async move { | ||
| let (control_sock_tx, mut control_sock_rx) = mpsc::unbounded_channel(); | ||
|
|
||
| let srv = make_http_server( | ||
| false, | ||
| args.control_plane_host, | ||
| secret_for_control_plane, | ||
| control_sock_tx, | ||
| ) | ||
| .await; | ||
| log::info!("Control panel server listening on: {}", srv.addr); | ||
|
|
||
| let control_websocket = control_sock_rx.recv().await.unwrap(); | ||
| let ws = control_websocket; | ||
|
|
||
| tokio::pin!(ws); | ||
|
|
||
|
|
@@ -641,24 +707,42 @@ pub async fn preview_main(args: PreviewCliArgs) -> anyhow::Result<()> { | |
|
|
||
| bind_streams(&mut previewer, websocket_rx); | ||
|
|
||
| let static_server = if let Some(static_file_host) = static_file_host { | ||
| log::warn!("--static-file-host is deprecated, which will be removed in the future. Use --data-plane-host instead."); | ||
| Some( | ||
| make_http_server( | ||
| true, | ||
| static_file_host, | ||
| Some(secret.clone()), | ||
| websocket_tx.clone(), | ||
| ) | ||
| .await, | ||
| ) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let srv = make_http_server( | ||
| true, | ||
| args.data_plane_host, | ||
| Some(secret.clone()), | ||
| websocket_tx, | ||
| ) | ||
| .await; | ||
| log::info!("Data plane server listening on: {}", srv.addr); | ||
|
|
||
| let static_server_addr = static_server.as_ref().map(|s| s.addr).unwrap_or(srv.addr); | ||
| let preview_url = format!( | ||
| "http://{static_server_addr}/#secret={secret}&previewMode={}", | ||
| match args.preview_mode { | ||
| PreviewMode::Document => "Doc", | ||
| PreviewMode::Slide => "Slide", | ||
| } | ||
| ); | ||
| log::info!("Static file server listening on: {preview_url}"); | ||
|
|
||
| if !args.dont_open_in_browser { | ||
| if let Err(e) = open::that_detached(preview_url) { | ||
| log::error!("failed to open browser: {e}"); | ||
| }; | ||
| } | ||
|
|
@@ -741,29 +825,26 @@ fn find_in_frame(frame: &Frame, span: Span, min_dis: &mut u64, p: &mut Point) -> | |
| None | ||
| } | ||
|
|
||
| fn bind_streams( | ||
| previewer: &mut Previewer, | ||
| websocket_rx: mpsc::UnboundedReceiver<HyperWebsocketStream>, | ||
| ) { | ||
| previewer.start_data_plane(websocket_rx, |conn: HyperWebsocketStream| { | ||
| conn.sink_map_err(|e| error_once!("cannot serve_with websocket", err: e.to_string())) | ||
| .map_err(|e| error_once!("cannot serve_with websocket", err: e.to_string())) | ||
| .with(|msg| { | ||
| Box::pin(async move { | ||
| let msg = match msg { | ||
| WsMessage::Text(msg) => Message::Text(msg), | ||
| WsMessage::Binary(msg) => Message::Binary(msg), | ||
| }; | ||
| Ok(msg) | ||
| }) | ||
| }) | ||
| .map_ok(|msg| match msg { | ||
| Message::Text(msg) => WsMessage::Text(msg), | ||
| Message::Binary(msg) => WsMessage::Binary(msg), | ||
| _ => WsMessage::Text("unsupported message".to_owned()), | ||
| }) | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may revert the change. It doesn't harm security to allow customizing frontend HTML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason to do it like this is that it makes it easier to reason about the security properties of the code. If the HTML can be customized and I want to convince myself that there is no security issue, I will have to check every single call site. By not allowing to customize the HTML, I just have to check this one function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what is bad about customization. We can take some responsibility to secure them instead of not providing the functionality. This is our work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'm not against customization. My point is that, since we currently don't need customization, it's easier to keep everything contained in a single function because it's easier to reason about. If it becomes necessary to add customization, we can of course change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, so we'll finally change it back. I'm not sure why you feel unconfident about the existing code. Less changes means less controversy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing code is fine. My only reason for this change is to make it easier to review for security (now and with future changes). I think that's worth it, but I'll change it back if you disagree.