allow first party iframes embedded in 3rd-party origins plus exceptio… #5433

bridiver · 2020-04-30T21:04:48Z

…ns for wp/wordpress and playstation/sonyentertainmentnetwork

fix brave/brave-browser#8629
fix brave/brave-browser#9564
fix brave/brave-browser#9105

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

pes10k

LGTM, as discussed

diracdeltas · 2020-04-30T21:26:22Z

browser/net/brave_network_delegate_browsertest.cc

+      "setTimeout(\""
+      "var iframes = document.getElementById('%s');iframes.src='%s';"
+      "\",0)",
+      iframe_id.c_str(), url.spec().c_str());


please confirm there's no way for any attacker controlled input to end up as part of this string thereby causing XSS

@diracdeltas this is a test :)

@bridiver yes i'm aware, just being paranoid. guessing the answer is no.

there's nothing special about the script and it was actually copied from a chromium test helper and modified to use the render frame instead of the webcontents. It isn't related to the functionality here in any way, it's just a way to update the iframe url in the test

iefremov · 2020-05-01T15:18:10Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+
+#include "components/content_settings/core/common/cookie_settings_base.h"
+
+#include "base/containers/flat_map.h"


yep, there was a flat map, but not anymore

iefremov · 2020-05-01T15:36:29Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+
+  GURL first_party_url = site_for_cookies;
+
+  if (!first_party_url.is_valid() && top_frame_origin)


hmm, shouldn't we use top_frame_origin by default and fallback to site_for_cookies?

It doesn't really matter which way you do it because if the parent frame is 3rd-party then the site_for_cookies is invalid

// this will be a 3rd party cookie. for (const RenderFrameHostImpl* rfh = render_frame_host; rfh; rfh = rfh->parent_) { if (!candidate.IsEquivalent( net::SiteForCookies::FromOrigin(rfh->last_committed_origin_))) { return net::SiteForCookies(); } }```

and it seems safer to me to use site_for_cookies when it is available because that is its purpose

I just want to be consistent with how the original class thinks about first party url

GetCookieSettingInternal( url, top_frame_origin ? top_frame_origin->GetURL() : site_for_cookies, false, nullptr, &setting);

@iefremov so the problem here is that site_for_cookies is treated differently in default 3rd-party cookie blocking and in content settings. The content settings default to the top_frame_origin, but the 3rd-party cookie settings default to the site_for_cookies. The reason we don't want to default to the top_frame_origin here is that there are times when it's not what we want (like redirects) and that is why site_for_cookies exists in the first place. We used only top frame origin in browser-laptop and there were times when it was wrong because of that.

so for the purposes of 3rd-party cookie blocking we want to use site_for_cookies whenever it's available and only use top_frame_origin when it's empty (3rd-party iframe)

in the folow-up I'll add a comment here explaining that and see if I can come up with a test for it

