1226490 - Allow administrators to prevent users from ignoring Safe Browsing warnings

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

As mentioned on mozilla.dev.security (https://groups.google.com/d/msg/mozilla.dev.security/8Cl-HCLmwTU/vg2byz_GCAAJ), the UK government guidelines on Firefox point out that users can bypass Safe Browsing warnings: https://www.gov.uk/government/publications/browser-security-guidance-mozilla-firefox/browser-security-guidance-mozilla-firefox#summary-of-browser-security

We should add an option to disable the "ignore warning" button.

Comment 1

[François Marier [:francois]]

Attachment: bug1226490.patch

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8690363 [details] [diff] [review]
bug1226490.patch

Review of attachment 8690363 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/blockedSite.xhtml
@@ +21,5 @@
>      <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
>  
>      <script type="application/javascript"><![CDATA[
>        // Error url MUST be formatted like this:
> +      //   about:blocked?e=error_code&u=url&o=(0|1)

Document the meaning of o here, as the param value doesn't explain it unlike error_code.

::: docshell/base/nsDocShell.cpp
@@ +5203,5 @@
>  
>    errorPageUrl.AppendASCII(escapedError.get());
>    errorPageUrl.AppendLiteral("&u=");
>    errorPageUrl.AppendASCII(escapedUrl.get());
> +  if (strncmp(aErrorPage, "blocked", sizeof("blocked") - 1) == 0) {

Can't you use EqualsLiteral("blocked") or something? Would be much cleaner if you can use nsCString.

Maybe find a peer for nsDocShell too.

Comment 3

[François Marier [:francois]]

Attachment: bug1226490.patch

Olli, this patch is related to the one you've just looked at (bug 1216723) and also includes a small docshell change.

Comment 4

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> ::: browser/base/content/blockedSite.xhtml
> @@ +21,5 @@
> >      <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/blacklist_favicon.png"/>
> >  
> >      <script type="application/javascript"><![CDATA[
> >        // Error url MUST be formatted like this:
> > +      //   about:blocked?e=error_code&u=url&o=(0|1)
> 
> Document the meaning of o here, as the param value doesn't explain it unlike
> error_code.

Good point. I've added a comment to document this.

> ::: docshell/base/nsDocShell.cpp
> @@ +5203,5 @@
> >  
> >    errorPageUrl.AppendASCII(escapedError.get());
> >    errorPageUrl.AppendLiteral("&u=");
> >    errorPageUrl.AppendASCII(escapedUrl.get());
> > +  if (strncmp(aErrorPage, "blocked", sizeof("blocked") - 1) == 0) {
> 
> Can't you use EqualsLiteral("blocked") or something? Would be much cleaner
> if you can use nsCString.

The problem is that aErrorPage is not an nsCString so I'm not sure that initializing an nsCString just to do that comparison is worth it.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8690637 [details] [diff] [review]
bug1226490.patch

>+  if (strncmp(aErrorPage, "blocked", sizeof("blocked") - 1) == 0) {
Don't know why strncmp, and why not simpler
strcmp(aErrorPage, "blocked") == 0 (strncmp wouldn't play well if one added some new error page which name happens to start with "blocked")

>+    if (Preferences::GetBool(PREF_SAFEBROWSING_ALLOWOVERRIDE, true)) {
and merge this 'if' with the previous one.
So
if (strcmp(...) == 0  &&
    Pref...) {
  errorPageUrl.AppendLiteral("&o=1"
}

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b45c40514f8

Comment 7

[François Marier [:francois]]

Attachment: bug1226490.patch

Rebased onto latest tip. Carrying r+ from smaug and gcp

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b45c40514f8