Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Block URL requests in Tor windows if Tor is not set up right. #1679

Merged
merged 3 commits into from
Feb 20, 2019
Merged

Block URL requests in Tor windows if Tor is not set up right. #1679

merged 3 commits into from
Feb 20, 2019

Conversation

riastradh-brave
Copy link
Contributor

@riastradh-brave riastradh-brave commented Feb 15, 2019 •
edited by kjozwiak
Loading

Not sure whether I should set QA/Yes or QA/No here: the steps to reproduce are kind of obscure, and the automatic test ought to cover this. But I'm also a little nervous about keeping the copypasta in tor_profile_service_impl.cc and mock_tor_profile_service_impl.cc synchronized.

fix brave/brave-browser#3058
fix brave/brave-browser#3349

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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • 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.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. npm test brave_unit_tests
    1. Launch Brave, open a window with Tor, visit check.torproject.org, and quit Brave. (This ensures the extension with the tor daemon executable is downloaded.)
    2. Find the tor-<version>-<os>-brave-<revision> file under the user data directory (~/.config/BraveSoftware/Brave-Browser-Dev or similar on Linux, ~/Library/Application Support/BraveSoftware/Brave-Browser-Dev or similar on macOS, %appdata%\BraveSoftware\Brave-Browser-Dev or similar on Windows).
    3. Change the permissions on that file to be nonexecutable and nonchangeable by your user id, e.g.: chmod -x tor-<version>-<os>-brave-<revision> && sudo chown nobody tor-<version>-<os>-brave-<revision>.
    4. Launch Brave, open a window with Tor (confirm the new tab page is as expected), and visit check.torproject.org.
      • If you get an error page, success.
      • If you get a page telling you you're not using Tor, failure.
      • If you get a page telling you you are using Tor, you didn't screw things up in your user data directory badly enough; maybe try attacking it with a blowtorch or chainsaw instead, or let an airline baggage handler at it.

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

@riastradh-brave
Test network request in Tor profile with broken tor config.
a847d57 
Should fail, but currently this is broken (#3058, #3349).
@riastradh-brave
Block all external URL requests if tor is not set up right.
a96d221 
Allow chrome, chrome-extension, and chrome-devtools URLs.

This way we don't have to rely on other logic to prevent usage of the
Tor profile before the daemon is configured, which turned out not be
as reliable as hoped.

fix #3058
fix #3349
darkdh
darkdh reviewed Feb 15, 2019
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

I added QA/Yes for brave/brave-browser#3349 because your manual test plan is detailed.
And maybe we can make MockTorProfileServiceImpl inheriting TorProfileServiceImpl to avoid duplicate logic

darkdh
darkdh reviewed Feb 20, 2019
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

can you squash lint changes into one?

@riastradh-brave
Appease lint and avoid C++ pitfalls.
0471a50 
- Fix copyright headings to appease lint.
- Fix header include guards to appease lint.
- Fix missing includes.
- Use explicit for unary constructors.
- Fix whitespace to appease lint.
@riastradh-brave riastradh-brave force-pushed the riastradh-3349-blockiftorhosed branch from fe5d37c to 0471a50 Compare February 20, 2019 18:44
@riastradh-brave
Copy link
Contributor Author

@darkdh Lint appeasement squashed: 0471a50

darkdh
darkdh approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

@riastradh-brave riastradh-brave merged commit a48af9c into master Feb 20, 2019
@bsclifton bsclifton deleted the riastradh-3349-blockiftorhosed branch February 20, 2019 20:08
This was referenced Feb 20, 2019
Merged
Merged
Closed
@bsclifton bsclifton added this to the 0.63.x - Nightly milestone Feb 26, 2019
This was referenced Feb 27, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@riastradh-brave riastradh-brave

Labels
feature/tor
Projects
None yet
Milestone
0.63.x - Release
Development

Successfully merging this pull request may close these issues.

No Tor in a Tor window Tor not working just after clean install
3 participants
@riastradh-brave @darkdh @bsclifton