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

Fix 7976: Adding a proxy for Crowd Deny #4482

Merged
merged 1 commit into from
Feb 6, 2020
Merged

Fix 7976: Adding a proxy for Crowd Deny #4482

merged 1 commit into from
Feb 6, 2020

Conversation

jumde
Copy link
Contributor

@jumde jumde commented Feb 3, 2020 •
edited by kjozwiak
Loading

Fix brave/brave-browser#7976

Submitter Checklist:

Test Plan:

  1. With a clean profile, verify that there are no connections to dl.google.com or *.gvt1.com
  2. Navigate to brave://components and verify that Crowd Deny is updated to v0.5

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.

@jumde jumde requested a review from iefremov as a code owner February 3, 2020 21:50
@jumde jumde self-assigned this Feb 3, 2020
@jumde jumde requested a review from bridiver February 3, 2020 21:59
@jumde jumde mentioned this pull request Feb 3, 2020
Merged
29 tasks
bridiver
bridiver reviewed Feb 3, 2020
View reviewed changes
browser/net/brave_static_redirect_network_delegate_helper.cc Outdated
@@ -48,6 +48,8 @@ int OnBeforeURLRequest_StaticRedirectWorkForGURL(
URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, kCRXDownloadPrefix);
static URLPattern autofill_pattern(
URLPattern::SCHEME_HTTPS, kAutofillPrefix);
static URLPattern crowdDeny_pattern(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not continue to follow this *_pattern convention that is not consistent with the style guide.

@diracdeltas
Copy link
Member

Do we want to include crowddeny at all or just disable it?

@jumde jumde force-pushed the crowd_deny_proxy branch 6 times, most recently from ff76f6f to 95c1b07 Compare February 4, 2020 21:39
@jumde jumde mentioned this pull request Feb 4, 2020
Closed
@jumde jumde force-pushed the crowd_deny_proxy branch 2 times, most recently from d8c5bea to abbaf08 Compare February 5, 2020 06:15
jumde
jumde commented Feb 5, 2020
View reviewed changes
browser/net/brave_static_redirect_network_delegate_helper.cc
static URLPattern gvt1_pattern(
URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, "*://*.gvt1.com/*");
static URLPattern googleDl_pattern(
URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback URL if redirector is not working.

@jumde jumde force-pushed the crowd_deny_proxy branch 2 times, most recently from 1f8b793 to 028e39a Compare February 5, 2020 16:03
diracdeltas
diracdeltas reviewed Feb 5, 2020
View reviewed changes
browser/net/brave_static_redirect_network_delegate_helper.cc
@@ -48,6 +48,11 @@ int OnBeforeURLRequest_StaticRedirectWorkForGURL(
URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, kCRXDownloadPrefix);
static URLPattern autofill_pattern(
URLPattern::SCHEME_HTTPS, kAutofillPrefix);
static URLPattern gvt1_pattern(
Copy link
Member

Choose a reason for hiding this comment

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

this also matches the domains that we use for redirecting to crlsets.brave.com. does crlsets still work correctly with this change? it seems like it would work only if the crlsets matching happened before this matching

Copy link
Member

Choose a reason for hiding this comment

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

crlsets pattern: https://github.com/brave/brave-browser/pull/8035/files#diff-1561fc2f445953b6e5ac326a56299568R4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an issue to update crlsets.brave.com to redirector.brave.com - brave/brave-browser#4634 - The domains point to the same endpoint. I'll pick that up after this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ok, if crlsets.brave.com is the same as redirector.brave.com then this seems like it shouldn't break crlsets.

iefremov
iefremov reviewed Feb 5, 2020
View reviewed changes
browser/net/brave_static_redirect_network_delegate_helper.cc
@@ -48,6 +48,11 @@ int OnBeforeURLRequest_StaticRedirectWorkForGURL(
URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, kCRXDownloadPrefix);
static URLPattern autofill_pattern(
URLPattern::SCHEME_HTTPS, kAutofillPrefix);
static URLPattern gvt1_pattern(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should add comments and issue IDs to these patterns, for better understanding

@iefremov iefremov self-requested a review February 5, 2020 17:58
iefremov
iefremov approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

LGTM, given that all concerns are fixed

@jumde jumde merged commit b685a63 into master Feb 6, 2020
@jumde jumde deleted the crowd_deny_proxy branch February 6, 2020 05:41
This was referenced Feb 6, 2020
Merged
Merged
Merged
@jumde jumde added this to the 1.6.x - Nightly milestone Feb 6, 2020
jumde pushed a commit that referenced this pull request Feb 6, 2020
@brave-browser-releases @jumde
Uplift of #4482 (squashed) to release
8f68727
jumde pushed a commit that referenced this pull request Feb 6, 2020
@brave-browser-releases @jumde
Uplift of #4482 (squashed) to beta
799bded
jumde pushed a commit that referenced this pull request Feb 6, 2020
@brave-browser-releases @jumde
Uplift of #4482 (squashed) to dev
71d07d1
@bbondy bbondy modified the milestones: 1.6.x - Beta, 1.7.x - Dev Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diracdeltas diracdeltas diracdeltas left review comments

@bridiver bridiver bridiver left review comments

@iefremov iefremov iefremov approved these changes

Assignees

@jumde jumde

Labels
None yet
Projects
None yet
Milestone
1.7.x - Release
Development

Successfully merging this pull request may close these issues.

Add proxies to handle Crowd Deny download
5 participants
@jumde @diracdeltas @bridiver @iefremov @bbondy