1696841 - can't click "Accept the risk and continue" on secure connection failed page

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Timothy Nikkel (:tnikkel)]

Connect to a site that gives you the secure connection failed error page (I don't have an example that is likely to work for everyone but someone must know an example page that does this). Click Advanced and then try to click "accept risk and continue", it does not work. Putting the page in desktop mode does not fix it but changes the layout.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

To be clear is this in Geckoview example or Fenix? The error pages for Fenix are in the Android components project.

Comment 2

[Timothy Nikkel (:tnikkel)]

This is in Firefox for android, release and nightly. If this isn't the right place to file a bug could you please file a bug in the correct place and cc me? Thanks!

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

https://github.com/mozilla-mobile/fenix/issues/18441

Comment 4

[codrut.topliceanu]

Hey, I was unable to reproduce on current Nightly, Pixel 3xl (Android 10)
Could we get some more info on this, maybe some steps to reproduce?

Tested using self signed server.

https://github.com/mozilla-mobile/fenix/issues/18441#issuecomment-799437301

Comment 5

[Timothy Nikkel (:tnikkel)]

I posted in the github issue.

Comment 6

[codrut.topliceanu]

https://github.com/mozilla-mobile/fenix/issues/18441#issuecomment-800999693

After some more investigations, and some help from my colleagues, it turns out that the issue stems from lowMediumErrorPages.js, specifically the addCertException method:

async function acceptAndContinue(temporary) {
try {
    await document.addCertException(temporary);
    location.reload();
} catch (error) {
    console.error("Unexpected error: " + error);
}
};

This results in : Unexpected error: InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable.

After running document.addCertException(true) in GeckoViewExample, I got the same exception: <IMAGE>

As document.addCertException is a GeckoView method I will post this on the relevant Bugzilla issue.

Comment 7

[Amedyne Moya [:amoya]]

More info from github: https://github.com/mozilla-mobile/fenix/issues/18441#issuecomment-801909286

"this bug has been driving me crazy for a lot of time today i tried to make some investigation i found that it happens only with sites that have old TLS versions e.g 1.0 and 1.1 here is an example to reproduce: http://tls-v1-0.badssl.com:1010/.
it happens on all firefox branches: stable, beta, nightly and even fennec."

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

I can reproduce this using revoked.badssl.com but interestingly not other kinds of certificate errors

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

On desktop I get an error page when trying to "continue", I suspect we don't handle this error case well.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Looks like GetServerCert returns null for this case here: https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/dom/base/Document.cpp#1585

Comment 11

[Makoto Kato [:m_kato]]

Some error types are overrideable error, but this isn't all. (bug 991177, bug 1123671 and etc). So Firefox desktop hides "exceptionDialogButton". Ex. https://searchfox.org/mozilla-central/rev/192a45a34e9dd9d6760c55b6ca80fe598136be3c/security/manager/ssl/NSSErrorsService.cpp#127-131 and https://searchfox.org/mozilla-central/rev/ac36d76c7aea37a18afc9dd094d121f40f7c5441/browser/base/content/certerror/aboutNetError.js#802-812.

So I guess that Fenix (android-component) has to hide "add cert exception" / "Accept the Risk and Continue" button for some cases such as "nssFailure2" (In other word, nss error category isn't `nsINSSErrorsService::ERROR_CLASS_BAD_CERT).

Dana, is this right? Original case is https://github.com/mozilla-mobile/fenix/issues/18441 and I would like to clear this issue's detail.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Yes - there are some errors that are not overridable, as you've identified. For errors that are overridable, if a host is HSTS, those errors are not overridable for that host. Let me know if you have any further questions.

Comment 13

[Makoto Kato [:m_kato]]

Thank you, Dana.

Comment 14

[Makoto Kato [:m_kato]]

This is moved to a-c as https://github.com/mozilla-mobile/android-components/issues/10588

Comment 15

[[ex-Mozilla] Agi Sferro | :agi]

Sorry to reopen this, if some error pages are not overridable we need to provide an API for the embedder to know when an error can be overridden, we cannot expect apps to know which pages are overridable and which are not, especially if the API is still present but doesn't do anything / fails when used.

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

Maybe Document::IsErrorOverridable or something like that?

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The canonical function that determines if an error is overridable is ErrorIsOverridable: https://searchfox.org/mozilla-central/rev/42ae3bea104c37a9986c6f18d17bd9ddb387129c/security/manager/ssl/NSSErrorsService.cpp#136
Where/how should this function be exposed so embedders can use it?

Comment 18

[[ex-Mozilla] Agi Sferro | :agi]

Dana(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #17)

Where/how should this function be exposed so embedders can use it?

We expose addCertException to the document object in error pages: https://searchfox.org/mozilla-central/rev/42ae3bea104c37a9986c6f18d17bd9ddb387129c/dom/webidl/Document.webidl#324-325

if we could have attribute boolean errorIsOverridable there too, it should work for us.

Comment 19

[Mira Lobontiu (Android QA)]

Verified as fixed on the 92.0a1 Firefox Nightly build from 7/21 with https://expired.badssl.com/ page with the following devices:

• Google Pixel (Android 10),
• Samsung Galaxy Tab A6 (Android 5.1.1), and
• HTC 10 (Android 8).

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I'm confused - given https://github.com/mozilla-mobile/android-components/commit/7bfbbb8c6d79e4b34c2edc5cb443c1643923c2c1, is what's described in comment 18 still necessary?

Comment 21

[Makoto Kato [:m_kato]]

I'm confused - given https://github.com/mozilla-mobile/android-components/commit/7bfbbb8c6d79e4b34c2edc5cb443c1643923c2c1, is what's described in comment 18 still necessary?

Actually, we can confirm this from error code. ERROR_SECURITY_BAD_CERT is override-able and other isn't. agi, does comment #15 means that we need Chrome-only WebIDL API for this instead of using error code?

Comment 22

[[ex-Mozilla] Agi Sferro | :agi]

I don't think every BAD_CERT is overridable, but that's besides the point. The app should not be in charge of deciding what's overridable or what isn't, GeckoView (and really, Gecko) should. It's a much better proposition to tell the app "call this method to figure out whether the error can be overidden or not" rather than "here's a list (which can potentially change over time) of the errors you can override and the ones that you can't".

Comment 23

[Makoto Kato [:m_kato]]

I filed new issue as bug 1723875 for API.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #22)

I don't think every BAD_CERT is overridable, but that's besides the point. The app should not be in charge of deciding what's overridable or what isn't, GeckoView (and really, Gecko) should. It's a much better proposition to tell the app "call this method to figure out whether the error can be overidden or not" rather than "here's a list (which can potentially change over time) of the errors you can override and the ones that you can't".

The docshell already does this: https://searchfox.org/mozilla-central/rev/7b1de5e29d878cc163dec7beaf9b57a2f0f41aaa/docshell/base/nsDocShell.cpp#3515-3517
Is that information not available to GeckoView to pass to the app?

Comment 25

[[ex-Mozilla] Agi Sferro | :agi]

We do not have access to cssClass in GeckoView, no, but we're fixing that.

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler]]

So it seems like bug 1723875 will be unnecessary when that's fixed, right? (also, is there a bug for fixing that?)

Comment 27

[[ex-Mozilla] Agi Sferro | :agi]

It's not, as I said before, we can't reasonably expect the front end to know which errors are overridable and which are not. Since the API that they use to override the error is addCertException it seems reasonable to have a isErrorOverridable API too, but it can be anything as long as it's not "read this doc".

Comment 28

[Dana Keeler (she/her) (use needinfo) [:keeler]]

But the front-end doesn't need to know - it just needs to check cssClass, like the front-end on desktop does now: https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/browser/base/content/certerror/aboutNetError.js#201-202