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

Hide query tracking parameter from Twitter Service Worker #16217

Closed
wants to merge 3 commits into from
Closed

Hide query tracking parameter from Twitter Service Worker #16217

wants to merge 3 commits into from

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Dec 3, 2022 •
edited
Loading

Fixes brave/brave-browser#23742

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:

Renderer-initiated:

  1. Open https://twitter.com/brave and leave open for 10 seconds so that the Service Worker is installed and active.
  2. Open https://fmarier.org/query-filter-test.html.
  3. Click on the last link on the page ("Twitter").
  4. Check that the fbclid= parameter is missing from the URL bar.

Browser-initiated:

  1. Open https://twitter.com/brave and leave open for 10 seconds so that the Service Worker is installed and active.
  2. Paste https://twitter.com/fmarier/status/1574869891978784769?s=21&t=lkfjeslkfjes&fbclid=1234 into the URL bar and press Enter.
  3. Check that the fbclid= parameter is missing from the URL bar.

@fmarier fmarier requested a review from goodov December 3, 2022 05:05
@fmarier fmarier self-assigned this Dec 3, 2022
@github-actions github-actions bot added the CI/run-network-audit Run network-audit label Dec 3, 2022
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch 2 times, most recently from f17e383 to c2be567 Compare December 8, 2022 03:03
@fmarier fmarier added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Dec 8, 2022
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from c2be567 to b1f3d8a Compare December 8, 2022 19:30
@goodov goodov force-pushed the twitter-t-serviceworker-26910 branch from b1f3d8a to 7891d96 Compare December 8, 2022 21:31
@goodov
Copy link
Member

goodov commented Dec 8, 2022

I hit rebase via Github UI to trigger CI, no other changes.

@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 7891d96 to cc60159 Compare December 9, 2022 04:05
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch 3 times, most recently from a4eb0c0 to 7640e4b Compare January 13, 2023 00:37
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch 13 times, most recently from 74828f1 to dff8766 Compare January 24, 2023 04:41
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch 3 times, most recently from 3caf8f8 to 760b7ce Compare January 24, 2023 21:34
@fmarier fmarier marked this pull request as ready for review January 25, 2023 00:42
@fmarier fmarier requested review from iefremov and a team as code owners January 25, 2023 00:42
goodov
goodov reviewed Jan 25, 2023
View reviewed changes
browser/net/BUILD.gn Outdated Show resolved Hide resolved
browser/ui/BUILD.gn Outdated Show resolved Hide resolved
chromium_src/content/browser/renderer_host/DEPS Outdated Show resolved Hide resolved
net/BUILD.gn Outdated Show resolved Hide resolved
net/query_filter/DEPS Outdated Show resolved Hide resolved
net/query_filter/query_filter.cc Outdated Show resolved Hide resolved
patches/content-browser-renderer_host-navigation_controller_impl.cc.patch Outdated Show resolved Hide resolved
patches/content-browser-renderer_host-render_frame_host_impl.cc.patch Outdated Show resolved Hide resolved
chromium_src/content/browser/renderer_host/navigation_controller_impl.cc Outdated Show resolved Hide resolved
browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc Outdated Show resolved Hide resolved
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 760b7ce to 88c2947 Compare January 26, 2023 04:28
goodov
goodov reviewed Jan 26, 2023
View reviewed changes
browser/ui/BUILD.gn Outdated Show resolved Hide resolved
net/BUILD.gn Outdated Show resolved Hide resolved
net/query_filter/query_filter.cc Outdated Show resolved Hide resolved
patches/content-browser-renderer_host-render_frame_host_impl.cc.patch Outdated Show resolved Hide resolved
net/query_filter/query_filter.cc Outdated Show resolved Hide resolved
net/BUILD.gn Outdated Show resolved Hide resolved
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 88c2947 to 0edbf9c Compare January 26, 2023 08:08
goodov
goodov approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

lgtm. please rebase and update patches before merging (cr110 has landed).
I'd also like someone else to have a second look into this. cc @bridiver @iefremov

chromium_src/content/browser/renderer_host/DEPS Outdated Show resolved Hide resolved
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 0edbf9c to 69a7ddf Compare January 26, 2023 21:29
@fmarier
Copy link
Member Author

fmarier commented Jan 26, 2023

Rebased onto latest master.

@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 69a7ddf to 0569dc6 Compare January 26, 2023 23:00
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 0569dc6 to 55a1345 Compare January 27, 2023 19:49
@fmarier
Copy link
Member Author

fmarier commented Feb 2, 2023

Due to brave/brave-browser#28184, I will be revising this PR to tackle the other query string trackers when interacting with service workers.

@fmarier fmarier marked this pull request as draft February 10, 2023 00:51
@fmarier fmarier changed the title Hide Twitter's tracking parameter from Service Workers Hide query tracking parameter from Twitter Service Worker Feb 10, 2023
@fmarier fmarier force-pushed the twitter-t-serviceworker-26910 branch from 55a1345 to f2c591a Compare February 10, 2023 01:02
@mihaiplesa mihaiplesa added the CI/skip Do not run CI builds (except noplatform) label Jun 9, 2023
@fmarier fmarier closed this Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodov goodov goodov approved these changes

@iefremov iefremov Awaiting requested review from iefremov iefremov is a code owner

@bridiver bridiver Awaiting requested review from bridiver bridiver will be requested when the pull request is marked ready for review bridiver is a code owner

Assignees

@fmarier fmarier

Labels
CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/skip Do not run CI builds (except noplatform)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query filter doesn't work with Service Workers
3 participants
@fmarier @goodov @mihaiplesa