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 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.

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

(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" }

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