Closed Bug 1411646 Opened 2 years ago Closed 2 years ago

browser.identity.launchWebAuthFlow API raises a warning regarding an invalid certificate for the redirect domain

Categories

(WebExtensions :: General, defect, P3)

x86_64
All
defect

Tracking

(firefox56 wontfix, firefox57 wontfix, firefox58 verified, firefox59 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: emilpasca, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

Attached image notes_issue_2.0.0a5.gif
[Affected versions]:
- Firefox Notes 2.0.0a5
- Firefox 56.0 and up

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Prerequisites]:
- Have a Firefox profile with the latest "Firefox Notes" add-on version (2.0.0a5) installed. (https://github.com/mozilla/notes/pull/342#issuecomment-339334192)
- Have a valid Sync Account.

[Steps to reproduce]:
1. Open the browser with the profiles from prerequisites.
2. Click the "Sync your Notes" button from the bottom of the sidebar.
3. Enter your credentials and confirm the e-mail.
4. Observe the string from the bottom part of the Sidebar.

[Expected result]:
- "Synced at (hour)" is displayed.

[Actual result]:
- "Synced at Invalid Date" string is displayed.

[Notes]:
- The following error is displayed in the Browser console:
"dee85c67bd72f3de1f0a0fb62a8fe9b9b1a166d7.extensions.allizom.org:443 uses an invalid security certificate.

The certificate is only valid for the following names:
  *.allizom.org, allizom.org  

Error code: <a id="errorCode" title="SSL_ERROR_BAD_CERT_DOMAIN">SSL_ERROR_BAD_CERT_DOMAIN</a>
  (unknown)
Login succeeded Object { access_token: "e4635f7a43a594a08a3b8e2b86a9d209ed5…", refresh_token: "3c0e1faf80f56f654582995a823009fb3b3…", key: Object }"

- Attached a screen recording of the issue:
The Synced at Invalid Date is something we are going to take care of in the Notes extensions.

However the issue we wanted to share with you is that as part as the Identity laucheWebFlow API we have the follwing error:


"dee85c67bd72f3de1f0a0fb62a8fe9b9b1a166d7.extensions.allizom.org:443 uses an invalid security certificate.

The certificate is only valid for the following names:
  *.allizom.org, allizom.org  

Error code: <a id="errorCode" title="SSL_ERROR_BAD_CERT_DOMAIN">SSL_ERROR_BAD_CERT_DOMAIN</a>
Summary: [Firefox Notes] "Synced at Invalid Date" is wrongly displayed when logging in the Sync Account → browser.identity.launchWebAuthFlow API raises a warning regarding an invalid certificate for the redirect domain
Shane, can you take a look?  I thought the redirected domain was supposed to be caught locally and never actually connected to...
Flags: needinfo?(mixedpuppy)
Hrm, it's a minor issue.  The request is not used, but it is also not canceled.  We just need to add request.cancel(NS_BIDING_ABORTED) to the onLocationChange handler in ext-identity.js.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Priority: -- → P3
I was able to intermittently cause this to happen (using mochitest --repeat), even after adding a request.cancel.  Moving to a different use of the webprogresslistener has made it so I can no longer intermittently cause this to happen.
Comment on attachment 8926187 [details]
Bug 1411646 prevent oauth redirect requests from happening,

https://reviewboard.mozilla.org/r/197456/#review202902

Hi Shane,
I took a look at this patch and I've added some review comments (mostly related to possibly simplify the change further and a very small tweak to the test case).

I'm not setting the r+ on it yet, mainly to evaluate if we can switch this patch from `gBrowser.selectedBrowser.webProgress.addProgressListener` to `gBrowser.addProgressListener` before landing it (nevertheless I'm ready to r+ it if it turns out that we have reasons to prefer the approach currently used in this patch).

::: toolkit/components/extensions/ext-identity.js:76
(Diff revision 1)
>      wpl = {
> -      onLocationChange(browser, webProgress, request, locationURI) {
> -        if (locationURI.spec.startsWith(redirectURI)) {
> -          resolve(locationURI.spec);
> +      onStateChange(progress, request, flags, status) {
> +        if (request instanceof Ci.nsIHttpChannel &&
> +          request.URI.spec.startsWith(redirectURI)) {
> +          request.cancel(Components.results.NS_BINDING_ABORTED);
> +          resolve(request.URI.spec);

I'm wondering if it would be reasonable to resolve the promise around line 80 (after the window.close) instead of resolving it here.

::: toolkit/components/extensions/ext-identity.js:92
(Diff revision 1)
> +      QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> +                                             Ci.nsISupportsWeakReference]),
>      };
>  
>      promiseDocumentLoaded(window.document).then(() => {
> -      window.gBrowser.addTabsProgressListener(wpl);
> +      let progress = window.gBrowser.selectedBrowser.webProgress;

Can we use window.gBrowser.addProgressListener here?

At a first glance it looks that it should basically have the same outcome for this fix (but I don't exclude that I'm not seeing something that you have seen while working on this fix) and it should make this change even simpler (e.g. there would be no need to add the QueryInterface property and we can also remove all the empty methods, because it looks that they will be called only if they exist: http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/browser/base/content/tabbrowser.xml#474-486)

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:185
(Diff revision 1)
>        },
>        "permissions": [
> +        "webRequest",
>          "identity",
>          "https://example.com/",
> +        "<all_urls>",

How about adding to the permissions "https://*.example.com/*" instead of "<all_urls>"?

Same for line 209 and 236.
(In reply to Luca Greco [:rpl] from comment #6)
> Comment on attachment 8926187 [details]

> I'm not setting the r+ on it yet, mainly to evaluate if we can switch this
> patch from `gBrowser.selectedBrowser.webProgress.addProgressListener` to
> `gBrowser.addProgressListener` before landing it 

That is how I originally did the patch but I was still able to reliably intermittently cause the failure in the test file (though less often than without the fix).  Looking into the tabbrowser progress listener code, there is a bunch of stuff going on in there, I decided to try it this way to weed out any possible side effect and it seems to solve the issue.  I'm not 100% convinced that there isn't a race condition lurking, but in my testing I could no longer reproduce.  I expect the webrequest listener to cause intermittent failures on treeherder if there is a race condition still.
Comment on attachment 8926187 [details]
Bug 1411646 prevent oauth redirect requests from happening,

https://reviewboard.mozilla.org/r/197456/#review203454

Thanks Shane, it looks great, it only needs the small fix described below.

r+me with the fix applied and green try

::: toolkit/components/extensions/ext-identity.js:84
(Diff revision 2)
> -      onStatusChange() {},
> -      onSecurityChange() {},
>      };
>  
>      promiseDocumentLoaded(window.document).then(() => {
> -      window.gBrowser.addTabsProgressListener(wpl);
> +      window.gBrowser.addProgressListener(wpl, Ci.nsIWebProgress.NOTIFY_ALL);

gBrowser.addProgressListener only support one parameter, it will not raise an exception but it is going to report it on the console:

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/tabbrowser.xml#3798-3803

(it is worth to mention that gBrowser passes NOTIFY_ALL when it subscribes its progress listeners internally: http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/tabbrowser.xml#5955,5958)
Attachment #8926187 - Flags: review?(lgreco) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e74c96a287ce
prevent oauth redirect requests from happening, r=rpl
https://hg.mozilla.org/mozilla-central/rev/e74c96a287ce
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Since this bug was fixed I realized that I can't use the launchWebFlow anymore.

Mozregression gives me: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2226bec0730a0e234d21b04d94677e49e4ff1ac&tochange=d32ac3e582b643d0938a3f094dd1b9d31dc8f065

You can reproduce the error using the notes plugin on the sync-v4 branch (npm i and then load the manifest.json in about:debugging)
Yea seems like there was a regression. I was on Nightly 58, tested the flow. Then upgraded to Nightly 59 and it stopped working.
Flags: needinfo?(mixedpuppy)
This landed on 58, are you saying it works fine on 58 still but not on 59?  Or did you just happen to test a version of 58 prior to this patch?
Flags: needinfo?(mixedpuppy) → needinfo?(vlad)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> This landed on 58, are you saying it works fine on 58 still but not on 59? 
> Or did you just happen to test a version of 58 prior to this patch?

Yeap exactly, I saw this bug, did the OAuth flow and it worked. Then I went to "About Firefox" pressed Update, restarted and it didn't work on 59.0a1 (2017-11-13) (64-bit). Could be a different regression?
Flags: needinfo?(vlad)
Blocks: 1416872
I have verified this issue on Mac 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Nightly(59.0a1-20171122220056) and it is no longer reproducible.

The following error is no longer displayed in the browser console:

"dee85c67bd72f3de1f0a0fb62a8fe9b9b1a166d7.extensions.allizom.org:443 uses an invalid security certificate.

The certificate is only valid for the following names:
  *.allizom.org, allizom.org
I have verified this issue on Mac 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Dev Edition(58.0b6-20171123161455) and it is no longer reproducible.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.