Skip to content

FIRStorageUploadTask: fail on invalid file URL #2458

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Mar 4, 2019
Merged

FIRStorageUploadTask: fail on invalid file URL #2458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bd55a09
FIRStorageUploadTask: validate data and fileURL before upload (#2350)
maksymmalyhin Feb 27, 2019
ef986b9
FIRStorageReferenceTests: Verify a specific error (#2350)
maksymmalyhin Feb 27, 2019
0f11535
Code cleanup. More informative error.
maksymmalyhin Feb 28, 2019
92bca47
FIRStorageReference: error message, test fix (#2350)
maksymmalyhin Feb 28, 2019
fc70bdb
Run ./scripts/style.sh
maksymmalyhin Feb 28, 2019
5009de3
#2350: Punctuation
maksymmalyhin Feb 28, 2019
6a731ae
FIRStorageReferenceTests: naming (#2350)
maksymmalyhin Feb 28, 2019
c335c43
Run ./scripts/style.sh
maksymmalyhin Feb 28, 2019
3dbf22c
Tests fixed (#2350)
maksymmalyhin Feb 28, 2019
1155787
FIRStorageReferenceTests fixed for Mac OS (#2350)
maksymmalyhin Feb 28, 2019
6c4ee95
Merge branch 'master' into mm-2350-putFileCompletion
maksymmalyhin Mar 3, 2019
d376ee1
FIRStorageUploadTask: validate content just before uploading (#2350)
maksymmalyhin Mar 3, 2019
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
25 changes: 25 additions & 0 deletions Example/Storage/Tests/Unit/FIRStorageReferenceTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,4 +162,29 @@ - (void)testCopy {
XCTAssertNotEqual(ref, copiedRef);
}

- (void)testReferenceWithNonExistentFileFailsWithCompletion {
NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:@"temp.data"];
FIRStorageReference *ref = [self.storage referenceWithPath:tempFilePath];

NSURL *dummyFileURL = [NSURL fileURLWithPath:@"some_non_existing-folder/file.data"];

XCTestExpectation *expectation = [self expectationWithDescription:@"completionExpectation"];

[ref putFile:dummyFileURL
metadata:nil
completion:^(FIRStorageMetadata *_Nullable metadata, NSError *_Nullable error) {
[expectation fulfill];
XCTAssertNotNil(error);
XCTAssertNil(metadata);

XCTAssertEqualObjects(error.domain, FIRStorageErrorDomain);
XCTAssertEqual(error.code, FIRStorageErrorCodeUnknown);
NSString *expectedDescription = [NSString
stringWithFormat:@"File at URL: %@ is not reachable.", dummyFileURL.absoluteString];
XCTAssertEqualObjects(error.localizedDescription, expectedDescription);
}];

[self waitForExpectationsWithTimeout:0.5 handler:NULL];
}

@end
47 changes: 41 additions & 6 deletions Firebase/Storage/FIRStorageUploadTask.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ - (void)enqueue {
return;
}

NSError *contentValidationError;
if (![strongSelf isContentToUploadValid:&contentValidationError]) {
strongSelf.error = contentValidationError;
[strongSelf finishTaskWithStatus:FIRStorageTaskStatusFailure snapshot:strongSelf.snapshot];
return;
}

strongSelf.state = FIRStorageTaskStateQueueing;

NSMutableURLRequest *request = [strongSelf.baseRequest mutableCopy];
Expand Down Expand Up @@ -145,9 +152,8 @@ - (void)enqueue {
self.state = FIRStorageTaskStateFailed;
self.error = [FIRStorageErrors errorWithServerError:error reference:self.reference];
self.metadata = self->_uploadMetadata;
[self fireHandlersForStatus:FIRStorageTaskStatusFailure snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;

[self finishTaskWithStatus:FIRStorageTaskStatusFailure snapshot:self.snapshot];
return;
}

Expand All @@ -164,9 +170,7 @@ - (void)enqueue {
self.error = [FIRStorageErrors errorWithInvalidRequest:data];
}

[self fireHandlersForStatus:FIRStorageTaskStatusSuccess snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;
[self finishTaskWithStatus:FIRStorageTaskStatusSuccess snapshot:self.snapshot];
};
#pragma clang diagnostic pop

Expand All @@ -177,6 +181,37 @@ - (void)enqueue {
}];
}

- (void)finishTaskWithStatus:(FIRStorageTaskStatus)status
snapshot:(FIRStorageTaskSnapshot *)snapshot {
[self fireHandlersForStatus:status snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;
}

- (BOOL)isContentToUploadValid:(NSError **)outError {
if (_uploadData != nil) {
return YES;
}

NSError *fileReachabilityError;
if (![_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError]) {
if (outError != NULL) {
NSString *description =
[NSString stringWithFormat:@"File at URL: %@ is not reachable.", _fileURL.absoluteString];
*outError = [NSError errorWithDomain:FIRStorageErrorDomain
code:FIRStorageErrorCodeUnknown
Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

I think this is fine for now, but we probably want to add a case to the to the FIRStorageErrorCode enum.

That will require an API review (I can go over the process with you) so we won't be able to do it this release.

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Feb 28, 2019

Choose a reason for hiding this comment

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

I may create an issue for that so we don't forget about it.

Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

Once this is approved by the Storage team and lands, that's a great idea.

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Feb 28, 2019

Choose a reason for hiding this comment

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

Created - #2459

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Mar 2, 2019

Choose a reason for hiding this comment

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

Just throwing this out there: Did you consider FIRStorageErrorCodeObjectNotFound? It's used today when a file doesn't exist on the backend, which isn't all that different from a locally missing file.

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 3, 2019

Choose a reason for hiding this comment

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

My concern with FIRStorageErrorCodeObjectNotFound is following: a typical file upload operation involves a remote object reference and a file URL, so if we have a single error code for both it may be confusing for users.

userInfo:@{
NSUnderlyingErrorKey : fileReachabilityError,
Copy link

@LinusU LinusU Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to upload an mp4 file from Flutter with firebase_storage (got the file from image_picker, seems to work fine with jpeg from image_picker) and I'm hitting an app crash here.

fileReachabilityError is nil and thus I'm getting an NSInvalidException since you cannot add nil values to an NSDitctionary.

For some reason _fileURL is nil, which makes [_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError] return just nil, which triggers the if (!...) branch.

Sorry for not providing too much info, I'm still in the process of tracking it down but thought it would be good to see if anyone had any ideas ☺️

ping @maksymmalyhin

edit: the problem is that both _uploadData and _fileURL is null, because it was created with a nil data from Flutter

edit2: the problem is that image_picker returns a File object with a path like /file:///foo/bar/baz.mp4...

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 17, 2019

Choose a reason for hiding this comment

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

@LinusU Thank you for the report. That's a good case. Yeah, this part of code definitely does not expect _fileURL to be nil. We will fix it.

In the meanwhile, would you be able to post a comment about it to #2350 or create a separate ticket, so it is not lost.

As a workaround you should check the fileURL != nil before passing it to the putFile method.

Copy link

@LinusU LinusU Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've submitted flutter/plugins#1504 and flutter/plugins#1505 to fix this in Flutter, but would be great to see a fix here as well ☺️

I'll put a comment in the issue you linked open a new issue about it here as well!

edit: here we go 👉 #2852

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 17, 2019

Choose a reason for hiding this comment

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

@LinusU thanks!

NSLocalizedDescriptionKey : description
}];
}

return NO;
}

return YES;
}

#pragma mark - Upload Management

- (void)cancel {
Expand Down