Last Comment Bug 852809 - Explain what happens when Safe Browsing blocks a website
: Explain what happens when Safe Browsing blocks a website
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Help Documentation (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.19
Assigned To: rsx11m
:
Mentors:
Depends on: 631566
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-19 19:18 PDT by rsx11m
Modified: 2013-03-27 07:52 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (5.16 KB, patch)
2013-03-20 05:34 PDT, rsx11m
no flags Details | Diff | Review
Proposed patch (v2) (5.24 KB, patch)
2013-03-20 17:45 PDT, rsx11m
stefanh: feedback+
Details | Diff | Review
Proposed patch (v3) (5.25 KB, patch)
2013-03-23 10:28 PDT, rsx11m
stefanh: review+
Details | Diff | Review
Final patch (v4) [checked in, comment #13] (5.23 KB, patch)
2013-03-23 12:16 PDT, rsx11m
rsx11m.pub: review+
Details | Diff | Review
Follow-up for more quotes (1.93 KB, patch)
2013-03-25 16:12 PDT, rsx11m
no flags Details | Diff | Review
Follow-up for more quotes (v2) [comment #17] (1.93 KB, patch)
2013-03-25 16:26 PDT, rsx11m
stefanh: review+
Details | Diff | Review

Description rsx11m 2013-03-19 19:18:56 PDT
While bug 631566 introduced the general description of the Safe Browsing (malware and phishing protection) feature and the UI elements in the preference pane, it does not provide any information on what happens when such a site is identified and blocked. Similar to bug 771534 on Offline Web Applications, some brief instructions on how to use the buttons in the warning should be given.
Comment 1 Philip Chee 2013-03-20 04:02:37 PDT
We can probably steal some text from support.mozilla.org:
http://www.mozilla.org/en-US/firefox/phishing-protection/
Comment 2 rsx11m 2013-03-20 05:34:23 PDT
Created attachment 727159 [details] [diff] [review]
Proposed patch

This also contains some minor modifications of the descriptions for the checkboxes and the concluding note.

Phil, thanks for the link. I had looked into this page when educating myself about the feature, but as you can see from the patch, I'm more specific on the warning and the actions which are possible than that more general article. Quite likely a user may be in some distress when running into such a site and seeing the warning show up. Thus, I'm giving step-by-step explanations what to expect with the exact labels and explain up to which point the user is safe.
Comment 3 rsx11m 2013-03-20 06:42:14 PDT
Comment on attachment 727159 [details] [diff] [review]
Proposed patch

>+      <li><strong>This isn&apos;t an attack site</strong> or <strong>This
>+        isn&apos;t a web forgery</strong>: Click this button to open a new tab
>+        or window allowing you to report false warnings (this is an external
>+        page).</li>

By the way: The notification bar has a "Get me out of here!" button as well, but I don't mention it here as it seems redundant, given that it's explained just two bullet items up. Thus, it should be unambiguous.
Comment 4 Stefan [:stefanh] 2013-03-20 13:36:52 PDT
(In reply to Philip Chee from comment #1)
> We can probably steal some text from support.mozilla.org:
> http://www.mozilla.org/en-US/firefox/phishing-protection/

No we can't. If we did that, we need to license our files and put our names of the contributors in them as well.
Comment 5 Stefan [:stefanh] 2013-03-20 16:07:59 PDT
Comment on attachment 727159 [details] [diff] [review]
Proposed patch

Some initial comments - I haven't looked at the patch in detail (will do that on Saturday), but I found some things that might be worth adjusting/discussing.

>+  <li><strong>Get me out of here!</strong> Do <em>not</em> load this website
>+    and instead go to &brandShortName;&apos;s start page.</li>

Maybe we should mention that this is the default start page. So users don't think we'll load their home page (set in the home page prefs). "and instead" sounds a bit strange to me. Perhaps "... . Instead, go to &brandShortName;&apos;s default start page." (start page sounds better, but we call it home page in the prefs ui).

>+  <li><strong>Why was this page blocked?</strong> Opens a new tab or window

For me, the page opens in the same window. We use loadURI to open the page (getMeOutOfHere() in utilityOverlay.js) and I can't see that we honor any new tab/window prefs. I only skimmed the code, though. But it's probably best to just mention that we open a web page.

>+    showing you the information reported on this specific website (this is an
>+    external page located with the provider of this service).</li>

The page you mention does only load if it's a malware page (browser.safebrowsing.malware.reportURL + page URL). If it's a phising site, we load the page specified in 'browser.safebrowsing.warning.infoURL' ("http://www.mozilla.org/%LOCALE%/firefox/phishing-protection/"). You could either mention both pages or try to say something general that covers both pages.
Comment 6 rsx11m 2013-03-20 17:20:47 PDT
(In reply to Stefan [:stefanh] from comment #5)
> Maybe we should mention that this is the default start page. So users don't
> think we'll load their home page (set in the home page prefs). "and instead"
> sounds a bit strange to me. Perhaps "... . Instead, go to
> &brandShortName;&apos;s default start page." (start page sounds better, but
> we call it home page in the prefs ui).

I'll go with that version. It reads a bit bumpier but is more accurate, I agree.

> For me, the page opens in the same window. We use loadURI to open the page
> (getMeOutOfHere() in utilityOverlay.js) and I can't see that we honor any
> new tab/window prefs.

You are correct. The /report/ page opens in a new tab or window depending on the settings, but the explanation page opens in the same window as the warning. I'll change that.

> The page you mention does only load if it's a malware page
> (browser.safebrowsing.malware.reportURL + page URL). If it's a phising site,
> we load the page specified in 'browser.safebrowsing.warning.infoURL'

I can add "or general background information if such data isn't available." at the end after the parentheses to explain the fallback. I don't really want to distinguish phishing vs. malware but the availability of site-specific data, given that this part may or may not change in the future.
Comment 7 rsx11m 2013-03-20 17:45:28 PDT
Created attachment 727467 [details] [diff] [review]
Proposed patch (v2)

Comment #5 addressed. As for the last sentence in the second list item, I've split the proposed addition off into its own sentence, otherwise it's getting rather long (though the parentheses have some separating effect).
Comment 8 Stefan [:stefanh] 2013-03-23 09:48:29 PDT
Comment on attachment 727467 [details] [diff] [review]
Proposed patch (v2)

Just a few things (f+):

> <ul>
>   <li><strong>Block reported attack sites</strong>: Check this if you want
>-    &brandShortName; to actively check whether the website you are about to
>-    visit has been reported trying to interfere with normal computer functions
>-    or to send personal data about you to unauthorized parties over the
>-    Internet. Such <q><a href="glossary.xhtml#malware">malware</a></q>) is
>-    most often used to steal personal information, send junk email (spam),
>-    or spread more malware.</li>
>+    &brandShortName; to actively verify if the website you are about to visit

How about "warn you" instead of "actively verify"?

>+    has been reported as a site that is trying to infect your computer with
>+    malicious software (<q><a href="glossary.xhtml#malware">malware</a></q>).
>+    Such malware may interfere with your computer&apos;s functions or steal

I don't think the paranthesis part is needed. You can just link to malware in "Such malware".

>+    your personal information. It is also frequently used to send spam email
>+    or to spread more malware.</li>
>   <li><strong>Block reported web forgeries</strong>: Check this if you want
>-    &brandShortName; to actively check whether the website you are about to
>-    visit has been reported as an attempt to mislead you into providing personal
>-    information such as username and password (this is often referred to as
>-    <q><a href="glossary.xhtml#phishing">phishing</a></q>). These websites
>-    frequently imitate legitimate websites of well-known organizations.</li>
>+    &brandShortName; to actively verify if the website you are about to visit

See above ("warn you")
 
>+    has been reported as a site that pretends to be a legitimate website of
>+    some well-known organization or service. Those are frequently set up to

"These websites" is much better (and more correct) than "Those". But since we're re-writing... How about "Websites like this"

>+    mislead you in providing username and password or other sensitive personal
>+    information (<q><a href="glossary.xhtml#phishing">phishing</a></q>).</li>

I think we should have something in line with "known as phishing" or "commonly known as phishing". The glossary term doesn't really give you more information than you've already provided. Will users be disappointed if they click the link?

> </ul>
> 
>+<p>When you are trying to visit a website that has been reported as malicious
>+  (and the feature is enabled), you will see one of the following warnings:</p>
>+
>+<ul>
>+  <li><strong>Reported Attack Page!</strong> This website has been reported
>+    as a website trying to infect your computer with malware.</li>
>+  <li><strong>Reported Web Forgery!</strong> This website has been reported
>+    as a website trying to steal your personal information.</li>
>+</ul>
>+
>+<p>No harm has been done at this point. Select one of the following options:</p>
>+
>+<ul>
>+  <li><strong>Get me out of here!</strong> Do <em>not</em> load this website.
>+    Instead, go to &brandShortName;&apos;s default start page.</li>
>+  <li><strong>Why was this page blocked?</strong> Opens a web page showing you
>+    the information reported on this specific website (this is an external
>+    page located with the provider of this service). If no such report data is
>+    available, a page with general information is shown.</li>
>+  <li><strong>Ignore this warning</strong>: Proceed with loading the page. Only
>+    use this when you are <em>certain</em> that the selected page is authentic
>+    and safe to visit. At the top of the page, a notification bar will appear,
>+    giving you an option to report errors to the provider of this service:
>+    <ul>
>+      <li><strong>This isn&apos;t an attack site</strong> or <strong>This
>+        isn&apos;t a web forgery</strong>: Click this button to open a new tab
>+        or window allowing you to report false warnings (this is an external
>+        page).</li>

Hmm. I can't find any evidence in the code that it ever opens in a new window ('openUILinkIn(reportUrl, "tabfocused"').
Comment 9 rsx11m 2013-03-23 10:03:02 PDT
(In reply to Stefan [:stefanh] from comment #8)
> How about "warn you" instead of "actively verify"?

I wanted to emphasize the process of looking up the URL in that list, so that's more the technical description, whereas the "warn you" is more what the user can expect when the event happens from the UX perspective. May be better, sure.

> I don't think the paranthesis part is needed. You can just link to malware
> in "Such malware".

Makes it shorter, agreed.

> "These websites" is much better ... How about "Websites like this"

Sounds good to me.

> I think we should have something in line with "known as phishing" or
> "commonly known as phishing". The glossary term doesn't really give you more
> information than you've already provided. Will users be disappointed if they
> click the link?

Yeah, I didn't extend the glossary entry on phishing in bug 847182, thus it's indeed not adding much compared to the background information here. Making it explicit that it's about the term rather than more details may be a good idea.

> Hmm. I can't find any evidence in the code that it ever opens in a new
> window ('openUILinkIn(reportUrl, "tabfocused"').

Eh, insufficient testing. I was assuming that it just follows the "Link Behavior" settings, but indeed it doesn't. Even with everything set to open in a new window, that page will still be opened in a new tab. So the "window" part has to go.
Comment 10 rsx11m 2013-03-23 10:28:44 PDT
Created attachment 728639 [details] [diff] [review]
Proposed patch (v3)

Comment #8 addressed, no other changes.

>+    reported as a site that is trying to infect your computer with malicious
>+    software. Such <q><a href="glossary.xhtml#malware">malware</a></q> may
and
>+    personal information (commonly known as
>+    <q><a href="glossary.xhtml#phishing">phishing</a></q>).</li>

I've left the quotes around those terms to emphasize that it's about the term.

>+    been reported as a site that pretends to be a legitimate website of some
>+    well-known organization or service. Websites like this are frequently set

I was wondering for a moment if it's supposed to be "Websites like these" rather, but as we are talking about the specific website the user is about to visit, grammar should be fine here with "like this" referring to the specific site.
Comment 11 Stefan [:stefanh] 2013-03-23 11:33:54 PDT
Comment on attachment 728639 [details] [diff] [review]
Proposed patch (v3)

> I've left the quotes around those terms to emphasize that it's about the
> term.

I actually wanted to get rid of the quotes :-). That was one reason to shorten the malware part. I think it looks weird having quotes around links. The quotes are also a bit ambigious since we generally use them to indicate something in the ui. But if you feel strongly about having them there, I won't insist.
Comment 12 rsx11m 2013-03-23 12:16:34 PDT
Created attachment 728652 [details] [diff] [review]
Final patch (v4) [checked in, comment #13]

Well, I'm not feeling *that* strongly about it. ;-)  The rationale was that those are neither abbreviations nor acronyms, but rather new artificial word creations derived from real words. As such, I would introduce them in quotes.

Your argumentation that the meaning of quotes may be ambiguous in this way makes sense, and the link kind-of conveys the same as the quotes already, thus let's loose the quotes.

Push for trunk, please (and I'm ready for a break now).
Comment 13 Stefan [:stefanh] 2013-03-24 10:13:37 PDT
Thanks for the patch :-)

http://hg.mozilla.org/comm-central/rev/cd98d04ab41c
Comment 14 rsx11m 2013-03-25 16:12:57 PDT
Created attachment 729275 [details] [diff] [review]
Follow-up for more quotes

Since we removed the quotes around the glossary links in the Safe Browsing description, this follow-up patch removes them for the User Tracking section instances introduced in bug 847182 too.

No changes other than trivial removal of the <q> tags. These are the only other <q>+<a> combinations in all help/ pages.
Comment 15 rsx11m 2013-03-25 16:26:54 PDT
Created attachment 729281 [details] [diff] [review]
Follow-up for more quotes (v2) [comment #17]

On a second thought, let's make the label match the actual glossary entry.
Comment 16 Stefan [:stefanh] 2013-03-26 03:23:26 PDT
Comment on attachment 729281 [details] [diff] [review]
Follow-up for more quotes (v2) [comment #17]

Looks good, thanks.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-03-27 07:40:20 PDT
https://hg.mozilla.org/comm-central/rev/ab8cfe8058f8

Note You need to log in before you can comment on or make changes to this bug.