504980 - Blocker bar can fail to appear after dismissing safebrowsing overlay if too much time has passed since displaying overlay

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

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

STR:

1) Visit http://caminobrowser.org/documentation/security/test-phishing/ or http://caminobrowser.org/documentation/security/test-malware/ or http://www.mozilla.com/firefox/its-a-trap.html or http://www.mozilla.com/firefox/its-an-attack.html
2) Click on "Ignore this warning"
3) Look for the blocker bar

ER: Blocker bar present
AR: Blocker bar absent

Firefox shows its bar in this case, and we should, too, to accustom our users to the UI/UE expected.

Comment 1

[Christopher Henderson]

WFM with 2.0b4pre (1.9.0.12pre 2009071216).

Comment 2

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

This is working for me now, but it absolutely was not working last night.  Any idea what could possibly cause a condition where our test sites were blocked but the blocker bar wasn't getting the message?

Comment 3

[Sean Murphy]

(In reply to comment #2)
> This is working for me now, but it absolutely was not working last night.  Any
> idea what could possibly cause a condition where our test sites were blocked
> but the blocker bar wasn't getting the message?

Hmmm, this does work for me too and I've never happened to see it not working.  I use our test sites all the time for debugging, as it's not always convenient  to find an actual phishing site to test with...

The code that controls the blocker bar receives notification whenever a site has been blocked.  The test sites should definitely be treated the same way any other blocked site would be.

The only case I can think of is that you ignored the warning more than 15 minutes after the site was first visited and blocked.  The browser will cache all of the blocks and remember them (this is what allows us to display the bar even when it was not reached via clicking "ignore warning," such as navigating back in history).  Cached blocks will expire after a certain time, so maybe just by chance it expired after it was initially blocked?  (Note that safe browsing will always block a nasty site even after it expires from our local cache; the cache is just for tracking when the bar should be shown on ignored sites).

Comment 4

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

(In reply to comment #3)
> The only case I can think of is that you ignored the warning more than 15
> minutes after the site was first visited and blocked.  The browser will cache

That was likely the case; I have those sites in tabs (along with others for testing) in that profile, and when I went to start looking at things in the bar, I'd certainly been running for quite a while.

I take it there's nothing we can do about this (admittedly edge) case?

Comment 5

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

Sean said a while ago he would try to take a look at this at some point; fixing the summary to better reflect the actual issue.

Comment 6

[Sean Murphy]

Attachment: Patch

Fixes the issue and also improves the code which determines when to show the bar.

Rather than remembering all blocked sites, our cache will only remember blocked sites for which the error warning was ignored.  This makes the purpose of tracking blocked sites clearer, in that it is not done for actual blocking but instead just for displaying the bar on ignored sites.

Comment 7

[Sean Murphy]

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

cl, do you have time for a review?  Trying to save Stuart for a sr instead.

Comment 8

[Sean Murphy]

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

Actually, targeting kreeger since he's up for some reviews.  Thanks Nick.

Comment 9

[Nick Kreeger]

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

 // if the command was sent from an error page overlay.
 - (void)performCommandForXULElementWithID:(NSString*)elementIdentifier onPage:(NSString*)pageURI
 {
   if ([elementIdentifier isEqualToString:@"exceptionDialogButton"]) {
     [mDelegate addCertificateOverrideForSite:[self currentURI]];
   }
   else if ([pageURI hasPrefix:@"about:safebrowsingblocked"]) {
+    // pageURI contains an |e| parameter to indicate the type of
+    // blocking error, such as e=malwareBlocked.
+    ESafeBrowsingBlockedReason blockedReason = eSafeBrowsingBlockedAsPhishing;
+    if ([pageURI rangeOfString:@"e=malwareBlocked"].location != NSNotFound)
+      blockedReason = eSafeBrowsingBlockedAsMalware;
+

Might be beneficial to make "e=malwareBlocked" a static string somewhere.

Looks fine to me, r+.

Comment 10

[Sean Murphy]

Attachment: Patch, v1.1

Updated patch with Nick's suggestion to use a const NSString for the e=malwareBlocked parameter.

Comment 11

[Stuart Morgan]

Comment on attachment 393588 [details] [diff] [review]
Patch, v1.1

sr=smorgan

>+  NSDictionary* blockedSiteInformation = 
>+    [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithInt:aBlockedReason], kBlockedSiteInformationBlockedReasonKey,
>+                                               [NSDate date], kBlockedSiteInformationBlockedDateKey,
>+                                               nil];

Wrap everything after the dictionaryWithObjectsAndKeys: down to another line indented a couple spaces relative to the |[NSDictionary ...| so that first line isn't crazy-long

Comment 12

[Sean Murphy]

Attachment: Patch, for Check In

Indentation adjusted.

Comment 13

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

Landed on CAMINO_2_0_BRANCH and cvs trunk.