1453448 - Capture thumbnails with safe browsing always enabled

Status: NEW

(Firefox :: New Tab Page, enhancement, P3)

Description

[Ed Lee :Mardak]

From bug 1449294 comment 7:

> You should also add `nsIChannel::LOAD_CLASSIFY_URI` to that list so that Activity Stream thumbnails are automatically run through Safe Browsing. This will avoid taking screenshots of phishing or malware sites.

https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#43-46

Comment 1

[Daniel Veditz [:dveditz]]

It's probably not all that likely we'll be using a thumbnail of a phishing site, but if we did then the thumbnail might add to the risk of phishing if users clicked on it because of how it looked rather than reading the origin. Then again, if the site was on the phishing list when we took the thumbnail it probably still is, and the user will be warned when they arrive. Adding "sec-want" as an easy "better safe than sorry" measure.

Comment 2

[premk]

Should I just add Ci.nsIChannel.LOAD_CLASSIFY_URI to the list?

I couldn't find a reference anywhere in the code-base to this flag.

Closest was this

https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIChannel.h#81-92

But it only has a flag to bypass the classifier.

Comment 3

[Ed Lee :Mardak]

Looks like it got removed in bug 1522412. I believe the behavior is that by default the requests run through the classifier now ? So this bug should be just resolved fixed ?

Comment 4

[Dimi Lee [:dimi]]

(In reply to Ed Lee :Mardak from comment #3)

Looks like it got removed in bug 1522412. I believe the behavior is that by default the requests run through the classifier now ? So this bug should be just resolved fixed ?

Yes, the flag is no longer needed. But it doesn't mean all the requests run through the classifier, we have an algorithm to decide this[1].
Before resolving the bug, I think we should verify if the algorithm works correctly in this scenario.

Hi Ed, do you have time to help verify this? If not, can you let me know the reproduce step and I'll check it. Thanks!

[1] https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/netwerk/base/nsNetUtil.cpp#3096

Comment 5

[Shrey Gupta]

Hello, I would like to work on this issue. Could you please explain what has to be done exactly as it is not very clear.
Thanks

Comment 6

[Ivan]

Hi guys,i see this file is now backgroundPageThumbs.xhtml instead of backgroundPageThumbs.js.

Could you please clarify so I can take this over?

Comment 7

[seal]

Hi! I'd like to work on this bug. Is there any other action I need to take?

Comment 8

[Harshit Sohaney [:hsohaney]]

Is this bug still looking to be worked on? I want to include it on my University's good-first-bug list!