From 444dc11b2605961768856a0dda0bed11bd68288b Mon Sep 17 00:00:00 2001 From: Ken Loeffler Date: Sun, 7 Jun 2026 15:50:04 -0700 Subject: [PATCH 1/4] Fix init.plugin.lua/init.plugin.luau not being supported (#1252) --- CHANGELOG.md | 2 ++ ...end_to_end__tests__build__plugin_init.snap | 27 +++++++++++++++++++ .../plugin_init/default.project.json | 6 +++++ .../plugin_init/folder/lua/init.plugin.lua | 1 + .../plugin_init/folder/luau/init.plugin.luau | 1 + src/snapshot_middleware/dir.rs | 2 ++ src/snapshot_middleware/lua.rs | 1 + src/snapshot_middleware/mod.rs | 13 ++++++++- ...pshot_middleware__csv__test__csv_init.snap | 2 ++ ...leware__csv__test__csv_init_with_meta.snap | 2 ++ ...t_middleware__dir__test__empty_folder.snap | 2 ++ ...ddleware__dir__test__folder_in_folder.snap | 4 +++ ...ware__lua__test__init_module_from_vfs.snap | 2 ++ ..._test__init_module_from_vfs_with_meta.snap | 2 ++ src/syncback/file_names.rs | 2 ++ src/syncback/mod.rs | 1 + tests/tests/build.rs | 1 + 17 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 rojo-test/build-test-snapshots/end_to_end__tests__build__plugin_init.snap create mode 100644 rojo-test/build-tests/plugin_init/default.project.json create mode 100644 rojo-test/build-tests/plugin_init/folder/lua/init.plugin.lua create mode 100644 rojo-test/build-tests/plugin_init/folder/luau/init.plugin.luau diff --git a/CHANGELOG.md b/CHANGELOG.md index e9e92892..8ceff750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Making a new release? Simply add the new header with the version and date undern * Fixed a bug where the notification timeout thread would fail to cancel on unmount ([#1211]) * Added a "Forget" option to the sync reminder notification to avoid being reminded for that place in the future ([#1215]) * Improves relative path calculation for sourcemap generation to avoid issues with Windows UNC paths. ([#1217]) +* Fixed missing support for init.plugin.lua and init.plugin.luau. ([#1252]) * Add support for gitignore-style negation in `globIgnorePaths` and syncback's `ignorePaths` ([#1256]) * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265]) * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) @@ -52,6 +53,7 @@ Making a new release? Simply add the new header with the version and date undern [#1211]: https://github.com/rojo-rbx/rojo/pull/1211 [#1215]: https://github.com/rojo-rbx/rojo/pull/1215 [#1217]: https://github.com/rojo-rbx/rojo/pull/1217 +[#1252]: https://github.com/rojo-rbx/rojo/pull/1252 [#1256]: https://github.com/rojo-rbx/rojo/pull/1256 [#1265]: https://github.com/rojo-rbx/rojo/pull/1265 [#1266]: https://github.com/rojo-rbx/rojo/pull/1266 diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__plugin_init.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__plugin_init.snap new file mode 100644 index 00000000..40def8cd --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__plugin_init.snap @@ -0,0 +1,27 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + plugin_init + + + + lua + 3 + + + + + + luau + 3 + + + + + diff --git a/rojo-test/build-tests/plugin_init/default.project.json b/rojo-test/build-tests/plugin_init/default.project.json new file mode 100644 index 00000000..c683b506 --- /dev/null +++ b/rojo-test/build-tests/plugin_init/default.project.json @@ -0,0 +1,6 @@ +{ + "name": "plugin_init", + "tree": { + "$path": "folder" + } +} diff --git a/rojo-test/build-tests/plugin_init/folder/lua/init.plugin.lua b/rojo-test/build-tests/plugin_init/folder/lua/init.plugin.lua new file mode 100644 index 00000000..9761a96d --- /dev/null +++ b/rojo-test/build-tests/plugin_init/folder/lua/init.plugin.lua @@ -0,0 +1 @@ +return "From folder/lua/init.plugin.lua" diff --git a/rojo-test/build-tests/plugin_init/folder/luau/init.plugin.luau b/rojo-test/build-tests/plugin_init/folder/luau/init.plugin.luau new file mode 100644 index 00000000..f5de5239 --- /dev/null +++ b/rojo-test/build-tests/plugin_init/folder/luau/init.plugin.luau @@ -0,0 +1 @@ +return "From folder/luau/init.plugin.luau" diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index fac1d0ff..b9f62743 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -72,6 +72,8 @@ pub fn snapshot_dir_no_meta( normalized_path.join("init.server.luau"), normalized_path.join("init.client.lua"), normalized_path.join("init.client.luau"), + normalized_path.join("init.plugin.lua"), + normalized_path.join("init.plugin.luau"), normalized_path.join("init.csv"), ]; diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index a0058555..98f43e40 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -182,6 +182,7 @@ pub fn syncback_lua_init<'sync>( ScriptType::Server => "init.server.luau", ScriptType::Client => "init.client.luau", ScriptType::Module => "init.luau", + ScriptType::Plugin => "init.plugin.luau", _ => anyhow::bail!("syncback is not yet implemented for {script_type:?}"), }); diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 9eec5f83..fadce5f6 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -91,7 +91,9 @@ pub fn snapshot_from_vfs( // TODO: Is this even necessary anymore? match file_name { "init.server.luau" | "init.server.lua" | "init.client.luau" | "init.client.lua" - | "init.luau" | "init.lua" | "init.csv" => return Ok(None), + | "init.plugin.luau" | "init.plugin.lua" | "init.luau" | "init.lua" | "init.csv" => { + return Ok(None) + } _ => {} } @@ -124,6 +126,8 @@ fn get_dir_middleware<'path>( (Middleware::ServerScriptDir, "init.server.lua"), (Middleware::ClientScriptDir, "init.client.luau"), (Middleware::ClientScriptDir, "init.client.lua"), + (Middleware::PluginScriptDir, "init.plugin.lua"), + (Middleware::PluginScriptDir, "init.plugin.luau"), (Middleware::CsvDir, "init.csv"), ] }); @@ -205,6 +209,8 @@ pub enum Middleware { #[serde(skip_deserializing)] ClientScriptDir, #[serde(skip_deserializing)] + PluginScriptDir, + #[serde(skip_deserializing)] ModuleScriptDir, #[serde(skip_deserializing)] CsvDir, @@ -255,6 +261,9 @@ impl Middleware { Self::ClientScriptDir => { snapshot_lua_init(context, vfs, path, name, ScriptType::Client) } + Self::PluginScriptDir => { + snapshot_lua_init(context, vfs, path, name, ScriptType::Plugin) + } Self::ModuleScriptDir => { snapshot_lua_init(context, vfs, path, name, ScriptType::Module) } @@ -297,6 +306,7 @@ impl Middleware { Middleware::Dir => syncback_dir(snapshot), Middleware::ServerScriptDir => syncback_lua_init(ScriptType::Server, snapshot), Middleware::ClientScriptDir => syncback_lua_init(ScriptType::Client, snapshot), + Middleware::PluginScriptDir => syncback_lua_init(ScriptType::Plugin, snapshot), Middleware::ModuleScriptDir => syncback_lua_init(ScriptType::Module, snapshot), Middleware::CsvDir => syncback_csv_init(snapshot), @@ -318,6 +328,7 @@ impl Middleware { Middleware::Dir | Middleware::ServerScriptDir | Middleware::ClientScriptDir + | Middleware::PluginScriptDir | Middleware::ModuleScriptDir | Middleware::CsvDir ) diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap index a275a6cf..813a2933 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap @@ -15,6 +15,8 @@ metadata: - /root/init.server.luau - /root/init.client.lua - /root/init.client.luau + - /root/init.plugin.lua + - /root/init.plugin.luau - /root/init.csv - /root/init.meta.json - /root/init.meta.jsonc diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap index 603bd2df..01ce0a0f 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap @@ -15,6 +15,8 @@ metadata: - /root/init.server.luau - /root/init.client.lua - /root/init.client.luau + - /root/init.plugin.lua + - /root/init.plugin.luau - /root/init.csv - /root/init.meta.json - /root/init.meta.jsonc diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__empty_folder.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__empty_folder.snap index 813a6767..865c3c27 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__empty_folder.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__empty_folder.snap @@ -15,6 +15,8 @@ metadata: - /foo/init.server.luau - /foo/init.client.lua - /foo/init.client.luau + - /foo/init.plugin.lua + - /foo/init.plugin.luau - /foo/init.csv - /foo/init.meta.json - /foo/init.meta.jsonc diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__folder_in_folder.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__folder_in_folder.snap index 2f44988d..00a4bdf0 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__folder_in_folder.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__dir__test__folder_in_folder.snap @@ -15,6 +15,8 @@ metadata: - /foo/init.server.luau - /foo/init.client.lua - /foo/init.client.luau + - /foo/init.plugin.lua + - /foo/init.plugin.luau - /foo/init.csv - /foo/init.meta.json - /foo/init.meta.jsonc @@ -40,6 +42,8 @@ children: - /foo/Child/init.server.luau - /foo/Child/init.client.lua - /foo/Child/init.client.luau + - /foo/Child/init.plugin.lua + - /foo/Child/init.plugin.luau - /foo/Child/init.csv - /foo/Child/init.meta.json - /foo/Child/init.meta.jsonc diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs.snap index 80178738..f5de8064 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs.snap @@ -15,6 +15,8 @@ metadata: - /root/init.server.luau - /root/init.client.lua - /root/init.client.luau + - /root/init.plugin.lua + - /root/init.plugin.luau - /root/init.csv - /root/init.meta.json - /root/init.meta.jsonc diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs_with_meta.snap index d5f8aa9f..60b43c20 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__lua__test__init_module_from_vfs_with_meta.snap @@ -15,6 +15,8 @@ metadata: - /root/init.server.luau - /root/init.client.lua - /root/init.client.luau + - /root/init.plugin.lua + - /root/init.plugin.luau - /root/init.csv - /root/init.meta.json - /root/init.meta.jsonc diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 4653aabd..7654fed8 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -35,6 +35,7 @@ pub fn name_for_inst<'old>( | Middleware::CsvDir | Middleware::ServerScriptDir | Middleware::ClientScriptDir + | Middleware::PluginScriptDir | Middleware::ModuleScriptDir => Cow::Owned(new_inst.name.clone()), _ => { let extension = extension_for_middleware(middleware); @@ -78,6 +79,7 @@ pub fn extension_for_middleware(middleware: Middleware) -> &'static str { | Middleware::CsvDir | Middleware::ServerScriptDir | Middleware::ClientScriptDir + | Middleware::PluginScriptDir | Middleware::ModuleScriptDir => { unimplemented!("directory middleware requires special treatment") } diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index e1666abc..7fe3992d 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -359,6 +359,7 @@ pub fn get_best_middleware(snapshot: &SyncbackSnapshot) -> Middleware { middleware = match middleware { Middleware::ServerScript => Middleware::ServerScriptDir, Middleware::ClientScript => Middleware::ClientScriptDir, + Middleware::PluginScript => Middleware::PluginScriptDir, Middleware::ModuleScript => Middleware::ModuleScriptDir, Middleware::Csv => Middleware::CsvDir, Middleware::JsonModel | Middleware::Text => Middleware::Dir, diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 8c533115..c7873050 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -65,6 +65,7 @@ gen_build_tests! { no_name_default_project, no_name_project, no_name_top_level_project, + plugin_init, } fn run_build_test(test_name: &str) { From ac6941f05483b0875fda52c7664cd42033db7fa2 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Sun, 7 Jun 2026 15:51:05 -0700 Subject: [PATCH 2/4] Add origin/host validation and warning for exposed serves (#1270) --- CHANGELOG.md | 2 + src/cli/serve.rs | 37 ++- src/project.rs | 44 +++ src/serve_session.rs | 4 + src/web/api.rs | 43 ++- src/web/interface.rs | 8 + src/web/mod.rs | 31 +- src/web/origin.rs | 589 ++++++++++++++++++++++++++++++++++ tests/rojo_test/serve_util.rs | 37 +++ tests/tests/serve.rs | 68 ++++ 10 files changed, 852 insertions(+), 11 deletions(-) create mode 100644 src/web/origin.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ceff750..cff39688 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ Making a new release? Simply add the new header with the version and date undern * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265]) * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267]) +* `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. The accepted hosts can be extended with the `--allowed-hosts` option or a project's `serveAllowedHosts` field, for example to reach a network-exposed server by hostname. ([#1270]) [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 @@ -58,6 +59,7 @@ Making a new release? Simply add the new header with the version and date undern [#1265]: https://github.com/rojo-rbx/rojo/pull/1265 [#1266]: https://github.com/rojo-rbx/rojo/pull/1266 [#1267]: https://github.com/rojo-rbx/rojo/pull/1267 +[#1270]: https://github.com/rojo-rbx/rojo/pull/1270 ## [7.7.0-rc.1] (November 27th, 2025) diff --git a/src/cli/serve.rs b/src/cli/serve.rs index e33c3fc5..b22beb69 100644 --- a/src/cli/serve.rs +++ b/src/cli/serve.rs @@ -31,6 +31,14 @@ pub struct ServeCommand { /// it has none. #[clap(long)] pub port: Option, + + /// Extra `Host`/`Origin` values the server will accept, beyond localhost and + /// the bind address (for example a hostname like `mypc.lan`). Repeat the + /// option or comma-separate to allow several. When given, this overrides the + /// project's `serveAllowedHosts`. Listing any host also turns on Host/Origin + /// validation for binds where it is otherwise off (such as `0.0.0.0`). + #[clap(long, value_delimiter = ',')] + pub allowed_hosts: Vec, } impl ServeCommand { @@ -51,9 +59,17 @@ impl ServeCommand { .or_else(|| session.project_port()) .unwrap_or(DEFAULT_PORT); + // The CLI flag, when given, replaces the project's list rather than + // merging with it, matching how --address and --port override theirs. + let allowed_hosts = if self.allowed_hosts.is_empty() { + session.serve_allowed_hosts().to_vec() + } else { + self.allowed_hosts + }; + let server = LiveServer::new(session); - server.start((ip, port).into(), || { + server.start((ip, port).into(), allowed_hosts, || { let _ = show_start_message(ip, port, global.color.into()); })?; @@ -87,6 +103,25 @@ fn show_start_message(bind_address: IpAddr, port: u16, color: ColorChoice) -> io writeln!(&mut buffer)?; + if !bind_address.is_loopback() { + let mut warning = ColorSpec::new(); + warning.set_fg(Some(Color::Yellow)).set_bold(true); + + buffer.set_color(&warning)?; + writeln!( + &mut buffer, + "WARNING: This server is bound to {address_string}, which is reachable from the \ + network.\n\ + The serve API is unauthenticated, so anyone who can reach {address_string}:{port} \ + can read\n\ + and modify your project's source. Prefer binding to localhost and tunneling (e.g. \ + SSH,\n\ + Tailscale, or WireGuard) when you need remote access." + )?; + buffer.set_color(&ColorSpec::new())?; + writeln!(&mut buffer)?; + } + buffer.set_color(&ColorSpec::new())?; write!(&mut buffer, "Visit ")?; diff --git a/src/project.rs b/src/project.rs index bfcc7f0e..093529b1 100644 --- a/src/project.rs +++ b/src/project.rs @@ -106,6 +106,15 @@ pub struct Project { #[serde(skip_serializing_if = "Option::is_none")] pub serve_address: Option, + /// Additional `Host`/`Origin` header values that `rojo serve` will accept + /// beyond `localhost` and the bind address, such as a hostname like + /// `mypc.lan` used to reach a network-exposed server by name. Listing any + /// host also turns on `Host`/`Origin` validation for binds where it is + /// otherwise off (such as `0.0.0.0`). The `--allowed-hosts` CLI option + /// overrides this field when provided. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub serve_allowed_hosts: Vec, + /// Determines if Rojo should emit scripts with the appropriate `RunContext` /// for `*.client.lua` and `*.server.lua` files in the project instead of /// using `Script` and `LocalScript` Instances. @@ -595,6 +604,41 @@ mod test { assert!(project.sync_rules[1].include.is_match("init.module.lua")); } + #[test] + fn project_with_serve_allowed_hosts() { + let project_json = r#"{ + "name": "TestProject", + "tree": { "$path": "src" }, + "serveAllowedHosts": ["mypc.lan", "192.168.1.5"] + }"#; + + let project = Project::load_from_slice( + project_json.as_bytes(), + PathBuf::from("/test/default.project.json"), + None, + ) + .expect("Failed to parse project with serveAllowedHosts"); + + assert_eq!(project.serve_allowed_hosts, vec!["mypc.lan", "192.168.1.5"]); + } + + #[test] + fn project_without_serve_allowed_hosts_defaults_to_empty() { + let project_json = r#"{ + "name": "TestProject", + "tree": { "$path": "src" } + }"#; + + let project = Project::load_from_slice( + project_json.as_bytes(), + PathBuf::from("/test/default.project.json"), + None, + ) + .expect("Failed to parse project"); + + assert!(project.serve_allowed_hosts.is_empty()); + } + #[test] fn glob_ignore_paths_negation() { let project_json = r#"{ diff --git a/src/serve_session.rs b/src/serve_session.rs index f50d3926..e46cdfe9 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -207,6 +207,10 @@ impl ServeSession { self.root_project.serve_address } + pub fn serve_allowed_hosts(&self) -> &[String] { + &self.root_project.serve_allowed_hosts + } + pub fn root_dir(&self) -> &Path { self.root_project.folder_location() } diff --git a/src/web/api.rs b/src/web/api.rs index 38163bb9..2875f2aa 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -1,7 +1,7 @@ //! Defines Rojo's HTTP API, all under /api. These endpoints generally return //! JSON. -use std::{collections::HashMap, fs, path::PathBuf, str::FromStr, sync::Arc}; +use std::{collections::HashMap, fs, net::SocketAddr, path::PathBuf, str::FromStr, sync::Arc}; use futures::{sink::SinkExt, stream::StreamExt}; use hyper::{body, Body, Method, Request, Response, StatusCode}; @@ -21,6 +21,7 @@ use crate::{ ServerInfoResponse, SocketPacket, SocketPacketBody, SocketPacketType, SubscribeMessage, WriteRequest, WriteResponse, PROTOCOL_VERSION, SERVER_VERSION, }, + origin::canonical, util::{deserialize_msgpack, msgpack, msgpack_ok, serialize_msgpack}, }, web_api::{ @@ -28,8 +29,12 @@ use crate::{ }, }; -pub async fn call(serve_session: Arc, mut request: Request) -> Response { - let service = ApiService::new(serve_session); +pub async fn call( + serve_session: Arc, + remote_addr: SocketAddr, + mut request: Request, +) -> Response { + let service = ApiService::new(serve_session, remote_addr); match (request.method(), request.uri().path()) { (&Method::GET, "/api/rojo") => service.handle_api_rojo().await, @@ -65,11 +70,15 @@ pub async fn call(serve_session: Arc, mut request: Request) pub struct ApiService { serve_session: Arc, + remote_addr: SocketAddr, } impl ApiService { - pub fn new(serve_session: Arc) -> Self { - ApiService { serve_session } + pub fn new(serve_session: Arc, remote_addr: SocketAddr) -> Self { + ApiService { + serve_session, + remote_addr, + } } /// Get a summary of information about the server @@ -348,6 +357,30 @@ impl ApiService { /// Open a script with the given ID in the user's default text editor. async fn handle_api_open(&self, request: Request) -> Response { + // Opening a file launches a local program, so it must never be reachable + // by a remote client even when the server is bound to an exposed address. + // + // `remote_addr` is the immediate peer, which is the best locality signal + // we have: the legitimate caller is a sandboxed Roblox plugin whose only + // credential is being able to reach the port, so there is no secret to + // authenticate it with. A connection forwarded over loopback by an + // SSH/Tailscale tunnel or a local reverse proxy therefore appears local + // and is allowed. That is delegated trust rather than a bypass: by + // standing up that tunnel or proxy the user has decided the remote end is + // trusted, and reachability is bounded by that hop's own authentication + // (e.g. SSH keys or Tailscale ACLs). This gate only stops direct, + // unauthenticated peers. + // + // An IPv4 client reaching a dual-stack (`::`) bind appears as an + // IPv4-mapped IPv6 peer (`::ffff:127.0.0.1`), so canonicalize to the bare + // IPv4 form before the loopback test, matching `origin`'s handling. + if !canonical(self.remote_addr.ip()).is_loopback() { + return msgpack( + ErrorResponse::forbidden("/api/open is only available to local clients"), + StatusCode::FORBIDDEN, + ); + } + let argument = &request.uri().path()["/api/open/".len()..]; let requested_id = match Ref::from_str(argument) { Ok(id) => id, diff --git a/src/web/interface.rs b/src/web/interface.rs index 51307b67..8ec332ed 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -290,6 +290,13 @@ impl ErrorResponse { } } + pub fn forbidden>(details: S) -> Self { + Self { + kind: ErrorResponseKind::Forbidden, + details: details.into(), + } + } + pub fn internal_error>(details: S) -> Self { Self { kind: ErrorResponseKind::InternalError, @@ -302,5 +309,6 @@ impl ErrorResponse { pub enum ErrorResponseKind { NotFound, BadRequest, + Forbidden, InternalError, } diff --git a/src/web/mod.rs b/src/web/mod.rs index 8e33d9c5..8a003285 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -5,6 +5,7 @@ mod api; mod assets; pub mod interface; +mod origin; mod ui; mod util; @@ -14,7 +15,7 @@ use std::sync::Arc; use anyhow::Context; use hyper::{ - server::Server, + server::{conn::AddrStream, Server}, service::{make_service_fn, service_fn}, Body, Request, }; @@ -33,22 +34,42 @@ impl LiveServer { /// Starts the server on the given address, blocking until it stops. /// + /// `allowed_hosts` are extra `Host`/`Origin` values to accept in addition to + /// localhost and the bind address (see [`origin::allowed_hosts`]). + /// /// `on_listening` is invoked once the server has successfully bound to the /// address, so callers can defer printing any "listening" message until /// after binding can no longer fail (e.g. due to the port being in use). - pub fn start(self, address: SocketAddr, on_listening: impl FnOnce()) -> anyhow::Result<()> { + pub fn start( + self, + address: SocketAddr, + allowed_hosts: Vec, + on_listening: impl FnOnce(), + ) -> anyhow::Result<()> { let serve_session = Arc::clone(&self.serve_session); + let allowed_hosts = origin::allowed_hosts(address.ip(), address.port(), &allowed_hosts); - let make_service = make_service_fn(move |_conn| { + let make_service = make_service_fn(move |conn: &AddrStream| { let serve_session = Arc::clone(&serve_session); + let allowed_hosts = allowed_hosts.clone(); + let remote_addr = conn.remote_addr(); - async { + async move { let service = move |req: Request| { let serve_session = Arc::clone(&serve_session); + let allowed_hosts = allowed_hosts.clone(); async move { + // Reject cross-origin requests before doing any work, to + // defend the local server against DNS rebinding. + if let Some(response) = + origin::check_request_origin(&req, allowed_hosts.as_ref()) + { + return Ok::<_, Infallible>(response); + } + if req.uri().path().starts_with("/api") { - Ok::<_, Infallible>(api::call(serve_session, req).await) + Ok::<_, Infallible>(api::call(serve_session, remote_addr, req).await) } else { Ok::<_, Infallible>(ui::call(serve_session, req).await) } diff --git a/src/web/origin.rs b/src/web/origin.rs new file mode 100644 index 00000000..bb36a7e7 --- /dev/null +++ b/src/web/origin.rs @@ -0,0 +1,589 @@ +//! Host/Origin validation used to defend the local Rojo server against DNS +//! rebinding attacks. +//! +//! When Rojo is bound to a loopback address (the default), a malicious web page +//! the developer visits cannot read the API responses directly because of the +//! browser Same-Origin Policy. However, a DNS rebinding attack can defeat that: +//! the page points its own hostname at `127.0.0.1` after loading, so the browser +//! treats requests to the Rojo server as same-origin. Validating that the `Host` +//! (and, if present, `Origin`) header refers to an address we recognize blocks +//! this, because the rebound request still carries the attacker's hostname, which +//! is a domain name rather than one of the IP literals we accept. +//! +//! Enforcement covers two kinds of bind: +//! +//! * Loopback (the default): only `localhost` and loopback literals are +//! accepted. +//! * A specific private/LAN address: `localhost`, loopback literals, and that exact bind +//! IP are accepted. Because the defense works by rejecting any `Host` that +//! isn't a recognized IP literal, clients must connect to a private bind by +//! IP. A hostname (e.g. `mypc.local`) is indistinguishable from an attacker +//! domain and is rejected. +//! +//! Enforcement is disabled for unspecified (`0.0.0.0`/`::`) and public binds: the +//! user has asked for broad, possibly-public exposure where arbitrary hostnames +//! may legitimately resolve to the server, so we can't build a meaningful +//! allowlist. Those binds get a startup warning instead. Two consequences worth +//! being explicit about: such a bind has no rebinding protection even for a +//! browser on the same machine; and even on a protected private bind this check +//! does nothing against a hostile peer already on the LAN, who can reach the +//! unauthenticated API directly. Both are the network-exposure risk the startup +//! warning addresses, not something the `Host` check is meant to cover. +//! +//! The allowlist can be widened with extra hosts (the `--allowed-hosts` CLI +//! option or a project's `serveAllowedHosts`), for example a hostname like +//! `mypc.lan` for reaching a network-exposed server by name. Listing any extra +//! host also turns enforcement back on for an unspecified or public bind that +//! would otherwise disable it, restricting that bind to localhost, the bind IP, +//! and the listed hosts. + +use std::net::IpAddr; + +use hyper::{ + header::{HOST, ORIGIN}, + http::uri::{Authority, Uri}, + Body, Request, Response, StatusCode, +}; + +use crate::web::util::response; + +/// The set of `Host`/`Origin` values accepted while enforcement is active. +#[derive(Debug, Clone)] +pub struct AllowedHosts { + port: u16, + /// The bind IP accepted in addition to `localhost`/loopback, set only for a + /// private (LAN) bind or a specific public bind that has extra hosts. `None` + /// for a loopback or unspecified bind, which has no single address to add. + bind_ip: Option, + /// Extra `Host`/`Origin` values the user explicitly allowed, via the + /// `--allowed-hosts` option or a project's `serveAllowedHosts`. These are + /// hostnames such as `mypc.lan`, or IP literals, accepted in addition to + /// localhost and the bind IP. Each entry is already passed through + /// [`normalize_host`]. + extra_hosts: Vec, +} + +impl AllowedHosts { + /// Returns whether the given host and optional port are allowed. The host is + /// accepted if it is `localhost`, a loopback IP literal, (on a private or + /// specific public bind) the exact bind IP, or one of the explicitly allowed + /// extra hosts. A request with no explicit port is accepted (the host + /// already has to be one we recognize). + fn allows(&self, host: &str, port: Option) -> bool { + let host = normalize_host(host); + let host_ok = host.eq_ignore_ascii_case("localhost") + || host + .parse::() + .ok() + .map(canonical) + .is_some_and(|ip| ip.is_loopback() || self.bind_ip == Some(ip)) + || self.allows_extra(host); + + host_ok && port.is_none_or(|port| port == self.port) + } + + /// Returns whether `host` matches one of the explicitly allowed extra hosts. + /// Entries that are IP literals are compared as addresses, so equivalent + /// forms (such as an IPv4-mapped IPv6 literal) match; everything else is + /// compared as a case-insensitive hostname. + fn allows_extra(&self, host: &str) -> bool { + let host_ip = host.parse::().ok().map(canonical); + self.extra_hosts.iter().any(|allowed| { + match (host_ip, allowed.parse::().map(canonical)) { + (Some(host_ip), Ok(allowed_ip)) => host_ip == allowed_ip, + _ => host.eq_ignore_ascii_case(allowed), + } + }) + } +} + +/// Builds the allowlist for a given bind address and any extra allowed hosts. +/// Returns `None` (validation disabled) when bound to an unspecified +/// (`0.0.0.0`/`::`) or public address and no extra hosts were given, where +/// arbitrary hostnames may legitimately resolve to the server. Listing extra +/// hosts keeps validation on even for those binds. +pub fn allowed_hosts(bind: IpAddr, port: u16, extra: &[String]) -> Option { + let extra_hosts: Vec = extra + .iter() + .map(|host| normalize_host(host.trim()).to_owned()) + .filter(|host| !host.is_empty()) + .collect(); + + if bind.is_loopback() { + Some(AllowedHosts { + port, + bind_ip: None, + extra_hosts, + }) + } else if is_private_bind(bind) { + Some(AllowedHosts { + port, + bind_ip: Some(canonical(bind)), + extra_hosts, + }) + } else if !extra_hosts.is_empty() { + // The bind is unspecified or public, where validation is normally + // disabled. By listing explicit hosts the user has opted back into it, + // so we accept localhost, the bind IP (when it is a specific address), + // and those hosts. An unspecified bind (`0.0.0.0`/`::`) has no single + // address to add. + let bind_ip = (!bind.is_unspecified()).then(|| canonical(bind)); + Some(AllowedHosts { + port, + bind_ip, + extra_hosts, + }) + } else { + None + } +} + +/// Collapses an IPv4-mapped IPv6 address (`::ffff:192.168.0.1`) to its IPv4 form +/// so it classifies and compares consistently with the bare IPv4 address. Shared +/// with the `/api/open` peer check so it recognizes a mapped loopback peer too. +pub(crate) fn canonical(ip: IpAddr) -> IpAddr { + match ip { + IpAddr::V6(v6) => v6.to_ipv4_mapped().map(IpAddr::V4).unwrap_or(ip), + v4 => v4, + } +} + +/// Returns whether a bind address is a specific private/link-local address, the +/// case where enforcement stays on with the bind IP added to the allowlist. +/// Unspecified, loopback, and public addresses are excluded (loopback is handled +/// separately by [`allowed_hosts`]). +fn is_private_bind(ip: IpAddr) -> bool { + match canonical(ip) { + IpAddr::V4(v4) => v4.is_private() || v4.is_link_local(), + IpAddr::V6(v6) => { + let first = v6.segments()[0]; + // Unique local (fc00::/7) or link-local (fe80::/10). The std methods + // for these are still nightly-only, so test the prefix directly. + (first & 0xfe00) == 0xfc00 || (first & 0xffc0) == 0xfe80 + } + } +} + +/// Validates the `Host` and `Origin` headers of an incoming request against the +/// allowlist. Returns `Some` with a ready-to-send `404` response when the +/// request should be rejected, or `None` when it is allowed to proceed. When +/// `allowed` is `None` (unspecified or public bind) every request is accepted. +pub fn check_request_origin( + request: &Request, + allowed: Option<&AllowedHosts>, +) -> Option> { + let allowed = allowed?; + + // The Host header is mandatory and must refer to a local address. + let host_ok = request + .headers() + .get(HOST) + .and_then(|value| value.to_str().ok()) + .and_then(parse_authority) + .is_some_and(|(host, port)| allowed.allows(&host, port)); + + if !host_ok { + return Some(reject()); + } + + // The Origin header is optional: non-browser clients such as the Roblox + // plugin never send it. When it is present (i.e. a browser made the request) + // it must also be local, which rejects a rebound page whose origin is still + // its own non-local hostname. + if let Some(origin) = request.headers().get(ORIGIN) { + let origin_ok = origin + .to_str() + .ok() + .and_then(parse_origin) + .is_some_and(|(host, port)| allowed.allows(&host, port)); + + if !origin_ok { + return Some(reject()); + } + } + + None +} + +/// Normalizes a host literal for parsing/comparison: strips the surrounding +/// brackets from an IPv6 literal (e.g. `[::1]`) and drops any IPv6 zone id (e.g. +/// `fe80::1%eth0`), which `Ipv6Addr::from_str` would otherwise reject. +fn normalize_host(host: &str) -> &str { + let host = host + .strip_prefix('[') + .and_then(|host| host.strip_suffix(']')) + .unwrap_or(host); + + host.split('%').next().unwrap_or(host) +} + +/// Parses a `Host` header value (an authority such as `localhost:34872`) into +/// its host and optional port. Returns `None` if the authority carries userinfo +/// (e.g. `evil.com@localhost`), so a value whose host looks local only after the +/// userinfo is stripped can never sneak past the allowlist. +fn parse_authority(value: &str) -> Option<(String, Option)> { + let authority: Authority = value.parse().ok()?; + reject_userinfo(&authority)?; + Some((authority.host().to_owned(), authority.port_u16())) +} + +/// Parses an `Origin` header value (an absolute URI such as +/// `http://localhost:34872`) into its host and optional port. Returns `None` for +/// origins without a host, such as the opaque `null` origin, or for origins whose +/// authority carries userinfo (see [`parse_authority`]). +fn parse_origin(value: &str) -> Option<(String, Option)> { + let uri: Uri = value.parse().ok()?; + reject_userinfo(uri.authority()?)?; + Some((uri.host()?.to_owned(), uri.port_u16())) +} + +/// Returns `None` (rejecting the value) when an authority contains a userinfo +/// component, identified by the `@` separator. A bare host or `host:port` never +/// contains `@`, so this only fires on `userinfo@host` forms. +fn reject_userinfo(authority: &Authority) -> Option<()> { + if authority.as_str().contains('@') { + None + } else { + Some(()) + } +} + +/// Builds the response sent when a request fails Host/Origin validation. It is a +/// generic `404` with no Rojo-identifying body: a rejected request may be a +/// prober (or a DNS-rebound page's same-origin script, which could read the +/// body), so we reveal nothing rather than confirming a Rojo server is here. +fn reject() -> Response { + response(StatusCode::NOT_FOUND, "text/plain", "Not Found") +} + +#[cfg(test)] +mod test { + use super::*; + + use std::net::{Ipv4Addr, Ipv6Addr}; + + const PORT: u16 = 34872; + + fn loopback_allowlist() -> Option { + allowed_hosts(IpAddr::V4(Ipv4Addr::LOCALHOST), PORT, &[]) + } + + /// An allowlist for a server bound to a specific private (LAN) address. + fn private_allowlist() -> Option { + allowed_hosts(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), PORT, &[]) + } + + /// An allowlist for `bind` with the given extra allowed hosts. + fn allowlist_with_hosts(bind: IpAddr, hosts: &[&str]) -> Option { + let hosts: Vec = hosts.iter().map(|host| host.to_string()).collect(); + allowed_hosts(bind, PORT, &hosts) + } + + fn request_with(headers: &[(&'static str, &str)]) -> Request { + let mut builder = Request::builder().uri("/api/rojo"); + for (name, value) in headers { + builder = builder.header(*name, *value); + } + builder.body(Body::empty()).unwrap() + } + + #[test] + fn loopback_bind_enables_enforcement() { + assert!(allowed_hosts(IpAddr::V4(Ipv4Addr::LOCALHOST), PORT, &[]).is_some()); + assert!(allowed_hosts(IpAddr::V6(Ipv6Addr::LOCALHOST), PORT, &[]).is_some()); + } + + #[test] + fn private_bind_enables_enforcement() { + for bind in [ + IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), // 192.168.0.0/16 + IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1)), // 10.0.0.0/8 + IpAddr::V4(Ipv4Addr::new(172, 16, 0, 1)), // 172.16.0.0/12 + IpAddr::V4(Ipv4Addr::new(169, 254, 0, 1)), // link-local + IpAddr::V6(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 1)), // unique local + IpAddr::V6(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 1)), // link-local + ] { + assert!( + allowed_hosts(bind, PORT, &[]).is_some(), + "private bind {bind} should enable enforcement" + ); + } + } + + #[test] + fn unspecified_or_public_bind_disables_enforcement() { + for bind in [ + IpAddr::V4(Ipv4Addr::UNSPECIFIED), // 0.0.0.0 + IpAddr::V6(Ipv6Addr::UNSPECIFIED), // :: + IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)), // public + IpAddr::V6(Ipv6Addr::new(0x2001, 0x4860, 0, 0, 0, 0, 0, 0x8888)), // public + ] { + assert!( + allowed_hosts(bind, PORT, &[]).is_none(), + "bind {bind} should disable enforcement" + ); + } + } + + #[test] + fn accepts_local_hosts() { + let allowed = loopback_allowlist(); + for host in [ + format!("localhost:{PORT}"), + format!("127.0.0.1:{PORT}"), + format!("[::1]:{PORT}"), + "localhost".to_owned(), + ] { + let request = request_with(&[("host", &host)]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_none(), + "host {host} should be allowed" + ); + } + } + + #[test] + fn rejects_foreign_host() { + let allowed = loopback_allowlist(); + let request = request_with(&[("host", "evil.com")]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_wrong_port() { + let allowed = loopback_allowlist(); + let request = request_with(&[("host", "localhost:1234")]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_missing_host() { + let allowed = loopback_allowlist(); + let request = request_with(&[]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_host_with_userinfo() { + let allowed = loopback_allowlist(); + // The host parses to `localhost`, but the userinfo prefix must keep it + // from being treated as a local address. + let request = request_with(&[("host", &format!("evil.com@localhost:{PORT}"))]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_origin_with_userinfo() { + let allowed = loopback_allowlist(); + let request = request_with(&[ + ("host", &format!("localhost:{PORT}")), + ("origin", &format!("http://evil.com@localhost:{PORT}")), + ]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_foreign_origin_even_with_local_host() { + let allowed = loopback_allowlist(); + let request = request_with(&[ + ("host", &format!("localhost:{PORT}")), + ("origin", &format!("http://evil.com:{PORT}")), + ]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn rejects_null_origin() { + let allowed = loopback_allowlist(); + let request = request_with(&[("host", &format!("localhost:{PORT}")), ("origin", "null")]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn accepts_local_origin() { + let allowed = loopback_allowlist(); + let request = request_with(&[ + ("host", &format!("localhost:{PORT}")), + ("origin", &format!("http://localhost:{PORT}")), + ]); + assert!(check_request_origin(&request, allowed.as_ref()).is_none()); + } + + #[test] + fn private_bind_accepts_local_and_bind_ip_hosts() { + let allowed = private_allowlist(); + for host in [ + format!("192.168.1.5:{PORT}"), + format!("localhost:{PORT}"), + format!("127.0.0.1:{PORT}"), + format!("[::1]:{PORT}"), + "192.168.1.5".to_owned(), + ] { + let request = request_with(&[("host", &host)]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_none(), + "host {host} should be allowed on a private bind" + ); + } + } + + #[test] + fn private_bind_rejects_other_hosts() { + let allowed = private_allowlist(); + for host in [ + "evil.com", // a rebound attacker domain + "192.168.1.6", // a different private IP + "8.8.8.8", // a public IP + ] { + let request = request_with(&[("host", host)]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_some(), + "host {host} should be rejected on a private bind" + ); + } + } + + #[test] + fn private_bind_keeps_origin_strict() { + // A different private IP as Origin must be rejected even though the Host + // is the valid bind IP: the Origin check is not widened to arbitrary + // private addresses. + let allowed = private_allowlist(); + let request = request_with(&[ + ("host", &format!("192.168.1.5:{PORT}")), + ("origin", &format!("http://192.168.1.6:{PORT}")), + ]); + assert!(check_request_origin(&request, allowed.as_ref()).is_some()); + } + + #[test] + fn private_bind_accepts_bind_ip_origin() { + let allowed = private_allowlist(); + let request = request_with(&[ + ("host", &format!("192.168.1.5:{PORT}")), + ("origin", &format!("http://192.168.1.5:{PORT}")), + ]); + assert!(check_request_origin(&request, allowed.as_ref()).is_none()); + } + + #[test] + fn accepts_ipv4_mapped_bind_ip() { + // `::ffff:192.168.1.5` is the IPv4-mapped form of the bind IP and must + // be treated as equal to it. + let allowed = private_allowlist(); + let request = request_with(&[("host", &format!("[::ffff:192.168.1.5]:{PORT}"))]); + assert!(check_request_origin(&request, allowed.as_ref()).is_none()); + } + + #[test] + fn canonical_collapses_ipv4_mapped_loopback() { + // The `/api/open` peer gate relies on this: an IPv4 loopback client + // reaching a dual-stack (`::`) bind arrives as `::ffff:127.0.0.1`, which + // is only recognized as loopback after canonicalization. + let mapped: IpAddr = "::ffff:127.0.0.1".parse().unwrap(); + assert!(!mapped.is_loopback()); + assert!(canonical(mapped).is_loopback()); + } + + #[test] + fn normalize_host_strips_brackets_and_zone_id() { + // Brackets and an IPv6 zone id must be removed so the result parses as an + // `IpAddr` (`Ipv6Addr::from_str` rejects zone ids). + assert_eq!(normalize_host("[::1]"), "::1"); + assert_eq!(normalize_host("[fe80::1%eth0]"), "fe80::1"); + assert_eq!(normalize_host("localhost"), "localhost"); + assert!(normalize_host("[fe80::1%eth0]").parse::().is_ok()); + } + + #[test] + fn allowed_hosts_extend_the_allowlist() { + // A hostname listed as an allowed host is accepted on a loopback bind, + // with the usual port check still applied; an unlisted host is not. + let allowed = allowlist_with_hosts(IpAddr::V4(Ipv4Addr::LOCALHOST), &["mypc.lan"]); + + let ok = request_with(&[("host", &format!("mypc.lan:{PORT}"))]); + assert!(check_request_origin(&ok, allowed.as_ref()).is_none()); + + let wrong_port = request_with(&[("host", "mypc.lan:1234")]); + assert!(check_request_origin(&wrong_port, allowed.as_ref()).is_some()); + + let other = request_with(&[("host", &format!("other.lan:{PORT}"))]); + assert!(check_request_origin(&other, allowed.as_ref()).is_some()); + } + + #[test] + fn allowed_hosts_apply_to_origin() { + let allowed = allowlist_with_hosts(IpAddr::V4(Ipv4Addr::LOCALHOST), &["mypc.lan"]); + + let ok = request_with(&[ + ("host", &format!("mypc.lan:{PORT}")), + ("origin", &format!("http://mypc.lan:{PORT}")), + ]); + assert!(check_request_origin(&ok, allowed.as_ref()).is_none()); + + let foreign_origin = request_with(&[ + ("host", &format!("mypc.lan:{PORT}")), + ("origin", &format!("http://evil.com:{PORT}")), + ]); + assert!(check_request_origin(&foreign_origin, allowed.as_ref()).is_some()); + } + + #[test] + fn allowed_hosts_enable_enforcement_on_exposed_bind() { + // Binding to 0.0.0.0 normally disables validation, but listing a host + // turns it back on: localhost and the listed host are accepted while + // everything else is rejected. + let allowed = allowlist_with_hosts(IpAddr::V4(Ipv4Addr::UNSPECIFIED), &["mypc.lan"]); + assert!(allowed.is_some()); + + for host in [format!("mypc.lan:{PORT}"), format!("localhost:{PORT}")] { + let request = request_with(&[("host", &host)]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_none(), + "host {host} should be allowed" + ); + } + + let evil = request_with(&[("host", "evil.com")]); + assert!(check_request_origin(&evil, allowed.as_ref()).is_some()); + } + + #[test] + fn allowed_hosts_on_public_bind_accept_the_bind_ip() { + // A specific public bind with an allowed host accepts localhost, the + // listed host, and the bind IP itself, but nothing else. (203.0.113.0/24 + // is the TEST-NET-3 documentation range, treated here as a public IP.) + let bind = IpAddr::V4(Ipv4Addr::new(203, 0, 113, 5)); + let allowed = allowlist_with_hosts(bind, &["mypc.lan"]); + + for host in [ + format!("203.0.113.5:{PORT}"), + format!("mypc.lan:{PORT}"), + format!("localhost:{PORT}"), + ] { + let request = request_with(&[("host", &host)]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_none(), + "host {host} should be allowed" + ); + } + + let evil = request_with(&[("host", "evil.com")]); + assert!(check_request_origin(&evil, allowed.as_ref()).is_some()); + } + + #[test] + fn disabled_allowlist_accepts_foreign_host() { + for bind in [ + IpAddr::V4(Ipv4Addr::UNSPECIFIED), + IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)), + ] { + let allowed = allowed_hosts(bind, PORT, &[]); + let request = request_with(&[("host", "evil.com")]); + assert!( + check_request_origin(&request, allowed.as_ref()).is_none(), + "disabled allowlist for bind {bind} should accept any host" + ); + } + } +} diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 4b67a883..4a817a8a 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -126,6 +126,10 @@ impl TestServeSession { &self.project_path } + pub fn port(&self) -> usize { + self.port + } + /// Waits for the `rojo serve` server to come online with expontential /// backoff. pub fn wait_to_come_online(&mut self) -> ServerInfoResponse { @@ -241,6 +245,39 @@ impl TestServeSession { Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) } + + /// Sends a GET to `/api/rojo` with the given extra request headers and + /// returns the full response. Used to exercise the Host/Origin allowlist that + /// guards against DNS rebinding, including asserting that a rejection reveals + /// nothing about the server. + pub fn api_rojo_response_with_headers( + &self, + headers: &[(&str, &str)], + ) -> reqwest::blocking::Response { + let client = reqwest::blocking::Client::new(); + let url = format!("http://localhost:{}/api/rojo", self.port); + + let mut request = client.get(url); + for (name, value) in headers { + request = request.header(*name, *value); + } + + request.send().expect("Failed to send request") + } + + /// Sends a POST to `/api/open/` and returns the response status code. + /// Used to verify that the local-only gate on `/api/open` admits loopback + /// peers (the test harness always connects over loopback). + pub fn api_open_status(&self, id: &str) -> reqwest::StatusCode { + let client = reqwest::blocking::Client::new(); + let url = format!("http://localhost:{}/api/open/{}", self.port, id); + + client + .post(url) + .send() + .expect("Failed to send request") + .status() + } } fn serialize_msgpack(value: T) -> Result, rmp_serde::encode::Error> { diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 748f1687..490c13db 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -10,6 +10,74 @@ use crate::rojo_test::{ use librojo::web_api::SocketPacketType; +#[test] +fn rejects_dns_rebinding_requests() { + run_serve_test("empty", |session, _redactions| { + let port = session.port(); + let local_host = format!("localhost:{port}"); + + // A request carrying a local Host header is served normally. + assert_eq!( + session + .api_rojo_response_with_headers(&[("host", &local_host)]) + .status(), + reqwest::StatusCode::OK, + ); + + // A request whose Host is a foreign hostname, as a DNS-rebound page + // would send, is rejected with a generic 404 that reveals nothing about + // the server. + assert_rejected(session.api_rojo_response_with_headers(&[("host", "evil.com")])); + + // Even with a local Host, a present-but-foreign Origin is rejected. + let foreign_origin = format!("http://evil.com:{port}"); + assert_rejected( + session.api_rojo_response_with_headers(&[ + ("host", &local_host), + ("origin", &foreign_origin), + ]), + ); + }); +} + +/// Asserts that a Host/Origin rejection is a generic 404 whose body and +/// content-type do not identify the server as Rojo. +fn assert_rejected(response: reqwest::blocking::Response) { + assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND); + + let content_type = response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .to_owned(); + assert!( + !content_type.contains("msgpack"), + "rejection should not use the msgpack API content-type, got {content_type:?}", + ); + + let body = response.text().expect("Failed to read response body"); + let body_lower = body.to_lowercase(); + assert!( + !body_lower.contains("rojo") && !body_lower.contains("rebinding"), + "rejection body should not identify the server, got {body:?}", + ); +} + +#[test] +fn allows_api_open_from_loopback_peer() { + run_serve_test("empty", |session, _redactions| { + // The harness always connects over loopback, so the local-only gate on + // /api/open must let the request through. A bogus instance id then fails + // id parsing with 400, which confirms we got past the gate rather than + // being rejected with 403. + assert_eq!( + session.api_open_status("not-a-real-ref"), + reqwest::StatusCode::BAD_REQUEST, + ); + }); +} + #[test] fn empty() { run_serve_test("empty", |session, mut redactions| { From d12120bab6ec11c861c7b17f832df163ead17738 Mon Sep 17 00:00:00 2001 From: Floyd Horng Date: Thu, 11 Jun 2026 02:47:50 +0100 Subject: [PATCH 3/4] Fix syncback not removing stale $properties at engine defaults (#1244) --- CHANGELOG.md | 2 + ...ject_default_properties_remove-stdout.snap | 5 ++ ...roperties_remove-default.project.json.snap | 59 +++++++++++++++ .../input-project/default.project.json | 21 ++++++ .../input.rbxl | Bin 0 -> 44361 bytes src/snapshot_middleware/project.rs | 70 +++++++++++++++--- tests/tests/syncback.rs | 3 + 7 files changed, 150 insertions(+), 10 deletions(-) create mode 100644 rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap create mode 100644 rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap create mode 100644 rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json create mode 100644 rojo-test/syncback-tests/project_default_properties_remove/input.rbxl diff --git a/CHANGELOG.md b/CHANGELOG.md index cff39688..00524693 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Making a new release? Simply add the new header with the version and date undern * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267]) * `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. The accepted hosts can be extended with the `--allowed-hosts` option or a project's `serveAllowedHosts` field, for example to reach a network-exposed server by hostname. ([#1270]) +* Fixed syncback not removing stale `$properties` entries when Studio resets a property to its engine default. ([#1244]) [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 @@ -60,6 +61,7 @@ Making a new release? Simply add the new header with the version and date undern [#1266]: https://github.com/rojo-rbx/rojo/pull/1266 [#1267]: https://github.com/rojo-rbx/rojo/pull/1267 [#1270]: https://github.com/rojo-rbx/rojo/pull/1270 +[#1244]: https://github.com/rojo-rbx/rojo/pull/1244 ## [7.7.0-rc.1] (November 27th, 2025) diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap new file mode 100644 index 00000000..3ef76951 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap @@ -0,0 +1,5 @@ +--- +source: tests/rojo_test/syncback_util.rs +expression: "String::from_utf8_lossy(&output.stdout)" +--- +Writing default.project.json diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap new file mode 100644 index 00000000..1b8d07d2 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap @@ -0,0 +1,59 @@ +--- +source: tests/tests/syncback.rs +expression: default.project.json +--- +{ + "name": "SyncbackTest", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "TestPart": { + "$className": "Part", + "$properties": { + "Anchored": true, + "Color": { + "Color3uint8": [ + 0, + 0, + 255 + ] + } + } + }, + "$properties": { + "EnableSLIMAvatars": { + "Enum": 0 + }, + "ImprovedAnimationConstraint": { + "Enum": 0 + }, + "ImprovedPhysicsReplication": { + "Enum": 0 + }, + "LayeredClothingCacheOptimizations": { + "Enum": 0 + }, + "MeshStreamingAndImprovedLods": { + "Enum": 0 + }, + "NextGenerationReplication": { + "Enum": 0 + }, + "PlayerScriptsUseInputActionSystem": { + "Enum": 0 + }, + "UseFixedSimulation": { + "Enum": 0 + }, + "UseNewLuauTypeSolver": "Disabled", + "ValidateEnabledProximityPrompt": { + "Enum": 0 + } + }, + "$attributes": { + "Rojo_Target_CurrentCamera": "302d573157260ee80a3baa32000003b5" + } + } + } +} diff --git a/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json b/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json new file mode 100644 index 00000000..6f0b1ade --- /dev/null +++ b/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json @@ -0,0 +1,21 @@ +{ + "name": "SyncbackTest", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "TestPart": { + "$className": "Part", + "$properties": { + "Transparency": 1.0, + "Anchored": true, + "Color": [ + 1.0, + 0.0, + 0.0 + ] + } + } + } + } +} \ No newline at end of file diff --git a/rojo-test/syncback-tests/project_default_properties_remove/input.rbxl b/rojo-test/syncback-tests/project_default_properties_remove/input.rbxl new file mode 100644 index 0000000000000000000000000000000000000000..7ca3700a7bd1b8c41ba7aa37e4e615585f43e662 GIT binary patch literal 44361 zcmb`QdyE{%o!@7d!>34zq)5FVBONVKvM9bJS(YE%6~+NUoXs`B}dSxDjq;>bPH(c)y@1 zgB0zm%HJJw%``T=uwe?=K>?qYpP!P86g{wA{@y2-RI1bQ+RKfVZeiVyKnn04xk%AY z@~0fyV_JVu)#Vxa6%LT12Y1R}Y6ot|`b zenxP=A{Qw-v|Ik72s1&u?J3S>*!GgDs?BauD9!Gm)AMfhriCG->JnN>&{@t9ns=-@v2i;kWbQ7$&R+04%>k|zZ~ z`$^F~RON2D&iDP+lDFn}dpV_fRgpa^poirmMR!sPExFL^ZDyLVs^C-#5>m8(zx<_g z^PR32W)0`dih#ULwf;{PaUf73=ruY`Pqb%3ta5X5auWGrvM`Ah-EmO<0$%Xf)-f=n zVe2YmEW~6yMN_L%|AnC0X#3w#HkGOW3yR7X8g>Pz7J}BSSaODoXULao9a?3{*A(>Nq zQnUwM!_{cQq{)XA^rSpEE*B}P;roFvhC$Qoc8xw3SMqBr16!^b0dh?xH0Oui#0%b| zNNj&p6F3HtlGLSLA{?XGZzv+$#JEG@>dnyK%$nFGML;`X1S#4J_wjsnFLrt+_X`Sa zdpp{M5eRcdH;WDDhN2rZw`e;`fz2TlFVuD0Tlr|9+gvGV3+zyB(NuQhTS@gyzwwJb z6pA9pvcKYY{T09M_qGmHe%mzbHAPnwRN9}TOSsp1(C@S|inB-I?LG#kU=nrd&BaWm zs|g!T`G;;;*Vx1fd=#aa&?4H0=F(-vl;Unpy`EF$*zT25aj!8N+cfY~3U14uep>NM zS4*q4A$Ud+*j}GrSBZ0wAk&Cv^Nl!`R4WuWd!^Ctd#1@#dGxb_)=loW`~bgcs`Rv~ zl5HoL9CorS>bM!?I*At*iQO@t5IAPM?1f>&Phz7gRk8VrO~cP+ubVZEPC;Ex2~^kR zMvrhJqZtn=lA3^0O~NaqNz1-OQ5pO$g|}_|Il*G%%R#@nu0ov*(NaMnxPMj1b^P%|;qOS0)yhE-%`OEjZ=jr`zq|x(*)ZLeEp-cUQu6A+S3U2x!D~uNV3& z{hkW2l*jTWcm{>78vIKArNs^9GbAXZ5-JtLDPY!H^*dg7kV@Bw+^G{O+Qwg6dqD!L zAl#a7J$kUR&5SRKbEkv7Bmvw6wHh2~0I$h!snW@fw)>zTHZ@n~TgJmHc2ZT;j1*1s zm+Y4tG6>;@ENwI7SHgi;I{w%Dp47Ke*>-);7k+%|jn{s%@{@&!KZ3;OIXRW%ITR&$ zZVDL6bF|*@{B{+%&2tqy%CD-ukIDZL5-HjbIN>oKEhtAx%XSPZ%%|$^^2?$bBbRT2{wJF(h*yxDKhhC?-wf#4LK>FurAhf*}gh)aUfoJ29 zZ}r!HuI1+fckAL&q-(0acT0n9TPNoddI#KT>$Y_Pr#m{=X~@vMHCbSQK+v{188i|h zAEfnPm4Cb`7wMBzKu`uCbK)Lr#2n$u4H?dPy_b$3UzOjx$5$I065n{;)Utr|{0E*X+;S1HS3DRlRQ7NqDQ zz{pb#opgv%@AX^V%d4wWGX$0-N+Q}OR3b&YASM>L*tns^+O6KYrA1`-=>Sn|0nUog zWb9Y{_J_lSiCSLtJN}0M4KIPeKc(S$i%ATfiVc}zq3$(#oO)TRRL0`LQ<7!zl$Z{r zOe>cgep>~rZBMJqV**Q^6EL{8EE5c|&3*HyKmE7g{Ifs%^KbseU;HIiwFfp3pga)N zg(s#jr~$Pdy@3uo`hS#lI*&_UHMvO90Z35!D>8ze(Lo^D4&uC9R#bcx=U)=c zlw73$xdVzFa?Q+z5`ADqe<<%>`cfI;;8Bx+WdmsmrvB3cHV$e!!ZFf(gw}TFv{qBAYNTv6tG%xU{Xcj5j z4j5is@ER@gve!%>!pIgRDcT82>9C;cVufDyru;%h?Jy|8C84Jk29c@p&r@J!T1>~C zN(q$c^2!a+t=vSLCgP0q{m`M>WSCX$Kgg)>^5L!-{_;W~^F}#2yd#mPxnL(z3bG*S7ZoUp zii**58N(O8v4FI=SUoA`wd5toK9*zKiAGtj?Qfw#^V8tYatyZF0WM*7rZz*(&i22+9qMlPKY$6L=eyJWUU1Q0 zlh9qF4LN!+J#yh zr#=O`TKUjwu04(fVY&39^22bK{{J+Nq%4Q3+W{0{6!=%Yt(Bk=w&edaaR7B3<+L5L zZfNYw2_4bl!dJu4(#9v`B1QK?3&&)z+Vs|?Uy>R7c;>?MR^N@Uyg@)$#vn``ErX~@ zCTZVbsT5H}keTC(2@Qbiv~VyKBq= z(%}f~-W@Q_z7T}|H)K|;(VlMmYn=^G&y6XoARwe=r09Mqk`9f&4rfrX)+2AyP?j-i zQ<7!Uwuu56VWO}RpGAr#7EO*}p=A9jEEKXaEfh)69FnQoo|wrP)>XgNTemgHRtmIdl;zv~EEFXy(G)P$67j3li8KYeTKQ+`>lSUB?7RFL`=pGiI^%5`k#(lBGh1% zL~N%I1qF>M5>vpKA_49YiS0ZACsx3~CbRkyrc-&-c}{79Xs0YX8tbjSDZs&VAXzKe zaPu5{Eyr^xO7PqiFecA|J9y5hj_#He9R{56oK_SpU(G8!RB&os#+y|eGdNL`RA_g)+6`DDAA^q<2j%4gx{O>*?hd&C z$;#!2S-IG~838vl&Y87FyxeIQt~G+7oFJhnAxKlen1TfE5M)gr8y^(i1vrrq%(s?i z=Vb)GjEqeF4jBQ-%BY={5wYGga*=M|NmM*2-7Odx(|74m!ZD_>kkH~(2`JYVA$%`y6^kfNYJYYCnd2&(hD4;Ra z?S+{3lU)!ONJ6z{LJ|}htqEWLvnfrBVvtLh1@@IV<6mq8XUk(k#@tm&ckMrLaF3)f zNcyp)Q%!qVASUpMRim%ZkCM<)2zwj}mg^}Uv!N0;^I+Z0#q7>N>y*w2Q{ zYRFo?Zt^14jx_-0wMTv=QS+nn|8&+7IIL8YYV~!0wfFkjpw|mF$RkP6F(t>?jT9Y% zj_zcDZaLV@5#{R!n+g?`GVFenGSH24owxijP*W|l{_1MK>*+DMJW?&PM_5qt8f}28 zzUeh(xxFlW<^{ANvj?J(F&;a2V>9T=V$vCzqxU*pwdW!sIU)e`n-tv*2|oDTjh@$O zDY-5+I&0o?aPrg%9;({qS%BKk1#60Rd*8h!iWOh5fDGXgDXNmo4!Hz=uG6Z1_k-V7 zdD`6^pg2#p1Usp2O&(A@X$q8#3wE9N0(oDvx$ZX?q;aw)%B#MA@jv~Hvhym3>7H)~ zE3$lMy&;>F;;zLW0F!gho21UfUTy)HT#Xsjds}U_9ziL+Jx#2u91jd=p&=TskC*$s zRuF{4%oYigD=FH>zsOdI+N_V&uch*e4qp~SUuOP#GFfdZ@s)mqMf>`-7aXDTq-ZZ0 zV;!1l^ky9y>#M){0Ba7B_IZeOMjT{1?kRea*?K}yQtmn@fTG=;q+jz&h!^1 z{~bepimj_;?$A|akCBr;(p8VuZ=YCW!9J`!OQ9+wPE(2{PMhmMvRq%ua@~#zP;|(~ zXFC*5w%d-(6fh>=fjjt)${0@-?FWp8t0fWGk-d{G&vOXM@Z1#O;5m>i&;P{DbK|>))C&Plc&;4u;9kHy%fXjnxyj$bav)ij|D7z$ZB5|?0wcER z?&f-<-BSvq!Zv+RwQakApqyPmp(3wF*OWkyd{*63fbIH$!*mJMn%7i)wwpV^wh;k# z%AeNa3rN4%1stBw$ct$8Hd^Js`(R~J-4*9g-GyM&)7+u%`W868?kTxQx9=eT{c_F7 z*6IzvDGS!Tt}0^h;mV@Ag9ab@S-D96YcDh^fRt^GvW|Z>CE9n-01=HKP9yzt4@3-l zX9D((%Vv{~-V|`hp~|AVqa51@LZVm|bef^8olx~cTV&s~`oU4PAPi+(+`8z=TRTNZ z)zqCSr=V)GS5DS6E;Qb1C=#ls?jXra5uF-pmc?A2^&1;OrzP+E-S81rj#o192U}`$w)Y`L6|C?ON5xpjT zyc?kEcD%;wR$A>ncUKmZY8P{H!E5x^2e=sDQ$(>TFG%2(>OZEO=zlR|E;d3jnS?R= z)-WcGKn-J3ormd(&KEQ0V!x#_hB5MQ!Z8ILCxxw+V0?*!KqwsS-NHOW5>UeL=j!HCgg zM=$~;8;rb?4My^Uq#t;t>zz7r>XfmK?yDQ^qP3BxgsVZ2N{1WTSL~3~({`hCP5V(C zlDab#H7T7G9bkr_T$}@iGZwKMYYG_J>Dl8h;0{N9S{|R0i}Z7_XsQzWW4om_qmGQo zYXo_e(FjwfLnDA>HDWHS5q7l6JXAR;Kv6;prhqY}0Nf!39;20{Xs4;le0SDc-)aqA z;|nekmv-&?a1Nwm`)Z)fMAX;4cH5lcF(aEadvrN!Zsdsk-}LX%-4HM=s9*CpH@%iQ z&_aKkKRB6SAPlo7qDzeDsuvZDW1R`iS*X&foqMh^?gbf>u<$Yi;mTe8&y%qaD+ zyCNv)cc(d|V5ZSw(T?nZ*O}X$I(K^9A{~{S{ID@yeYbE^Qx13*pc00?vJN1FEjBpR z`{AmrQ&sIn&bD&^$v|QNsaC-z@O-OKdOh=;sq9E-T3vglkxHNkI@f^;SM!PqmdaUJ zyUHk7`pysnHI&l(&nD$INo0>1p%A2KJ7_%)vn5unG9uTn^u2IPOLZtl9mb$BdG0%X z$gs-EaT98mS7&NlB29=V{M&M1NzfrA-<%+D66eFh{^4q%v^1faZzaY~rnps7w2M3R zOHGWQ?)#nIi=u#^kZ-;&nB+`fLX{1Dx=o`khu&wA+Qr0w6^b)CZf4cOxoJ62|?v2-#deQXqt+k|gYhP;Io9VDcE0*s_7%BoiWYQmtdG~}qEPOrYN@}ayUXiWfu zC@)m2tK^6mQ5a=A_P~h(Fos3{t)QoOt*1LOP42~vUGi3WF{^f?k3jM=aV&*hN|32o z^KHZHC}dWyHF=Qs(6kNX8id+{9g;QYMCkW0TZs?Jcd4*#Dd=7C+CfuZ(BQR_giLC3 zXIG|7xS%DUdHsSnN+TyA7?`{f%`n%EOIb?}c^j1c1{k}Iw&Z&y!h;kz4qFFzMxdQa zar4@k4S;?(8vx~v7m}f3DDJISgK$Gml92OfmKxWwZ}naE~|A|KXagGg#56FwAZn6wV$t4&Q zd7eX1hUcaL2hV|IdG5P;j?tClITTKw+k62|XBx+5fgJQG5t$C7;w_@KahMtn6_Twq z2CffKloKQw35IG@Uef92YlLAK3Rb+TH@WE{TXuXz5e_{#DvwPZ5Zwzn(E}PTGcx(} z9rI2HI(m#jxJ1AJS7)tO4lwrD61LuxneED zg+(N@HQ!kc61LTZBJCF;Eu*ixs7$|T_aZ>>?*yonQLsMfOXx7djqBHAEHIb$GTI1w!P?;_7G}2cBPF32Z^1l=S>_mDNbTnxQc zImOB2jTXqk0#vV?FF-|Vo}99Ey~7HW>U&d?jTBIm>{3mnfHIm0o~k=MCy=aKUCydi zo`cx8%tX!n%QNW4# z;g<7A*}Tl4FC#Nkosk z2xNZs<%5@ahaQ$`_ zxW)BH9s42yCrnp^)a0=0x<1-B&vt0au-%m5P<9|$wtv^ncHC7twnI^Z?WTY+*$&*n z_L@A#I!Mu7fD^VWUJjlNoo6}tGAuXwJ6H}R%ksC}EJw%6u^fsLEH?#=$#UQhmJ@%` z36r8b04FSmDp}8;XE)d~>^6Bj*bOAh?oBtl2@T7!8;TO_HU*5yZr~1f+hG#mgxy5J zCMH)A#L(?Vkd@)QDaOHhAQ{f@{3AE#j|fRQ&O=dx^QM3?IS<^ydHg?vV^XviFrJ*i zTSB*i2ww%8jOBokqQm^FDwlfsOp}p}oF3+OM^KfKk}1U@B_J6oZTrwIC1Ub&Qi7s{ zluQ9*N(s0_N;P?$_5gqr#w%V9o~NI8N#M({+~n_IIgl*NzvpH-%2STzP?TV~DPSne z>9z>cNYQ@a3}r9)tyXgCdUpcGl3=4996{ia(P!oHQ&38Z9sr!kh%sO6w$e`+4E1A> zmJydJ${{WwS#f=Pw77_*<6~^&+S>d<3viL7=nzhvS(oW%Aa5J}X5Cj`?#NEn*j+0` zG&bIbvgC;eSz#hT34orj#*L)t0N7C)R1-{UDNx|g)FOcCP5Je#7v6r z1n<;10JNGE-NPNSO(reW_a?eB?!Dw)m*SFA;6Gl96di_)R~B(y7UUpLuj7TxwDEbT zgeqS*T0d01ZC6eqsJA&jS%o0VD7Et}sOQi^+@OoGA2yuY6k*>!D(UAM&71XdUKEiVji?3|*255_z4c zcU@*Xui_SS#%Qp*Y9C!wle}ytDr1k-hRQqa5lGe^KcBTnY5?;{(LP9&JqGRmh9`Y< zQ*jbyQL}HhsUU_?red=k3a9Gl;Ul$?I+1Qy$=#tf%)|mAMfdZsMEb}z{#;6GW-7!` z6KpkkoQev{GNLjiIYb3ymt2GUxc)&_R5tVJ1o#RmrMyz_*87`$Vx=pubX}gr)X(WjQrQ9DE!U7!du+D@IFTKzA!6CdK@Qlh-30?? zqEPJC>*={r!CmgiY5&(ddTC8QcbwW%udR;T((O$gW=$TY%0TWoe+fwCvnrQEb|)un zQR`l83j{IDl`J!kr1Hwdlcvuz8(-pg4C6iy4V-IFO`Oce*dTdpB0ZtDr) zM8%nxk5!yin&@~neR zr;!>2#wkU2n+DCO1I^`K&1*rhVPc)?uYagR7blJ1v1s0v{1G2f5r#^Nc2UkAx#DsL zPlUHM4xm#(>A=&_5jgxy`n0p5*SMxw(Y@T6Wpj-7I=10JhFB6ZIIYibtiJcNpQ*j+ zdCB9|EQ-b8x))a8kGW)r@f5-9)`UQqTNhvGO|vVb`t6oxQ@#}RDLbD#;b+f_Lu}o*h(qd|4|3#% z2U~}_i+mZ>OnM2e7nhsOcAsUNmd|@eAlND?dXNUtn?WQeCdfCeLZeiA!;eL4cOKB9 zlLNFDOB`=DYHQ>fGmSVwVoJPjf9ZicY1tYUfUH^Q@99kxaC!GkVR zR`SdfpfQ@@q6`^XYH;oHCPN^tG4PJ?ixk~&c)+3r^Tpk1KAtl_D`T7FlPIdFu_>t` zW~dTMN03EHr05~T!GbbbX*nHke0t&qZ_Og6EaXRu9^eiGC@6W6P|qUeWhBZ*iXMiP z#|5Nn8DX~8<)}Y(wPll6*!y(VUiQ2^_1OGc4kQ|}3eIaHN^0>-eb|{_Rri(m-C5Zz zoqRr7yPaq}Jsy!eDXLLn;)20))VNa}g>p7%ZSk3uxa+y4&L$qMjcEp`^1Y`|cO`h# z#P*nh(iu$_m#({zdskOi&#bO~pvum7*|nkMEGUB})W5j% zmS(nl>(bM2%G!yf!#AIj!oiv#;dYfaJHooW)1&sxeJ9=6wCtsk1u62iD(#Z;N(WCZ z5*3)sYOA*F-;+6rL>Y>^5c!U9Qkd+|mhUx4L?0$ZHOW1VB`Lismzr@`LFM3gK5<{g z(E|8k9LtwbCR#N$azv{@veD|-veBwN%!7pCgai@t`QJdk_Gm3MvKxuxqwgO7ED0#~ z$~80Ixu2$hp}~Qzbif@-XS))>iA2?_u0e^$Uv5RPH36oI09hF^nPMDb0+JQe-_MH4 z<~-i19Ot3fE7zEuHwBEzdEgGt*W_`^Lcj^<_sNYOT#F04(CDt`Sq>i9xQ9%d{2eR@ zl41Fw+%lb)1oWa@r06L~w#%iy(K|tq`1Z-?PR@Pqh1utiO`kmV;<4vWzi|54+0)OR zI(G8($+OSDc<$thxf9R7llKky=5oS=qJ;2F0YimHTd;Ie^bm0RWo_wN^XX9=E6Jq& zB$v7WSkc};C*1mJk-@>?hreWjM%q*)op2X<>iD0 zMG0Y<0)`69_Hn=+{I)w*zzM(i%Z>RIbZzJ>Fi6U9-4x;AI*=^aSF>EN2`F_DC*+Q9 zXuA3JzHH#*}jr>2_2A(2Z8(G1Ok|eQe`fdh_yU2C)MpmFGG@D5? z+iC1sjT|uDPh&4V1$alC;8QNY{=U;OOTV8(9mWX>|#|(uF!e(Hm=X zDgr9l$O@K67S^sZs*t{cfnbMR@rM*A<)2ByaWEr*LXe{Eppiv(>v6HlxHhtq6KW-x zp&MDjIL&=nMMiu8!_m@H%U~_3FDaCPW-|2S9Zl9YC?S8M!f*T(Ox1nDU^w zq66Dq6BZP^r2(YP0qFM2MT#b<0XEK5lsSo@*kWB9W61@JHCtY#481Y-Yx3h8a*;mX z0}T5zFl7YYiE{#uPP}>Z%$b|lZmypBn^(U4-FILATN{^G+ky8_|ME}%&7Xhgwev4M zd*qQvKC}AOFZ|cvdb#`dwfEnC?Y%$#dH4O7|MkE6gMar=zW@F2|7}D@c*sTitKF1_ zTuf=h_Tge>*byn(2^E4iR9)o{$vrQTq?6w%1j~G;+V6loDz~N{(Y@q|$;Z3E&WC<$ z0xnE$lRrs<&JH6Iy79O98j{i5d=q4{c{e=+BHvJ|KYE)ld19!8dv_Bpt;vJDp$T~e!ZN5p%9NT>JnX9l^9YLhzLti(s9hv0ojd|$Pimn7VxA=kaCY zIx3G=S#e#lf{xljOjane!KJRH*jc3;gOORm4q`GSE=z&qNNkK9#AJ;37+i7iqim#_ z_i~U7-9c>6cv1oqzAA4>^r!Roe&Yu}_|12|^4~sqXY+^u5jp4c-){&8t?8!xSgaQG zkt%e`Pb!rU z{&%Iq|EUAPMNKYJbl{}?9Tk|{JpmLjofn=<{Z8RVH89S&%;vMH#L!s8_C>()MJxdc zef8#KLT(Er0+QOFyCLU$_q?mVe633@DUga64XD98`uESu}ZoTb!o9gwnLE6zbg(MvejpjgzfdOP5jd5P;Z^%h;msjJB zcB+d(_V#2fd1K}AibZ+7)Rf~Uy9+1Ee3@y0<;(K*z#AEs#q3-TmYDRG zZGUS&a5doxeJ#ZD* z%NPHYr;tPPBv7AQe&tox;lytOj|wV{AfUm4$i(>|&2q3NFR0B-u`&XiQ_!I~(8&kx zcwc)^ZWYpYkRa9T?7^4F^44~PGyk*@Y{@}@@+fAN%um*3 zLDQ_a8G22{Ejb@mzK&32#}whPIHY zSEw62XqNdB%GeAwq6EukY^4U0QR;11+)8~~NXjWS6i%hKZNL;T)Q#JNDc}wv=Wj*lr3KlkLDAY_}C1aKd(_ z=-0*T4dpoSG8{MAJ2(y`%kgD5$6pYVavX<((3&W6L%eyDSK@>3jqN@RI02JhI(J@T zAo&7Iys6&iAIK7-H1$YdLbI}{t(NboH4ypSY3NYN+g$~@-y_#tqvgprM6USW^~?Hz zKYC%^quNdp3Wu)PaXw($y3pu);Uen^^*eM`aeJ%*G*K%GxUw`i(qC*hL zmY?g(vK+0&fr49dBDnFOw$lPXO8&NUfdVdyGnb=mrTS)drgB7BMH*IsJq%VYHB(P#duQ33XF&*)$jWD>9eSVnrK4 zuw$$a6_H!KxL{%aqUL(KIMtp$FdUFAYn;l}qjT14v(v7M9*7j}HM!3<+HJj%&J048 z*o$QX*z2Hx?BJ1gUt94q?qB~{uQ{~)eac|$I`F~?5fzMGL1IJtV`NwTPK$UUNk{<8 z!4OE%b_ke2kheZ>VR0Y)jiNU`pd8NUDS1poNzr|PssH@1#^HqgPueK&1LPc2wMQwb z{L=zEDiT3p&U|VOc~Fen1f&j$NEemJ}Va~dXyp={b$zg z%`}?p9;=Zz;-kY-T`5-f1-1;ym%-%IDSkVtWKD8TM`M(Q9?Gh}K8rnP*1hI6%ifFG zanX|{NunxhLG^T}HQki+b!Cf^T86Rmov4&Fuc3_Q zRs0A2uovFNd`QS}7(kSc=6xT2GQl$8n^RMJiiH-TxQZre(phg^v^GdrOxhDU5Wyjv zh`WSu`Bo&|Ci;Fg2(QVpTTO4c>$)Vn7P{*6+2q5NdM*aNPN%J?%l&d)l39}TZ1`1k zH$@6sEXqaM6C!76wM3AzFwzLJF6&d9>q%?u$q|ag&|_mrXqanFNb;hgTZ&2QJ#TYU zO3W;LIVK2dNs1n#7?fPkvdXlPKEf^bYdH}W+n*+mmpszIj_i?`Rv$~PZ^;hnjikcG zy|f&6yCFwguW!ljkZwI_%SQ5qqS%)zif{|VqDyf&>m~0^*=9G;u(ro6WokjiX1gT& zGG#wgXU*YEa753@MT#DVlk|^9D})PObv$v*%Xs6%h=whR4+>?Epp={-i>1s*OjhKz zUGlvE%QTAFrq*o7+sSWAT``P&_lXCo`)hJ+o{|5lq%QZ$Gjxm;?V!!XiS@R>!P?8j zXVm0J+jqJn)I1(vt6hZ*I!!&2YUC}S@FsCBrW02UEcQhSme6x;FyI@~F;jLzbkXlD zHCleZ%WH{SuyKfxjQ?I%QcY-fsX|a)+DVEgVJ7~|ip%XcXh0x{-_T$6+O6)KFFK%O z80HS`Ym~F8Y_ove7GSILT^j{v9W1yRy5(z$9KR) zdN=7y+AY;l&^JCJr( let mut new_child_map = HashMap::new(); let mut node_changed_map = Vec::new(); + // Tracks whether any stale default-valued properties were removed from + // project nodes. If so, we must reserialize even if + // project_node_should_reserialize wouldn't otherwise detect a change + // (it only compares node properties forward, not in reverse). + let mut removed_stale_properties = false; let mut node_queue = VecDeque::with_capacity(1); node_queue.push_back((&mut project.tree, old_inst, snapshot.new_inst())); @@ -402,10 +407,12 @@ pub fn syncback_project<'sync>( // We only want to set properties if it needs it. if !middleware.handles_own_properties() { - project_node_property_syncback_path(snapshot, new_inst, node); + removed_stale_properties |= + project_node_property_syncback_path(snapshot, new_inst, node); } } else { - project_node_property_syncback_no_path(snapshot, new_inst, node); + removed_stale_properties |= + project_node_property_syncback_no_path(snapshot, new_inst, node); } for child_ref in new_inst.children() { @@ -507,12 +514,18 @@ pub fn syncback_project<'sync>( } let mut fs_snapshot = FsSnapshot::new(); - for (node_properties, node_attributes, old_inst) in node_changed_map { - if project_node_should_reserialize(node_properties, node_attributes, old_inst)? { - fs_snapshot.add_file(project_path, serde_json::to_vec_pretty(&project)?); - break; + let mut needs_reserialize = removed_stale_properties; + if !needs_reserialize { + for (node_properties, node_attributes, old_inst) in node_changed_map { + if project_node_should_reserialize(node_properties, node_attributes, old_inst)? { + needs_reserialize = true; + break; + } } } + if needs_reserialize { + fs_snapshot.add_file(project_path, serde_json::to_vec_pretty(&project)?); + } Ok(SyncbackReturn { fs_snapshot, @@ -521,15 +534,18 @@ pub fn syncback_project<'sync>( }) } +/// Syncs properties from the new instance into the project node. +/// Returns `true` if any stale properties were removed (i.e. properties +/// that existed in the project node but are now at their engine default). fn project_node_property_syncback( _snapshot: &SyncbackSnapshot, filtered_properties: UstrMap<&Variant>, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let properties = &mut node.properties; let mut attributes = BTreeMap::new(); - for (name, value) in filtered_properties { + for (&name, &value) in &filtered_properties { match value { Variant::Attributes(attrs) => { for (attr_name, attr_value) in attrs.iter() { @@ -552,14 +568,48 @@ fn project_node_property_syncback( } } } + + // Remove stale properties: entries that exist in the project node's + // $properties but are no longer in the filtered (non-default) properties + // from the instance. This handles the case where Studio resets a property + // to its engine default — filter_properties won't include it, so we need + // to clean up the now-stale project entry. + let class_data = rbx_reflection_database::get() + .ok() + .and_then(|db| db.classes.get(new_inst.class.as_str())); + let len_before = properties.len(); + properties.retain(|prop_name, _| { + if filtered_properties.contains_key(prop_name) { + return true; + } + // Only remove if the property has a known default value in the + // reflection database. If there's no default, the property might be + // absent from the instance for other reasons (e.g. unknown property), + // so we conservatively keep it. + if let Some(data) = &class_data { + if data.default_properties.contains_key(prop_name.as_str()) { + log::debug!( + "Removing stale property '{}' from project node for class '{}': \ + value has been reset to engine default", + prop_name, + new_inst.class + ); + return false; + } + } + true + }); + let removed_stale = properties.len() < len_before; + node.attributes = attributes; + removed_stale } fn project_node_property_syncback_path( snapshot: &SyncbackSnapshot, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let filtered_properties = snapshot .get_path_filtered_properties(new_inst.referent()) .unwrap(); @@ -570,7 +620,7 @@ fn project_node_property_syncback_no_path( snapshot: &SyncbackSnapshot, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let filtered_properties = filter_properties(snapshot.project(), new_inst); project_node_property_syncback(snapshot, filtered_properties, new_inst, node) } diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 31553496..cffccccb 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -86,4 +86,7 @@ syncback_tests! { sync_rules => ["src/module.modulescript", "src/text.text"], // Ensures that the `syncUnscriptable` setting works unscriptable_properties => ["default.project.json"], + // Ensures that syncback correctly removes default values from projects rather + // than leaving an incorrect value. + project_default_properties_remove => ["default.project.json"], } From 16633e3c658e9b94edaffa3a80cca46268c39b5c Mon Sep 17 00:00:00 2001 From: AK-Khan02 Date: Wed, 10 Jun 2026 21:48:33 -0400 Subject: [PATCH 4/4] Return 400 for missing /api/serialize IDs (#1272) --- CHANGELOG.md | 2 ++ src/web/api.rs | 2 +- tests/rojo_test/serve_util.rs | 10 ++++------ tests/tests/serve.rs | 36 +++++++++++++++++++++++++++++------ 4 files changed, 37 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00524693..339c0c6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ Making a new release? Simply add the new header with the version and date undern * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265]) * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267]) +* Fixed `/api/serialize` returning success when a requested instance ID is missing from the serve session tree. ([#1272]) * `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. The accepted hosts can be extended with the `--allowed-hosts` option or a project's `serveAllowedHosts` field, for example to reach a network-exposed server by hostname. ([#1270]) * Fixed syncback not removing stale `$properties` entries when Studio resets a property to its engine default. ([#1244]) @@ -60,6 +61,7 @@ Making a new release? Simply add the new header with the version and date undern [#1265]: https://github.com/rojo-rbx/rojo/pull/1265 [#1266]: https://github.com/rojo-rbx/rojo/pull/1266 [#1267]: https://github.com/rojo-rbx/rojo/pull/1267 +[#1272]: https://github.com/rojo-rbx/rojo/pull/1272 [#1270]: https://github.com/rojo-rbx/rojo/pull/1270 [#1244]: https://github.com/rojo-rbx/rojo/pull/1244 diff --git a/src/web/api.rs b/src/web/api.rs index 2875f2aa..4fd21c0a 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -276,7 +276,7 @@ impl ApiService { response_dom.transfer_within(child_ref, object_value); } else { - msgpack( + return msgpack( ErrorResponse::bad_request(format!("provided id {id} is not in the tree")), StatusCode::BAD_REQUEST, ); diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 4a817a8a..458743e0 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -228,11 +228,11 @@ impl TestServeSession { } } - pub fn get_api_serialize( + pub fn post_api_serialize( &self, ids: &[Ref], session_id: SessionId, - ) -> Result { + ) -> Result { let client = reqwest::blocking::Client::new(); let url = format!("http://localhost:{}/api/serialize", self.port); let body = serialize_msgpack(&SerializeRequest { @@ -241,9 +241,7 @@ impl TestServeSession { }) .unwrap(); - let body = client.post(url).body(body).send()?.bytes()?; - - Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) + client.post(url).body(body).send() } /// Sends a GET to `/api/rojo` with the given extra request headers and @@ -291,7 +289,7 @@ fn serialize_msgpack(value: T) -> Result, rmp_serde::encod Ok(serialized) } -fn deserialize_msgpack<'a, T: Deserialize<'a>>( +pub fn deserialize_msgpack<'a, T: Deserialize<'a>>( input: &'a [u8], ) -> Result { let mut deserializer = rmp_serde::Deserializer::new(input).with_human_readable(); diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 490c13db..0d6f75cc 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -1,14 +1,16 @@ use std::fs; use insta::{assert_snapshot, assert_yaml_snapshot, with_settings}; +use rbx_dom_weak::types::Ref; +use reqwest::StatusCode; use tempfile::tempdir; use crate::rojo_test::{ internable::InternAndRedact, - serve_util::{run_serve_test, serialize_to_xml_model}, + serve_util::{deserialize_msgpack, run_serve_test, serialize_to_xml_model}, }; -use librojo::web_api::SocketPacketType; +use librojo::web_api::{SerializeResponse, SocketPacketType}; #[test] fn rejects_dns_rebinding_requests() { @@ -713,9 +715,13 @@ fn meshpart_with_id() { .find(|(_, inst)| inst.class_name == "ObjectValue") .unwrap(); - let serialize_response = session - .get_api_serialize(&[*meshpart, *objectvalue], info.session_id) + let body = session + .post_api_serialize(&[*meshpart, *objectvalue], info.session_id) + .unwrap() + .bytes() .unwrap(); + let serialize_response: SerializeResponse = + deserialize_msgpack(&body).expect("Server returned malformed response"); // We don't assert a snapshot on the SerializeResponse because the model includes the // Refs from the DOM as names, which means it will obviously be different every time @@ -727,6 +733,20 @@ fn meshpart_with_id() { }); } +#[test] +fn serialize_missing_id() { + run_serve_test("empty", |session, _| { + let info = session.get_api_rojo().unwrap(); + let missing_id = Ref::new(); + + let response = session + .post_api_serialize(&[missing_id], info.session_id) + .unwrap(); + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + }); +} + #[test] fn forced_parent() { run_serve_test("forced_parent", |session, mut redactions| { @@ -741,9 +761,13 @@ fn forced_parent() { read_response.intern_and_redact(&mut redactions, root_id) ); - let serialize_response = session - .get_api_serialize(&[root_id], info.session_id) + let body = session + .post_api_serialize(&[root_id], info.session_id) + .unwrap() + .bytes() .unwrap(); + let serialize_response: SerializeResponse = + deserialize_msgpack(&body).expect("Server returned malformed response"); assert_eq!(serialize_response.session_id, info.session_id);