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.

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

Sanitize URL when it is copied into clipboard by JS. #24207

Merged
merged 10 commits into from
Jun 28, 2024
Merged

Sanitize URL when it is copied into clipboard by JS. #24207

merged 10 commits into from
Jun 28, 2024

Conversation

boocmp
Copy link
Contributor

@boocmp boocmp commented Jun 14, 2024 •
edited
Loading

Resolves brave/brave-browser#33037

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:

See in the issue
Enable feature --enable-features=BraveCopyCleanLinkFromJs
Test page https://boocmp.github.io/clipboard/

@boocmp boocmp requested review from fmarier and iefremov June 14, 2024 11:58
@boocmp boocmp self-assigned this Jun 14, 2024
@boocmp boocmp requested review from a team as code owners June 14, 2024 11:58
@boocmp boocmp force-pushed the clipboard branch 2 times, most recently from ff393b6 to 116bd7a Compare June 14, 2024 15:53
fmarier
fmarier reviewed Jun 14, 2024
View reviewed changes
browser/url_sanitizer/url_sanitizer_browsertest.cc Outdated
constexpr const char kYoutubeRules[] = R"json(
[{
"include": [
"*://youtu.be/*?*",
Copy link
Member

Choose a reason for hiding this comment

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

This can be just *://youtu.be/*.

@boocmp boocmp force-pushed the clipboard branch 5 times, most recently from 81628fd to b0c50f5 Compare June 17, 2024 09:51
iefremov
iefremov reviewed Jun 19, 2024
View reviewed changes
browser/brave_browser_features.cc Outdated
@@ -24,6 +24,10 @@ BASE_FEATURE(kBraveCopyCleanLinkByDefault,
#endif
);

BASE_FEATURE(kBraveCopyCleanLinkFromJs,
"brave-copy-clean-link-from-js",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we name it BraveCopyCleanLinkFromJs for ease of search

iefremov
iefremov reviewed Jun 27, 2024
View reviewed changes
test/BUILD.gn Outdated
@@ -1119,6 +1119,7 @@ test("brave_browser_tests") {
"//brave/browser/brave_resources_browsertest.cc",
"//brave/browser/ssl/certificate_transparency_browsertest.cc",
"//brave/browser/ui/views/toolbar/wallet_button_notification_source_browsertest.cc",
"//brave/browser/url_sanitizer/url_sanitizer_browsertest.cc",
Copy link
Contributor

Choose a reason for hiding this comment

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

put it into a sepratate browser_tests target in b/b/url_sanitizer/BUILD.gn
this old way is kinda deprecated

iefremov
iefremov reviewed Jun 27, 2024
View reviewed changes
browser/brave_content_browser_client.cc Outdated
if (!base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkFromJs)) {
return url;
}
DCHECK(render_frame_host);
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer CHECK over DCHECK
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#check_dcheck_notreached_noreturn_and-notreached_in_migration

iefremov
iefremov reviewed Jun 27, 2024
View reviewed changes
browser/brave_content_browser_client.cc
}
DCHECK(render_frame_host);
DCHECK(render_frame_host->GetBrowserContext());
auto* url_sanitizer_service =
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe CHECK it as well? is it always non-null in tor/incognito ?

@github-actions GitHub Actions
Copy link
Contributor

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

Description

This PR introduces a new feature for sanitizing URLs when copying them to the clipboard. It adds the ability to clean URLs based on predefined rules, particularly focusing on YouTube URLs. The feature can be enabled or disabled using a feature flag.

Changes

Changes

  1. browser/brave_browser_features.cc and browser/brave_browser_features.h:

    • Added a new feature flag kBraveCopyCleanLinkFromJs for enabling URL sanitization from JavaScript.

  2. browser/brave_content_browser_client.cc and browser/brave_content_browser_client.h:

    • Implemented SanitizeURL method to clean URLs before copying to clipboard.

  3. browser/ui/views/omnibox/brave_omnibox_view_views_browsertest.cc:

    • Updated tests to use the new SetSanitizerRules method.

  4. browser/url_sanitizer/:

    • Added new files for URL sanitizer browser tests.

  5. components/url_sanitizer/browser/:

    • Updated URLSanitizerComponentInstaller and URLSanitizerService to support new configuration structure and JavaScript API permissions.

  6. chromium_src/:

    • Added patches to integrate URL sanitization into Chromium's clipboard operations.

  7. test/data/url_sanitizer/js_api.html:

    • Added a test HTML file for JavaScript API testing.

Possible Issues

  1. The feature is currently disabled by default, which might limit its initial impact.
  2. The implementation assumes that all YouTube URLs should be sanitized, which might not always be desirable.

Security Hotspots

  1. browser/brave_content_browser_client.cc: The SanitizeURL method checks permissions before sanitizing, but ensure that this check is comprehensive and cannot be bypassed.
  2. components/url_sanitizer/browser/url_sanitizer_service.cc: The CheckJsPermission method should be carefully reviewed to ensure it correctly restricts access to the sanitization API.

This PR significantly enhances Brave's URL handling capabilities, particularly for cleaning potentially sensitive information from URLs before they are copied to the clipboard. The implementation appears thorough, with appropriate test coverage and integration into existing browser components.

@boocmp boocmp enabled auto-merge (squash) June 28, 2024 12:08
@boocmp boocmp force-pushed the clipboard branch 2 times, most recently from 6b26d6e to 8e2864e Compare June 28, 2024 12:50
@boocmp boocmp merged commit 8c27621 into master Jun 28, 2024
19 of 20 checks passed
@boocmp boocmp deleted the clipboard branch June 28, 2024 15:22
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jun 28, 2024
@MadhaviSeelam MadhaviSeelam mentioned this pull request Jul 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmarier fmarier fmarier approved these changes

@iefremov iefremov iefremov approved these changes

Assignees

@boocmp boocmp

Labels
puLL-Merge
Projects
None yet
Milestone
1.69.x - Release
Development

Successfully merging this pull request may close these issues.

Add YouTube si param (JS) to copy clean link filter
3 participants
@boocmp @fmarier @iefremov