Repository navigation
Sync email-only breaches to Remote Settings (mntor-5363) - #6818
Merged
Merged
Conversation
GregHilstonMozilla
requested review from
Vinnl,
codemist,
groovecoder,
joeherm and
mansaj
August 27, 2026 19:59
GregHilstonMozilla
marked this pull request as ready for review
August 27, 2026 20:00
| PwnCount: breach.PwnCount, | ||
| AddedDate: breach.AddedDate, | ||
| DataClasses: breach.DataClasses, | ||
| // The panel hides these, the credential manager may not. |
Collaborator
Author
There was a problem hiding this comment.
this is where I'm adding the new property to the remote settings
GregHilstonMozilla
force-pushed
the
mntor-5363
branch
from
August 31, 2026 13:04
bc57f1e to
1087f24
Compare
joeherm
reviewed
Aug 31, 2026
joeherm
left a comment
joeherm
left a comment
Collaborator
There was a problem hiding this comment.
Code change LGTM- but left a question about the impact on older FF clients.
mansaj
reviewed
Aug 31, 2026
2 tasks done
GregHilstonMozilla
force-pushed
the
mntor-5363
branch
2 times, most recently
from
September 4, 2026 14:14
138b06e to
7cad22c
Compare
Vinnl
approved these changes
Sep 7, 2026
HIBP spells data classes differently to us, and the difference is not mechanical: pins is PINs, ip-addresses is IP addresses, and General is ours alone. Nothing in the codebase recorded that, and I found it confusing. I was concerned that the next person needing it would write the naive transform and be silently wrong for two of thirteen. The Omit makes a missing or misspelled key a compile error. Also documents what distinguishes the three data type lookups, which sit next to each other and are VERY easy to confuse.
The type said keyof HibpBreachDataTypes, which is the keys of our own lookup, but HIBP sends its own Title Case labels. What originally tripped me up is t only ever compiled because "Passwords" is both a key and a real label, and that was the only value any code compared against. `string`` is used , and not something narrower because 163 distinct labels appear across real breaches and we name only 12 of them. So a narrower type would reject valid data. The narrow type already exists a layer down on `HibpLikeDbBreach`, once formatDataClass has converted at ingestion. Both fields now carry a comment, since they share a name and differ only in vocabulary. Which would have originally helped me.
fxmonitor-breaches is how Firefox learns a site was breached, and it held password breaches only, so an email-only breach was invisible and the privacy panel could never render it. IsSensitive ships in the same change because the cron only POSTs records it has not seen and never updates them, so a record that lands without the flag never gets it. Sensitive breaches stay in the collection on purpose: as the panel must hide them, while the credential manager may not.
Vincent and I agreed phase 2 does not need it. Phase 2 only shows breach alerts for breaches the API returns as affecting a user, and the API does not return sensitive breaches, so Firefox never sees one to hide
Vincent pointed out in review that this list is far from exhaustive. HIBP coins a new data class whenever it meets a kind of data it has not seen, and locales/en/data-classes.ftl already lists around 170 against our thirteen
GregHilstonMozilla
force-pushed
the
mntor-5363
branch
from
September 9, 2026 14:31
d00df6d to
d82c515
Compare
Vinnl
approved these changes
Sep 9, 2026
Vinnl
left a comment
Vinnl
left a comment
Collaborator
There was a problem hiding this comment.
Changes look good to me! As discussed, no more need for IsSensitive, considering that we're not even going to be telling the trust panel about sensitive breaches.
GregHilstonMozilla
changed the title
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
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.
References:
Jira: MNTOR-5363
Description
We are going to be iterating on our openapi.yml contract, which is defined between the frontend and the backend for a breach integration. To achieve that with a thin and simple API, we'll be relying on Remote Settings data that is already being pushed. That dataset was missing records.
The sync previously only carried breaches that leaked passwords, so a breach that leaked a user's email but not their password would be invisible to Firefox and the privacy panel could never render it. This PR widens the periodic cron that pushes this data.
The rest of the PR is the types and doc strings that came with that change. HIBP spells data classes differently to us, and nothing in the codebase recorded that.
Changed since the first review: no
IsSensitiveThis PR originally also wrote an
IsSensitivefield onto each new record. Vincent and I agreed it is not needed: phase 2 only shows breach alerts for breaches the API returns as affecting the user, and the API does not return sensitive breaches, so Firefox never sees one to hide.How to test
Checklist (Definition of Done)
BreachDataTypes,ResolutionRelevantBreachDataTypesandHibpLabelByDataType, which sit together and were easy to confuse.IsSensitive.