Undefined string for PHA threat type and broken advisory text

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
Safe Browsing
P1
normal
VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: francois, Assigned: hchang)

Tracking

({regression})

unspecified
mozilla57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56+ verified, firefox57 verified)

Details

(Whiteboard: #sbv4-m10)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

4 months ago
Created attachment 8895048 [details]
Advisory text not filtered properly

Steps:

1. Navigate to https://itisatrap.org/firefox/harmful.html
2. See that the Advisory text is not filtered properly.

Expected:

No advisory text should be shown since the provider is empty.

Actual:

"Advisory provided by" is shown.
(Reporter)

Comment 1

4 months ago
Created attachment 8895051 [details]
harmful-warning-undefined-string.png

Here are the steps for the second problem:

1. Navigate to https://itisatrap.org/firefox/harmful.html
2. Click "Ignore this warning"
3. See that there is an undefined string in the notification bar at the top.

Expected:

The notification bar says "Potentially Harmful Site!".

Actual:

The notification bar says "undefined"
Comment hidden (mozreview-request)
(Assignee)

Comment 3

4 months ago
The bug causes and solution are in the commit message:

1) About 'advisory text': The advisory text is supposed to be removed if no
   provider. However, there is an exception right before the call to remove
   the empty advisory text. We fixed that exception in 'blockedSite.xhtml'
   so that the filtering can be properly done.

2) 'undefined' string: Properly initialize 'title' in browser.js.

What I am uncertain is whether we should or how to report harmful sites.
Should we repeat [1]?

