Allow administrators to prevent users from ignoring Safe Browsing warnings

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: francois, Assigned: francois)

Tracking

(Depends on 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Posted patch bug1226490.patch (obsolete) — Splinter Review
Attachment #8690363 - Flags: review?(gpascutto)
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.
Attachment #8690363 - Flags: review?(gpascutto) → review+
(Assignee)

Comment 3

3 years ago
Posted patch bug1226490.patch (obsolete) — Splinter Review
Olli, this patch is related to the one you've just looked at (bug 1216723) and also includes a small docshell change.
Attachment #8690363 - Attachment is obsolete: true
Attachment #8690637 - Flags: review?(bugs)
(Assignee)

Comment 4

3 years ago
(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 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"
}
Attachment #8690637 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

3 years ago
Rebased onto latest tip. Carrying r+ from smaug and gcp
Attachment #8690637 - Attachment is obsolete: true
Attachment #8691017 - Flags: review+

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b45c40514f8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

3 years ago
Depends on: 1228698
(Assignee)

Updated

3 years ago
Depends on: 1239836
You need to log in before you can comment on or make changes to this bug.