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.

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

Disable CNAME uncloaking when a proxy extension with a socks fallback is enabled #10742

Merged
merged 1 commit into from Oct 29, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Oct 28, 2021

Resolves brave/brave-browser#19070

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. Install the Proxy SwitchyOmega extension
  2. In the extension settings page, set the Proxy Servers section to have a single entry, as follows:
    Scheme Protocol Server Port
    (default) SOCKS5 127.0.0.1 8081
  3. Clear the "Bypass List" section in the extension settings page
  4. Ensure the above changes are applied by clicking the "Apply changes" button in the left column
  5. Start a SOCKS5 proxy server on the device. This can be done by entering ssh 127.0.0.1 -D 8081 on Linux and probably macOS. You're welcome to run it on a different device, but the Proxy Servers setting above will need to be modified accordingly.
  6. Add the following line to the Custom filters section in brave://adblock:
    ||dev-pages.brave.software/static/images/test.jpg
    
  7. Visit https://test-cname.brave.software/cname-uncloaking.html
  8. Use the Proxy SwitchyOmega icon in the "puzzle piece" extensions menu to enable proxy mode (should be the 3rd option).
  9. Press the Run test button
  10. The request should be allowed (green).
  11. In the "puzzle piece" extensions menu, change to [System Proxy] and run the test again. The request should be blocked (red).
  12. In the "puzzle piece" extensions menu, change to [Direct] and run the test again. The request should be blocked (red).

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

Based on the Slack discussion, that looks good to me.

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.

We need unit test for ProxySettingsAllowUncloaking
but since we need to get this out ASAP, it can be done as follow up

net::ProxyConfig::ProxyRules::Type::PROXY_LIST) {
net::ProxyConfig::ProxyRules::Type::PROXY_LIST ||
(config.value().proxy_rules().type ==
net::ProxyConfig::ProxyRules::Type::PROXY_LIST_PER_SCHEME &&
Copy link
Contributor

Choose a reason for hiding this comment

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

need to update the function comment, it describes only SingleProxy aka PROXY_LIST, not PROXY_LIST_PER_SCHEME

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, updated the comment

@iefremov
Copy link
Contributor

agree that we need a follow-up with tests

@antonok-edm antonok-edm force-pushed the cname-proxy-extension-per-scheme branch from 645a6d6 to 133af65 Compare October 29, 2021 17:54
@antonok-edm antonok-edm added this to the 1.33.x - Nightly milestone Oct 29, 2021
@antonok-edm antonok-edm merged commit 0c2580e into master Oct 29, 2021
@antonok-edm antonok-edm deleted the cname-proxy-extension-per-scheme branch October 29, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants