PoC: Jkt/bk/km c/ad attribution dashboard fix#4441
Draft
jonathanKingston wants to merge 94 commits intomainfrom
Draft
PoC: Jkt/bk/km c/ad attribution dashboard fix#4441jonathanKingston wants to merge 94 commits intomainfrom
jonathanKingston wants to merge 94 commits intomainfrom
Conversation
This reverts commit edac4d5.
The `currentAdClickAttributionTrackerData` property uses the `TrackerData` type from TrackerRadarKit, but only ContentBlocking was imported. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The override logic was too broad — it converted ALL trackers found in the attribution TrackerData to .allowed(reason: .adClickAttribution), but only trackers whose host is in the attribution allowlist (e.g. convert.ad-company.site) should be allowed. Other trackers from the same entity (e.g. ad-company.site) should remain blocked. Changes: - Replace attributionTrackerData with allowlistHosts in the override check - iOS: add attribution rule list to C-S-S data source so attribution trackers are visible to the JavaScript tracker resolver - Add 7 tests covering allowlist matching, subdomain matching, and edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allowlist rules are already regex patterns (e.g. tracker\.com/.*) used directly in JavaScript's .match(). The escapedForRegex() call was double-escaping them, breaking matching in C-S-S's tracker resolver. Also remove unused parameter from injectTrackerProtectionSettings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…1 invariant tests Restores files deleted by this PR that are required for A1 parity: Legacy scripts (from main): - ContentBlockerRulesUserScript.swift + contentblockerrules.js - SurrogatesUserScript.swift + surrogates.js - TrackerResolver.swift Legacy tests (from main): - TrackerResolverTests, ContentBlockerRulesUserScriptsTests - SurrogatesUserScriptTests, ContentBlockerReferenceTests - SurrogatesReferenceTests, TrackerAllowlistReferenceTests - ClickToLoadBlockingTests, DomainMatchingReportTests - iOS DomainMatchingReportTests, macOS PopupHandlingUITests New A1 tests: - ContentBlockingA1InvariantTests (single-authority gate, CTL parity, dataset contract) - ContentBlockingDatasetContractTests (extractSurrogates contract, perf structural assertions) A1 hard gate: legacy processRule path is sole classification authority. C-S-S trackerProtection is removed from Apple platform support (separate C-S-S commit).
PR #4073 modified this file to remove MockRulesUserScriptDelegate, MockSurrogatesUserScriptDelegate, TestSchemeContentBlockerUserScriptConfig, and TestSchemeSurrogatesUserScriptConfig. These are required by the restored legacy test suites.
SPM does not auto-discover .js resources when the target has an explicit resources: block. Add contentblockerrules.js and surrogates.js to the BrowserServicesKit target resources so loadJS can find them via Bundle.module.
…Scope contexts TrackerProtectionDataSource.encodedTrackerData now applies extractSurrogates() before encoding, reducing the payload from ~1.5M chars to ~30-50K chars per the dataset contract. ContentScopeUserScript.generateSource() nils out trackerData for non-contentScope contexts (e.g. contentScopeIsolated), preventing the 1.5M TDS from being duplicated into scripts that don't use it. Expected perf improvement: contentScope source ~670K (from 2.2M), contentScopeIsolated source ~973K (from 2.5M), makeUserScripts back to ~280ms baseline.
The call sites were passing .trackerData (full TDS) directly to ContentScopeProperties, bypassing the encodedTrackerData filtering. Added surrogateFilteredTrackerData property to TrackerProtectionDataSource that returns extractSurrogates(from:) result as a TrackerData object. Updated iOS and macOS call sites to use it.
…Source Protocol conformance fix after adding surrogateFilteredTrackerData to TrackerProtectionDataSource in A2.
TrackerProtectionSubfeature:
- ResourceObservation: raw {url, resourceType, potentiallyBlocked, pageUrl}
- SurrogateInjection: minimal {url, pageUrl, surrogateName} with migration compat
- Handles new resourceObserved + legacy trackerDetected (mapped to ResourceObservation)
- Delegate protocol uses didObserveResource instead of didDetectTracker
TrackerProtectionEventMapper:
- Accepts ResourceObservation, classifies via TrackerResolver with full TDS
- No JS classification authority — native TrackerResolver is sole classifier
- potentiallyBlocked treated as hint, TrackerResolver determines final state
…tion/SurrogateInjection types Tests updated for: - ResourceObservation decoding (url, resourceType, potentiallyBlocked, pageUrl) - SurrogateInjection new minimal schema (url, pageUrl, surrogateName) - SurrogateInjection legacy schema migration compatibility - resourceObserved handler registration - Legacy trackerDetected handler still registered (migration) Remaining compile errors in: - TrackerProtectionEventMapperTests.swift (needs TrackerResolver fixtures) - ContentBlockingTabExtension.swift (macOS delegate conformance) - TabViewController.swift (iOS delegate conformance)
Tests: - TrackerProtectionSubfeatureTests: ResourceObservation/SurrogateInjection decoding - TrackerProtectionEventMapperTests: TrackerResolver-based classification with test TDS - Added memberwise inits to ResourceObservation and SurrogateInjection for test construction App layer (shadow mode — legacy path remains dashboard-authoritative): - macOS ContentBlockingTabExtension: didObserveResource logs shadow observations - iOS TabViewController: didObserveResource/didInjectSurrogate are shadow-only stubs - C-S-S surrogateInjectionEnabled=false during shadow: no C-S-S surrogate injection - Legacy processRule + surrogates.js paths unchanged and authoritative
- Add contentBlockerRulesConfig and surrogatesConfig to protocol and struct - Remove dangling newTabPageActionsManager assignment (removed in main) - Restore mergeTrackerDataSets and encodeTrackerData methods needed by buildSurrogatesConfig
The merge from main dropped ContentBlockerRulesUserScript and SurrogatesUserScript from both iOS and macOS UserScripts. These are the legacy scripts that send processRule messages to native, feeding the privacy dashboard with blocked/allowed request data. - macOS: Add contentBlockerRulesScript + surrogatesScript declarations, init, and userScripts array - iOS: Add protocol requirements, stored properties, build methods, script creation, and userScripts array
macOS: - ContentBlockingTabExtension: restore to main's version exactly (init params, ContentBlockerRulesUserScriptDelegate + SurrogatesUserScriptDelegate conformances, delegate sinks via publishers). Removes shadow-only code (tld, dedup struct, TrackerProtectionSubfeatureDelegate). - TabExtensions.swift: pass contentBlockerRulesUserScriptPublisher + surrogatesUserScriptPublisher instead of trackerProtectionSubfeaturePublisher. iOS: - TabViewController: restore surrogatesScript.delegate and contentBlockerUserScript.delegate assignments, restore ContentBlockerRulesUserScriptDelegate + SurrogatesUserScriptDelegate conformance extensions from main, restore contentBlockerUserScript accessor, restore ad-click attribution wiring to use contentBlockerUserScript. Remove TrackerProtectionSubfeatureDelegate conformance (moved to follow-up commit).
…led promise rejections) The subfeature was registered on contentScopeUserScript but had no delegate after the legacy restore, causing C-S-S tracker-protection.js to fire resourceObserved messages that timed out with unhandled promise rejections. These interfered with the legacy content blocking scripts, causing some tracker detections to be lost. Removed from both iOS and macOS UserScripts. Will be re-added in a separate shadow delegate commit with proper delegate wiring.
…sScriptPublisher The branch's AdClickAttributionTabExtension.init takes trackerProtectionSubfeaturePublisher (optional, defaults to nil), not contentBlockerRulesScriptPublisher. Pass neither — let the default nil apply. The legacy ad-click attribution wiring via contentBlockerUserScript is handled on the iOS side directly in TabViewController.
…line Both paths now run in parallel. Legacy ContentBlockerRulesUserScriptDelegate + SurrogatesUserScriptDelegate remain dashboard-authoritative. The new TrackerProtectionSubfeatureDelegate receives C-S-S resourceObserved/surrogateInjected signals and logs them (macOS) or silently acknowledges them (iOS) for parity comparison without affecting the dashboard. macOS: - UserScripts: re-add trackerProtectionSubfeature property + registration - ContentBlockingTabExtension: add trackerProtectionSubfeaturePublisher param + sink + TrackerProtectionSubfeatureDelegate conformance (shadow logging) - TabExtensions: pass trackerProtectionSubfeature publisher at call site iOS: - UserScripts: re-add trackerProtectionSubfeature property + registration - TabViewController: add delegate assignment + TrackerProtectionSubfeatureDelegate conformance (shadow, no-op acknowledgement)
…elegates Legacy remains sole dashboard authority. Shadow delegates now classify via TrackerProtectionEventMapper and log parity results instead of raw URLs. macOS ContentBlockingTabExtension: - Add trackerProtectionMapper property (constructed from TDS + privacyConfig) - Init takes tld + contentBlockingManager for mapper construction - didObserveResource: classify via mapper, log blocked/allowed + entity - didInjectSurrogate: classify via mapper, log surrogate host + entity macOS TabExtensions: - Pass tld + contentBlockingManager at call site iOS TabViewController: - Add makeMapper() helper using ContentBlocking.shared - Delegate methods remain no-op (iOS parity instrumentation deferred to Commit B)
…at init time) currentMainRules is nil during ContentBlockingTabExtension.init because content blocking assets haven't been compiled yet. Changed from eager let (always nil) to lazy var that constructs the mapper on first access when rules are available.
- macOS ContentBlockingTabExtension: replace legacy ContentBlockerRulesUserScriptDelegate and SurrogatesUserScriptDelegate with authoritative TrackerProtectionSubfeatureDelegate using TrackerProtectionEventMapper classification (blocked → .tracker, cross-site non-blocked → .thirdPartyRequest, same-site → skip) - macOS TabExtensions: remove legacy publisher args, wire trackerProtectionSubfeaturePublisher into AdClickAttributionTabExtension - iOS TabViewController: replace legacy delegates with authoritative TrackerProtectionSubfeatureDelegate, update AdClickAttributionLogicDelegate to use trackerProtectionSubfeature instead of contentBlockerUserScript, use DetectedRequest.entityName for privacy stats - Both platforms UserScripts: remove contentBlockerRulesScript/contentBlockerUserScript and surrogatesScript declarations, initialization, and userScripts array entries - Both platforms ScriptSourceProviding: remove contentBlockerRulesConfig/surrogatesConfig from protocol, struct, init, and related build/merge/encode methods
This reverts commit 3ee9c29.
Fix 1: TrackerProtectionEventMapper now computes potentiallyBlocked from privacyConfig.isEnabled(.contentBlocking) instead of forwarding the C-S-S observation.potentiallyBlocked flag. C-S-S sends pre-block signals where potentiallyBlocked reflects JS heuristics, not WKContentRuleList verdicts. The native-computed candidate mirrors legacy contentblockerrules.js semantics. Fix 2: classifyResource/classifySurrogate now accept an optional adClickAttributionVendor parameter. When provided, a per-call resolver is built with the vendor so ad attribution requests classify as .allowed(.adClickAttribution) instead of .allowed(.ruleException). TrackerResolver remains immutable.
The A1 naming referred to a past migration phase and is misleading. Move 3 non-duplicate tests (legacy JS source structure assertions and encoded surrogate TDS round-trip) into the contract tests file. Drop the duplicate trackerData-absent test already covered there. Delete ContentBlockingA1InvariantTests.swift.
- Allowlist override: TrackerProtectionEventMapper now checks the native allowlist when classifying known trackers, preventing allowlisted trackers from being misclassified as blocked. - Affiliated entity reporting: cross-site non-tracker resources belonging to the same entity as the page now emit .ownedByFirstParty instead of being silently dropped. - Temp-unprotected subdomain parity: temp-unprotected domains now cover subdomains in the native classification path. - Same-site known-tracker suppression: same-site requests to known tracker domains are now suppressed before reaching consumers. - CTL Option A (macOS): exclude CTL supplementary TDS from the mapper when fbBlockingEnabled is false. - iOS callsite: pass trackerAllowlist to the mapper constructor. Made-with: Cursor
…ains Replaces the old test:// scheme handler with a loopback HTTP proxy (TestLoopbackProxy) using WKWebsiteDataStore.proxyConfigurations. Uses real PSL-listed domains for faithful eTLD+1/same-site semantics. Supports split TDS (supplementaryTrackerData, cssTrackerData) for CTL test scenarios. Config builder sets both trackerProtection.settings and features.clickToLoad for pre-built C-S-S bundle compatibility. Legacy helper types preserved for unmigrated tests. Made-with: Cursor
Migrated test files: - ContentBlockerReferenceTests: domain_matching_tests.json scenarios via proxy-based harness with .test->.org domain normalization - TrackerAllowlistReferenceTests: tracker_allowlist_reference.json scenarios with blocked/allowlisted dual-layer assertions - ContentBlockerRulesUserScriptsTests: tests 1-12 migrated; test 13 (contentBlockingEnabled=false) deferred via XCTSkip New replacement suites: - TrackerProtectionBundledSurrogatesSmokeTests: proves real bundled surrogates (analytics.js, gpt.js) inject end-to-end - TrackerProtectionClickToLoadSmokeTests: proves CTL gating/reporting semantics for active/inactive/non-CTL scenarios Focused regression tests added to TrackerProtectionEventMapperTests for allowlist override, port/query stripping, subdomain matching, and CTL supplementary TDS validation. Made-with: Cursor
Temporarily references the kmC/native-content-blocking-tests-059d branch which includes the raw resource observation pipeline and CTL Option B (potentiallyBlocked suppression for CTL-disabled block-ctl-* rules). Made-with: Cursor
Resolved conflicts: - Package.swift: keep C-S-S branch ref (pr-releases/kmC/native-content-blocking-tests-059d) - Package.resolved (both): update C-S-S pin to branch SHA, take main for other deps - macOS UserScripts.swift: keep both trackerProtectionSubfeature and duckAiNativeStorageUserScript Auto-merged cleanly: - HistoryTabExtension.swift: incorporated lazy-load-tabs changes (visits(matching:), restoreLocalHistoryIDs) - TabExtensions.swift: incorporated AutoplayPolicyTabExtension + TabSuspension changes - TabViewModel.swift: incorporated lazy-load title/suspension refactors - iOS TabViewController.swift: incorporated voice mode, privacy dashboard anchor, contextual onboarding - iOS UserScripts.swift: incorporated duckAiNativeStorageUserScript
…iolations - macOS UserScripts.swift: add 'import PrivacyConfig' for PrivacyFeature type used in ContentScopePrivacyConfigurationJSONGenerator excludedFeatures param - TrackerProtectionClickToLoadSmokeTests: fix statement_position lint (else on same line) - TrackerProtectionBundledSurrogatesSmokeTests: fix statement_position lint (else on same line)
- Cache macOS TrackerProtectionEventMapper with vendor-based invalidation, matching the iOS caching pattern. Avoids per-event reconstruction on tracker-heavy pages (bwaresiak feedback on ContentBlockingTabExtension:59) - Rename ContentScopeProperties.trackerData → surrogateTrackerData to clarify it contains surrogate-filtered data, not the full TDS. CodingKey preserved as 'trackerData' for C-S-S JS compatibility (bwaresiak feedback on :100) - Add explanatory comment on AdClickAttributionLogic.onRequestDetected ruleException fallback for vendor-page matching (bwaresiak feedback on :242)
- Remove contentblockerrules.js and surrogates.js from Package.swift resources and delete the files (no longer used in production — C-S-S trackerProtection handles blocking/surrogates via the ContentScopeUserScript pipeline) - Delete SurrogatesReferenceTests (superseded by TrackerProtectionBundledSurrogatesSmokeTests) - Delete SurrogatesUserScriptTests (superseded by rewritten proxy-based tests) - Delete ClickToLoadBlockingTests (superseded by TrackerProtectionClickToLoadSmokeTests) - Remove legacy JS source structure assertions from ContentBlockingDatasetContractTests (the deleted generateSource tests validated the removed JS files)
Production source (no longer referenced by iOS or macOS app code): - Delete ContentBlockerRulesUserScript.swift (241 lines) — replaced by TrackerProtectionSubfeature + C-S-S trackerProtection pipeline - Delete SurrogatesUserScript.swift (221 lines) — surrogate injection now handled by C-S-S with surrogateInjectionEnabled setting Test infrastructure (no remaining consumers after previous test deletions): - Remove MockRulesUserScriptDelegate from WebViewTestHelper - Remove MockSurrogatesUserScriptDelegate from WebViewTestHelper - Remove TestSchemeContentBlockerUserScriptConfig from WebViewTestHelper - Remove TestSchemeSurrogatesUserScriptConfig from WebViewTestHelper Retained (still actively used): - TrackerResolver.swift — used by TrackerProtectionEventMapper - MockNavigationDelegate, TestSchemeHandler, MockWebsite — used by fingerprinting, GPC, rules manager, and HTTPS upgrade tests
DefaultTrackerProtectionDataSource now computes all three derived values (trackerData, surrogateFilteredTrackerData, encodedTrackerData) eagerly at init from a single currentRules snapshot, then stores them as let properties. This eliminates: - Per-access TDS merging (main + CTL rule lists) - Per-access extractSurrogates filtering - Per-access JSON encoding (~30-50K payload) Previously these were computed properties that re-derived on every read. On tracker-heavy pages, encodedTrackerData could be accessed multiple times per page load via ContentScopeUserScript.generateSource(). The init signature, protocol, and all call sites are unchanged. The ScriptSourceProvider already constructs the data source once at init time, so the eager computation simply front-loads the same work.
…Reason Fix #3 from deep review: isTrackerAllowlisted was calling NSRegularExpression(pattern:) for every allowlist entry on every blocked tracker URL — O(blocked_requests × allowlist_entries) compilations. Now pre-compiled at TrackerProtectionEventMapper.init into a [String: [(regex: NSRegularExpression, domains: [String])]] lookup. Matching is pure lookup + firstMatch with zero compilation overhead. Fix #11 from deep review: TrackerBlockingReason.swift was dead code with zero references in any production source file. Deleted.
jonathanKingston
added
the
Run PR Checks
Contributor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 315e80b. Configure here.
…tion-dashboard-fix
Identical to testTempListSubdomainUnblocksTracker: same mapper construction, same URL, same page URL, same assertions. The comment claimed to test 'exception-list domains merged into tempList' but the test used tempList directly without any merging.
- Delete TrackerProtectionMemoryGuardrailTests.swift (159 lines): inlined dict merge instead of testing DefaultTrackerProtectionDataSource, giving false confidence and CI flake risk without guarding real code paths. - Fix testWhenTrackerAllowlistPresentThenItIsExcluded: add trackerAllowlist to the source fixture so the nil assertion proves real stripping, not vacuous absence. - Tighten testNonTrackerThirdPartyRequestClassification: replace silently- passing empty-events branch with XCTSkipIf + XCTAssertNotNil so the test either proves the path works or explicitly skips with explanation.
The mapper cache was keyed only on ad-click attribution vendor. Toggling FB blocking changes supplementary TDS composition (CTL list is included or excluded) without changing the vendor, so the cached mapper could return stale supplementary data. Add fbBlockingEnabled to the cache key so the mapper rebuilds when CTL gating changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1199237043598868/task/1214038914184192?focus=true
Tech Design URL:
CC:
Description
Testing Steps
Impact and Risks
What could go wrong?
Quality Considerations
Notes to Reviewer
Internal references:
Definition of Done | Engineering Expectations | Tech Design Template
Note
Medium Risk
Touches tracker classification/reporting paths (content blocking, allowlist/temp-unprotected handling, ad-click attribution) and bumps core dependencies; regressions could impact blocking accuracy and dashboard reporting, though changes are largely additive with updated tests.
Overview
Upgrades
content-scope-scriptsto14.0.0(and updates SwiftPM pins, includingswift-cryptoand newswift-asn1) and removes the legacyContentBlockerRulesUserScript/SurrogatesUserScriptand their bundled JS resources fromBrowserServicesKit.Introduces a new Content-Scope–driven tracker protection pipeline: adds
TrackerProtectionSubfeaturemessage handling plus a nativeTrackerProtectionEventMapperthat re-classifies C-S-S observations withTrackerResolver, including allowlist normalization and temp-unprotected subdomain parity, and addsTrackerProtectionDataSource/JavaScriptTrackerDatato provide a trimmed tracker-data payload for JS injection.Fixes ad-click attribution activity pixel firing to also treat
ruleExceptionrequests on the active vendor’s own pages as attribution matches, loosens dashboard page association matching, and migrates/rewrites WebKit content-blocking tests to the new proxy-based harness (dropping the old CTL blocking test).Reviewed by Cursor Bugbot for commit 3ecfff8. Bugbot is set up for automated code reviews on this repo. Configure here.