Last Comment Bug 769178 - Proper invalid certificate errors on HTTPS
: Proper invalid certificate errors on HTTPS
Status: VERIFIED FIXED
Poland, IOT, Buri
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: B2G C4 (2jan on)
Assigned To: Patrick Wang (Chih-Kai Wang) (:kk1fff)
: Jason Smith [:jsmith]
Mentors:
: 796362 821421 825092 868763 879969 (view as bug list)
Depends on:
Blocks: browser-api 775293 839784 846734
  Show dependency treegraph
 
Reported: 2012-06-28 01:51 PDT by Antonio Manuel Amaya Calvo (:amac)
Modified: 2013-06-06 10:50 PDT (History)
39 users (show)
jsmith: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
leo+
-
wontfix
fixed
fixed
wontfix
wontfix


Attachments
Patch: report bad certificate (15.66 KB, patch)
2013-01-03 04:32 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Patch: report bad certificate v2 (9.18 KB, patch)
2013-01-25 03:18 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Patch: Part 1 - add about:certerror page for ceritificate error. (18.22 KB, patch)
2013-01-30 20:15 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Patch: Part 2 - handle "get me out of here" click event. (3.15 KB, patch)
2013-01-30 20:39 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Patch: Part 1 - add about:certerror page for ceritificate error. (18.21 KB, patch)
2013-01-31 19:58 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
fabrice: review+
Details | Diff | Splinter Review
Patch: Part 2 - hide "get me out of here" button. (2.46 KB, patch)
2013-02-04 02:51 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
Patch: add about:certerror page for ceritificate error. (17.91 KB, patch)
2013-02-07 20:13 PST, Patrick Wang (Chih-Kai Wang) (:kk1fff)
fabrice: review+
Details | Diff | Splinter Review

Description Antonio Manuel Amaya Calvo (:amac) 2012-06-28 01:51:55 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

Access some HTTPS url that has either a certificate signed by an unknown CA (like an internal one) or a certificate for a wrong address (can try accessing a server through the IP address instead of its name)


Actual results:

The error page shown was 'The URL is invalid' 


Expected results:

It should have given the proper error info. Something like  'The certificate for this page is invalid'
Comment 1 JP Rosevear [:jpr] 2012-07-10 13:26:11 PDT
Sounds like we need proper error page handling?
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-11 11:55:33 PDT
Actually, not sure that this is a blocker. Sounds like a bad experience, but users don't hit certificate errors often enough that it's worth holding a release for.
Comment 3 Antonio Manuel Amaya Calvo (:amac) 2012-07-11 13:43:21 PDT
(In reply to Jonas Sicking (:sicking) from comment #2)
> Actually, not sure that this is a blocker. Sounds like a bad experience, but
> users don't hit certificate errors often enough that it's worth holding a
> release for.

Depends on what kind of users we're talking about. Anyone that works on a medium to big company will probably find this error if he brings his device to work.
Comment 4 Justin Lebar (not reading bugmail) 2012-07-11 13:44:55 PDT
> Anyone that works on a medium to big company will probably find this error if he brings his device 
> to work.

I don't think it's correct to assert that every, or even most, medium- to large-sized companies use self-signed certs.  Do you have any evidence for this?
Comment 5 Antonio Manuel Amaya Calvo (:amac) 2012-07-11 14:05:19 PDT
(In reply to Justin Lebar [:jlebar] from comment #4)
> > Anyone that works on a medium to big company will probably find this error if he brings his device 
> > to work.
> 
> I don't think it's correct to assert that every, or even most, medium- to
> large-sized companies use self-signed certs.  Do you have any evidence for
> this?

Actually that's a good question, and I couldn't find any statistics about that. From my own experience, though, I don't know of a single company here that's using public certificates for internal servers. That's restricted to the companies I've worked for, or my friends work for though and that's mostly Spain on a bunch of medium to big companies and government departments.

So in truth I don't have how the public versus private statistics is on Brazil. I just assumed on the rest of the places companies wouldn't like paying yearly for something that doesn't really add anything to an internal service :)
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-12 13:16:02 PDT
Note that this bug isn't about enabling custom certs. It's just about showing an error page when we get an invalid cert.

I don't think that can be a blocker even if we decide that custom certs are a blocker.
Comment 7 Antonio Manuel Amaya Calvo (:amac) 2012-07-12 13:28:37 PDT
Dunno about blocker or not. I just lost an hour debugging something related to this error though. Turns out that if you connect to a URL with a correct certificate (say https://server.com) that then redirects you to a URL with an incorrect certificate (trusted CA, bad name, say https://m.server.com) you get the same 'URL invalid'... and when you click on the address bar it shows https://server.com, NOT https://m.server.com which was the one that actually failed.

Probably not blocking, definitely irritating.
Comment 8 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-13 18:41:49 PDT
(In reply to Antonio Manuel Amaya Calvo from comment #7)
> Dunno about blocker or not. I just lost an hour debugging something related
> to this error though. Turns out that if you connect to a URL with a correct
> certificate (say https://server.com) that then redirects you to a URL with
> an incorrect certificate (trusted CA, bad name, say https://m.server.com)
> you get the same 'URL invalid'... and when you click on the address bar it
> shows https://server.com, NOT https://m.server.com which was the one that
> actually failed.
> 
> Probably not blocking, definitely irritating.

I filed bug 773899 about that specific problem.
Comment 9 Mossroy 2012-09-24 12:31:15 PDT
I faced this issue when trying to access a self-signed https website.

More than an error message, it should also give a way for the user to check and accept this certificate (temporarily or definitly). So that he might accept the risk and access the website anyway.
Just like Firefox does on computers, and on Android too.

Currently, any problem with the certificate prevents the user from accessing the website at all.
Comment 10 Walter Chen[:ypwalter][:wachen] 2012-11-27 02:27:23 PST
I now confirmed that this issue still exists.
It will lead you to a strange page instead of asking user to accept the certificate like the desktop Firefox browser.
It should be fixed, this should be the basic function of browser.

Example:
You can go to https://mail.spreadtrum.com.cn/ in your B2G phone and see the error message. But, you will see difference in desktop Firefox browser.

Also, https://bugzilla.mozilla.org/show_bug.cgi?id=773899 is a related bug.
Comment 11 Justin Lebar (not reading bugmail) 2012-11-27 07:35:59 PST
Unless we have resources to tackle this bug, I don't think it should be re-triaged to blocking+.
Comment 12 Dave Hylands [:dhylands] 2012-11-27 10:11:07 PST
minusing unless somebody gets assigned
Comment 13 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-12-13 11:55:57 PST
*** Bug 821421 has been marked as a duplicate of this bug. ***
Comment 14 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-03 04:32:14 PST
Created attachment 697403 [details] [diff] [review]
Patch: report bad certificate

DisplayLoadError uses macro to identify nss error. So I add a service to process the error code in C++. The error strings ('nssBadCert' and 'nssFailure2') are from DisplayLoadError. Justin, would you help to review this?
Comment 15 Justin Lebar (not reading bugmail) 2013-01-03 14:52:25 PST
> DisplayLoadError uses macro to identify nss error.

We talked about this on IRC.  Apparently the canonical way of handling this in JS is to hardcode the math that the macros do.

That's ugly, but perhaps not as ugly as creating a whole new XPCOM component just so we can do

>+  if (NS_ERROR_GET_MODULE(aStatus) == NS_ERROR_MODULE_SECURITY) {

:)  (At least, I think that line is the only reason you had to make the XPCOM component.)

Would you mind changing the patch?
Comment 16 Justin Lebar (not reading bugmail) 2013-01-03 14:52:57 PST
> We talked about this on IRC.

I should say, I spoke with people who know more about this than I do.  :)  They pointed me to other similar uses in the tree, e.g.

http://mxr.mozilla.org/mozilla-central/search?string=const+NS_ERROR&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 17 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-03 16:10:40 PST
nsINSSErrorsService is implemented by NSSErrorsService:

NSSErrorsService::GetErrorClass(nsresult aXPCOMErrorCode, uint32_t *aErrorClass)
{
  NS_ENSURE_ARG(aErrorClass);

  if (NS_ERROR_GET_MODULE(aXPCOMErrorCode) != NS_ERROR_MODULE_SECURITY
      || NS_ERROR_GET_SEVERITY(aXPCOMErrorCode) != NS_ERROR_SEVERITY_ERROR)
    return NS_ERROR_FAILURE;
}

So, you can do something like:

    isCertError = false;
    try {
      const NS_ERROR_MODULE_SECURITY = 21;
      errorClass = errorService.getErrorClass(error);
      isCertError = errorClass == NS_ERROR_MODULE_SECURITY;
    } catch (e) {
      // no-op
    }

That said, I am not sure why we need to have special error handling logic for B2G. Why doesn't the error page logic in nsDocShell work for B2G? That logic is pretty complicated and it's going to get more complicated, especially for SSL-related stuff. I would rather we try to keep it centralized instead of having to maintain multiple implementations in multiple languages, if it is at all reasonable to do so.
Comment 18 Justin Lebar (not reading bugmail) 2013-01-03 16:16:08 PST
> Why doesn't the error page logic in nsDocShell work for B2G?

UX has flip-flopped on this a few times, but AIUI there are some circumstances under which UX currently does not want to show a Gecko error page.  They want to show an error page in Gaia.  Which means we need to notice when there's an SSL error and inform Gaia, which is what this patch does.
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-03 18:07:58 PST
(In reply to Justin Lebar [:jlebar] from comment #18)
> > Why doesn't the error page logic in nsDocShell work for B2G?
> 
> UX has flip-flopped on this a few times, but AIUI there are some
> circumstances under which UX currently does not want to show a Gecko error
> page.  They want to show an error page in Gaia.  Which means we need to
> notice when there's an SSL error and inform Gaia, which is what this patch
> does.

Gecko already supports mobile-specific error pages for network errors and SSL errors. Fennec uses them. Are they really not good enough for B2G?

The logic for determining which error page should be shown, and what parts of those error pages should be hidden/enabled/disabled, will become much smarter (i.e. more complicated) going forward. I am concerned that we will not be able to keep that security-sensitive logic in sync between products. It has been difficult for us to get resources for updating just *one* set of error pages with smarter designs. So, at least long-term, we will have to find some solution that allows Fennec and B2G to use the same error pages.

There is a new set of designs of error pages designs for Fennec have gone through multiple iterations based on feedback from the UX team and the security team, and they will be in Gecko. There is a separate set of B2G error page designs that is completely different, and which does not incorporate the security design elements that the security team asked for. I think it is unfortunate that there is a duplication of effort here and that the designs have diverged so much. We should try to eliminate that duplication of effort. Putting the B2G error pages in Gaia seems counter-productive in resolving that duplication of effort.

Considering this is blocking-basecamp-, and considering all the factors in the above paragraphs, I suggest we just re-use the Fennec error pages as-is as a minimal-risk, minimal-effort solution to this problem.
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2013-01-03 18:21:12 PST
We should do whatever the simplest thing to do is. Note that this bug isn't even blocking so doing more advanced error pages is certainly not blocking.
Comment 21 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-03 18:22:24 PST
(In reply to Brian Smith (:bsmith) from comment #19)
> Gecko already supports mobile-specific error pages for network errors and
> SSL errors. Fennec uses them. Are they really not good enough for B2G?
> 
> So, at least long-term, we will have to
> find some solution that allows Fennec and B2G to use the same error pages.

Also, to be clear, one goal is to have even Desktop Firefox use the new Fennec error pages, using some kind of "responsive design" for them, because we've already seen that it is difficult to keep the logic for just two implementations in sync.
Comment 22 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-03 21:47:22 PST
Currently, each embedder is allowed to decide how to handle the error reported form child. We should consider it to be an alert message, instead of an error page. Is it possible to reuse  the existing logic to understand the error code, and we can tell embedder the type of error, and let embedder handle the error (rather than just showing the error page)?
Comment 23 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-03 22:06:48 PST
(In reply to Patrick Wang [:kk1fff] from comment #22)
> Currently, each embedder is allowed to decide how to handle the error
> reported form child. We should consider it to be an alert message, instead
> of an error page. Is it possible to reuse  the existing logic to understand
> the error code, and we can tell embedder the type of error, and let embedder
> handle the error (rather than just showing the error page)?

I am not sure I understand the question, so let me know if my answer does not make sense.

The error is actually reported from the parent to the child, not from the child to the parent. If you need to take some action different than showing an error page then you can set up a web progress listener (or whatever) just like the nsDocShell code does. There is already a way within for the embedder to specify its own error pages. Note that, for security reasons, I believe it is critical that the web content displayed in the docshell *always* be replaced by some error page, because otherwise you may end up with the old document being displayed with the new document's URL, which can lead to spoofing attacks. So, I think it would be a bad idea to have an option to not display the error page.
Comment 24 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-03 22:43:26 PST
(In reply to Brian Smith (:bsmith) from comment #23)
> The error is actually reported from the parent to the child, not from the
> child to the parent. If you need to take some action different than showing
> an error page then you can set up a web progress listener (or whatever) just
> like the nsDocShell code does.

We do listen to web progress, and what we are trying to do in this patch is to translate the status code got from nsIWebProgressListener::onStateChange to the message that Gaia understands and pass the error message to Gaia.

No matter how Gaia handle this error message, the iframe will be redirected to the error page. This patch just try to inform Gaia.

(In reply to Patrick Wang [:kk1fff] from comment #22)
> Currently, each embedder is allowed to decide how to handle the error
> reported form child. We should consider it to be an alert message, instead
> of an error page. Is it possible to reuse  the existing logic to understand
    ^ I missed a "just" :)
Comment 25 Justin Lebar (not reading bugmail) 2013-01-03 23:11:10 PST
> Gecko already supports mobile-specific error pages for network errors and SSL errors. Fennec uses 
> them. Are they really not good enough for B2G?

I want to emphasize that the audience for this argument is not me and Patrick, but is instead the Firefox OS UI team, lead by Josh Carpenter.

I don't really care how we do the error pages, but I understand that our UX folks have strong opinions about this.  It sounds like you have strong opinions; you should seek them out in some forum and figure this out, keeping in mind that we've already flip-flopped on this issue and that, despite appearances, we'd like to ship B2G sometime soon.  :)

But like Patrick says, this patch is merely informing Gaia that an SSL error page is being displayed.  It's then up to Gaia to take some action (or not).  In this sense, we're merely forwarding the nsIWebProgressListener data up to Gaia.  I don't think your fight is with this patch, but instead with what Gaia might do with this information.
Comment 26 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-25 03:18:15 PST
Created attachment 706330 [details] [diff] [review]
Patch: report bad certificate v2

Implement in Javascript.
Comment 27 Justin Lebar (not reading bugmail) 2013-01-28 12:42:33 PST
> I don't really care how we do the error pages

Actually, that's not true -- I do care, and I don't want us to implement them in Gaia.  We've created all sorts of problems for ourselves by doing so, e.g. bug 829486.  But I've also become resigned to the way we're currently doing things, because I haven't been able to change them.

>+        if (messageStr || messageStr.length > 0) {
>+          if (errorClass == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {
>+            error = 'badcert';
>+          }
>+        }

Like bsmith warned, this code is pretty complicated and non-intuitive.  But I find this part most confusing.  Why do we need the |if (messageStr)| branch here?  If you're copying this logic from somewhere, where is it, exactly?
Comment 28 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-28 13:51:24 PST
FWIW:

(In reply to Justin Lebar [:jlebar] from comment #25)
> > Gecko already supports mobile-specific error pages for network errors and SSL errors. Fennec uses 
> I want to emphasize that the audience for this argument is not me and
> Patrick, but is instead the Firefox OS UI team, lead by Josh Carpenter.
> 
> I don't really care how we do the error pages, but I understand that our UX
> folks have strong opinions about this.  It sounds like you have strong
> opinions; you should seek them out in some forum and figure this out,
> keeping in mind that we've already flip-flopped on this issue and that,
> despite appearances, we'd like to ship B2G sometime soon.  :)
>
> But like Patrick says, this patch is merely informing Gaia that an SSL error
> page is being displayed.  It's then up to Gaia to take some action (or not).
> In this sense, we're merely forwarding the nsIWebProgressListener data up to
> Gaia.  I don't think your fight is with this patch, but instead with what
> Gaia might do with this information.

I did contact the mobile UX team by private email and we all seemed to agree that the proposed Gaia UI should be improved to de-emphasize the "Ignore this warning and continue" option.

Also, I don't have a strong opinion on whether Gaia presents the information or Gecko does. My main concern is making sure we have a reaosnable way to ensure that all the products meet the security requirements we have. I think keeping the implementations in sync (parity with each other) is a good way to do that, and I think that having the implementations share as much code as possible is a good way to keep them in sync. But, also, I do think it is important that the UX on B2G also be consistent with the rest of the B2G UX.

There *is* a problem with having Gecko present the error page inline like other pages: We should not have the UI controls for bypassing the error be in the same process as the web content, because if the UI controls for bypassing the error are in the same process as the web content, then a compromised child process could just send the "add cert override" message to the parent.

If passing the error to Gaia is the best way to ensure this, then I am definitely in favor of having Gaia handle the UI. But, if so, then I would like to see us factor out as much of the logic as possible into a JSM that can be used by both Gaia and Gecko.
Comment 29 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-28 20:18:20 PST
(In reply to Justin Lebar [:jlebar] from comment #27)
> Like bsmith warned, this code is pretty complicated and non-intuitive.  But
> I find this part most confusing.  Why do we need the |if (messageStr)|
> branch here?  If you're copying this logic from somewhere, where is it,
> exactly?

This is from 
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=ee7cb4422133#4231

The origin code doesn't check if messageStr is null. messageStr is impossible to be null in C++ code, but it could be null in js. So I add this condition to prevent exception of calling length of null object.
Comment 30 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-28 20:25:58 PST
Oh... I realized that the problem we are actually facing. I think, in addition to report this error to Gaia, we should fix the "the address isn't valid" error page. It should not be "the address isn't valid", it should be error page of "bad certificate".
Comment 31 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-30 20:15:05 PST
Created attachment 708442 [details] [diff] [review]
Patch: Part 1 - add about:certerror page for ceritificate error.

The "invalid url" is for "about:certerror". We don't have an error page for certificate error. This patch copies certificate error page from mobile/android and makes a few change to make it able to be shown.

Hi Chris, about:neterror page is reviewed by you, would you help to review this patch? Thanks.
Comment 32 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-30 20:39:21 PST
Created attachment 708446 [details] [diff] [review]
Patch: Part 2 - handle "get me out of here" click event.

There is a "Get me out of here" button shown in certerror page. In desktop and Android firefox, this button redirects user to "about:home". But it seems there's no "about:home" in b2g.

I am not sure what should we do when user click this button. My suggestion is redirecting user to some address, like "www.mozilla.org", or we can fire a windowclose event to embedder to close this window. It needs some change in browser app, because browser app doesn't close the tab if there is only one tab.
Comment 33 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-01-31 19:58:00 PST
Created attachment 708918 [details] [diff] [review]
Patch: Part 1 - add about:certerror page for ceritificate error.

I missed a change in previous patch. The previous patch has an xml error because an l10n dtd file is not found by the error page.
Comment 34 [:fabrice] Fabrice Desré 2013-01-31 20:10:36 PST
Comment on attachment 708918 [details] [diff] [review]
Patch: Part 1 - add about:certerror page for ceritificate error.

Review of attachment 708918 [details] [diff] [review]:
-----------------------------------------------------------------

Works fine, thanks! The only little issue is that it's not discoverable that "Technical details" and "I Understand the Risks" are actually clickable. Can we fix that with some link styling.
Comment 35 Josh Carpenter [:jcarpenter] 2013-02-03 22:55:46 PST
Hi folks, just getting caught up on this thread thanks to a heads up from Patrick. Feel free to needinfo me for stuff like this. I can save you the effort of trying to discern inscrutable UX intentions. Here are my October specs for App Errors (hosted, as always, in our team's Dropbox): https://www.dropbox.com/sh/c0dc8zf71vsbji2/KMfRp_2sOW

I take issue with the notion that we've flip flopped on this. Per the specs, I believe our take has consistently been:

* Use default Gecko errors in the Browser app
* Hide default Gecko errors in non-Browser apps, and use custom simplified errors instead. 

The aim being to shield the mobile user from irrelevant (as far as vast majority of users are concerned) complexity. Admittedly  this implementation was rushed for v1 (shocker!), so I'm game to be convinced otherwise, so long as we don't hurt ease of use.
Comment 36 Josh Carpenter [:jcarpenter] 2013-02-03 22:58:38 PST
I spoke w/ Patrick and Fabrice on IRC, and the conclusion was that we'd simply hide the "Get me out of here" button. Hopefulyl this does count as altering the underlying default error page? Alternatively I also suggested having it:

1. If user has history in the tab, take them one step back
2. Else, close tab

As Patrick has mentioned, however, this would require implementing "enable user to close tab even if it's only one" functionality, which we currently lack (but should add).
Comment 37 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-02-04 02:51:23 PST
Created attachment 709656 [details] [diff] [review]
Patch: Part 2 - hide "get me out of here" button.
Comment 38 Justin Lebar (not reading bugmail) 2013-02-04 02:58:58 PST
Comment on attachment 709656 [details] [diff] [review]
Patch: Part 2 - hide "get me out of here" button.

Wow, okay.

I have no idea if this is the right way to do this.  I'm not sure whom to ask.
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-02-04 16:58:43 PST
Comment on attachment 709656 [details] [diff] [review]
Patch: Part 2 - hide "get me out of here" button.

If there's no "getMeOutOfHereButton" then this patch will throw and break.  We should have a test for that.

Otherwise, yeah, not my thing either.
Comment 40 Josh Carpenter [:jcarpenter] 2013-02-04 20:41:00 PST
I've emailed Larissa and asked for her two cents on this one, as she's shifting her focus to all things Security + UX and working on new Gecko error pages with Ian Barlow. She or I will follow up.
Comment 41 [:fabrice] Fabrice Desré 2013-02-05 12:11:35 PST
Comment on attachment 709656 [details] [diff] [review]
Patch: Part 2 - hide "get me out of here" button.

Review of attachment 709656 [details] [diff] [review]:
-----------------------------------------------------------------

That looks very invasive to me. Since we have our own copy of aboutCertError.xhtml for b2g, why not just remove the <div id="whatShouldIDoContent"> ?
Comment 42 Josh Carpenter [:jcarpenter] 2013-02-05 23:08:29 PST
(In reply to Patrick Wang [:kk1fff] from comment #32)
> Created attachment 708446 [details] [diff] [review]
> Patch: Part 2 - handle "get me out of here" click event.
> 
> There is a "Get me out of here" button shown in certerror page. In desktop
> and Android firefox, this button redirects user to "about:home". But it
> seems there's no "about:home" in b2g.

Patrick, one more option: We actually do have a start page in the browser; it's a screen displaying a 2x2 grid of Top Sites and a Firefox logo in the background. Whether or not it's called "about:home", this is the page our "Get me out of here" button should redirect to. Is that feasible, instead of removing the button? Sorry, I realize you've already submitted a patch for that change... :p
Comment 43 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-02-06 09:03:13 PST
I've took a look at the start page. Looks like the start page is made in Gaia, it can't be displayed by just redirecting. As there are several bugs filed for wrong error message when browsing a site with bad certificate, can we fix this by simply removing the button for now, so people who browse the site with a bad certificate can see the correct error message (instead of invalid url), and get the error page better in a follow up?
Comment 44 Justin Lebar (not reading bugmail) 2013-02-06 09:28:47 PST
I totally agree with the proposed plan in comment 43, fwiw.
Comment 45 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-02-07 20:13:37 PST
Created attachment 711672 [details] [diff] [review]
Patch: add about:certerror page for ceritificate error.

According to comment 41, remove "what should I do".
Comment 46 Ryan VanderMeulen [:RyanVM] 2013-02-09 08:01:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56fb4d49ea7
Comment 47 Ryan VanderMeulen [:RyanVM] 2013-02-09 17:52:47 PST
https://hg.mozilla.org/mozilla-central/rev/c56fb4d49ea7
Comment 48 Naoki Hirata :nhirata (please use needinfo instead of cc) 2013-02-12 14:30:41 PST
*** Bug 825092 has been marked as a duplicate of this bug. ***
Comment 49 Mossroy 2013-02-14 12:44:44 PST
I downloaded the latest b2g from https://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/latest-mozilla-central/ and also pulled the latest gaia code from git.
The warning page correctly appears when finding a self-signed certificate, but nothing happens if I click on "Visit page" or "Add permanent exception".
I suppose I missed something?
Or maybe this bug is fixed only on the b2g side (and not yet on gaia)?
Comment 50 Mossroy 2013-02-24 01:34:35 PST
Same behavior with latest b2g-22.0a1.multi.linux-x86_64.tar.bz2 and latest gaia from git.
If some of you managed to go through a self-signed certificate within Firefox OS, how did you do?
Comment 51 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-02-24 04:52:29 PST
Sorry I missed this comment last week. The two buttons are not implemented in this bug. There's another bug 769183 tracks this.
Comment 52 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-03-01 05:02:33 PST
bug 769183 looks more complex than just fixing these two button, so I filed bug 846734 to fix these two buttons.
Comment 53 Ryan VanderMeulen [:RyanVM] 2013-03-08 06:48:56 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bbf58761f059
Comment 54 Jason Smith [:jsmith] 2013-03-15 08:23:04 PDT
*** Bug 796362 has been marked as a duplicate of this bug. ***
Comment 55 Ben Francis [:benfrancis] 2013-03-18 06:37:12 PDT
Have we checked in broken UI?

IIRC the reason this error page was disabled in the first place was because implementing the "add certificate" UI would be non-trivial. Is this actually a planned new feature for v1.x?
Comment 56 Jason Smith [:jsmith] 2013-03-18 08:16:32 PDT
(In reply to Ben Francis [:benfrancis] from comment #55)
> Have we checked in broken UI?
> 
> IIRC the reason this error page was disabled in the first place was because
> implementing the "add certificate" UI would be non-trivial. Is this actually
> a planned new feature for v1.x?

Yes, we have. That's bug 846734.

Reference - https://bugzilla.mozilla.org/show_bug.cgi?id=846734#c4.
Comment 57 Alex Keybl [:akeybl] 2013-03-19 16:46:31 PDT
Can we get more context around why this is blocking leo? We found bug 846734, which suggests we shouldn't take this unnecessary for v1.1
Comment 58 Jason Smith [:jsmith] 2013-03-19 17:31:50 PDT
Test Cases:

* Test that an error is reported when accessing HTTPS content with a self-signed certificate within the browser
* Test that an error is reported when accessing HTTPS content with a self-signed certificate within an app
* Test that an error is reported when accessing HTTPS content with a expired certificate within the browser
* Test that an error is reported when accessing HTTPS content with a expired certificate within an app
* Test that no error is reported when accessing HTTPS content with a OV certificate
* Test that no error is reported when accessing HTTPS content with a DV certificate
Comment 59 Ralph Daub [:rdaub] 2013-03-21 01:59:39 PDT
Hi Alex,
To add context to this issue, please see bug #850556.

SUMMARY:
There are major banking and government websites in Brazil, one of the launching locales, that have security certificate issues.

The problem is that the SSL certs for these websites chain up to ICP-Brasil's root cert which is not currently included in NSS (Bug #438825). 

That bug first documented in 2008. If it's still not resolved by the time the phone launches, the users may need a workaround to access these websites.

Here are some test cases of banking and government websites in Brazil:
https://cav.receita.fazenda.gov.br/eCAC/Aplicacao.aspx
https://cav.receita.fazenda.gov.br/eCAC/publico/login.aspx
https://www1.cav.receita.fazenda.gov.br/Servicos/ATRJO/IRPF_EXTRATO/index.asppode
https://ccd.serpro.gov.br/accmb/
https://ccd.serpro.gov.br/acserpro/docs/serprov2.crt
https://ccd.serpro.gov.br/acserpro/docs/serprofinalv2.crt
https://ccd.serpro.gov.br/certificados/index.htm
https://internetbanking.caixa.gov.br/SIIBC/index.processa

I hope this information helps. Thanks!!
Comment 60 [:fabrice] Fabrice Desré 2013-03-21 09:18:28 PDT
It's a clear improvement over the current situation, with no risk for additional breakage.
Comment 61 Ben Francis [:benfrancis] 2013-03-21 09:39:20 PDT
(In reply to Fabrice Desré [:fabrice] from comment #60)
> It's a clear improvement over the current situation, with no risk for
> additional breakage.

Does it still introduce broken UI?
Comment 62 Jason Smith [:jsmith] 2013-03-21 09:52:08 PDT
(In reply to Ben Francis [:benfrancis] from comment #61)
> (In reply to Fabrice Desré [:fabrice] from comment #60)
> > It's a clear improvement over the current situation, with no risk for
> > additional breakage.
> 
> Does it still introduce broken UI?

Yes. That's bug 846734.
Comment 63 Jason Smith [:jsmith] 2013-03-21 10:01:51 PDT
Talked in IRC about this with tchung. Back to nom again for these reasons:

1. This isn't a low risk change. bug 846734 proves we introduced broken UI here to the error page.

2. The original reason for renom in comment 57 wasn't addressed.

3. This is a new feature for v1.1 that was added without product input.

I agree that I want to get this fixed, but as for blocking analysis, the items above need to be discussed before determining if this should remain on b2g 18, backed out entirely, or back out the broken UI in bug 846734.
Comment 64 [:fabrice] Fabrice Desré 2013-03-21 10:09:32 PDT
This bug blocks bug 846734 which is already leo+. Can we stop the churn here?
Comment 65 Antonio Manuel Amaya Calvo (:amac) 2013-03-21 10:22:18 PDT
For what it's worth and in case you want some more use cases:

Main Spanish government portal: 
https://administracionelectronica.gob.es/

Spanish Tax Office:
https://www.agenciatributaria.gob.es/

Even the broken UI is preferable to what we have in 1.0 and 1.0.1. At least now the user is told *why* he can't see the page, instead of getting an invalid URL error.
Comment 66 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2013-03-21 10:31:22 PDT
Sorry for introducing the broken UI, I should remove the buttons here and get them back in the follow-up. I have a patch for bug 846734, but if we need to fix the broken UI now, I suggest that we can hide the button for now, thus people can get the correct error from the error page.
Comment 67 Jason Smith [:jsmith] 2013-03-21 10:56:16 PDT
(In reply to Patrick Wang [:kk1fff] from comment #66)
> Sorry for introducing the broken UI, I should remove the buttons here and
> get them back in the follow-up. I have a patch for bug 846734, but if we
> need to fix the broken UI now, I suggest that we can hide the button for
> now, thus people can get the correct error from the error page.

That sounds fine to me.
Comment 68 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-22 08:35:40 PDT
Since this is blocking a blocker and we could land it now to get the most testing around this, we'll leo+ - please uplift asap for testing window.
Comment 70 Daniel Coloma:dcoloma 2013-04-05 16:46:12 PDT
*** Bug 856529 has been marked as a duplicate of this bug. ***
Comment 71 Jason Smith [:jsmith] 2013-04-08 20:29:01 PDT
Did some sanity testing around this - marking as verified with some followup bugs.
Comment 72 Jason Smith [:jsmith] 2013-05-04 10:58:06 PDT
*** Bug 868763 has been marked as a duplicate of this bug. ***
Comment 73 Ben Francis [:benfrancis] 2013-06-06 10:50:19 PDT
*** Bug 879969 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.