[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/base/content/browser.js#3209
(Assignee)

Comment 4

4 months ago
BTW, there requires similar works to be done for mobile/android. Should I do
them here in this bug or in bug 1388501?
(Reporter)

Comment 5

4 months ago
(In reply to Henry Chang [:hchang] from comment #3)
> What I am uncertain is whether we should or how to report harmful sites.
> Should we repeat [1]?

No report URL. It's just like the unwanted list:

http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/base/content/browser.js#3239-3240
(Reporter)

Comment 6

4 months ago
(In reply to Henry Chang [:hchang] from comment #4)
> BTW, there requires similar works to be done for mobile/android. Should I do
> them here in this bug or in bug 1388501?

Up to you, it doesn't really matter as long as it works in the end :)
(Assignee)

Comment 7

4 months ago
(In reply to François Marier [:francois] from comment #6)
> (In reply to Henry Chang [:hchang] from comment #4)
> > BTW, there requires similar works to be done for mobile/android. Should I do
> > them here in this bug or in bug 1388501?
> 
> Up to you, it doesn't really matter as long as it works in the end :)

okay I will do them in bug 1388501. Thanks!
(Assignee)

Updated

4 months ago
Attachment #8895655 - Flags: review?(francois)
(Reporter)

Comment 8

4 months ago
(In reply to Henry Chang [:hchang] from comment #3)
> 1) About 'advisory text': The advisory text is supposed to be removed if no
>    provider. However, there is an exception right before the call to remove
>    the empty advisory text. We fixed that exception in 'blockedSite.xhtml'
>    so that the filtering can be properly done.

Sorry I missed this yesterday. Looking at your patch, this is a regression that was introduced in bug 1366384 and that we'll have to uplift to 56.

Can you please split the patch into two so that we can uplift just that part?

[Tracking Requested - why for this release]: There is a small regression in the UI that's shown on non-Google Safe Browsing pages. See "Advisory provided by [blank]" in https://bug1388494.bmoattachments.org/attachment.cgi?id=8895048. The patch is trivial.
Blocks: 1366384
status-firefox56: --- → affected
status-firefox57: --- → affected
tracking-firefox56: --- → ?
Keywords: regression
Summary: Undefined string and broken advisory text filtering for PHA threat type → Undefined string for PHA threat type and broken advisory text
(Reporter)

Comment 9

4 months ago
mozreview-review
Comment on attachment 8895655 [details]
Bug 1388494 - Fix incomplete advisory info issue in about:blocked page.

https://reviewboard.mozilla.org/r/166926/#review172426

This looks good, but as per comment 8, we need to split it into two patches so that we can uplift part of it.

::: browser/base/content/blockedSite.xhtml:138
(Diff revision 1)
>            el = document.getElementById("errorLongDescText_harmful");
>            el.remove();
>          }
>  
>          // Set sitename
> -        document.getElementById(error + "_sitename").textContent = getHostString();
> +        let sitenameEle = document.getElementById(error + "_sitename");

nit: I would add an "m" and use `sitenameElem` since that's the usual way to abbreviate English words

::: browser/base/content/browser.js:3239
(Diff revision 1)
>      } else if (reason === "unwanted") {
>        title = gNavigatorBundle.getString("safebrowsing.reportedUnwantedSite");
>        // There is no button for reporting errors since Google doesn't currently
>        // provide a URL endpoint for these reports.
> +    } else if (reason === "harmful") {
> +      title = gNavigatorBundle.getString("safebrowsing.reportedHarmfulSite");

Maybe we should copy/paste the comment from unwanted  ("There is no button for reporting errors since Google...")
Attachment #8895655 - Flags: review?(francois) → review-
Track 56+ as new regression in 56.
tracking-firefox56: ? → +
(Assignee)

Comment 11

4 months ago
(In reply to François Marier [:francois] from comment #8)
> (In reply to Henry Chang [:hchang] from comment #3)
> > 1) About 'advisory text': The advisory text is supposed to be removed if no
> >    provider. However, there is an exception right before the call to remove
> >    the empty advisory text. We fixed that exception in 'blockedSite.xhtml'
> >    so that the filtering can be properly done.
> 
> Sorry I missed this yesterday. Looking at your patch, this is a regression
> that was introduced in bug 1366384 and that we'll have to uplift to 56.
> 
> Can you please split the patch into two so that we can uplift just that part?
> 
> [Tracking Requested - why for this release]: There is a small regression in
> the UI that's shown on non-Google Safe Browsing pages. See "Advisory
> provided by [blank]" in
> https://bug1388494.bmoattachments.org/attachment.cgi?id=8895048. The patch
> is trivial.

As far as I can tell, it's not a regression until <span id='[reason]_sitename'> 
is unexpectedly missing in the document. 

I checked desktop/fennec dtd files [1][2] and it appears all of them contain
expected <span...>. As a result, this regression will not be seen in 56 
because the content is totally managed by us.

That said, I can't guarantee I checked all possible path so it's still worth
an uplift. What do you think?

[1] http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd
[2] http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/mobile/android/locales/en-US/chrome/phishing.dtd#23
Flags: needinfo?(francois)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
Created attachment 8896198 [details] [diff] [review]
bug1388494_for_uplift_to_56.patch
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8895655 - Flags: review?(francois)
Attachment #8896186 - Flags: review?(francois)
(Assignee)

Updated

4 months ago
Attachment #8895655 - Flags: review?(francois)
(Reporter)

Updated

4 months ago
Attachment #8896198 - Flags: review+
(Reporter)

Comment 16

4 months ago
mozreview-review
Comment on attachment 8896186 [details]
Bug 1388494 - Fix 'undefined' text issue after you clicked "Ignore this warning".

https://reviewboard.mozilla.org/r/167460/#review174172
Attachment #8896186 - Flags: review?(francois) → review+
(Reporter)

Comment 17

4 months ago
mozreview-review
Comment on attachment 8895655 [details]
Bug 1388494 - Fix incomplete advisory info issue in about:blocked page.

https://reviewboard.mozilla.org/r/166926/#review174174

::: commit-message-1d042:3
(Diff revision 2)
> +Bug 1388494 - Fix incomplete advisory info issue in about:blocked page.
> +
> +This is a regression caused by bug 1366384 and needed to be uplifted to 56.

I think we can drop the entire commit message. All of this information is captured in the bug and isn't  necessary for anybody doing code archeology.

And the uplift part will be done against a different patch anyways.
Attachment #8895655 - Flags: review?(francois) → review+
(Reporter)

Comment 18

4 months ago
(In reply to Henry Chang [:hchang] from comment #11)
> As far as I can tell, it's not a regression until <span
> id='[reason]_sitename'> 
> is unexpectedly missing in the document. 
> 
> I checked desktop/fennec dtd files [1][2] and it appears all of them contain
> expected <span...>. As a result, this regression will not be seen in 56 
> because the content is totally managed by us.
> 
> That said, I can't guarantee I checked all possible path so it's still worth
> an uplift. What do you think?

Given it's a really small and low-risk patch without any new strings, I think we may as well request an uplift.
Flags: needinfo?(francois)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

4 months ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/653bbf8bc36e
Fix incomplete advisory info issue in about:blocked page. r=francois
https://hg.mozilla.org/integration/autoland/rev/eb60d0337b16
Fix 'undefined' text issue after you clicked "Ignore this warning". r=francois
(Assignee)

Comment 22

4 months ago
Comment on attachment 8896198 [details] [diff] [review]
bug1388494_for_uplift_to_56.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1366384
[User impact if declined]: Warning page may show incomplete advisory info
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: The patch will land shortly
[Needs manual test from QE? If yes, steps to reproduce]: 
Visit http://itisatrap.org/firefox/unwanted.html and see if incomplete advisory info shows.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: no
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8896198 - Flags: approval-mozilla-beta?
(Assignee)

Updated

4 months ago
Blocks: 1388501

Comment 23

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/653bbf8bc36e
https://hg.mozilla.org/mozilla-central/rev/eb60d0337b16
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
(Assignee)

Updated

4 months ago
No longer blocks: 1388501
Hi Florin, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
(Reporter)

Updated

4 months ago
Whiteboard: #sbv4-m9 → #sbv4-m10
(In reply to Gerry Chang [:gchang] from comment #24)
> Hi Florin, could you help find someone to verify if this issue was fixed as
> expected on the latest Nightly build? Thanks!

Added to our todo list.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Comment on attachment 8896198 [details] [diff] [review]
bug1388494_for_uplift_to_56.patch

Fix a regression about the advisory info. Beta56+.
Attachment #8896198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I managed to reproduce the described issues (in comment 0 and comment 1) using Firefox 57.0a1 (Build Id:20170808114032).

This issue is verified fixed on Firefox 57.0a1 (BuildId:20170820100343) using Windows 10 64 bit, Ubuntu 16.04 64bit and macOS 10.11.6.

Leaving a needinfo on myself and the qe-verify+ flag until this gets fixed in Fx56 as well.
status-firefox57: fixed → verified
Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)

Comment 28

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5c4087baab44
status-firefox56: affected → fixed
Created attachment 8899828 [details]
iframe Warning

Hi Francois !

While verifying with Firefox 56.0b5 if the warnings from the "Webpage Warnings" section (found in http://testsafebrowsing.appspot.com/) are properly displayed, I noticed that the last warning, which shows a Malware website inside an iframe, is displaying an incomplete Advisory text.

Any thoughts on this?

Thanks!
Flags: needinfo?(emil.ghitta) → needinfo?(francois)
(Reporter)

Comment 30

4 months ago
(In reply to Emil Ghitta, QA [:emilghitta] from comment #29)
> While verifying with Firefox 56.0b5 if the warnings from the "Webpage
> Warnings" section (found in http://testsafebrowsing.appspot.com/) are
> properly displayed, I noticed that the last warning, which shows a Malware
> website inside an iframe, is displaying an incomplete Advisory text.
> 
> Any thoughts on this?

That's a real bug (see bug 1195242), but not a blocker for this one.
Flags: needinfo?(francois)
Thank you Francois !

The warnings from the "Webpage Warnings" section (found in https://testsafebrowsing.appspot.com/) are properly displayed. Therefore, this issue is verified fixed on Firefox 56.0b5 using Windows 10 64 bit, Ubuntu 16.04 64 bit and macOS 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.