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.

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

Disabling getBattery | bluetooth | credential.get/store | navigator.usb apis #114

Merged
merged 1 commit into from
Jul 11, 2018

Conversation

jumde
Copy link
Contributor

@jumde jumde commented May 24, 2018

See here: brave/brave-browser#13 (comment)

Test Instructions:

  1. Open JS console ( Inspect > Console )
  2. Try using the following apis:
>navigator.credentials.get()
Promise {<resolved>: null}

>const credentials = new PasswordCredential({
    id: 'test@test.com',
    name: 'test',
    password: 'test'
});
navigator.credentials.store(credentials);
Promise {<resolved>: null}

> navigator.credentials.preventSilentAccess().then(function(data){ console.log(arguments); });
Arguments [undefined, callee: ƒ, Symbol(Symbol.iterator): ƒ]
Promise {<resolved>: undefined}

>navigator.getBattery()
Promise {<rejected>: TypeError: This api has been disabled.
    at <anonymous>:1:11}

>let options = {
    filters: [
      {namePrefix: 'Prefix'}
    ]
  }
undefined
navigator.bluetooth.requestDevice(options);
Promise {<pending>}
Bluetooth permission has been blocked.
Uncaught (in promise) DOMException: Web Bluetooth API globally disabled.

>navigator.usb
undefined

@jumde jumde requested review from bridiver and bbondy May 24, 2018 00:22
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

I want to look at this to see if we can avoid all these patches

+#if defined(MUON_CHROMIUM_BUILD)
return NavigatorBattery::From(navigator).getBattery(script_state);
+#else
+ return ScriptPromise{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think just an empty script promise will work correctly. In browser-laptop we call reject

Copy link
Collaborator

Choose a reason for hiding this comment

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

for this I'm wondering if we should override BatteryMonitorImpl::DidChange to always return 100%. @diracdeltas ?

Copy link
Member

Choose a reason for hiding this comment

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

@bridiver for webcompat, it's usually better if we pretend the API isn't supported at all (vs returning a fake value)

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, to accomplish that in browser laptop without breaking things we called reject in promise so I think we need to do return ScriptPromise::Reject(script_state, v8::Exception::TypeError(...))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also think it's probably better to replace the idl file if we're going to do things like this and have our own class for ImplementedAs


String NavigatorDoNotTrack::doNotTrack(Navigator& navigator) {
+#if defined(MUON_CHROMIUM_BUILD)
return NavigatorDoNotTrack::From(navigator).doNotTrack();
Copy link
Collaborator

Choose a reason for hiding this comment

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

do not track can be managed from a preference

Copy link
Collaborator

Choose a reason for hiding this comment

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

prefs::kEnableDoNotTrack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent here is to let the feature to work, but not return the status if DoNotTrack is enabled or not using the navigator api in js. cc: @diracdeltas

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I don't understand the purpose of that. We're sending the DNT header, so why would we return an empty value in js? If anything that would seem to make fingerprinting easier because only Brave would return an empty string

Copy link
Member

Choose a reason for hiding this comment

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

i'm also confused what this is doing. the code in browser laptop navigator.js is to make DNT work correctly in js, not to disable it.

}

USB* NavigatorUSB::usb(Navigator& navigator) {
+#if defined(MUON_CHROMIUM_BUILD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be disabled by feature kWebUsb

}

Bluetooth* NavigatorBluetooth::bluetooth(Navigator& navigator) {
+#if defined(MUON_CHROMIUM_BUILD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be controlled with WebRuntimeFeatures::EnableWebBluetooth

// Checks if the icon URL of |credential| is an a-priori authenticated URL.
// https://w3c.github.io/webappsec-credential-management/#dom-credentialuserdata-iconurl
+#if defined(MUON_CHROMIUM_BUILD)
bool IsIconURLEmptyOrSecure(const Credential* credential) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be controlled with features::kWebAuth

@bridiver
Copy link
Collaborator

shared array buffer can be controlled with features::kSharedArrayBuffer

@jumde
Copy link
Contributor Author

jumde commented May 24, 2018

SharedArrayBuffer is disabled by default: https://cs.chromium.org/chromium/src/content/public/common/content_features.cc?l=350

@bridiver
Copy link
Collaborator

@jumde yes, but we should also explicitly disable it in case they change the default in the future

@jumde jumde changed the title Disabling getBattery | bluetooth | credential.get/store | navigator.donottrack | navigator.usb apis [WIP] Disabling getBattery | bluetooth | credential.get/store | navigator.donottrack | navigator.usb apis May 25, 2018
@da2x
Copy link

da2x commented May 31, 2018

Now is a really bad time to remove Do Not Track. The current ePrivacy Regulation proposal (vote expected in 2018-Q3) would require websites to actually honor it. Think GDPR fines for ignoring it. Seriously. (The current draft also removes the need for cookie consent banners in favor of “browser settings”.)

Please keep Do Not Track! Both the HTTP header signal and the JavaScript API. Not supporting it would leave Brave users without some of the legal protections offered by the ePrivacy Regulation proposal.

@jumde
Copy link
Contributor Author

jumde commented Jun 1, 2018

Hello @da2x, this PR is still in progress. The plan is not to disable DNT, but prevent fingerprinting using the navigator.donottrack api.

@da2x
Copy link

da2x commented Jun 1, 2018

Hello @da2x, this PR is still in progress. The plan is not to disable DNT, but prevent fingerprinting using the navigator.donottrack api.

Stop! Listen to yourself. The plan is not to disable DNT but to disable DNT? The JavaScript API can be used for fingerprinting but so can the HTTP request header. Please read my comment again and reconsider this decision. Removing one but not the other would make Brave’s fingerprint more unique and not less.

The navigator.doNotTrack API is respected by almost 60 000 websites! and open-source tracking tools like Fathom and Matomo (formerly Piwik).

@jumde jumde changed the title [WIP] Disabling getBattery | bluetooth | credential.get/store | navigator.donottrack | navigator.usb apis [WIP] Disabling getBattery | bluetooth | credential.get/store | navigator.usb apis Jun 1, 2018
@jumde jumde force-pushed the disabling_tracking_apis branch 5 times, most recently from 94b3458 to 408996a Compare June 13, 2018 01:07
@jumde jumde changed the title [WIP] Disabling getBattery | bluetooth | credential.get/store | navigator.usb apis Disabling getBattery | bluetooth | credential.get/store | navigator.usb apis Jun 13, 2018
@@ -71,7 +72,14 @@ bool BraveMainDelegate::ShouldEnableProfilerRecording() {
return false;
}

void BraveMainDelegate::DisableAPIs() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be calling these in ContentRendererClient:: SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() in BraveContentRendererClient

@@ -8,6 +8,14 @@ if (is_win) {
}
}

source_set("modules") {
sources = [
"modules/battery/battery_disabled_api_s.h",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this source set should be 'blink' and match the path/class names. Then the patches aren't necessary

public:
NavigatorCredentialsDisabledAPIs() {
}
static CredentialsContainer* credentials(Navigator& navigator) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this still doesn't match the implementation in browser-laptop and it this will break some sites. We need to handle get and store and return a promise that resolves to false

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

need to investigate the patches because they should not be required - please do not merge

"This api has been disabled.")));
}

