-
Notifications
You must be signed in to change notification settings - Fork 871
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
WebTorrent: Support rendering images, PDFs, more media formats, and text-like files #3053
Conversation
…ext-like files Fixes: brave/brave-browser#5326 Previously, images and PDFs would not render at all. Other formats would render in a very tiny iframe which was unusable. Unsupported formats would render as gibberish text. Now, we properly support images, centered, with a black background behind them, just like for audio and video. For PDFs, we use the native built-in PDF viewer and render at 100% width and height. For text-like files, we render them in a full-screen iframe with the default white page background. For audio, we now support FLAC, M4B, M4P, and OGA formats. The browser still has to support the specific codec used within the file, but we'll render an <audio> tag and at least attempt to play it.
d84ebff
to
64822b6
Compare
@@ -30,7 +30,7 @@ export const createServer = (torrent: WebTorrent.Torrent, cb: (serverURL: string | |||
origin: window.location.origin, | |||
// Use hostname option to mitigate DNS rebinding | |||
// Ref: https://github.com/brave/browser-laptop/issues/12616 | |||
hostname: 'localhost' | |||
hostname: '127.0.0.1' |
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.
Now that the hostname
option is being enforced properly, we need to change this to 127.0.0.1
so it matches the way that we refer to the files in the download links and <video>
, <audio>
, <iframe>
, etc. src=
urls.
|
||
const SUPPORTED_PDF_EXTENSIONS = [ | ||
'pdf' | ||
] |
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.
PDFs are no longer displayed in an iframe, so we break them into their own type.
) | ||
} else if (fileIsType(file, 'pdf')) { | ||
content = ( | ||
<object id='object' type='application/pdf' data={fileURL} /> |
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.
PDFs use the <object>
tag so that we can use the browser's built-in PDF viewer.
) | ||
} else { | ||
content = ( | ||
<div>Unsupported file type</div> |
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.
We no longer attempt to render files not explicitly included in the list above. This prevents non-ascii files from rendering as text and showing up as gibberish.
@@ -20,6 +20,6 @@ | |||
"listen": "*:*" | |||
} | |||
}, | |||
"content_security_policy": "default-src 'self'; connect-src 'self' http: https:; font-src 'self' data:; script-src 'self'; media-src 'self' http://127.0.0.1:*; form-action 'none'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; frame-src 'self' http://127.0.0.1:*;", | |||
"content_security_policy": "default-src 'self'; connect-src 'self' http: https:; font-src 'self' data:; script-src 'self'; media-src 'self' http://127.0.0.1:*; form-action 'none'; style-src 'self' 'unsafe-inline'; img-src 'self' data: http://127.0.0.1:*; object-src 'self' http://127.0.0.1:*; frame-src 'self' http://127.0.0.1:*;", |
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.
Two changes here:
- Images need to load from
http://127.0.0.1:*
- We allow use of the
object
tag , loading from'self'
andhttp://127.0.0.1:*
defer to @yrliou for review but these changes look reasonable security-wise |
Thanks for the review, @diracdeltas! |
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
Fixes: brave/brave-browser#5326
Previously, images and PDFs would not render at all. Other formats would render in a very tiny iframe which was unusable. Unsupported formats would render as gibberish text.
Now, we properly support images, centered, with a black background behind them, just like for audio and video.
For PDFs, we use the native built-in PDF viewer and render at 100% width and height.
For text-like files, we render them in a full-screen iframe with the default white page background.
For audio, we now support FLAC, M4B, M4P, and OGA formats. The browser still has to support the specific codec used within the file, but we'll render an tag and at least attempt to play it.
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.