1494328 - identity.launchWebAuthFlow requires a redirectURI parameter in the URL despite that parameter being optional according to the spec

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Amndeep]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180920131237

Steps to reproduce:

Called `browser.identity.launchWebAuthFlow` with `interactive` set to true and `url` set to a url that has as parameters only `response_type` and `client_id`.


Actual results:

The promise was rejected with a message of "redirect_uri is invalid".


Expected results:

The OAuth window should've popped up.

Comment 1

[Amndeep]

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/parent/ext-identity.js at line 93 is where launchWebAuthFlow is defined.

In particular: ```
          try {
            redirectURI = new URL(url.searchParams.get("redirect_uri"));
            if (!redirectURI) {
              return Promise.reject({message: "redirect_uri is missing"});
            }
          } catch (e) {
            return Promise.reject({message: "redirect_uri is invalid"});
          }
```
is where it's failing since it expects a redirectURI parameter in the URL provided.

https://tools.ietf.org/html/rfc6749#section-4.2.1 is the official spec for implicit grants

In particular: ```
4.2.1.  Authorization Request

   The client constructs the request URI by adding the following
   parameters to the query component of the authorization endpoint URI
   using the "application/x-www-form-urlencoded" format, per Appendix B:

   response_type
         REQUIRED.  Value MUST be set to "token".

   client_id
         REQUIRED.  The client identifier as described in Section 2.2.

   redirect_uri
         OPTIONAL.  As described in Section 3.1.2.

   scope
         OPTIONAL.  The scope of the access request as described by
         Section 3.3.

   state
         RECOMMENDED.  An opaque value used by the client to maintain
         state between the request and callback.  The authorization
         server includes this value when redirecting the user-agent back
         to the client.  The parameter SHOULD be used for preventing
         cross-site request forgery as described in Section 10.12.
```
is where it states that the redirect_uri is optional.

Comment 2

[Martin Giger [:freaktechnik]]

Based on an IRC conversation, we've found out that it should fall back to using the generated redirect URI when none is passed inside the URL. That's also how Chrome seems to behave.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1494328 fix launchWebAuthFlow to use default redirect_uri

Some oauth services require the redirect uri be configured on their service,
and the reject the redirect_uri param if we send it.  Chrome works fine in this scenario,
but we have been requiring the redirect_uri be provided.  This addresses that requirement
by using our own default redirect url, which would be the url used to configure the
oauth service.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 9012269 [details]
Bug 1494328 fix launchWebAuthFlow to use default redirect_uri

Andrew Swan [:aswan] has approved the revision.

Comment 5

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d303c08dfc4de0f7097528213205c18fa0cbc0a0

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd95cb66e479
fix launchWebAuthFlow to use default redirect_uri r=aswan

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9012269 [details]
Bug 1494328 fix launchWebAuthFlow to use default redirect_uri

Approval Request Comment
[Feature/Bug causing the regression]: identity api
[User impact if declined]: some oauth services are unusable
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no, covered by tests
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk, limited to identity api in webextensions
[Why is the change risky/not risky?]: minimal change, moving some code to a child process and defaulting a value using pre-existing code
[String changes made/needed]: none

Comment 8

[Cristian Brindusan [:cbrindusan]]

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

Comment 9

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9012269 [details]
Bug 1494328 fix launchWebAuthFlow to use default redirect_uri

Minimal patch, fixes a P1, uplift approved for 63 beta 12, thanks.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3947b51293f6

Comment 11

[Cornel Ionce [:noni]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)

> [Is this code covered by automated tests?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no, covered by tests


Based on Shane's assessment and the automated coverage, marking as qe-verify-.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR60?

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1494328 esr60 fix launchWebAuthFlow to use default redirect_uri r=aswan a=pascalc

Some oauth services require the redirect uri be configured on their service,
and the reject the redirect_uri param if we send it.  Chrome works fine in this scenario,
but we have been requiring the redirect_uri be provided.  This addresses that requirement
by using our own default redirect url, which would be the url used to configure the
oauth service.

Comment 14

[Shane Caraveo (:mixedpuppy)]

attachment 9015269 [details] is the esr60 patch.

Comment 15

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26cdd1955e81f002692f8d3669fe186c4f44ae1c

Comment 16

[Shane Caraveo (:mixedpuppy)]

actual try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58b13b8c7cdc0464a61da533236921845d90531

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9015269 [details]
Bug 1494328 esr60 fix launchWebAuthFlow to use default redirect_uri r=aswan a=pascalc

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes the use of oauth with some oauth providers.  Since esr is so long lived, it would be good to get this is.

User impact if declined: some extensions will fail to work if they use certain oauth providers.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): covered by tests and is primarily relocating some code into the content process.

String or UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9015269 [details]
Bug 1494328 esr60 fix launchWebAuthFlow to use default redirect_uri r=aswan a=pascalc

Fixes issues with some OAuth providers. Approved for ESR 60.3.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/333276fac37c