587552 - Consider disabling view-source on pages where the cert error overlay is present

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

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

When the certificate error overlay is present, view-source doesn't really work.  It pops up the source window, with an overlay there, too.  On the overlay page, however, the Exception button doesn't work ("cert for (null) can't be found"), so, in essence, you have to go back to the original page and add the exception there before you can see source.

Note that we already disable view-source keyboard shortcut and toolbar icon on safebrowsing-blocked pages (where ignoring the warning in the view-source view *does* actually work, if you invoke view-source by manually typing the protocol), so that seems like an additional reason to do so on cert error cases, too.

Actually, we may want to do it for netError, too; it won't work for connection failures, and though there may be an odd error condition where it will work (maybe the gzip thing?), it's unlikely to be helpful generally (and people can always append the protocol if they want to try).

Do we have some general method of knowing when an overlay is showing?

Comment 1

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

(In reply to comment #0)
> Do we have some general method of knowing when an overlay is showing?

To answer my own question, no.  hendy added isBlockedOverlay in bug 501246, but that checks specifically for blockedSite.

We could add another one(s) for certError and netError (for the latter, the "Quick heuristic" is "Page Load Error", I think--the "title" entities are actually for <H1>s: http://mxr.mozilla.org/camino/source/camino/embed-replacements/locale/en-US/global/netError.dtd) and then have view-source check both of them in addition to whatever else it checks.

(Though it seems to me that we'd want to refactor the existing method to handle all three cases and just return which overlay is blocked, or just make the existing method handle all three and disable all the same things--except I think we want reload enabled for real network errors.)

Comment 2

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

Per the meeting, we should have one method for each of the two behaviors/sets of things to disable.  So certError can join safebrowsing's isBlockedOverlay and we'll make a new one for netError (and check for it the same places, less Reload, which we want enabled).

Comment 3

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

Oh, hmm, I read the patch wrong, or rather the wrong patch (looks like I read the initial patch instead of the checked-in one). "Reload" is left enabled in the existing isBlockedOverlay, so we can just make the existing check check for all three error pages :)

Comment 4

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

I didn't do my homework well enough on this at at all.

2) We have an existing validation method for netError, isPageLoadErrorOverlayShowing.

3) We don't want to prevent bookmarking of pages that we hit via network errors, I don't think, since they're not dangerous, and because you may want to bookmark them for the very purpose of visiting them later when your network is working OK.

So, isBlockedErrorOverlayShowing is checked to disable:
* bookmarking
* opt-return downloading
* mail page url
* view-source
* fill form
* print
* page setup
* save
* reporting page as phishing

isPageLoadErrorOverlayShowing is checked to disable:
* Reporting page as phishing

We should additionally disable for network error pages
* Save
* Fill form
* View-source
* Opt-return downloading

Save/download and view-source actually fail; Fill Form is extraneous since there will never be form elements.  

Print/Page Setup are harmless, as is Mail Page URL, so we should keep them enabled (these are not harmful pages, and you might want to mail the URL or print the error page to show someone/ask if they see the problem, too).  Bookmarking I mentioned above.

For certificate error pages, I think we actually want to copy the behavior of network error pages; we don't know for certain that cert error sites are harmless, but bookmarking or mailing the page URL don't seem like serious vectors, nor does printing the certificate error page.

So my new plan is to beef up the validation for isPageLoadErrorOverlayShowing and then add certError to that method.

Comment 5

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

Attachment: More and better validation

This implements my final plan from comment 4 (aka "once I spent more than 30s auditing our existing behavior" :P ).

There's a lot of churn in this patch from reformatting lines for the new conditions, moved selectors, lining stuff up, and so forth, but there's remarkably little substance ;)

Comment 6

[Stuart Morgan]

Comment on attachment 467322 [details] [diff] [review]
More and better validation

>-- (BOOL)isEmpty;                      // is about:blank loaded?
>+- (BOOL)isEmpty;                       // is about:blank loaded?
> - (BOOL)isInternalURI;
>-- (BOOL)isBlockedErrorOverlayShowing; // is about:safebrowsingblocked loaded?
>-- (BOOL)isPageLoadErrorOverlayShowing;
>+- (BOOL)isBlockedErrorOverlayShowing;  // is about:safebrowsingblocked loaded?
>+- (BOOL)isPageLoadErrorOverlayShowing; // is about:neterror or about:certerror loaded?

As long as you are touching these lines:
- Move the comments up above the lines they are commenting, the way we normally do method comments, and make them sentences.
- Describe them without including the specific implementation details of the address. So for example:

  // Returns YES if a network or certificate error overlay is showing.
  - (BOOL)isPageLoadErrorOverlayShowing; // is about:neterror or about:certerror loaded?

sr=smorgan with that change.

Comment 7

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

http://hg.mozilla.org/camino/rev/f70bb96c693f with that change.

(This doesn't actually depend on bug 521137 at all, because right now certificate error pages are just a subset of netError, but when we do have stand-alone certError, the checks will check it, too.)