From c8f130ed455b41263f25d0233ad4e9aad72ed040 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89mile=20R=C3=A9?= Date: Mon, 11 May 2026 12:12:47 +0400 Subject: [PATCH] cookiebanner: address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Detectors: keep batched entries in `pending` until the POST succeeds and guard against concurrent flushes, so transient network errors no longer silently drop detection reports. - Worker: add stable tie-breakers to the merge-candidate sort so the greedy assignment produces deterministic groups across runs. - Handler: skip resource entries with an empty URL (zero-value `uri.URI` when the `url` field is missing) before persisting them. - Third-party detector: allow same-origin service worker scripts through `processResource` -- service workers are always same-origin by spec, so the previous filter made `wrapServiceWorker` unreachable. - Resource row edit: bump the description cell `colSpan` to 3 so the edit row spans all five table columns. - Resolver: handle `ErrSameResourceCategoryMove` explicitly so the no-op move returns a validation error instead of an internal one. Signed-off-by: Émile Ré --- .../_components/TrackerResourceRowEdit.tsx | 2 +- .../src/detectors/cookie-detector.ts | 39 +++++++++++------ .../src/detectors/storage-detector.ts | 36 ++++++++++------ .../src/detectors/third-party-detector.ts | 43 +++++++++++++------ pkg/cookiebanner/worker.go | 14 +++++- .../api/console/v1/cookie_banner_resolvers.go | 2 + pkg/server/api/cookiebanner/v1/handler.go | 8 ++++ 7 files changed, 103 insertions(+), 41 deletions(-) diff --git a/apps/console/src/pages/organizations/cookie-banners/configuration/resources/_components/TrackerResourceRowEdit.tsx b/apps/console/src/pages/organizations/cookie-banners/configuration/resources/_components/TrackerResourceRowEdit.tsx index 8221c5010..ad5c4bb8d 100644 --- a/apps/console/src/pages/organizations/cookie-banners/configuration/resources/_components/TrackerResourceRowEdit.tsx +++ b/apps/console/src/pages/organizations/cookie-banners/configuration/resources/_components/TrackerResourceRowEdit.tsx @@ -61,7 +61,7 @@ export function TrackerResourceRowEdit({ placeholder={__("Display name")} /> - +
= new Set(); private readonly pending: Map = new Map(); private timer: ReturnType | null = null; + private flushing = false; private originalDescriptor: PropertyDescriptor | null = null; private cookieStoreHandler: ((event: CookieChangeEvent) => void) | null = null; @@ -165,36 +166,46 @@ export class CookieDetector implements Detector { } private scheduleFlush(): void { - if (this.timer) return; + if (this.timer || this.flushing) return; this.timer = setTimeout(() => { this.timer = null; this.flush(); }, DEBOUNCE_MS); } + // flush sends one batch from `pending` and only removes entries on + // success. Transient failures leave entries in `pending` so they are + // retried on the next flush. `flushing` guards against re-sending an + // in-flight batch when new entries arrive mid-request. private flush(): void { + if (this.flushing) return; if (this.pending.size === 0) return; - const iter = this.pending.entries(); + const batchKeys: string[] = []; const entries: DetectedCookieEntry[] = []; - for (const [key, entry] of iter) { + for (const [key, entry] of this.pending) { + batchKeys.push(key); entries.push(entry); - this.pending.delete(key); if (entries.length >= MAX_COOKIES_PER_REQUEST) break; } + this.flushing = true; void fetchJSON(this.reportUrl, { method: "POST", body: { cookies: entries }, - }).catch((err) => { - if (err instanceof NotFoundError) { - this.pending.clear(); - this.stop(); - } - }); - - if (this.pending.size > 0) { - this.scheduleFlush(); - } + }) + .then(() => { + for (const key of batchKeys) this.pending.delete(key); + }) + .catch((err) => { + if (err instanceof NotFoundError) { + this.pending.clear(); + this.stop(); + } + }) + .finally(() => { + this.flushing = false; + if (this.pending.size > 0) this.scheduleFlush(); + }); } } diff --git a/packages/cookie-banner/src/detectors/storage-detector.ts b/packages/cookie-banner/src/detectors/storage-detector.ts index 7ae7a3444..f6d45aac1 100644 --- a/packages/cookie-banner/src/detectors/storage-detector.ts +++ b/packages/cookie-banner/src/detectors/storage-detector.ts @@ -44,6 +44,7 @@ export class StorageDetector implements Detector { private readonly reported: Set = new Set(); private readonly pending: Map = new Map(); private timer: ReturnType | null = null; + private flushing = false; private originalSetItem: typeof Storage.prototype.setItem | null = null; private originalIDBOpen: typeof IDBFactory.prototype.open | null = null; private originalCachesOpen: typeof CacheStorage.prototype.open | null = null; @@ -230,35 +231,46 @@ export class StorageDetector implements Detector { } private scheduleFlush(): void { - if (this.timer) return; + if (this.timer || this.flushing) return; this.timer = setTimeout(() => { this.timer = null; this.flush(); }, DEBOUNCE_MS); } + // flush sends one batch from `pending` and only removes entries on + // success. Transient failures leave entries in `pending` so they are + // retried on the next flush. `flushing` guards against re-sending an + // in-flight batch when new entries arrive mid-request. private flush(): void { + if (this.flushing) return; if (this.pending.size === 0) return; + const batchKeys: string[] = []; const entries: DetectedStorageEntry[] = []; for (const [key, entry] of this.pending) { + batchKeys.push(key); entries.push(entry); - this.pending.delete(key); if (entries.length >= MAX_ITEMS_PER_REQUEST) break; } + this.flushing = true; void fetchJSON(this.reportUrl, { method: "POST", body: { storage: entries }, - }).catch((err) => { - if (err instanceof NotFoundError) { - this.pending.clear(); - this.stop(); - } - }); - - if (this.pending.size > 0) { - this.scheduleFlush(); - } + }) + .then(() => { + for (const key of batchKeys) this.pending.delete(key); + }) + .catch((err) => { + if (err instanceof NotFoundError) { + this.pending.clear(); + this.stop(); + } + }) + .finally(() => { + this.flushing = false; + if (this.pending.size > 0) this.scheduleFlush(); + }); } } diff --git a/packages/cookie-banner/src/detectors/third-party-detector.ts b/packages/cookie-banner/src/detectors/third-party-detector.ts index b985003d6..1963c7c20 100644 --- a/packages/cookie-banner/src/detectors/third-party-detector.ts +++ b/packages/cookie-banner/src/detectors/third-party-detector.ts @@ -79,6 +79,7 @@ export class ThirdPartyDetector implements Detector { private readonly reported: Set = new Set(); private readonly pending: Map = new Map(); private timer: ReturnType | null = null; + private flushing = false; private observer: MutationObserver | null = null; private perfObserver: PerformanceObserver | null = null; private originalSWRegister: typeof ServiceWorkerContainer.prototype.register | null = null; @@ -247,7 +248,12 @@ export class ThirdPartyDetector implements Detector { } if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return; - if (parsed.origin === this.pageOrigin || parsed.origin === this.proboOrigin) return; + // Service workers are always same-origin per browser security rules, + // so we never drop them based on the page origin -- they are tracked + // regardless of where the script lives. The proboOrigin guard still + // applies so we never report our own SDK assets. + if (parsed.origin === this.proboOrigin) return; + if (resourceType !== "service_worker" && parsed.origin === this.pageOrigin) return; const identifier = parsed.origin + parsed.pathname; const reportKey = `${resourceType}:${identifier}`; @@ -259,35 +265,46 @@ export class ThirdPartyDetector implements Detector { } private scheduleFlush(): void { - if (this.timer) return; + if (this.timer || this.flushing) return; this.timer = setTimeout(() => { this.timer = null; this.flush(); }, DEBOUNCE_MS); } + // flush sends one batch from `pending` and only removes entries on + // success. Transient failures leave entries in `pending` so they are + // retried on the next flush. `flushing` guards against re-sending an + // in-flight batch when new entries arrive mid-request. private flush(): void { + if (this.flushing) return; if (this.pending.size === 0) return; + const batchKeys: string[] = []; const entries: DetectedResourceEntry[] = []; for (const [key, entry] of this.pending) { + batchKeys.push(key); entries.push(entry); - this.pending.delete(key); if (entries.length >= MAX_ITEMS_PER_REQUEST) break; } + this.flushing = true; void fetchJSON(this.reportUrl, { method: "POST", body: { resources: entries }, - }).catch((err) => { - if (err instanceof NotFoundError) { - this.pending.clear(); - this.stop(); - } - }); - - if (this.pending.size > 0) { - this.scheduleFlush(); - } + }) + .then(() => { + for (const key of batchKeys) this.pending.delete(key); + }) + .catch((err) => { + if (err instanceof NotFoundError) { + this.pending.clear(); + this.stop(); + } + }) + .finally(() => { + this.flushing = false; + if (this.pending.size > 0) this.scheduleFlush(); + }); } } diff --git a/pkg/cookiebanner/worker.go b/pkg/cookiebanner/worker.go index fac4b519f..3841c7276 100644 --- a/pkg/cookiebanner/worker.go +++ b/pkg/cookiebanner/worker.go @@ -272,8 +272,20 @@ func findMergeGroups( } } + // Sort by descending specificity (more fixed characters first), then + // descending coverage (more patterns matched first), then template + // name for a fully deterministic order. Without these tie-breakers + // the greedy assignment below depends on Go's randomised map + // iteration and the same input can produce different merge groups + // across runs. sort.Slice(candidates, func(i, j int) bool { - return candidates[i].fixedChars > candidates[j].fixedChars + if candidates[i].fixedChars != candidates[j].fixedChars { + return candidates[i].fixedChars > candidates[j].fixedChars + } + if len(candidates[i].patterns) != len(candidates[j].patterns) { + return len(candidates[i].patterns) > len(candidates[j].patterns) + } + return candidates[i].key.template < candidates[j].key.template }) assigned := make(map[*coredata.TrackerPattern]bool) diff --git a/pkg/server/api/console/v1/cookie_banner_resolvers.go b/pkg/server/api/console/v1/cookie_banner_resolvers.go index 3d8a00afb..b828f5770 100644 --- a/pkg/server/api/console/v1/cookie_banner_resolvers.go +++ b/pkg/server/api/console/v1/cookie_banner_resolvers.go @@ -1155,6 +1155,8 @@ func (r *mutationResolver) MoveTrackerResourceToCategory(ctx context.Context, in return nil, gqlutils.NotFound(ctx, err) case errors.Is(err, cookiebanner.ErrCategoriesBannerMismatch): return nil, gqlutils.NotFoundf(ctx, "tracker resource or target category not found") + case errors.Is(err, cookiebanner.ErrSameResourceCategoryMove): + return nil, gqlutils.Invalidf(ctx, "tracker resource is already in the target category") default: r.logger.ErrorCtx(ctx, "cannot move tracker resource to category", log.Error(err)) return nil, gqlutils.Internal(ctx) diff --git a/pkg/server/api/cookiebanner/v1/handler.go b/pkg/server/api/cookiebanner/v1/handler.go index 3956a31b3..1de0d98fa 100644 --- a/pkg/server/api/cookiebanner/v1/handler.go +++ b/pkg/server/api/cookiebanner/v1/handler.go @@ -441,6 +441,14 @@ func (h *Handler) handleReportDetectedTrackers(w http.ResponseWriter, r *http.Re } for _, res := range body.Resources { + // The URI type validates scheme+host on UnmarshalText, so a + // non-empty value here is guaranteed to be a valid URL. We + // still need to reject the zero value, which occurs when the + // `url` JSON field is missing. + if res.URL == "" { + continue + } + var resourceType coredata.TrackerResourceType switch strings.TrimSpace(res.ResourceType) { case "script":