Skip to content
New issue

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

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

Already on GitHub? # to your account

[iOS] convert NSNull to nil before checking type in readAsDataURL #46635

Closed
wants to merge 2 commits into from

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented Sep 24, 2024

Summary:

This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

  • Have a response that has an empty body
  • Do not include a content-type header in the response
  • Set the x-content-type-options header to nosniff

RN handles the response for a request in this block of code:

- (id)handleNetworkingResponse:(NSURLResponse *)response data:(NSData *)data
{
// An empty body will have nil for data, in this case we need to return
// an empty blob as per the XMLHttpRequest spec.
data = data ?: [NSData new];
return @{
@"blobId" : [self store:data],
@"offset" : @0,
@"size" : @(data.length),
@"name" : RCTNullIfNil([response suggestedFilename]),
@"type" : RCTNullIfNil([response MIMEType]),
};
}

Here, we see that values of nil - which [response MIMEType] will return when no content-type is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to NSNull by RCTNullIfNil.

When we get back over to readAsDataURL, we see that we grab the type from the dictionary and check if its nil before calling length on the string.

NSString *type = [RCTConvert NSString:blob[@"type"]];
NSString *text = [NSString stringWithFormat:@"data:%@;base64,%@",
type != nil && [type length] > 0 ? type : @"application/octet-stream",
[data base64EncodedStringWithOptions:0]];

However, this check is dubious, because the value will never actually be nil. It will always either be NSString or NSNull because of the RCTNullIfNil call made above and [RCTConvert NSString] seems to just return the input if it is NSNull.

Changelog:

[IOS] [FIXED] - Convert NSNull to nil before checking type in readAsDataURL

Test Plan:

This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

Before

Screen.Recording.2024-09-24.at.2.20.07.PM.mov

After

Screen.Recording.2024-09-24.at.2.23.09.PM.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 24, 2024
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 25, 2024
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 99ab845.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @haileyok in 99ab845

When will my fix make it into a release? | How to file a pick request?

blakef pushed a commit that referenced this pull request Sep 30, 2024
…46635)

Summary:
This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

- Have a response that has an empty body
- Do not include a `content-type` header in the response
- Set the `x-content-type-options` header to `nosniff`

RN handles the response for a request in this block of code: https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTBlobManager.mm#L314-L326

Here, we see that values of `nil` - which `[response MIMEType]` will return when no `content-type` is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to `NSNull` by `RCTNullIfNil`.

When we get back over to `readAsDataURL`, we see that we grab the type from the dictionary and check if its `nil` before calling `length` on the string. https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L74-L77

However, this check is dubious, because the value will never actually be `nil`. It will always either be `NSString` or `NSNull` because of the `RCTNullIfNil` call made above and `[RCTConvert NSString]` seems to just return the input if it is `NSNull`.

## Changelog:

[IOS] [FIXED] - Convert `NSNull` to `nil` before checking `type` in `readAsDataURL`

Pull Request resolved: #46635

Test Plan:
This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

### Before

https://github.com/user-attachments/assets/deb86c68-2251-4fef-9705-a1c93584e83e

### After

https://github.com/user-attachments/assets/9ffab11b-b2c8-4a83-afd6-0a55fed3ae9b

Reviewed By: dmytrorykun

Differential Revision: D63381947

Pulled By: cipolleschi

fbshipit-source-id: b2b4944d998133611592eed8d112faa6195587bd
blakef pushed a commit that referenced this pull request Sep 30, 2024
…46635)

Summary:
This issue original arose out of bluesky-social/social-app#5100. Copying the description (with my general understanding of the problem) from the patch PR to here as well.

There's a crash that comes up in the following, pretty specific scenario:

- Have a response that has an empty body
- Do not include a `content-type` header in the response
- Set the `x-content-type-options` header to `nosniff`

RN handles the response for a request in this block of code: https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTBlobManager.mm#L314-L326

Here, we see that values of `nil` - which `[response MIMEType]` will return when no `content-type` is provided in the response and the actual type cannot be determined (https://developer.apple.com/documentation/foundation/nsurlresponse/1411613-mimetype) - gets converted to `NSNull` by `RCTNullIfNil`.

When we get back over to `readAsDataURL`, we see that we grab the type from the dictionary and check if its `nil` before calling `length` on the string. https://github.com/facebook/react-native/blob/303e0ed7641409acf2d852c077f6be426afd7a0c/packages/react-native/Libraries/Blob/RCTFileReaderModule.mm#L74-L77

However, this check is dubious, because the value will never actually be `nil`. It will always either be `NSString` or `NSNull` because of the `RCTNullIfNil` call made above and `[RCTConvert NSString]` seems to just return the input if it is `NSNull`.

## Changelog:

[IOS] [FIXED] - Convert `NSNull` to `nil` before checking `type` in `readAsDataURL`

Pull Request resolved: #46635

Test Plan:
This is a little awkward to test, but essentially this comes up in the following scenario that is described (and "tested" as being fixed by tweaking) in bluesky-social/social-app#5100. I have personally tested by using Cloudflare rules to add/remove that particular header from an empty body response. You could also test this with a little local web server if you want.

### Before

https://github.com/user-attachments/assets/deb86c68-2251-4fef-9705-a1c93584e83e

### After

https://github.com/user-attachments/assets/9ffab11b-b2c8-4a83-afd6-0a55fed3ae9b

Reviewed By: dmytrorykun

Differential Revision: D63381947

Pulled By: cipolleschi

fbshipit-source-id: b2b4944d998133611592eed8d112faa6195587bd
@blakef blakef mentioned this pull request Oct 30, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants