3rd party cookie registry domains #5390
Conversation
| @@ -33,7 +34,10 @@ Rule CloneRule(const Rule& rule, bool reverse_patterns = false) { | |||
| ContentSettingsPattern::FromString("https://firstParty/*")) { | |||
| if (!rule.primary_pattern.MatchesAllHosts()) { | |||
| secondary_pattern = ContentSettingsPattern::FromString( | |||
| "*://[*.]" + rule.primary_pattern.GetHost() + "/*"); | |||
There was a problem hiding this comment.
@bridiver can you say a bit about whats going on here. Both google.com and googleapis.com should be diff eTLD+1, so I think should be unrelated to dealing with private registries
There was a problem hiding this comment.
@pes10k pulling in your changes was an accident because I was already on your branch
bridiver
force-pushed
the
3rd-party-cookie-registry-domains
branch
from
ad0a278
to
6e865b4
Compare
bridiver
force-pushed
the
3rd-party-cookie-registry-domains
branch
from
6e865b4
to
1adf0af
Compare
|
|
||
| domain_registry_url_ = embedded_test_server()->GetURL("mobile.twitter.com", | ||
| "/cookie_iframe.html"); | ||
| iframe_domain_registry_url_ = embedded_test_server()->GetURL("blah.twitter.com", |
There was a problem hiding this comment.
for some reason when I just used "twitter.com" it would hang forever loading, but this serves the same purpose
|
|
||
| namespace content_settings { | ||
|
|
||
| namespace { | ||
|
|
||
| Rule CloneRule(const Rule& rule, bool reverse_patterns = false) { | ||
| auto secondary_pattern = rule.secondary_pattern; | ||
| if (secondary_pattern == | ||
| // brave plugin rules incorrectly use first party url as primary |
There was a problem hiding this comment.
it was confusing to flip things back and forth so I reversed them at the start instead and rephrased the comment
There was a problem hiding this comment.
As for me it became more confusing, because now I need to keep in mind that primary/secondary are probably not what they mean. Maybe renaming to reversed_primary/reversed_secondary would help
There was a problem hiding this comment.
actually they are what they mean now, before it was confusing because they were reversed at the end of the method
There was a problem hiding this comment.
still I would rename them. Maybe new_primary and new_secondary, or cloned_primary etc
| domain_registry_url_ = embedded_test_server()->GetURL("mobile.twitter.com", | ||
| "/cookie_iframe.html"); | ||
| iframe_domain_registry_url_ = | ||
| embedded_test_server()->GetURL("blah.twitter.com", |
There was a problem hiding this comment.
for some reason just using "twitter.com" would fail to load in the test server, but this serves the same purpose
…ns can set and send cookies for each other, fixes brave/brave-browser#1122
bridiver
force-pushed
the
3rd-party-cookie-registry-domains
branch
from
a40231a
to
340dc92
Compare
| if (!rule.primary_pattern.MatchesAllHosts()) { | ||
| secondary_pattern = ContentSettingsPattern::FromString( | ||
| "*://[*.]" + rule.primary_pattern.GetHost() + "/*"); | ||
| DCHECK(reverse_patterns); // we should only hit this for brave plugin rules |
There was a problem hiding this comment.
I guess we still run tests without DCHECKs - mb just for curiosity add DumpWithoutCrashing() here
There was a problem hiding this comment.
I ran locally with dcheck enabled
3rd party cookie registry domains This excludes changes to `browser/net/brave_network_delegate_browsertest.cc`
3rd party cookie registry domains This excludes changes to `browser/net/brave_network_delegate_browsertest.cc`
|
Reproduced the original issue with
Verification PASSED with STR:
|
|
Verification passed on 1.10.22 xr64 nightly build
|
Resolves brave/brave-browser#9489
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.