Bug 2039258 - Send security event in case of unsafe site visit#1116
Bug 2039258 - Send security event in case of unsafe site visit#1116mz-darnold wants to merge 22 commits into
Conversation
No Taskcluster jobs started for this pull requestThe |
There was a problem hiding this comment.
Pull request overview
This PR adds enterprise-only telemetry for Safe Browsing “unsafe site” hits, recording a new Glean event and submitting it via the enterprise ping when users land on blocked top-level threat pages in MOZ_ENTERPRISE builds.
Changes:
- Add a new Glean event metric (
security.unsafe_site_visit) gated to MOZ_ENTERPRISE and sent in theenterpriseping. - Emit a new actor message from the blocked-site error page (child actor) and record/submit the telemetry in the parent actor.
- Add an enterprise-only browser-chrome test validating event recording and URL logging policy behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| browser/components/safebrowsing/content/test/browser.toml | Registers the new enterprise-only browser-chrome test. |
| browser/components/safebrowsing/content/test/browser_enterprise_unsafe_site_telemetry.js | Adds coverage for unsafe-site event recording and URL logging policy modes. |
| browser/actors/metrics.yaml | Defines the new enterprise-only security.unsafe_site_visit Glean event and its extra keys. |
| browser/actors/BlockedSiteParent.sys.mjs | Records the event and submits the enterprise ping when notified of an unsafe-site visit. |
| browser/actors/BlockedSiteChild.sys.mjs | Detects top-level unsafe-site hits and notifies the parent actor with threat details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jonathanmendez
left a comment
jonathanmendez
left a comment
There was a problem hiding this comment.
Looks structurally good overall, but there are a few things to follow up on.
|
|
||
| // Safe Browsing threat types that represent an unsafe site visit. Note that | ||
| // "addon" (addonBlocked) is intentionally excluded as it is not a site threat. | ||
| const UNSAFE_SITE_THREAT_TYPES = new Set([ |
There was a problem hiding this comment.
Since the bug is requesting a security event "in case of unsafe site visit", it's not clear to me why we would only care about a subset of unsafe site threats instead of reporting them all. I don't think we want to filter the threat type at all, unless that has been requested at the Product level (i.e. by @bsmth )
| if ( | ||
| !Services.prefs.getBoolPref( | ||
| "browser.safebrowsing.enterprise.telemetry.unsafeSiteVisit.enabled", | ||
| true |
There was a problem hiding this comment.
This matches what we have elsewhere, but I think we should use false for the default value, i.e. only send these events if the policy to do so has been set (and we should fix the other uses). Probably worth double-checking with @gcp in case we want to default to true for some reason.
There was a problem hiding this comment.
I don't know what the default is for the others but it should at least be consistent.
mz-darnold
changed the title
| if ( | ||
| AppConstants.MOZ_ENTERPRISE && | ||
| this.browsingContext === this.browsingContext.top && | ||
| UNSAFE_SITE_THREAT_TYPES.has(aEvent.detail.err) | ||
| ) { |
| if (nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aChannel)) { | ||
| if (nsCOMPtr<nsIReferrerInfo> referrerInfo = | ||
| httpChannel->GetReferrerInfo()) { | ||
| referrerUri = referrerInfo->GetOriginalReferrer(); | ||
| } | ||
| } |
| void MaybeSubmitEnterprisePing() { | ||
| MOZ_ASSERT(NS_IsMainThread()); | ||
|
|
Description
Bugzilla: Bug-2039258
Send telemetry when a user visits a URL identified as malicious by Google Safe Browsing. Also applies to unsafe downloads.
Testing
Steps to verify changes: