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

Skip initialization of referral system for the default referral code #8103

Merged
merged 3 commits into from
Mar 2, 2021

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Mar 2, 2021

Fixes brave/brave-browser#14428

Security review here:
https://github.com/brave/security/issues/349

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:

See #8103 (comment)

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathansampson
Copy link
Contributor

jonathansampson commented Mar 2, 2021

Test Plan

  1. Close all Brave instances
  2. Fresh profile
  3. Install Brave (no referral code) and launch
  4. No call to https://laptop-updates.brave.com/promo/initialize/nonua should be observed (can check using fiddler)

@bsclifton bsclifton requested a review from kkuehlz March 2, 2021 16:36
@bsclifton
Copy link
Member Author

Updated approach after talking out with @aekeus and @jsecretan 😄 Much more simple

@bsclifton bsclifton force-pushed the bsc-remove-default-promo-code branch from e8ab576 to b299160 Compare March 2, 2021 16:41
@bsclifton bsclifton changed the title Don't set a default referral code for non-referral builds Skip initialization of referral system for the default referral code Mar 2, 2021
@bsclifton
Copy link
Member Author

Created security review - we'll need approval here before we can merge

Copy link
Contributor

@kkuehlz kkuehlz left a comment

Choose a reason for hiding this comment

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

LGTM.

I like your test plan ;).

@bsclifton
Copy link
Member Author

Updated test plan to assume that this patch is included in the build 😄 Original steps by @jonathansampson help prove the code path works (without having this patch present)

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@stephendonner
Copy link
Contributor

stephendonner commented Mar 3, 2021

Verified FIXED on nightly using

Brave 1.23.14 Chromium: 89.0.4389.72 (Official Build) nightly (x86_64)
Revision 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS macOS Version 11.2.2 (Build 20D80)

Verified using Charles Proxy that I can no longer see the referral_code: BRV001 section in a JSON blob under initalize, nonua, as I can, with release build 1.20.110, for the host https://laptop-updates.brave.com/.

Before (1.20.110) After (1.23.13) Referral/promo code via brave://local-state (`1.23.131)
Screen Shot 2021-03-02 at 3 11 47 PM Screen Shot 2021-03-02 at 4 25 00 PM Screen Shot 2021-03-02 at 4 32 33 PM

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.23.14 Chromium: 89.0.4389.72 (Official Build) nightly (64-bit)
-- | --
Revision | 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS | Windows 10 OS Version 2009 (Build 19042.844)
  • ensured that /promo/initialize/nonua wasn't being called at startup as per the examples below
  • ensured that the BRV001 referral was being used in brave://local-state

Example of the issue originally occurring on 1.21.71 CR: 89.0.4389.72:

refCode

Examples if the issue being fixed via 1.23.14 CR: 89.0.4389.72:

/promo/initialize/nonua removed referral still being used under brave://local-state
refCodeFixed redCodeLocalState

@kjozwiak
Copy link
Member

kjozwiak commented Mar 3, 2021

Verification PASSED on Samsung S10+ running Android 11 using the following build:

1.23.14 CR: 89.0.4389.72
  • ensured that the download_id was removed/not present in brave://local-state via the referral header
1.21.71 CR: 89.0.4389.72 (not fixed) 1.23.14 CR: 89.0.4389.72 (fixed)
Screenshot_20210302-195311_Brave Screenshot_20210302-195759_Brave - Nightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip initialization of referral system for the default referral code
7 participants