From e887f95c5ab17f5449d9332e1883606b5cd1b692 Mon Sep 17 00:00:00 2001
From: Florent FAYOLLE <florent.fayolle@beta.gouv.fr>
Date: Mon, 13 Nov 2023 12:20:44 +0100
Subject: [PATCH 1/2] Fix OIDC redirects from team site to personal page after
 login #740

Also:
 - compare state in session and state passed through parameters
 (otherwise the state won't have any effect regarding the security).
 - delete the session even after an authentication failure
---
 app/server/lib/BrowserSession.ts |  2 ++
 app/server/lib/OIDCConfig.ts     | 30 ++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/app/server/lib/BrowserSession.ts b/app/server/lib/BrowserSession.ts
index 69b5d97b03..07136e3e4c 100644
--- a/app/server/lib/BrowserSession.ts
+++ b/app/server/lib/BrowserSession.ts
@@ -72,6 +72,8 @@ export interface SessionObj {
   oidc?: {
     // codeVerifier is used during OIDC authentication, to protect against attacks like CSRF.
     codeVerifier?: string;
+    state?: string;
+    targetUrl?: string;
   }
 }
 
diff --git a/app/server/lib/OIDCConfig.ts b/app/server/lib/OIDCConfig.ts
index d5436169f3..18de53f9f6 100644
--- a/app/server/lib/OIDCConfig.ts
+++ b/app/server/lib/OIDCConfig.ts
@@ -85,15 +85,18 @@ export class OIDCConfig {
   }
 
   public async handleCallback(sessions: Sessions, req: express.Request, res: express.Response): Promise<void> {
+    const mreq = req as RequestWithLogin;
     try {
       const params = this._client.callbackParams(req);
-      const { state } = params;
+      const { state, targetUrl } = mreq.session?.oidc ?? {};
       if (!state) {
         throw new Error('Login or logout failed to complete');
       }
 
       const codeVerifier = await this._retrieveCodeVerifierFromSession(req);
 
+      // The callback function will compare the state present in the params and the one we retrieved from the session.
+      // If they don't match, it will throw an error.
       const tokenSet = await this._client.callback(
         this._redirectUrl,
         params,
@@ -108,17 +111,22 @@ export class OIDCConfig {
         profile,
       }));
 
-      res.redirect('/');
+      delete mreq.session.oidc;
+      res.redirect(targetUrl ?? '/');
     } catch (err) {
-      log.error(`OIDC callback failed: ${err.message}`);
-      res.status(500).send(`OIDC callback failed: ${err.message}`);
+      log.error(`OIDC callback failed: ${err.stack}`);
+      // Delete the session data even if the login failed.
+      // This way, we prevent several login attempts.
+      //
+      // Also session deletion must be done before sending the response.
+      delete mreq.session.oidc;
+      res.status(500).send(`OIDC callback failed.`);
     }
   }
 
-  public async getLoginRedirectUrl(req: express.Request): Promise<string> {
-    const codeVerifier = await this._generateAndStoreCodeVerifier(req);
+  public async getLoginRedirectUrl(req: express.Request, targetUrl: URL): Promise<string> {
+    const { codeVerifier, state } = await this._generateAndStoreConnectionInfo(req, targetUrl.href);
     const codeChallenge = generators.codeChallenge(codeVerifier);
-    const state = generators.state();
 
     const authUrl = this._client.authorizationUrl({
       scope: process.env.GRIST_OIDC_IDP_SCOPES || 'openid email profile',
@@ -135,15 +143,18 @@ export class OIDCConfig {
     });
   }
 
-  private async _generateAndStoreCodeVerifier(req: express.Request) {
+  private async _generateAndStoreConnectionInfo(req: express.Request, targetUrl: string) {
     const mreq = req as RequestWithLogin;
     if (!mreq.session) { throw new Error('no session available'); }
     const codeVerifier = generators.codeVerifier();
+    const state = generators.state();
     mreq.session.oidc = {
       codeVerifier,
+      state,
+      targetUrl
     };
 
-    return codeVerifier;
+    return { codeVerifier, state };
   }
 
   private async _retrieveCodeVerifierFromSession(req: express.Request) {
@@ -151,7 +162,6 @@ export class OIDCConfig {
     if (!mreq.session) { throw new Error('no session available'); }
     const codeVerifier = mreq.session.oidc?.codeVerifier;
     if (!codeVerifier) { throw new Error('Login is stale'); }
-    delete mreq.session.oidc?.codeVerifier;
     return codeVerifier;
   }
 

From aa40dc7ad3794c7037b9889816c38ef20170c04a Mon Sep 17 00:00:00 2001
From: Florent FAYOLLE <florent.fayolle@beta.gouv.fr>
Date: Mon, 13 Nov 2023 12:24:16 +0100
Subject: [PATCH 2/2] More logs for OIDC #740

---
 app/server/lib/OIDCConfig.ts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/server/lib/OIDCConfig.ts b/app/server/lib/OIDCConfig.ts
index 18de53f9f6..b08892365a 100644
--- a/app/server/lib/OIDCConfig.ts
+++ b/app/server/lib/OIDCConfig.ts
@@ -78,6 +78,7 @@ export class OIDCConfig {
       redirect_uris: [ this._redirectUrl ],
       response_types: [ 'code' ],
     });
+    log.info(`OIDCConfig: initialized with issuer ${issuerUrl}`);
   }
 
   public addEndpoints(app: express.Application, sessions: Sessions): void {
@@ -105,6 +106,7 @@ export class OIDCConfig {
 
       const userInfo = await this._client.userinfo(tokenSet);
       const profile = this._makeUserProfileFromUserInfo(userInfo);
+      log.info(`OIDCConfig: got OIDC response for ${profile.email} (${profile.name}) redirecting to ${targetUrl}`);
 
       const scopedSession = sessions.getOrCreateSessionFromRequest(req);
       await scopedSession.operateOnScopedSession(req, async (user) => Object.assign(user, {