From 685f76d91367401280b22264fb07b27be6045a70 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Mon, 3 Aug 2020 15:33:32 +0200
Subject: [PATCH 01/10] Encode `env` in OAuth flow into the state parameter.
 refs #1775

`Env` passed as a separate parameter in the request might break the flow
of the authorization flow. Spotify considers the `env` as a first class
abstraction and use it across their application stack.
To ensure both the existing flow and newer flows are supported, use the
`state` parameter to encode an object, consisting of the `nonce` and the
`env` which is then passed to the authorization server. This parameter
is returned to the application in the callbackURL
---
 .../src/lib/EnvironmentHandler.ts             | 17 +++++++-
 plugins/auth-backend/src/lib/OAuthProvider.ts | 43 ++++++++++++++++++-
 .../auth-backend/src/providers/factories.ts   | 11 ++++-
 .../src/providers/oauth2/provider.ts          |  4 +-
 .../src/providers/saml/provider.ts            |  4 ++
 plugins/auth-backend/src/providers/types.ts   |  9 ++++
 6 files changed, 83 insertions(+), 5 deletions(-)

diff --git a/plugins/auth-backend/src/lib/EnvironmentHandler.ts b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
index 3eab4e793c..c9a182fc05 100644
--- a/plugins/auth-backend/src/lib/EnvironmentHandler.ts
+++ b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
@@ -25,13 +25,28 @@ export class EnvironmentHandler implements AuthProviderRouteHandlers {
   constructor(
     private readonly providerId: string,
     private readonly providers: EnvironmentHandlers,
+    private readonly envIdentifier: (req: express.Request) => string,
   ) {}
 
+  /* Return the value of `env` key, if it is exists, encoded within
+     the `state` parameter in the request
+  */
+  private getEnv(stateParams: Array<string>): string {
+    const envParams = stateParams.filter(
+      param => param.split('=')[0] === 'env',
+    );
+
+    if (envParams.length > 0) {
+      return envParams[0].split('=')[1];
+    }
+    return '';
+  }
+
   private getProviderForEnv(
     req: express.Request,
     res: express.Response,
   ): AuthProviderRouteHandlers | undefined {
-    const env = req.query.env?.toString();
+    const env: string = this.envIdentifier(req);
 
     if (env && this.providers.hasOwnProperty(env)) {
       return this.providers[env];
diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 7a4a9e2537..ca8872cd95 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -39,6 +39,21 @@ export type Options = {
   tokenIssuer: TokenIssuer;
 };
 
+/* Return the value of `env` key, if it is exists, encoded within
+     the `state` parameter in the request
+  */
+const getEnv = (stateParams: Array<string>): string => {
+  const envParams = stateParams.filter(param => param.split('=')[0] === 'env');
+
+  if (envParams.length > 0) {
+    return envParams[0].split('=')[1];
+  }
+  return '';
+};
+
+const readState = (stateString: string): Array<string> => {
+  return decodeURIComponent(stateString).split('&');
+};
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
   const stateNonce = req.query.state;
@@ -103,6 +118,7 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
   async start(req: express.Request, res: express.Response): Promise<void> {
     // retrieve scopes from request
     const scope = req.query.scope?.toString() ?? '';
+    const env = req.query.env?.toString() ?? '';
 
     if (this.options.persistScopes) {
       this.setScopesCookie(res, scope);
@@ -112,9 +128,21 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     // set a nonce cookie before redirecting to oauth provider
     this.setNonceCookie(res, nonce);
 
+    // const stateObject: {nonce: string,
+    //                    env: string}
+
+    const stateObject = { nonce: nonce, env: env };
+
+    const state = Object.keys(stateObject)
+      .map(
+        key =>
+          `${encodeURIComponent(key)}=${encodeURIComponent(stateObject[key])}`,
+      )
+      .join('&');
+
     const queryParameters = {
       scope,
-      state: nonce,
+      state: state,
     };
 
     const { url, status } = await this.providerHandlers.start(
@@ -224,6 +252,19 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     }
   }
 
+  identifyEnv(req: express.Request): string {
+    const reqEnv = req.query.env?.toString();
+    if (reqEnv) {
+      return reqEnv;
+    }
+    const stateParam = req.query.state?.toString();
+    if (!stateParam) {
+      return '';
+    }
+    const env = getEnv(readState(stateParam));
+    return env;
+  }
+
   /**
    * If the response from the OAuth provider includes a Backstage identity, we
    * make sure it's populated with all the information we can derive from the user ID.
diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts
index dcce98a986..f4144ddc6c 100644
--- a/plugins/auth-backend/src/providers/factories.ts
+++ b/plugins/auth-backend/src/providers/factories.ts
@@ -15,6 +15,7 @@
  */
 
 import Router from 'express-promise-router';
+import express from 'express';
 import { Logger } from 'winston';
 import { TokenIssuer } from '../identity';
 import { createGithubProvider } from './github';
@@ -54,16 +55,24 @@ export const createAuthProviderRouter = (
   const router = Router();
   const envs = providerConfig.keys();
   const envProviders: EnvironmentHandlers = {};
+  let envIdentifier: (req: express.Request) => string;
 
   for (const env of envs) {
     const envConfig = providerConfig.getConfig(env);
     const provider = factory(globalConfig, env, envConfig, logger, issuer);
     if (provider) {
       envProviders[env] = provider;
+      if (envIdentifier === undefined) {
+        envIdentifier = provider.identifyEnv;
+      }
     }
   }
 
-  const handler = new EnvironmentHandler(providerId, envProviders);
+  const handler = new EnvironmentHandler(
+    providerId,
+    envProviders,
+    envIdentifier,
+  );
 
   router.get('/start', handler.start.bind(handler));
   router.get('/handler/frame', handler.frameHandler.bind(handler));
diff --git a/plugins/auth-backend/src/providers/oauth2/provider.ts b/plugins/auth-backend/src/providers/oauth2/provider.ts
index 967fd011f0..c03f7c4371 100644
--- a/plugins/auth-backend/src/providers/oauth2/provider.ts
+++ b/plugins/auth-backend/src/providers/oauth2/provider.ts
@@ -143,7 +143,7 @@ export class OAuth2AuthProvider implements OAuthProviderHandlers {
 
 export function createOAuth2Provider(
   { baseUrl }: AuthProviderConfig,
-  env: string,
+  _: string,
   envConfig: Config,
   logger: Logger,
   tokenIssuer: TokenIssuer,
@@ -153,7 +153,7 @@ export function createOAuth2Provider(
   const appOrigin = envConfig.getString('appOrigin');
   const clientID = envConfig.getString('clientId');
   const clientSecret = envConfig.getString('clientSecret');
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;
+  const callbackURL = `${baseUrl}/${providerId}/handler/frame`;
   const authorizationURL = envConfig.getString('authorizationURL');
   const tokenURL = envConfig.getString('tokenURL');
 
diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts
index 3ddfba4bb0..605a46ece4 100644
--- a/plugins/auth-backend/src/providers/saml/provider.ts
+++ b/plugins/auth-backend/src/providers/saml/provider.ts
@@ -106,6 +106,10 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers {
   async logout(_req: express.Request, res: express.Response): Promise<void> {
     res.send('noop');
   }
+
+  identifyEnv(_req: express.Request): string {
+    return '';
+  }
 }
 
 type SAMLProviderOptions = {
diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts
index 93d459bb0d..c2570b8768 100644
--- a/plugins/auth-backend/src/providers/types.ts
+++ b/plugins/auth-backend/src/providers/types.ts
@@ -203,6 +203,15 @@ export interface AuthProviderRouteHandlers {
    * @param {express.Response} res
    */
   logout?(req: express.Request, res: express.Response): Promise<void>;
+
+  /**
+   *(Optional) A method to identify the environment Context of the Request
+   *
+   *Request
+   *- contains the environment context information encoded in the request
+   *  @param {express.Request} req
+   */
+  identifyEnv?(req: express.Request): string;
 }
 
 export type AuthProviderFactory = (

From 081fe6045352e064af44fdf8cc7c63b88644e1e0 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Mon, 3 Aug 2020 15:53:22 +0200
Subject: [PATCH 02/10] Refactor: parse state query param to read nonce

The `state` was modified to be an encoded object of `nonce` and `env`.
When verifying the Nonce value in the callback from the authorization
server, parse the state parameter string to read the right value of
nonce
---
 plugins/auth-backend/src/lib/OAuthProvider.ts | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index ca8872cd95..d92d31c4ff 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -42,8 +42,13 @@ export type Options = {
 /* Return the value of `env` key, if it is exists, encoded within
      the `state` parameter in the request
   */
-const getEnv = (stateParams: Array<string>): string => {
-  const envParams = stateParams.filter(param => param.split('=')[0] === 'env');
+const readStateParameter = (
+  stateParams: Array<string>,
+  parameter: string,
+): string => {
+  const envParams = stateParams.filter(
+    param => param.split('=')[0] === parameter,
+  );
 
   if (envParams.length > 0) {
     return envParams[0].split('=')[1];
@@ -54,9 +59,11 @@ const getEnv = (stateParams: Array<string>): string => {
 const readState = (stateString: string): Array<string> => {
   return decodeURIComponent(stateString).split('&');
 };
+
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
-  const stateNonce = req.query.state;
+  const state = req.query.state;
+  const stateNonce = readStateParameter(state, 'nonce');
 
   if (!cookieNonce) {
     throw new Error('Auth response is missing cookie nonce');
@@ -257,11 +264,11 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     if (reqEnv) {
       return reqEnv;
     }
-    const stateParam = req.query.state?.toString();
-    if (!stateParam) {
+    const stateParams = req.query.state?.toString();
+    if (!stateParams) {
       return '';
     }
-    const env = getEnv(readState(stateParam));
+    const env = readStateParameter(readState(stateParams), 'env');
     return env;
   }
 

From 3e5dfbc5f62352adaedb69d01e8e68d85fdaee67 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Mon, 3 Aug 2020 17:42:48 +0200
Subject: [PATCH 03/10] Refactor: move encoding state parameters into a
 separate function

---
 plugins/auth-backend/src/lib/OAuthProvider.ts | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index d92d31c4ff..4dfed10016 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -60,9 +60,19 @@ const readState = (stateString: string): Array<string> => {
   return decodeURIComponent(stateString).split('&');
 };
 
+export const encodeState = (stateObject: object): string => {
+  const vals = Object.keys(stateObject).map(
+    key =>
+      `${encodeURIComponent(key)}=${encodeURIComponent(
+        Reflect.get(stateObject, key),
+      )}`,
+  );
+
+  return vals.join('&');
+};
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
-  const state = req.query.state;
+  const state: Array<string> = readState(req.query.state?.toString() ?? '');
   const stateNonce = readStateParameter(state, 'nonce');
 
   if (!cookieNonce) {
@@ -135,21 +145,12 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     // set a nonce cookie before redirecting to oauth provider
     this.setNonceCookie(res, nonce);
 
-    // const stateObject: {nonce: string,
-    //                    env: string}
-
     const stateObject = { nonce: nonce, env: env };
-
-    const state = Object.keys(stateObject)
-      .map(
-        key =>
-          `${encodeURIComponent(key)}=${encodeURIComponent(stateObject[key])}`,
-      )
-      .join('&');
+    const stateParameter = encodeState(stateObject);
 
     const queryParameters = {
       scope,
-      state: state,
+      state: stateParameter,
     };
 
     const { url, status } = await this.providerHandlers.start(

From 51fad0346066f2c27f38005d75ae61500b29d9c0 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Mon, 3 Aug 2020 17:43:16 +0200
Subject: [PATCH 04/10] Tests: update tests for OAuthProvider to handle

Update OAuthProvider tests to ensure that changes to nonce and
environment handling do not cause regressions
---
 plugins/auth-backend/src/lib/OAuthProvider.test.ts | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.test.ts b/plugins/auth-backend/src/lib/OAuthProvider.test.ts
index 3d631bd4ac..8c6c902b72 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.test.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.test.ts
@@ -21,6 +21,7 @@ import {
   THOUSAND_DAYS_MS,
   TEN_MINUTES_MS,
   verifyNonce,
+  encodeState,
   OAuthProvider,
 } from './OAuthProvider';
 import { WebMessageResponse, OAuthProviderHandlers } from '../providers/types';
@@ -43,10 +44,11 @@ const mockResponseData = {
 describe('OAuthProvider Utils', () => {
   describe('verifyNonce', () => {
     it('should throw error if cookie nonce missing', () => {
+      const state = { nonce: 'NONCE', env: 'development' };
       const mockRequest = ({
         cookies: {},
         query: {
-          state: 'NONCE',
+          state: encodeState(state),
         },
       } as unknown) as express.Request;
       expect(() => {
@@ -67,12 +69,13 @@ describe('OAuthProvider Utils', () => {
     });
 
     it('should throw error if nonce mismatch', () => {
+      const state = { nonce: 'NONCEB', env: 'development' };
       const mockRequest = ({
         cookies: {
           'providera-nonce': 'NONCEA',
         },
         query: {
-          state: 'NONCEB',
+          state: encodeState(state),
         },
       } as unknown) as express.Request;
       expect(() => {
@@ -81,12 +84,13 @@ describe('OAuthProvider Utils', () => {
     });
 
     it('should not throw any error if nonce matches', () => {
+      const state = { nonce: 'NONCE', env: 'development' };
       const mockRequest = ({
         cookies: {
           'providera-nonce': 'NONCE',
         },
         query: {
-          state: 'NONCE',
+          state: encodeState(state),
         },
       } as unknown) as express.Request;
       expect(() => {
@@ -249,12 +253,13 @@ describe('OAuthProvider', () => {
       disableRefresh: false,
     });
 
+    const state = { nonce: 'nonce', env: 'development' };
     const mockRequest = ({
       cookies: {
         'test-provider-nonce': 'nonce',
       },
       query: {
-        state: 'nonce',
+        state: encodeState(state),
       },
     } as unknown) as express.Request;
 

From 2d20cf389cccd51953d1b07f565ef140d9915738 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Tue, 4 Aug 2020 14:45:03 +0200
Subject: [PATCH 05/10] Refactor: Address PR Comments

Create a new `OAuthState` type for storing the fields we want in the
`state` parameter.

Update the `encodeState` and `readState` fns to use the new `OAuthState`
type.

Update tests to reflect the new type
---
 .../src/lib/EnvironmentHandler.ts             | 14 -------
 plugins/auth-backend/src/lib/OAuthProvider.ts | 41 ++++++++++---------
 .../auth-backend/src/providers/factories.ts   | 10 +++--
 .../src/providers/saml/provider.ts            |  2 +-
 plugins/auth-backend/src/providers/types.ts   |  7 ++++
 5 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/plugins/auth-backend/src/lib/EnvironmentHandler.ts b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
index c9a182fc05..d2fe25c947 100644
--- a/plugins/auth-backend/src/lib/EnvironmentHandler.ts
+++ b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
@@ -28,20 +28,6 @@ export class EnvironmentHandler implements AuthProviderRouteHandlers {
     private readonly envIdentifier: (req: express.Request) => string,
   ) {}
 
-  /* Return the value of `env` key, if it is exists, encoded within
-     the `state` parameter in the request
-  */
-  private getEnv(stateParams: Array<string>): string {
-    const envParams = stateParams.filter(
-      param => param.split('=')[0] === 'env',
-    );
-
-    if (envParams.length > 0) {
-      return envParams[0].split('=')[1];
-    }
-    return '';
-  }
-
   private getProviderForEnv(
     req: express.Request,
     res: express.Response,
diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 4dfed10016..608e7f2075 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -22,6 +22,7 @@ import {
   OAuthProviderHandlers,
   WebMessageResponse,
   BackstageIdentity,
+  OAuthState,
 } from '../providers/types';
 import { InputError } from '@backstage/backend-common';
 import { TokenIssuer } from '../identity';
@@ -39,27 +40,26 @@ export type Options = {
   tokenIssuer: TokenIssuer;
 };
 
-/* Return the value of `env` key, if it is exists, encoded within
-     the `state` parameter in the request
-  */
-const readStateParameter = (
-  stateParams: Array<string>,
-  parameter: string,
-): string => {
-  const envParams = stateParams.filter(
-    param => param.split('=')[0] === parameter,
-  );
-
-  if (envParams.length > 0) {
-    return envParams[0].split('=')[1];
+const readState = (stateString: string): OAuthState => {
+  try {
+    const state = JSON.parse(decodeURIComponent(stateString));
+    return {
+      nonce: state.nonce,
+      env: state.env,
+    };
+  } catch (e) {
+    return {
+      nonce: undefined,
+      env: undefined,
+    };
   }
-  return '';
 };
 
-const readState = (stateString: string): Array<string> => {
-  return decodeURIComponent(stateString).split('&');
+export const encodeState = (state: OAuthState): string => {
+  return encodeURIComponent(JSON.stringify(state));
 };
 
+/*
 export const encodeState = (stateObject: object): string => {
   const vals = Object.keys(stateObject).map(
     key =>
@@ -70,10 +70,11 @@ export const encodeState = (stateObject: object): string => {
 
   return vals.join('&');
 };
+*/
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
-  const state: Array<string> = readState(req.query.state?.toString() ?? '');
-  const stateNonce = readStateParameter(state, 'nonce');
+  const state: OAuthState = readState(req.query.state?.toString() ?? '');
+  const stateNonce = state.nonce;
 
   if (!cookieNonce) {
     throw new Error('Auth response is missing cookie nonce');
@@ -269,8 +270,8 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     if (!stateParams) {
       return '';
     }
-    const env = readStateParameter(readState(stateParams), 'env');
-    return env;
+    const env = readState(stateParams).env;
+    return env ?? '';
   }
 
   /**
diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts
index f4144ddc6c..403b39312c 100644
--- a/plugins/auth-backend/src/providers/factories.ts
+++ b/plugins/auth-backend/src/providers/factories.ts
@@ -55,19 +55,21 @@ export const createAuthProviderRouter = (
   const router = Router();
   const envs = providerConfig.keys();
   const envProviders: EnvironmentHandlers = {};
-  let envIdentifier: (req: express.Request) => string;
+  let envIdentifier: ((req: express.Request) => string) | undefined;
 
   for (const env of envs) {
     const envConfig = providerConfig.getConfig(env);
     const provider = factory(globalConfig, env, envConfig, logger, issuer);
     if (provider) {
       envProviders[env] = provider;
-      if (envIdentifier === undefined) {
-        envIdentifier = provider.identifyEnv;
-      }
+      envIdentifier = provider.identifyEnv;
     }
   }
 
+  if (typeof envIdentifier === 'undefined') {
+    throw Error(`No envIdentifier provided for '${providerId}'`);
+  }
+
   const handler = new EnvironmentHandler(
     providerId,
     envProviders,
diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts
index 605a46ece4..9db88c8fa9 100644
--- a/plugins/auth-backend/src/providers/saml/provider.ts
+++ b/plugins/auth-backend/src/providers/saml/provider.ts
@@ -107,7 +107,7 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers {
     res.send('noop');
   }
 
-  identifyEnv(_req: express.Request): string {
+  identifyEnv(): string {
     return '';
   }
 }
diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts
index c2570b8768..afb136fd0f 100644
--- a/plugins/auth-backend/src/providers/types.ts
+++ b/plugins/auth-backend/src/providers/types.ts
@@ -341,3 +341,10 @@ export type SAMLProviderConfig = {
 export type SAMLEnvironmentProviderConfig = {
   [key: string]: SAMLProviderConfig;
 };
+
+export type OAuthState = {
+  /* A type for the serialized value in the `state` parameter of the OAuth authorization flow
+   */
+  nonce?: string;
+  env?: string;
+};

From cb80b2b4f8fc66189faf8c444e2f3e5eb05af8d4 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Tue, 4 Aug 2020 18:42:16 +0200
Subject: [PATCH 06/10] Refactor: encode state Parameters as URLSearchParams
 instead of JSON

---
 plugins/auth-backend/src/lib/OAuthProvider.ts | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 608e7f2075..6faaa9f733 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -42,10 +42,12 @@ export type Options = {
 
 const readState = (stateString: string): OAuthState => {
   try {
-    const state = JSON.parse(decodeURIComponent(stateString));
+    const state = Object.fromEntries(
+      new URLSearchParams(decodeURIComponent(stateString)),
+    );
     return {
-      nonce: state.nonce,
-      env: state.env,
+      nonce: state.nonce ?? undefined,
+      env: state.env ?? undefined,
     };
   } catch (e) {
     return {
@@ -56,7 +58,11 @@ const readState = (stateString: string): OAuthState => {
 };
 
 export const encodeState = (state: OAuthState): string => {
-  return encodeURIComponent(JSON.stringify(state));
+  const searchParams = new URLSearchParams();
+  searchParams.append('nonce', state.nonce ?? '');
+  searchParams.append('env', state.env ?? '');
+
+  return encodeURIComponent(searchParams.toString());
 };
 
 /*

From 4ae351b06f15a7c4902fc07dbd93ab5022f89dc9 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Tue, 4 Aug 2020 18:48:38 +0200
Subject: [PATCH 07/10] Refactor: Make properties of `OAuthState` non-optional

---
 plugins/auth-backend/src/lib/OAuthProvider.ts | 10 +++++-----
 plugins/auth-backend/src/providers/types.ts   |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 6faaa9f733..8d8743d24b 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -46,13 +46,13 @@ const readState = (stateString: string): OAuthState => {
       new URLSearchParams(decodeURIComponent(stateString)),
     );
     return {
-      nonce: state.nonce ?? undefined,
-      env: state.env ?? undefined,
+      nonce: state.nonce ?? '',
+      env: state.env ?? '',
     };
   } catch (e) {
     return {
-      nonce: undefined,
-      env: undefined,
+      nonce: '',
+      env: '',
     };
   }
 };
@@ -85,7 +85,7 @@ export const verifyNonce = (req: express.Request, providerId: string) => {
   if (!cookieNonce) {
     throw new Error('Auth response is missing cookie nonce');
   }
-  if (!stateNonce) {
+  if (stateNonce.length === 0) {
     throw new Error('Auth response is missing state nonce');
   }
   if (cookieNonce !== stateNonce) {
diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts
index afb136fd0f..74ed75d57a 100644
--- a/plugins/auth-backend/src/providers/types.ts
+++ b/plugins/auth-backend/src/providers/types.ts
@@ -345,6 +345,6 @@ export type SAMLEnvironmentProviderConfig = {
 export type OAuthState = {
   /* A type for the serialized value in the `state` parameter of the OAuth authorization flow
    */
-  nonce?: string;
-  env?: string;
+  nonce: string;
+  env: string;
 };

From 75467ecb6090e2823f9333d15cb11f31806d37ed Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Tue, 4 Aug 2020 18:51:00 +0200
Subject: [PATCH 08/10] Cleanup: Remove deadcode

---
 plugins/auth-backend/src/lib/OAuthProvider.ts | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 8d8743d24b..2e4c85a372 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -65,18 +65,6 @@ export const encodeState = (state: OAuthState): string => {
   return encodeURIComponent(searchParams.toString());
 };
 
-/*
-export const encodeState = (stateObject: object): string => {
-  const vals = Object.keys(stateObject).map(
-    key =>
-      `${encodeURIComponent(key)}=${encodeURIComponent(
-        Reflect.get(stateObject, key),
-      )}`,
-  );
-
-  return vals.join('&');
-};
-*/
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
   const state: OAuthState = readState(req.query.state?.toString() ?? '');

From 5f06a08daa4ae48a0dc8f197f26c1b289e2b290e Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Wed, 5 Aug 2020 17:04:25 +0200
Subject: [PATCH 09/10] Address PR comments: Use `undefined` instead of passing
 Empty strings

Type fn that identifies the env (environment) of a Request
Fail early if `env` is not present in the request or has an invalid
`env` in the state
---
 .../src/lib/EnvironmentHandler.ts             |  9 ++--
 .../src/lib/OAuthProvider.test.ts             |  3 +-
 plugins/auth-backend/src/lib/OAuthProvider.ts | 43 +++++++++++--------
 .../auth-backend/src/providers/factories.ts   |  9 ++--
 .../src/providers/saml/provider.ts            |  4 +-
 plugins/auth-backend/src/providers/types.ts   |  6 ++-
 6 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/plugins/auth-backend/src/lib/EnvironmentHandler.ts b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
index d2fe25c947..fb39ed86b8 100644
--- a/plugins/auth-backend/src/lib/EnvironmentHandler.ts
+++ b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
@@ -15,7 +15,10 @@
  */
 
 import express from 'express';
-import { AuthProviderRouteHandlers } from '../providers/types';
+import {
+  AuthProviderRouteHandlers,
+  EnvironmentIdentifierFn,
+} from '../providers/types';
 
 export type EnvironmentHandlers = {
   [key: string]: AuthProviderRouteHandlers;
@@ -25,14 +28,14 @@ export class EnvironmentHandler implements AuthProviderRouteHandlers {
   constructor(
     private readonly providerId: string,
     private readonly providers: EnvironmentHandlers,
-    private readonly envIdentifier: (req: express.Request) => string,
+    private readonly envIdentifier: EnvironmentIdentifierFn,
   ) {}
 
   private getProviderForEnv(
     req: express.Request,
     res: express.Response,
   ): AuthProviderRouteHandlers | undefined {
-    const env: string = this.envIdentifier(req);
+    const env: string | undefined = this.envIdentifier(req);
 
     if (env && this.providers.hasOwnProperty(env)) {
       return this.providers[env];
diff --git a/plugins/auth-backend/src/lib/OAuthProvider.test.ts b/plugins/auth-backend/src/lib/OAuthProvider.test.ts
index 8c6c902b72..771e991c8e 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.test.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.test.ts
@@ -65,7 +65,7 @@ describe('OAuthProvider Utils', () => {
       } as unknown) as express.Request;
       expect(() => {
         verifyNonce(mockRequest, 'providera');
-      }).toThrowError('Auth response is missing state nonce');
+      }).toThrowError('Invalid state passed via request');
     });
 
     it('should throw error if nonce mismatch', () => {
@@ -221,6 +221,7 @@ describe('OAuthProvider', () => {
     const mockRequest = ({
       query: {
         scope: 'user',
+        env: 'development',
       },
     } as unknown) as express.Request;
 
diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 2e4c85a372..ca8fc5bf0a 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -41,26 +41,27 @@ export type Options = {
 };
 
 const readState = (stateString: string): OAuthState => {
-  try {
-    const state = Object.fromEntries(
-      new URLSearchParams(decodeURIComponent(stateString)),
-    );
-    return {
-      nonce: state.nonce ?? '',
-      env: state.env ?? '',
-    };
-  } catch (e) {
-    return {
-      nonce: '',
-      env: '',
-    };
+  const state = Object.fromEntries(
+    new URLSearchParams(decodeURIComponent(stateString)),
+  );
+  if (
+    !state.nonce ||
+    !state.env ||
+    state.nonce?.length === 0 ||
+    state.env?.length === 0
+  ) {
+    throw Error(`Invalid state passed via request`);
   }
+  return {
+    nonce: state.nonce,
+    env: state.env,
+  };
 };
 
 export const encodeState = (state: OAuthState): string => {
   const searchParams = new URLSearchParams();
-  searchParams.append('nonce', state.nonce ?? '');
-  searchParams.append('env', state.env ?? '');
+  searchParams.append('nonce', state.nonce);
+  searchParams.append('env', state.env);
 
   return encodeURIComponent(searchParams.toString());
 };
@@ -130,7 +131,11 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
   async start(req: express.Request, res: express.Response): Promise<void> {
     // retrieve scopes from request
     const scope = req.query.scope?.toString() ?? '';
-    const env = req.query.env?.toString() ?? '';
+    const env = req.query.env?.toString();
+
+    if (!env) {
+      throw new InputError('No env provided in request query parameters');
+    }
 
     if (this.options.persistScopes) {
       this.setScopesCookie(res, scope);
@@ -255,17 +260,17 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     }
   }
 
-  identifyEnv(req: express.Request): string {
+  identifyEnv(req: express.Request): string | undefined {
     const reqEnv = req.query.env?.toString();
     if (reqEnv) {
       return reqEnv;
     }
     const stateParams = req.query.state?.toString();
     if (!stateParams) {
-      return '';
+      return undefined;
     }
     const env = readState(stateParams).env;
-    return env ?? '';
+    return env;
   }
 
   /**
diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts
index 403b39312c..701eef81af 100644
--- a/plugins/auth-backend/src/providers/factories.ts
+++ b/plugins/auth-backend/src/providers/factories.ts
@@ -15,7 +15,6 @@
  */
 
 import Router from 'express-promise-router';
-import express from 'express';
 import { Logger } from 'winston';
 import { TokenIssuer } from '../identity';
 import { createGithubProvider } from './github';
@@ -24,7 +23,11 @@ import { createGoogleProvider } from './google';
 import { createOAuth2Provider } from './oauth2';
 import { createOktaProvider } from './okta';
 import { createSamlProvider } from './saml';
-import { AuthProviderConfig, AuthProviderFactory } from './types';
+import {
+  AuthProviderConfig,
+  AuthProviderFactory,
+  EnvironmentIdentifierFn,
+} from './types';
 import { Config } from '@backstage/config';
 import {
   EnvironmentHandlers,
@@ -55,7 +58,7 @@ export const createAuthProviderRouter = (
   const router = Router();
   const envs = providerConfig.keys();
   const envProviders: EnvironmentHandlers = {};
-  let envIdentifier: ((req: express.Request) => string) | undefined;
+  let envIdentifier: EnvironmentIdentifierFn | undefined;
 
   for (const env of envs) {
     const envConfig = providerConfig.getConfig(env);
diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts
index 9db88c8fa9..1ccee1a9cb 100644
--- a/plugins/auth-backend/src/providers/saml/provider.ts
+++ b/plugins/auth-backend/src/providers/saml/provider.ts
@@ -107,8 +107,8 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers {
     res.send('noop');
   }
 
-  identifyEnv(): string {
-    return '';
+  identifyEnv(): string | undefined {
+    return undefined;
   }
 }
 
diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts
index 74ed75d57a..d372fd1b94 100644
--- a/plugins/auth-backend/src/providers/types.ts
+++ b/plugins/auth-backend/src/providers/types.ts
@@ -211,7 +211,7 @@ export interface AuthProviderRouteHandlers {
    *- contains the environment context information encoded in the request
    *  @param {express.Request} req
    */
-  identifyEnv?(req: express.Request): string;
+  identifyEnv?(req: express.Request): string | undefined;
 }
 
 export type AuthProviderFactory = (
@@ -348,3 +348,7 @@ export type OAuthState = {
   nonce: string;
   env: string;
 };
+
+export type EnvironmentIdentifierFn = (
+  req: express.Request,
+) => string | undefined;

From 959cc6fe86ff5a3dc6fa31a6e2f4fa644d031ce2 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Thu, 6 Aug 2020 12:48:02 +0200
Subject: [PATCH 10/10] Refactor: remove `env` parameter from callbackURL of
 all OAuth based providers

---
 plugins/auth-backend/src/providers/github/provider.ts | 4 ++--
 plugins/auth-backend/src/providers/gitlab/provider.ts | 4 ++--
 plugins/auth-backend/src/providers/google/provider.ts | 4 ++--
 plugins/auth-backend/src/providers/okta/provider.ts   | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/plugins/auth-backend/src/providers/github/provider.ts b/plugins/auth-backend/src/providers/github/provider.ts
index e9c719ea1c..863cfa9f5a 100644
--- a/plugins/auth-backend/src/providers/github/provider.ts
+++ b/plugins/auth-backend/src/providers/github/provider.ts
@@ -126,7 +126,7 @@ export class GithubAuthProvider implements OAuthProviderHandlers {
 
 export function createGithubProvider(
   { baseUrl }: AuthProviderConfig,
-  env: string,
+  _: string,
   envConfig: Config,
   logger: Logger,
   tokenIssuer: TokenIssuer,
@@ -148,7 +148,7 @@ export function createGithubProvider(
   const userProfileURL = enterpriseInstanceUrl
     ? `${enterpriseInstanceUrl}/api/v3/user`
     : undefined;
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;
+  const callbackURL = `${baseUrl}/${providerId}/handler/frame`;
 
   const opts = {
     clientID,
diff --git a/plugins/auth-backend/src/providers/gitlab/provider.ts b/plugins/auth-backend/src/providers/gitlab/provider.ts
index c5c546c6f3..22d0bd4f0a 100644
--- a/plugins/auth-backend/src/providers/gitlab/provider.ts
+++ b/plugins/auth-backend/src/providers/gitlab/provider.ts
@@ -133,7 +133,7 @@ export class GitlabAuthProvider implements OAuthProviderHandlers {
 
 export function createGitlabProvider(
   { baseUrl }: AuthProviderConfig,
-  env: string,
+  _: string,
   envConfig: Config,
   logger: Logger,
   tokenIssuer: TokenIssuer,
@@ -145,7 +145,7 @@ export function createGitlabProvider(
   const clientSecret = envConfig.getString('clientSecret');
   const audience = envConfig.getString('audience');
   const baseURL = audience || 'https://gitlab.com';
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;
+  const callbackURL = `${baseUrl}/${providerId}/handler/frame`;
 
   const opts = {
     clientID,
diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts
index 5749d1465f..b8070949f9 100644
--- a/plugins/auth-backend/src/providers/google/provider.ts
+++ b/plugins/auth-backend/src/providers/google/provider.ts
@@ -145,7 +145,7 @@ export class GoogleAuthProvider implements OAuthProviderHandlers {
 
 export function createGoogleProvider(
   { baseUrl }: AuthProviderConfig,
-  env: string,
+  _: string,
   envConfig: Config,
   logger: Logger,
   tokenIssuer: TokenIssuer,
@@ -155,7 +155,7 @@ export function createGoogleProvider(
   const appOrigin = envConfig.getString('appOrigin');
   const clientID = envConfig.getString('clientId');
   const clientSecret = envConfig.getString('clientSecret');
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;
+  const callbackURL = `${baseUrl}/${providerId}/handler/frame`;
 
   const opts = {
     clientID,
diff --git a/plugins/auth-backend/src/providers/okta/provider.ts b/plugins/auth-backend/src/providers/okta/provider.ts
index 56e8b1220d..058300cef6 100644
--- a/plugins/auth-backend/src/providers/okta/provider.ts
+++ b/plugins/auth-backend/src/providers/okta/provider.ts
@@ -165,7 +165,7 @@ export class OktaAuthProvider implements OAuthProviderHandlers {
 
 export function createOktaProvider(
   { baseUrl }: AuthProviderConfig,
-  env: string,
+  _: string,
   envConfig: Config,
   logger: Logger,
   tokenIssuer: TokenIssuer,
@@ -176,7 +176,7 @@ export function createOktaProvider(
   const clientID = envConfig.getString('clientId');
   const clientSecret = envConfig.getString('clientSecret');
   const audience = envConfig.getString('audience');
-  const callbackURL = `${baseUrl}/${providerId}/handler/frame?env=${env}`;
+  const callbackURL = `${baseUrl}/${providerId}/handler/frame`;
 
   const opts = {
     audience,