1408779 - link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 58.0a1 latest and 57.0b8 on Windows 8.1

The urls to the report why a page has been blocked by Safe Browsing contain variables which should be replaced by their values, e.g.
https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=http://testsafebrowsing.appspot.com/s/phishing.html

Steps to reproduce:
1. Open http://testsafebrowsing.appspot.com/
2. Click on the first link: "Should show a phishing warning"
3. On the loaded page informing about the blocking, open the Details.
4. Move your mouse to the link "reported as a deceptive site".
Actual result: Link shown with variables.
Expected result: Variables replaced with values.

|Services.urlFormatter.formatURLPref(<prefname>)| should be used for this - or is the current status intentional (the site doesn't seem to offer localized versions)?

Comment 1

[Thomas Nguyen (:tnguyen)]

pref("browser.safebrowsing.provider.google.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
pref("browser.safebrowsing.provider.google.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
pref("browser.safebrowsing.provider.google.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
pref("browser.safebrowsing.provider.google4.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
pref("browser.safebrowsing.provider.google4.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
pref("browser.safebrowsing.provider.google4.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?

pref("browser.safebrowsing.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url=");

FWIW, from client view, the reportURL should use LOCALE, and we used |Services.urlFormatter.formatURLPref(<prefname>)|

Comment 2

[François Marier [:francois]]

Given that these query string parameters are not used, we should simply remove them.

Comment 3

[Manish [:manishkk]]

As Expected result say: 'Variables replaced with values.'
Do I need to replace query string with some values?

(In reply to François Marier [:francois] from comment #2)

Given that these query string parameters are not used, we should simply
remove them.

Comment 4

[Dimi Lee [:dimi]]

(In reply to Manish [:manishkk][Less Active until 24 Feb] from comment #3)

As Expected result say: 'Variables replaced with values.'
Do I need to replace query string with some values?

Hi Manish,
Sorry for the super late reply.
No, we just remove them.

Comment 5

[Manish [:manishkk]]

Attachment: Bug 1408779- link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced

Comment 6

[aarushivij]

Hello, Can I work on this issue?
I read the last patch attached.
So I have to remove query string hl?
Thanks :)
Aarushi

Comment 7

[Johann Hofmann [:johannh]]

Hey, I think you can work on this. It seems like that's what you need to do, but please tag dimi for review and he'll know for sure.

Comment 8

[aarushivij]

Okay , Thanks Johann
Shall do the same :)

Comment 9

[aarushivij]

Attachment: Bug 1408779 - link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1332b0d2dcba
link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced   r=dimi

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1332b0d2dcba

Comment 12

[Cristian Baica [:cbaica], Release Desktop QA]

Attachment: new_bug

I have managed to reproduce the issue using Fx58.0a1 buildID: 20170922220129.
The issue is NOT entirely fixed. When the mouse cursor hovers over the reported as a deceptive site link, it can be noticed that the %NAME% variable is still displayed (only the %LOCALE% variable was removed).

Comment 13

[Dimi Lee [:dimi]]

(In reply to Cristian Baica [:cbaica], Release Desktop QA from comment #12)

I have managed to reproduce the issue using Fx58.0a1 buildID: 20170922220129.
The issue is NOT entirely fixed. When the mouse cursor hovers over the reported as a deceptive site link, it can be noticed that the %NAME% variable is still displayed (only the %LOCALE% variable was removed).

Thanks for helping verify this.
The client=%NAME% should also be removed from the pref.
reopen this issue

Comment 14

[Dimi Lee [:dimi]]

Hi, let me know if you still want to work on this one? thanks!

Comment 15

[aarushivij]

Okay, will submit the patch for the same :)
Thanks
Aarushi

Comment 16

[aarushivij]

Attachment: Bug 1408779 - link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced r=dimi

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f43ff29d1ba1
link to report why page has been classified as malicious/deceptive/... should have variables %NAME% and %LOCALE% replaced r=dimi

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f43ff29d1ba1