From 17ed2be5f6774956d37d1b205147f339bfa29006 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 26 Jan 2026 20:03:18 -0500 Subject: [PATCH 1/3] fix: Mitigate crashes from unsafe assets Check for valid asset paths before attempting to access them. --- .../Sources/Model/EditorAssetBundle.swift | 17 +++++++ .../Services/EditorAssetBundleProvider.swift | 11 +++++ .../Model/EditorAssetBundleTests.swift | 47 +++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/ios/Sources/GutenbergKit/Sources/Model/EditorAssetBundle.swift b/ios/Sources/GutenbergKit/Sources/Model/EditorAssetBundle.swift index 9ba256767..92c313d88 100644 --- a/ios/Sources/GutenbergKit/Sources/Model/EditorAssetBundle.swift +++ b/ios/Sources/GutenbergKit/Sources/Model/EditorAssetBundle.swift @@ -97,10 +97,27 @@ public struct EditorAssetBundle: Sendable, Equatable, Hashable { ) } + /// Checks whether the given asset URL resolves to a valid path within the bundle root. + /// + /// Use this method to validate URLs before calling `assetDataPath(for:)` or `hasAssetData(for:)` + /// to avoid precondition failures for paths that escape the bundle root (e.g., plugin assets + /// referenced in CSS that weren't downloaded into the bundle). + /// + /// - Parameter url: The asset URL to validate. + /// - Returns: `true` if the URL resolves to a path within the bundle root, `false` otherwise. + public func isValidAssetPath(for url: URL) -> Bool { + let path = url.path(percentEncoded: false) + let bundlePath = self.bundleRoot.appending(rawPath: path).standardizedFileURL + let bundleRootPath = self.bundleRoot.standardizedFileURL.path + return bundlePath.path.hasPrefix(bundleRootPath + "/") || bundlePath.path == bundleRootPath + } + /// Checks whether this bundle contains cached data for the given asset URL. /// /// - Parameter url: The original remote URL of the asset. /// - Returns: `true` if the asset is cached in this bundle, `false` otherwise. + /// - Precondition: The URL path must not escape the bundle root directory. + /// Use `isValidAssetPath(for:)` to check validity before calling this method. public func hasAssetData(for url: URL) -> Bool { FileManager.default.fileExists(at: self.assetDataPath(for: url)) } diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift index b3a623d66..ece605ef4 100644 --- a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift @@ -108,6 +108,17 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { preconditionFailure("Cannot read asset with no bundle present. This is a programmer error.") } + // Check if the path is valid before attempting to access the asset. + // This prevents crashes for paths that escape the bundle root (e.g., plugin + // SVGs referenced in CSS that weren't downloaded into the bundle). + guard bundle.isValidAssetPath(for: url) else { + loggerMessages.append(" Path escapes bundle root – sending 404") + let response = HTTPURLResponse(url: url, statusCode: 404, httpVersion: nil, headerFields: nil)! + urlSchemeTask.didReceive(response) + urlSchemeTask.didFinish() + return + } + guard bundle.hasAssetData(for: url) else { loggerMessages.append(" Path: ") diff --git a/ios/Tests/GutenbergKitTests/Model/EditorAssetBundleTests.swift b/ios/Tests/GutenbergKitTests/Model/EditorAssetBundleTests.swift index 504ddc3c8..dbb261609 100644 --- a/ios/Tests/GutenbergKitTests/Model/EditorAssetBundleTests.swift +++ b/ios/Tests/GutenbergKitTests/Model/EditorAssetBundleTests.swift @@ -257,6 +257,53 @@ struct EditorAssetBundleTests { try? FileManager.default.removeItem(at: tempDir) } + // MARK: - isValidAssetPath Tests + + @Test("isValidAssetPath returns true for valid path within bundle") + func isValidAssetPathReturnsTrueForValidPath() { + let tempDir = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + let bundle = makeBundle(bundleRoot: tempDir) + + let url = URL(string: "https://example.com/wp-content/plugins/script.js")! + #expect(bundle.isValidAssetPath(for: url)) + } + + @Test("isValidAssetPath returns true for nested paths") + func isValidAssetPathReturnsTrueForNestedPaths() { + let tempDir = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + let bundle = makeBundle(bundleRoot: tempDir) + + let url = URL(string: "https://example.com/wp-content/plugins/jetpack/assets/js/script.js")! + #expect(bundle.isValidAssetPath(for: url)) + } + + @Test("isValidAssetPath returns false for path traversal attempt") + func isValidAssetPathReturnsFalseForPathTraversal() { + let tempDir = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + let bundle = makeBundle(bundleRoot: tempDir) + + let url = URL(string: "https://example.com/../../../etc/passwd")! + #expect(!bundle.isValidAssetPath(for: url)) + } + + @Test("isValidAssetPath returns false for path escaping via encoded traversal") + func isValidAssetPathReturnsFalseForEncodedTraversal() { + let tempDir = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + let bundle = makeBundle(bundleRoot: tempDir) + + let url = URL(string: "https://example.com/%2e%2e/%2e%2e/etc/passwd")! + #expect(!bundle.isValidAssetPath(for: url)) + } + + @Test("isValidAssetPath handles paths with dot segments that stay within bundle") + func isValidAssetPathHandlesDotSegmentsWithinBundle() { + let tempDir = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + let bundle = makeBundle(bundleRoot: tempDir) + + let url = URL(string: "https://example.com/wp-content/./plugins/script.js")! + #expect(bundle.isValidAssetPath(for: url)) + } + // MARK: - assetDataPath Tests @Test("assetDataPath returns correct path based on URL path") From b03787a1807377940be03f215860f1a783ebf1f5 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 27 Jan 2026 09:15:30 -0500 Subject: [PATCH 2/3] fix: Fetch remote site asset when local cache is unavailable Ensure assets not cached locally still load in the editor. E.g., we do not download and cache images loaded within CSS files. --- .../Services/EditorAssetBundleProvider.swift | 94 +++++++++++++++---- 1 file changed, 77 insertions(+), 17 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift index ece605ef4..352589d8d 100644 --- a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift @@ -9,6 +9,10 @@ import WebKit /// asset manifest to JavaScript) and a URL scheme handler (to serve individual cached /// files via the `gbk-cache-https` scheme). /// +/// When an asset is not found in the local cache (e.g., images referenced in CSS that +/// weren't downloaded), the provider fetches the asset from its original HTTPS URL +/// and serves it to the WebView. +/// /// ## Usage /// /// ```swift @@ -23,6 +27,12 @@ public final class EditorAssetBundleProvider: NSObject, @unchecked Sendable { private let lock = NSLock() private var bundle: EditorAssetBundle? + private let urlSession: URLSession + + override public init() { + self.urlSession = URLSession(configuration: .default) + super.init() + } /// Sets the asset bundle to serve to the WebView. /// @@ -108,24 +118,14 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { preconditionFailure("Cannot read asset with no bundle present. This is a programmer error.") } - // Check if the path is valid before attempting to access the asset. - // This prevents crashes for paths that escape the bundle root (e.g., plugin - // SVGs referenced in CSS that weren't downloaded into the bundle). - guard bundle.isValidAssetPath(for: url) else { - loggerMessages.append(" Path escapes bundle root – sending 404") - let response = HTTPURLResponse(url: url, statusCode: 404, httpVersion: nil, headerFields: nil)! - urlSchemeTask.didReceive(response) - urlSchemeTask.didFinish() - return - } - - guard bundle.hasAssetData(for: url) else { - loggerMessages.append(" Path: ") + // Check if the path is valid and the asset exists in the bundle. + // If not, fetch from the original HTTPS URL (e.g., for plugin SVGs + // referenced in CSS that weren't downloaded into the bundle). + let shouldFetchFromRemote = !bundle.isValidAssetPath(for: url) || !bundle.hasAssetData(for: url) - loggerMessages.append(" Not found – sending 404") - let response = HTTPURLResponse(url: url, statusCode: 404, httpVersion: nil, headerFields: nil)! - urlSchemeTask.didReceive(response) - urlSchemeTask.didFinish() + guard !shouldFetchFromRemote else { + loggerMessages.append(" Asset not in bundle – fetching from remote") + self.fetchFromRemote(url: url, urlSchemeTask: urlSchemeTask, loggerMessages: &loggerMessages) return } @@ -148,4 +148,64 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { public func webView(_ webView: WKWebView, stop urlSchemeTask: any WKURLSchemeTask) { // No-op: since we're reading from disk synchronously, there's nothing to cancel } + + /// Fetches an asset from its original remote URL and serves it to the WebView. + /// + /// This is used when an asset isn't in the local bundle (e.g., images referenced + /// in CSS files that weren't downloaded because only JS/CSS files are cached). + private func fetchFromRemote( + url: URL, + urlSchemeTask: any WKURLSchemeTask, + loggerMessages: inout [String] + ) { + guard let originalURL = self.originalURL(for: url) else { + loggerMessages.append(" Failed to construct original URL") + let response = HTTPURLResponse(url: url, statusCode: 400, httpVersion: nil, headerFields: nil)! + urlSchemeTask.didReceive(response) + urlSchemeTask.didFinish() + return + } + + loggerMessages.append(" Fetching from: \(originalURL)") + + let task = urlSession.dataTask(with: originalURL) { data, response, error in + if let error { + Logger.assetLibrary.error("📚 Failed to fetch remote asset: \(error.localizedDescription)") + urlSchemeTask.didFailWithError(error) + return + } + + guard let httpResponse = response as? HTTPURLResponse, let data else { + urlSchemeTask.didFailWithError(URLError(.badServerResponse)) + return + } + + // Create a new response with the original URL scheme task's URL + let schemeResponse = HTTPURLResponse( + url: url, + statusCode: httpResponse.statusCode, + httpVersion: "HTTP/1.1", + headerFields: httpResponse.allHeaderFields as? [String: String] + )! + + urlSchemeTask.didReceive(schemeResponse) + urlSchemeTask.didReceive(data) + urlSchemeTask.didFinish() + } + task.resume() + } + + /// Converts a `gbk-cache-https` URL back to its original `https` URL. + /// + /// For example: `gbk-cache-https://example.com/path` → `https://example.com/path` + private func originalURL(for url: URL) -> URL? { + var components = URLComponents(url: url, resolvingAgainstBaseURL: false) + // The scheme "gbk-cache-https" encodes the original scheme after the prefix + let schemePrefix = "gbk-cache-" + guard let scheme = components?.scheme, scheme.hasPrefix(schemePrefix) else { + return nil + } + components?.scheme = String(scheme.dropFirst(schemePrefix.count)) + return components?.url + } } From f706cb98275c590819a80992d0fc0e1bc32d4b3a Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 28 Jan 2026 08:31:49 -0700 Subject: [PATCH 3/3] Suggestions (#302) * Suggestions * docs: Fix inline comment typo --------- Co-authored-by: David Calhoun --- .../Sources/EditorViewController.swift | 14 ++- .../Services/EditorAssetBundleProvider.swift | 101 ++++++++---------- .../Stores/EditorAssetLibraryTests.swift | 2 +- 3 files changed, 59 insertions(+), 58 deletions(-) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 9edaff7a8..c729f45b4 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -107,7 +107,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let editorService: EditorService private let mediaPicker: MediaPickerController? private let controller: GutenbergEditorController - private let bundleProvider = EditorAssetBundleProvider() + private let bundleProvider: EditorAssetBundleProvider // MARK: - Private Properties (UI) @@ -153,11 +153,21 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro configuration: EditorConfiguration, dependencies: EditorDependencies? = nil, mediaPicker: MediaPickerController? = nil, + httpClient: EditorHTTPClient? = nil, isWarmupMode: Bool = false ) { + let httpClient = httpClient ?? EditorHTTPClient( + urlSession: URLSession.shared, + authHeader: configuration.authHeader + ) + self.configuration = configuration self.dependencies = dependencies - self.editorService = EditorService(configuration: configuration) + self.editorService = EditorService( + configuration: configuration, + httpClient: httpClient + ) + self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker self.controller = GutenbergEditorController(configuration: configuration) diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift index 352589d8d..05a78cf07 100644 --- a/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorAssetBundleProvider.swift @@ -23,14 +23,15 @@ import WebKit /// /// The provider must be bound to the WebView configuration before loading the editor, /// and must have a bundle set before the editor requests assets. -public final class EditorAssetBundleProvider: NSObject, @unchecked Sendable { +@MainActor +public final class EditorAssetBundleProvider: NSObject { - private let lock = NSLock() private var bundle: EditorAssetBundle? - private let urlSession: URLSession + private let httpClient: EditorHTTPClient + private var runningTasks: [Task] = [] - override public init() { - self.urlSession = URLSession(configuration: .default) + public init(httpClient: EditorHTTPClient) { + self.httpClient = httpClient super.init() } @@ -40,9 +41,7 @@ public final class EditorAssetBundleProvider: NSObject, @unchecked Sendable { /// /// - Parameter bundle: The downloaded asset bundle containing cached scripts and styles. public func set(bundle: EditorAssetBundle) { - lock.withLock { - self.bundle = bundle - } + self.bundle = bundle } /// Registers this provider with a WebView configuration. @@ -100,19 +99,15 @@ extension EditorAssetBundleProvider: WKScriptMessageHandlerWithReply { extension EditorAssetBundleProvider: WKURLSchemeHandler { public func webView(_ webView: WKWebView, start urlSchemeTask: any WKURLSchemeTask) { logExecutionTime("Retrieved cached asset") { - var loggerMessages = ["📚 Editor requested a cached asset"] - - defer { - Logger.assetLibrary.info("\(loggerMessages.joined(separator: "\n"))") - } + Logger.assetLibrary.info("📚 Editor requested a cached asset") guard let url = urlSchemeTask.request.url else { - loggerMessages.append(" URL: ") + Logger.assetLibrary.info(" URL: ") urlSchemeTask.didFailWithError(URLError(.badURL)) return } - loggerMessages.append(" URL: \(url)") + Logger.assetLibrary.info(" URL: \(url)") guard let bundle else { preconditionFailure("Cannot read asset with no bundle present. This is a programmer error.") @@ -124,13 +119,13 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { let shouldFetchFromRemote = !bundle.isValidAssetPath(for: url) || !bundle.hasAssetData(for: url) guard !shouldFetchFromRemote else { - loggerMessages.append(" Asset not in bundle – fetching from remote") - self.fetchFromRemote(url: url, urlSchemeTask: urlSchemeTask, loggerMessages: &loggerMessages) + Logger.assetLibrary.info(" Asset not in bundle – fetching from remote") + self.fetchFromRemote(for: urlSchemeTask) return } do { - loggerMessages.append(" Path: \(bundle.assetDataPath(for: url))") + Logger.assetLibrary.info(" Path: \(bundle.assetDataPath(for: url))") let data = try bundle.assetData(for: url) let response = HTTPURLResponse(url: url, statusCode: 200, httpVersion: "HTTP/1.1", headerFields: nil)! @@ -138,10 +133,9 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { urlSchemeTask.didReceive(data) urlSchemeTask.didFinish() } catch { - loggerMessages.append(" Error: \(error.localizedDescription)") + Logger.assetLibrary.warning(" Error: \(error.localizedDescription)") urlSchemeTask.didFailWithError(error) } - } } @@ -153,59 +147,56 @@ extension EditorAssetBundleProvider: WKURLSchemeHandler { /// /// This is used when an asset isn't in the local bundle (e.g., images referenced /// in CSS files that weren't downloaded because only JS/CSS files are cached). - private func fetchFromRemote( - url: URL, - urlSchemeTask: any WKURLSchemeTask, - loggerMessages: inout [String] - ) { - guard let originalURL = self.originalURL(for: url) else { - loggerMessages.append(" Failed to construct original URL") - let response = HTTPURLResponse(url: url, statusCode: 400, httpVersion: nil, headerFields: nil)! + private func fetchFromRemote(for urlSchemeTask: any WKURLSchemeTask) { + guard let originalRequest = self.originalRequest(for: urlSchemeTask.request) else { + Logger.assetLibrary.info(" Failed to construct original URL") + let response = HTTPURLResponse( + url: urlSchemeTask.request.url!, + statusCode: 400, + httpVersion: nil, + headerFields: nil + )! urlSchemeTask.didReceive(response) urlSchemeTask.didFinish() return } - loggerMessages.append(" Fetching from: \(originalURL)") + Logger.assetLibrary.info(" Fetching: \(originalRequest)") - let task = urlSession.dataTask(with: originalURL) { data, response, error in - if let error { + let taskHandle = Task { + do { + let (data, response) = try await self.httpClient.perform(originalRequest) + + urlSchemeTask.didReceive(response) + urlSchemeTask.didReceive(data) + urlSchemeTask.didFinish() + } + catch { Logger.assetLibrary.error("📚 Failed to fetch remote asset: \(error.localizedDescription)") urlSchemeTask.didFailWithError(error) - return } - - guard let httpResponse = response as? HTTPURLResponse, let data else { - urlSchemeTask.didFailWithError(URLError(.badServerResponse)) - return - } - - // Create a new response with the original URL scheme task's URL - let schemeResponse = HTTPURLResponse( - url: url, - statusCode: httpResponse.statusCode, - httpVersion: "HTTP/1.1", - headerFields: httpResponse.allHeaderFields as? [String: String] - )! - - urlSchemeTask.didReceive(schemeResponse) - urlSchemeTask.didReceive(data) - urlSchemeTask.didFinish() } - task.resume() + + self.runningTasks.append(taskHandle) } /// Converts a `gbk-cache-https` URL back to its original `https` URL. /// /// For example: `gbk-cache-https://example.com/path` → `https://example.com/path` - private func originalURL(for url: URL) -> URL? { - var components = URLComponents(url: url, resolvingAgainstBaseURL: false) + private func originalRequest(for request: URLRequest) -> URLRequest? { + guard let url = request.url, var components = URLComponents(url: url, resolvingAgainstBaseURL: false) else { + return nil + } + // The scheme "gbk-cache-https" encodes the original scheme after the prefix let schemePrefix = "gbk-cache-" - guard let scheme = components?.scheme, scheme.hasPrefix(schemePrefix) else { + guard let scheme = components.scheme, scheme.hasPrefix(schemePrefix) else { return nil } - components?.scheme = String(scheme.dropFirst(schemePrefix.count)) - return components?.url + components.scheme = String(scheme.dropFirst(schemePrefix.count)) + + var mutableCopy = request + mutableCopy.url = components.url + return mutableCopy } } diff --git a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift index 0e354a441..95405b7fc 100644 --- a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift @@ -59,7 +59,7 @@ struct EditorAssetLibraryTests { func existingBundleReturnsNilForMissingChecksum() async throws { let library = makeLibrary() - let result = try await library.existingBundle(forManifestChecksum: "nonexistent-checksum-12345") + let result = await library.existingBundle(forManifestChecksum: "nonexistent-checksum-12345") #expect(result == nil) }