Malformed callback redirect when it already contains query string

[unrevised6419]

First of all, thanks for the library.

When I have a Provider callback that contains already a query string, the final redirect location seems to be malformed

Callback

http://localhost:3000/connect/keycloak/redirect?referrer=%2Fen%2Fsearch

Redirect Location

http://localhost:3000/connect/keycloak/redirect?referrer=%2Fen%2Fsearch?id_token=abcdefg

As you can see the query symbol (?) is already included in the Callback.
When the Redirect Location is created, the query symbol gets added again (see before id_token).

This issue creates problems when you try to parse the final URL

let url = new URL(`http://localhost:3000/connect/keycloak/redirect?referrer=%2Fen%2Fsearch?id_token=abcdefg`)

console.log(url.searchParams.get('id_token')) // null
console.log(url.searchParams.get('referrer')) // "/en/search?id_token=abcdefg"

This comes from this line

grant/lib/response.js

Line 108 in 500ab20

? `${provider.callback || '/'}?${qs.stringify(output)}`

Meanwhile, this gets fixed, I'm using this patch workaround

patches/grant+5.4.24.patch

diff --git a/node_modules/grant/lib/response.js b/node_modules/grant/lib/response.js
index e67a013..a52812b 100644
--- a/node_modules/grant/lib/response.js
+++ b/node_modules/grant/lib/response.js
@@ -105,7 +105,7 @@ var transport = ({provider, input, input:{params, state, session}, output}) => (
     ? output
 
     : (!provider.transport || provider.transport === 'querystring')
-    ? `${provider.callback || '/'}?${qs.stringify(output)}`
+    ? createCallback(provider.callback, output)
 
     : provider.transport === 'session'
     ? provider.callback
@@ -121,4 +121,16 @@ var transport = ({provider, input, input:{params, state, session}, output}) => (
   ),
 })
 
+var createCallback = (callback, output) => {
+  if (!callback) return `/?${qs.stringify(output)}`
+
+  var url = new URL(callback)
+  url.search = new URLSearchParams([
+    ...new URL(callback).searchParams,
+    ...new URLSearchParams(qs.stringify(output))
+  ]).toString()
+
+  return url.href
+}
+
 module.exports = {data, transport}

Redirect Location with patch

http://localhost:3000/connect/keycloak/redirect?referrer=%2Fen%2Fsearch&id_token=abcdef

[simov]

I see, thanks for sharing it. Not sure at what point precisely you add that additional querystring parameter to the callback URL (assuming that it is dynamic), but alternatively you can have a handler before the Grant middleware where you read that value and put it in session instead, you can even put it in the Grant session itself https://github.com/simov/grant/blob/master/examples/dynamic-state/express.js and then inside the final callback route you can read it back from the session.

[unrevised6419]

I'm using Strapi which uses grant/koa, and I don't have full control over it.

When the user clicks the login URL on web these redirects happen

web: the front face of the website
cms: Strapi instance
sso: Keycloack instance

We have these endpoints

• sso/login: sso login endpoint
• cms/keycloack: cms connect keycloack endpoint (does not create any user)
• cms/keycloack-callback: cms connect keycloack endpoint callback (does not create any user)
• web/callback: web endpoint that is called after login to call cms back to create the user
• cms/create: cms endpoint receives provider access token and creates the user

Callback

http://localhost:3000/connect/keycloak/redirect?referrer=%2Fen%2Fsearch&access_token=abcdef
                                       ^        ^                       ^ sso provider token
                                       ^        ^ user land in the end
                                       ^ this calls cms/create

1. web redirects to cms/keycloack
2. cms/keycloack goes to sso/login
3. User enters login info in the form, submits
4. sso/login redirects to cms/keycloack-callback
5. cms/keycloack-callback redirects to web/callback
6. web/callback calls cms/create
7. web/callback stores the session, and redirects to referrer from URL

I cannot change web/callback to anything arbitrary, that needs to be an endpoint that calls cms/create. That's why I put the real callback where user needs to land in the end in the query string.

I hope this was clear 😂

[simov]

Thanks for the thorough explanation, I might reconsider this feature 👍