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)
Toolkit
Safe Browsing
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)
22.48 KB,
image/png
|
Details | |
127.53 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
1.78 KB,
patch
|
francois
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.59 KB,
image/png
|
Details |
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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8895655 -
Flags: review?(francois)
Reporter | ||
Comment 8•7 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
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•7 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-
Assignee | ||
Comment 11•7 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) |
Assignee | ||
Comment 14•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895655 -
Flags: review?(francois)
Attachment #8896186 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8895655 -
Flags: review?(francois)
Reporter | ||
Updated•7 years ago
|
Attachment #8896198 -
Flags: review+
Reporter | ||
Comment 16•7 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•7 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•7 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•7 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•7 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?
Comment 23•7 years ago
|
||
bugherder |
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
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 24•7 years ago
|
||
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•7 years ago
|
Whiteboard: #sbv4-m9 → #sbv4-m10
Comment 25•7 years ago
|
||
(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 26•7 years ago
|
||
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+
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5c4087baab44
Comment 29•7 years ago
|
||
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•7 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)
Comment 31•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•