Fix WebTorrent instant streaming playback links #2990

feross · 2019-07-24T00:27:37Z

In short, this PR does not redirect URL requests with #ix= to the .torrent file.

When the user clicks on a file in the torrent file list, a new tab opens which points to the .torrent file, but with a file fragment appended. For example, the torrent https://webtorrent.io/torrents/big-buck-bunny.torrent would have a link to https://webtorrent.io/torrents/big-buck-bunny.torrent#ix=1 for the 2nd file in the torrent.

Without this PR, this request passes the IsWebtorrentInitiated(ctx) condition and is therefore allowed to reach the network. Instead, we should confirm that the request is not for an individual file before letting it go to the network. That's what this PR does.

Fixes: brave/brave-browser#3966

Branched from this PR: #2989 (so merge after that one is merged)

Submitter Checklist:

Test Plan:

Go to https://webtorrent.io/free-torrents
Click on Big Buck Bunny (torrent file)
Start download
Click on the .mp4 file (second file)
Confirm that it opens a new tab, and a video player shows up.
Confirm that the video plays Big Buck Bunny when it is clicked.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper.cc

feross · 2019-07-25T20:35:59Z

...ents/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper_unittest.cc

    non_torrent_url_ = GURL("https://webtorrent.io/torrents/sintel");
-    extension_url_ = GURL(


I renamed the extension_url_ variable to torrent_extension_url_ so the naming is consistent across all variables.

yrliou

Code itself LGTM, but CI build failed due to version mismatch, could you rebase & retry?
I would like to see jenkins CI pass before merging this.

feross · 2019-07-25T20:58:57Z

Sure thing. Looks like I got unlucky and the version just changed :/

yrliou · 2019-07-26T22:09:38Z

@feross Please help fix lint errors below.

14:31:59  components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper_unittest.cc:49:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
14:31:59  components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper_unittest.cc:333:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
14:31:59  components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper_unittest.cc:370:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

You may run npm run lint under brave-browser locally to check the result too.

feross · 2019-07-26T22:19:25Z

You may run npm run lint under brave-browser locally to check the result too.

Thanks - didn't realize that there was an npm run lint for brave-browser that differed from the one under brave-core. Good to know, thanks!

- `renderFileLink` is never called when `torrent` is null - Remove duplicate props deconstruction

When the user clicks on a file in the torrent file list, a new tab opens which points to the .torrent file, but with a file fragment appended. For example, the torrent https://webtorrent.io/torrents/big-buck-bunny.torrent would have a link to https://webtorrent.io/torrents/big-buck-bunny.torrent#ix=1 for the 2nd file in the torrent. Without this PR, this request passes the `IsWebtorrentInitiated(ctx)` condition and is therefore allowed to reach the network. Instead, we should confirm that the request is not for an individual file before letting it go to the network. That's what this commit does. Fixes: brave/brave-browser#3966

yrliou · 2019-07-27T03:48:35Z

18:51:19  [227/227] BraveRewardsBrowserTest.ProcessPendingContributions (TIMED OUT)
18:51:19  1 test timed out:
18:51:19      BraveRewardsBrowserTest.ProcessPendingContributions (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2153)

Above was the test failure, which isn't relevant to this PR.

iefremov · 2019-08-02T20:03:01Z

components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper.cc

@@ -41,6 +41,16 @@ bool URLMatched(const GURL& url) {
      base::CompareCase::INSENSITIVE_ASCII);
 }

+/**
+ * Returns true if the URL contains a URL fragment that starts with "ix=". For


@feross Just some nits - we use google/chromium style guide that requires //-style comments

iefremov · 2019-08-02T20:03:28Z

...ents/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper_unittest.cc

+  request->set_initiator(url::Origin::Create(
+    torrent_extension_url().GetOrigin()));
+  scoped_refptr<net::HttpResponseHeaders> orig_response_headers =
+    new net::HttpResponseHeaders(std::string());


it is handy to use clang-format to make whitespaces CC-conformant

feross · 2019-08-04T00:22:46Z

@iefremov Lmk if you want me to send a PR to fix these issues.

feross added this to the 0.70.x - Nightly milestone Jul 24, 2019

feross requested a review from yrliou July 24, 2019 00:27

feross self-assigned this Jul 24, 2019

feross force-pushed the webtorrent-stream-fix branch from 2bbc6c9 to e56ef6a Compare July 24, 2019 00:27

feross added the feature/webtorrent Label for webtorrent related issues label Jul 24, 2019

yrliou reviewed Jul 25, 2019

View reviewed changes

components/brave_webtorrent/browser/net/brave_torrent_redirect_network_delegate_helper.cc Show resolved Hide resolved

feross force-pushed the webtorrent-stream-fix branch from e56ef6a to d374114 Compare July 25, 2019 20:33

feross requested a review from yrliou July 25, 2019 20:34

feross commented Jul 25, 2019

View reviewed changes

yrliou reviewed Jul 25, 2019

View reviewed changes

feross force-pushed the webtorrent-stream-fix branch from d374114 to 238072c Compare July 25, 2019 20:58

feross force-pushed the webtorrent-stream-fix branch from 238072c to 42c7b8f Compare July 26, 2019 21:14

feross force-pushed the webtorrent-stream-fix branch from 42c7b8f to e6f061b Compare July 26, 2019 22:18

small style fix

8c15aef

- `renderFileLink` is never called when `torrent` is null - Remove duplicate props deconstruction

feross force-pushed the webtorrent-stream-fix branch from e6f061b to f073021 Compare July 26, 2019 22:25

feross force-pushed the webtorrent-stream-fix branch from f073021 to 623a37f Compare July 26, 2019 23:16

feross mentioned this pull request Jul 26, 2019

WebTorrent: intercept only top-level frame requests for .torrent files #3035

Merged

32 tasks

yrliou approved these changes Jul 27, 2019

View reviewed changes

yrliou merged commit ee64e40 into master Jul 27, 2019

yrliou deleted the webtorrent-stream-fix branch July 27, 2019 03:48

iefremov reviewed Aug 2, 2019

View reviewed changes

btlechowski mentioned this pull request Sep 18, 2019

Webtorrent file doesn't stream when file name is clicked on brave/brave-browser#3966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WebTorrent instant streaming playback links #2990

Fix WebTorrent instant streaming playback links #2990

feross commented Jul 24, 2019 •

edited

Loading

feross Jul 25, 2019

yrliou left a comment

feross commented Jul 25, 2019

yrliou commented Jul 26, 2019

feross commented Jul 26, 2019

yrliou commented Jul 27, 2019

iefremov Aug 2, 2019

iefremov Aug 2, 2019

feross commented Aug 4, 2019

		non_torrent_url_ = GURL("https://webtorrent.io/torrents/sintel");
		extension_url_ = GURL(

Fix WebTorrent instant streaming playback links #2990

Fix WebTorrent instant streaming playback links #2990

Conversation

feross commented Jul 24, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

feross Jul 25, 2019

Choose a reason for hiding this comment

yrliou left a comment

Choose a reason for hiding this comment

feross commented Jul 25, 2019

yrliou commented Jul 26, 2019

feross commented Jul 26, 2019

yrliou commented Jul 27, 2019

iefremov Aug 2, 2019

Choose a reason for hiding this comment

iefremov Aug 2, 2019

Choose a reason for hiding this comment

feross commented Aug 4, 2019

feross commented Jul 24, 2019 •

edited

Loading