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

Added Custom Scriptlet section in the settings page. #25999

Merged
merged 21 commits into from
Dec 13, 2024
Merged

Conversation

boocmp
Copy link
Contributor

@boocmp boocmp commented Oct 15, 2024

Resolves brave/brave-browser#25586

Empty list

image

Add new scriplet

image

Add new scriplet with code

image

Scriptlet in the list and custom filter

image

Execution on the page

image

Dev mode toggle
dev_mode.mp4

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 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:

@boocmp boocmp self-assigned this Oct 15, 2024
@github-actions github-actions bot added chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch CI/storybook-url Deploy storybook and provide a unique URL for each build and removed chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch labels Oct 15, 2024
@brave-builds
Copy link
Collaborator

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

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Oct 20, 2024
@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Oct 21, 2024
@boocmp boocmp marked this pull request as ready for review October 21, 2024 11:10
@boocmp boocmp requested review from a team as code owners October 21, 2024 11:10
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.

strings++ (with 1 change)

@boocmp boocmp requested review from ShivanKaul and a team October 24, 2024 01:10
app/brave_settings_strings.grdp Outdated Show resolved Hide resolved
@ShivanKaul
Copy link
Collaborator

@boocmp how do we handle naming collisions? For instance, if a user tries to create a new scriptlet called brave-fix which conflicts with an existing scriptlet, how do we handle that?

@boocmp
Copy link
Contributor Author

boocmp commented Oct 31, 2024

We don't check. Default resources have priority over customs so if it's called 'brave-fix` it will be ignored.

how do we handle naming collisions? For instance, if a user tries to create a new scriptlet called brave-fix which conflicts with an existing scriptlet, how do we handle that?

Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

@diracdeltas diracdeltas self-requested a review November 13, 2024 17:39
@boocmp boocmp requested review from a team as code owners November 14, 2024 15:19

if (*mime == kAppJs) {
// Resource is a scriptlet:
if (!name->starts_with("brave-") || !name->ends_with(".js")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to enforce certain prefixes on user scriptlets, we should reserve brave- for Brave's CRX-provided scriptlets, and give user-provided scriptlets a user- prefix instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense to me too

Copy link
Contributor

[puLL-Merge] - brave/brave-core@25999

Description

This PR adds support for custom scriptlets in the AdBlock feature of the Brave browser. It introduces a developer mode that allows users to add, edit, and remove custom scriptlets, which are small JavaScript snippets used for ad blocking and content modification.

Possible Issues

  1. Enabling custom scriptlets could potentially introduce security risks if users add malicious scripts.
  2. The feature is behind a feature flag, which might limit its availability to users.

Security Hotspots

  1. Custom scriptlet execution: The ability for users to add and execute custom JavaScript could be a potential security risk if not properly sanitized or restricted.
  2. Developer mode toggle: Ensure that enabling developer mode doesn't inadvertently expose sensitive functionality to regular users.
Changes

Changes

  1. app/brave_settings_strings.grdp:

    • Added new string resources for custom scriptlet UI elements and messages.
  2. browser/about_flags.cc:

    • Added a new feature flag for Brave AdBlock Custom Scriptlets.
  3. browser/brave_profile_prefs.cc:

    • Registered a new boolean preference for AdBlock developer mode.
  4. browser/brave_shields/ad_block_custom_resources_browsertest.cc:

    • Added new browser tests for custom scriptlet functionality.
  5. browser/brave_shields/ad_block_pref_service_factory.cc:

    • Updated to include the new developer mode preference.
  6. browser/resources/settings/default_brave_shields_page/:

    • Added new UI components for managing custom scriptlets.
  7. browser/ui/webui/brave_adblock_ui.cc:

    • Updated to handle custom scriptlet-related UI interactions.
  8. browser/ui/webui/settings/brave_adblock_handler.cc:

    • Added methods to handle custom scriptlet operations (add, update, remove).
  9. components/brave_shields/content/browser/ad_block_service.cc:

    • Modified to support custom scriptlets and developer mode.
  10. components/brave_shields/core/browser/ad_block_custom_resource_provider.cc:

    • Implemented the core functionality for managing custom scriptlets.
  11. components/brave_shields/core/common/features.cc:

    • Added a new feature flag for custom scriptlets.
  12. components/brave_shields/core/common/pref_names.h:

    • Added a new preference name for AdBlock developer mode.
sequenceDiagram
    User->>BraveSettings: Enable AdBlock Developer Mode
    BraveSettings->>AdBlockService: EnableDeveloperMode(true)
    User->>BraveSettings: Add Custom Scriptlet
    BraveSettings->>AdBlockCustomResourceProvider: AddResource(scriptlet)
    AdBlockCustomResourceProvider->>Storage: Save Custom Scriptlet
    AdBlockCustomResourceProvider->>AdBlockService: ReloadResourcesAndNotify()
    AdBlockService->>AdBlockEngine: Update with new scriptlet
    User->>Webpage: Visit
    Webpage->>AdBlockEngine: Check for blocking
    AdBlockEngine->>Webpage: Apply Custom Scriptlet
Loading

pref_change_registrar_->Add(
prefs::kAdBlockDeveloperMode,
base::BindRepeating(&AdBlockPrefService::OnDeveloperModeChanged,
base::Unretained(this)));
Copy link
Contributor

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated


Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml


Cc @thypon @goodov @iefremov

@boocmp boocmp merged commit 1f0e2fc into master Dec 13, 2024
19 of 20 checks passed
@boocmp boocmp deleted the issues/25586 branch December 13, 2024 05:22
@github-actions github-actions bot added this to the 1.75.x - Nightly milestone Dec 13, 2024
@brave-builds
Copy link
Collaborator

Released in v1.75.99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/storybook-url Deploy storybook and provide a unique URL for each build needs-security-review puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to load custom adblock scriptlets
10 participants