Skip to content

Commit b4c5874

Browse files
tomerdtomerd
authored
test that downloaded binary artifacts are valid archives before computing checksum (#4116)
motivation: a web-server may return an invalid archive file (say as a result of a redirect) which should emit a nicer error message instead of checksum mismatch changes: * add an Archiver::validate API * implement the validate API uzing unzip -t * add adjust tests rdar://73225887
1 parent 0a3e43b commit b4c5874

File tree

8 files changed

+285
-76
lines changed

8 files changed

+285
-76
lines changed

‎Sources/Basics/Archiver+Zip.swift

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,22 @@ public struct ZipArchiver: Archiver {
4444
Process.popen(arguments: ["unzip", archivePath.pathString, "-d", destinationPath.pathString], queue: .sharedConcurrent) { result in
4545
completion(result.tryMap { processResult in
4646
guard processResult.exitStatus == .terminated(code: 0) else {
47-
throw try StringError(processResult.utf8stderrOutput())
47+
throw try StringError(processResult.utf8stderrOutput())
4848
}
4949
})
5050
}
5151
}
52+
53+
public func validate(path: AbsolutePath, completion: @escaping (Result<Bool, Error>) -> Void) {
54+
guard fileSystem.exists(path) else {
55+
completion(.failure(FileSystemError(.noEntry, path)))
56+
return
57+
}
58+
59+
Process.popen(arguments: ["unzip", "-t", path.pathString], queue: .sharedConcurrent) { result in
60+
completion(result.tryMap { processResult in
61+
return processResult.exitStatus == .terminated(code: 0)
62+
})
63+
}
64+
}
5265
}

‎Sources/Basics/Archiver.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,14 @@ public protocol Archiver {
2626
to destinationPath: AbsolutePath,
2727
completion: @escaping (Result<Void, Error>) -> Void
2828
)
29+
30+
/// Asynchronously validates if a file is an archive.
31+
///
32+
/// - Parameters:
33+
/// - path: The `AbsolutePath` to the archive to validate.
34+
/// - completion: The completion handler that will be called when the operation finishes to notify of its success.
35+
func validate(
36+
path: AbsolutePath,
37+
completion: @escaping (Result<Bool, Error>) -> Void
38+
)
2939
}

‎Sources/SPMTestSupport/MockArchiver.swift

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ import Basics
1212
import TSCBasic
1313

