From d5014573124d294543f16ee26184d2d422d52239 Mon Sep 17 00:00:00 2001 From: teamleaderleo Date: Fri, 25 Sep 2026 23:01:49 -0400 Subject: [PATCH 1/2] Strip control characters from feedback attachment filenames The multipart Content-Disposition filename only had quotes removed, so a filename containing CR/LF could inject extra part headers into the upload body. Strip control characters as well, via a small helper with a focused unit test. Co-authored-by: Austin Wang Co-Authored-By: Claude Opus 5.5 --- .../Client/FeedbackComposerClient.swift | 12 +++++++++++- .../FeedbackComposerClientTests.swift | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift diff --git a/Packages/macOS/CmuxFeedback/Sources/CmuxFeedback/Client/FeedbackComposerClient.swift b/Packages/macOS/CmuxFeedback/Sources/CmuxFeedback/Client/FeedbackComposerClient.swift index e27c23cdc566..3e9a3acc731e 100644 --- a/Packages/macOS/CmuxFeedback/Sources/CmuxFeedback/Client/FeedbackComposerClient.swift +++ b/Packages/macOS/CmuxFeedback/Sources/CmuxFeedback/Client/FeedbackComposerClient.swift @@ -231,13 +231,23 @@ public struct FeedbackComposerClient { return "\(baseName.isEmpty ? "feedback-image" : baseName).jpg" } + /// Strips characters that would break out of the quoted multipart + /// `filename` parameter: quotes, and CR/LF or other control characters + /// that could inject extra part headers. + static func multipartFileName(_ fileName: String) -> String { + fileName + .components(separatedBy: .controlCharacters) + .joined() + .replacingOccurrences(of: "\"", with: "") + } + private func appendFile( named fieldName: String, attachment: PreparedFeedbackComposerAttachment, to body: inout Data, boundary: String ) { - let sanitizedFileName = attachment.fileName.replacingOccurrences(of: "\"", with: "") + let sanitizedFileName = Self.multipartFileName(attachment.fileName) body.append(Data("--\(boundary)\r\n".utf8)) body.append( diff --git a/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift b/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift new file mode 100644 index 000000000000..35a4c592fbca --- /dev/null +++ b/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift @@ -0,0 +1,18 @@ +import Testing + +@testable import CmuxFeedback + +@Suite("Feedback composer client") +struct FeedbackComposerClientTests { + @Test("multipart filenames drop quotes and control characters") + func multipartFileNameStripsQuotesAndControlCharacters() { + let unsafeFileName = "capture\"\r\ninjected\u{0001}\t\u{007F}.png" + #expect(FeedbackComposerClient.multipartFileName(unsafeFileName) == "captureinjected.png") + } + + @Test("ordinary filenames pass through unchanged") + func multipartFileNameKeepsOrdinaryNames() { + #expect(FeedbackComposerClient.multipartFileName("Screen Shot 2026-09-25 at 10.00.00.png") == "Screen Shot 2026-09-25 at 10.00.00.png") + #expect(FeedbackComposerClient.multipartFileName("日本語.png") == "日本語.png") + } +} From 56df6bd8fe92ca847bee88db193de0a790510748 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Sun, 27 Sep 2026 06:39:50 -0700 Subject: [PATCH 2/2] Test feedback filename sanitization through multipart submission --- .../FeedbackComposerClientTests.swift | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift b/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift index 35a4c592fbca..fa599c63e276 100644 --- a/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift +++ b/Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/FeedbackComposerClientTests.swift @@ -1,9 +1,47 @@ +import Foundation import Testing @testable import CmuxFeedback @Suite("Feedback composer client") struct FeedbackComposerClientTests { + @Test("submitted multipart body keeps hostile filenames inside the attachment header") + func submittedMultipartFileNameStripsControlCharacters() async throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + let file = directory.appendingPathComponent("capture\"\r\nX-Injected: yes\u{0001}\t\u{007F}.png") + try Data("attachment-payload".utf8).write(to: file) + + try #require(URLProtocol.registerClass(FeedbackMultipartCaptureProtocol.self)) + defer { URLProtocol.unregisterClass(FeedbackMultipartCaptureProtocol.self) } + let settings = FeedbackComposerSettings( + endpointEnvironmentKey: UUID().uuidString, + defaultEndpoint: "https://feedback-multipart-test.invalid/upload" + ) + try await FeedbackComposerClient(settings: settings).submit( + email: "test@example.com", + message: "Multipart regression", + attachments: [try FeedbackComposerAttachment(url: file)] + ) + + let captured = try #require(FeedbackMultipartCaptureProtocol.capturedRequest()) + let contentType = try #require(captured.request.value(forHTTPHeaderField: "Content-Type")) + let prefix = "multipart/form-data; boundary=" + try #require(contentType.hasPrefix(prefix)) + let boundary = String(contentType.dropFirst(prefix.count)) + let body = String(decoding: captured.body, as: UTF8.self) + let parts = body.components(separatedBy: "--\(boundary)") + #expect(captured.request.httpMethod == "POST") + #expect(parts.first == "") + #expect(parts.last == "--\r\n") + #expect(parts.filter { $0.contains("name=\"attachments\"") } == [ + "\r\nContent-Disposition: form-data; name=\"attachments\"; filename=\"captureX-Injected: yes.png\"\r\n" + + "Content-Type: image/png\r\n\r\nattachment-payload\r\n", + ]) + #expect(!body.contains("\r\nX-Injected:")) + } + @Test("multipart filenames drop quotes and control characters") func multipartFileNameStripsQuotesAndControlCharacters() { let unsafeFileName = "capture\"\r\ninjected\u{0001}\t\u{007F}.png" @@ -16,3 +54,42 @@ struct FeedbackComposerClientTests { #expect(FeedbackComposerClient.multipartFileName("日本語.png") == "日本語.png") } } + +private final class FeedbackMultipartCaptureProtocol: URLProtocol, @unchecked Sendable { + private static let lock = NSLock() + nonisolated(unsafe) private static var captured: (request: URLRequest, body: Data)? + + static func capturedRequest() -> (request: URLRequest, body: Data)? { + lock.withLock { captured } + } + + override class func canInit(with request: URLRequest) -> Bool { + request.url?.host == "feedback-multipart-test.invalid" + } + + override class func canonicalRequest(for request: URLRequest) -> URLRequest { request } + + override func startLoading() { + var body = request.httpBody ?? Data() + if let stream = request.httpBodyStream { + stream.open() + defer { stream.close() } + var buffer = [UInt8](repeating: 0, count: 4096) + while true { + let count = stream.read(&buffer, maxLength: buffer.count) + if count < 0 { + client?.urlProtocol(self, didFailWithError: stream.streamError ?? URLError(.cannotDecodeRawData)) + return + } + if count == 0 { break } + body.append(contentsOf: buffer.prefix(count)) + } + } + Self.lock.withLock { Self.captured = (request, body) } + let response = HTTPURLResponse(url: request.url!, statusCode: 200, httpVersion: nil, headerFields: nil)! + client?.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed) + client?.urlProtocolDidFinishLoading(self) + } + + override func stopLoading() {} +}