Strip cookies from requests fetch favicons #14286
Conversation
app/renderer/lib/faviconUtil.js
Outdated
| wrapFaviconUrl: (url) => `${url}${suffix}`, | ||
| unwrapFaviconUrl: (url) => url.substring(0, url.length - suffix.length), | ||
| isWrappedFaviconUrl: (url) => url.endsWith(suffix) | ||
| } |
There was a problem hiding this comment.
minor: please move to js/lib
|
@diracdeltas added tests |
test/fixtures/cookie-favicon.html
Outdated
| var host = window.location + '' | ||
| var parts = host.split('/') | ||
| parts.splice(-1, 1) | ||
| host = parts.join('/') |
There was a problem hiding this comment.
you could just do var host = window.location.origin
test/unit/js/lib/faviconUtilTest.js
Outdated
| require('../../braveUnit') | ||
|
|
||
| describe('faviconUtil', function () { | ||
| it('wraps, identifies and unwarps URLs', function () { |
test/fixtures/cookie-favicon.html
Outdated
| host = parts.join('/') | ||
|
|
||
| function loadIco() { | ||
| var faviconLocation = host.replace('127.0.0.1', 'localhost') + '/cookie-favicon.ico' |
There was a problem hiding this comment.
do you need to git add cookie-favicon.ico?
There was a problem hiding this comment.
|
some lint errors: https://travis-ci.org/brave/browser-laptop/jobs/386331308#L5472 |
| @@ -1053,4 +1053,28 @@ describe('Bravery Panel', function () { | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| // see #14250 | |||
| describe('Favicon requests cannot set cookies', function () { | |||
Fixes brave#14250 Auditors: @diracdeltas Test Plan: Manually navigate to https://www.grc.com/cookies/forensics.htm and observe no problems in the report Navigate to a website with a favicon (such as github.com) and observe the favicon rendering Unit tests: run the faviconUtil unit tests Webdriver test: Runs a test page that sets a 3rd party cookie and checks if the favicon request carries over the cookies
|
Weird, |
There was a problem hiding this comment.
confirmed that the test page fails without this change and succeeds with it.
one issue is that Favicon requests cannot set cookies test passes even without this change. we looked into it for a while and couldn't figure out why cookies were not being sent in the webdriver favicon request even when they are set.
|
master / 0.24.x: c08c815 |
bsclifton
modified the milestones:
0.24.x (Developer Channel),
0.23.x Release 5 (Beta Channel)
Strip cookies from requests fetch favicons
|
Uplifted to 0.23.x with e104f5a |
Fixes #14250
Auditors: @diracdeltas
This is a quick and dirty fix for #14250. It involves some sketchy action on a distance and we can discuss what can be improved about it.
Test Plan:
Manually navigate to https://www.grc.com/cookies/forensics.htm and observe no problems in the report
Navigate to a website with a favicon (such as github.com) and observe the favicon rendering
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests