855543 - Opening app content that generates a certificate error results in the "app not loading properly" modal dialog on top of the cert error handling page, making it impossible to provide a cert exception to the web content

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 3/27/2013
Device: Unagi

Steps:

1. Load app content that generates a cert error

Expected:

The cert error page in bug 769178 should be shown allowing for cert error details analysis and cert exception overrides.

Actual:

The cert error page quickly appears, but then the modal dialog of "this app isn't loading properly" sits on top of the error page. Repeatedly trying to try again won't work. So it's basically impossible to ever get to the correct cert error page.

This probably derives from the fact that our error handling in app content is done as modal prompts. Which I think isn't right, given this example scenario we've just hit. There's better other cases like this, FWIW.

Comment 1

[Jason Smith [:jsmith]]

Really poor UX. No workaround either. We technically shipped with this for v1, but we've added support for this now in v1.1 per bug 769178.

I'll start with tracking here, but let triage decide if this should bounce up to a nom or not. But generally, the UX is pretty bad here.

Comment 2

[Jason Smith [:jsmith]]

Jamie - Can you help find someone UX who can look into this?

Comment 3

[Dietrich Ayala (:dietrich)]

Fabrice, Patrich: We should fix the prompt-loop, regardless of whether we properly support cert exceptions in apps. Should that block? What's the right approach there?

Comment 4

[Jason Smith [:jsmith]]

Discussion for followup on comment 3:

jsmith	fabrice: bug 855543 for context. Basically, the cert error handling page only works within the browser, but fails completely within an app
	firebot	Bug https://bugzilla.mozilla.org/show_bug.cgi?id=855543 nor, --, ---, nobody, NEW, Opening app content that generates a certificate error results in the "app not loading properly" mod
	jsmith	fabrice: We're getting the cert error handling page content with the modal prompt repeatedly showing up saying "this app isn't loading properly" that always sits on top of the cert error handling page
	fabrice	jsmith: so the modal prompt is on top of the cert error page?
	firebot	Bug https://bugzilla.mozilla.org/show_bug.cgi?id=797627 nor, P1, ---, jimb, ASSI, Remote Debugging Protocol needs a way to contact B2G subprocesses
	jsmith	fabrice: Basically, yeah. And doing "try again" will cause the modal prompt to appear on top again.
	fabrice	that's unfortunate
	jsmith	fabrice: Knowing that, do we need leo? nom this if the cert error handling functionality is entirely blocked in app?
	fabrice	jsmith: I think so, yes
	fabrice	on the other hand, do we want users to use apps with invalid certs?
	-->|	nkot ([e-mail]) has joined #gaia
	fabrice	I'm not sure about the attack surface / risks there. We'll need to loop in security people
	jsmith	fabrice: okay, I'll flip on sec-review on it then

Comment 5

[Jason Smith [:jsmith]]

Actually, I'm going to redirect the UX request to Josh, as I believe he did the original UX here for app errors.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

To fix the prompt loop, I think we should let System app know if the error event it listened is for cert error. Currently, when cert error occurs, an error event with type='other' is dispatched to the app's frame.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this can't be worked around by the user we should block here, starting off with Fabrice to investigate - please re-assign to another engineer if needed.

Comment 9

[Josh Carpenter [:jcarpenter]]

(In reply to Patrick Wang [:kk1fff] from comment #6)
> To fix the prompt loop, I think we should let System app know if the error
> event it listened is for cert error. Currently, when cert error occurs, an
> error event with type='other' is dispatched to the app's frame.

That makes sense to me also. Something like: "if error is of type "cert", do not display default system "app error" prompt, and instead show cert error."

Jason, I would like to play around with this on device before confirming UX. Can you suggest both an app and a URL that will reliably trigger cert errors? Thanks

Comment 10

[Jason Smith [:jsmith]]

(In reply to Josh Carpenter [:jcarpenter] from comment #9)
> (In reply to Patrick Wang [:kk1fff] from comment #6)
> > To fix the prompt loop, I think we should let System app know if the error
> > event it listened is for cert error. Currently, when cert error occurs, an
> > error event with type='other' is dispatched to the app's frame.
> 
> That makes sense to me also. Something like: "if error is of type "cert", do
> not display default system "app error" prompt, and instead show cert error."
> 
> Jason, I would like to play around with this on device before confirming UX.
> Can you suggest both an app and a URL that will reliably trigger cert
> errors? Thanks

Example URL to access in the browser: https://summitbook.mozilla.org/

Example app - follow this STR:

1. Go to http://mozqa.com/webapi-permissions-tests/ in the browser
2. Install "Hosted App Test Case 1"
3. Launch that app titled "Test WebAPI Permissions"
4. Select "Unknown Cert Site" at the bottom of the page in the app

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Justin I add a XXX in the bug since I'm not 100% sure about a boolean or not. Also all the new code at the top of the file for this simple check seems a bit overkill but is there a different way to do that from JS?

Comment 13

[Justin Lebar (not reading bugmail)]

> Also all the new code at the top of the file for this simple check seems a bit overkill but is 
> there a different way to do that from JS?

It's gross, but I think it's the best we can do.

>+        if (NS_ERROR_GET_MODULE(status) == NS_ERROR_MODULE_SECURITY) {
>+          if (getErrorClass(status) == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {

Please make this one if statement with &&.

>+            // XXX Is there a point to fire the event if the page is different
>+            // from certerror? If yes, maybe a boolean to indicate that there
>+            // is a custom page would let the embedder have more control over
>+            // the desired behavior.

s/fire/firing/
s/the page/the error page/
s/is different from/is not/
s/maybe a boolean/maybe we should add a property to the event/
s/that there is/whether there is/
s/ would let/. That would let/

>+            var errorPage = Services.prefs.getCharPref(CERTIFICATE_ERROR_PAGE_PREF);

We'll throw if this pref doesn't exist.  Is that the right behavior?

>+            if (errorPage == 'certerror') {
>+              sendAsyncMsg('error', { type: 'certerror' });
>+              return;
>+            }

It would be awesome if we could have a test for this.  You may only have to add
a line or two to browserElement_ErrorSecurity.js.

Comment 14

[Justin Lebar (not reading bugmail)]

> Justin I add a XXX in the bug since I'm not 100% sure about a boolean or not.

I'm happy to leave the comment (or even get rid of it); we can always add the property later.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm going to add a test as you said.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + tests

This is the patch with review comments addressed and tests. Thanks for fixing my language btw :)

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 747935 [details] [diff] [review]
Patch + tests

r=me with two nits.

>@@ -822,16 +868,32 @@ BrowserElementChild.prototype = {
> 
>         // Ignoring NS_BINDING_ABORTED, which is set when loading page is
>         // stopped.
>         if (status == Cr.NS_OK ||
>             status == Cr.NS_BINDING_ABORTED) {
>           return;
>         }
> 
>+        if (NS_ERROR_GET_MODULE(status) == NS_ERROR_MODULE_SECURITY && 
>+            getErrorClass(status) == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {
>+
>+          // XXX Is there a point firing the event if the error page is not
>+          // from certerror? If yes, maybe we should add a property to the
>+          // event to to indicate whether there is a custom page. That would
>+          // let the embedder have more control over the desired behavior.

s/is not from/is not/
s/to to/to/

>+          try {
>+            var errorPage = Services.prefs.getCharPref(CERTIFICATE_ERROR_PAGE_PREF);
>+            if (errorPage == 'certerror') {
>+              sendAsyncMsg('error', { type: 'certerror' });
>+              return;
>+            }
>+          } catch(e) {}
>+        }

Nit: Please scope this try/catch more tightly; we don't want to try/catch
around the sendAsyncMessage.

>diff --git a/dom/browser-element/mochitest/browserElement_ErrorSecurity.js b/dom/browser-element/mochitest/browserElement_ErrorSecurity.js
>--- a/dom/browser-element/mochitest/browserElement_ErrorSecurity.js
>+++ b/dom/browser-element/mochitest/browserElement_ErrorSecurity.js

<3 tests.  Thanks.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + tests v2.

Same with review comments addressed.

Comment 19

[Justin Lebar (not reading bugmail)]

You don't need to ask for review again; I gave you r+.  I checked the interdiff anyway, and it looks great.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The test fails for some reasons on Android. Currently investigating.

Comment 21

[Justin Lebar (not reading bugmail)]

Most of the mozbrowser tests fail on Android, for reasons we have not investigated.  I wouldn't worry about it; I'd just disable it.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/projects/birch/rev/d87886c0bb7b

I have disable the test on Android.

Comment 23

[Casey Yee]

Has this landed on master where I can test the UX for this?   "Hosted App Test Case 1" in Comment 10 no longer seems to be working.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Casey Yee [:cyee] from comment #23)
> Has this landed on master where I can test the UX for this?   "Hosted App
> Test Case 1" in Comment 10 no longer seems to be working.

Ack. My server got borked in the process apparently. Let me work on resolving that problem...

Comment 25

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #24)
> (In reply to Casey Yee [:cyee] from comment #23)
> > Has this landed on master where I can test the UX for this?   "Hosted App
> > Test Case 1" in Comment 10 no longer seems to be working.
> 
> Ack. My server got borked in the process apparently. Let me work on
> resolving that problem...

Works for me on 1.01.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d87886c0bb7b

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: b2g-18 patch

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1290f7c5cb20

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1290f7c5cb20

Comment 31

[Jason Smith [:jsmith]]

So this works as specified in the bug, but I think this might have regressed the behavior seen in bug 877903 to be worse - now granting a temporary exception is permanent even on a restart. Filing a followup.

Comment 32

[:CSchmoeckel]

Added App Errors Suite Test Case #8936 - Test that the proper error page displays when an app generates a cert error.