iefremov · 2020-05-01T15:37:49Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+  if (!first_party_url.is_valid() && top_frame_origin)
+      first_party_url = top_frame_origin->GetURL();
+
+  if (net::registry_controlled_domains::GetDomainAndRegistry(


I think here we need some detailed explanations with examples and links, otherwise our ancestors will curse us

I'm not sure what you're looking for detailed explanations for?

a good example was written for wp exceptions in brave_content_settings_pref_provider.cc https://github.com/brave/brave-core/pull/5433/files#diff-9981585c3d9184fc54b96697c59d2028L317

Basically i want that anyone reading this could understand easily why we implemented this behaviour and get some links to the issue tracker

My question is about what specific behavior you're referring to. The overall behavior is defined by the issue and that's why we have the link in the PR that you can find through git blame. Are you talking specifically about why we're using GetDomainAndRegistry here or more generally about what this code is doing?

iefremov · 2020-05-01T15:38:40Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES))
+    return true;
+
+  for (auto i = entity_list->begin();


for (const auto& entity : entity_list)

it doesn't like this because of base::NoDestructor (even when dereferenced) and I didn't feel like fighting with it

iefremov · 2020-05-01T15:39:58Z

chromium_src/components/content_settings/core/common/cookie_settings_base.h

+#define BRAVE_COOKIE_SETTINGS_BASE_H                              \
+ private:                                                         \
+  bool IsChromiumCookieAccessAllowed(const GURL& url,             \
+                             const GURL& first_party_url) const;  \


better indent

iefremov · 2020-05-01T15:44:36Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+    const GURL& site_for_cookies,
+    const base::Optional<url::Origin>& top_frame_origin) const {
+
+  // get content settings only - do not consider default 3rd-party blocking


Mind improving some punctuation here and below? I.e. start with a capital letter and end with a dot.
("Comments should be as readable as narrative text, with proper capitalization and punctuation." )

jumde · 2020-05-01T17:32:50Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+      std::vector<std::pair<ContentSettingsPattern,
+                            ContentSettingsPattern>>> entity_list({
+          {
+            ContentSettingsPattern::FromString(kWp),


Instead of hard-coding the exceptions in the code, we can use the local-data-updater component to create a cookie exception list similar to: https://github.com/brave/referrer-whitelist/blob/master/data/ReferrerWhitelist.json

cc: @diracdeltas

I'm pretty sure that eventually we will end up with a server-updated list of "entities" (something like https://github.com/krgovind/first-party-sets or another similar attempt)

First party sets are not getting consensus from all browser vendors. There are too many edge-cases.

Strong vote against full party sets from me too. But would be open to pulling in user-agent controlled set of site associations (a la disconnect entities list).

I dig the local-data-updater idea too, but for my two cents, i'd like to separate that from this PR, since a this PR solves web compat issues folks are complaining about now, and local-data-updater would take a while longer (and, this machinery would more or less be needed anyway for the local-data-updater approach, so this can be one step on the way to that)

I don't think local-data-updater idea will take much time. We can use the referrer-exceptions list for cookies. I'm happy to quickly set-up a separate list if there are any concerns with using the referrer whitelist

I think a separate list would be better. @bridiver would probably know best whether changing this to local-data-updater is better to do in this PR, or in some future one.

For my two cents, if it pushes landing this PR back more than a day or two, its best doing separately though

yeah I also think we can make a separate list separately

Again, no argument about it going in a separate list, thats fine. But time is sensitive here, for biz reasons. If it will take longer to make a sep list, lets please merge this, and then work on a separate list as a next time.

@pes10k and I already discussed a list as a downloadable component, but that is outside the scope of this PR. We are only adding one exception to the existing hardcoded list from https://github.com/brave/brave-core/pull/5433/files#diff-9981585c3d9184fc54b96697c59d2028L332

and that exception was just added to this PR because it was somewhat related and we were moving the existing exception

pes10k · 2020-05-05T23:31:01Z

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

+          },
+          {
+            ContentSettingsPattern::FromString(kPlaystation),
+            ContentSettingsPattern::FromString(kSonyentertainmentnetwork)},


tiniest nit in the world, but this } is doing something the others aren't ;)

…ns for wp/wordpress and playstation/sonyentertainmentnetwork fix brave/brave-browser#8629 fix brave/brave-browser#9564

allow first party iframes embedded in 3rd-party origins plus exceptio…

bridiver requested review from pes10k and jumde April 30, 2020 21:04

bridiver requested a review from iefremov as a code owner April 30, 2020 21:04

bridiver self-assigned this Apr 30, 2020

pes10k approved these changes Apr 30, 2020

View reviewed changes

diracdeltas reviewed Apr 30, 2020

View reviewed changes

iefremov reviewed May 1, 2020

View reviewed changes

jumde reviewed May 1, 2020

View reviewed changes

pes10k mentioned this pull request May 1, 2020

3rd-party cookie shield prevents slideshare.net navigation brave/brave-browser#7868

Closed

pes10k reviewed May 5, 2020

View reviewed changes

bridiver force-pushed the fix-google-drive-autoplay-bj branch from 7e6cedd to 0a404b3 Compare May 5, 2020 23:44

bridiver mentioned this pull request May 6, 2020

add cookie exceptions so *.google.com <=> *.googleapis.com #5313

Closed

bridiver added 2 commits May 6, 2020 07:14

allow first party iframes embedded in 3rd-party origins plus exceptio…

8a70586

…ns for wp/wordpress and playstation/sonyentertainmentnetwork fix brave/brave-browser#8629 fix brave/brave-browser#9564

formatting

c6e2be4

bridiver force-pushed the fix-google-drive-autoplay-bj branch from 0a404b3 to c6e2be4 Compare May 6, 2020 14:15

lint

cb6746b

bridiver merged commit c1a339a into master May 7, 2020

bridiver deleted the fix-google-drive-autoplay-bj branch May 7, 2020 16:32

bridiver added this to the 1.10.x - Nightly milestone May 7, 2020

bsclifton pushed a commit that referenced this pull request May 7, 2020

Merge pull request #5433 from brave/fix-google-drive-autoplay-bj

6555d7d

allow first party iframes embedded in 3rd-party origins plus exceptio…

bsclifton mentioned this pull request May 7, 2020

allow first party iframes embedded in 3rd-party origins plus exceptio… (uplift to 1.9.x) #5494

Merged

4 tasks

pes10k mentioned this pull request May 12, 2020

Allow storage in 1p iframes, regardless of the relative parents of that 1p iframe brave/brave-browser#9105

Closed

bsclifton pushed a commit that referenced this pull request May 26, 2020

Merge pull request #5433 from brave/fix-google-drive-autoplay-bj

04690c0

allow first party iframes embedded in 3rd-party origins plus exceptio…

bsclifton pushed a commit that referenced this pull request May 28, 2020

Merge pull request #5433 from brave/fix-google-drive-autoplay-bj

5db6e73

allow first party iframes embedded in 3rd-party origins plus exceptio…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow first party iframes embedded in 3rd-party origins plus exceptio… #5433

allow first party iframes embedded in 3rd-party origins plus exceptio… #5433

bridiver commented Apr 30, 2020 •

edited by bsclifton

Loading

pes10k left a comment

diracdeltas Apr 30, 2020

bridiver Apr 30, 2020

diracdeltas Apr 30, 2020

bridiver May 5, 2020

iefremov May 1, 2020

bridiver May 5, 2020

iefremov May 1, 2020

bridiver May 5, 2020

bridiver May 5, 2020

iefremov May 6, 2020

bridiver May 8, 2020

bridiver May 8, 2020

bridiver May 8, 2020

iefremov May 1, 2020

bridiver May 5, 2020

iefremov May 6, 2020

bridiver May 8, 2020

iefremov May 1, 2020

bridiver May 5, 2020

iefremov May 1, 2020

iefremov May 1, 2020

jumde May 1, 2020 •

edited

Loading

iefremov May 1, 2020

jumde May 1, 2020

pes10k May 1, 2020

jumde May 1, 2020

pes10k May 1, 2020

iefremov May 4, 2020

pes10k May 4, 2020

bridiver May 5, 2020

bridiver May 5, 2020

pes10k May 5, 2020


		#include "components/content_settings/core/common/cookie_settings_base.h"

		#include "base/containers/flat_map.h"


		GURL first_party_url = site_for_cookies;

		if (!first_party_url.is_valid() && top_frame_origin)

allow first party iframes embedded in 3rd-party origins plus exceptio… #5433

allow first party iframes embedded in 3rd-party origins plus exceptio… #5433

Conversation

bridiver commented Apr 30, 2020 • edited by bsclifton Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

pes10k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jumde May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Apr 30, 2020 •

edited by bsclifton

Loading

jumde May 1, 2020 •

edited

Loading