Skip to content

Fix Allow Scripts Once logic for embedded scripts and iframes. #12023

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
Merged
merged 1 commit into from
Jan 28, 2022
Merged

Fix Allow Scripts Once logic for embedded scripts and iframes. #12023

merged 1 commit into from
Jan 28, 2022

Conversation

goodov
Copy link
Member

@goodov goodov commented Jan 27, 2022 •
edited by kjozwiak
Loading

Fixed Allow Scripts Once to work for page-embedded scripts.

Resolves brave/brave-browser#20744

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, npm run lint, 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:

@goodov goodov requested review from iefremov and darkdh January 27, 2022 16:39
@goodov goodov force-pushed the issues/20503-allowallonce branch from f74aa67 to 3e37e67 Compare January 27, 2022 16:41
@goodov goodov force-pushed the issues/20503-allowallonce branch from 3e37e67 to 92bc807 Compare January 27, 2022 16:42
@kjozwiak kjozwiak mentioned this pull request Jan 27, 2022
Closed
goodov
goodov commented Jan 28, 2022
View reviewed changes
components/content_settings/renderer/brave_content_settings_agent_impl.h
@@ -99,9 +95,6 @@ class BraveContentSettingsAgentImpl
// cache blocked script url which will later be used in `DidNotAllowScript()`
GURL blocked_script_url_;

// temporary allowed script origins we preloaded for the next load
base::flat_set<std::string> preloaded_temporarily_allowed_scripts_;
Copy link
Member Author

@goodov goodov Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this doesn't work on reloads, an embedded script is executed before this is applied.

@goodov
Copy link
Member Author

goodov commented Jan 28, 2022

@darkdh @iefremov PTAL

goodov
goodov commented Jan 28, 2022
View reviewed changes
components/content_settings/renderer/brave_content_settings_agent_impl.cc
@@ -342,7 +350,7 @@ bool BraveContentSettingsAgentImpl::AllowAutoplay(bool play_requested) {

void BraveContentSettingsAgentImpl::SetAllowScriptsFromOriginsOnce(
const std::vector<std::string>& origins) {
preloaded_temporarily_allowed_scripts_ = std::move(origins);
temporarily_allowed_scripts_ = origins;
Copy link
Member Author

Choose a reason for hiding this comment

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

origins is not movable (it's a mojo-generated method).

darkdh
darkdh approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

lgtm

@mkarolin
Copy link
Collaborator

Merging per @rebron

@mkarolin mkarolin merged commit 6d6635b into master Jan 28, 2022
@mkarolin mkarolin deleted the issues/20503-allowallonce branch January 28, 2022 22:45
@mkarolin mkarolin added this to the 1.37.x - Nightly milestone Jan 28, 2022
brave-builds pushed a commit that referenced this pull request Jan 28, 2022
@brave-browser-releases
Uplift of #12023 (squashed) to beta
665531c
brave-builds pushed a commit that referenced this pull request Jan 28, 2022
@brave-browser-releases
Uplift of #12023 (squashed) to release
59a95fc
This was referenced Jan 28, 2022
Merged
Merged
@goodov goodov mentioned this pull request Jan 29, 2022
Closed
@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build:

Brave | 1.37.10 Chromium: 98.0.4758.74 (Official Build) nightly (64-bit)
-- | --
Revision | d0fe1ec4df090cd3eb02b591228505e12ea476e9-refs/branch-heads/4758@{#935}
OS | Windows 11 Version 21H2 (Build 22000.438)

Used https://www.blizzard.com/en-gb for the following cases to ensure that both Scripts blocked & Allow scripts once are working as expected:

  • ensured that scripts are correctly being blocked when Scripts blocked has been enabled
  • ensured that the page loads correctly once Scripts blocked has been disabled
  • ensured that the correct state is maintained when restarting the browser or closing the tab/website
  • ensured that clicking on down arrow near the number of scripts being blocked expands shields with more information
  • ensured that Scripts on this site is correctly listing the scripts currently being blocked
  • ensured that you can allow/block individual scripts via Scripts on this site
  • ensured that Allow all works as expected under Scripts on this site
  • ensured Allow scripts once only works for that instance/tab (scripts should be blocked when loading the page again in a different tab if enabled)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@iefremov iefremov Awaiting requested review from iefremov

Assignees

@goodov goodov

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

Successfully merging this pull request may close these issues.

[Security] shields not tracking blocked embedded scripts breaking Allow scripts once
4 participants
@goodov @mkarolin @kjozwiak @darkdh