add cookie exceptions so *.google.com <=> *.googleapis.com #5313
Conversation
|
Note that there is not an automated test for this included, because the issue is addresses very complex interactions between multiple google properties and cookie settings. Would be best handled with manual QA |
pes10k
force-pushed
the
fix-google-drive-autoplay
branch
from
5f7d4c7 to
b62a7c6
Compare
| for (const auto &outer : patterns) { | ||
| for (const auto &inner : patterns) { | ||
| const auto a_rule = Rule( | ||
| outer, |
There was a problem hiding this comment.
This adds an exception for allowing cookies from https://[*.]googleapis.com/* to https://[*.]googleapis.com/* - Why do we need that?
There was a problem hiding this comment.
TBH, i dont fully understand why the same domain rules were needed. making the exception for [*.]google.com to [*.]google.com seemed like it shouldn't have been needed (same with the above).
We can remove the above, i don't think it'll be a big deal. It just makes the thing a bit more clumsy to express. If you'd prefer to just have the three exceptions though:
[*.]google.com -> [*.]google.com
[*.]google.com -> [*.]googleapis.com
[*.]googleapis.com -> [*.]google.com
I can update accordingly
There was a problem hiding this comment.
Shouldn't these two exceptions resolve the issue:
[*.]google.com -> [*.]googleapis.com
[*.]googleapis.com -> [*.]google.com
There was a problem hiding this comment.
From discussion with @pes10k:
there were cookies that wouldn’t be sent from (deeply nested sub domains).drive.google.com to drive.google.com
@iefremov @bridiver - This looks like a separate issue being fixed by [*.]google.com -> [*.]google.com exception. Any thoughts why this might be happening?
More details here: https://bravesoftware.slack.com/archives/C2FQMN4AD/p1587530010205900?thread_ts=1586811883.160700&cid=C2FQMN4AD
There was a problem hiding this comment.
@jumde correct, we should only need to handle googleapis.com <-> google.com
There was a problem hiding this comment.
[*.]google.com -> [*.]google.com should already be fine unless you have a site exception added for blocking 3rd-party cookies (toggled to a different setting and then back again)
There was a problem hiding this comment.
This may have to do with the other 3p cookie issues uncovered and discussed today, but i can promise that this was needed to unbreak the site, and that *.google.com domains were not sending cookies to other *.google.com w/o it as of last night
There was a problem hiding this comment.
I've retested this after #5318 and cherry picking in #5341, and the [*.]google.com -> [*.]google.com exception is still needed.
@bridiver whatever is going is going on at a level of chromium / brave-core that I dont understand or can dig into anytime soon. What do you think if I create a follow up issue (assigned to you?) to investigate further, and add a similar todo / ¯\_(ツ)_/¯ comment in the code?
There was a problem hiding this comment.
bc it would be good to get this fix out ASAP
…ns can set and send cookies for each other, fixes brave/brave-browser#1122
bridiver
force-pushed
the
fix-google-drive-autoplay
branch
from
b62a7c6 to
766725e
Compare
|
closed in favor of #5433 |
Adds several storage allowances, so that google.com can set cookies for googleapis.com, and vise versa. Note that additional exceptions are needed so that
[*.].google.comcan set cookies on[*.].google.combecause of subdomain / eTLD+1 issues in how content setting rules are applied.Second half of whats needed to fix brave/brave-browser#1122