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

feat: $removeparam #4528

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

feat: $removeparam #4528

wants to merge 15 commits into from

Conversation

seia-soto
Copy link
Member

@seia-soto seia-soto commented Dec 13, 2024 •
edited
Loading

This adds experimental $removeparam support for adblocker: https://github.com/gorhill/ublock/wiki/static-filter-syntax#removeparam

The suggested matching process changed by this PR is illustrated as follows:

  1. $important (not subject to exceptions)
  2. redirection ($removeparam and $redirect=resource)
  3. normal filters
  4. exceptions
Details 
if $important is NOT found
	match filters IN redirect bucket
	+ categorize filters by the type: resources or removeparam
	+ check if any removeparam filters are applicable with the following conditions:
		1. `removeparam` filters not matched
		2. URL doesn't have `?` — parameter separator
		3. URL has `?` but ends right after the separator
		+ if empty removeparam filter found
			+ break the loop and set redirectUrl without any removeparam (remove all)
		+ if removeparam matched in the url
			+ break the loop and set redirectUrl
	match resource redirect filters
if filter is found AND exception is NOT found AND filter is redirectable
	+ if redirectUrl set
		+ redirect the request to redirectUrl
	+ ELSE
		if redirect of none found, set it as an exception and continue
		ELSE
			get resource and return as redirect

TODO

  • Add tests
  • Parsing regex as value (in separate PR with refactor and clean up of regex util funcs)

@seia-soto seia-soto added the PR: New Feature 🚀 Increment minor version when merged label Dec 13, 2024
@seia-soto seia-soto self-assigned this Dec 13, 2024
@seia-soto seia-soto added the WIP label Dec 13, 2024
chrmod
chrmod reviewed Dec 23, 2024
View reviewed changes
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/declarativeNetRequest/URLTransform

@seia-soto
Copy link
Member Author

Note: we're going to apply filters one by one

@seia-soto seia-soto removed the WIP label Dec 25, 2024
@seia-soto
Copy link
Member Author

Leaving a note: regexp wrapping: [\\?&](${value!.slice(1, flagSeparatorIndex)}[^=]*)

@seia-soto
refactor: improve matching search param key
47b1302
@seia-soto
test: removeparam
7ea4488 
test: add `isRemoveParam` and `isRedirectable`

chore: reject on `removeparam` negation

test: parsing removeparam

test: removeparam

test: removeparam
@seia-soto
test: removeparam removes parameter sequentially
29e9022
@seia-soto
fix(test): unintentional isRedirectable definition
3cb0861
@seia-soto seia-soto marked this pull request as ready for review December 27, 2024 11:03
@seia-soto seia-soto requested a review from remusao as a code owner December 27, 2024 11:03
@seia-soto seia-soto requested a review from chrmod December 27, 2024 11:03
remusao
remusao reviewed Jan 4, 2025
View reviewed changes
packages/adblocker/src/filters/network.ts Show resolved Hide resolved
packages/adblocker/src/filters/network.ts Outdated Show resolved Hide resolved
packages/adblocker/src/filters/network.ts Show resolved Hide resolved
packages/adblocker/test/parsing.test.ts Show resolved Hide resolved
packages/adblocker/src/engine/engine.ts Outdated Show resolved Hide resolved
@seia-soto seia-soto requested a review from remusao January 6, 2025 06:02
chrmod
chrmod reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@chrmod chrmod left a comment

Choose a reason for hiding this comment

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

Does this support exceptions like $removeparam=igshid,domain=instagram.com|threads.net?

packages/adblocker/src/filters/network.ts
@@ -1535,6 +1554,14 @@ export default class NetworkFilter implements IFilter {
options.push('badfilter');
}

if (this.isRemoveParam()) {
if (this.removeparam!.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

wont this throw if this.removeparam === undefined ?

Copy link
Member Author

@seia-soto seia-soto Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

removeparam is always string here, as we always set string in parse time. I'll add some comments or improve the type clarity.

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this type change then?

public get removeparam(): string | undefined {

https://github.com/ghostery/adblocker/pull/4528/files#diff-f45592ce8a6bd8a6e8e39391572f4a38406614b3649a6628891ca6fc38a516a9R1279

Copy link
Member

Choose a reason for hiding this comment

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

or just change it if (!this.removeparam) { or typeof this.removeparam === 'string' && this.removeparam.length > 0

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Other methods like get csp() has a same shape.
  • We use an empty value for the full removal of parameters.

chrmod
chrmod reviewed Jan 13, 2025
View reviewed changes
packages/adblocker/test/engine/engine.test.ts Outdated Show resolved Hide resolved
packages/adblocker/test/engine/engine.test.ts Outdated
@@ -600,6 +600,96 @@ $csp=baz,domain=bar.com
expect((filter as NetworkFilter).toString()).to.equal('||foo.com$image,redirect=foo.js');
expect(redirect).to.be.undefined;
});

context('removeparam', () => {
Copy link
Member

Choose a reason for hiding this comment

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

please add some tests for exceptions

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

@chrmod chrmod chrmod left review comments

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

Assignees

@seia-soto seia-soto

Labels
PR: New Feature 🚀 Increment minor version when merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seia-soto @chrmod @remusao