Frank-Rainer Grahl (:frg) Assignee
	Description • 9 years ago

Currently Safebrowsing is broken in Seamonkey because of api changes in the mozilla-central code.

This bug covers the changes to the source code.

Bug 920951 covers the changes to the preferences.

	Frank-Rainer Grahl (:frg) Assignee
	Comment 1 • 8 years ago

Do not use. Broke the buttons by copying a litte too much from FF. Strings should be ok and all itsatrap websites show as blocked.

	Frank-Rainer Grahl (:frg) Assignee
	Comment 2 • 8 years ago

Patch for safebrowsing. Tested on c-a only due to current c-c breakage.
VS2015 build.

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0 SeaMonkey/2.44a2

Tested urls:

http://itisatrap.org/firefox/unwanted.html
http://itisatrap.org/firefox/its-a-trap.html
http://itisatrap.org/firefox/its-an-attack.html

With browser.safebrowsing.forbiddenURIs.enabled set to true:
http://itisatrap.org/firefox/forbidden.html

Contains string changes copied from current m-c.

	Frank-Rainer Grahl (:frg) Assignee
	Comment 3 • 8 years ago

I forgot to include the css in the first patch.

	Philip Chee
	Comment 4 • 8 years ago

Comment on attachment 8729981 [details] [diff] [review]
1250600-safebrowsingsites-V2.patch

blockedSite.xhtml
=================

>      <div id="errorPageContainer">
> -
Keep empty line.

>        <div id="errorLongContent">
> -
Keep empty line.

> -          <!-- Commands handled in utilityOverlay.js -->
Keep comment.

blockedSite.js
==============

>  function getErrorCode()
>  {
>    var url = document.documentURI;
> -  var error = url.indexOf("e=");
> -  var duffUrl = url.indexOf("&u=");
> -  return url.slice(error + 2, duffUrl);
> +  var error = url.search(/e\=/);
> +  var duffUrl = url.search(/\&u\=/);
Nit: these are fixed strings, just use indexOf.

> +  return decodeURIComponent(url.slice(error + 2, duffUrl));
Nit: this is ASCII, no need to decode it.

> + function getOverride()
> + {
> +   var url = document.documentURI;
> +   var match = url.match(/&o=1&/);
> +   return !!match;
(1) I think you want /&o=1/ or /(\?|&)o=1/
(2) Use /regexp/.test(str) which returns a boolean which means that you also don't have to !!coerce the return value.

> +    case "malwareBlocked" :
No space before : (several instances)

> +  if (error !== "malware") {
> +    el = document.getElementById("errorTitleText_malware");
> +    el.parentNode.removeChild(el);
You can now use el.remove() (Supported since Firefox 23) See the part you removed :P  Also could use a helper function. e.g.

function deleteElement(element) {
  var el = document.getElementById(element);
  if (el)
    el.remove();
}

> +  if (error !== "malware") {
Both sides are strings. Just use != .

notification.xml
================
>        <method name="ignoreSafeBrowsingWarning">
> -        <parameter name="aIsMalware"/>
> +        <parameter name="aReasonBlocked"/>
Just "reason" or "aReason".

> +            if (reportName != null) {
if (reportName) {

> +                callback: function () { openUILinkIn(reportUrl, "tabfocused"); }
There is now a shorter ES6 syntax for method definitions on objects initializers:
callback() { ... }
See: https://developer.mozilla.org/Web/JavaScript/Reference/Functions/Method_definitions

utilityOverlay.js
=================
Please move the white-space only changes to a separate patch attached to this bug.

> +    let reasonBlocked = 'phishing';
"Double quotes" please.
Also just |reason| and not |reasonBlocked|

> +      case "whyForbiddenButton":
> +        // This is the "Why is this site blocked" button for family friendly browsing.
> +        // There's no desktop focused support page yet.
The Firefox comment makes more sense! Please use it.

> // This is the "Why is this site blocked" button for family friendly browsing
> // for Fennec. There's no desktop focused support page yet.

> +        loadURI("https://support.mozilla.org/kb/controlledaccess");
Don't hard code URLs in code (generally). Instead use formatURLPref() like a few lines up in your patch.
browser.safebrowsing.warning.infoURL

safeBrowsing.dtd
================
> +<!-- Localization note (safeb.palm.notdeceptive.label) - Label of the Help menu item. -->
> +<!ENTITY safeb.palm.notdeceptive.label "This isn't a deceptive site…">
Unused?

> +<!ENTITY reportDeceptiveSite.label        "Report deceptive site…">
> +<!ENTITY reportDeceptiveSite.accesskey    "D">
> +<!ENTITY notADeceptiveSite.label          "This isn't a deceptive site…">
> +<!ENTITY notADeceptiveSite.accesskey      "D">
Access keys should match the case in the label. (d)
Also please don't vertically align in this file. After your patch nothing else is.

	Frank-Rainer Grahl (:frg) Assignee
	Comment 5 • 8 years ago

All (hopefully) taken care of but:

>> blockedSite.js
>> ==============
>> (1) I think you want /&o=1/ or /(\?|&)o=1/

The check is for &o=1& so the expression was ok. Changed it to o=0 for a test and the button got removed so it works. The current blocked urls all have o=1 in them even the forbidden ones which I find inconsistent. 

>> utilityOverlay.js
>> =================
>> Please move the white-space only changes to a separate patch attached to this bug.

Could I ask for an exception here. Not that many and I did set the editor to remove them. If not ok I will just delete them in the patch.

I did some minor additional cleanups which I found when checking the patch. access keys were not always consistent. 

Test on c-a and patch build against current c-c.

	Philip Chee
	Comment 6 • 8 years ago

Comment on attachment 8735465 [details] [diff] [review]
1250600-safebrowsingsites-V3.patch

Great patch! r=me

>  pref("browser.safebrowsing.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/phishing-protection/");
> +pref("browser.safebrowsing.controlledaccess.infoURL", "https://support.mozilla.org/kb/controlledaccess/");
To be consistent it should be camel case: controlledAccess.

> +    // Reason isn't currently used but it may be useful in the future
> +    // for further testing and setting menu items.
> +    let reason;

Suggestion:
// "reason" isn't currently used but it's here to make porting
// from Firefox easier.

> +  if (!getOverride()) {
> +    var btn = document.getElementById("ignoreWarningButton");
> +    if (btn) {
> +      btn.parentNode.removeChild(btn);

if (!getOverride())
  deleteElement("ignoreWarningButton");


>>> (1) I think you want /&o=1/ or /(\?|&)o=1/
> 
> The check is for &o=1& so the expression was ok. Changed it to o=0 for a
> test and the button got removed so it works. The current blocked urls all
> have o=1 in them even the forbidden ones which I find inconsistent. 
OK. Makes sense.

>>> Please move the white-space only changes to a separate patch attached to this bug.
> 
> Could I ask for an exception here. Not that many and I did set the editor to
OK. I guess.

	Frank-Rainer Grahl (:frg) Assignee
	Comment 7 • 8 years ago

>> if (!getOverride())
>>  deleteElement("ignoreWarningButton");

Does this work without throwing an exception if the button is not there? For forbidden it already got deleted in the if a few lines earlier.

	Philip Chee
	Comment 8 • 8 years ago

(In reply to Frank-Rainer Grahl from comment #7)
> >> if (!getOverride())
> >>  deleteElement("ignoreWarningButton");
> 
> Does this work without throwing an exception if the button is not there? For
> forbidden it already got deleted in the if a few lines earlier.
There is an if() test in deleteElement(), so if the button isn't there it is a NOOP.

+function deleteElement(element) {
+  var el = document.getElementById(element);
+  if (el)
+    el.remove();

	Frank-Rainer Grahl (:frg) Assignee
	Comment 9 • 8 years ago

Latest patch incorporating some additional minor suggestions. Review+ from Philip Chee carried forward.

	Rainer Bielefeld
	Comment 10 • 8 years ago

Since what Version is a patch required?

	Frank-Rainer Grahl (:frg) Assignee
	Comment 11 • 8 years ago

This broke a long time ago. First the lists and then the api. I think between 2.26.1 and 2.35. Would need to check the FF bugs. 

I didn't see any complaints so I wouldn't port it back. Also in 2.44 strings where changed by Mozilla. Phishing was partially replaced by deceptive. Put a slightly modified version in my private c-r and c-b tree but even this has string changes.

FRG

	Rainer Bielefeld
	Comment 12 • 8 years ago

So this one can't be the reason for some 2.40 Safe Browsing problems like "Bug 1260457 - Seamonkey 2.40 does not warn that page contains elements meant to track, and is blocking; while Seamonkey 2.39 does warn of such"

	Frank-Rainer Grahl (:frg) Assignee
	Comment 13 • 8 years ago

Need to check this. This bug is/was primary for Safe Browsing. I didn't try tracking protection.

Might have something to do with the bug 920951 and the Mozilla tracking lists no longer being fetched. Maybe someone can try it out with a nightly. The lists bug is already in the tree.

FRG

	Philip Chee
	Comment 14 • 8 years ago

http://hg.mozilla.org/comm-central/rev/1759686baf84