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

TransferUtility.DownloadDirectoryAsync throws an exception under certain specific conditions #3017

Closed
rbluff opened this issue Aug 2, 2023 · 9 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue queued s3

Comments

@rbluff
Copy link

rbluff commented Aug 2, 2023

Describe the bug

TransferUtility.DownloadDirectoryAsync(...) throws a AmazonClientException if one of the files inside the S3 directory has a filename that contains a tilde ~ character and is in 8:3 format AND the case of the local destination directory path supplied to TransferUtilityDownloadDirectoryRequest.LocalDirectory does not match the actual case of the directory path on the system.

Expected Behavior

I expected TransferUtility.DownloadDirectoryAsync(...) to successfully download the S3 directory to the local directory without errors.

Current Behavior

Amazon.Runtime.AmazonClientException: The file C:\Windows\TEMP\abc\Imports\01_4791\Job52\files\Audio\vc~ended.caf is not allowed outside of the target directory C:\Windows\TEMP\abc\Imports\01_4791\Job52.\r\n at Amazon.S3.Transfer.Internal.DownloadDirectoryCommand.ConstructTransferUtilityDownloadRequest(S3Object s3Object, Int32 prefixLength) in D:\JenkinsWorkspaces\trebuchet-stage-release\AWSDotNetPublic\sdk\src\Services\S3\Custom\Transfer\Internal\DownloadDirectoryCommand.cs:line 105\r\n at Amazon.S3.Transfer.Internal.DownloadDirectoryCommand.d__22.MoveNext() in D:\JenkinsWorkspaces\trebuchet-stage-release\AWSDotNetPublic\sdk\src\Services\S3\Custom\Transfer\Internal\_bcl45+netstandard\DownloadDirectoryCommand.cs:line 105\r\n--- End of stack trace from previous location where exception was thrown ---

Note that the filename of the file in S3 (vc~ended.caf) contains 8 characters in the filename and 3 characters in the extension, and also contains a tilde ~ character.

The directory path supplied to TransferUtilityDownloadDirectoryRequest.LocalDirectory was C:\Windows\TEMP\abc\Imports\01_4791\Job52 but the actual case of the temp folder on the system is C:\Windows\Temp. ie. the supplied path contained TEMP but the actual path on the system was Temp.

Reproduction Steps

public async Task DownloadDirectoryAsync(string sourceDirectoryKey, string destinationDirectoryPath)
{
	var directoryTransferUtility =
		new TransferUtility(Client);

	var request = new TransferUtilityDownloadDirectoryRequest
	{
		BucketName = _context.BucketName,
		S3Directory = sourceDirectoryKey,
		LocalDirectory = destinationDirectoryPath,
		DownloadFilesConcurrently = true,
	};

	await directoryTransferUtility.DownloadDirectoryAsync(request).ConfigureAwait(false);
}

The key requirements to reproduce the bug are:

  1. The S3 directory referred to by sourceDirectoryKey must contain a file with a filename in 8:3 format, and with a tilde character in the filename. eg. vc~ended.caf or tm~tilde.tmp
  2. The destinationDirectoryPath should have a case that is different to the actual case of the directory path on the system. For example, you could call destinationDirectoryPath.ToLowerInvariant() to achieve this.

Possible Solution

DownloadDirectoryCommand.ConstructTransferUtilityDownloadRequest(...) contains the following check

if (!InternalSDKUtils.IsFilePathRootedWithDirectoryPath(utilityDownloadRequest.FilePath, this._request.LocalDirectory))
        throw new AmazonClientException("The file " + utilityDownloadRequest.FilePath + " is not allowed outside of the target directory " + this._request.LocalDirectory + ".");

and the source of the IsFilePathRootedWithDirectoryPath method is as follows:

/// <summary>
    /// Tests if the filePath is rooted with the directoryPath when resolved. The filePath and
    /// directoryPath do not need to exist to call this method.
    /// </summary>
    /// <param name="filePath">The filePath to test against the directoryPath.</param>
    /// <param name="directoryPath">The directoryPath to use as root in the test.</param>
    /// <returns>true if directoryPath is root of filePath else false</returns>
    public static bool IsFilePathRootedWithDirectoryPath(string filePath, string directoryPath)
    {
      string path = directoryPath;
      if (!path.EndsWith(Path.DirectorySeparatorChar.ToString()))
        path += Path.DirectorySeparatorChar.ToString();
      DirectoryInfo directoryInfo = new DirectoryInfo(path);
      return new FileInfo(filePath).FullName.StartsWith(directoryInfo.FullName);
    }

If filePath refers to a file with a filename in 8:3 format and that contains a tilde character, then FileInfo(filePath).FullName will contain the path with the correct case of the directory on the system, even if filePath contains a path with the incorrect case.

eg. new FileInfo(@"C:\Windows\TEMP\vc~ended.caf").FullName will return C:\Windows\Temp\vc~ended.caf if C:\Windows\Temp is the correct case of the directory path.

Instead of return new FileInfo(filePath).FullName.StartsWith(directoryInfo.FullName); you could use return new FileInfo(filePath).FullName.StartsWith(directoryInfo.FullName, StringComparison.InvariantCultureIgnoreCase);

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.S3 3.7.106.1

Targeted .NET Platform

.NET Standard 2.0

Operating System and version

Windows 10, Windows Server 2019

@rbluff rbluff added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2023
@rbluff rbluff changed the title (short issue description) TransferUtility.DownloadDirectoryAsync throws an exception under certain specific conditions Aug 2, 2023
@rbluff
Copy link
Author

rbluff commented Aug 2, 2023

My current workaround is to fix the case of the supplied destination directory path.

public async Task DownloadDirectoryAsync(string sourceDirectoryKey, string destinationDirectoryPath)
{
	var directoryTransferUtility =
		new TransferUtility(Client);

	var request = new TransferUtilityDownloadDirectoryRequest
	{
		BucketName = _context.BucketName,
		S3Directory = sourceDirectoryKey,
		LocalDirectory = GetDirectoryPathWithCorrectCase(destinationDirectoryPath),
		DownloadFilesConcurrently = true,
	};

	await directoryTransferUtility.DownloadDirectoryAsync(request).ConfigureAwait(false);
}

/// <summary>
/// Returns the supplied directory path, adjusted to reflect the
/// actual case of the path on the system. This can be used to avoid
/// errors when downloading a directory from S3 to a local directory.
/// </summary>
private string GetDirectoryPathWithCorrectCase(string directoryPath)
{
	if (String.IsNullOrWhiteSpace(directoryPath) || !Directory.Exists(directoryPath))
	{
		return directoryPath;
	}

	// using a filename in 8:3 format and with a tilde ~ character forces
	// the path returned by FileInfo.Fullname to match the actual case of the directory path 
	// in the system
	var nonExistentFileInfo = new FileInfo(Path.Combine(directoryPath, "tm~tilde.tmp"));
	
	return Path.GetDirectoryName(nonExistentFileInfo.FullName);
}

@ashishdhingra ashishdhingra added s3 needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2023
@ashishdhingra ashishdhingra self-assigned this Aug 2, 2023
@ashishdhingra
Copy link
Contributor

Needs reproduction. The exception is thrown at

throw new AmazonClientException($"The file {downloadRequest.FilePath} is not allowed outside of the target directory {_request.LocalDirectory}.");
.

@ashishdhingra
Copy link
Contributor

ashishdhingra commented Aug 3, 2023

Reproducible on Windows.

The issue appears to be with System.IO.FileInfo at InternalSDKUtils.IsFilePathRootedWithDirectoryPath() where:

  • For normal file (e.g. Somefile.txt), it returns C:\Windows\TEMP\SomeDirectory\Somefile.txt in FullName property.
  • For file containing ~ character (e.g. vc~ended.caf), it returns C:\Windows\Temp\SomeDirectory\vc~ended.caf in FullName property (the OriginalPath property of FileInfo object has casing supplied by user).

We should not use StringComparison.InvariantCultureIgnoreCase (as proposed in issue description) since on Linux, the file system is case sensitive, and we would want to match the exact directory name.

Needs review with the team.

@rbluff Please advise if you are unable to use Path.GetTempPath() for your use case as workaround, as it is the recommended best practice. Kindly note that it returns path for current user's temp folder.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue needs-review and removed needs-reproduction This issue needs reproduction. labels Aug 3, 2023
@ashishdhingra
Copy link
Contributor

The proposed solution could be that on Windows we could use StringComparison.InvariantCultureIgnoreCase in

var fileInfo = new FileInfo(filePath);
, for other platforms it should use existing logic.

@rbluff
Copy link
Author

rbluff commented Aug 9, 2023

Thanks for your reply @ashishdhingra. The destination path that is passed to our DownloadDirectoryAsync method is arbitrary - it could be the user's temp path or some other directory path.

@ashishdhingra ashishdhingra removed their assignment Nov 2, 2023
@ashishdhingra
Copy link
Contributor

Repro Steps:

  • In S3 bucket, create a folder named DownloadDirectory-Windows83-Test.
  • Upload 2 text files named basic.txt and vc~ended.txt (this one has Windows 8:3 name) to the above folder.
  • Using code below, try to download file to where actual directory on Windows file system is D:\temp.
    TransferUtility directoryTransferUtility = new TransferUtility(amazonS3Client);
    TransferUtilityDownloadDirectoryRequest request = new TransferUtilityDownloadDirectoryRequest
    {
        BucketName = bucketName,
        S3Directory = "DownloadDirectory-Windows83-Test",
        LocalDirectory = @"D:\TEMP\abc\Imports\01_4791\Job52",
        DownloadFilesConcurrently = true,
    };
    await directoryTransferUtility.DownloadDirectoryAsync(request).ConfigureAwait(false);
    

We cannot access OriginalPath property since it is protected in base FileSystemInfo class.

So the only option appears to be here is to update property documentation for TransferUtilityDownloadDirectoryRequest.LocalDirectory to add a note that the path is case-sensitive and users should specify it in exact case as in the file system.

Kindly note that on Windows, there is a registry setting which when set, makes file system case-sensitive (just like Linux). Refer Enabling Case Sensitivity on Windows 10 for example, which has example command fsutil.exe file SetCaseSensitiveInfo C:\folder\path enable.

@ashishdhingra
Copy link
Contributor

Created PR #3509 to update the API documentation for TransferUtilityDownloadDirectoryRequest.LocalDirectory mentioning that it is case sensitive for certain platforms/scenarios.

@ashishdhingra
Copy link
Contributor

@rbluff Please refer to #3017 (comment). On Windows, there is a registry setting which when set, makes file system case-sensitive (just like Linux) (refer Enabling Case Sensitivity on Windows 10 for example, which has example command fsutil.exe file SetCaseSensitiveInfo C:\folder\path enable). So it might not be feasible to handle your use case:

  • On Windows, which in most cases has case insensitive file system, but due to registry setting, could become case-sensitive; and
  • Underlying .NET API doesn't expose OriginalPath property.

We have updated the API reference documentation for TransferUtilityDownloadDirectoryRequest.LocalDirectory mentioning that The correct casing to the actual path must be used..

Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue queued s3
Projects
None yet
Development

No branches or pull requests

2 participants