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

Restrict custom-headers for partners #3301

Closed
bsclifton opened this issue Feb 11, 2019 · 5 comments · Fixed by brave/brave-core#1633
Closed

Restrict custom-headers for partners #3301

bsclifton opened this issue Feb 11, 2019 · 5 comments · Fixed by brave/brave-core#1633

Comments

@bsclifton
Copy link
Member

bsclifton commented Feb 11, 2019

Test plan

  • install 0.59.35 Chromium: 72.0.3626.81 (which doesn't have the fix)
  • launch 0.59.35 using BRAVE_REFERRALS_SERVER=laptop-updates-pre.brave.com
  • visit brave.com and you should see X-Brave-Access-Key: key in the headers
  • uninstall 0.59.35 Chromium: 72.0.3626.81 & install 0.60.44 Chromium: 72.0.3626.109
  • launch 0.60.44 using BRAVE_REFERRALS_SERVER=laptop-updates-pre.brave.com
  • you shouldn't see any X-Brave-Access-Key: headers when visiting brave.com
  • visit marketwatch.com & barrons.com and ensure you receive X-Brave-Partner: dowjones
  • visit cheddar.com and ensure that you receive x-brave-partner: cheddar
  • visit coinbase.com and ensure that you receive x-brave-partner: coinbase

Also go through the Dow Jones flow for both MW & Barrons using 0.60.44 Chromium: 72.0.3626.109 and ensure that you can redeem a promotional code and create an account.

Background

When creating the referral program, we designed it so that partners can send custom headers. The intention is so that partners can detect a user is using Brave and customize the experience for them (ex: allow them to read articles or use the service for free, etc)

An example of the headers (which are all X-Brave-Partner) can be seen here:
https://laptop-updates.brave.com/promo/custom-headers

This design and implementation was originally security reviewed (and approved) by @tomlowenthal here (private repo link):
https://github.com/brave/internal/issues/250#issuecomment-379076770

Description

We should restrict this list so that it can ONLY use this list for sending the X-Brave-Partner header. No custom header names should be allowed

Related

@bsclifton bsclifton added this to the 0.60.x - Beta milestone Feb 11, 2019
diracdeltas added a commit to brave/brave-core that referenced this issue Feb 11, 2019
Fix brave/brave-browser#3301

Currently the only whitelisted header is 'X-Brave-Partner'.
diracdeltas added a commit to brave/brave-core that referenced this issue Feb 11, 2019
Fix brave/brave-browser#3301

Currently the only whitelisted header is 'X-Brave-Partner'.
diracdeltas added a commit to brave/brave-core that referenced this issue Feb 11, 2019
Fix brave/brave-browser#3301

Currently the only whitelisted header is 'X-Brave-Partner'.
@kjozwiak
Copy link
Member

kjozwiak commented Feb 12, 2019

@bsclifton @diracdeltas is there anything QA can do here? Can we get some test cases added into the issue or the PR? Assuming we need to check and make sure that only X-Brave-Partner is being accepted and anything else throws an error.

@diracdeltas
Copy link
Member

@kjozwiak at the very least QA can test that referral promo sites like dow jones still work (same as original test plan for DJ promo)

if @aekeus adds an entry like [{"domains":["brave.com"],"headers":{"foo":"bar"},"cookieNames":[],"expiration":31536000000}] to the promo headers endpoint, then we can also test that going to brave.com does not add any extra headers.

@kjozwiak
Copy link
Member

Thanks @diracdeltas 👍 @aekeus would it be possible to add the above? Maybe we can use the staging server for that portion of the test so we don't need to add the above entry into production?

@aekeus
Copy link
Member

aekeus commented Feb 20, 2019

Yes, we can add brave.com to the custom-headers to help in testing.

@kjozwiak
Copy link
Member

kjozwiak commented Feb 21, 2019

Verification PASSED on macOS 10.14.2 x64 using the following build:

Brave 0.60.44 Chromium: 72.0.3626.109 (Official Build) (64-bit)
Revision fae8db7ab9280fa6704a59980263c804f809ebd5-refs/branch-heads/3626@{#857}
OS Mac OS X

Verification passed on

Brave 0.60.44 Chromium: 72.0.3626.109 (Official Build) (64-bit)
Revision fae8db7ab9280fa6704a59980263c804f809ebd5-refs/branch-heads/3626@{#857}
OS Windows 7

Used test plan from #3301 (comment)

Verification PASSED on Mint 19.1 x64 using the following build:

Brave 0.60.45 Chromium: 72.0.3626.109 (Official Build) (64-bit)
Revision fae8db7ab9280fa6704a59980263c804f809ebd5-refs/branch-heads/3626@{#857}
OS Linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment