Closed Bug 1388494 Opened 7 years ago Closed 7 years ago

Undefined string for PHA threat type and broken advisory text

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 --- verified

People

(Reporter: francois, Assigned: hchang)

References

Details

(Keywords: regression, Whiteboard: #sbv4-m10)

Attachments

(6 files)

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.
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"
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
BTW, there requires similar works to be done for mobile/android. Should I do
them here in this bug or in bug 1388501?
(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
(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 :)
(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!
Attachment #8895655 - Flags: review?(francois)
(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
Keywords: regression
Summary: Undefined string and broken advisory text filtering for PHA threat type → Undefined string for PHA threat type and broken advisory text
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.
(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)
Attachment #8895655 - Flags: review?(francois)
Attachment #8896186 - Flags: review?(francois)
Attachment #8895655 - Flags: review?(francois)
Attachment #8896198 - Flags: 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+
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+
(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)
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
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?
Blocks: 1388501
https://hg.mozilla.org/mozilla-central/rev/653bbf8bc36e
https://hg.mozilla.org/mozilla-central/rev/eb60d0337b16
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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)
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.
Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)
Attached image 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)
(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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.