Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix prefix removal for folder paths with symlinks when copying a folder to S3 #21

Merged

Conversation

billowsofcode
Copy link
Contributor

@billowsofcode billowsofcode commented Mar 11, 2022

Problem

(May be related to open issue: #20)

The folder argument passed to copy(from folder: String, to s3Folder: S3Folder... or sync(from folder: String, to s3Folder: S3Folder,... may be non-standardized and contain symlinks. In this case targetFiles(files: [FileDescriptor], from srcFolder: String, to destFolder: S3Folder) may not remove the folder prefix in all cases. It's because fileEnumerator = FileManager.default.enumerator( in listFiles(in folder: String) may return file paths that are standardized and with different use of symlinks (e.g. /private/var instead of /var).

Example illustrating problem:

let temporaryFolder = try! FileManager.default.url(for: .itemReplacementDirectory,
                                                      in: .userDomainMask,
                                                      appropriateFor: URL(fileURLWithPath: NSString("~").expandingTildeInPath),
                                                      create: true)

let folder = temporaryFolder.appendingPathComponent("somefolder", isDirectory: true).appendingPathComponent("..")
print("folder.path: \(folder.path)")
print("folder resolved (fix): \(folder.standardizedFileURL.resolvingSymlinksInPath().path)")

try FileManager.default.createDirectory(at: folder, withIntermediateDirectories: true)
FileManager.default.createFile(atPath: folder.appendingPathComponent("file1.txt").path, contents: nil)

// assume folder is passed to `copy(from folder: String, to s3Folder: S3Folder...`.  That method will enumerate the files in the folder with paths like this:

guard let fileEnumerator = FileManager.default.enumerator(
    at: folder,
    includingPropertiesForKeys: [.isRegularFileKey],
    options: .skipsHiddenFiles
) else {
    fatalError("failedToEnumerateFolder")
}

while let file = fileEnumerator.nextObject() as? URL {
    if !file.hasDirectoryPath {
        print("file.path: \(file.path)")
        print("file resolved (fix): \(file.resolvingSymlinksInPath().path)")
    }
}

Output:

folder.path: /var/folders/9b/9ht20vfx44d6qy3hzg0_j0jc0000gp/T/TemporaryItems/NSIRD_com.apple.dt.Xcode.PlaygroundStub-macosx_ILT2Uc/somefolder/..
folder resolved (fix): /var/folders/9b/9ht20vfx44d6qy3hzg0_j0jc0000gp/T/TemporaryItems/NSIRD_com.apple.dt.Xcode.PlaygroundStub-macosx_ILT2Uc
file.path: /private/var/folders/9b/9ht20vfx44d6qy3hzg0_j0jc0000gp/T/TemporaryItems/NSIRD_com.apple.dt.Xcode.PlaygroundStub-macosx_ILT2Uc/file1.txt
file resolved (fix): /var/folders/9b/9ht20vfx44d6qy3hzg0_j0jc0000gp/T/TemporaryItems/NSIRD_com.apple.dt.Xcode.PlaygroundStub-macosx_ILT2Uc/file1.txt

Solution

Run the same path standardization methods on both the user supplied folder argument and the enumerated file paths. This way remove prefix will work in targetFiles(

Side-Note

Working with file paths is difficult. Apple recommends to use URL instead of String for file paths. You should switch the API from using String to URL. In your code only convert to a path string (URL.path) when a framework method requires it. https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/LowLevelFileMgmt/Articles/StandardDirectories.html

…er to S3

The local folder argument to a copy or sync to S3 method may be non-standardized (e.g. contain ../) or have symlinks. Removing the folder prefix from listed file paths will not work in such case. This change standardizes and resolves symlinks of both the upload/sync folder argument and the listed file paths in that folder.
@adam-fowler
Copy link
Member

Sorry haven't managed to get to this yet. I'll try tomorrow

@@ -265,7 +265,8 @@ public struct S3FileTransferManager {
return listFiles(in: folder)
.flatMap { files in
let taskQueue = TaskQueue<Void>(maxConcurrentTasks: configuration.maxConcurrentTasks, on: eventLoop)
let transfers = Self.targetFiles(files: files, from: folder, to: s3Folder)
let folderResolved = URL(fileURLWithPath: folder).standardizedFileURL.resolvingSymlinksInPath()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you don't put this inside of targetFiles

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I had this inside of targetFiles initially, but then I read the comments for all 3 targetFiles methods that say Function assumes the files have srcFolder prefixed. So I didn't want to change the assumption for one of them. But I can move let folderResolved = URL... inside targetFiles if you wish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

@adam-fowler adam-fowler merged commit 74899d6 into soto-project:main Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants