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.

Sign up for GitHub

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

Jump to bottom

Issue 2095: use the proper tab origin. #942

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Issue 2095: use the proper tab origin. #942

merged 2 commits into from
Nov 28, 2018

Conversation

iefremov
Copy link
Contributor

@iefremov iefremov commented Nov 21, 2018 •
edited
Loading

Fix brave/brave-browser#2095

As per URLRequest documentation and the corresponding RFC,
site_for_cookies cannot be used as a top-level document origin since it
can be empty in certain cases. We need to replace it with something more
reliable.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

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

@iefremov
Copy link
Contributor Author

Just a quick fix without tests since the issue is urgent. At the moment I'm continuing to make a reasonable test, but mocking site_for_cookies() is not obvious for me now.

I'm also worried with the presence of a global lock for all requests with cookies, so probably we should create something more clever here. I can suggest at least one (not so clever) improvement:

  • Keep a copy of the global map on the IO thread to avoid locking from URLRequests
  • When the global map on UI changes, make a new copy and atomically swap the new copy with the instance on IO thread.

@iefremov
Copy link
Contributor Author

Status update: SNAFU

  1. I was hasty in making this fix, obviously patching tab_origin must be inside of FillCTXFromRequest
  2. Testing this particular scenario turned out quite hard, it is still not clear how to get a pure cross-site iframe in tests (i.e. with empty site_for_cookies()). So I managed to do it by hand using URLRequestInterceptor, just to discover that frame ids attached to URLRequests in tests are not initialized (?)
  3. The fix doesn't work at all, and basically the main scenario of using that global map (frame id -> URL) in BraveContentBrowserClient is not working, because tracking of created/deleted frames is obviously not enough. There could be navigations in frames without any *Created, etc. events.
  4. Prepared a new fix that also tracks finished navigations, seems to work with draw.io, etc. Still not sure how to make proper tests for this.
  5. I believe that we have to get rid of this global map based approach and develop something less error-prone and architecture violating in the mid-term perspective. Any ideas welcome, I've also asked for help from some network layer gurus.

@iefremov iefremov force-pushed the issue_2095_allow_cookies branch 2 times, most recently from b169f65 to 9c0f0f0 Compare November 23, 2018 13:40
@iefremov
Copy link
Contributor Author

Pushed fix that seems working. Still no tests.

iefremov
iefremov commented Nov 23, 2018
View reviewed changes
browser/net/url_context.cc
auto* request_info = content::ResourceRequestInfo::ForRequest(request);
if (request_info) {
ctx->resource_type = request_info->GetResourceType();
}
brave_shields::GetRenderFrameInfo(request, &ctx->render_process_id, &ctx->render_frame_id,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - wrong order of render_process_id and render_frame_id

@iefremov iefremov force-pushed the issue_2095_allow_cookies branch 4 times, most recently from c6d14c4 to ab95843 Compare November 27, 2018 12:50
@iefremov
Copy link
Contributor Author

@bbondy PTAL

  • Added tests.
  • Also added additional map to track subframes which process/frame ids we cannot obtain on the IO thread.
  • Fixed wrong order and some other typos.

iefremov
iefremov commented Nov 27, 2018
View reviewed changes
components/brave_shields/browser/brave_shields_util.cc
@@ -107,9 +107,9 @@ void GetRenderFrameInfo(const URLRequest* request,
if (request_info) {
*frame_tree_node_id = request_info->GetFrameTreeNodeId();
}
extensions::ExtensionApiFrameIdMap::FrameData frame_data;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead code

iefremov
iefremov commented Nov 27, 2018
View reviewed changes
components/brave_shields/browser/brave_shields_web_contents_observer.cc
BraveShieldsWebContentsObserver::render_frame_key_to_tab_url;
BraveShieldsWebContentsObserver::frame_key_to_tab_url_;
std::map<int, GURL>
BraveShieldsWebContentsObserver::frame_tree_node_id_to_tab_url_;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional map for tracking frame tree node ids. They are available on the IO thread for frames/subframes instead of the pair procces_id/frame_id

@iefremov iefremov force-pushed the issue_2095_allow_cookies branch 2 times, most recently from 12d4e3e to 1152dfe Compare November 28, 2018 09:40
@iefremov
Copy link
Contributor Author

Rebased

Ivan Efremov added 2 commits November 28, 2018 13:34
@bbondy
Issue 2095: use the proper tab origin.
783ee0b 
brave/brave-browser#2095

As per URLRequest documentation and the corresponding RFC,
site_for_cookies cannot be used as a top-level document origin since it
can be empty in certain cases. We need to replace it with something more
reliable.
@bbondy
Revert using frame_tree_node_id, since this breaks tests.
51e6a10
@bbondy bbondy merged commit 25a24f5 into master Nov 28, 2018
bbondy added a commit that referenced this pull request Nov 28, 2018
@bbondy
Merge pull request #942 from brave/issue_2095_allow_cookies
d16bb42 
Issue 2095: use the proper tab origin.
@bbondy
Copy link
Member

bbondy commented Nov 28, 2018

0.58.x: d16bb42
master: 25a24f5

@kjozwiak
Copy link
Member

Approving uplift to 0.57.x after deliberating with @rebron 👍

bbondy added a commit that referenced this pull request Nov 28, 2018
@bbondy
Merge pull request #942 from brave/issue_2095_allow_cookies
48cb280 
Issue 2095: use the proper tab origin.
@bbondy
Copy link
Member

bbondy commented Nov 28, 2018

0.57.x: 48cb280

@bbondy bbondy deleted the issue_2095_allow_cookies branch December 5, 2018 15:28
@bbondy bbondy mentioned this pull request Dec 5, 2018
Closed
@bbondy bbondy added this to the 0.57.x - Release milestone Jan 14, 2019
@stephendonner stephendonner mentioned this pull request Oct 24, 2022
Merged
25 tasks
@btlechowski btlechowski mentioned this pull request Oct 24, 2022
Merged
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.57.x - Release
Development

Successfully merging this pull request may close these issues.

Third party cookies are still blocked even if per site shield is set to allow all cookies
3 participants
@iefremov @bbondy @kjozwiak