From e7e0ac90c28b0f3dcbc0ea23124aed0cb6162f35 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 31 Mar 2025 19:52:54 +0100 Subject: [PATCH 1/6] Add patching changes --- injected/entry-points/integration.js | 1 + injected/src/config-feature.js | 72 +++++++++++--- injected/src/content-feature.js | 1 + injected/src/utils.js | 40 +++++--- injected/unit-test/content-feature.js | 96 +++++++++++++++++++ .../helpers/polyfill-process-globals.js | 14 +-- injected/unit-test/utils.js | 2 + package-lock.json | 12 ++- package.json | 3 +- 9 files changed, 202 insertions(+), 39 deletions(-) diff --git a/injected/entry-points/integration.js b/injected/entry-points/integration.js index 7972a2d739..95bd90c3fc 100644 --- a/injected/entry-points/integration.js +++ b/injected/entry-points/integration.js @@ -24,6 +24,7 @@ function generateConfig() { }, site: { domain: topLevelUrl.hostname, + url: topLevelUrl.href, isBroken: false, allowlisted: false, enabledFeatures: [ diff --git a/injected/src/config-feature.js b/injected/src/config-feature.js index 10a65cd406..e4d99371df 100644 --- a/injected/src/config-feature.js +++ b/injected/src/config-feature.js @@ -1,5 +1,6 @@ import { immutableJSONPatch } from 'immutable-json-patch'; import { camelcase, computeEnabledFeatures, matchHostname, parseFeatureSettings } from './utils.js'; +import { URLPattern } from 'urlpattern-polyfill'; export default class ConfigFeature { /** @type {import('./utils.js').RemoteConfig | undefined} */ @@ -48,19 +49,55 @@ export default class ConfigFeature { * @protected */ matchDomainFeatureSetting(featureKeyName) { - const domain = this.args?.site.domain; - if (!domain) return []; const domains = this._getFeatureSettings()?.[featureKeyName] || []; return domains.filter((rule) => { - if (Array.isArray(rule.domain)) { - return rule.domain.some((domainRule) => { - return matchHostname(domain, domainRule); - }); - } - return matchHostname(domain, rule.domain); + return this.matchConditionalChanges(rule); }); } + /** + * Used to match conditional changes for a settings feature. + * @typedef {object} ConditionBlock + * @property {string[] | string} domain? + * @property {object} urlPattern? + */ + + /** + * Takes a conditional block and returns true if it applies. + * All conditions must be met to return true. + * @param {ConditionBlock} conditionBlock + * @returns {boolean} + */ + matchConditionalChanges(conditionBlock) { + // Check domain condition + if (conditionBlock.domain && !this._matchDomainConditional(conditionBlock)) { + return false; + } + + // Check URL pattern condition + if (conditionBlock.urlPattern) { + const pattern = new URLPattern(conditionBlock.urlPattern); + if (!this.args?.site.url || !pattern.test(this.args?.site.url)) { + return false; + } + } + + // All conditions are met + return true; + } + + _matchDomainConditional(conditionBlock) { + if (!conditionBlock.domain) return false; + const domain = this.args?.site.domain; + if (!domain) return false; + if (Array.isArray(conditionBlock.domain)) { + return conditionBlock.domain.some((domainRule) => { + return matchHostname(domain, domainRule); + }); + } + return matchHostname(domain, conditionBlock.domain); + } + /** * Return the settings object for a feature * @param {string} [featureName] - The name of the feature to get the settings for; defaults to the name of the feature @@ -131,13 +168,20 @@ export default class ConfigFeature { */ getFeatureSetting(featureKeyName, featureName) { let result = this._getFeatureSettings(featureName); - if (featureKeyName === 'domains') { - throw new Error('domains is a reserved feature setting key name'); + if (featureKeyName in ['domains', 'conditionalChanges']) { + throw new Error(`${featureKeyName} is a reserved feature setting key name`); } - const domainMatch = [...this.matchDomainFeatureSetting('domains')].sort((a, b) => { - return a.domain.length - b.domain.length; - }); - for (const match of domainMatch) { + // We only support one of these keys at a time, where conditionalChanges takes precedence + // TODO should we rename these? + // TODO should we only support conditionalChanges to support other types of settings? + let conditionalMatches = []; + // Presence check using result to avoid the [] default response + if (result?.conditionalChanges) { + conditionalMatches = this.matchDomainFeatureSetting('conditionalChanges'); + } else { + conditionalMatches = this.matchDomainFeatureSetting('domains'); + } + for (const match of conditionalMatches) { if (match.patchSettings === undefined) { continue; } diff --git a/injected/src/content-feature.js b/injected/src/content-feature.js index bc1208c7ea..5912cfa508 100644 --- a/injected/src/content-feature.js +++ b/injected/src/content-feature.js @@ -17,6 +17,7 @@ import ConfigFeature from './config-feature.js'; /** * @typedef {object} Site * @property {string | null} domain + * @property {string | null} url * @property {boolean} [isBroken] * @property {boolean} [allowlisted] * @property {string[]} [enabledFeatures] diff --git a/injected/src/utils.js b/injected/src/utils.js index 2a5691b88d..3c041b3732 100644 --- a/injected/src/utils.js +++ b/injected/src/utils.js @@ -118,31 +118,40 @@ export function hasThirdPartyOrigin(scriptOrigins) { } /** - * Best guess effort of the tabs hostname; where possible always prefer the args.site.domain - * @returns {string|null} inferred tab hostname + * @returns {URL | null} */ -export function getTabHostname() { - let framingOrigin = null; +export function getTabUrl() { + let framingURLString = null; try { // @ts-expect-error - globalThis.top is possibly 'null' here - framingOrigin = globalThis.top.location.href; + framingURLString = globalThis.top.location.href; } catch { - framingOrigin = globalThis.document.referrer; + framingURLString = globalThis.document.referrer; } + let framingURL; + try { + framingURL = new URL(framingURLString); + } catch { + framingURL = null; + } + return framingURL; +} + +/** + * Best guess effort of the tabs hostname; where possible always prefer the args.site.domain + * @returns {string|null} inferred tab hostname + */ +export function getTabHostname() { + let topURLString = getTabUrl()?.hostname; + // For about:blank, we can't get the top location // Not supported in Firefox if ('ancestorOrigins' in globalThis.location && globalThis.location.ancestorOrigins.length) { // ancestorOrigins is reverse order, with the last item being the top frame - framingOrigin = globalThis.location.ancestorOrigins.item(globalThis.location.ancestorOrigins.length - 1); - } - - try { - // @ts-expect-error - framingOrigin is possibly 'null' here - framingOrigin = new URL(framingOrigin).hostname; - } catch { - framingOrigin = null; + // @ts-expect-error - globalThis.top is possibly 'null' here + topURLString = globalThis.location.ancestorOrigins.item(globalThis.location.ancestorOrigins.length - 1); } - return framingOrigin; + return topURLString || null; } /** @@ -524,6 +533,7 @@ export function computeLimitedSiteObject() { const topLevelHostname = getTabHostname(); return { domain: topLevelHostname, + url: getTabUrl()?.href || null, }; } diff --git a/injected/unit-test/content-feature.js b/injected/unit-test/content-feature.js index ce88337871..584ee36672 100644 --- a/injected/unit-test/content-feature.js +++ b/injected/unit-test/content-feature.js @@ -16,6 +16,7 @@ describe('ContentFeature class', () => { const args = { site: { domain: 'beep.example.com', + url: 'http://beep.example.com', }, featureSettings: { test: { @@ -48,6 +49,101 @@ describe('ContentFeature class', () => { expect(didRun).withContext('Should run').toBeTrue(); }); + it('Should trigger getFeatureSettingEnabled for the correct domain', () => { + let didRun = false; + class MyTestFeature2 extends ContentFeature { + init() { + expect(this.getFeatureSetting('test')).toBe('enabled3'); + expect(this.getFeatureSetting('otherTest')).toBe('enabled'); + expect(this.getFeatureSetting('otherOtherTest')).toBe('ding'); + expect(this.getFeatureSetting('arrayTest')).toBe('enabledArray'); + expect(this.getFeatureSetting('pathTest')).toBe('beep'); + expect(this.getFeatureSetting('pathTestNotApply')).toBe('nope'); + expect(this.getFeatureSetting('pathTestShort')).toBe('beep'); + expect(this.getFeatureSetting('pathTestAsterix')).toBe('comic'); + expect(this.getFeatureSetting('pathTestPlaceholder')).toBe('place'); + expect(this.getFeatureSetting('domainWildcard')).toBe('wildwest'); + expect(this.getFeatureSetting('domainWildcardNope')).toBe('nope'); + didRun = true; + } + } + + const args = { + site: { + domain: 'beep.example.com', + url: 'http://beep.example.com/path/path/me', + }, + featureSettings: { + test: { + test: 'enabled', + otherTest: 'disabled', + otherOtherTest: 'ding', + arrayTest: 'enabled', + pathTest: 'nope', + pathTestNotApply: 'nope', + pathTestShort: 'nope', + pathTestAsterix: 'nope', + pathTestPlaceholder: 'nope', + domainWildcard: 'nope', + domainWildcardNope: 'nope', + conditionalChanges: [ + { + domain: 'example.com', + patchSettings: [ + { op: 'replace', path: '/test', value: 'enabled2' }, + { op: 'replace', path: '/otherTest', value: 'enabled' }, + ], + }, + { + domain: 'beep.example.com', + patchSettings: [{ op: 'replace', path: '/test', value: 'enabled3' }], + }, + { + domain: ['meep.com', 'example.com'], + patchSettings: [{ op: 'replace', path: '/arrayTest', value: 'enabledArray' }], + }, + { + urlPattern: { + path: '/path/path/me', + }, + patchSettings: [{ op: 'replace', path: '/pathTest', value: 'beep' }], + }, + { + urlPattern: { + hostname: 'beep.nope.com', + path: '/path/path/me', + }, + patchSettings: [{ op: 'replace', path: '/pathTestNotApply', value: 'yep' }], + }, + { + urlPattern: 'http://beep.example.com/path/path/me', + patchSettings: [{ op: 'replace', path: '/pathTestShort', value: 'beep' }], + }, + { + urlPattern: 'http://beep.example.com/*/path/me', + patchSettings: [{ op: 'replace', path: '/pathTestAsterix', value: 'comic' }], + }, + { + urlPattern: 'http://beep.example.com/:something/path/me', + patchSettings: [{ op: 'replace', path: '/pathTestPlaceholder', value: 'place' }], + }, + { + urlPattern: 'http://beep.*.com/*/path/me', + patchSettings: [{ op: 'replace', path: '/domainWildcard', value: 'wildwest' }], + }, + { + urlPattern: 'http://nope.*.com/*/path/me', + patchSettings: [{ op: 'replace', path: '/domainWildcardNope', value: 'wildwest' }], + }, + ], + }, + }, + }; + const me = new MyTestFeature2('test', {}, args); + me.callInit(args); + expect(didRun).withContext('Should run').toBeTrue(); + }); + describe('addDebugFlag', () => { class MyTestFeature extends ContentFeature { // eslint-disable-next-line diff --git a/injected/unit-test/helpers/polyfill-process-globals.js b/injected/unit-test/helpers/polyfill-process-globals.js index 3ae7bff555..b03d1bd4eb 100644 --- a/injected/unit-test/helpers/polyfill-process-globals.js +++ b/injected/unit-test/helpers/polyfill-process-globals.js @@ -2,6 +2,7 @@ export function polyfillProcessGlobals() { // Store original values to restore later const originalDocument = globalThis.document; const originalLocation = globalThis.location; + const originalTop = globalThis.top; // Apply the patch globalThis.document = { @@ -15,17 +16,16 @@ export function polyfillProcessGlobals() { }, }; - globalThis.location = { - href: 'http://localhost:8080', - // @ts-expect-error - ancestorOrigins is not defined in the type definition - ancestorOrigins: { - length: 0, - }, - }; + globalThis.location = globalThis.document.location; + + globalThis.top = Object.assign({}, originalTop, { + location: globalThis.location, + }); // Return a cleanup function return function cleanup() { globalThis.document = originalDocument; globalThis.location = originalLocation; + globalThis.top = originalTop; }; } diff --git a/injected/unit-test/utils.js b/injected/unit-test/utils.js index 7c0ba602f1..ef37ac8686 100644 --- a/injected/unit-test/utils.js +++ b/injected/unit-test/utils.js @@ -69,6 +69,7 @@ describe('Helpers checks', () => { expect(processedConfig).toEqual({ site: { domain: 'localhost', + url: 'http://localhost:8080/', isBroken: false, allowlisted: false, // testFeatureTooBig is not enabled because it's minSupportedVersion is 100 @@ -147,6 +148,7 @@ describe('Helpers checks', () => { expect(processedConfig).toEqual({ site: { domain: 'localhost', + url: 'http://localhost:8080/', isBroken: false, allowlisted: false, // testFeatureTooBig is not enabled because it's minSupportedVersion is 100 diff --git a/package-lock.json b/package-lock.json index 21fd8f4038..c047529f9c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,7 +27,8 @@ "stylelint-csstree-validator": "^3.0.0", "typedoc": "^0.27.9", "typescript": "^5.8.2", - "typescript-eslint": "^8.27.0" + "typescript-eslint": "^8.27.0", + "urlpattern-polyfill": "^10.0.0" } }, "injected": { @@ -39,7 +40,7 @@ }, "devDependencies": { "@canvas/image-data": "^1.0.0", - "@duckduckgo/privacy-configuration": "github:duckduckgo/privacy-configuration#e159899e1301fb36c76fd60126f9840a70dd2c3a", + "@duckduckgo/privacy-configuration": "github:duckduckgo/privacy-configuration#main", "@fingerprintjs/fingerprintjs": "^4.5.1", "@types/chrome": "^0.0.308", "@types/jasmine": "^5.1.7", @@ -6541,6 +6542,13 @@ "dev": true, "license": "MIT" }, + "node_modules/urlpattern-polyfill": { + "version": "10.0.0", + "resolved": "https://registry.npmjs.org/urlpattern-polyfill/-/urlpattern-polyfill-10.0.0.tgz", + "integrity": "sha512-H/A06tKD7sS1O1X2SshBVeA5FLycRpjqiBeqGKmBwBDBy28EnRjORxTNe269KSSr5un5qyWi1iL61wLxpd+ZOg==", + "dev": true, + "license": "MIT" + }, "node_modules/util-deprecate": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/util-deprecate/-/util-deprecate-1.0.2.tgz", diff --git a/package.json b/package.json index 01f431f6eb..1a3bc6e8f4 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,8 @@ "stylelint-csstree-validator": "^3.0.0", "typedoc": "^0.27.9", "typescript": "^5.8.2", - "typescript-eslint": "^8.27.0" + "typescript-eslint": "^8.27.0", + "urlpattern-polyfill": "^10.0.0" }, "dependencies": { "immutable-json-patch": "^6.0.1" From aed4fb34ce54c43dd474cda05ef3ce70e18661c9 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Mon, 31 Mar 2025 21:41:41 +0100 Subject: [PATCH 2/6] Rename matchDomainFeatureSetting to matchConditionalFeatureSetting Additionally ensure all checks match --- injected/src/config-feature.js | 44 ++++++++++++-------- injected/src/features/element-hiding.js | 4 +- injected/src/features/navigator-interface.js | 2 +- injected/unit-test/content-feature.js | 43 ++++++++++++++----- 4 files changed, 62 insertions(+), 31 deletions(-) diff --git a/injected/src/config-feature.js b/injected/src/config-feature.js index e4d99371df..dcad1021eb 100644 --- a/injected/src/config-feature.js +++ b/injected/src/config-feature.js @@ -42,16 +42,17 @@ export default class ConfigFeature { } /** - * Given a config key, interpret the value as a list of domain overrides, and return the elements that match the current page - * Consider using patchSettings instead as per `getFeatureSetting`. + * Given a config key, interpret the value as a list of conditionals objects, and return the elements that match the current page + * Consider in your feature using patchSettings instead as per `getFeatureSetting`. * @param {string} featureKeyName * @return {any[]} * @protected */ - matchDomainFeatureSetting(featureKeyName) { + matchConditionalFeatureSetting(featureKeyName) { const domains = this._getFeatureSettings()?.[featureKeyName] || []; return domains.filter((rule) => { - return this.matchConditionalChanges(rule); + // Support shorthand for domain matching for backwards compatibility + return this.matchConditionalChanges(rule.condition || { domain: rule.domain }); }); } @@ -69,16 +70,25 @@ export default class ConfigFeature { * @returns {boolean} */ matchConditionalChanges(conditionBlock) { - // Check domain condition - if (conditionBlock.domain && !this._matchDomainConditional(conditionBlock)) { - return false; - } - - // Check URL pattern condition - if (conditionBlock.urlPattern) { - const pattern = new URLPattern(conditionBlock.urlPattern); - if (!this.args?.site.url || !pattern.test(this.args?.site.url)) { - return false; + for (const key in conditionBlock) { + switch (key) { + case 'domain': { + if (!this._matchDomainConditional(conditionBlock)) { + return false; + } + break; + } + case 'urlPattern': { + const pattern = new URLPattern(conditionBlock.urlPattern); + if (!this.args?.site.url || !pattern.test(this.args?.site.url)) { + return false; + } + break; + } + // For unknown keys we should assume the condition is not met + default: { + return false; + } } } @@ -172,14 +182,12 @@ export default class ConfigFeature { throw new Error(`${featureKeyName} is a reserved feature setting key name`); } // We only support one of these keys at a time, where conditionalChanges takes precedence - // TODO should we rename these? - // TODO should we only support conditionalChanges to support other types of settings? let conditionalMatches = []; // Presence check using result to avoid the [] default response if (result?.conditionalChanges) { - conditionalMatches = this.matchDomainFeatureSetting('conditionalChanges'); + conditionalMatches = this.matchConditionalFeatureSetting('conditionalChanges'); } else { - conditionalMatches = this.matchDomainFeatureSetting('domains'); + conditionalMatches = this.matchConditionalFeatureSetting('domains'); } for (const match of conditionalMatches) { if (match.patchSettings === undefined) { diff --git a/injected/src/features/element-hiding.js b/injected/src/features/element-hiding.js index ded7c38a2d..75d3afaabb 100644 --- a/injected/src/features/element-hiding.js +++ b/injected/src/features/element-hiding.js @@ -320,11 +320,11 @@ export default class ElementHiding extends ContentFeature { // determine whether strict hide rules should be injected as a style tag if (shouldInjectStyleTag) { - shouldInjectStyleTag = this.matchDomainFeatureSetting('styleTagExceptions').length === 0; + shouldInjectStyleTag = this.matchConditionalFeatureSetting('styleTagExceptions').length === 0; } // collect all matching rules for domain - const activeDomainRules = this.matchDomainFeatureSetting('domains').flatMap((item) => item.rules); + const activeDomainRules = this.matchConditionalFeatureSetting('domains').flatMap((item) => item.rules); const overrideRules = activeDomainRules.filter((rule) => { return rule.type === 'override'; diff --git a/injected/src/features/navigator-interface.js b/injected/src/features/navigator-interface.js index 57843d3abd..bc57dfccc7 100644 --- a/injected/src/features/navigator-interface.js +++ b/injected/src/features/navigator-interface.js @@ -4,7 +4,7 @@ import { createPageWorldBridge } from './message-bridge/create-page-world-bridge export default class NavigatorInterface extends ContentFeature { load(args) { - if (this.matchDomainFeatureSetting('privilegedDomains').length) { + if (this.matchConditionalFeatureSetting('privilegedDomains').length) { this.injectNavigatorInterface(args); } } diff --git a/injected/unit-test/content-feature.js b/injected/unit-test/content-feature.js index 584ee36672..41b0b426d5 100644 --- a/injected/unit-test/content-feature.js +++ b/injected/unit-test/content-feature.js @@ -64,6 +64,7 @@ describe('ContentFeature class', () => { expect(this.getFeatureSetting('pathTestPlaceholder')).toBe('place'); expect(this.getFeatureSetting('domainWildcard')).toBe('wildwest'); expect(this.getFeatureSetting('domainWildcardNope')).toBe('nope'); + expect(this.getFeatureSetting('invalidCheck')).toBe('nope'); didRun = true; } } @@ -86,6 +87,7 @@ describe('ContentFeature class', () => { pathTestPlaceholder: 'nope', domainWildcard: 'nope', domainWildcardNope: 'nope', + invalidCheck: 'nope', conditionalChanges: [ { domain: 'example.com', @@ -103,38 +105,59 @@ describe('ContentFeature class', () => { patchSettings: [{ op: 'replace', path: '/arrayTest', value: 'enabledArray' }], }, { - urlPattern: { - path: '/path/path/me', + condition: { + urlPattern: { + path: '/path/path/me', + }, }, patchSettings: [{ op: 'replace', path: '/pathTest', value: 'beep' }], }, { - urlPattern: { - hostname: 'beep.nope.com', - path: '/path/path/me', + condition: { + urlPattern: { + hostname: 'beep.nope.com', + path: '/path/path/me', + }, }, patchSettings: [{ op: 'replace', path: '/pathTestNotApply', value: 'yep' }], }, { - urlPattern: 'http://beep.example.com/path/path/me', + condition: { + urlPattern: 'http://beep.example.com/path/path/me', + }, patchSettings: [{ op: 'replace', path: '/pathTestShort', value: 'beep' }], }, { - urlPattern: 'http://beep.example.com/*/path/me', + condition: { + urlPattern: 'http://beep.example.com/*/path/me', + }, patchSettings: [{ op: 'replace', path: '/pathTestAsterix', value: 'comic' }], }, { - urlPattern: 'http://beep.example.com/:something/path/me', + condition: { + urlPattern: 'http://beep.example.com/:something/path/me', + }, patchSettings: [{ op: 'replace', path: '/pathTestPlaceholder', value: 'place' }], }, { - urlPattern: 'http://beep.*.com/*/path/me', + condition: { + urlPattern: 'http://beep.*.com/*/path/me', + }, patchSettings: [{ op: 'replace', path: '/domainWildcard', value: 'wildwest' }], }, { - urlPattern: 'http://nope.*.com/*/path/me', + condition: { + urlPattern: 'http://nope.*.com/*/path/me', + }, patchSettings: [{ op: 'replace', path: '/domainWildcardNope', value: 'wildwest' }], }, + { + condition: { + somethingInvalid: true, + urlPattern: 'http://beep.example.com/*/path/me', + }, + patchSettings: [{ op: 'replace', path: '/invalidCheck', value: 'neverhappened' }], + }, ], }, }, From ee0ba6574392bfa0eab158a1ff65df001670aab2 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Wed, 2 Apr 2025 12:58:31 +0100 Subject: [PATCH 3/6] Add in array matching of conditions and simplify logic to prevent errors in future --- injected/src/config-feature.js | 205 +++++++++++++++++++------- injected/unit-test/content-feature.js | 105 +++++++++++++ 2 files changed, 257 insertions(+), 53 deletions(-) diff --git a/injected/src/config-feature.js b/injected/src/config-feature.js index dcad1021eb..8021dd8351 100644 --- a/injected/src/config-feature.js +++ b/injected/src/config-feature.js @@ -49,19 +49,48 @@ export default class ConfigFeature { * @protected */ matchConditionalFeatureSetting(featureKeyName) { - const domains = this._getFeatureSettings()?.[featureKeyName] || []; - return domains.filter((rule) => { + const conditionalChanges = this._getFeatureSettings()?.[featureKeyName] || []; + return conditionalChanges.filter((rule) => { + let condition = rule.condition; // Support shorthand for domain matching for backwards compatibility - return this.matchConditionalChanges(rule.condition || { domain: rule.domain }); + if (condition === undefined && 'domain' in rule) { + condition = this._domainToConditonBlocks(rule.domain); + } + return this._matchConditionalBlockOrArray(condition); }); } + /** + * Takes a list of domains and returns a list of condition blocks + * @param {string|string[]} domain + * @returns {ConditionBlock[]} + */ + _domainToConditonBlocks(domain) { + if (Array.isArray(domain)) { + return domain.map((domain) => ({ domain })); + } else { + return [{ domain }]; + } + } + /** * Used to match conditional changes for a settings feature. * @typedef {object} ConditionBlock - * @property {string[] | string} domain? - * @property {object} urlPattern? + * @property {string[] | string} [domain] + * @property {object} [urlPattern] + */ + + /** + * Takes multiple conditional blocks and returns true if any apply. + * @param {ConditionBlock|ConditionBlock[]} conditionBlock + * @returns {boolean} */ + _matchConditionalBlockOrArray(conditionBlock) { + if (Array.isArray(conditionBlock)) { + return conditionBlock.some((block) => this._matchConditionalBlock(block)); + } + return this._matchConditionalBlock(conditionBlock); + } /** * Takes a conditional block and returns true if it applies. @@ -69,41 +98,71 @@ export default class ConfigFeature { * @param {ConditionBlock} conditionBlock * @returns {boolean} */ - matchConditionalChanges(conditionBlock) { + _matchConditionalBlock(conditionBlock) { + // List of conditions that we support currently, these return truthy if the condition is met + /** @type {Record boolean>} */ + const conditionChecks = { + domain: this._matchDomainConditional, + urlPattern: this._matchUrlPatternConditional, + }; + for (const key in conditionBlock) { - switch (key) { - case 'domain': { - if (!this._matchDomainConditional(conditionBlock)) { - return false; + /* + Unsupported condition so fail for backwards compatibility + If you wish to support older clients you should create an old condition block + without the unsupported key also. + Such as: + [ + { + condition: { + domain: 'example.com' + } + }, + { + condition: { + domain: 'example.com', + newKey: 'value' + } } - break; - } - case 'urlPattern': { - const pattern = new URLPattern(conditionBlock.urlPattern); - if (!this.args?.site.url || !pattern.test(this.args?.site.url)) { - return false; - } - break; - } - // For unknown keys we should assume the condition is not met - default: { - return false; - } + ] + */ + if (!conditionChecks[key]) { + return false; + } else if (!conditionChecks[key].call(this, conditionBlock)) { + return false; } } - - // All conditions are met return true; } + /** + * Takes a condtion block and returns true if the current url matches the urlPattern. + * @param {ConditionBlock} conditionBlock + * @returns {boolean} + */ + _matchUrlPatternConditional(conditionBlock) { + const url = this.args?.site.url; + if (!url) return false; + if (typeof conditionBlock.urlPattern === 'string') { + // Use the current URL as the base for matching + return new URLPattern(conditionBlock.urlPattern, url).test(url); + } + const pattern = new URLPattern(conditionBlock.urlPattern); + return pattern.test(url); + } + + /** + * Takes a condition block and returns true if the current domain matches the domain. + * @param {ConditionBlock} conditionBlock + * @returns {boolean} + */ _matchDomainConditional(conditionBlock) { if (!conditionBlock.domain) return false; const domain = this.args?.site.domain; if (!domain) return false; if (Array.isArray(conditionBlock.domain)) { - return conditionBlock.domain.some((domainRule) => { - return matchHostname(domain, domainRule); - }); + // Explicitly check for an empty array as matchHostname will return true a single item array that matches + return false; } return matchHostname(domain, conditionBlock.domain); } @@ -151,31 +210,71 @@ export default class ConfigFeature { } /** - * Return a specific setting from the feature settings - * If the "settings" key within the config has a "domains" key, it will be used to override the settings. - * This uses JSONPatch to apply the patches to settings before getting the setting value. - * For example.com getFeatureSettings('val') will return 1: - * ```json - * { - * "settings": { - * "domains": [ - * { - * "domain": "example.com", - * "patchSettings": [ - * { "op": "replace", "path": "/val", "value": 1 } - * ] - * } - * ] - * } - * } - * ``` - * "domain" can either be a string or an array of strings. - - * For boolean states you should consider using getFeatureSettingEnabled. - * @param {string} featureKeyName - * @param {string} [featureName] - * @returns {any} - */ + * Return a specific setting from the feature settings + * If the "settings" key within the config has a "conditionalChanges" key, it will be used to override the settings. + * This uses JSONPatch to apply the patches to settings before getting the setting value. + * For example.com getFeatureSettings('val') will return 1: + * ```json + * { + * "settings": { + * "conditionalChanges": [ + * { + * "domain": "example.com", + * "patchSettings": [ + * { "op": "replace", "path": "/val", "value": 1 } + * ] + * } + * ] + * } + * } + * ``` + * "domain" can either be a string or an array of strings. + * Additionally we support urlPattern for more complex matching. + * For example.com getFeatureSettings('val') will return 1: + * ```json + * { + * "settings": { + * "conditionalChanges": [ + * { + * "condition": { + * "urlPattern": "https://example.com/*", + * }, + * "patchSettings": [ + * { "op": "replace", "path": "/val", "value": 1 } + * ] + * } + * ] + * } + * } + * ``` + * We also support multiple conditions: + * ```json + * { + * "settings": { + * "conditionalChanges": [ + * { + * "condition": [ + * { + * "urlPattern": "https://example.com/*", + * }, + * { + * "urlPattern": "https://other.com/path/something", + * }, + * ], + * "patchSettings": [ + * { "op": "replace", "path": "/val", "value": 1 } + * ] + * } + * ] + * } + * } + * ``` + * + * For boolean states you should consider using getFeatureSettingEnabled. + * @param {string} featureKeyName + * @param {string} [featureName] + * @returns {any} + */ getFeatureSetting(featureKeyName, featureName) { let result = this._getFeatureSettings(featureName); if (featureKeyName in ['domains', 'conditionalChanges']) { diff --git a/injected/unit-test/content-feature.js b/injected/unit-test/content-feature.js index 41b0b426d5..cb2d2caf4d 100644 --- a/injected/unit-test/content-feature.js +++ b/injected/unit-test/content-feature.js @@ -167,6 +167,111 @@ describe('ContentFeature class', () => { expect(didRun).withContext('Should run').toBeTrue(); }); + it('Should trigger getFeatureSetting for the correct conditions', () => { + let didRun = false; + class MyTestFeature3 extends ContentFeature { + init() { + expect(this.getFeatureSetting('test')).toBe('enabled'); + expect(this.getFeatureSetting('otherTest')).toBe('disabled'); + expect(this.getFeatureSetting('test2')).toBe('noop'); + expect(this.getFeatureSetting('otherTest2')).toBe('me'); + expect(this.getFeatureSetting('test3')).toBe('yep'); + expect(this.getFeatureSetting('otherTest3')).toBe('expected'); + expect(this.getFeatureSetting('test4')).toBe('yep'); + expect(this.getFeatureSetting('otherTest4')).toBe('expected'); + expect(this.getFeatureSetting('test5')).toBe('yep'); + expect(this.getFeatureSetting('otherTest5')).toBe('expected'); + didRun = true; + } + } + + const args = { + site: { + domain: 'beep.example.com', + url: 'http://beep.example.com/path/path/me', + }, + featureSettings: { + test: { + test: 'enabled', + otherTest: 'disabled', + test4: 'yep', + otherTest4: 'expected', + conditionalChanges: [ + { + condition: { + // This array case is unsupported currently. + domain: ['example.com'], + }, + patchSettings: [ + { op: 'replace', path: '/test', value: 'enabled2' }, + { op: 'replace', path: '/otherTest', value: 'bloop' }, + ], + }, + { + condition: [ + { + domain: 'example.com', + }, + { + domain: 'other.com', + }, + ], + patchSettings: [ + { op: 'replace', path: '/test2', value: 'noop' }, + { op: 'replace', path: '/otherTest2', value: 'me' }, + ], + }, + { + condition: [ + { + urlPattern: '*://*.example.com', + }, + { + urlPattern: '*://other.com', + }, + ], + patchSettings: [ + { op: 'replace', path: '/test3', value: 'yep' }, + { op: 'replace', path: '/otherTest3', value: 'expected' }, + ], + }, + { + condition: [ + { + // This is at the apex so should not match + urlPattern: '*://example.com', + }, + { + urlPattern: '*://other.com', + }, + ], + patchSettings: [ + { op: 'replace', path: '/test4', value: 'nope' }, + { op: 'replace', path: '/otherTest4', value: 'notexpected' }, + ], + }, + { + condition: [ + { + urlPattern: { + hostname: '*.example.com', + }, + }, + ], + patchSettings: [ + { op: 'replace', path: '/test5', value: 'yep' }, + { op: 'replace', path: '/otherTest5', value: 'expected' }, + ], + }, + ], + }, + }, + }; + const me = new MyTestFeature3('test', {}, args); + me.callInit(args); + expect(didRun).withContext('Should run').toBeTrue(); + }); + describe('addDebugFlag', () => { class MyTestFeature extends ContentFeature { // eslint-disable-next-line From f8be42d265523fb0cceb1522e5e7645d51fbb8fb Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Wed, 9 Apr 2025 16:51:35 +0100 Subject: [PATCH 4/6] Use frame ancestors for tab url too --- injected/src/utils.js | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/injected/src/utils.js b/injected/src/utils.js index 3c041b3732..2cb80d16f2 100644 --- a/injected/src/utils.js +++ b/injected/src/utils.js @@ -129,6 +129,14 @@ export function getTabUrl() { framingURLString = globalThis.document.referrer; } + if (!framingURLString) { + // This is suboptimal, but we need to get the top level origin in an about:blank frame + const topLevelOriginFromFrameAncestors = getTopLevelOriginFromFrameAncestors(); + if (topLevelOriginFromFrameAncestors) { + framingURLString = topLevelOriginFromFrameAncestors; + } + } + let framingURL; try { framingURL = new URL(framingURLString); @@ -139,18 +147,24 @@ export function getTabUrl() { } /** - * Best guess effort of the tabs hostname; where possible always prefer the args.site.domain - * @returns {string|null} inferred tab hostname + * @returns {string | null} */ -export function getTabHostname() { - let topURLString = getTabUrl()?.hostname; +function getTopLevelOriginFromFrameAncestors() { // For about:blank, we can't get the top location // Not supported in Firefox if ('ancestorOrigins' in globalThis.location && globalThis.location.ancestorOrigins.length) { // ancestorOrigins is reverse order, with the last item being the top frame - // @ts-expect-error - globalThis.top is possibly 'null' here - topURLString = globalThis.location.ancestorOrigins.item(globalThis.location.ancestorOrigins.length - 1); + return globalThis.location.ancestorOrigins.item(globalThis.location.ancestorOrigins.length - 1); } + return null; +} + +/** + * Best guess effort of the tabs hostname; where possible always prefer the args.site.domain + * @returns {string|null} inferred tab hostname + */ +export function getTabHostname() { + const topURLString = getTabUrl()?.hostname; return topURLString || null; } From 83f0dca23870effea6a7f19de176e1d17d80f00b Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Tue, 15 Apr 2025 15:20:37 +0100 Subject: [PATCH 5/6] Add test cases for getTabHostname --- injected/src/utils.js | 12 +--- .../helpers/polyfill-process-globals.js | 59 +++++++++++++++---- injected/unit-test/utils.js | 23 +++++++- 3 files changed, 73 insertions(+), 21 deletions(-) diff --git a/injected/src/utils.js b/injected/src/utils.js index f51be54341..ae71e2a9b4 100644 --- a/injected/src/utils.js +++ b/injected/src/utils.js @@ -134,15 +134,9 @@ export function getTabUrl() { // @ts-expect-error - globalThis.top is possibly 'null' here framingURLString = globalThis.top.location.href; } catch { - framingURLString = globalThis.document.referrer; - } - - if (!framingURLString) { - // This is suboptimal, but we need to get the top level origin in an about:blank frame - const topLevelOriginFromFrameAncestors = getTopLevelOriginFromFrameAncestors(); - if (topLevelOriginFromFrameAncestors) { - framingURLString = topLevelOriginFromFrameAncestors; - } + // If there's no URL then let's fall back to using the frame ancestors origin which won't have path + // Fall back to the referrer if we can't get the top level origin + framingURLString = getTopLevelOriginFromFrameAncestors() ?? globalThis.document.referrer; } let framingURL; diff --git a/injected/unit-test/helpers/polyfill-process-globals.js b/injected/unit-test/helpers/polyfill-process-globals.js index b03d1bd4eb..292ca138c7 100644 --- a/injected/unit-test/helpers/polyfill-process-globals.js +++ b/injected/unit-test/helpers/polyfill-process-globals.js @@ -1,26 +1,63 @@ -export function polyfillProcessGlobals() { +/** + * Creates a mock location object for testing purposes. + * @returns {Location} A mock location object. + */ +export function createLocationObject(href, frameAncestorsList = []) { + return { + href, + // @ts-expect-error - ancestorOrigins is not defined in the type definition + ancestorOrigins: createDomStringList(frameAncestorsList) + }; +} + +export function createDomStringList(list) { + const domStringList = { + length: list.length, + item(index) { + if (index < 0 || index >= list.length) { + return null; + } + return list[index]; + }, + contains(item) { + return list.includes(item); + }, + }; + + // Add index access support + for (let i = 0; i < list.length; i++) { + Object.defineProperty(domStringList, i, { + get() { + return list[i]; + }, + enumerable: true, + }); + } + + return domStringList; +} + +export function polyfillProcessGlobals(defaultLocation = 'http://localhost:8080', frameAncestorsList = [], topisNull = false) { // Store original values to restore later const originalDocument = globalThis.document; const originalLocation = globalThis.location; const originalTop = globalThis.top; // Apply the patch + // @ts-expect-error - document is not defined in the type definition globalThis.document = { - referrer: 'http://localhost:8080', - location: { - href: 'http://localhost:8080', - // @ts-expect-error - ancestorOrigins is not defined in the type definition - ancestorOrigins: { - length: 0, - }, - }, + referrer: defaultLocation, + location: createLocationObject(defaultLocation, frameAncestorsList) }; - globalThis.location = globalThis.document.location; + globalThis.location = createLocationObject(defaultLocation, frameAncestorsList); globalThis.top = Object.assign({}, originalTop, { - location: globalThis.location, + location: createLocationObject(defaultLocation, frameAncestorsList) }); + if (topisNull) { + globalThis.top = null; + } // Return a cleanup function return function cleanup() { diff --git a/injected/unit-test/utils.js b/injected/unit-test/utils.js index ef37ac8686..3b739602fb 100644 --- a/injected/unit-test/utils.js +++ b/injected/unit-test/utils.js @@ -1,4 +1,4 @@ -import { matchHostname, postDebugMessage, initStringExemptionLists, processConfig, satisfiesMinVersion } from '../src/utils.js'; +import { matchHostname, postDebugMessage, initStringExemptionLists, processConfig, satisfiesMinVersion, getTabHostname } from '../src/utils.js'; import { polyfillProcessGlobals } from './helpers/polyfill-process-globals.js'; polyfillProcessGlobals(); @@ -243,4 +243,25 @@ describe('Helpers checks', () => { expect(counters.get('testf')).toEqual(5000); }); }); + + describe('utils.getTabHostname', () => { + it('returns the hostname of the URL', () => { + const hostname = getTabHostname(); + expect(hostname).toEqual('localhost'); + + const reset = polyfillProcessGlobals('http://example.com'); + const hostname2 = getTabHostname(); + expect(hostname2).toEqual('example.com'); + reset(); + + const hostname3 = getTabHostname(); + expect(hostname3).toEqual('localhost'); + + // Validates when we're in a frame thats sandboxed so top is null + const reset2 = polyfillProcessGlobals('https://bloop.com', ['http://example.com'], true); + const hostname5 = getTabHostname(); + expect(hostname5).toEqual('example.com'); + reset2(); + }); + }); }); From e38f97842d4aed2eae2daaa43437f263dffbe099 Mon Sep 17 00:00:00 2001 From: Jonathan Kingston Date: Tue, 15 Apr 2025 15:23:17 +0100 Subject: [PATCH 6/6] lint --- injected/unit-test/helpers/polyfill-process-globals.js | 6 +++--- injected/unit-test/utils.js | 9 ++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/injected/unit-test/helpers/polyfill-process-globals.js b/injected/unit-test/helpers/polyfill-process-globals.js index 292ca138c7..b3bb45a563 100644 --- a/injected/unit-test/helpers/polyfill-process-globals.js +++ b/injected/unit-test/helpers/polyfill-process-globals.js @@ -6,7 +6,7 @@ export function createLocationObject(href, frameAncestorsList = []) { return { href, // @ts-expect-error - ancestorOrigins is not defined in the type definition - ancestorOrigins: createDomStringList(frameAncestorsList) + ancestorOrigins: createDomStringList(frameAncestorsList), }; } @@ -47,13 +47,13 @@ export function polyfillProcessGlobals(defaultLocation = 'http://localhost:8080' // @ts-expect-error - document is not defined in the type definition globalThis.document = { referrer: defaultLocation, - location: createLocationObject(defaultLocation, frameAncestorsList) + location: createLocationObject(defaultLocation, frameAncestorsList), }; globalThis.location = createLocationObject(defaultLocation, frameAncestorsList); globalThis.top = Object.assign({}, originalTop, { - location: createLocationObject(defaultLocation, frameAncestorsList) + location: createLocationObject(defaultLocation, frameAncestorsList), }); if (topisNull) { globalThis.top = null; diff --git a/injected/unit-test/utils.js b/injected/unit-test/utils.js index 3b739602fb..127789de5d 100644 --- a/injected/unit-test/utils.js +++ b/injected/unit-test/utils.js @@ -1,4 +1,11 @@ -import { matchHostname, postDebugMessage, initStringExemptionLists, processConfig, satisfiesMinVersion, getTabHostname } from '../src/utils.js'; +import { + matchHostname, + postDebugMessage, + initStringExemptionLists, + processConfig, + satisfiesMinVersion, + getTabHostname, +} from '../src/utils.js'; import { polyfillProcessGlobals } from './helpers/polyfill-process-globals.js'; polyfillProcessGlobals();