From ed3b40bb846e2c1183f86276397fd9ea438a07f3 Mon Sep 17 00:00:00 2001 From: onevcat Date: Thu, 27 Aug 2026 10:44:43 +0900 Subject: [PATCH] fix: sanitise user-editable names in default screenshot filenames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #118, which fixed this for ios-device screenshot: the simulator default filename interpolated the FBSimulator name verbatim, and simctl accepts any free text as a name — "My iPhone/Work" turned the default output into a directory hierarchy, and ".." segments could walk out of the current directory. The name now passes through OutputFilePath.safeFilenameComponent, same as the physical-device verb. The Android default filename embeds the adb serial, whose accepted router charset already excludes separators; it is sanitised too as defence in depth. Video default filenames (sim-use-video-) embed no user-controlled text and need no change. Regression tests pin the slash and traversal cases; verified live with a simulator actually named "Slash/Name Probe". Co-Authored-By: Claude Fable 5 Signed-off-by: onevcat --- CHANGELOG.md | 1 + Sources/SimUse/Commands/Screenshot.swift | 5 ++++- .../Verbs/IOSSimScreenshotCommand.swift | 8 ++++++-- Tests/ScreenshotForwarderTests.swift | 20 +++++++++++++++++++ 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4585bff..684559e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `screenshot` / `ios screenshot`: a simulator name containing path separators (simctl accepts any free text, e.g. `My iPhone/Work`) no longer turns the default output filename into a directory hierarchy — the name is collapsed into a single path component, matching `ios-device screenshot`. The Android default filename embeds the adb serial, whose accepted charset already excludes separators; it is now sanitised too as defence in depth. Video default filenames embed no user-controlled text and were already safe. - `sim-use ios-device ui` no longer drops the navigation-bar back button (and any other element whose token the daemon aliases with the root). On a pushed screen `deviceFetchSpecialElement: 0` returns the back button as the root, so seeding the walk's visited set with the raw root token silently discarded it; the walk now dedups on `(token, summary, role)`, so the back button appears in the outline and is tappable with the ordinary `tap`. - `sim-use ios-device` discovery bails in ~1 s when no iPhone is attached, instead of waiting the full 5 s timeout. An empty attachment set never satisfies the quiescence rule (it needs a non-empty, unchanged set), so the discovery loop used to run to the deadline on every device-less host; it now gives up after a short grace once nothing has appeared. A device that is present still settles in ~0.4 s, and a multi-device attach burst still coalesces (the grace only applies until the first device is seen). - Top-level and `sim-use ios` verbs now reject a physical iOS device UDID at resolution time with a pointer to `sim-use ios-device`, instead of misclassifying it as an Android serial and diagnosing a plugged-in iPhone as "not reachable via adb". diff --git a/Sources/SimUse/Commands/Screenshot.swift b/Sources/SimUse/Commands/Screenshot.swift index de296576..90d4f15f 100644 --- a/Sources/SimUse/Commands/Screenshot.swift +++ b/Sources/SimUse/Commands/Screenshot.swift @@ -84,7 +84,10 @@ struct Screenshot: SimUseExecutableCommand { private func resolveAndroidOutputPath(serial: String) -> String { let stamp = IOSSimScreenshotCommand.formatTimestamp(Date()) - let defaultName = "Android Screenshot - \(serial) - \(stamp).png" + // The adb-serial charset the Android router accepts already excludes + // path separators; the sanitiser is defence in depth should those + // routing rules ever loosen. + let defaultName = "Android Screenshot - \(OutputFilePath.safeFilenameComponent(serial)) - \(stamp).png" guard let provided = output?.trimmingCharacters(in: .whitespacesAndNewlines), !provided.isEmpty else { return FileManager.default.currentDirectoryPath + "/" + defaultName } diff --git a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift index 4c110ba8..2a6e8834 100644 --- a/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift +++ b/Sources/iOSSimBackend/Verbs/IOSSimScreenshotCommand.swift @@ -81,10 +81,14 @@ public struct IOSSimScreenshotCommand: SimUseExecutableCommand { /// file URL using iOS naming conventions. Public so tests can /// pin the path expansion behaviour without spinning up an /// FBSimulator. Path semantics live in `OutputFilePath`, shared - /// with the video verbs and the physical-device screenshot. + /// with the video verbs and the physical-device screenshot. The + /// simulator name is user-editable free text (simctl accepts any + /// name), so it is collapsed into a single safe path component — + /// "My iPhone/Work" must not turn the default output into a + /// directory hierarchy. public static func prepareOutputURL(output: String?, simulatorName: String) throws -> URL { let url = OutputFilePath.resolve(output: output) { - "Simulator Screenshot - \(simulatorName) - \(formatTimestamp(Date())).png" + "Simulator Screenshot - \(OutputFilePath.safeFilenameComponent(simulatorName)) - \(formatTimestamp(Date())).png" } try OutputFilePath.prepare(url) return url diff --git a/Tests/ScreenshotForwarderTests.swift b/Tests/ScreenshotForwarderTests.swift index f08cdba3..12e4a967 100644 --- a/Tests/ScreenshotForwarderTests.swift +++ b/Tests/ScreenshotForwarderTests.swift @@ -61,6 +61,26 @@ struct ScreenshotForwarderTests { #expect(url.pathExtension == "png") } + @Test("A simulator name containing path separators stays a single filename component") + func slashedSimulatorNameStaysSingleComponent() throws { + let url = try IOSSimScreenshotCommand.prepareOutputURL( + output: nil, + simulatorName: "My iPhone/Work" + ) + #expect(url.deletingLastPathComponent().path == FileManager.default.currentDirectoryPath) + #expect(url.lastPathComponent.hasPrefix("Simulator Screenshot - My iPhone-Work - ")) + } + + @Test("A traversal-shaped simulator name cannot escape the current directory") + func traversalSimulatorNameResolvesIntoCwd() throws { + let url = try IOSSimScreenshotCommand.prepareOutputURL( + output: nil, + simulatorName: "../../evil" + ) + #expect(url.deletingLastPathComponent().path == FileManager.default.currentDirectoryPath) + #expect(!url.lastPathComponent.contains("/")) + } + @Test("Tilde-prefixed --output expands the home directory") func tildeExpansion() throws { let url = try IOSSimScreenshotCommand.prepareOutputURL(