From 94f58686a91ab19ea202514b2119f840b7d42422 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 13 Dec 2024 16:06:49 +0900 Subject: [PATCH 01/15] feat: `$removeparam` --- packages/adblocker/src/engine/engine.ts | 70 +++++++++++++++-------- packages/adblocker/src/filters/network.ts | 10 ++++ 2 files changed, 56 insertions(+), 24 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index 007710642a..218aebc902 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -8,6 +8,8 @@ import type { IMessageFromBackground } from '@ghostery/adblocker-content'; +import { URLSearchParams } from 'node:url'; + import Config from '../config.js'; import { StaticDataView, sizeOfASCII, sizeOfByte, sizeOfBool } from '../data-view.js'; import { EventEmitter } from '../events.js'; @@ -779,7 +781,7 @@ export default class FilterEngine extends EventEmitter { exceptions.push(filter); } else if (filter.isImportant()) { importants.push(filter); - } else if (filter.isRedirect()) { + } else if (filter.isRedirect() || filter.isRemoveParam()) { redirects.push(filter); } else { filters.push(filter); @@ -1450,6 +1452,7 @@ export default class FilterEngine extends EventEmitter { let redirectNone: NetworkFilter | undefined; let redirectRule: NetworkFilter | undefined; + let redirectUrl: string | undefined; // If `result.filter` is `undefined`, it means there was no $important // filter found so far. We look for a $redirect filter. There is some @@ -1460,21 +1463,40 @@ export default class FilterEngine extends EventEmitter { // * Else if redirect-rule is found, only redirect if request would be blocked. // * Else if redirect is found, redirect. if (result.filter === undefined) { - const redirects = this.redirects - .matchAll(request, this.isFilterExcluded.bind(this)) - // highest priorty wins - .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); - - if (redirects.length !== 0) { - for (const filter of redirects) { - if (filter.getRedirectResource() === 'none') { - redirectNone = filter; - } else if (filter.isRedirectRule()) { - if (redirectRule === undefined) { - redirectRule = filter; + const redirectFilters = this.redirects.matchAll(request, this.isFilterExcluded.bind(this)); + const resourceRedirects: NetworkFilter[] = []; + const requestRedirects: NetworkFilter[] = []; + for (const filter of redirectFilters) { + if (filter.isRemoveParam()) { + requestRedirects.push(filter); + } else { + resourceRedirects.push(filter); + } + } + + const searchParamsIndex = request.url.indexOf('?') + 1; + if (searchParamsIndex !== 0 && resourceRedirects.length !== 0) { + const searchParams = new URLSearchParams(request.url.slice(searchParamsIndex + 1)); + for (const redirect of requestRedirects) { + searchParams.delete(redirect.optionValue!); + } + redirectUrl = `${request.url.slice(0, searchParamsIndex)}?${searchParams.toString()}`; + } else { + const redirects = resourceRedirects + // highest priorty wins + .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); + + if (redirects.length !== 0) { + for (const filter of redirects) { + if (filter.getRedirectResource() === 'none') { + redirectNone = filter; + } else if (filter.isRedirectRule()) { + if (redirectRule === undefined) { + redirectRule = filter; + } + } else if (result.filter === undefined) { + result.filter = filter; } - } else if (result.filter === undefined) { - result.filter = filter; } } } @@ -1505,15 +1527,15 @@ export default class FilterEngine extends EventEmitter { // // 1. Check if a redirect=none rule was found, which acts as exception. // 2. If no exception was found, prepare `result.redirect` response. - if ( - result.filter !== undefined && - result.exception === undefined && - result.filter.isRedirect() - ) { - if (redirectNone !== undefined) { - result.exception = redirectNone; - } else { - result.redirect = this.resources.getResource(result.filter.getRedirectResource()); + if (result.filter !== undefined && result.exception === undefined) { + if (result.filter.isRedirect()) { + if (redirectNone !== undefined) { + result.exception = redirectNone; + } else { + result.redirect = this.resources.getResource(result.filter.getRedirectResource()); + } + } else if (result.filter.isRemoveParam()) { + result.redirect = { body: '', contentType: 'text', dataUrl: redirectUrl! }; } } } diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index 16e487f5b3..cec0ae7c44 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -153,6 +153,7 @@ export const enum NETWORK_FILTER_MASK { isHostnameAnchor = 1 << 28, isRedirectRule = 1 << 29, isRedirect = 1 << 30, + isRemoveParam = 1 << 31, // IMPORTANT: the mask is now full, no more options can be added // Consider creating a separate fitler type for isReplace if a new // network filter option is needed. @@ -887,6 +888,11 @@ export default class NetworkFilter implements IFilter { mask = setBit(mask, NETWORK_FILTER_MASK.isReplace); optionValue = value; + break; + case 'removeparam': + mask = setBit(mask, NETWORK_FILTER_MASK.isRemoveParam); + optionValue = value; + break; default: { // Handle content type options separatly @@ -1608,6 +1614,10 @@ export default class NetworkFilter implements IFilter { return getBit(this.getMask(), NETWORK_FILTER_MASK.isReplace); } + public isRemoveParam(): boolean { + return getBit(this.getMask(), NETWORK_FILTER_MASK.isRemoveParam); + } + // Expected to be called only with `$replace` modifiers public getHtmlModifier(): HTMLModifier | null { // Empty `$replace` modifier is to disable all replace modifiers on exception From 29e9c39852275f8adbc4cf712540b6b29b8ea5f4 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 13 Dec 2024 16:18:32 +0900 Subject: [PATCH 02/15] fix: check `parametersRemoved` to prevent infinite loop --- packages/adblocker/src/engine/engine.ts | 48 ++++++++++++++----------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index 218aebc902..02c9603b27 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -1477,26 +1477,34 @@ export default class FilterEngine extends EventEmitter { const searchParamsIndex = request.url.indexOf('?') + 1; if (searchParamsIndex !== 0 && resourceRedirects.length !== 0) { const searchParams = new URLSearchParams(request.url.slice(searchParamsIndex + 1)); + let parametersRemoved = 0; for (const redirect of requestRedirects) { - searchParams.delete(redirect.optionValue!); + if (searchParams.has(redirect.optionValue!)) { + parametersRemoved++; + searchParams.delete(redirect.optionValue!); + } } - redirectUrl = `${request.url.slice(0, searchParamsIndex)}?${searchParams.toString()}`; - } else { - const redirects = resourceRedirects - // highest priorty wins - .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); - - if (redirects.length !== 0) { - for (const filter of redirects) { - if (filter.getRedirectResource() === 'none') { - redirectNone = filter; - } else if (filter.isRedirectRule()) { - if (redirectRule === undefined) { - redirectRule = filter; - } - } else if (result.filter === undefined) { - result.filter = filter; + if (parametersRemoved !== 0) { + redirectUrl = `${request.url.slice(0, searchParamsIndex)}?${searchParams.toString()}`; + // TODO: More than one filter needs to be returned + result.filter = requestRedirects[0]; + } + } + + const redirects = resourceRedirects + // highest priorty wins + .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); + + if (redirects.length !== 0) { + for (const filter of redirects) { + if (filter.getRedirectResource() === 'none') { + redirectNone = filter; + } else if (filter.isRedirectRule()) { + if (redirectRule === undefined) { + redirectRule = filter; } + } else if (result.filter === undefined) { + result.filter = filter; } } } @@ -1528,14 +1536,14 @@ export default class FilterEngine extends EventEmitter { // 1. Check if a redirect=none rule was found, which acts as exception. // 2. If no exception was found, prepare `result.redirect` response. if (result.filter !== undefined && result.exception === undefined) { - if (result.filter.isRedirect()) { + if (redirectUrl !== undefined) { + result.redirect = { body: '', contentType: 'text', dataUrl: redirectUrl }; + } else { if (redirectNone !== undefined) { result.exception = redirectNone; } else { result.redirect = this.resources.getResource(result.filter.getRedirectResource()); } - } else if (result.filter.isRemoveParam()) { - result.redirect = { body: '', contentType: 'text', dataUrl: redirectUrl! }; } } } From 3ec118108ca1980450395f65f8ea5359768dead3 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 13 Dec 2024 16:23:45 +0900 Subject: [PATCH 03/15] fix: parsing and restoration --- packages/adblocker/src/filters/network.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index cec0ae7c44..8297ae4595 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -890,6 +890,10 @@ export default class NetworkFilter implements IFilter { break; case 'removeparam': + if (value.length === 0) { + return null; + } + mask = setBit(mask, NETWORK_FILTER_MASK.isRemoveParam); optionValue = value; @@ -1541,6 +1545,10 @@ export default class NetworkFilter implements IFilter { options.push('badfilter'); } + if (this.isRemoveParam()) { + options.push(`removeparam=${this.optionValue!}`); + } + if (options.length > 0) { if (typeof modifierReplacer === 'function') { filter += `$${options.map(modifierReplacer).join(',')}`; From fd43ad1e1012375d0da9355f3b851205fd261b35 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 13 Dec 2024 16:29:05 +0900 Subject: [PATCH 04/15] feat: removeparam without option value --- packages/adblocker/src/engine/engine.ts | 15 ++++++++++++--- packages/adblocker/src/filters/network.ts | 10 +++++----- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index 02c9603b27..cfe0811a7f 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -1479,9 +1479,18 @@ export default class FilterEngine extends EventEmitter { const searchParams = new URLSearchParams(request.url.slice(searchParamsIndex + 1)); let parametersRemoved = 0; for (const redirect of requestRedirects) { - if (searchParams.has(redirect.optionValue!)) { - parametersRemoved++; - searchParams.delete(redirect.optionValue!); + // When removeparam used without any option value, remove all parameter. + if (redirect.optionValue === undefined) { + redirectUrl = request.url.slice(0, searchParamsIndex); + result.filter = requestRedirects[0]; + // Prevent next branch after the loop to be executed. + parametersRemoved = 0; + break; + } else { + if (searchParams.has(redirect.optionValue)) { + parametersRemoved++; + searchParams.delete(redirect.optionValue); + } } } if (parametersRemoved !== 0) { diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index 8297ae4595..d3d84f2db1 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -890,10 +890,6 @@ export default class NetworkFilter implements IFilter { break; case 'removeparam': - if (value.length === 0) { - return null; - } - mask = setBit(mask, NETWORK_FILTER_MASK.isRemoveParam); optionValue = value; @@ -1546,7 +1542,11 @@ export default class NetworkFilter implements IFilter { } if (this.isRemoveParam()) { - options.push(`removeparam=${this.optionValue!}`); + if (this.optionValue !== undefined) { + options.push(`removeparam=${this.optionValue}`); + } else { + options.push('removeparam'); + } } if (options.length > 0) { From 9497047a6879aad4682d93c203ca954505eeac55 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Wed, 25 Dec 2024 16:33:08 +0900 Subject: [PATCH 05/15] refactor: drop use of URLSearchParams --- packages/adblocker/src/engine/engine.ts | 81 +++++++++++++---------- packages/adblocker/src/filters/network.ts | 8 +++ 2 files changed, 54 insertions(+), 35 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index cfe0811a7f..e8641db0ab 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -8,8 +8,6 @@ import type { IMessageFromBackground } from '@ghostery/adblocker-content'; -import { URLSearchParams } from 'node:url'; - import Config from '../config.js'; import { StaticDataView, sizeOfASCII, sizeOfByte, sizeOfBool } from '../data-view.js'; import { EventEmitter } from '../events.js'; @@ -1445,7 +1443,7 @@ export default class FilterEngine extends EventEmitter { if (request.isSupported) { // Check the filters in the following order: // 1. $important (not subject to exceptions) - // 2. redirection ($redirect=resource) + // 2. redirection ($removeparam and $redirect=resource) // 3. normal filters // 4. exceptions result.filter = this.importants.match(request, this.isFilterExcluded.bind(this)); @@ -1455,13 +1453,7 @@ export default class FilterEngine extends EventEmitter { let redirectUrl: string | undefined; // If `result.filter` is `undefined`, it means there was no $important - // filter found so far. We look for a $redirect filter. There is some - // extra logic to handle special cases like redirect-rule and - // redirect=none. - // - // * If redirect=none is found, then cancel all redirects. - // * Else if redirect-rule is found, only redirect if request would be blocked. - // * Else if redirect is found, redirect. + // filter found so far. We look for $removeparam and $redirect filter. if (result.filter === undefined) { const redirectFilters = this.redirects.matchAll(request, this.isFilterExcluded.bind(this)); const resourceRedirects: NetworkFilter[] = []; @@ -1474,37 +1466,55 @@ export default class FilterEngine extends EventEmitter { } } - const searchParamsIndex = request.url.indexOf('?') + 1; - if (searchParamsIndex !== 0 && resourceRedirects.length !== 0) { - const searchParams = new URLSearchParams(request.url.slice(searchParamsIndex + 1)); - let parametersRemoved = 0; - for (const redirect of requestRedirects) { - // When removeparam used without any option value, remove all parameter. - if (redirect.optionValue === undefined) { - redirectUrl = request.url.slice(0, searchParamsIndex); - result.filter = requestRedirects[0]; - // Prevent next branch after the loop to be executed. - parametersRemoved = 0; - break; - } else { - if (searchParams.has(redirect.optionValue)) { - parametersRemoved++; - searchParams.delete(redirect.optionValue); + // We don't need to match `removeparam` at all if: + // * `removeparam` filters not matched + // * URL doesn't have `?` — parameter separator + // * URL has `?` but ends right after the separator + if (resourceRedirects.length !== 0) { + const searchParamsSeparatorAt = request.url.indexOf('?'); + if ( + searchParamsSeparatorAt !== -1 && + searchParamsSeparatorAt + 1 !== request.url.length + ) { + for (const redirect of requestRedirects) { + // If `removeparam` without a value found, we drop all params + if (redirect.removeparam!.length === 0) { + result.filter = redirect; + redirectUrl = request.url.slice(0, searchParamsSeparatorAt); + break; + } + const searchParamStartsWith = `${redirect.removeparam!}=`; + const searchParamStartsAt = request.url.indexOf( + searchParamStartsWith, + searchParamsSeparatorAt, + ); + if (searchParamStartsAt === -1) { + continue; } + let searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt + 1); + if (searchParamEndsAt === -1) { + searchParamEndsAt = request.url.length; + } + result.filter = redirect; + redirectUrl = + request.url.slice(0, searchParamStartsAt) + request.url.slice(searchParamEndsAt); + break; } } - if (parametersRemoved !== 0) { - redirectUrl = `${request.url.slice(0, searchParamsIndex)}?${searchParams.toString()}`; - // TODO: More than one filter needs to be returned - result.filter = requestRedirects[0]; - } } - const redirects = resourceRedirects - // highest priorty wins - .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); + // If `result.filter` is still remains `undefined`, there + // is some extra logic to handle special cases like + // redirect-rule and redirect=none. + // + // * If redirect=none is found, then cancel all redirects. + // * Else if redirect-rule is found, only redirect if request would be blocked. + // * Else if redirect is found, redirect. + if (result.filter === undefined && resourceRedirects.length !== 0) { + const redirects = resourceRedirects + // highest priorty wins + .sort((a, b) => b.getRedirectPriority() - a.getRedirectPriority()); - if (redirects.length !== 0) { for (const filter of redirects) { if (filter.getRedirectResource() === 'none') { redirectNone = filter; @@ -1545,6 +1555,7 @@ export default class FilterEngine extends EventEmitter { // 1. Check if a redirect=none rule was found, which acts as exception. // 2. If no exception was found, prepare `result.redirect` response. if (result.filter !== undefined && result.exception === undefined) { + // Prioritize if `redirectUrl` is set. if (redirectUrl !== undefined) { result.redirect = { body: '', contentType: 'text', dataUrl: redirectUrl }; } else { diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index d3d84f2db1..bbb3d7377f 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -1271,6 +1271,14 @@ export default class NetworkFilter implements IFilter { return this.optionValue; } + public get removeparam(): string | undefined { + if (!this.isRemoveParam()) { + return undefined; + } + + return this.optionValue || ''; + } + public isCosmeticFilter(): this is CosmeticFilter { return false; } From 56f90cc1dfee926723066601140ccc88a32a8bb4 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Wed, 25 Dec 2024 17:55:56 +0900 Subject: [PATCH 06/15] feat: introduce `NetworkFilter.prototype.isRedirectable` --- packages/adblocker/src/engine/engine.ts | 23 +++++++++++-------- packages/adblocker/src/filters/network.ts | 4 ++++ packages/adblocker/test/engine/engine.test.ts | 10 ++++---- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index e8641db0ab..cec9336c3f 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -779,7 +779,7 @@ export default class FilterEngine extends EventEmitter { exceptions.push(filter); } else if (filter.isImportant()) { importants.push(filter); - } else if (filter.isRedirect() || filter.isRemoveParam()) { + } else if (filter.isRedirectable()) { redirects.push(filter); } else { filters.push(filter); @@ -1470,7 +1470,7 @@ export default class FilterEngine extends EventEmitter { // * `removeparam` filters not matched // * URL doesn't have `?` — parameter separator // * URL has `?` but ends right after the separator - if (resourceRedirects.length !== 0) { + if (requestRedirects.length !== 0) { const searchParamsSeparatorAt = request.url.indexOf('?'); if ( searchParamsSeparatorAt !== -1 && @@ -1491,13 +1491,12 @@ export default class FilterEngine extends EventEmitter { if (searchParamStartsAt === -1) { continue; } - let searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt + 1); - if (searchParamEndsAt === -1) { - searchParamEndsAt = request.url.length; - } + const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt + 1); result.filter = redirect; - redirectUrl = - request.url.slice(0, searchParamStartsAt) + request.url.slice(searchParamEndsAt); + redirectUrl = request.url.slice(0, searchParamStartsAt); + if (searchParamEndsAt !== -1) { + redirectUrl += request.url.slice(searchParamEndsAt); + } break; } } @@ -1554,10 +1553,14 @@ export default class FilterEngine extends EventEmitter { // // 1. Check if a redirect=none rule was found, which acts as exception. // 2. If no exception was found, prepare `result.redirect` response. - if (result.filter !== undefined && result.exception === undefined) { + if ( + result.filter !== undefined && + result.exception === undefined && + result.filter.isRedirectable() + ) { // Prioritize if `redirectUrl` is set. if (redirectUrl !== undefined) { - result.redirect = { body: '', contentType: 'text', dataUrl: redirectUrl }; + result.redirect = { body: '', contentType: 'text/plain', dataUrl: redirectUrl }; } else { if (redirectNone !== undefined) { result.exception = redirectNone; diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index bbb3d7377f..b439df7848 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -1634,6 +1634,10 @@ export default class NetworkFilter implements IFilter { return getBit(this.getMask(), NETWORK_FILTER_MASK.isRemoveParam); } + public isRedirectable(): boolean { + return this.isRedirect() || this.isRemoveParam(); + } + // Expected to be called only with `$replace` modifiers public getHtmlModifier(): HTMLModifier | null { // Empty `$replace` modifier is to disable all replace modifiers on exception diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index a002f84234..d246731ef0 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -77,14 +77,14 @@ function test({ expect(importants).to.include(result.filter.rawLine); // Handle case where important filter is also a redirect - if (filter.isRedirect()) { + if (filter.isRedirectable()) { expect(redirects).to.include(result.filter.rawLine); } } expect(result.exception).to.be.undefined; - if (!filter.isRedirect()) { + if (!filter.isRedirectable()) { expect(result.redirect).to.be.undefined; } @@ -108,7 +108,7 @@ function test({ expect(result.filter).not.to.be.undefined; expect(result.redirect).to.be.undefined; expect(result.match).to.be.false; - } else if (filter.isRedirect() && exceptions.length === 0 && importants.length === 0) { + } else if (filter.isRedirectable() && exceptions.length === 0 && importants.length === 0) { const result = engine.match(request); expect(result.filter).not.to.be.undefined; if ( @@ -661,11 +661,11 @@ $csp=baz,domain=bar.com importants.push(filter); } - if (parsed.isRedirect()) { + if (parsed.isRedirectable()) { redirects.push(filter); } - if (!parsed.isRedirect() && !parsed.isException() && !parsed.isImportant()) { + if (!parsed.isRedirectable() && !parsed.isException() && !parsed.isImportant()) { normalFilters.push(filter); } } From 47b13023667f356306f23dc6acd0e4108ce5b589 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Wed, 25 Dec 2024 19:37:51 +0900 Subject: [PATCH 07/15] refactor: improve matching search param key --- packages/adblocker/src/engine/engine.ts | 11 ++++++++--- packages/adblocker/src/filters/network.ts | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index cec9336c3f..4c8675bb2f 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -1483,11 +1483,16 @@ export default class FilterEngine extends EventEmitter { redirectUrl = request.url.slice(0, searchParamsSeparatorAt); break; } - const searchParamStartsWith = `${redirect.removeparam!}=`; - const searchParamStartsAt = request.url.indexOf( - searchParamStartsWith, + let searchParamStartsAt = request.url.indexOf( + `?${redirect.removeparam!}`, searchParamsSeparatorAt, ); + if (searchParamStartsAt === -1) { + searchParamStartsAt = request.url.indexOf( + `&${redirect.removeparam!}`, + searchParamsSeparatorAt + 3 /* '?x='.length */, + ); + } if (searchParamStartsAt === -1) { continue; } diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index b439df7848..8b2fe7650f 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -890,6 +890,7 @@ export default class NetworkFilter implements IFilter { break; case 'removeparam': + // TODO: Support regex mask = setBit(mask, NETWORK_FILTER_MASK.isRemoveParam); optionValue = value; From 7ea4488708df86e666c0c8d56ef9fb3fb2877c95 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 27 Dec 2024 17:57:16 +0900 Subject: [PATCH 08/15] test: removeparam test: add `isRemoveParam` and `isRedirectable` chore: reject on `removeparam` negation test: parsing removeparam test: removeparam test: removeparam --- packages/adblocker/src/engine/engine.ts | 70 +++++++++++++----- packages/adblocker/src/filters/network.ts | 4 + packages/adblocker/test/engine/engine.test.ts | 74 +++++++++++++++++++ packages/adblocker/test/parsing.test.ts | 32 ++++++++ 4 files changed, 161 insertions(+), 19 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index 4c8675bb2f..f9a506611c 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -1477,32 +1477,64 @@ export default class FilterEngine extends EventEmitter { searchParamsSeparatorAt + 1 !== request.url.length ) { for (const redirect of requestRedirects) { + const target = redirect.removeparam!; // If `removeparam` without a value found, we drop all params - if (redirect.removeparam!.length === 0) { + if (target.length === 0) { result.filter = redirect; redirectUrl = request.url.slice(0, searchParamsSeparatorAt); break; } - let searchParamStartsAt = request.url.indexOf( - `?${redirect.removeparam!}`, - searchParamsSeparatorAt, - ); - if (searchParamStartsAt === -1) { - searchParamStartsAt = request.url.indexOf( - `&${redirect.removeparam!}`, - searchParamsSeparatorAt + 3 /* '?x='.length */, - ); + // Try to find `searchParamStartsAt` recursively + let searchParamStartsAt: number = -1; + for ( + let lookupAfter = searchParamsSeparatorAt; + lookupAfter < request.url.length; + searchParamStartsAt = -1 + ) { + searchParamStartsAt = request.url.indexOf(target, lookupAfter); + // No match found in this string, try to fast exit in the first loop. + if (searchParamStartsAt === -1) { + break; + } + + // VALID only if the next character is equal sign or EOL: + if ( + searchParamStartsAt + target.length === request.url.length || + request.url.charCodeAt(searchParamStartsAt + target.length) === 61 /* '=' */ + ) { + // In case of first param: + if (searchParamsSeparatorAt === searchParamStartsAt - 1) { + result.filter = redirect; + const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt); + if (searchParamEndsAt === -1) { + redirectUrl = request.url.slice(0, searchParamStartsAt); + } else { + redirectUrl = + request.url.slice(0, searchParamStartsAt) + + request.url.slice(searchParamEndsAt + 1); + } + break; + } else if (request.url.charCodeAt(searchParamStartsAt - 1) === 38 /* '&' */) { + result.filter = redirect; + const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt); + // In case of last param: + if (searchParamEndsAt === -1) { + redirectUrl = request.url.slice(0, searchParamStartsAt - 1); + } else { + // In case of param in the middle: + redirectUrl = + request.url.slice(0, searchParamStartsAt) + + request.url.slice(searchParamEndsAt + 1); + } + break; + } + } + + lookupAfter = searchParamStartsAt + target.length; } - if (searchParamStartsAt === -1) { - continue; - } - const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt + 1); - result.filter = redirect; - redirectUrl = request.url.slice(0, searchParamStartsAt); - if (searchParamEndsAt !== -1) { - redirectUrl += request.url.slice(searchParamEndsAt); + if (searchParamStartsAt !== -1) { + break; } - break; } } } diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index 8b2fe7650f..b66c24380f 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -891,6 +891,10 @@ export default class NetworkFilter implements IFilter { break; case 'removeparam': // TODO: Support regex + if (negation || value.startsWith('/')) { + return null; + } + mask = setBit(mask, NETWORK_FILTER_MASK.isRemoveParam); optionValue = value; diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index d246731ef0..c8ff8cfc44 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -600,6 +600,80 @@ $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', () => { + function urlToDocumentRequest(url: string) { + return Request.fromRawDetails({ + sourceUrl: 'https://foo.com', + url, + type: 'document', + }); + } + + const requests = [ + 'https://foo.com?utm', + 'https://foo.com?utm=', + 'https://foo.com?utm=a', + 'https://foo.com?utm=a&utm_source=organic', + 'https://foo.com?utm_source=organic&utm=a', + ].map(urlToDocumentRequest); + + describe('removeparam removes all parameters', () => { + const engine = createEngine('||foo.com$removeparam'); + for (const { match, redirect, request } of requests.map((request) => ({ + ...engine.match(request), + request, + }))) { + it(`removeparam all from "${request.url}"`, () => { + expect(match).to.be.true; + expect(redirect).not.to.be.undefined; + expect(redirect!.body).to.be.eql(''); + expect(redirect!.contentType).to.be.eql('text/plain'); + expect(redirect!.dataUrl).to.be.eql('https://foo.com'); + }); + } + }); + + describe('removeparam removes specific parameter', () => { + const engine = createEngine('||foo.com$removeparam=utm'); + for (const { match, redirect, request } of requests.map((request) => ({ + ...engine.match(request), + request, + }))) { + it(`removeparam "utm" from "${request.url}"`, () => { + expect(match).to.be.true; + expect(redirect).not.to.be.undefined; + expect(redirect!.body).to.be.eql(''); + expect(redirect!.contentType).to.be.eql('text/plain'); + expect(redirect!.dataUrl).not.to.include('?utm='); + expect(redirect!.dataUrl).not.to.include('&utm='); + }); + } + }); + + describe('removeparam removes specific parameter regardless of ordering', () => { + const engine = createEngine('||foo.com$removeparam=utm'); + for (const { match, redirect, request } of [ + // First + 'https://foo.com?utm=a&utm_source=organic&utm_event=b', + // Middle + 'https://foo.com?utm_source=organic&utm=a&utm_event=b', + // Last + 'https://foo.com?utm_source=organic&utm_event=b&utm=a', + ] + .map(urlToDocumentRequest) + .map((request) => ({ + ...engine.match(request), + request, + }))) { + it(`removeparam "utm" from "${request.url}"`, () => { + expect(match).to.be.true; + expect(redirect).not.to.be.undefined; + expect(redirect!.dataUrl).to.be.eql('https://foo.com?utm_source=organic&utm_event=b'); + }); + } + }); + }); }); describe('network filters', () => { diff --git a/packages/adblocker/test/parsing.test.ts b/packages/adblocker/test/parsing.test.ts index 1b9da5b8b5..421c79d57b 100644 --- a/packages/adblocker/test/parsing.test.ts +++ b/packages/adblocker/test/parsing.test.ts @@ -34,6 +34,7 @@ function network(filter: string, expected: any) { denyallow: parsed.denyallow, domains: parsed.domains, redirect: parsed.getRedirect(), + removeparam: parsed.removeparam, // Filter type isBadFilter: parsed.isBadFilter(), @@ -48,6 +49,8 @@ function network(filter: string, expected: any) { isPlain: parsed.isPlain(), isRedirect: parsed.isRedirect(), isRedirectRule: parsed.isRedirectRule(), + isRemoveParam: parsed.isRemoveParam(), + isRedirectable: parsed.isRedirectable(), isRegex: parsed.isRegex(), isRightAnchor: parsed.isRightAnchor(), @@ -101,6 +104,8 @@ const DEFAULT_NETWORK_FILTER = { isPlain: false, isRedirect: false, isRedirectRule: false, + isRemoveParam: false, + isRedirectable: false, isRegex: false, isRightAnchor: false, @@ -845,11 +850,13 @@ describe('Network filters', () => { network('||foo.com$redirect-rule=bar.js', { isRedirect: true, isRedirectRule: true, + isRedirectable: true, redirect: 'bar.js', }); network('$redirect-rule=bar.js', { isRedirect: true, isRedirectRule: true, + isRedirectable: true, redirect: 'bar.js', }); }); @@ -872,6 +879,31 @@ describe('Network filters', () => { network('||foo.com', { isRedirectRule: false, redirect: '', + isRedirectable: true, + }); + }); + }); + + describe('removeparam', () => { + it('parses ~removeparam', () => { + // ~removeparam is not a valid option + network('||foo.com$~removeparam=utm', null); + network('||fo.com$~removeparam', null); + }); + + it('parses removeparam', () => { + network('||foo.com$removeparam=utm', { + removeparam: 'utm', + isRemoveParam: true, + isRedirectable: true, + }); + }); + + it('parses removeparam without a value', () => { + network('||foo.com$removeparam', { + removeparam: '', + isRemoveParam: true, + isRedirectable: true, }); }); }); From 29e90221cdb5e0928f814e6e3e10d7859139ee98 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 27 Dec 2024 19:55:09 +0900 Subject: [PATCH 09/15] test: removeparam removes parameter sequentially --- packages/adblocker/test/engine/engine.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index c8ff8cfc44..3379635b04 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -673,6 +673,22 @@ $csp=baz,domain=bar.com }); } }); + + it('removeparam removes parameter sequentially', () => { + const params = ['utm', 'utm_source', 'utm_event']; + let request = urlToDocumentRequest('https://foo.com?utm_source=organic&utm_event=b&utm=a'); + const engine = createEngine( + params.map((param) => `||foo.com$removeparam=${param}`).join('\n'), + ); + for (let i = 0; i < params.length; i++) { + const { redirect } = engine.match(request); + expect(redirect).not.to.be.undefined; + request = urlToDocumentRequest(redirect!.dataUrl); + } + for (const param of params) { + expect(request.url).not.to.include(param); + } + }); }); }); From 3cb0861f1031c86226572930dc8d668206a68bab Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Fri, 27 Dec 2024 19:56:51 +0900 Subject: [PATCH 10/15] fix(test): unintentional isRedirectable definition --- packages/adblocker/test/parsing.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/adblocker/test/parsing.test.ts b/packages/adblocker/test/parsing.test.ts index 421c79d57b..7f65daa518 100644 --- a/packages/adblocker/test/parsing.test.ts +++ b/packages/adblocker/test/parsing.test.ts @@ -879,7 +879,6 @@ describe('Network filters', () => { network('||foo.com', { isRedirectRule: false, redirect: '', - isRedirectable: true, }); }); }); From 9c1e12b1dc1b60bc68d026427823166eff73d0a2 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Mon, 6 Jan 2025 14:58:39 +0900 Subject: [PATCH 11/15] refactor: use `URL` constructor refs https://github.com/ghostery/adblocker/pull/4528#discussion_r1902696043 --- packages/adblocker/src/engine/engine.ts | 80 +++++-------------- packages/adblocker/test/engine/engine.test.ts | 14 ++-- 2 files changed, 25 insertions(+), 69 deletions(-) diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts index f9a506611c..64832010b9 100644 --- a/packages/adblocker/src/engine/engine.ts +++ b/packages/adblocker/src/engine/engine.ts @@ -1468,71 +1468,27 @@ export default class FilterEngine extends EventEmitter { // We don't need to match `removeparam` at all if: // * `removeparam` filters not matched - // * URL doesn't have `?` — parameter separator - // * URL has `?` but ends right after the separator + // * URL doesn't have search parameter if (requestRedirects.length !== 0) { - const searchParamsSeparatorAt = request.url.indexOf('?'); - if ( - searchParamsSeparatorAt !== -1 && - searchParamsSeparatorAt + 1 !== request.url.length - ) { - for (const redirect of requestRedirects) { - const target = redirect.removeparam!; - // If `removeparam` without a value found, we drop all params - if (target.length === 0) { - result.filter = redirect; - redirectUrl = request.url.slice(0, searchParamsSeparatorAt); + const url = new URL(request.url); + if (url.searchParams.size !== 0) { + for (const filter of requestRedirects) { + const key = filter.removeparam!; + + // Remove all params in case of option value is empty + if (key === '') { + result.filter = filter; + redirectUrl = request.url.slice(0, request.url.indexOf('?')); + break; } - // Try to find `searchParamStartsAt` recursively - let searchParamStartsAt: number = -1; - for ( - let lookupAfter = searchParamsSeparatorAt; - lookupAfter < request.url.length; - searchParamStartsAt = -1 - ) { - searchParamStartsAt = request.url.indexOf(target, lookupAfter); - // No match found in this string, try to fast exit in the first loop. - if (searchParamStartsAt === -1) { - break; - } - - // VALID only if the next character is equal sign or EOL: - if ( - searchParamStartsAt + target.length === request.url.length || - request.url.charCodeAt(searchParamStartsAt + target.length) === 61 /* '=' */ - ) { - // In case of first param: - if (searchParamsSeparatorAt === searchParamStartsAt - 1) { - result.filter = redirect; - const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt); - if (searchParamEndsAt === -1) { - redirectUrl = request.url.slice(0, searchParamStartsAt); - } else { - redirectUrl = - request.url.slice(0, searchParamStartsAt) + - request.url.slice(searchParamEndsAt + 1); - } - break; - } else if (request.url.charCodeAt(searchParamStartsAt - 1) === 38 /* '&' */) { - result.filter = redirect; - const searchParamEndsAt = request.url.indexOf('&', searchParamStartsAt); - // In case of last param: - if (searchParamEndsAt === -1) { - redirectUrl = request.url.slice(0, searchParamStartsAt - 1); - } else { - // In case of param in the middle: - redirectUrl = - request.url.slice(0, searchParamStartsAt) + - request.url.slice(searchParamEndsAt + 1); - } - break; - } - } - - lookupAfter = searchParamStartsAt + target.length; - } - if (searchParamStartsAt !== -1) { + + if (url.searchParams.has(key)) { + url.searchParams.delete(key); + + result.filter = filter; + redirectUrl = url.toString(); + break; } } diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index 3379635b04..eeef041ef5 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -618,13 +618,13 @@ $csp=baz,domain=bar.com 'https://foo.com?utm_source=organic&utm=a', ].map(urlToDocumentRequest); - describe('removeparam removes all parameters', () => { + describe('removes all parameters', () => { const engine = createEngine('||foo.com$removeparam'); for (const { match, redirect, request } of requests.map((request) => ({ ...engine.match(request), request, }))) { - it(`removeparam all from "${request.url}"`, () => { + it(`removes all params from "${request.url}"`, () => { expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -634,13 +634,13 @@ $csp=baz,domain=bar.com } }); - describe('removeparam removes specific parameter', () => { + describe('removes specific parameter', () => { const engine = createEngine('||foo.com$removeparam=utm'); for (const { match, redirect, request } of requests.map((request) => ({ ...engine.match(request), request, }))) { - it(`removeparam "utm" from "${request.url}"`, () => { + it(`removes "utm" from "${request.url}"`, () => { expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -651,7 +651,7 @@ $csp=baz,domain=bar.com } }); - describe('removeparam removes specific parameter regardless of ordering', () => { + describe('removes specific parameter regardless of ordering', () => { const engine = createEngine('||foo.com$removeparam=utm'); for (const { match, redirect, request } of [ // First @@ -669,12 +669,12 @@ $csp=baz,domain=bar.com it(`removeparam "utm" from "${request.url}"`, () => { expect(match).to.be.true; expect(redirect).not.to.be.undefined; - expect(redirect!.dataUrl).to.be.eql('https://foo.com?utm_source=organic&utm_event=b'); + expect(redirect!.dataUrl).to.be.eql('https://foo.com/?utm_source=organic&utm_event=b'); }); } }); - it('removeparam removes parameter sequentially', () => { + it('removes parameter sequentially', () => { const params = ['utm', 'utm_source', 'utm_event']; let request = urlToDocumentRequest('https://foo.com?utm_source=organic&utm_event=b&utm=a'); const engine = createEngine( From c8da3bbec6949a4869d9f010806ab001ad0c6411 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Mon, 6 Jan 2025 15:02:29 +0900 Subject: [PATCH 12/15] refactor: clarify `removeparam` getter --- packages/adblocker/src/filters/network.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/adblocker/src/filters/network.ts b/packages/adblocker/src/filters/network.ts index b66c24380f..d7999a3593 100644 --- a/packages/adblocker/src/filters/network.ts +++ b/packages/adblocker/src/filters/network.ts @@ -1281,7 +1281,7 @@ export default class NetworkFilter implements IFilter { return undefined; } - return this.optionValue || ''; + return this.optionValue; } public isCosmeticFilter(): this is CosmeticFilter { @@ -1555,7 +1555,7 @@ export default class NetworkFilter implements IFilter { } if (this.isRemoveParam()) { - if (this.optionValue !== undefined) { + if (this.removeparam!.length > 0) { options.push(`removeparam=${this.optionValue}`); } else { options.push('removeparam'); From 533110ecee31387ea0d6c3881886b35f6d30cd9b Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Tue, 14 Jan 2025 19:30:22 +0900 Subject: [PATCH 13/15] refactor: move heavy calls to `before` --- packages/adblocker/test/engine/engine.test.ts | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index eeef041ef5..50f1a30904 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -610,16 +610,22 @@ $csp=baz,domain=bar.com }); } - const requests = [ - 'https://foo.com?utm', - 'https://foo.com?utm=', - 'https://foo.com?utm=a', - 'https://foo.com?utm=a&utm_source=organic', - 'https://foo.com?utm_source=organic&utm=a', - ].map(urlToDocumentRequest); + let requests: Request[]; + before(() => { + requests = [ + 'https://foo.com?utm', + 'https://foo.com?utm=', + 'https://foo.com?utm=a', + 'https://foo.com?utm=a&utm_source=organic', + 'https://foo.com?utm_source=organic&utm=a', + ].map(urlToDocumentRequest); + }); describe('removes all parameters', () => { - const engine = createEngine('||foo.com$removeparam'); + let engine: FilterEngine; + before(() => { + engine = createEngine('||foo.com$removeparam'); + }); for (const { match, redirect, request } of requests.map((request) => ({ ...engine.match(request), request, @@ -635,7 +641,10 @@ $csp=baz,domain=bar.com }); describe('removes specific parameter', () => { - const engine = createEngine('||foo.com$removeparam=utm'); + let engine: FilterEngine; + before(() => { + engine = createEngine('||foo.com$removeparam=utm'); + }); for (const { match, redirect, request } of requests.map((request) => ({ ...engine.match(request), request, @@ -652,7 +661,10 @@ $csp=baz,domain=bar.com }); describe('removes specific parameter regardless of ordering', () => { - const engine = createEngine('||foo.com$removeparam=utm'); + let engine: FilterEngine; + before(() => { + engine = createEngine('||foo.com$removeparam=utm'); + }); for (const { match, redirect, request } of [ // First 'https://foo.com?utm=a&utm_source=organic&utm_event=b', @@ -676,10 +688,10 @@ $csp=baz,domain=bar.com it('removes parameter sequentially', () => { const params = ['utm', 'utm_source', 'utm_event']; - let request = urlToDocumentRequest('https://foo.com?utm_source=organic&utm_event=b&utm=a'); const engine = createEngine( params.map((param) => `||foo.com$removeparam=${param}`).join('\n'), ); + let request = urlToDocumentRequest('https://foo.com?utm_source=organic&utm_event=b&utm=a'); for (let i = 0; i < params.length; i++) { const { redirect } = engine.match(request); expect(redirect).not.to.be.undefined; From 1c070255fd0ac03ee89a3204a4f8e18f4203a784 Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Wed, 15 Jan 2025 16:17:07 +0900 Subject: [PATCH 14/15] fix(test): invalid scopes --- packages/adblocker/test/engine/engine.test.ts | 41 +++++++------------ 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index 50f1a30904..5ae6efd57c 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -601,7 +601,7 @@ $csp=baz,domain=bar.com expect(redirect).to.be.undefined; }); - context('removeparam', () => { + context.only('removeparam', () => { function urlToDocumentRequest(url: string) { return Request.fromRawDetails({ sourceUrl: 'https://foo.com', @@ -610,27 +610,22 @@ $csp=baz,domain=bar.com }); } - let requests: Request[]; - before(() => { - requests = [ - 'https://foo.com?utm', - 'https://foo.com?utm=', - 'https://foo.com?utm=a', - 'https://foo.com?utm=a&utm_source=organic', - 'https://foo.com?utm_source=organic&utm=a', - ].map(urlToDocumentRequest); - }); + const requests = [ + 'https://foo.com?utm', + 'https://foo.com?utm=', + 'https://foo.com?utm=a', + 'https://foo.com?utm=a&utm_source=organic', + 'https://foo.com?utm_source=organic&utm=a', + ].map(urlToDocumentRequest); describe('removes all parameters', () => { let engine: FilterEngine; before(() => { engine = createEngine('||foo.com$removeparam'); }); - for (const { match, redirect, request } of requests.map((request) => ({ - ...engine.match(request), - request, - }))) { + for (const request of requests) { it(`removes all params from "${request.url}"`, () => { + const { match, redirect } = engine.match(request); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -645,11 +640,9 @@ $csp=baz,domain=bar.com before(() => { engine = createEngine('||foo.com$removeparam=utm'); }); - for (const { match, redirect, request } of requests.map((request) => ({ - ...engine.match(request), - request, - }))) { + for (const request of requests) { it(`removes "utm" from "${request.url}"`, () => { + const { match, redirect } = engine.match(request); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -665,20 +658,16 @@ $csp=baz,domain=bar.com before(() => { engine = createEngine('||foo.com$removeparam=utm'); }); - for (const { match, redirect, request } of [ + for (const request of [ // First 'https://foo.com?utm=a&utm_source=organic&utm_event=b', // Middle 'https://foo.com?utm_source=organic&utm=a&utm_event=b', // Last 'https://foo.com?utm_source=organic&utm_event=b&utm=a', - ] - .map(urlToDocumentRequest) - .map((request) => ({ - ...engine.match(request), - request, - }))) { + ].map(urlToDocumentRequest)) { it(`removeparam "utm" from "${request.url}"`, () => { + const { match, redirect } = engine.match(request); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.dataUrl).to.be.eql('https://foo.com/?utm_source=organic&utm_event=b'); From 90d051b886b9a517777823521ef7dace9a78ee2c Mon Sep 17 00:00:00 2001 From: HoJeong Go Date: Wed, 15 Jan 2025 16:23:50 +0900 Subject: [PATCH 15/15] refactor: remove url transforms outside of before hook --- packages/adblocker/test/engine/engine.test.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/adblocker/test/engine/engine.test.ts b/packages/adblocker/test/engine/engine.test.ts index 5ae6efd57c..1615c224ed 100644 --- a/packages/adblocker/test/engine/engine.test.ts +++ b/packages/adblocker/test/engine/engine.test.ts @@ -610,22 +610,22 @@ $csp=baz,domain=bar.com }); } - const requests = [ + const urls = [ 'https://foo.com?utm', 'https://foo.com?utm=', 'https://foo.com?utm=a', 'https://foo.com?utm=a&utm_source=organic', 'https://foo.com?utm_source=organic&utm=a', - ].map(urlToDocumentRequest); + ]; describe('removes all parameters', () => { let engine: FilterEngine; before(() => { engine = createEngine('||foo.com$removeparam'); }); - for (const request of requests) { - it(`removes all params from "${request.url}"`, () => { - const { match, redirect } = engine.match(request); + for (const url of urls) { + it(`removes all params from "${url}"`, () => { + const { match, redirect } = engine.match(urlToDocumentRequest(url)); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -640,9 +640,9 @@ $csp=baz,domain=bar.com before(() => { engine = createEngine('||foo.com$removeparam=utm'); }); - for (const request of requests) { - it(`removes "utm" from "${request.url}"`, () => { - const { match, redirect } = engine.match(request); + for (const url of urls) { + it(`removes "utm" from "${url}"`, () => { + const { match, redirect } = engine.match(urlToDocumentRequest(url)); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.body).to.be.eql(''); @@ -658,16 +658,16 @@ $csp=baz,domain=bar.com before(() => { engine = createEngine('||foo.com$removeparam=utm'); }); - for (const request of [ + for (const url of [ // First 'https://foo.com?utm=a&utm_source=organic&utm_event=b', // Middle 'https://foo.com?utm_source=organic&utm=a&utm_event=b', // Last 'https://foo.com?utm_source=organic&utm_event=b&utm=a', - ].map(urlToDocumentRequest)) { - it(`removeparam "utm" from "${request.url}"`, () => { - const { match, redirect } = engine.match(request); + ]) { + it(`removeparam "utm" from "${url}"`, () => { + const { match, redirect } = engine.match(urlToDocumentRequest(url)); expect(match).to.be.true; expect(redirect).not.to.be.undefined; expect(redirect!.dataUrl).to.be.eql('https://foo.com/?utm_source=organic&utm_event=b');