-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Prevent crash from invalid media URLs #54834
Conversation
Size Change: 0 B Total Size: 1.62 MB ℹ️ View Unchanged
|
Flaky tests detected in a3e958f6cf1f56fb1c60aa8050bc805711963dca. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6316142778
|
ca09707
to
1907faf
Compare
The `isURL` utility considers URLs using uncommon protocols to be valid, as constructing `URL` with, say, `h://a-path.com` is valid. Contrastingly, the `react-native-video` package only considers a URL using the `http:` or `https:` protocols to be a "network" asset. This leads to an attempt to load the URL as a local asset, which fails as the asset does not exist in the app bundle. https://github.com/react-native-video/react-native-video/blob/81ae785090f5ce3d5e45cc51f68a3360a85be853/Video.js#L277
1907faf
to
0a540b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎊 !
I tested on both platforms (iOS and Android) but I could only reproduce the issue on iOS. On Android, the editor doesn't crash and when trying to play the video, it displays the message "Problem opening the video".
Correct, I should've noted that. Thank you for calling this out.
|
What?
Prevent crashes resulting from attempts to set an invalid URL for a video or
audio file, e.g.
h://domain.org/video.mp4
.Why?
Fixes wordpress-mobile/gutenberg-mobile#6233. The crash is disruptive for users.
How?
Detect and avoid sending URLs that do not begin with
http:
orhttp://
to thereact-native-video
package as it will attempt and fail to load the URL as alocal app bundle resource. Instead, display a notice to the user that the
provided URL is invalid.
The
isURL
utility considers URLs using uncommon protocols to be valid,as constructing
URL
with, say,h://a-path.com
is valid.Contrastingly, the
react-native-video
package only considers a URLusing the
http:
orhttps:
protocols to be a "network" asset. Thisleads to an attempt to load the URL as a local asset, which fails as the
asset does not exist in the app bundle.
Testing Instructions
h://wordpress.org/video.mp4
.Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a