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

Brave News image requests should omit certain headers #12684

Open
fmarier opened this issue Nov 13, 2020 · 7 comments · Fixed by brave/brave-core#7245
Open

Brave News image requests should omit certain headers #12684

fmarier opened this issue Nov 13, 2020 · 7 comments · Fixed by brave/brave-core#7245
Assignees
Labels
feature/brave-news formerly brave-today OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. security

Comments

@fmarier
Copy link
Member

fmarier commented Nov 13, 2020

Every application using our private CDN is asked to remove the following headers from their requests:

  • Accept-Language
  • Cookie
  • DNT
  • Referer
  • User-Agent

I suspect we already omit the Cookie header when loading images in Brave Today.

@srirambv srirambv added the feature/brave-news formerly brave-today label Nov 13, 2020
@rebron rebron added this to Untriaged Backlog in General Nov 24, 2020
@rebron rebron added priority/P2 A bad problem. We might uplift this to the next planned release. priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Nov 24, 2020
@rebron rebron moved this from Untriaged Backlog to In progress in General Nov 24, 2020
@petemill petemill self-assigned this Nov 25, 2020
@petemill
Copy link
Member

These are the current request headers made when we request an item

image

Looks like we just need to remove

  • user-agent
  • accept-language

Is that right @fmarier ?

@petemill
Copy link
Member

@fmarier
Copy link
Member Author

fmarier commented Nov 25, 2020

Looks like we just need to remove

  • user-agent
  • accept-language

Yes, those two, and potentially DNT though you'd only see that one if you enabled it in brave://settings/cookies:
Screenshot from 2020-11-25 13-05-49

General automation moved this from In progress to Completed Dec 1, 2020
@petemill
Copy link
Member

petemill commented Dec 2, 2020

Going to re-open this and leave it open since the user-agent and dnt headers are still being transmitted. Unavoidable with the current fetch API in chromium until we can move to C++

@petemill petemill reopened this Dec 2, 2020
@petemill
Copy link
Member

petemill commented Dec 2, 2020

@fmarier are you ok with moving this to a p3 since there should not be that many different buckets of users if we're only sending UA and dnt?

@rebron rebron moved this from Completed to P1 & P2 Backlog in General Dec 8, 2020
@fmarier
Copy link
Member Author

fmarier commented Dec 10, 2020

P3 sounds fine. Let's not forget about it, but it doesn't need to be urgently fixed given we're not leaking very much info anymore.

@rebron rebron removed this from P1 & P2 Backlog in General Jan 25, 2021
@rebron rebron added this to Backlog in Brave News Jan 25, 2021
@rebron rebron moved this from Backlog to P2/P3/P4 in Brave News Jan 25, 2021
@bsclifton bsclifton changed the title Brave Today image requests should omit certain headers Brave News image requests should omit certain headers Jun 14, 2021
@mattmcalister
Copy link

@petemill is this addressable now? I'd like to close out old issues, if possible.

@mattmcalister mattmcalister moved this from P2/P3/P4 to Backlog - Content in Brave News Mar 8, 2022
@mattmcalister mattmcalister moved this from Backlog - Content to Backlog - Content and UX in Brave News Mar 8, 2022
@mattmcalister mattmcalister removed this from Content and UX in Brave News Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/brave-news formerly brave-today OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants