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

Bypass private browsing mode detection #6622

Merged
merged 2 commits into from
Nov 2, 2020
Merged

Bypass private browsing mode detection #6622

merged 2 commits into from
Nov 2, 2020

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Sep 10, 2020 •
edited
Loading

Resolves brave/brave-browser#11543
Resolves brave/brave-browser#12370

Just hard-codes the response to the navigator.storage.estimate() API to have a quota value of 2147483648, which is the same as what Firefox uses. See brave/brave-browser#12370 (comment) for justification.

Submitter Checklist:

Test Plan:

Open any webpage, open the Developer tools (Hamburger menu -> More tools -> Developer tools), select the "Console" tab, and enter the following code at the prompt:

navigator.storage.estimate().then(estimate => console.log(estimate.quota))

The returned number should be 2147483648, which corresponds to 2GB.

Verify that the behavior is the same in a private window as well.

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.

@antonok-edm antonok-edm self-assigned this Sep 10, 2020
@antonok-edm
Copy link
Collaborator Author

Windows CI test-install failure is a known issue unrelated to this PR.

@antonok-edm antonok-edm force-pushed the bypass-incognito-detection branch 2 times, most recently from fc610a4 to 04db2c2 Compare September 24, 2020 18:26
@antonok-edm antonok-edm requested a review from a team as a code owner October 28, 2020 20:01
mkarolin
mkarolin reviewed Oct 29, 2020
View reviewed changes
patches/third_party-blink-renderer-modules-quota-storage_manager.cc.patch Outdated
StorageEstimate* estimate = StorageEstimate::Create();
estimate->setUsage(usage_in_bytes);
- estimate->setQuota(quota_in_bytes);
+ estimate->setQuota(2147483648);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this would work, but maybe we can use chromium_src override that will redefine StorageEstimate as BraveStorageEstimate, where BraveStorageEstimate would subclass StorageEstimate and hide base's setQuota method with one that just sets the 2GB value?

So the override would look something like:

#include "third_party/blink/renderer/modules/quota/storage_estimate.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_storage_estimate.h" 

namespace blink {
class BraveStorageEstimate : public StorageEstimage {
public:
...
  void setQuota(uint64_t quota) {
    quota = 2147483648;
    StorageEstimage::setQuota(quota);
  }
};
}  // namespace blink

#define StorageEstimage BraveStorageEstimate
#include "..../storage_manager.cc"
#undef StorageEstimage

Or something like that...

Copy link
Collaborator Author

@antonok-edm antonok-edm Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mkarolin I just tried the below override at chromium_src/third_party/blink/renderer/modules/quota/storage_manager.cc - it doesn't appear to be working though, I'm not getting 2GB anymore.

#include "third_party/blink/renderer/modules/quota/storage_estimate.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_storage_estimate.h" 

namespace blink {
class BraveStorageEstimate : public StorageEstimate {
public:
  static BraveStorageEstimate* Create() {
    return MakeGarbageCollected<BraveStorageEstimate>();
  }
  void setQuota(uint64_t quota) {
    quota = 2147483648;
    StorageEstimate::setQuota(quota);
  }
};
}  // namespace blink

#define StorageEstimate BraveStorageEstimate
#include "../../../../../../../third_party/blink/renderer/modules/quota/storage_manager.cc"
#undef StorageEstimate

Copy link
Collaborator

@mkarolin mkarolin Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It would also need

static BraveStorageEstimate* Create() {
  return MakeGarbageCollected<BraveStorageEstimate>();
}

Copy link
Collaborator Author

@antonok-edm antonok-edm Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

added that, didn't appear to make any difference

edit: StorageEstimage should be StorageEstimate 😄

@antonok-edm antonok-edm force-pushed the bypass-incognito-detection branch 4 times, most recently from 52b2fcb to 606690c Compare October 30, 2020 02:16
mkarolin
mkarolin reviewed Oct 30, 2020
View reviewed changes
chromium_src/third_party/blink/renderer/modules/quota/navigator_storagetest.cc Outdated
EXPECT_EQ(url, contents->GetURL());

bool getStorageEstimateIs2Gb;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: ExecuteScriptAndExtract* functions are semi-deprecated. EvalJS is preferred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • updated to use EvalJs

mkarolin
mkarolin reviewed Oct 30, 2020
View reviewed changes
test/BUILD.gn Outdated
@@ -644,6 +644,7 @@ if (!is_android) {
"//brave/chromium_src/third_party/blink/renderer/core/frame/reporting_observer_browsertest.cc",
"//brave/chromium_src/third_party/blink/renderer/modules/battery/navigator_batterytest.cc",
"//brave/chromium_src/third_party/blink/renderer/modules/bluetooth/navigator_bluetoothtest.cc",
"//brave/chromium_src/third_party/blink/renderer/modules/quota/navigator_storagetest.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We no longer want to add tests directly to //brave/test/BUILD.gn. They are supposed to go into BUILD.gn where the test lives and then added as a dep here. (A recent example is #6964).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • moved into //chromium_src/third_party/blink/renderer/modules/BUILD.gn

mkarolin
mkarolin reviewed Oct 30, 2020
View reviewed changes
chromium_src/third_party/blink/renderer/modules/quota/navigator_storagetest.cc Outdated
Comment on lines 23 to 27
content_client_.reset(new ChromeContentClient);
content::SetContentClient(content_client_.get());
browser_content_client_.reset(new BraveContentBrowserClient());
content::SetBrowserClientForTesting(browser_content_client_.get());
content::SetupCrossSiteRedirector(embedded_test_server());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if any of this is needed in all of these 3 navigator_* tests. And if not, then you won't need the dependency on //brave/browser. They seem to pass with all this removed. cc: @jumde

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! Works for me as well, I updated all 3

mkarolin
mkarolin approved these changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

LGTM

@antonok-edm
Copy link
Collaborator Author

CI failures on Windows and MacOS are known and unrelated to this change:

Windows

RewardsContributionBrowserTest.RecurringTipForUnverifiedPublisher (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:327)

MacOS

RewardsContributionBrowserTest.PanelMonthlyTipAmount (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:927)
RewardsContributionBrowserTest.TipNonIntegralAmount (../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:603)
RewardsNotificationBrowserTest.InsufficientNotificationForInsufficientAmount (../../brave/components/brave_rewards/browser/test/rewards_notification_browsertest.cc:364)

@antonok-edm antonok-edm merged commit 1e53eb4 into master Nov 2, 2020
@antonok-edm antonok-edm deleted the bypass-incognito-detection branch November 2, 2020 22:33
bridiver
bridiver reviewed Nov 3, 2020
View reviewed changes
chromium_src/third_party/blink/renderer/modules/BUILD.gn
defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]

sources = [
"quota/navigator_storagetest.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

browser tests should be test_name_browsertest.cc - please create a follow-up to fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

also this is a layering violation, you cannot have browser tests inside blink. These tests could be moved into brave, but ideally they should be converted to blink::PageTestBase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just moved them there at @mkarolin's suggestion for #6622 (comment)

@antonok-edm antonok-edm added this to the 1.18.x - Nightly milestone Nov 3, 2020
@srirambv srirambv mentioned this pull request Nov 27, 2020
Closed
@antonok-edm antonok-edm mentioned this pull request Jul 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@mkarolin mkarolin mkarolin approved these changes

@pes10k pes10k Awaiting requested review from pes10k

@fmarier fmarier Awaiting requested review from fmarier

Assignees

@antonok-edm antonok-edm

Labels
None yet
Projects
None yet
Milestone
1.18.x - Release
Development

Successfully merging this pull request may close these issues.

The navigator.storage quota API returns stable identifiers Block private browsing mode detection
3 participants
@antonok-edm @bridiver @mkarolin