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)
Tracking
(firefox-esr6063+ fixed, firefox62 wontfix, firefox63+ fixed, firefox64 fixed)
RESOLVED
FIXED
mozilla64
People
(Reporter: amndeep.vass, Assigned: mixedpuppy)
Details
(Keywords: parity-chrome)
Attachments
(1 file, 1 obsolete file)
46 bytes,
text/x-phabricator-request
|
aswan
:
review+
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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.
Updated•6 years ago
|
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
status-firefox62:
--- → affected
status-firefox63:
--- → affected
status-firefox64:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox63:
--- → ?
Component: Untriaged → Request Handling
Priority: -- → P1
Updated•6 years ago
|
Comment 4•6 years ago
|
||
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+
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d303c08dfc4de0f7097528213205c18fa0cbc0a0
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cd95cb66e479 fix launchWebAuthFlow to use default redirect_uri r=aswan
Assignee | ||
Comment 7•6 years ago
|
||
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?
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd95cb66e479
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 9•6 years ago
|
||
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+
Comment 10•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3947b51293f6
Comment 11•6 years ago
|
||
(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
Comment 12•6 years ago
|
||
Do we need this on ESR60?
Updated•6 years ago
|
QA Contact: ddurst
Assignee | ||
Comment 13•6 years ago
|
||
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 | ||
Comment 14•6 years ago
|
||
attachment 9015269 [details] is the esr60 patch.
tracking-firefox-esr60:
--- → ?
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26cdd1955e81f002692f8d3669fe186c4f44ae1c
Assignee | ||
Comment 16•6 years ago
|
||
actual try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58b13b8c7cdc0464a61da533236921845d90531
Assignee | ||
Comment 17•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/333276fac37c
Updated•5 years ago
|
Attachment #9015269 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•