1414
public class MockArchiver: Archiver {
15-
public typealias Handler = (MockArchiver, AbsolutePath, AbsolutePath, (Result<Void, Error>) -> Void) throws -> Void
15+
public typealias ExtractionHandler = (MockArchiver, AbsolutePath, AbsolutePath, (Result<Void, Error>) -> Void) throws -> Void
16+
public typealias ValidationHandler = (MockArchiver, AbsolutePath, (Result<Bool, Error>) -> Void) throws -> Void
1617

1718
public struct Extraction: Equatable {
1819
public let archivePath: AbsolutePath
@@ -26,10 +27,16 @@ public class MockArchiver: Archiver {
2627

2728
public let supportedExtensions: Set<String> = ["zip"]
2829
public let extractions = ThreadSafeArrayStore<Extraction>()
29-
public let handler: Handler?
30+
public let extractionHandler: ExtractionHandler?
31+
public let validationHandler: ValidationHandler?
3032

31-
public init(handler: Handler? = nil) {
32-
self.handler = handler
33+
public convenience init(handler: ExtractionHandler? = nil) {
34+
self.init(extractionHandler: handler, validationHandler: nil)
35+
}
36+
37+
public init(extractionHandler: ExtractionHandler? = nil, validationHandler: ValidationHandler? = nil) {
38+
self.extractionHandler = extractionHandler
39+
self.validationHandler = validationHandler
3340
}
3441

3542
public func extract(
@@ -38,7 +45,7 @@ public class MockArchiver: Archiver {
3845
completion: @escaping (Result<Void, Error>) -> Void
3946
) {
4047
do {
41-
if let handler = self.handler {
48+
if let handler = self.extractionHandler {
4249
try handler(self, archivePath, destinationPath, completion)
4350
} else {
4451
self.extractions.append(Extraction(archivePath: archivePath, destinationPath: destinationPath))
@@ -48,4 +55,16 @@ public class MockArchiver: Archiver {
4855
completion(.failure(error))
4956
}
5057
}
58+
59+
public func validate(path: AbsolutePath, completion: @escaping (Result<Bool, Error>) -> Void) {
60+
do {
61+
if let handler = self.validationHandler {
62+
try handler(self, path, completion)
63+
} else {
64+
completion(.success(true))
65+
}
66+
} catch {
67+
completion(.failure(error))
68+
}
69+
}
5170
}

‎Sources/SPMTestSupport/MockRegistry.swift

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -288,12 +288,10 @@ private struct MockRegistryArchiver: Archiver {
288288

289289
func extract(from archivePath: AbsolutePath, to destinationPath: AbsolutePath, completion: @escaping (Result<Void, Error>) -> Void) {
290290
do {
291-
let content: String = try self.fileSystem.readFileContents(archivePath)
292-
let lines = content.split(separator: "\n").map(String.init)
291+
let lines = try self.readFileContents(archivePath)
293292
guard lines.count >= 2 else {
294293
throw StringError("invalid mock zip format, not enough lines")
295294
}
296-
297295
let rootPath = lines[1]
298296
for path in lines[2..<lines.count] {
299297
let relativePath = String(path.dropFirst(rootPath.count + 1))
@@ -308,4 +306,18 @@ private struct MockRegistryArchiver: Archiver {
308306
completion(.failure(error))
309307
}
310308
}
309+
310+
func validate(path: AbsolutePath, completion: @escaping (Result<Bool, Error>) -> Void) {
311+
do {
312+
let lines = try self.readFileContents(path)
313+
completion(.success(lines.count >= 2))
314+
} catch {
315+
completion(.failure(error))
316+
}
317+
}
318+
319+
private func readFileContents(_ path: AbsolutePath) throws -> [String] {
320+
let content: String = try self.fileSystem.readFileContents(path)
321+
return content.split(separator: "\n").map(String.init)
322+
}
311323
}

‎Sources/Workspace/Diagnostics.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,10 @@ extension Basics.Diagnostic {
130130
.warning("dependency '\(packageName)' is missing; retrieving again")
131131
}
132132

133+
static func artifactInvalidArchive(artifactURL: Foundation.URL, targetName: String) -> Self {
134+
.error("invalid archive returned from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)'")
135+
}
136+
133137
static func artifactChecksumChanged(targetName: String) -> Self {
134138
.error("artifact of binary target '\(targetName)' has changed checksum; this is a potential security risk so the new artifact won't be downloaded")
135139
}
@@ -142,6 +146,10 @@ extension Basics.Diagnostic {
142146
.error("failed downloading '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
143147
}
144148

149+
static func artifactFailedValidation(artifactURL: Foundation.URL, targetName: String, reason: String) -> Self {
150+
.error("failed validating archive from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
151+
}
152+
145153
static func artifactFailedExtraction(artifactURL: Foundation.URL, targetName: String, reason: String) -> Self {
146154
.error("failed extracting '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
147155
}

‎Sources/Workspace/Workspace.swift

Lines changed: 84 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -2539,85 +2539,105 @@ extension Workspace {
25392539
semaphore.signal()
25402540
}
25412541

2542+
// TODO: Use the same extraction logic for both remote and local archived artifacts.
25422543
switch downloadResult {
25432544
case .success:
2544-
guard let archiveChecksum = observabilityScope.trap ({ try self.checksum(forBinaryArtifactAt: archivePath) }) else {
2545-
return
2546-
}
2547-
guard archiveChecksum == artifact.checksum else {
2548-
observabilityScope.emit(.artifactInvalidChecksum(targetName: artifact.targetName, expectedChecksum: artifact.checksum, actualChecksum: archiveChecksum))
2549-
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
2550-
return
2551-
}
25522545

2553-
guard let tempExtractionDirectory = observabilityScope.trap({ () -> AbsolutePath in
2554-
let path = self.location.artifactsDirectory.appending(components: "extract", artifact.packageRef.identity.description, artifact.targetName, UUID().uuidString)
2555-
try self.fileSystem.forceCreateDirectory(at: path)
2556-
return path
2557-
}) else {
2558-
return
2559-
}
2560-
2561-
// TODO: Use the same extraction logic for both remote and local archived artifacts.
25622546
group.enter()
2563-
observabilityScope.emit(debug: "extracting \(archivePath) to \(tempExtractionDirectory)")
2564-
self.archiver.extract(from: archivePath, to: tempExtractionDirectory, completion: { extractResult in
2547+
observabilityScope.emit(debug: "validating \(archivePath)")
2548+
self.archiver.validate(path: archivePath, completion: { validationResult in
25652549
defer { group.leave() }
25662550

2567-
switch extractResult {
2568-
case .success:
2569-
var artifactPath: AbsolutePath? = nil
2570-
observabilityScope.trap {
2571-
try self.fileSystem.withLock(on: parentDirectory, type: .exclusive) {
2572-
// strip first level component if needed
2573-
if try self.fileSystem.shouldStripFirstLevel(archiveDirectory: tempExtractionDirectory, acceptableExtensions: BinaryTarget.Kind.allCases.map({ $0.fileExtension })) {
2574-
observabilityScope.emit(debug: "stripping first level component from \(tempExtractionDirectory)")
2575-
try self.fileSystem.stripFirstLevel(of: tempExtractionDirectory)
2576-
} else {
2577-
observabilityScope.emit(debug: "no first level component stripping needed for \(tempExtractionDirectory)")
2578-
}
2579-
let content = try self.fileSystem.getDirectoryContents(tempExtractionDirectory)
2580-
// copy from temp location to actual location
2581-
for file in content {
2582-
let source = tempExtractionDirectory.appending(component: file)
2583-
let destination = parentDirectory.appending(component: file)
2584-
if self.fileSystem.exists(destination) {
2585-
try self.fileSystem.removeFileTree(destination)
2586-
}
2587-
try self.fileSystem.copy(from: source, to: destination)
2588-
if destination.basenameWithoutExt == artifact.targetName {
2589-
artifactPath = destination
2590-
}
2591-
}
2592-
}
2593-
// remove temp location
2594-
try self.fileSystem.removeFileTree(tempExtractionDirectory)
2551+
switch validationResult {
2552+
case .success(let valid):
2553+
guard valid else {
2554+
observabilityScope.emit(.artifactInvalidArchive(artifactURL: artifact.url, targetName: artifact.targetName))
2555+
return
2556+
}
2557+
2558+
guard let archiveChecksum = observabilityScope.trap ({ try self.checksum(forBinaryArtifactAt: archivePath) }) else {
2559+
return
2560+
}
2561+
guard archiveChecksum == artifact.checksum else {
2562+
observabilityScope.emit(.artifactInvalidChecksum(targetName: artifact.targetName, expectedChecksum: artifact.checksum, actualChecksum: archiveChecksum))
2563+
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
2564+
return
25952565
}
25962566

2597-
guard let mainArtifactPath = artifactPath else {
2598-
return observabilityScope.emit(.artifactNotFound(targetName: artifact.targetName, artifactName: artifact.targetName))
2567+
guard let tempExtractionDirectory = observabilityScope.trap({ () -> AbsolutePath in
2568+
let path = self.location.artifactsDirectory.appending(components: "extract", artifact.packageRef.identity.description, artifact.targetName, UUID().uuidString)
2569+
try self.fileSystem.forceCreateDirectory(at: path)
2570+
return path
2571+
}) else {
2572+
return
25992573
}
26002574

2601-
result.append(
2602-
.remote(
2603-
packageRef: artifact.packageRef,
2604-
targetName: artifact.targetName,
2605-
url: artifact.url.absoluteString,
2606-
checksum: artifact.checksum,
2607-
path: mainArtifactPath
2608-
)
2609-
)
2610-
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .success(mainArtifactPath), duration: downloadStart.distance(to: .now()))
2575+
group.enter()
2576+
observabilityScope.emit(debug: "extracting \(archivePath) to \(tempExtractionDirectory)")
2577+
self.archiver.extract(from: archivePath, to: tempExtractionDirectory, completion: { extractResult in
2578+
defer { group.leave() }
2579+
2580+
switch extractResult {
2581+
case .success:
2582+
var artifactPath: AbsolutePath? = nil
2583+
observabilityScope.trap {
2584+
try self.fileSystem.withLock(on: parentDirectory, type: .exclusive) {
2585+
// strip first level component if needed
2586+
if try self.fileSystem.shouldStripFirstLevel(archiveDirectory: tempExtractionDirectory, acceptableExtensions: BinaryTarget.Kind.allCases.map({ $0.fileExtension })) {
2587+
observabilityScope.emit(debug: "stripping first level component from \(tempExtractionDirectory)")
2588+
try self.fileSystem.stripFirstLevel(of: tempExtractionDirectory)
2589+
} else {
2590+
observabilityScope.emit(debug: "no first level component stripping needed for \(tempExtractionDirectory)")
2591+
}
2592+
let content = try self.fileSystem.getDirectoryContents(tempExtractionDirectory)
2593+
// copy from temp location to actual location
2594+
for file in content {
2595+
let source = tempExtractionDirectory.appending(component: file)
2596+
let destination = parentDirectory.appending(component: file)
2597+
if self.fileSystem.exists(destination) {
2598+
try self.fileSystem.removeFileTree(destination)
2599+
}
2600+
try self.fileSystem.copy(from: source, to: destination)
2601+
if destination.basenameWithoutExt == artifact.targetName {
2602+
artifactPath = destination
2603+
}
2604+
}
2605+
}
2606+
// remove temp location
2607+
try self.fileSystem.removeFileTree(tempExtractionDirectory)
2608+
}
2609+
2610+
guard let mainArtifactPath = artifactPath else {
2611+
return observabilityScope.emit(.artifactNotFound(targetName: artifact.targetName, artifactName: artifact.targetName))
2612+
}
2613+
2614+
result.append(
2615+
.remote(
2616+
packageRef: artifact.packageRef,
2617+
targetName: artifact.targetName,
2618+
url: artifact.url.absoluteString,
2619+
checksum: artifact.checksum,
2620+
path: mainArtifactPath
2621+
)
2622+
)
2623+
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .success(mainArtifactPath), duration: downloadStart.distance(to: .now()))
2624+
case .failure(let error):
2625+
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
2626+
observabilityScope.emit(.artifactFailedExtraction(artifactURL: artifact.url, targetName: artifact.targetName, reason: reason))
2627+
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
2628+
}
2629+
2630+
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
2631+
})
26112632
case .failure(let error):
2612-
let reason = (error as? LocalizedError)?.errorDescription ?? error.localizedDescription
2613-
observabilityScope.emit(.artifactFailedExtraction(artifactURL: artifact.url, targetName: artifact.targetName, reason: reason))
2633+
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
2634+
observabilityScope.emit(.artifactFailedValidation(artifactURL: artifact.url, targetName: artifact.targetName, reason: "\(reason)"))
26142635
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
26152636
}
2616-
2617-
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
26182637
})
26192638
case .failure(let error):
2620-
observabilityScope.emit(.artifactFailedDownload(artifactURL: artifact.url, targetName: artifact.targetName, reason: "\(error)"))
2639+
let reason = (error as? LocalizedError)?.errorDescription ?? "\(error)"
2640+
observabilityScope.emit(.artifactFailedDownload(artifactURL: artifact.url, targetName: artifact.targetName, reason: "\(reason)"))
26212641
self.delegate?.didDownloadBinaryArtifact(from: artifact.url.absoluteString, result: .failure(error), duration: downloadStart.distance(to: .now()))
26222642
}
26232643
})

0 commit comments

Comments
 (0)