Closed Bug 1494328 Opened 6 years ago Closed 6 years ago

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

Categories

(WebExtensions :: Request Handling, defect, P1)

62 Branch
defect

Tracking

(firefox-esr6063+ fixed, firefox62 wontfix, firefox63+ fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 --- fixed

People

(Reporter: amndeep.vass, Assigned: mixedpuppy)

Details

(Keywords: parity-chrome)

Attachments

(1 file, 1 obsolete file)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: parity-chrome
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.
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.
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Component: Untriaged → Request Handling
Priority: -- → P1
Comment on attachment 9012269 [details]
Bug 1494328 fix launchWebAuthFlow to use default redirect_uri

Andrew Swan [:aswan] has approved the revision.
Attachment #9012269 - Flags: review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cd95cb66e479
fix launchWebAuthFlow to use default redirect_uri r=aswan
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
Attachment #9012269 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/cd95cb66e479
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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.
Attachment #9012269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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-.
Flags: qe-verify-
QA Contact: ddurst
Do we need this on ESR60?
Flags: needinfo?(mixedpuppy)
Flags: in-testsuite+
QA Contact: ddurst
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.
attachment 9015269 [details] is the esr60 patch.
Flags: needinfo?(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
Attachment #9015269 - Flags: approval-mozilla-esr60?
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.
Attachment #9015269 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9015269 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.