ScriptPromise CredentialsContainer::preventSilentAccess(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can just resolve without doing anything

ScriptPromise CredentialsContainer::get(
ScriptState* script_state,
const CredentialRequestOptions& options) {
v8::Isolate* isolate = V8PerIsolateData::MainThreadIsolate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

these should also resolve to null based on navigator.js from browser-laptop

const CredentialRequestOptions& options) {
ScriptPromise::InternalResolver resolver(script_state);
ScriptPromise promise = resolver.Promise();
v8::Isolate* isolate = V8PerIsolateData::MainThreadIsolate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can't assume the main thread isolate here. You should get the isolate from script_state->GetIsolate()

result = data;
});

function credentialsGetBlocked() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

result will be null either way. You should initialize result to a non-null value. In this particular case the test should be fine because we're resolving immediately, but in other cases a test like this would have a race condition because credentialsGetBlocked could be called before the promise resolves

@jumde jumde force-pushed the disabling_tracking_apis branch 3 times, most recently from 80e17a1 to dc26d3f Compare July 9, 2018 23:19
ScriptPromise::InternalResolver resolver(script_state);
ScriptPromise promise = resolver.Promise();
v8::Isolate* isolate = script_state->GetIsolate();
resolver.Resolve(v8::Null(isolate));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jumde jumde force-pushed the disabling_tracking_apis branch 2 times, most recently from 883e6af to 642efee Compare July 10, 2018 06:51
@bbondy
Copy link
Member

bbondy commented Jul 11, 2018

🎉

@bbondy bbondy merged commit 7cd5d53 into master Jul 11, 2018
@bbondy bbondy deleted the disabling_tracking_apis branch August 23, 2018 13:40
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
Youtube and Twitch videos now provide more accurate times than before
NejcZdovc pushed a commit that referenced this pull request Mar 6, 2019
@simonhong simonhong mentioned this pull request Jun 3, 2019
29 tasks
@yrliou yrliou mentioned this pull request Mar 8, 2023
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants