-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add links to company websites for breach resolution #2961
Conversation
6eaf2e5
to
858f19f
Compare
858f19f
to
fdac319
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one blocking comment, unfortunately, though probably fairly easy to resolve :)
Not sure if I mentioned this in another thread, but there is a small-ish risk of linking to external sites. A lot of the domains via HIBP are no longer resolving. I looked at one point and think my scripts were guessing about 25% of the outbound domains were invalid. Some very rough stats, if you like grepping JSON files:
BUT the misleading thing is that a lot of those broken 31% reported dead links still work, but the npm module is just reporting bad HTTPS certificates as broken (or getting caught by Cloudflare captchas). PROTIP: If you want to play a fun game, we could WHOIS all the broken domains and see which domains are available to register, then pick them all up and redirect them to https://monitor.firefox.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes to make sure the rationale is clarified.
That’s interesting. Thank you so much for the tool @pdehaan and for listing the stats! As @flodolo mentioned there had been some back and forth on if we would like to show those links or not. We communicated the risks with PM but I’m raising this again to make sure we are aligned. |
@flozia To be clear, the broken links are already on the site on the details page, ie: https://monitor.firefox.com/breach-details/LeakedReality curl -fL https://leakedreality.com # HTTPS
curl: (6) Could not resolve host: leakedreality.com
curl -fL http://leakedreality.com # try HTTP
curl: (6) Could not resolve host: leakedreality.com
echo $? # 6 … but I can see how adding a new [broken] CTA link would be frustrating and confusing to end users. It took a few clicks, but https://monitor.firefox.com/breach-details/Abandonia2022 is another example. Clicking the domain link on the details page takes me to a "Unable to connect. An error occurred during a connection to abandonia.com." error page in Nightly. Changing Or https://monitor.firefox.com/breach-details/GGCorp external domain link takes me to a scary error page (presumably because of a bad/invalid HTTPS cert):
|
@pdehaan Looking at the JSON generated by the script.. I noticed that lots of the sites marked as “dead” / 403s are accessible? Are these false positives? |
I think the link-check module I used might be considering certificate errors to be "dead" links? So there were sites that were reported as dead, but clicking them showed me the website but there was some console logging and/or certificate issues that most people wouldn't really notice. Per tcort/link-check#47 (comment) there might also be some 403 issues if sites are behind Cloudflare or maybe other proxy services as well. Per link-check README:
GitHub search showed this as one of the very few results for "dead" in their codebase: https://github.com/tcort/link-check/blob/430027f6be03b904db508042945bf2a75472d330/lib/LinkCheckResult.js#L10 I can't think of another great solution. I tried using |
d7a1f84
to
e930adb
Compare
3e51d02
to
a0290e5
Compare
a0290e5
to
1b25b2c
Compare
Thanks for your input, everyone! After this PR was on hold while aligning with PM and UX, I’m moving this out of “draft” mode.
|
When you say stripped, you mean that the markup is removed, but the text remains? i.e. it's become inactive text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(never mind my previous comment, the code is clear enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only easy fixes or things that can be ignored, I think :)
src/utils/breachResolution.js
Outdated
case BreachDataTypes.Passwords: | ||
case BreachDataTypes.SecurityQuestions: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, so feel free to ignore, but I'd keep the code simple here and just replace <breached-company-link>
in every string, rather than just the passwords and security questions resolutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No hard preference from my side: I thought being a bit more specific here was OK since we were cautious about how and where we link out to. Less complex is also a good thing, though: d574bee.
// There should be a resolution for `BreachDataTypes.Phone`, | ||
// `BreachDataTypes.Passwords` and `BreachDataTypes.SecurityQuestions`. | ||
// The last two should fallback to a more generic header string that does not | ||
// include the breached company's domain, which we don't know: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how easy it is to mock AppConstants, but if it's easy (but only then - you've been working on this PR for long enough), maybe a test for the blocklist would be a good addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I create an issue for this and will address this in a follow-up in order to not block this PR.
Co-authored-by: Vincent <[email protected]>
Co-authored-by: Vincent <[email protected]>
…/blurts-server into MNTOR-1504-Add-links-to-websites
@@ -58,6 +58,8 @@ HIBP_THROTTLE_DELAY=2000 | |||
HIBP_THROTTLE_MAX_TRIES=5 | |||
# Authorization token for HIBP to present to /hibp/notify endpoint | |||
HIBP_NOTIFY_TOKEN=unsafe-default-token-for-dev | |||
# Domains we prefer to not link to | |||
HIBP_BREACH_DOMAIN_BLOCKLIST=a-blocked-domain.com,another-blocked-domain.org |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a limit how long an ENV var can be?
It feels like this could be REALLY long if we end up blocking over 20-30 domains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to SRE, there is no limit that we would hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it's better as an ENV var that has to be coordinated w/ SRE, versus maybe some JSON file that lives in the repo that is a single source of truth that we can audit occasionally. (unless there are reasons to keep it as a secret/ENV). 🤷
Although I guess I could technically recreate the blocklist locally by scraping the 650 breaches on the Monitor site and see if the outbound link is a link or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still not sure about that as well, but one good argument for handling the list in the env is that we would be able to make adjustments without a release. Especially in the beginning, when we might need to test and audit the sites manually.
const args = { | ||
companyName: b.Name, | ||
breachedCompanyLink: showLink ? `https://${b.Domain}` : '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside: in my personal testing, I think http://
had better results than https://
(somewhere between 5-10% more 2xx/3xx results).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, thanks for the note. With us trying to be cautious where we link out to I think I’d feel more comfortable linking out to https://
.
Thank you for your renewed review @flodolo. Unfortunately, there has been another change to the strings with two additions: A note on using 2FA and the link to Firefox Password Manager — sorry to burn your cycles on these. |
References:
Jira: MNTOR-1504
Description
Adds a link to the breach resolution when passwords or security questions were involved.
Screenshot
How to test
/user/breaches
Checklist (Definition of Done)