501246 - Disable certain menu items on blocked phishing and malware pages

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Christopher Henderson]

On blocked pages, Safari disables: Add Bookmark, View Source, Save As, Send *, Print, and Reload. This seems like a good idea, as these functions are mostly useless when we're displaying the blocked site overlay.

I have a patch ready to go once we confirm this bug.

Comment 1

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

I don't see why we wouldn't do this, given our validation fixation and our dislike of useless or confusing items enabled on useless pages ;-)  To summarize my questions from irc:

* Fill Form is also disabled
* For tab group bookmarking, the tab is simply dropped, just like other dangerous pages
* Page Info is still enabled

Page Info is the only thing I'm a little iffy about; I don't have a very good reason for why I feel that way, though ;)

We'll also want to ensure the respective context menu items are also disabled.

Comment 2

[Christopher Henderson]

Attachment: Fix v1.0

Adds an |isBlockedOverlay| method to BrowserWrapper that looks for the about:safebrowsingblocked overlay. To make it more efficient, it only inspects the DOM if the page has an appropriate title. Also includes validation code for menu, toolbar, and context menu items.

Requesting a review from Sean, since the method uses some of his code from |onXULCommand|.

Comment 3

[Sean Murphy]

Comment on attachment 385945 [details] [diff] [review]
Fix v1.0

Nice job on the patch Hendy, and noticing that we need to do this.  I have only few changes, most really are kind of optional and I just wanted to at least express my ideas for them.  With the few changes below (and some of the optional ones considered) r=murph.

>+NSString *const kPhishingTitleText = @"PhishingTitleText";
>+NSString *const kMalwareTitleText = @"MalwareTitleText";
...
>+  if ([currentTitle isEqualToString:NSLocalizedString(kPhishingTitleText, nil)] ||
>+      [currentTitle isEqualToString:NSLocalizedString(kMalwareTitleText, nil)]) {

I think we might want to just declare the localized string keys inline here, rather than declaring constants.  I'm always a believer of constants, but in this case we don't typically use them for NSLocalizedString, so maybe they're not as beneficial here.  I only used constants in the SafeBrowsingAboutModule because the actual values were placeholders in the HTML template file, and I designed it so that the same value for the placeholders were also the keys for localized strings to swap in.

>	+  // validate Reload
>	+  if (![self validateActionBySelector:@selector(reload:)])
>	+    [[result itemWithTarget:self andAction:@selector(reload:)] setEnabled:NO];

Reload is the only disabled item in the patch I actually think we might want to consider leaving enabled.  All of the others are disabled because they do not make much sense, or they are a security implication.  As a user, I'd feel too restricted by this.  The site will be blocked each load.  Also, the safe browsing database does actually remove false positives (though I know it's a rare situation to have a false blocked site removed in between reloads).

> +- (BOOL)isBlockedOverlay;             // is about:safebrowsingblocked loaded?

I'd suggest maybe changing this name slightly to something like |isBlockedErrorOverlayShowing|, since the BrowserWrapper is not necessarily the actual blocked overlay (it just holds changing content).

> +    nsCOMPtr<nsIDOMWindow> domWindow = [mBrowserView contentWindow];
> +    if (!domWindow)
> +      return NO;
> +    nsCOMPtr<nsIDOMDocument> domDocument;
> +    domWindow->GetDocument(getter_AddRefs(domDocument));
> +    if (!domDocument)
> +      return NO;
> +    nsCOMPtr<nsIDOM3Document> doc = do_QueryInterface(domDocument);
> +    if (!doc)
> +      return NO;
> +    nsAutoString docURISpec;
> +    nsresult rv = doc->GetDocumentURI(docURISpec);
> +    if (NS_FAILED(rv))
> +      return NO;
> +    NSString* documentURI = [NSString stringWith_nsAString:docURISpec];
> +    return [documentURI hasPrefix:@"about:safebrowsingblocked"];

I'm not sure if it's worth it to refractor this code for fetching the actual document URI, which is duplicated in |onXULCommand:|, into it's own method.  I wanted to at least indicate that this same procedure is used twice, to see if it's worth breaking out.  It's not a block of code that's going to need change necessarily, but it is duplicated nonetheless.

>    // disable non-BWC items that aren't relevant if there's no main browser window open
>    // or the bookmark/history manager is open
>    if (action == @selector(savePage:))
> -    return (browserController && ![browserController bookmarkManagerIsVisible]);
> +    return (browserController && [browserController validateActionBySelector:action]);

The comment above this block is too specific, as we're now validating more than just what it indicates.  As with most comments, I'd change it to not mention too much about the actual implementation of the code it's referring to, but generally just indicate the purpose of the block so we can see at a glance why it's there in general.

Comment 4

[Christopher Henderson]

Attachment: Fix v1.1

Thanks for the review, Sean.

>I think we might want to just declare the localized string keys inline here,
>rather than declaring constants.

Done.

>Reload is the only disabled item in the patch I actually think we might want to
>consider leaving enabled.

This patch re-enables Reload for blocked pages, as it is harmless.

>I'd suggest maybe changing this name slightly to something like
>|isBlockedErrorOverlayShowing|

Done.

>I'm not sure if it's worth it to refractor this code for fetching the actual
>document URI, which is duplicated in |onXULCommand:|, into it's own method.

Refactored, including onXULCommand, which seems to still work.

>The comment above this block is too specific, as we're now validating more than
>just what it indicates.

Generalised.

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 389611 [details] [diff] [review]
Fix v1.1

+  else {
+    return NO;
+  }

don't have an else after a return. it's un-necessary.

sr=pink

Comment 6

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

Checked in on cvs trunk and the CAMINO_2_0_BRANCH with pink's comment addressed.