re-add the ability to allow/block individual scripts in Shields (webui part) #17221

spylogsster · 2023-02-15T05:33:35Z

Resolves brave/brave-browser#28510

block.mp4

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Go to cnn.com and block/allow scripts in Shield panel

brave-builds · 2023-02-17T08:15:09Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-20T08:03:00Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-20T11:29:15Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-20T13:27:26Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-22T12:37:17Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-24T15:04:49Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-03-09T09:14:09Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-03-09T10:52:29Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-03-09T14:03:14Z

A Storybook has been deployed to preview UI for the latest push

nullhook · 2023-03-09T18:30:42Z

components/brave_shields/resources/panel/components/tree-list/resource-element.tsx

+      <div className={urlTextClass} onClick={handleTextClick}>
+        {props.path ? props.path : props.host}
+      </div>
+      {props.onPermissionButtonClick && (


If onPermissionButtonClick is a required prop, why are we checking if it exists here? If the permission button is conditionally rendering, it would make sense to allow null/undefined values for the prop, which should be consistent throughout the call stack.

it can be undefined as suggested by @fallaciousreasoning above #17221 (comment)

export type PermissionButtonHandler = ((name: string) => void) | undefined

false was not compiled, undefined works

the handler was suggested in #17221 (comment)

Changed type of onPermissionButtonClick and made parameter optional

nullhook

Looks good!

brave-builds · 2023-03-09T20:50:13Z

A Storybook has been deployed to preview UI for the latest push

fallaciousreasoning

Thanks for that @spylogsster!

brave-builds · 2023-03-10T06:25:23Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2023-03-13T18:53:59Z

browser/ui/brave_shields_data_controller.h

@@ -78,6 +78,7 @@ class BraveShieldsDataController
  void SetIsNoScriptEnabled(bool is_enabled);
  void SetIsHTTPSEverywhereEnabled(bool is_enabled);
  void AllowScriptsOnce(const std::vector<std::string>& origins);
+  void BlockAllowedScripts(const std::vector<std::string>& origins);


block allowed scripts? Can you please add a comment explaining what it does because it makes no sense to block an allowed script.

I think this should probably be BlockScriptsOnce to match AllowScriptsOnce?

bridiver · 2023-03-13T19:05:45Z

I don't see any tests to verify that this actually blocks/allows the individual scripts on the page and I'm not really sure how it can work correctly given that

bool BraveContentSettingsAgentImpl::IsScriptTemporilyAllowed(
    const GURL& script_url) {
  // Check if scripts from this origin are temporily allowed or not.
  // Also matches the full script URL to support data URL cases which we use
  // the full URL to allow it.
  bool allow = base::Contains(temporarily_allowed_scripts_,
                              url::Origin::Create(script_url).Serialize()) ||
               base::Contains(temporarily_allowed_scripts_, script_url.spec());

hasn't changed. The test here https://github.com/brave/brave-core/pull/17152/files#diff-7274b75afeaaeb25be8467441f13ad371269f1ba1c637e3fee2a1714f80c8003R201 is not sufficient because you need two different scripts with the same origin to verify that you are blocking by the full script url and not just the origin.

If I'm missing something here and this does work then at the very least the comment needs to be changed, but I'm pretty sure it won't work correctly with more than one script from the same domain

bridiver · 2023-03-13T19:50:40Z

browser/brave_shields/brave_shields_web_contents_observer.cc

+    bool is_origin = origin.Serialize() == script;
+    base::EraseIf(allowed_scripts_, [is_origin, script,
+                                     origin](const std::string& value) {
+      // scripts array may have both origins or full scripts paths.


SetAllowScriptsFromOriginsOnce only matches based on origin so having a full script url in this list seems confusing at best

github-actions bot assigned spylogsster Feb 15, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Feb 15, 2023

spylogsster force-pushed the brave-24593-webui branch from 63d2d85 to d52ed16 Compare February 15, 2023 06:41

spylogsster force-pushed the brave-24593 branch from c49dffa to 0c7be0e Compare February 15, 2023 06:44

spylogsster force-pushed the brave-24593-webui branch from d52ed16 to 4ffdd84 Compare February 15, 2023 06:45

spylogsster requested review from nullhook and fallaciousreasoning February 15, 2023 06:47

spylogsster added the CI/run-draft label Feb 16, 2023

spylogsster force-pushed the brave-24593-webui branch from 28b10c7 to c77b2e3 Compare February 16, 2023 20:08

spylogsster force-pushed the brave-24593 branch from 0c7be0e to 344cbb4 Compare February 17, 2023 05:23

spylogsster force-pushed the brave-24593-webui branch from c77b2e3 to e55e091 Compare February 17, 2023 05:24

spylogsster force-pushed the brave-24593 branch 4 times, most recently from dc72839 to 131a0ed Compare February 20, 2023 06:49

spylogsster force-pushed the brave-24593-webui branch from befa071 to 8161ddb Compare February 20, 2023 06:49

Base automatically changed from brave-24593 to master February 20, 2023 10:21

spylogsster force-pushed the brave-24593-webui branch 3 times, most recently from 1c213e4 to 03127ab Compare February 20, 2023 10:49

spylogsster force-pushed the brave-24593-webui branch from 03127ab to 689e903 Compare February 20, 2023 12:31

spylogsster force-pushed the brave-24593-webui branch from 689e903 to 4eb1f38 Compare February 22, 2023 11:47

spylogsster force-pushed the brave-24593-webui branch from 4eb1f38 to c7069a4 Compare February 24, 2023 14:02

spylogsster force-pushed the brave-24593-webui branch from c7069a4 to a349b43 Compare February 27, 2023 08:54

spylogsster marked this pull request as draft March 9, 2023 07:46

spylogsster marked this pull request as ready for review March 9, 2023 10:04

spylogsster mentioned this pull request Mar 9, 2023

Scripts disappear during grouping brave/brave-browser#28972

Open

spylogsster force-pushed the brave-24593-webui branch from 91f5589 to a6c5106 Compare March 9, 2023 10:41

spylogsster requested a review from nullhook March 9, 2023 10:48

spylogsster force-pushed the brave-24593-webui branch from a6c5106 to cbb16e6 Compare March 9, 2023 13:42

nullhook suggested changes Mar 9, 2023

View reviewed changes

spylogsster force-pushed the brave-24593-webui branch from be2c26d to 510212d Compare March 9, 2023 20:35

nullhook self-requested a review March 9, 2023 20:45

nullhook approved these changes Mar 9, 2023

View reviewed changes

fallaciousreasoning approved these changes Mar 9, 2023

View reviewed changes

spylogsster added 6 commits March 10, 2023 09:13

Allow individual scripts once

82cd793

Review/Style fixes

57baec6

Removed view states and refactor lists

2b15529

Replaced allowedSectionVisible by allowedList.length

f5f3f61

Sorted parameters and fixed block specific scripts by full urls

04e6fb3

onPermissionButtonClick type change

ab765c0

spylogsster force-pushed the brave-24593-webui branch from 510212d to ab765c0 Compare March 10, 2023 06:13

spylogsster merged commit 825c012 into master Mar 10, 2023

spylogsster deleted the brave-24593-webui branch March 10, 2023 08:15

github-actions bot added this to the 1.51.x - Nightly milestone Mar 10, 2023

bridiver reviewed Mar 13, 2023

View reviewed changes

antonok-edm mentioned this pull request Mar 21, 2023

Scripts can't be unblocked with custom filters brave/brave-browser#29215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-add the ability to allow/block individual scripts in Shields (webui part) #17221

re-add the ability to allow/block individual scripts in Shields (webui part) #17221

spylogsster commented Feb 15, 2023 •

edited

Loading

brave-builds commented Feb 17, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 22, 2023

brave-builds commented Feb 24, 2023

brave-builds commented Mar 9, 2023

brave-builds commented Mar 9, 2023

brave-builds commented Mar 9, 2023

nullhook Mar 9, 2023

spylogsster Mar 9, 2023 •

edited

Loading

spylogsster Mar 9, 2023

spylogsster Mar 9, 2023

nullhook left a comment

brave-builds commented Mar 9, 2023

fallaciousreasoning left a comment

brave-builds commented Mar 10, 2023

bridiver Mar 13, 2023

bridiver Mar 13, 2023

bridiver commented Mar 13, 2023 •

edited

Loading

bridiver Mar 13, 2023

re-add the ability to allow/block individual scripts in Shields (webui part) #17221

re-add the ability to allow/block individual scripts in Shields (webui part) #17221

Conversation

spylogsster commented Feb 15, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Feb 17, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 20, 2023

brave-builds commented Feb 22, 2023

brave-builds commented Feb 24, 2023

brave-builds commented Mar 9, 2023

brave-builds commented Mar 9, 2023

brave-builds commented Mar 9, 2023

nullhook Mar 9, 2023

Choose a reason for hiding this comment

spylogsster Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

spylogsster Mar 9, 2023

Choose a reason for hiding this comment

spylogsster Mar 9, 2023

Choose a reason for hiding this comment

nullhook left a comment

Choose a reason for hiding this comment

brave-builds commented Mar 9, 2023

fallaciousreasoning left a comment

Choose a reason for hiding this comment

brave-builds commented Mar 10, 2023

bridiver Mar 13, 2023

Choose a reason for hiding this comment

bridiver Mar 13, 2023

Choose a reason for hiding this comment

bridiver commented Mar 13, 2023 • edited Loading

bridiver Mar 13, 2023

Choose a reason for hiding this comment

spylogsster commented Feb 15, 2023 •

edited

Loading

spylogsster Mar 9, 2023 •

edited

Loading

bridiver commented Mar 13, 2023 •

edited

Loading