Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Block loading .onion sites in non-Tor tabs. #14715

Merged
merged 1 commit into from
Jul 13, 2018
Merged

Block loading .onion sites in non-Tor tabs. #14715

merged 1 commit into from
Jul 13, 2018

Conversation

riastradh-brave
Copy link
Contributor

@riastradh-brave riastradh-brave commented Jul 11, 2018 •
edited
Loading

fix #14431

Auditors: @diracdeltas @bsclifton

Test Plan:
I:

  1. Open a tab without Tor (private or nonprivate).
  2. Enter: https://nyttips4bmquxfzw.onion/
  3. Confirm that Brave blocks loading the URL.

II:

  1. Open a tab without Tor (private or nonprivate).
  2. Enter: https://nyttips4bmquxfzw.onion:12345/
  3. Confirm that Brave blocks loading the URL.

III:

  1. Open a private tab with Tor.
  2. Enter: https://nyttips4bmquxfzw.onion/
  3. Confirm that the NYT SecureDrop page loads.
  4. Bookmark it.
  5. Open a tab without Tor (private or nonprivate).
  6. Try to load the bookmark.
  7. Confirm that Brave blocks loading the bookmark.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@bsclifton bsclifton added this to the 0.24.x (Developer Channel) milestone Jul 11, 2018
diracdeltas
diracdeltas reviewed Jul 11, 2018
View reviewed changes
app/filtering.js Outdated
@@ -130,6 +130,14 @@ function registerForBeforeRequest (session, partition) {
onBlockedInTor(details, muonCb)
return
}
} else {
const host = urlParse(details.url).host
Copy link
Member

@diracdeltas diracdeltas Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably want the hostname property not host since host may include a port number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, thanks. Where is this API documented?

Copy link
Member

@diracdeltas diracdeltas Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find it easiest to just look at what it's mapped to in chromium's GURL in muon brave/common/extensions/url_bindings.cc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the API was intended to be consistent with Node's url.parse module

diracdeltas
diracdeltas reviewed Jul 11, 2018
View reviewed changes
app/filtering.js Outdated
const host = urlParse(details.url).host
// US-ASCII only in `.onion', so no need for locale-dependent
// case-insensitive comparisons.
if (host.toLowerCase().endsWith('.onion')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check typeof host === 'string' && host.toLowerCase().endsWith('.onion') since host may be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UGHHH WHY

diracdeltas
diracdeltas reviewed Jul 11, 2018
View reviewed changes
app/filtering.js Outdated
@@ -406,6 +414,19 @@ function onBlockedInTor (details, muonCb) {
}
}

function onBlockedOutsideTor (details, muonCb) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] to avoid code duplication, best to refactor this and onBlockedInTor into a single method that takes an argument indicating why it was blocked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@riastradh-brave
Block loading .onion sites in non-Tor tabs.
6efebe7 
fix #14431

Auditors: @diracdeltas @bsclifton

Test Plan:
I:
	1. Open a tab _without_ Tor (private or nonprivate).
	2. Enter: https://nyttips4bmquxfzw.onion/
	3. Confirm that Brave blocks loading the URL.

II:
	1. Open a tab _without_ Tor (private or nonprivate).
	2. Enter: https://nyttips4bmquxfzw.onion:12345/
	3. Confirm that Brave blocks loading the URL.

III:
	1. Open a private tab with Tor.
	2. Enter: https://nyttips4bmquxfzw.onion/
	3. Confirm that the NYT SecureDrop page loads.
	4. Bookmark it.
	5. Open a tab _without_ Tor (private or nonprivate).
	6. Try to load the bookmark.
	7. Confirm that Brave blocks loading the bookmark.
@riastradh-brave riastradh-brave force-pushed the riastradh-tor-blocktorlessonion branch from 855c5a4 to 6efebe7 Compare July 12, 2018 01:08
diracdeltas
diracdeltas approved these changes Jul 13, 2018
View reviewed changes
Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@diracdeltas diracdeltas merged commit 518ce29 into master Jul 13, 2018
@diracdeltas
Copy link
Member

master: 518ce29
0.24.x: c925b11

diracdeltas added a commit that referenced this pull request Jul 13, 2018
@diracdeltas
Merge pull request #14715 from brave/riastradh-tor-blocktorlessonion
c925b11 
Block loading .onion sites in non-Tor tabs.
@bsclifton bsclifton deleted the riastradh-tor-blocktorlessonion branch July 27, 2018 16:29
@bsclifton bsclifton mentioned this pull request Oct 1, 2018
Merged
@bsclifton bsclifton modified the milestones: 0.24.x-legacy, 0.25.0 Oct 1, 2018
@bsclifton
Copy link
Member

Will be re-merged with #15251

bsclifton added a commit that referenced this pull request Oct 2, 2018
@bsclifton
Merge pull request #15251 from bsclifton/riastradh-tor-blocktorlessonion
6d84bd2 
Merge pull request #14715 from brave/riastradh-tor-blocktorlessonion
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@diracdeltas diracdeltas diracdeltas approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees
No one assigned
Labels
feature/tor
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

Block .onion requests or transplant them to tabs with Tor enabled
3 participants
@riastradh-brave @diracdeltas @bsclifton