Improve OIDC security by using state and nonce variables & fix referrer redirect when using OIDC

Author: louise-davies

src/authentication/baseAPIAuthProvider.tsx

@@ -144,6 +144,62 @@ export default abstract class BaseAPIAuthProvider extends BaseAuthProvider {
       });
   }
 
+  public verifyOIDCStateParam(stateToTest: string | null): boolean {
+    const oidcState = sessionStorage.getItem('oidcState');
+
+    // TODO: should we verify if session storage has been cleared?
+    if (oidcState === null) return true;
+
+    if (oidcState === stateToTest) return true;
+
+    document.dispatchEvent(
+      new CustomEvent('scigateway', {
+        detail: {
+          type: NotificationType,
+          payload: {
+            message:
+              'It is not possible to authenticate you at the moment. Please, try again later.',
+            severity: 'error',
+          },
+        },
+      })
+    );
+
+    return false;
+  }
+
+  public async verifyOIDCNonce(accessToken: string): Promise<boolean> {
+    const oidcNonce = sessionStorage.getItem('oidcNonce');
+
+    // TODO: should we verify if session storage has been cleared?
+    if (oidcNonce === null) return true;
+
+    const encryptedOIDCNonce =
+      await generateCodeChallengeFromVerifier(oidcNonce);
+
+    const { nonce: nonceToTest } = JSON.parse(parseJwt(accessToken));
+
+    if (encryptedOIDCNonce === nonceToTest) {
+      return true;
+    }
+
+    log.error('Nonce verification failed');
+    document.dispatchEvent(
+      new CustomEvent('scigateway', {
+        detail: {
+          type: NotificationType,
+          payload: {
+            message:
+              'It is not possible to authenticate you at the moment. Please, try again later.',
+            severity: 'error',
+          },
+        },
+      })
+    );
+
+    return false;
+  }
+
   async pkceToken(
     token: string,
     oidcProvider: InitialisedOIDCProvider
@@ -191,7 +247,15 @@ export default abstract class BaseAPIAuthProvider extends BaseAuthProvider {
         id_token = await this.nonPKCEToken(token, oidcProvider);
       }
 
-      // TODO: sometimes login request fails here - maybe because it was too fast?
+      if (!(await this.verifyOIDCNonce(id_token)))
+        throw Error('Nonce verification failed');
+
+      // TODO: sometimes login request fails here because it was too fast
+      // aka JWT iat is not yet "in the past"
+      // need to talk to backend people about it
+      await new Promise<void>((resolve, _reject) => {
+        setTimeout(() => resolve(), 1_000);
+      });
 
       const { data: jwt } = await axios.post(
         `${this.authUrl}/oidc_login/${oidcProvider.provider_id}`,
@@ -206,6 +270,8 @@ export default abstract class BaseAPIAuthProvider extends BaseAuthProvider {
       this.storeToken(jwt);
       sessionStorage.removeItem('codeVerifier');
       sessionStorage.removeItem('oidcProviderId');
+      sessionStorage.removeItem('oidcState');
+      sessionStorage.removeItem('oidcNonce');
       const payload: {
         sessionId: string;
         username: string;
@@ -220,16 +286,28 @@ export default abstract class BaseAPIAuthProvider extends BaseAuthProvider {
     }
   }
 
-  public async setupOIDC(oidcProvider: InitialisedOIDCProvider): Promise<void> {
+  public async setupOIDC(
+    oidcProvider: InitialisedOIDCProvider,
+    referrer?: string
+  ): Promise<void> {
+    console.log('setupOIDC called');
     let codeChallenge: string | undefined;
     if (oidcProvider.pkce) {
       const codeVerifier = generateCodeVerifier();
       sessionStorage.setItem('codeVerifier', codeVerifier);
       codeChallenge = await generateCodeChallengeFromVerifier(codeVerifier);
     }
     sessionStorage.setItem('oidcProviderId', oidcProvider.provider_id);
+    const state = generateCodeVerifier();
+    sessionStorage.setItem('oidcState', state);
+    const nonce = generateCodeVerifier();
+    const encryptedNonce = await generateCodeChallengeFromVerifier(nonce);
+
+    sessionStorage.setItem('oidcNonce', nonce);
+
+    if (referrer) sessionStorage.setItem('referrer', referrer);
 
-    this.redirectUrl = `${oidcProvider.authorization_endpoint}?client_id=${oidcProvider.client_id}&redirect_uri=${window.location.origin}/login&response_type=code${oidcProvider.pkce ? `&code_challenge_method=S256&code_challenge=${codeChallenge}` : ''}&scope=${oidcProvider.scope}`;
+    this.redirectUrl = `${oidcProvider.authorization_endpoint}?client_id=${oidcProvider.client_id}&redirect_uri=${window.location.origin}/login&response_type=code${oidcProvider.pkce ? `&code_challenge_method=S256&code_challenge=${codeChallenge}` : ''}&scope=${oidcProvider.scope}&state=${state}&nonce=${encryptedNonce}`;
   }
 
   public async initialiseOIDCProviders(): Promise<InitialisedOIDCProvider[]> {

src/authentication/icatAuthProvider.tsx

@@ -91,14 +91,15 @@ export default class ICATAuthProvider extends BaseAPIAuthProvider {
 
   public async setAuthenticator(
     mnemonic: string,
-    disableSideEffects?: boolean
+    disableSideEffects?: boolean,
+    referrer?: string
   ): Promise<void> {
     const oidcProvider = this.oidcProviders.find(
       (op) => op.provider_id === mnemonic
     );
     this.mnemonic = mnemonic;
     if (oidcProvider && !disableSideEffects) {
-      await this.setupOIDC(oidcProvider);
+      await this.setupOIDC(oidcProvider, referrer);
     }
     return Promise.resolve();
   }
@@ -133,7 +134,10 @@ export default class ICATAuthProvider extends BaseAPIAuthProvider {
     if (oidcProvider) {
       const params = new URLSearchParams(password);
       const code = params.get('code');
-      if (!code) {
+
+      const state = params.get('state');
+
+      if (!code || !this.verifyOIDCStateParam(state)) {
         this.logOut();
         return Promise.resolve();
       }

src/authentication/oidcAuthProvider.tsx

@@ -37,15 +37,16 @@ export default class OIDCAuthProvider extends BaseAPIAuthProvider {
 
   public async setAuthenticator(
     provider: string,
-    disableSideEffects?: boolean
+    disableSideEffects?: boolean,
+    referrer?: string
   ): Promise<void> {
     const oidcProvider = this.oidcProviders.find(
       (oidc) => oidc.provider_id === provider
     );
     if (oidcProvider) {
       this.oidcProvider = oidcProvider;
       if (!disableSideEffects) {
-        await this.setupOIDC(oidcProvider);
+        await this.setupOIDC(oidcProvider, referrer);
       }
     } else {
       log.error(
@@ -59,7 +60,9 @@ export default class OIDCAuthProvider extends BaseAPIAuthProvider {
     const params = new URLSearchParams(password);
     const code = params.get('code');
 
-    if (!code || !this.oidcProvider) {
+    const state = params.get('state');
+
+    if (!code || !this.oidcProvider || !this.verifyOIDCStateParam(state)) {
       this.logOut();
       return Promise.resolve();
     }

src/authentication/passwordAndOIDCAuthProvider.tsx

@@ -42,7 +42,8 @@ export default class PasswordAndOIDCAuthProvider extends BaseAPIAuthProvider {
 
   public async setAuthenticator(
     authenticatorKey: string,
-    disableSideEffects?: boolean
+    disableSideEffects?: boolean,
+    referrer?: string
   ): Promise<void> {
     const newAuthenticator = this.authenticators.find(
       (a) => a.key === authenticatorKey
@@ -61,7 +62,7 @@ export default class PasswordAndOIDCAuthProvider extends BaseAPIAuthProvider {
           (oidc) => oidc.provider_id === this.authenticator?.key
         );
         if (oidcProvider) {
-          await this.setupOIDC(oidcProvider);
+          await this.setupOIDC(oidcProvider, referrer);
         } else {
           log.error(
             `Can't find oidc provider matching the specified authenticator: ${authenticatorKey}`
@@ -81,7 +82,9 @@ export default class PasswordAndOIDCAuthProvider extends BaseAPIAuthProvider {
         (op) => op.provider_id === this.authenticator?.key
       );
 
-      if (!code || !oidcProvider) {
+      const state = params.get('state');
+
+      if (!code || !oidcProvider || !this.verifyOIDCStateParam(state)) {
         this.logOut();
         return Promise.resolve();
       }

src/loginPage/loginPage.component.tsx

@@ -340,10 +340,11 @@ export const LoginPageComponent = (
       setAuthenticator(newAuthenticator);
       props.auth.provider.setAuthenticator?.(
         newAuthenticator,
-        disableSideEffects
+        disableSideEffects,
+        location.state?.referrer
       );
     },
-    [props.auth.provider]
+    [location.state?.referrer, props.auth.provider]
   );
 
   React.useEffect(() => {

src/routing/routing.component.tsx

@@ -124,6 +124,14 @@ export const UnauthorisedPlugin = PluginPlaceHolder;
 // Prevents the component from updating when the draw is opened/closed
 export const AuthorisedAdminPage = withAuth(true)(AdminPage);
 
+const popSessionStorageItem = (
+  key: string
+): ReturnType<typeof sessionStorage.getItem> => {
+  const result = sessionStorage.getItem(key);
+  sessionStorage.removeItem(key);
+  return result;
+};
+
 const Routing: React.FC<RoutingProps> = (props: RoutingProps) => {
   const adminRoutes = getAdminRoutes({ plugins: props.plugins });
   // only set to false if we're on a plugin route i.e. not a scigateway route
@@ -248,8 +256,10 @@ const Routing: React.FC<RoutingProps> = (props: RoutingProps) => {
               to={{
                 pathname:
                   prevUserIsLoggedIn === false && props.userIsLoggedIn
-                    ? (props.location.state as { referrer?: string })
-                        ?.referrer ?? scigatewayRoutes.home
+                    ? (((props.location.state as { referrer?: string })
+                        ?.referrer ||
+                        popSessionStorageItem('referrer')) ??
+                      scigatewayRoutes.home)
                     : scigatewayRoutes.logout,
                 state: { referrer: props.location.pathname },
               }}

src/state/state.types.tsx

@@ -111,7 +111,8 @@ export interface AuthProvider {
   }[];
   setAuthenticator?: (
     key: string,
-    disableSideEffects?: boolean
+    disableSideEffects?: boolean,
+    referrer?: string
   ) => Promise<void>;
   getAuthenticator?: () => string;
   initialise?: () => Promise<void>;