Add .torrent support #405
Add .torrent support #405
Conversation
yrliou
force-pushed
the
torrent_support
branch
6 times, most recently
from
160977d to
4863b33
Compare
browser/net/BUILD.gn
Outdated
| "url_context.h" | ||
| "url_context.h", | ||
| "//brave/components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper.cc", | ||
| "//brave/components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper.h", |
There was a problem hiding this comment.
These two files have been moved into a separate source set inside brave_webtorrent component by cf81e8d
| @@ -27,6 +28,9 @@ struct BraveRequestInfo { | |||
| uint64_t request_identifier = 0; | |||
| size_t next_url_request_index = 0; | |||
| net::HttpRequestHeaders* headers = nullptr; | |||
| const net::HttpResponseHeaders* original_response_headers = nullptr; | |||
| scoped_refptr<net::HttpResponseHeaders>* override_response_headers = nullptr; | |||
| GURL* allowed_unsafe_redirect_url = nullptr; | |||
There was a problem hiding this comment.
Not needed for now, but just noting it might be cleaner to make a subclass with these extra members instead and cast as needed in the headers received code.
There was a problem hiding this comment.
Sounds good, I'll create a small follow-up issue to do this if that's fine with you.
There was a problem hiding this comment.
Just an issue in the backlog is fine for now.
| // Handle http and https here for making reverse_on_redirect to be true in | ||
| // BrowserURLHandlerImpl::RewriteURLIfNecessary to trigger ReverseURLRewrite | ||
| // for updating the virtual URL. | ||
| if (url->SchemeIs("http") || url->SchemeIs("https") || |
| return false; | ||
| } | ||
|
|
||
| if (mimeType == "application/x-bittorrent") { |
There was a problem hiding this comment.
nit: pls add this to common/network_constants.h and use the constant here and in tests
| return false; | ||
| } | ||
|
|
||
| bool URLMatched(net::URLRequest* request) { |
There was a problem hiding this comment.
nit: this would be easy to add a unittest for
There was a problem hiding this comment.
These util functions are covered in BraveTorrentRedirectNetworkDelegateHelperTest unit tests.
| return false; | ||
| } | ||
|
|
||
| net::HttpContentDisposition cd_headers(disposition, std::string()); |
There was a problem hiding this comment.
++ on using HttpContentDisposition since the header can be complex like
Content-Disposition: attachment; filename=genome.jpeg; modification-date="Wed, 12 Feb 1997 16:29:51 -0500";
| return true; | ||
| } | ||
|
|
||
| if (mimeType == "application/octet-stream" && |
There was a problem hiding this comment.
nit: I prefer you do a constant for this one too for use here and in tests
| return false; | ||
| } | ||
|
|
||
| bool IsWebtorrentInitiated(net::URLRequest* request) { |
There was a problem hiding this comment.
nit: unit test as well, you can adjust the signature to pass a const GURL & to make it easier.
| request->url().spec()})); | ||
| (*override_response_headers)->AddHeader( | ||
| "Location: " + url.spec()); | ||
| *allowed_unsafe_redirect_url = url; |
|
|
||
| *override_response_headers = | ||
| new net::HttpResponseHeaders(original_response_headers->raw_headers()); | ||
| (*override_response_headers)->ReplaceStatusLine("HTTP/1.1 301 Moved Permanently"); |
There was a problem hiding this comment.
Should we use 302 here instead?
There was a problem hiding this comment.
Maybe 307 if we ever want to also preserve the HTTP method.
There was a problem hiding this comment.
Was following browser-laptop for using 301, but 307 Temporary Redirect seems more reasonable to me, so I'll change it to 307.
|
|
||
| bool FileNameMatched(const net::HttpResponseHeaders* headers) { | ||
| std::string disposition; | ||
| if (!headers->GetNormalizedHeader("content-disposition", &disposition)) { |
There was a problem hiding this comment.
nit: I know header names are case insensitive but prefer "Content-Disposition" since it's more canonical.
| @@ -7,7 +7,7 @@ | |||
| "scripts": ["extension/out/brave_webtorrent_background.bundle.js"], | |||
| "persistent": true | |||
| }, | |||
| "permissions": ["tabs", "windows", "http://127.0.0.1:*/*"], | |||
There was a problem hiding this comment.
cc @diracdeltas for security review
There was a problem hiding this comment.
seems ok since <all_urls> is required in the browser-laptop extension
| @@ -20,6 +20,6 @@ | |||
| "listen": "*:*" | |||
| } | |||
| }, | |||
| "content_security_policy": "default-src 'self'; connect-src 'self'; 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:*;", | |||
There was a problem hiding this comment.
cc @diracdeltas for security review
There was a problem hiding this comment.
@yrliou could you explain why this change is needed? it isn't in the browser-laptop version of webtorrent so i wonder if we can work around it.
There was a problem hiding this comment.
When we click start torrent, the background page of webtorrent component extension will initiate a request to fetch the actual .torrent, for example, from https://webtorrent.io/torrents/big-buck-bunny.torrent
Without this change, I'm getting "Refused to connect to 'https://webtorrent.io/torrents/big-buck-bunny.torrent' because it violates the following Content Security Policy directive: "connect-src 'self'". in the background page.
I think for the browser-laptop case, these things are happened in browser process instead of the extension.
There was a problem hiding this comment.
@yrliou thanks for the explanation. Have you tried seeing if this works with an HTTP (not HTTPS) endpoint? I'm afraid Chromium might block that for being mixed content even if it is whitelisted in CSP.
If so we should take another approach like downloading the .torrent file to a tempfile (which would remove the need to whitelist http/https connections in the CSP)
There was a problem hiding this comment.
@diracdeltas I just tried fetching things from HTTP endpoint in the background page using simple-get(https://github.com/feross/simple-get), which is the same as how webtorrent libs make those requests.
Adding http: in CSP's connect-src did allow us to fetch things normally. Without http: specified, same error as above is shown .
There was a problem hiding this comment.
Tried with http://www.slackware.com/torrents/slackware-14.2-install-d1.torrent, .torrent file was successfully fetched.
package.json
Outdated
| @@ -126,6 +126,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@types/parse-torrent": "^5.8.2", | |||
| "@types/query-string": "^6.1.0", | |||
There was a problem hiding this comment.
cc @diracdeltas for security review
package.json
Outdated
| @@ -135,6 +136,7 @@ | |||
| "http-node": "^1.2.0", | |||
| "prettier-bytes": "^1.0.4", | |||
| "qr-image": "^3.2.0", | |||
| "query-string": "^6.1.0", | |||
There was a problem hiding this comment.
cc @diracdeltas for security review
There was a problem hiding this comment.
is it not possible here to use node's builtin querystring module? https://nodejs.org/api/querystring.html
There was a problem hiding this comment.
Sorry, my bad, I'll revert 86fb70b and use this one instead.
| @@ -14,6 +14,7 @@ transpile_web_ui("brave_webtorrent") { | |||
| "background/actions/webtorrentActions.ts", | |||
| "background/actions/windowActions.ts", | |||
| "background/api/tabs_api.ts", | |||
| "background/api/torrent_api.ts", | |||
There was a problem hiding this comment.
@cezaraugusto please take a look at this commit changes for review
|
There was a problem hiding this comment.
left a nit, please also check https://github.com/brave/brave-core/pull/405/files#r216344528.
Other than that front-end LGTM
| try { | ||
| const { name, infoHash, ix } = ParseTorrent(torrentId) | ||
| newInfoHash = infoHash | ||
| newTorrentState = { tabId, torrentId, name, infoHash, ix } | ||
| } catch (error) { | ||
| newTorrentState = { tabId, torrentId, errorMsg: error.message } | ||
| } | ||
| } else if (parsedURL.protocol === 'https:' || parsedURL.protocol === 'http:') { | ||
| const name = parsedURL.pathname.substr(parsedURL.pathname.lastIndexOf('/') + 1) | ||
| let ix: number | undefined = Number(parse(parsedURL.hash).ix) |
There was a problem hiding this comment.
nit: please reference in a comment that this is the file index to select a specific file from that torrent so other people not familiar w/ this code can understand faster what this does
package.json
Outdated
| @@ -134,6 +136,7 @@ | |||
| "http-node": "^1.2.0", | |||
| "prettier-bytes": "^1.0.4", | |||
| "qr-image": "^3.2.0", | |||
| "query-string": "^6.1.0", | |||
| "react-chrome-redux": "^1.5.1", | |||
| "throttleit": "^1.0.0", | |||
| "webtorrent": "github:brave/webtorrent#600664249258dd7e893789e991d4cc5f481a7bae" | |||
There was a problem hiding this comment.
general question, is there a plan for keeping these github forks up to date? we have had some security issues with webtorrent in the past, and i think by forking them in this way, we will no longer get security reports for them with npm audit.
There was a problem hiding this comment.
brave/brave-browser#856, in the long-term, we want to solve the browser field issue without forking them.
In the short term, I could keep an eye from webtorrent upstream changes and rebase onto their latest releases, I'm not sure if that's sufficient tho.
I wonder if we have guidelines on how to follow upstream changes for projects we're forking, like how frequently should we check or update to the upstream?
There was a problem hiding this comment.
rebasing them sgtm for now. we don't really have guidelines right now; probably once per release? if there is a release checklist, this should be added to it.
There was a problem hiding this comment.
Per release sounds good to me.
I didn't see one in our wiki tho I might missed it, @bbondy, please correct me if there is.
I could create bugs for our next few releases first and mark with milestone labels, how does that sound, @diracdeltas ?
yrliou
force-pushed
the
torrent_support
branch
from
78942a3 to
d8924bd
Compare
If callbacks are run before we returning net::ERR_IO_PENDING in BraveNetworkDelegateBase::OnHeadersReceived, URLRequestHttpJob::awaiting_callback_ might remain true and hit CHECK(!awaiting_callback_) in ~URLRequestHttpJob().
…urce set inside brave_webtorrent component
- Add kBittorrentMimeType and kOctetStreamMimeType constants - Use 307 instead of 301 when redirecting torrent links - Use querystring instead of query-string - fixing nit
yrliou
force-pushed
the
torrent_support
branch
from
d8924bd to
f3f3437
Compare
|
Debug builds and tests on all platforms seem fine, this PR is ready to merge. |
|
Btw we use 0.55.x label as an indication that it is merged, so best to let the merge add it. Thanks! |
Fix brave/brave-browser#855
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Manual Test:
(Note: It might take a while for the media to be ready to play.)
Automation Test:
npm run test -- brave_browser_tests --filter=BraveContentBrowserClientTest.ReverseRewriteTorrentURLnpm run test -- brave_unit_tests --filter=BraveTorrentRedirectNetworkDelegateHelperTest.*npm run test-unitReviewer Checklist: