441733 - Display a warning bar while browsing an unsafe site after dismissing the "site blocked" page

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect, P4)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

From bug 358299 comment 10:

possibly adding a warning bar
to the top of the content area while browsing a site after deciding to ignore
the warning/override the blocked.

Comment 1

[Chris Lawson (gone)]

Agreed in today's meeting.

<smorgan> I just don't want the bar to go away while they are still on the bad site, just a different page
<murph> well, most of the blocking is actually done by Core during the load, but we can find a way to keep track of the blocked site
<smorgan> Sounds like we should confirm it then

Assigning to murph for now, but if someone else wants to step up and take this, I'm sure he won't mind.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Low priority, though.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is one of those "if we have time" bugs; I don't think we'll block, but nominating just to keep track.

Comment 4

[Samuel Sidler (old account; do not CC)]

Might not be as low priority anymore, we should probably investigate this.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> Might not be as low priority anymore, we should probably investigate this.
Specifically, see Ian Fette's comments in bug 437490 comment 6.

Comment 6

[Sean Murphy]

Attachment: Patch

Patch to display a safe browsing bar when the blocked error overlay is ignored.

The browser keeps track of sites that were previously blocked and anytime they're visited without an error overlay present, the bar is displayed.  This includes going back in browser history to a previously ignored page (which would not trigger Core to re-check the page and display the main overlay).

In Firefox, I noticed a problem with the following scenario: If you ignore a safe browsing error, go ahead and load the suspected site, navigate elsewhere to normal sites, and then go back in session history to the dangerous site.  Going back to the dangerous site will not re-display the safe browsing bar (even if it was never closed).  This patch ensures we always present the bar, so the user always has the reminder that a site was suspected dangerous.

The code here also takes care of reporting incorrectly suspected sites.  Consistent with the approach Google prefers, a button is present on the bar to handle reporting the error.

Screenshot and nib to follow. Everyone is of course welcome to weigh in on the appearance of the bar, and we can also discuss the phrasing as well (maybe a more detailed title on the bar, etc).

Comment 7

[Sean Murphy]

Attachment: Screenshot

Comment 8

[Sean Murphy]

Attachment: SafeBrowsingBar.nib

Note: I didn't have access to IB 2.x right now, so attached is a 2.x nib created with IB 3.x.

I thought I saw elsewhere that we can just have someone open up the nib in an actual copy of 2.x and re-save it, to fix the issues that arise from us using 3.x to make nibs.  Hopefully that's the case.  Sorry!

Comment 9

[Sean Murphy]

Attachment: Patch

Sorry, the first patch was actually out-of-date.  (I changed the nib name after generating it).

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Pink?

Comment 11

[Stuart Morgan]

Comment on attachment 372691 [details] [diff] [review]
Patch

Awesome! Looks like the transient bar restructuring really paid off here.


>+"ReportBlockingError" = "Report an errorâ¦";

Where is this used? I didn't see it anywhere.

>+- (IBAction)runAwayFromBlockedSite:(id)sender;

Nice :)


>+    // If the blocked error overlay is not being shown, the user has chosen to
>+    // ignore the warning and proceed to the blocked site.  In this situation, 
>+    // we display a safe browsing transient bar.

This needs rewording; hopefully it's not the case that every navigation users ever do is to a blocked site ;)

>+  // We keep track of blocked sites for cases when the user ignores the main error
>+  // overlay and proceeds to the site. After ignoring a blocked error for a page 
>+  // we will always present a bar on it, especially when it was loaded unblocked
>+  // from session history.

Although speaking of that... what happens if there is another "the world is phishing" incident, or just a small mistake where a legit but high-profile site gets mistakenly inserted for a short time? This would mean users would have to quit to fix it; maybe we should store the time as well, and consider entries invalid after some period of time (10 minutes? 15? How often do we update the feed from Google?).

>+  if (aRequest)  // aRequest can be null (e.g. for relative anchors)
>+  {

Go ahead and fix this brace while you are moving the block.

>+- (void)onSafeBrowsingBlockedURI:(NSString*)aBlockedURI reason:(ESafeBrowsingBlockedReason)aBlockedReason;

Split the long line please.

>+- (void)onSafeBrowsingBlockedURI:(NSString*)aBlockedURI reason:(ESafeBrowsingBlockedReason)aBlockedReason

(Here too)

>+extern NSString* const kGeckoPrefSafeBrowsingDataProviderReportPhishingURL;      // string
>+extern NSString* const kGeckoPrefSafeBrowsingDataProviderReportMalwareURL;       // string

What are these two for? Are we going to be adding UI for reporting sites?

>+  NSColor* startColor = [NSColor colorWithCalibratedRed:0.749020f green:0.356863f blue:0.376471f alpha:1.0f];
>+  NSColor* endColor = [NSColor colorWithCalibratedRed:0.415686f green:0.117647f blue:0.129412f alpha:1.0f];

I had the same reaction as Smokey... is this matching the overlay? If not, I'd vote for more red and less pink.

>+  [backgroundGradient drawInRect:[self bounds] angle:270.0];  

Trailing whitespace.

>+- (NSString *)listReportingURLForPrefKey:(NSString *)prefKeyTemplate urlToReport:(NSString *)urlToReport

Break line

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #11)
> I had the same reaction as Smokey... is this matching the overlay? If not, I'd
> vote for more red and less pink.

(And Sam.)  I think we want to change the red on the overlay to better match the red in the site icon/future-overlay-image-badge (bug 479554), so we'll want this bar to better match that, too.

Right now the overlay red is sort of dull, greyish, and Windows-y, and we want a brighter red.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #8)
> Created an attachment (id=372687) [details]
> SafeBrowsingBar.nib

Also, how hard would it be to go ahead and fix bug 483140 for this nib before we even land it?

Comment 14

[Sean Murphy]

Attachment: Patch, v2

I'm real sorry guys for taking so long to update this patch.  I had a few things here keeping me busy in the meantime, projects and otherwise.  Nonetheless, I'll make sure it gets in asap now!

(In reply to comment #11)
> (From update of attachment 372691 [details] [diff] [review])
> Awesome! Looks like the transient bar restructuring really paid off here.
> 
> 
> >+"ReportBlockingError" = "Report an errorâ¦";
> 
> Where is this used? I didn't see it anywhere.

You're right, this text is now set in the nib so this line is not needed.

> Although speaking of that... what happens if there is another "the world is
> phishing" incident, or just a small mistake where a legit but high-profile site
> gets mistakenly inserted for a short time? This would mean users would have to
> quit to fix it; maybe we should store the time as well, and consider entries
> invalid after some period of time (10 minutes? 15? How often do we update the
> feed from Google?).

I changed the code to expire cached blocks after kTimeIntervalToConsiderSiteBlockingStatusValid (15 minutes for now).
 
> >+extern NSString* const kGeckoPrefSafeBrowsingDataProviderReportPhishingURL;      // string
> >+extern NSString* const kGeckoPrefSafeBrowsingDataProviderReportMalwareURL;       // string
> 
> What are these two for? Are we going to be adding UI for reporting sites?

Yes, I just threw them in now, in advance...

> >+  NSColor* startColor = [NSColor colorWithCalibratedRed:0.749020f green:0.356863f blue:0.376471f alpha:1.0f];
> >+  NSColor* endColor = [NSColor colorWithCalibratedRed:0.415686f green:0.117647f blue:0.129412f alpha:1.0f];
> 
> I had the same reaction as Smokey... is this matching the overlay? If not, I'd
> vote for more red and less pink.

Yeah, I actually agree with you guys on that.  The new colors are a little darker than the blocked-icon, because the icon would not stand out enough otherwise.  I do prefer this darker color.  I also added a small shaded line on the bottom.

-

Also, to explain why I removed much of Hendy's malware diagnostic info patch:

> -const char* const kGeckoPrefSafeBrowsingMalwareDiagnosticURL = "browser.safebrowsing.malware.reportURL";

As we do it now, when we show more information about why a malware site was blocked we actually fill in and forward to the malware reporting URL, just as this patch does when we actually do want to report a site.  Furthermore, the existing malware diagnostic URL was a global preference, whereas since it pertains to Google specifically it should actually be associated with the data provider, containing the provider ID in it.  I just chose to keep all the pref fetching and token replacement in the list manager, and have the -showMalwareDiagnosticInformation method just use this to prevent code duplication.

If we in the future want to show our own branded site to explain malware blocking, instead of just showing Google's diagnostic page, we can add a malware pref to compliment browser.safebrowsing.warning.infoURL.

Comment 15

[Stuart Morgan]

Comment on attachment 383304 [details] [diff] [review]
Patch, v2

r=me

Minor nit:
>+  IBOutlet SafeBrowsingBar* mSafeBrowsingBar; // loaded on demand, can be nil, strong
>+  IBOutlet NSTextField*     mSafeBrowsingBarLabel;
>+  NSMutableDictionary*      mBlockedSitesAndInfo; // strong
...
>+  if (aRequest) { // aRequest can be null (e.g. for relative anchors)

Two spaces before a same-line comment.

(The red is way better now, but it felt a tad dark to me. The exact shade certainly isn't the most critical part of this patch though ;) Just mentioning in case others feel the same way and we want to tweak it later, not because I think it should have any impact on us getting this into the tree.)

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #13)
> (In reply to comment #8)
> > Created an attachment (id=372687) [details] [details]
> > SafeBrowsingBar.nib
> 
> Also, how hard would it be to go ahead and fix bug 483140 for this nib before
> we even land it?

Please fix this before this patch+nib lands, since we've already fixed bug 483140 and are therefore liable to forget about it in this nib otherwise.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #16)
> Please fix this before this patch+nib lands, since we've already fixed bug
> 483140 and are therefore liable to forget about it in this nib otherwise.

By which I mean we should have some placeholder text in that text field (since the string appears to come from, uh, somewhere else) with the text color set to black, and then the code sets the text color back to white when it inserts the string.

One other thing I noticed about this patch is that now the "Why is this site blocked" button for malware pages opens a new tab but the button for phishing does not, whereas with hendy's code both reused the existing tab.  They should both do the same thing (and, given that Google's diagnostic page includes a "go back" link, my guess is both should reuse the existing tab).

Final item: why are we now hard-coding Camino in those report/diagnostic URLs instead of using {moz:client} like we do elsewhere?

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 383304 [details] [diff] [review]
Patch, v2

    mLoadingResources = [[NSMutableSet alloc] init];
 
     mDetectedSearchPlugins = [[NSMutableArray alloc] initWithCapacity:1];
 
+    mBlockedSitesAndInfo = [[NSMutableDictionary alloc] init];
+

what's with all the whitespace in this init? eegk.

+- (ESafeBrowsingBlockedReason)reasonForBlockingURL:(NSString*)aURL

these new methods all need comments.

+@interface SafeBrowsingBar : TransientBar {

needs a class-level comment.

clean these up and sr=pink.

Comment 19

[Sean Murphy]

Attachment: Patch, for Check In

Updated patch with pink's comments addressed (comment 18) as well as everything Smokey noticed in comment 17.

(In reply to comment #18)
> (From update of attachment 383304 [details] [diff] [review])
>     mLoadingResources = [[NSMutableSet alloc] init];
> 
>      mDetectedSearchPlugins = [[NSMutableArray alloc] initWithCapacity:1];
> 
> +    mBlockedSitesAndInfo = [[NSMutableDictionary alloc] init];
> +
> 
> what's with all the whitespace in this init? eegk.

Hehe, I'm not sure!  I was hesitant to touch the other lines under this bug, so I at least made my new line not feature as much space around it.

Comment 20

[Sean Murphy]

Attachment: SafeBrowsingBlocked.nib (Visible Text Placeholder)

Warning placeholder text is now visible when editing the nib.  Requesting review from Smokey for sanity checking.

Also, this was created with IB 3.x.  As the nib is loaded on demand, I figure'd this is alright?

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 387022 [details]
SafeBrowsingBlocked.nib (Visible Text Placeholder)

Nib looks good; r=ardissone

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk; thanks, Sean!

We need to figure out what we want to do with the URLs for all the buttons; that can go in bug 437488.

We should also fix the inability to tab into the blocker bar (or past the blocker bar into content) if at all possible; that's now bug 502697.