Implement per-origin noscript #97

yrliou · 2018-05-02T04:39:59Z

Fix brave/brave-browser#6
Support per-tab per-origin noScript settings which will be cleared when navigating away.
See also: brave/brave-extension/pull/21

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).
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.

Test Plan:

An automated browser test is added for this feature.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

yrliou · 2018-05-08T21:16:10Z

Built on Mac, will make sure other platforms built before merging.

yrliou · 2018-05-08T22:14:40Z

Please leave this PR for me to merge since I would like to include extension DEPS update commit in this PR when everything is reviewed and ready to merged.

bbondy

I think we can ship initially with the UI as is but ideally we'll one day modify this:

Please try to customize the "JavaScript was blocked on this page" message. To indicate they should check the Brave shields panel. But since this string is in chrome/app/generated_resources.grd and we don't override it yet, we should override that as a separate PR and do that at that point. Posting a task for this would be fine for now.

While testing I somehow had an extension show up, which makes me think we blocked some extension we shouldn't have. I don't have any extra extensions installed, but it looks like it's from chrome/browser/resources/media_router.

bbondy · 2018-05-11T14:57:12Z

renderer/brave_content_settings_observer.cc

@@ -25,14 +26,55 @@ BraveContentSettingsObserver::BraveContentSettingsObserver(
 BraveContentSettingsObserver::~BraveContentSettingsObserver() {
 }

+void BraveContentSettingsObserver::DidFinishLoad() {
+  temporarily_allowed_scripts_.clear();


Doing this clear here will cause a bad experience for NoScript users because a lot of resource loads for an origin happen after the onload. On browser-laptop in frame.js we clear it when the component is unmounted (tab is closed) or when the user navigates to a different origin.
I think we could instead clear it when there's a different origin than was set in allow once in WillCommitProvisionalLoad or something like that.
You can try it out on my website to see how it doesn't really work well. It probably works for some test sites just fine though.
We don't have to be consistent completely with how browser-laptop works but just more usable.

bbondy · 2018-05-11T15:01:39Z

renderer/brave_content_settings_observer.cc

+  temporarily_allowed_scripts_.clear();
+}
+
+bool BraveContentSettingsObserver::OnMessageReceived(const IPC::Message& message) {


Just noting an alternate way to do this might be to create temporary content settings that we later clear. But I'm ok with the current method you used.

that actually doesn't work because it will affect other loads that might be happening in the background

even this had to be tweaked slightly from the original to avoid any race conditoins

bbondy · 2018-05-11T15:34:01Z

test/data/load_js_from_origins.html

+<html><head><title>load js from origins</title></head>
+<body>
+<script src="/cross-site/a.com/create_iframe.js"></script>
+<script src="/cross-site/b.com/create_iframe.js"></script>


I think these paths are wrong which I think would make the tests fail.

Addressed, comments about how this works are added into html file.

yrliou · 2018-05-15T06:38:34Z

browser/extensions/api/brave_shields_api.cc

+
+  // Send IPC msg to update the list in this tab's content settings observers
+  contents->SendToAllFrames(
+      new BraveFrameMsg_AllowScriptsOnce(MSG_ROUTING_NONE, params->origins));


Noted there is an issue here that frames which might be created after this will not get this list.
Probably could be solved by saving the list (which is specific to one webcontent) in the browser process (maybe in BraveWebContentsObserver or BraveTabSpecificContentSetting) and send the list in RenderFrameCreated to those subsequent frames that are created later.

addressed by 00cfdb3

bridiver · 2018-05-15T17:20:15Z

renderer/brave_content_settings_observer.cc

+
+void BraveContentSettingsObserver::OnAllowScriptsOnce(
+    const std::vector<std::string>& origins) {
+  temporarily_allowed_scripts_.clear();


do we need an extra setting here or can we just set the existing cached_script_permissions_? cached_script_permissions_ gets cleared on navigation

yrliou · 2018-05-19T05:30:08Z

@bbondy, @bridiver PR is updated, please take a look again when you're available, thanks!
Main change here is to maintain a set in BraveShieldsWebContentsObserver which will be propagated to render frames in ReadyToCommitNavigation, and the set would be cleared when navigating away.
In renderer side, origins passed from browser process will first be preloaded into preloaded_temporarily_allowed_scripts_.
Then it will be loaded into temporarily_allowed_scripts_ in DidCommitProvisionalLoad.
The reason of adding a preloaded var is to avoid wrongly apply the no-script setting which is meant for the navigation that's going to be committed to things in the current page load.

bbondy

lgtm

Autocontribute monthly values other than defaults are now added

yrliou added the pr/work-in-progress label May 2, 2018

yrliou self-assigned this May 3, 2018

yrliou force-pushed the noscript branch 2 times, most recently from eee42b2 to 300dda7 Compare May 8, 2018 20:43

yrliou changed the title ~~[WIP] Implement per-origin noscript~~ Implement per-origin noscript May 8, 2018

yrliou mentioned this pull request May 8, 2018

Per-origin noScript brave/brave-extension#21

Merged

yrliou requested a review from bbondy May 8, 2018 21:21

yrliou removed the pr/work-in-progress label May 8, 2018

bbondy requested changes May 11, 2018

View reviewed changes

yrliou force-pushed the noscript branch from 300dda7 to ea677d7 Compare May 15, 2018 05:24

yrliou mentioned this pull request May 15, 2018

Remove chrome's noscript UI brave/brave-browser#199

Closed

yrliou commented May 15, 2018

View reviewed changes

bridiver reviewed May 15, 2018

View reviewed changes

yrliou force-pushed the noscript branch 9 times, most recently from 6284061 to 9283244 Compare May 19, 2018 05:22

bbondy previously approved these changes May 22, 2018

View reviewed changes

yrliou added 3 commits May 23, 2018 08:39

Implement per-origin noscript

989fb32

Add a browser test for allowScriptsOnce API

ef5c809

Update brave-extension DEPS

071a1ee

yrliou dismissed bbondy’s stale review via 071a1ee May 23, 2018 16:54

yrliou force-pushed the noscript branch from 9283244 to 071a1ee Compare May 23, 2018 16:54

bbondy approved these changes May 23, 2018

View reviewed changes

yrliou merged commit c91d571 into master May 23, 2018

yrliou deleted the noscript branch May 30, 2018 20:09

NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018

Merge pull request #97 from brave-intl/monthly_value

8710d16

Autocontribute monthly values other than defaults are now added

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement per-origin noscript #97

Implement per-origin noscript #97

yrliou commented May 2, 2018 •

edited

Loading

yrliou commented May 8, 2018

yrliou commented May 8, 2018

bbondy left a comment

bbondy May 11, 2018

bbondy May 11, 2018

bridiver May 16, 2018

bridiver May 16, 2018

bbondy May 11, 2018

yrliou May 19, 2018

yrliou May 15, 2018

yrliou May 16, 2018

bridiver May 15, 2018

yrliou commented May 19, 2018 •

edited

Loading

bbondy left a comment

Implement per-origin noscript #97

Implement per-origin noscript #97

Conversation

yrliou commented May 2, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

yrliou commented May 8, 2018

yrliou commented May 8, 2018

bbondy 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

yrliou commented May 19, 2018 • edited Loading

bbondy left a comment

Choose a reason for hiding this comment

yrliou commented May 2, 2018 •

edited

Loading

yrliou commented May 19, 2018 •

edited

Loading