Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions FirebaseStorage/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Unreleased

- [added] Added a check to FIRStorageUploadTask's `putFile:` to check if the passed in `fileURL` is a directory, and provides a clear error if it is. (#5750)

# 3.6.1
- [fixed] Fix a rare case where a StorageTask would call its completion callbacks more than
once. (#5245)
Expand Down
17 changes: 14 additions & 3 deletions FirebaseStorage/Sources/FIRStorageUploadTask.m
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,14 @@ - (BOOL)isContentToUploadValid:(NSError **)outError {
}

NSError *fileReachabilityError;
if (![_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError]) {
if (![_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError] ||
![self fileURLisFile:_fileURL]) {
Comment on lines +196 to +197
Copy link
Member Author

Choose a reason for hiding this comment

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

The checkResourceIsReachableAndReturnError returns true or false depending on if the _fileURL can be reached. This is why attempts to upload directories weren't being caught in the first place, because they are reachable. Instead of doing a check in putFile:, I figured I should move the fileURLisFile helper method to FIRStorageUploadTask.m and call it in this isContentToUploadValid method so we have a single check to verify that the file we are trying to upload is both a reachable resource and it is a file.

if (outError != NULL) {
NSMutableDictionary *userInfo = [NSMutableDictionary dictionaryWithCapacity:2];
userInfo[NSLocalizedDescriptionKey] =
[NSString stringWithFormat:@"File at URL: %@ is not reachable.", _fileURL.absoluteString];
userInfo[NSLocalizedDescriptionKey] = [NSString
stringWithFormat:@"File at URL: %@ is not reachable. "
@"Ensure file URL is not a directory, symbolic link, or invalid url.",
_fileURL.absoluteString];
Comment on lines +200 to +203
Copy link
Member Author

Choose a reason for hiding this comment

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

I added some extra context to the error since we are now explicitly checking if the url points to a file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!


if (fileReachabilityError) {
userInfo[NSUnderlyingErrorKey] = fileReachabilityError;
Expand Down Expand Up @@ -257,4 +260,12 @@ - (void)resume {
}];
}

#pragma mark - Private Helpers

- (BOOL)fileURLisFile:(NSURL *)fileURL {
NSNumber *isFile = [NSNumber numberWithBool:NO];
[fileURL getResourceValue:&isFile forKey:NSURLIsRegularFileKey error:nil];
return [isFile boolValue];
}

@end
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The `HomeImprovement.numbers` directory exists to test Firebase Storage's ability to fail gracefully when one attempts to upload a directory. Since git doesn't track track empty directories. This .txt file exists to provide context and make git track the folder.
9 changes: 7 additions & 2 deletions FirebaseStorage/Tests/Unit/FIRStorageReferenceTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@ - (void)testReferenceWithNonExistentFileFailsWithCompletion {
XCTAssertEqualObjects(error.domain, FIRStorageErrorDomain);
XCTAssertEqual(error.code, FIRStorageErrorCodeUnknown);
NSString *expectedDescription = [NSString
stringWithFormat:@"File at URL: %@ is not reachable.", dummyFileURL.absoluteString];
stringWithFormat:@"File at URL: %@ is not reachable. "
@"Ensure file URL is not a directory, symbolic link, or invalid url.",
dummyFileURL.absoluteString];
XCTAssertEqualObjects(error.localizedDescription, expectedDescription);
}];

Expand All @@ -204,7 +206,10 @@ - (void)testReferenceWithNilFileURLFailsWithCompletion {

XCTAssertEqualObjects(error.domain, FIRStorageErrorDomain);
XCTAssertEqual(error.code, FIRStorageErrorCodeUnknown);
NSString *expectedDescription = @"File at URL: (null) is not reachable.";
NSString *expectedDescription = [NSString
stringWithFormat:@"File at URL: %@ is not reachable. "
@"Ensure file URL is not a directory, symbolic link, or invalid url.",
dummyFileURL.absoluteString];
XCTAssertEqualObjects(error.localizedDescription, expectedDescription);
}];

Expand Down
3 changes: 2 additions & 1 deletion FirebaseStorageSwift.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ Firebase Storage provides robust, secure file uploads and downloads from Firebas
int_tests.requires_app_host = true
# Resources are shared with FirebaseStorage's integration tests.
int_tests.resources = 'FirebaseStorage/Tests/Integration/Resources/1mb.dat',
'FirebaseStorage/Tests/Integration/Resources/GoogleService-Info.plist'
'FirebaseStorage/Tests/Integration/Resources/GoogleService-Info.plist',
'FirebaseStorage/Tests/Integration/Resources/HomeImprovement.numbers'
int_tests.dependency 'FirebaseAuth', '~> 6.5'
end
end
22 changes: 22 additions & 0 deletions FirebaseStorageSwift/Tests/Integration/StorageIntegration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,18 @@ class StorageIntegration: XCTestCase {
waitForExpectations()
}

func testAttemptToUploadDirectoryShouldFail() throws {
// This `.numbers` file is actually a directory.
let fileName = "HomeImprovement.numbers"
let bundle = Bundle(for: StorageIntegration.self)
let fileURL = try XCTUnwrap(bundle.url(forResource: fileName, withExtension: ""),
"Failed to get filePath")
let ref = storage.reference(withPath: "ios/public/" + fileName)
ref.putFile(from: fileURL) { result in
self.assertResultFailure(result)
}
}

func testPutFileWithSpecialCharacters() throws {
let expectation = self.expectation(description: #function)

Expand Down Expand Up @@ -607,4 +619,14 @@ class StorageIntegration: XCTestCase {
XCTFail("Unexpected error \(error)")
}
}

private func assertResultFailure<T>(_ result: Result<T, Error>,
file: StaticString = #file, line: UInt = #line) {
switch result {
case let .success(value):
XCTFail("Unexpected success with value: \(value)")
case let .failure(error):
XCTAssertNotNil(error, file: file, line: line)
}
}
}