Undefined string for PHA threat type and broken advisory text

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: francois, Assigned: hchang)

Tracking

({regression})

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: #sbv4-m10)

Attachments

(6 attachments)

Reporter

Description

2 years ago
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

2 years ago
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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8895655 - Flags: review?(francois)
Reporter

Comment 8

2 years 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
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

2 years 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.
Assignee

Comment 11

2 years 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)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8895655 - Flags: review?(francois)
Attachment #8896186 - Flags: review?(francois)
Assignee

Updated

2 years ago
Attachment #8895655 - Flags: review?(francois)
Reporter

Updated

2 years ago
Attachment #8896198 - Flags: review+
Reporter

Comment 16

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Blocks: 1388501

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/653bbf8bc36e
https://hg.mozilla.org/mozilla-central/rev/eb60d0337b16
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Updated

2 years 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

2 years 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.
Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)
Posted 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)
Reporter

Comment 30

2 years 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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.