diff --git a/CHANGELOG.md b/CHANGELOG.md index d042de3a..00524693 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ 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]) +* 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 @@ -49,6 +55,12 @@ 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 +[#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/CONTRIBUTING.md b/CONTRIBUTING.md index 0063e448..bf11ae72 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,12 +13,27 @@ Code contributions are welcome for features and bugs that have been reported in You'll want these tools to work on Rojo: -* Latest stable Rust compiler +* Rust 1.88 or newer * Rustfmt and Clippy are used for code formatting and linting. -* Latest stable [Rojo](https://github.com/rojo-rbx/rojo) * [Rokit](https://github.com/rojo-rbx/rokit) * [Luau Language Server](https://github.com/JohnnyMorganz/luau-lsp) (Only needed if working on the Studio plugin.) +Rokit installs the pinned Rojo, Selene, StyLua, Lune, and run-in-roblox versions listed in [`rokit.toml`](rokit.toml): + +```bash +rokit install +``` + +Before opening a pull request, run the relevant checks: + +```bash +cargo test +cargo fmt -- --check +cargo clippy +stylua --check plugin/src +selene plugin/src +``` + When working on the Studio plugin, we recommend using this command to automatically rebuild the plugin when you save a change: *(Make sure you've enabled the Studio setting to reload plugins on file change!)* @@ -29,7 +44,7 @@ bash scripts/watch-build-plugin.sh You can also run the plugin's unit tests with the following: -*(Make sure you have `run-in-roblox` installed first!)* +*(If you are not using Rokit, make sure you have `run-in-roblox` installed first!)* ```bash bash scripts/unit-test-plugin.sh @@ -49,27 +64,27 @@ Please file issues and we'll try to help figure out what the best way forward is ## Local Development Gotchas -If your build fails with "Error: failed to open file `D:\code\rojo\plugin\modules\roact\src`" you need to update your Git submodules. +If your build fails with an error about a missing path under `plugin/Packages`, such as `plugin/Packages/Roact`, you need to update your Git submodules. Run the command and try building again: `git submodule update --init --recursive`. ## Pushing a Rojo Release -The Rojo release process is pretty manual right now. If you need to do it, here's how: +The Rojo release process is driven by the GitHub Actions release workflow. If you need to do it, here's how: 1. Bump server version in [`Cargo.toml`](Cargo.toml) -2. Bump plugin version in [`plugin/src/Config.lua`](plugin/src/Config.lua) -3. Run `cargo test` to update `Cargo.lock` and run tests +2. Bump plugin version in [`plugin/Version.txt`](plugin/Version.txt) + * The build checks that the Cargo and plugin versions match. +3. Run `cargo test` to update `Cargo.lock` after the version bump and run tests 4. Update [`CHANGELOG.md`](CHANGELOG.md) 5. Commit! * `git add . && git commit -m "Release vX.Y.Z"` 6. Tag the commit * `git tag vX.Y.Z` -7. Publish the CLI - * `cargo publish` -8. Publish the Plugin - * `cargo run -- upload plugin --asset_id 6415005344` -9. Push commits and tags +7. Push commits and tags * `git push && git push --tags` +8. Wait for the GitHub Actions release workflow to create the draft release and upload CLI/plugin artifacts +9. Publish the CLI crate + * `cargo publish` 10. Copy GitHub release content from previous release * Update the leading text with a summary about the release * Paste the changelog notes (as-is!) from [`CHANGELOG.md`](CHANGELOG.md) - * Write a small summary of each major feature \ No newline at end of file + * Write a small summary of each major feature diff --git a/README.md b/README.md index c150ecb7..f3d1aeeb 100644 --- a/README.md +++ b/README.md @@ -25,12 +25,11 @@ Rojo enables: * Versioning your game, library, or plugin using Git or another VCS * Streaming `rbxmx` and `rbxm` models into your game in real time * Packaging and deploying your project to Roblox.com from the command line +* Pulling Instances from Roblox place and model files back into an existing Rojo project with `rojo syncback` -In the future, Rojo will be able to: +Rojo also has an optional two-way sync setting in the Studio plugin for syncing supported Studio edits back to the filesystem. -* Sync instances from Roblox Studio to the filesystem -* Automatically convert your existing game to work with Rojo -* Import custom instances like MoonScript code +Some workflows, like fully automatic conversion of every existing game into a Rojo project, are still limited and may require manual project configuration. ## [Documentation](https://rojo.space/docs) Documentation is hosted in the [rojo.space repository](https://github.com/rojo-rbx/rojo.space). diff --git a/crates/memofs/CHANGELOG.md b/crates/memofs/CHANGELOG.md index c35b5399..ec0b71f0 100644 --- a/crates/memofs/CHANGELOG.md +++ b/crates/memofs/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased Changes * Added `Vfs::canonicalize`. [#1201] +* **Breaking:** `StdBackend::new` and `Vfs::new_default` now return `io::Result`, so a failure to create the filesystem watcher is reported as an error instead of panicking. The `Default` implementation for `StdBackend` has been removed as a result. [#1267] + +[#1267]: https://github.com/rojo-rbx/rojo/pull/1267 ## 0.3.1 (2025-11-27) * Added `Vfs::exists`. [#1169] diff --git a/crates/memofs/src/lib.rs b/crates/memofs/src/lib.rs index 41d21100..11acf810 100644 --- a/crates/memofs/src/lib.rs +++ b/crates/memofs/src/lib.rs @@ -255,8 +255,11 @@ pub struct Vfs { impl Vfs { /// Creates a new `Vfs` with the default backend, `StdBackend`. - pub fn new_default() -> Self { - Self::new(StdBackend::new()) + /// + /// Returns an error if the filesystem watcher could not be initialized, + /// which can happen in restricted or sandboxed environments. + pub fn new_default() -> io::Result { + Ok(Self::new(StdBackend::new()?)) } /// Creates a new `Vfs` with the given backend. @@ -639,7 +642,7 @@ mod test { let file_path = dir.path().join("file.txt"); fs_err::write(&file_path, contents.to_string()).unwrap(); - let vfs = Vfs::new(StdBackend::new()); + let vfs = Vfs::new(StdBackend::new().unwrap()); let canonicalized = vfs.canonicalize(&file_path).unwrap(); assert_eq!(canonicalized, file_path.canonicalize().unwrap()); assert_eq!( @@ -653,7 +656,7 @@ mod test { let dir = tempfile::tempdir().unwrap(); let file_path = dir.path().join("test"); - let vfs = Vfs::new(StdBackend::new()); + let vfs = Vfs::new(StdBackend::new().unwrap()); let err = vfs.canonicalize(&file_path).unwrap_err(); assert_eq!(err.kind(), io::ErrorKind::NotFound); } diff --git a/crates/memofs/src/std_backend.rs b/crates/memofs/src/std_backend.rs index a25fa39d..d684bde5 100644 --- a/crates/memofs/src/std_backend.rs +++ b/crates/memofs/src/std_backend.rs @@ -17,9 +17,9 @@ pub struct StdBackend { } impl StdBackend { - pub fn new() -> StdBackend { + pub fn new() -> io::Result { let (notify_tx, notify_rx) = mpsc::channel(); - let watcher = watcher(notify_tx, Duration::from_millis(50)).unwrap(); + let watcher = watcher(notify_tx, Duration::from_millis(50)).map_err(io::Error::other)?; let (tx, rx) = crossbeam_channel::unbounded(); @@ -46,11 +46,11 @@ impl StdBackend { Result::<(), crossbeam_channel::SendError>::Ok(()) }); - Self { + Ok(Self { watcher, watcher_receiver: rx, watches: HashSet::new(), - } + }) } } @@ -134,9 +134,3 @@ impl VfsBackend for StdBackend { self.watcher.unwatch(path).map_err(io::Error::other) } } - -impl Default for StdBackend { - fn default() -> Self { - Self::new() - } -} diff --git a/plugin/http/init.lua b/plugin/http/init.lua index 106ebc45..3d05c6d3 100644 --- a/plugin/http/init.lua +++ b/plugin/http/init.lua @@ -14,6 +14,13 @@ local Http = {} Http.Error = HttpError Http.Response = HttpResponse +-- Monkey patch msgpack.UInt64.new to lossily convert the low and high bits of the integer +-- to a native Luau number. We should change the upstream decoder to emit a native +-- integer, once those are live. +function msgpack.UInt64.new(mostSignificantPart: number, leastSignificantPart: number): number + return (mostSignificantPart % 2 ^ 32) * 2 ^ 32 + (leastSignificantPart % 2 ^ 32) +end + local function performRequest(requestParams) local requestId = lastRequestId + 1 lastRequestId = requestId diff --git a/plugin/run-tests.server.lua b/plugin/run-tests.server.lua index 0ab3cc05..aa5daa9f 100644 --- a/plugin/run-tests.server.lua +++ b/plugin/run-tests.server.lua @@ -1,8 +1,8 @@ local ReplicatedStorage = game:GetService("ReplicatedStorage") -local TestEZ = require(ReplicatedStorage.Packages:WaitForChild("TestEZ", 10)) +local TestEZ = require(ReplicatedStorage:WaitForChild("Packages", 10):WaitForChild("TestEZ", 10)) -local Rojo = ReplicatedStorage.Rojo +local Rojo = ReplicatedStorage:WaitForChild("Rojo", 10) local Settings = require(Rojo.Plugin.Settings) Settings:set("logLevel", "Trace") diff --git a/plugin/src/Reconciler/countMatchingProperties.lua b/plugin/src/Reconciler/countMatchingProperties.lua new file mode 100644 index 00000000..0ddfdcc3 --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.lua @@ -0,0 +1,44 @@ +--[[ + Counts how many of a virtual instance's properties match the live values on a + candidate Roblox instance. `hydrate` uses this to break ties when several + existing children share the same Name and ClassName. + + This mirrors the read -> decode -> compare flow that `diff` uses, reusing the + same `getProperty`, `decodeValue`, and `trueEquals` helpers. +]] + +local getProperty = require(script.Parent.getProperty) +local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) + +local function countMatchingProperties(instance, virtualInstance, instanceMap) + local score = 0 + + for propertyName, virtualValue in virtualInstance.Properties do + -- Skip refs. During hydration the instanceMap is still being built + -- top-down, so a ref may point at an instance we haven't hydrated yet + -- and therefore can't decode reliably. Refs are also a poor + -- disambiguator between same-named siblings. + if next(virtualValue) == "Ref" then + continue + end + + local getSuccess, existingValue = getProperty(instance, propertyName) + if not getSuccess then + continue + end + + local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) + if not decodeSuccess then + continue + end + + if trueEquals(existingValue, decodedValue) then + score += 1 + end + end + + return score +end + +return countMatchingProperties diff --git a/plugin/src/Reconciler/countMatchingProperties.spec.lua b/plugin/src/Reconciler/countMatchingProperties.spec.lua new file mode 100644 index 00000000..3d6bb18d --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.spec.lua @@ -0,0 +1,91 @@ +return function() + local countMatchingProperties = require(script.Parent.countMatchingProperties) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + + it("counts properties whose values match the instance", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "hello" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("does not count properties whose values differ", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "different" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("counts multiple matching properties independently", function() + local instance = Instance.new("Part") + instance.Anchored = true + instance.CanCollide = false + + local virtualInstance = { + ClassName = "Part", + Name = "Part", + Properties = { + Anchored = { Bool = true }, + CanCollide = { Bool = false }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(2) + + -- Flip one so only a single property matches. + instance.CanCollide = true + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("skips unknown properties without counting or erroring", function() + local instance = Instance.new("Folder") + + local virtualInstance = { + ClassName = "Folder", + Name = "Folder", + Properties = { + FAKE_PROPERTY = { String = "nope" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("skips Ref properties without counting or erroring", function() + local instance = Instance.new("ObjectValue") + + local virtualInstance = { + ClassName = "ObjectValue", + Name = "ObjectValue", + Properties = { + -- A ref must be skipped rather than decoded: during hydration + -- the target may not be in the map yet. + Value = { Ref = "00000000000000000000000000000000" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) +end diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index c8da0939..73d41236 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -10,104 +10,12 @@ local invariant = require(script.Parent.Parent.invariant) local getProperty = require(script.Parent.getProperty) local Error = require(script.Parent.Error) local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) local function isEmpty(table) return next(table) == nil end -local function fuzzyEq(a: number, b: number, epsilon: number): boolean - return math.abs(a - b) < epsilon -end - -local function trueEquals(a, b): boolean - -- Exit early for simple equality values - if a == b then - return true - end - - -- Treat nil and { Ref = "000...0" } as equal - if - (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") - or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") - then - return true - end - - local typeA, typeB = typeof(a), typeof(b) - - -- For tables, try recursive deep equality - if typeA == "table" and typeB == "table" then - local checkedKeys = {} - for key, value in a do - checkedKeys[key] = true - if not trueEquals(value, b[key]) then - return false - end - end - for key, value in b do - if checkedKeys[key] then - continue - end - if not trueEquals(value, a[key]) then - return false - end - end - return true - - -- For NaN, check if both values are not equal to themselves - elseif a ~= a and b ~= b then - return true - - -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "number" and typeB == "number" then - return fuzzyEq(a, b, 0.0001) - - -- For EnumItem->number, compare the EnumItem's value - elseif typeA == "number" and typeB == "EnumItem" then - return a == b.Value - elseif typeA == "EnumItem" and typeB == "number" then - return a.Value == b - - -- For Color3s, compare to RGB ints to avoid floating point inequality - elseif typeA == "Color3" and typeB == "Color3" then - local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) - local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) - return aR == bR and aG == bG and aB == bB - - -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "CFrame" and typeB == "CFrame" then - local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector3" and typeB == "Vector3" then - local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector2" and typeB == "Vector2" then - local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - end - - return false -end - local function shouldDeleteUnknownInstances(virtualInstance) if virtualInstance.Metadata ~= nil then return not virtualInstance.Metadata.ignoreUnknownInstances diff --git a/plugin/src/Reconciler/hydrate.lua b/plugin/src/Reconciler/hydrate.lua index d7798160..fdd6c4a2 100644 --- a/plugin/src/Reconciler/hydrate.lua +++ b/plugin/src/Reconciler/hydrate.lua @@ -3,9 +3,22 @@ concrete instances and assigning them IDs. ]] -local invariant = require(script.Parent.Parent.invariant) +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) -local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) +local invariant = require(script.Parent.Parent.invariant) +local countMatchingProperties = require(script.Parent.countMatchingProperties) + +-- When several existing children share a Name and ClassName we disambiguate +-- them by scoring how well each one's properties match the virtual instance. +-- That scoring is far more expensive than a Name/ClassName check, so we only do +-- it for reasonably-sized groups. Larger groups (e.g. a folder with thousands of +-- identically-named parts) fall back to the original order-based matching, which +-- bounds the added work to roughly MAX_CANDIDATES_TO_SCORE^2 property reads per +-- group regardless of how large the group is. +local MAX_CANDIDATES_TO_SCORE = 32 + +local function hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) local virtualInstance = virtualInstances[rootId] if virtualInstance == nil then @@ -13,38 +26,163 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) end instanceMap:insert(rootId, rootInstance) + stats.hydrated += 1 local existingChildren = rootInstance:GetChildren() - -- For each existing child, we'll track whether it's been paired with an - -- instance that the Rojo server knows about. - local isExistingChildVisited = {} - for i = 1, #existingChildren do - isExistingChildVisited[i] = false + -- Group existing children by Name then ClassName so each virtual child can + -- find its candidate matches without scanning every sibling. This is what + -- keeps hydration fast for parents with thousands of children. Nesting the + -- two tables (rather than a combined key) keeps the Name and ClassName checks + -- exact, with no way for one to bleed into the other. + local buckets = {} + for _, childInstance in existingChildren do + -- We guard accessing Name and ClassName in order to avoid tripping over + -- children of DataModel that Rojo won't have permissions to access at all. + local accessSuccess, name, className = pcall(function() + return childInstance.Name, childInstance.ClassName + end) + if not accessSuccess then + continue + end + + local bucketsByClassName = buckets[name] + if bucketsByClassName == nil then + bucketsByClassName = {} + buckets[name] = bucketsByClassName + end + + local bucket = bucketsByClassName[className] + if bucket == nil then + bucket = { cursor = 1, instances = {} } + bucketsByClassName[className] = bucket + end + + table.insert(bucket.instances, childInstance) end + -- Tracks which existing children have already been paired, so one instance + -- isn't matched to two different virtual instances. + local visited = {} + for _, childId in ipairs(virtualInstance.Children) do local virtualChild = virtualInstances[childId] - for childIndex, childInstance in existingChildren do - if not isExistingChildVisited[childIndex] then - -- We guard accessing Name and ClassName in order to avoid - -- tripping over children of DataModel that Rojo won't have - -- permissions to access at all. - local accessSuccess, name, className = pcall(function() - return childInstance.Name, childInstance.ClassName - end) + local bucketsByClassName = buckets[virtualChild.Name] + local bucket = bucketsByClassName and bucketsByClassName[virtualChild.ClassName] + if bucket == nil then + -- No existing instance matches; diff will mark this id for creation. + Log.trace( + "hydrate: no existing instance matches {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end - -- This rule is very conservative and could be loosened in the - -- future, or more heuristics could be introduced. - if accessSuccess and name == virtualChild.Name and className == virtualChild.ClassName then - isExistingChildVisited[childIndex] = true - hydrate(instanceMap, virtualInstances, childId, childInstance) - break + local instances = bucket.instances + + -- Advance past any leading children that have already been paired. The + -- cursor makes order-based matching amortized O(1) per child even for + -- very large groups, rather than rescanning the visited prefix. + while bucket.cursor <= #instances and visited[instances[bucket.cursor]] do + bucket.cursor += 1 + end + if bucket.cursor > #instances then + -- Every matching instance has already been paired with an earlier id. + Log.trace( + "hydrate: no unpaired instance left for {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end + + -- The cursor points at the earliest unvisited child, so the slots from + -- here to the end bound how many candidates remain. Visited children + -- after the cursor (gaps) only appear once a group is small enough to be + -- scored -- the order-based path below always takes the earliest, which + -- keeps the visited region a contiguous prefix. So whenever this count + -- exceeds the cap it is exact, and we can pick the earliest match without + -- collecting anything. + local remaining = #instances - bucket.cursor + 1 + + local match + if remaining > MAX_CANDIDATES_TO_SCORE then + -- Too many to score affordably; take the earliest in child order, + -- reproducing the original Name + ClassName behavior. + match = instances[bucket.cursor] + Log.trace( + "hydrate: {} candidates named {} ({}) exceeds the scoring cap of {}; matching id {} by child order", + remaining, + virtualChild.Name, + virtualChild.ClassName, + MAX_CANDIDATES_TO_SCORE, + childId + ) + else + -- Collect the (at most `remaining`) unvisited candidates. + local candidates = {} + for index = bucket.cursor, #instances do + local childInstance = instances[index] + if not visited[childInstance] then + table.insert(candidates, childInstance) end end + + if #candidates == 1 then + -- Only one candidate, so there's nothing to disambiguate. + match = candidates[1] + else + -- Break the tie by choosing the candidate whose properties best + -- match the virtual instance, falling back to the earliest in + -- child order when scores are equal. + local bestScore = -1 + for _, childInstance in candidates do + local score = countMatchingProperties(childInstance, virtualChild, instanceMap) + if score > bestScore then + bestScore = score + match = childInstance + end + end + + stats.ambiguousGroups += 1 + stats.candidatesScored += #candidates + Log.trace( + "hydrate: disambiguated {} candidates named {} ({}) for id {} by property match (best score {})", + #candidates, + virtualChild.Name, + virtualChild.ClassName, + childId, + bestScore + ) + end end + + visited[match] = true + hydrateInner(stats, instanceMap, virtualInstances, childId, match) end end +local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) + -- Tallies of the work hydration did, surfaced in a single debug log below so + -- the cost of property-based disambiguation is visible without per-node spam. + local stats = { + hydrated = 0, + ambiguousGroups = 0, + candidatesScored = 0, + } + + hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) + + Log.debug( + "Hydrated {} instances ({} ambiguous name+class groups, {} candidates scored)", + stats.hydrated, + stats.ambiguousGroups, + stats.candidatesScored + ) +end + return hydrate diff --git a/plugin/src/Reconciler/hydrate.spec.lua b/plugin/src/Reconciler/hydrate.spec.lua index 226d37f3..7f6fea7c 100644 --- a/plugin/src/Reconciler/hydrate.spec.lua +++ b/plugin/src/Reconciler/hydrate.spec.lua @@ -126,4 +126,140 @@ return function() expect(knownInstances.fromIds["CHILD1"]).to.equal(child1) expect(knownInstances.fromIds["CHILD2"]).to.equal(child2) end) + + it("should disambiguate duplicate-named siblings by matching properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "first" } }, + Children = {}, + }, + + CHILD_B = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "second" } }, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + -- Created in the reverse order of the virtual children, so a purely + -- order-based tiebreak would mis-pair them. + local child1 = Instance.new("StringValue") + child1.Name = "a" + child1.Value = "second" + child1.Parent = rootInstance + + local child2 = Instance.new("StringValue") + child2.Name = "a" + child2.Value = "first" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child2) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child1) + end) + + it("should fall back to child order for duplicate-named siblings with no distinguishing properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + + CHILD_B = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local child1 = Instance.new("Folder") + child1.Name = "a" + child1.Parent = rootInstance + + local child2 = Instance.new("Folder") + child2.Name = "a" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + -- With equal scores the earliest unvisited child wins, preserving the + -- original order-based behavior. + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child1) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child2) + end) + + it("should fall back to child order for very large duplicate-named groups", function() + -- More candidates than hydrate is willing to score at once. The group + -- must fall back to order-based matching, so virtual child N pairs with + -- existing child N regardless of properties. + local count = 64 + + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local expectedInstances = {} + for i = 1, count do + local id = "CHILD_" .. i + table.insert(virtualInstances.ROOT.Children, id) + virtualInstances[id] = { + ClassName = "StringValue", + Name = "a", + -- Distinct values that, if scored, would pair by value rather + -- than by order. + Properties = { Value = { String = "value " .. i } }, + Children = {}, + } + + local child = Instance.new("StringValue") + child.Name = "a" + child.Value = "value " .. (count - i + 1) + child.Parent = rootInstance + expectedInstances[id] = child + end + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(count + 1) + for id, expectedInstance in expectedInstances do + expect(knownInstances.fromIds[id]).to.equal(expectedInstance) + end + end) end diff --git a/plugin/src/Reconciler/trueEquals.lua b/plugin/src/Reconciler/trueEquals.lua new file mode 100644 index 00000000..7dadf476 --- /dev/null +++ b/plugin/src/Reconciler/trueEquals.lua @@ -0,0 +1,100 @@ +--[[ + Fuzzy value-equality used to compare a decoded virtual property value against + the live value read from a real instance. Shared by `diff` (to decide whether + a property changed) and `hydrate` (to score candidate instances). +]] + +local function fuzzyEq(a: number, b: number, epsilon: number): boolean + return math.abs(a - b) < epsilon +end + +local function trueEquals(a, b): boolean + -- Exit early for simple equality values + if a == b then + return true + end + + -- Treat nil and { Ref = "000...0" } as equal + if + (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") + or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") + then + return true + end + + local typeA, typeB = typeof(a), typeof(b) + + -- For tables, try recursive deep equality + if typeA == "table" and typeB == "table" then + local checkedKeys = {} + for key, value in a do + checkedKeys[key] = true + if not trueEquals(value, b[key]) then + return false + end + end + for key, value in b do + if checkedKeys[key] then + continue + end + if not trueEquals(value, a[key]) then + return false + end + end + return true + + -- For NaN, check if both values are not equal to themselves + elseif a ~= a and b ~= b then + return true + + -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "number" and typeB == "number" then + return fuzzyEq(a, b, 0.0001) + + -- For EnumItem->number, compare the EnumItem's value + elseif typeA == "number" and typeB == "EnumItem" then + return a == b.Value + elseif typeA == "EnumItem" and typeB == "number" then + return a.Value == b + + -- For Color3s, compare to RGB ints to avoid floating point inequality + elseif typeA == "Color3" and typeB == "Color3" then + local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) + local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) + return aR == bR and aG == bG and aB == bB + + -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "CFrame" and typeB == "CFrame" then + local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector3" and typeB == "Vector3" then + local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector2" and typeB == "Vector2" then + local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + end + + return false +end + +return trueEquals diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index adee9e4d..7e9fc742 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -18,6 +18,7 @@ local PatchSet = require(script.Parent.PatchSet) local Reconciler = require(script.Parent.Reconciler) local strict = require(script.Parent.strict) local Settings = require(script.Parent.Settings) +local orderSwaps = require(script.Parent.orderSwaps) local Status = strict("Session.Status", { NotStarted = "NotStarted", @@ -320,6 +321,14 @@ function ServeSession:__replaceInstances(idList) return false end + -- Roblox appends to GetChildren() on every reparent, so the order in which + -- we re-parent replacements determines their final sibling order. + -- We process ancestors before descendants (so each replacement's + -- parent already exists when we re-parent it) and siblings in their original + -- GetChildren() order. Because the loop below moves the old instance's + -- children into the replacement *before* re-parenting the replacement, this + -- rebuilds GetChildren() exactly as it was before the swap. + local swaps = {} for id, replacement in replacements do local oldInstance = self.__instanceMap.fromIds[id] if not oldInstance then @@ -328,6 +337,16 @@ function ServeSession:__replaceInstances(idList) continue end + table.insert(swaps, { + id = id, + replacement = replacement, + oldInstance = oldInstance, + }) + end + + for _, swap in orderSwaps(swaps) do + local id, replacement, oldInstance = swap.id, swap.replacement, swap.oldInstance + self.__instanceMap:insert(id, replacement) Log.trace("Swapping Instance {} out via api/models/ endpoint", id) local oldParent = oldInstance.Parent diff --git a/plugin/src/orderSwaps.lua b/plugin/src/orderSwaps.lua new file mode 100644 index 00000000..aa4a438e --- /dev/null +++ b/plugin/src/orderSwaps.lua @@ -0,0 +1,44 @@ +--[[ + Determines the order in which `ServeSession:__replaceInstances` should swap + instances so that sibling order is preserved. + + Roblox appends to `GetChildren()` on every reparent, so the order in which we + re-parent replacements determines their final sibling order. To rebuild + `GetChildren()` exactly as it was before the swap we must: + + * process ancestors before descendants, so each replacement's parent already + exists when we re-parent the replacement, and + * process siblings in their original `GetChildren()` order. + + `swaps` is an array of `{ id, replacement, oldInstance }` entries. This sorts + the array in place (annotating each entry with `depth`/`siblingIndex`) and + returns it. +]] +local function orderSwaps(swaps) + for _, swap in swaps do + local depth = 0 + local ancestor = swap.oldInstance.Parent + while ancestor ~= nil do + depth += 1 + ancestor = ancestor.Parent + end + swap.depth = depth + + local siblingIndex = 0 + if swap.oldInstance.Parent ~= nil then + siblingIndex = table.find(swap.oldInstance.Parent:GetChildren(), swap.oldInstance) or 0 + end + swap.siblingIndex = siblingIndex + end + + table.sort(swaps, function(a, b) + if a.depth ~= b.depth then + return a.depth < b.depth + end + return a.siblingIndex < b.siblingIndex + end) + + return swaps +end + +return orderSwaps diff --git a/plugin/src/orderSwaps.spec.lua b/plugin/src/orderSwaps.spec.lua new file mode 100644 index 00000000..04c730fd --- /dev/null +++ b/plugin/src/orderSwaps.spec.lua @@ -0,0 +1,57 @@ +return function() + local orderSwaps = require(script.Parent.orderSwaps) + + it("orders same-named siblings by their original GetChildren order", function() + local parent = Instance.new("Model") + local a1 = Instance.new("Part") + a1.Name = "a" + a1.Parent = parent + local a2 = Instance.new("Part") + a2.Name = "a" + a2.Parent = parent + local a3 = Instance.new("Part") + a3.Name = "a" + a3.Parent = parent + + -- Input deliberately out of sibling order. + -- orderSwaps must restore the GetChildren() order. + local ordered = orderSwaps({ + { id = "3", oldInstance = a3 }, + { id = "1", oldInstance = a1 }, + { id = "2", oldInstance = a2 }, + }) + + expect(ordered[1].oldInstance).to.equal(a1) + expect(ordered[2].oldInstance).to.equal(a2) + expect(ordered[3].oldInstance).to.equal(a3) + end) + + it("orders ancestors before descendants", function() + local root = Instance.new("Model") + local child = Instance.new("Folder") + child.Parent = root + local grandchild = Instance.new("Part") + grandchild.Parent = child + + local ordered = orderSwaps({ + { id = "grandchild", oldInstance = grandchild }, + { id = "child", oldInstance = child }, + { id = "root", oldInstance = root }, + }) + + expect(ordered[1].oldInstance).to.equal(root) + expect(ordered[2].oldInstance).to.equal(child) + expect(ordered[3].oldInstance).to.equal(grandchild) + end) + + it("returns a single swap unchanged", function() + local part = Instance.new("Part") + + local ordered = orderSwaps({ + { id = "1", oldInstance = part }, + }) + + expect(#ordered).to.equal(1) + expect(ordered[1].oldInstance).to.equal(part) + end) +end 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/change_processor.rs b/src/change_processor.rs index 6c5c8224..c1f245f2 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -200,7 +200,15 @@ impl JobThreadContext { if let Some(instance) = tree.get_instance(id) { if let Some(instigating_source) = &instance.metadata().instigating_source { match instigating_source { - InstigatingSource::Path(path) => fs::remove_file(path).unwrap(), + InstigatingSource::Path(path) => { + if let Err(err) = fs::remove_file(path) { + log::error!( + "Failed to remove file {}: {}", + path.display(), + err + ); + } + } InstigatingSource::ProjectNode { .. } => { log::warn!( "Cannot remove instance {:?}, it's from a project file", @@ -244,7 +252,13 @@ impl JobThreadContext { match instigating_source { InstigatingSource::Path(path) => { if let Some(Variant::String(value)) = changed_value { - fs::write(path, value).unwrap(); + if let Err(err) = fs::write(path, value) { + log::error!( + "Failed to write file {}: {}", + path.display(), + err + ); + } } else { log::warn!("Cannot change Source to non-string value."); } diff --git a/src/cli/build.rs b/src/cli/build.rs index c2ddebb3..82e81387 100644 --- a/src/cli/build.rs +++ b/src/cli/build.rs @@ -75,10 +75,10 @@ impl BuildCommand { _ => unreachable!(), }; - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; log::trace!("Constructing in-memory filesystem"); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(self.watch); let session = ServeSession::new(vfs, project_path)?; @@ -87,11 +87,16 @@ impl BuildCommand { write_model(&session, &output_path, output_kind)?; if self.watch { - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for watch mode")?; loop { let receiver = session.message_queue().subscribe(cursor); - let (new_cursor, _patch_set) = rt.block_on(receiver).unwrap(); + let (new_cursor, _patch_set) = match rt.block_on(receiver) { + Ok(message) => message, + // The message queue was dropped, so there is nothing left + // to watch. Stop watching gracefully. + Err(_) => break, + }; cursor = new_cursor; write_model(&session, &output_path, output_kind)?; diff --git a/src/cli/fmt_project.rs b/src/cli/fmt_project.rs index 1c165974..e1c7adaa 100644 --- a/src/cli/fmt_project.rs +++ b/src/cli/fmt_project.rs @@ -18,10 +18,10 @@ pub struct FmtProjectCommand { impl FmtProjectCommand { pub fn run(self) -> anyhow::Result<()> { - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(false); - let base_path = resolve_path(&self.project); + let base_path = resolve_path(&self.project)?; let project = Project::load_fuzzy(&vfs, &base_path)? .context("A project file is required to run 'rojo fmt-project'")?; diff --git a/src/cli/init.rs b/src/cli/init.rs index eb323310..434c2e34 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -9,7 +9,7 @@ use std::{ io::{self, Write}, }; -use anyhow::{bail, format_err}; +use anyhow::{bail, format_err, Context}; use clap::Parser; use fs_err as fs; use fs_err::OpenOptions; @@ -42,9 +42,9 @@ pub struct InitCommand { impl InitCommand { pub fn run(self) -> anyhow::Result<()> { - let template = self.kind.template(); + let template = self.kind.template()?; - let base_path = resolve_path(&self.path); + let base_path = resolve_path(&self.path)?; fs::create_dir_all(&base_path)?; let canonical = fs::canonicalize(&base_path)?; @@ -128,7 +128,7 @@ pub enum InitKind { } impl InitKind { - fn template(&self) -> InMemoryFs { + fn template(&self) -> anyhow::Result { let template_path = match self { Self::Place => "place", Self::Model => "model", @@ -136,20 +136,24 @@ impl InitKind { }; let snapshot: VfsSnapshot = bincode::deserialize(TEMPLATE_BINCODE) - .expect("Rojo's templates were not properly packed into Rojo's binary"); + .context("Rojo's templates were not properly packed into Rojo's binary. This is a bug in Rojo; please file an issue.")?; - if let VfsSnapshot::Dir { mut children } = snapshot { - if let Some(template) = children.remove(template_path) { - let mut fs = InMemoryFs::new(); - fs.load_snapshot("", template) - .expect("loading a template in memory should never fail"); - fs - } else { - panic!("template for project type {:?} is missing", self) - } - } else { - panic!("Rojo's templates were packed as a file instead of a directory") - } + let VfsSnapshot::Dir { mut children } = snapshot else { + bail!("Rojo's templates were packed as a file instead of a directory. This is a bug in Rojo; please file an issue."); + }; + + let template = children.remove(template_path).ok_or_else(|| { + format_err!( + "The template for project type {:?} is missing. This is a bug in Rojo; please file an issue.", + self + ) + })?; + + let mut fs = InMemoryFs::new(); + fs.load_snapshot("", template) + .context("Failed to load Rojo's bundled template into memory")?; + + Ok(fs) } } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 7d3d87b1..caef95aa 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -12,6 +12,7 @@ mod upload; use std::{borrow::Cow, env, path::Path, str::FromStr}; +use anyhow::Context; use clap::Parser; use thiserror::Error; @@ -125,10 +126,14 @@ pub enum Subcommand { Syncback(SyncbackCommand), } -pub(super) fn resolve_path(path: &Path) -> Cow<'_, Path> { +pub(super) fn resolve_path(path: &Path) -> anyhow::Result> { if path.is_absolute() { - Cow::Borrowed(path) + Ok(Cow::Borrowed(path)) } else { - Cow::Owned(env::current_dir().unwrap().join(path)) + let current_dir = env::current_dir().context( + "Could not determine the current working directory. \ + It may have been deleted, or Rojo may not have permission to access it.", + )?; + Ok(Cow::Owned(current_dir.join(path))) } } diff --git a/src/cli/serve.rs b/src/cli/serve.rs index f89e4387..b22beb69 100644 --- a/src/cli/serve.rs +++ b/src/cli/serve.rs @@ -31,13 +31,21 @@ 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 { pub fn run(self, global: GlobalOptions) -> anyhow::Result<()> { - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; let session = Arc::new(ServeSession::new(vfs, project_path)?); @@ -51,10 +59,19 @@ 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); - let _ = show_start_message(ip, port, global.color.into()); - server.start((ip, port).into()); + server.start((ip, port).into(), allowed_hosts, || { + let _ = show_start_message(ip, port, global.color.into()); + })?; Ok(()) } @@ -86,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/cli/sourcemap.rs b/src/cli/sourcemap.rs index 94369e7c..10fff590 100644 --- a/src/cli/sourcemap.rs +++ b/src/cli/sourcemap.rs @@ -5,6 +5,7 @@ use std::{ path::{self, Path, PathBuf}, }; +use anyhow::Context; use clap::Parser; use fs_err::File; use memofs::Vfs; @@ -71,10 +72,10 @@ pub struct SourcemapCommand { impl SourcemapCommand { pub fn run(self) -> anyhow::Result<()> { - let project_path = fs_err::canonicalize(resolve_path(&self.project))?; + let project_path = fs_err::canonicalize(resolve_path(&self.project)?)?; log::trace!("Constructing filesystem with StdBackend"); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(self.watch); log::trace!("Setting up session for sourcemap generation"); @@ -100,11 +101,16 @@ impl SourcemapCommand { if self.watch { log::trace!("Setting up runtime for watch mode"); - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for watch mode")?; loop { let receiver = session.message_queue().subscribe(cursor); - let (new_cursor, patch_set) = rt.block_on(receiver).unwrap(); + let (new_cursor, patch_set) = match rt.block_on(receiver) { + Ok(message) => message, + // The message queue was dropped, so there is nothing left + // to watch. Stop watching gracefully. + Err(_) => break, + }; cursor = new_cursor; if patch_set_affects_sourcemap(&session, &patch_set, filter) { diff --git a/src/cli/syncback.rs b/src/cli/syncback.rs index 282d55c8..7bb274fb 100644 --- a/src/cli/syncback.rs +++ b/src/cli/syncback.rs @@ -58,8 +58,8 @@ pub struct SyncbackCommand { impl SyncbackCommand { pub fn run(&self, global: GlobalOptions) -> anyhow::Result<()> { - let path_old = resolve_path(&self.project); - let path_new = resolve_path(&self.input); + let path_old = resolve_path(&self.project)?; + let path_new = resolve_path(&self.input)?; let input_kind = FileKind::from_path(&path_new).context(UNKNOWN_INPUT_KIND_ERR)?; let dom_start_timer = Instant::now(); @@ -69,7 +69,7 @@ impl SyncbackCommand { dom_start_timer.elapsed().as_secs_f32() ); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(false); let project_start_timer = Instant::now(); diff --git a/src/cli/upload.rs b/src/cli/upload.rs index 3c3610fd..bc9d427e 100644 --- a/src/cli/upload.rs +++ b/src/cli/upload.rs @@ -38,9 +38,9 @@ pub struct UploadCommand { impl UploadCommand { pub fn run(self) -> Result<(), anyhow::Error> { - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; let session = ServeSession::new(vfs, project_path)?; diff --git a/src/glob.rs b/src/glob.rs index 96437c38..c0146863 100644 --- a/src/glob.rs +++ b/src/glob.rs @@ -48,3 +48,61 @@ impl<'de> Deserialize<'de> for Glob { Glob::new(&glob).map_err(D::Error::custom) } } + +/// A glob with optional gitignore-style negation. A leading `!` marks the +/// pattern as a negation (re-includes paths that an earlier rule excluded). +/// To match a literal `!` at the start of a pattern, escape it with `\!`. +#[derive(Debug, Clone)] +pub struct IgnorableGlob { + glob: Glob, + negated: bool, + raw: String, +} + +impl IgnorableGlob { + pub fn new(pattern: &str) -> Result { + let (negated, body) = if let Some(rest) = pattern.strip_prefix('!') { + (true, rest) + } else if pattern.starts_with(r"\!") { + (false, &pattern[1..]) + } else { + (false, pattern) + }; + + Ok(IgnorableGlob { + glob: Glob::new(body)?, + negated, + raw: pattern.to_owned(), + }) + } + + pub fn is_match>(&self, path: P) -> bool { + self.glob.is_match(path) + } + + pub fn is_negation(&self) -> bool { + self.negated + } +} + +impl PartialEq for IgnorableGlob { + fn eq(&self, other: &Self) -> bool { + self.negated == other.negated && self.glob == other.glob + } +} + +impl Eq for IgnorableGlob {} + +impl Serialize for IgnorableGlob { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.raw) + } +} + +impl<'de> Deserialize<'de> for IgnorableGlob { + fn deserialize>(deserializer: D) -> Result { + let pattern = String::deserialize(deserializer)?; + + IgnorableGlob::new(&pattern).map_err(D::Error::custom) + } +} diff --git a/src/project.rs b/src/project.rs index 5cc8278e..093529b1 100644 --- a/src/project.rs +++ b/src/project.rs @@ -12,7 +12,8 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::{ - glob::Glob, json, resolution::UnresolvedValue, snapshot::SyncRule, syncback::SyncbackRules, + glob::IgnorableGlob, json, resolution::UnresolvedValue, snapshot::SyncRule, + syncback::SyncbackRules, }; /// Represents 'default' project names that act as `init` files @@ -105,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. @@ -114,7 +124,7 @@ pub struct Project { /// A list of globs, relative to the folder the project file is in, that /// match files that should be excluded if Rojo encounters them. #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub glob_ignore_paths: Vec, + pub glob_ignore_paths: Vec, /// A list of rules for syncback with this project file. #[serde(skip_serializing_if = "Option::is_none")] @@ -593,4 +603,90 @@ mod test { assert!(project.sync_rules[0].include.is_match("data.data.json")); 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#"{ + "name": "TestProject", + "tree": { "$path": "src" }, + "globIgnorePaths": [ + "**/*.spec.lua", + "!keep.spec.lua", + "\\!literal.lua" + ] + }"#; + + let project = Project::load_from_slice( + project_json.as_bytes(), + PathBuf::from("/test/default.project.json"), + None, + ) + .expect("project should parse"); + + let paths = &project.glob_ignore_paths; + assert_eq!(paths.len(), 3); + + assert!(!paths[0].is_negation()); + assert!(paths[0].is_match("foo.spec.lua")); + + assert!(paths[1].is_negation()); + assert!(paths[1].is_match("keep.spec.lua")); + + // `\!literal.lua` should match a file literally named `!literal.lua`, + // not be parsed as a negation. + assert!(!paths[2].is_negation()); + assert!(paths[2].is_match("!literal.lua")); + + let rules: Vec<_> = paths + .iter() + .map(|g| crate::snapshot::PathIgnoreRule { + base_path: PathBuf::from("/test"), + glob: g.clone(), + }) + .collect(); + assert!(crate::snapshot::is_path_ignored( + &rules, + "/test/foo.spec.lua" + )); + assert!(!crate::snapshot::is_path_ignored( + &rules, + "/test/keep.spec.lua" + )); + assert!(!crate::snapshot::is_path_ignored(&rules, "/test/plain.lua")); + } } 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/snapshot/metadata.rs b/src/snapshot/metadata.rs index 79f7a2d0..a1bf0afe 100644 --- a/src/snapshot/metadata.rs +++ b/src/snapshot/metadata.rs @@ -8,7 +8,7 @@ use anyhow::Context; use serde::{Deserialize, Serialize}; use crate::{ - glob::Glob, + glob::{Glob, IgnorableGlob}, path_serializer, project::ProjectNode, snapshot_middleware::{emit_legacy_scripts_default, Middleware}, @@ -222,18 +222,37 @@ pub struct PathIgnoreRule { pub base_path: PathBuf, /// The actual glob that can be matched against the input path. - pub glob: Glob, + pub glob: IgnorableGlob, } impl PathIgnoreRule { - pub fn passes>(&self, path: P) -> bool { + pub fn matches>(&self, path: P) -> bool { let path = path.as_ref(); match path.strip_prefix(&self.base_path) { - Ok(suffix) => !self.glob.is_match(suffix), - Err(_) => true, + Ok(suffix) => self.glob.is_match(suffix), + Err(_) => false, } } + + pub fn is_negation(&self) -> bool { + self.glob.is_negation() + } +} + +/// Evaluates an ordered list of [`PathIgnoreRule`]s against a path using +/// gitignore-style "last match wins" semantics: a path is ignored if the last +/// rule whose pattern matches it is non-negated. Paths matched by no rule are +/// not ignored. +pub fn is_path_ignored>(rules: &[PathIgnoreRule], path: P) -> bool { + let path = path.as_ref(); + let mut ignored = false; + for rule in rules { + if rule.matches(path) { + ignored = !rule.is_negation(); + } + } + ignored } /// Represents where a particular Instance or InstanceSnapshot came from. diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 18fa1424..b4a86efc 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -195,7 +195,7 @@ struct LocalizationEntry<'a> { /// https://github.com/BurntSushi/rust-csv/issues/151 /// /// This function operates in one step in order to minimize data-copying. -fn convert_localization_csv(contents: &[u8]) -> Result { +fn convert_localization_csv(contents: &[u8]) -> anyhow::Result { let mut reader = csv::Reader::from_reader(contents); let headers = reader.headers()?.clone(); @@ -237,7 +237,7 @@ fn convert_localization_csv(contents: &[u8]) -> Result { } let encoded = - serde_json::to_string(&entries).expect("Could not encode JSON for localization table"); + serde_json::to_string(&entries).context("Could not encode JSON for localization table")?; Ok(encoded) } diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index fb444c37..b9f62743 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -7,7 +7,9 @@ use anyhow::Context; use memofs::{DirEntry, Vfs}; use crate::{ - snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, + snapshot::{ + is_path_ignored, InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource, + }, syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, }; @@ -41,12 +43,8 @@ pub fn snapshot_dir_no_meta( path: &Path, name: &str, ) -> anyhow::Result> { - let passes_filter_rules = |child: &DirEntry| { - context - .path_ignore_rules - .iter() - .all(|rule| rule.passes(child.path())) - }; + let passes_filter_rules = + |child: &DirEntry| !is_path_ignored(&context.path_ignore_rules, child.path()); let mut snapshot_children = Vec::new(); @@ -74,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 5dc3028f..7fe3992d 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -21,7 +21,7 @@ use std::{ }; use crate::{ - glob::Glob, + glob::{Glob, IgnorableGlob}, snapshot::{InstanceWithMeta, RojoTree}, snapshot_middleware::Middleware, syncback::ref_properties::{collect_referents, link_referents}, @@ -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, @@ -414,18 +415,18 @@ pub struct SyncbackRules { } impl SyncbackRules { - pub fn compile_globs(&self) -> anyhow::Result> { + pub fn compile_globs(&self) -> anyhow::Result> { let mut globs = Vec::with_capacity(self.ignore_paths.len()); let dir_ignore_paths = self.create_ignore_dir_paths.unwrap_or(true); for pattern in &self.ignore_paths { - let glob = Glob::new(pattern) + let glob = IgnorableGlob::new(pattern) .with_context(|| format!("the pattern '{pattern}' is not a valid glob"))?; globs.push(glob); if dir_ignore_paths { if let Some(dir_pattern) = pattern.strip_suffix("/**") { - if let Ok(glob) = Glob::new(dir_pattern) { + if let Ok(glob) = IgnorableGlob::new(dir_pattern) { globs.push(glob) } } @@ -436,7 +437,7 @@ impl SyncbackRules { } } -fn is_valid_path(globs: &Option>, base_path: &Path, path: &Path) -> bool { +fn is_valid_path(globs: &Option>, base_path: &Path, path: &Path) -> bool { let git_glob = GIT_IGNORE_GLOB.get_or_init(|| Glob::new(".git/**").unwrap()); let test_path = match path.strip_prefix(base_path) { Ok(suffix) => suffix, @@ -446,11 +447,16 @@ fn is_valid_path(globs: &Option>, base_path: &Path, path: &Path) -> bo return false; } if let Some(ref ignore_paths) = globs { + // Gitignore-style "last match wins" + let mut ignored = false; for glob in ignore_paths { if glob.is_match(test_path) { - return false; + ignored = !glob.is_negation(); } } + if ignored { + return false; + } } true } @@ -538,3 +544,71 @@ fn strip_unknown_root_children(new: &mut WeakDom, old: &RojoTree) { new.destroy(child_ref); } } + +#[cfg(test)] +mod test { + use super::*; + + fn rules(ignore_paths: &[&str], create_ignore_dir_paths: Option) -> SyncbackRules { + SyncbackRules { + ignore_trees: Vec::new(), + ignore_paths: ignore_paths.iter().map(|s| s.to_string()).collect(), + ignore_properties: IndexMap::new(), + sync_current_camera: None, + sync_unscriptable: None, + ignore_referents: None, + create_ignore_dir_paths, + } + } + + #[test] + fn ignore_paths_negation() { + let globs = Some( + rules(&["**/*.lua", "!keep.lua"], Some(false)) + .compile_globs() + .unwrap(), + ); + let base = Path::new("/test"); + + // A later negation re-includes a path matched by an earlier pattern. + assert!(!is_valid_path(&globs, base, Path::new("/test/foo.lua"))); + assert!(is_valid_path(&globs, base, Path::new("/test/keep.lua"))); + // Paths matched by no rule are valid. + assert!(is_valid_path(&globs, base, Path::new("/test/plain.txt"))); + } + + #[test] + fn ignore_paths_negation_with_dir_expansion() { + // With `create_ignore_dir_paths`, a negated `foo/**` pattern should also + // re-include the `foo` directory itself, mirroring the file rule. + let globs = Some( + rules(&["**/*", "!keep/**"], Some(true)) + .compile_globs() + .unwrap(), + ); + let base = Path::new("/test"); + + assert!(!is_valid_path(&globs, base, Path::new("/test/drop/a.lua"))); + assert!(is_valid_path(&globs, base, Path::new("/test/keep"))); + assert!(is_valid_path(&globs, base, Path::new("/test/keep/a.lua"))); + } + + #[test] + fn ignore_paths_escaped_bang_is_literal() { + // `\!literal.lua` should ignore a file literally named `!literal.lua` + // rather than being parsed as a negation. + let globs = Some( + rules(&[r"\!literal.lua"], Some(false)) + .compile_globs() + .unwrap(), + ); + let base = Path::new("/test"); + + assert!(!globs.as_ref().unwrap()[0].is_negation()); + assert!(!is_valid_path( + &globs, + base, + Path::new("/test/!literal.lua") + )); + } +} 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 d6df4b15..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; @@ -12,8 +13,9 @@ use std::convert::Infallible; use std::net::SocketAddr; use std::sync::Arc; +use anyhow::Context; use hyper::{ - server::Server, + server::{conn::AddrStream, Server}, service::{make_service_fn, service_fn}, Body, Request, }; @@ -30,19 +32,44 @@ impl LiveServer { LiveServer { serve_session } } - pub fn start(self, address: SocketAddr) { + /// 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, + 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) } @@ -53,9 +80,25 @@ impl LiveServer { } }); - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for the web server")?; let _guard = rt.enter(); - let server = Server::bind(&address).serve(make_service); - rt.block_on(server).unwrap(); + let server = Server::try_bind(&address) + .with_context(|| { + format!( + "Could not start the Rojo server on {address}.\n\ + The address may already be in use or reserved. Another Rojo server might already \ + be running, or another program may be using that port.\n\ + You can pick a different port with the --port option." + ) + })? + .serve(make_service); + + // Binding succeeded, so it's now safe to tell the user we're listening. + on_listening(); + + rt.block_on(server) + .context("The Rojo web server encountered a fatal error")?; + + Ok(()) } } 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/src/web/ui.rs b/src/web/ui.rs index 4d63eaad..f6361b4f 100644 --- a/src/web/ui.rs +++ b/src/web/ui.rs @@ -6,7 +6,7 @@ use std::{borrow::Cow, sync::Arc, time::Duration}; -use hyper::{header, Body, Method, Request, Response, StatusCode}; +use hyper::{Body, Method, Request, Response, StatusCode}; use rbx_dom_weak::types::{Ref, Variant}; use ritz::{html, Fragment, HtmlContent, HtmlSelfClosingTag}; @@ -16,7 +16,7 @@ use crate::{ web::{ assets, interface::{ErrorResponse, SERVER_VERSION}, - util::json, + util::{json, response}, }, }; @@ -45,17 +45,11 @@ impl UiService { } fn handle_logo(&self) -> Response { - Response::builder() - .header(header::CONTENT_TYPE, "image/png") - .body(Body::from(assets::logo())) - .unwrap() + response(StatusCode::OK, "image/png", assets::logo()) } fn handle_icon(&self) -> Response { - Response::builder() - .header(header::CONTENT_TYPE, "image/png") - .body(Body::from(assets::icon())) - .unwrap() + response(StatusCode::OK, "image/png", assets::icon()) } fn handle_home(&self) -> Response { @@ -66,10 +60,11 @@ impl UiService { }); - Response::builder() - .header(header::CONTENT_TYPE, "text/html") - .body(Body::from(format!("{}", page))) - .unwrap() + response( + StatusCode::OK, + "text/html", + format!("{}", page), + ) } fn handle_show_instances(&self) -> Response { @@ -80,10 +75,11 @@ impl UiService { { Self::instance(&tree, root_id) } }); - Response::builder() - .header(header::CONTENT_TYPE, "text/html") - .body(Body::from(format!("{}", page))) - .unwrap() + response( + StatusCode::OK, + "text/html", + format!("{}", page), + ) } fn instance(tree: &RojoTree, id: Ref) -> HtmlContent<'_> { diff --git a/src/web/util.rs b/src/web/util.rs index 1c1cbb50..bd1f6b81 100644 --- a/src/web/util.rs +++ b/src/web/util.rs @@ -1,6 +1,27 @@ use hyper::{header::CONTENT_TYPE, Body, Response, StatusCode}; use serde::{Deserialize, Serialize}; +/// Builds an HTTP response, falling back to an empty `500` response (rather than +/// panicking) if the response could not be constructed. With constant headers +/// and a valid status code this never actually fails, but routing every +/// response through here means a malformed response can never crash the server. +pub fn response( + code: StatusCode, + content_type: &'static str, + body: impl Into, +) -> Response { + Response::builder() + .status(code) + .header(CONTENT_TYPE, content_type) + .body(body.into()) + .unwrap_or_else(|err| { + log::error!("Failed to build HTTP response: {}", err); + let mut fallback = Response::new(Body::empty()); + *fallback.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + fallback + }) +} + pub fn msgpack_ok(value: T) -> Response { msgpack(value, StatusCode::OK) } @@ -12,18 +33,14 @@ pub fn msgpack(value: T, code: StatusCode) -> Response { .with_struct_map(); if let Err(err) = value.serialize(&mut serializer) { - return Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .header(CONTENT_TYPE, "text/plain") - .body(Body::from(err.to_string())) - .unwrap(); + return response( + StatusCode::INTERNAL_SERVER_ERROR, + "text/plain", + err.to_string(), + ); }; - Response::builder() - .status(code) - .header(CONTENT_TYPE, "application/msgpack") - .body(Body::from(serialized)) - .unwrap() + response(code, "application/msgpack", serialized) } pub fn serialize_msgpack(value: T) -> anyhow::Result> { @@ -49,17 +66,13 @@ pub fn json(value: T, code: StatusCode) -> Response { let serialized = match serde_json::to_string(&value) { Ok(v) => v, Err(err) => { - return Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .header(CONTENT_TYPE, "text/plain") - .body(Body::from(err.to_string())) - .unwrap(); + return response( + StatusCode::INTERNAL_SERVER_ERROR, + "text/plain", + err.to_string(), + ); } }; - Response::builder() - .status(code) - .header(CONTENT_TYPE, "application/json") - .body(Body::from(serialized)) - .unwrap() + response(code, "application/json", serialized) } 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/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) { 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| {