From 217a1116a15714544e397583bf86f3f01c40532b Mon Sep 17 00:00:00 2001
From: Alex Anderson <191496+alxndrsn@users.noreply.github.com>
Date: Thu, 5 Dec 2024 13:43:08 +0300
Subject: [PATCH] oidc: pass state to IdP in authorization URL (#1327)

* pass `next` value to IdP as `state`, with additional random prefix
* this makes the "next" value more trusted
* fix support for Authentik and other IdPs which do not support auth URLs without `state` (https://github.com/goauthentik/authentik/pull/9735)
* may also improve support for IdPs which lack PKCE support (see: https://github.com/panva/openid-client/blob/1486c3a020af8d12449d1d6a4bdf4f2bf4d32b77/README.md#authorization-code-flow)

Closes #1134
Closes #1135
---
 lib/resources/oidc.js | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/resources/oidc.js b/lib/resources/oidc.js
index 5c9f9b1cc..f216a1056 100644
--- a/lib/resources/oidc.js
+++ b/lib/resources/oidc.js
@@ -43,7 +43,7 @@ const ONE_HOUR = 60 * 60 * 1000;
 // * https://bugzilla.mozilla.org/show_bug.cgi?id=1648993
 // * https://bugs.chromium.org/p/chromium/issues/detail?id=1056543
 const CODE_VERIFIER_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'ocv';
-const NEXT_COOKIE          = (HTTPS_ENABLED ? '__Secure-' : '') + 'next'; // eslint-disable-line no-multi-spaces
+const STATE_COOKIE         = (HTTPS_ENABLED ? '__Secure-' : '') + 'next'; // eslint-disable-line no-multi-spaces
 const callbackCookieProps = {
   httpOnly: true,
   secure: HTTPS_ENABLED,
@@ -95,6 +95,9 @@ const loaderTemplate = `
 `;
 parse(loaderTemplate); // caches template for future perf.
 
+const stateFor = next => [ generators.state(), Buffer.from(next).toString('base64url') ].join(':');
+const nextFrom = state => Buffer.from(state.split(':')[1], 'base64url').toString();
+
 module.exports = (service, endpoint) => {
   if (!isEnabled()) return;
 
@@ -105,17 +108,19 @@ module.exports = (service, endpoint) => {
 
       const code_challenge = generators.codeChallenge(code_verifier); // eslint-disable-line camelcase
 
+      const next = req.query.next ?? '';
+      const state = stateFor(next);
+
       const authUrl = client.authorizationUrl({
         scope: SCOPES.join(' '),
         resource: `${envDomain}/v1`,
         code_challenge,
         code_challenge_method: CODE_CHALLENGE_METHOD,
+        state,
       });
 
       res.cookie(CODE_VERIFIER_COOKIE, code_verifier, { ...callbackCookieProps, maxAge: ONE_HOUR });
-
-      const { next } = req.query;
-      if (next) res.cookie(NEXT_COOKIE, next, { ...callbackCookieProps, maxAge: ONE_HOUR });
+      res.cookie(STATE_COOKIE,         state,         { ...callbackCookieProps, maxAge: ONE_HOUR }); // eslint-disable-line no-multi-spaces
 
       redirect(307, authUrl);
     } catch (err) {
@@ -131,15 +136,15 @@ module.exports = (service, endpoint) => {
   service.get('/oidc/callback', endpoint.html(async (container, _, req, res) => {
     try {
       const code_verifier = req.cookies[CODE_VERIFIER_COOKIE]; // eslint-disable-line camelcase
-      const next          = req.cookies[NEXT_COOKIE];          // eslint-disable-line no-multi-spaces
+      const state         = req.cookies[STATE_COOKIE];         // eslint-disable-line no-multi-spaces
       res.clearCookie(CODE_VERIFIER_COOKIE, callbackCookieProps);
-      res.clearCookie(NEXT_COOKIE,          callbackCookieProps); // eslint-disable-line no-multi-spaces
+      res.clearCookie(STATE_COOKIE,         callbackCookieProps); // eslint-disable-line no-multi-spaces
 
       const client = await getClient();
 
       const params = client.callbackParams(req);
 
-      const tokenSet = await client.callback(getRedirectUri(), params, { response_type: RESPONSE_TYPE, code_verifier });
+      const tokenSet = await client.callback(getRedirectUri(), params, { response_type: RESPONSE_TYPE, code_verifier, state });
 
       const { access_token } = tokenSet;
 
@@ -157,7 +162,7 @@ module.exports = (service, endpoint) => {
 
       await initSession(container, req, res, user);
 
-      const nextPath = safeNextPathFrom(next);
+      const nextPath = safeNextPathFrom(nextFrom(state));
 
       // This redirect would be ideal, but breaks `SameSite: Secure` cookies.
       // return redirect(303, nextPath);
@@ -180,7 +185,7 @@ function errorToFrontend(req, res, errorCode) {
 
   loginUrl.searchParams.append('oidcError', errorCode);
 
-  const next = req.cookies[NEXT_COOKIE];
+  const next = nextFrom(req.cookies[STATE_COOKIE]);
   if (next && !Array.isArray(next)) loginUrl.searchParams.append('next', next);
 
   // Append query string manually, because Central Frontend expects search/hash