Removes redirect functionality from /signin page to prevent open re…

…direction CVE (#8626) Co-authored-by: Daniel Cousens <dcousens@users.noreply.github.com>
keystonejs · Jun 13, 2023 · a30c7a1 · a30c7a1
1 parent 70c6c16
commit a30c7a1
Show file tree

Hide file tree

Showing 10 changed files with 97 additions and 106 deletions.
diff --git a/.changeset/no-redirect.md b/.changeset/no-redirect.md
@@ -0,0 +1,5 @@
+---
+'@keystone-6/core': patch
+---
+
+Removes `?from` redirect from `/signin` page to prevent open redirection.
diff --git a/docs/pages/404.tsx b/docs/pages/404.tsx
@@ -30,13 +30,13 @@ function ConstructionIllustration() {
   );
 }
 
-// Modifying this code may have security implications
-// See.. https://github.com/keystonejs/keystone/pull/6411#issuecomment-906085389
+// modifying this code may have security implications
+//   see https://github.com/keystonejs/keystone/pull/6411#issuecomment-906085389
 const v5PathList = ['/tutorials', '/guides', '/keystonejs', '/api', '/discussions'];
 
 export default function NotFoundPage() {
   const { asPath } = useRouter();
-  const tryV5Link = asPath.startsWith('/') && v5PathList.some(i => asPath.startsWith(i));
+  const tryV5Link = v5PathList.some(x => asPath.startsWith(x));
   return (
     <Page title={'Page Not Found (404)'} description={'Page Not Found (404)'}>
       <div

diff --git a/packages/auth/src/gql/getBaseAuthSchema.ts b/packages/auth/src/gql/getBaseAuthSchema.ts
@@ -53,6 +53,7 @@ export function getBaseAuthSchema<I extends string, S extends string>({
       return gqlNames.ItemAuthenticationWithPasswordFailure;
     },
   });
+
   const extension = {
     query: {
       authenticatedItem: graphql.field({
@@ -61,14 +62,17 @@ export function getBaseAuthSchema<I extends string, S extends string>({
           types: [base.object(listKey) as graphql.ObjectType<BaseItem>],
           resolveType: (root, context) => context.session?.listKey,
         }),
-        resolve(root, args, { session, db }) {
-          if (
-            (typeof session?.itemId === 'string' || typeof session?.itemId === 'number') &&
-            typeof session.listKey === 'string'
-          ) {
-            return db[session.listKey].findOne({ where: { id: session.itemId } });
-          }
-          return null;
+        resolve(root, args, context) {
+          const { session } = context;
+          if (!session) return null;
+          if (!session.itemId) return null;
+          if (session.listKey !== listKey) return null;
+
+          return context.db[listKey].findOne({
+            where: {
+              id: session.itemId,
+            },
+          });
         },
       }),
     },
@@ -106,10 +110,12 @@ export function getBaseAuthSchema<I extends string, S extends string>({
             },
             context,
           });
+
           // return Failure if sessionStrategy.start() returns null
           if (typeof sessionToken !== 'string' || sessionToken.length === 0) {
             return { code: 'FAILURE', message: 'Failed to start session.' };
           }
+
           return { sessionToken, item: result.item };
         },
       }),

diff --git a/packages/auth/src/gql/getInitFirstItemSchema.ts b/packages/auth/src/gql/getInitFirstItemSchema.ts
@@ -1,8 +1,8 @@
 import { graphql } from '@keystone-6/core';
-import { BaseItem } from '@keystone-6/core/types';
+import type { BaseItem } from '@keystone-6/core/types';
 import { assertInputObjectType, GraphQLInputObjectType, GraphQLSchema } from 'graphql';
 
-import { AuthGqlNames, InitFirstItemConfig } from '../types';
+import type { AuthGqlNames, InitFirstItemConfig } from '../types';
 
 export function getInitFirstItemSchema({
   listKey,

diff --git a/packages/auth/src/gql/getMagicAuthLinkSchema.ts b/packages/auth/src/gql/getMagicAuthLinkSchema.ts
@@ -1,6 +1,6 @@
 import type { BaseItem } from '@keystone-6/core/types';
 import { graphql } from '@keystone-6/core';
-import { AuthGqlNames, AuthTokenTypeConfig, SecretFieldImpl } from '../types';
+import type { AuthGqlNames, AuthTokenTypeConfig, SecretFieldImpl } from '../types';
 
 import { createAuthToken } from '../lib/createAuthToken';
 import { validateAuthToken } from '../lib/validateAuthToken';

diff --git a/packages/auth/src/gql/getPasswordResetSchema.ts b/packages/auth/src/gql/getPasswordResetSchema.ts
@@ -1,5 +1,5 @@
 import { graphql } from '@keystone-6/core';
-import { AuthGqlNames, AuthTokenTypeConfig, SecretFieldImpl } from '../types';
+import type { AuthGqlNames, AuthTokenTypeConfig, SecretFieldImpl } from '../types';
 
 import { createAuthToken } from '../lib/createAuthToken';
 import { validateAuthToken } from '../lib/validateAuthToken';

diff --git a/packages/auth/src/index.ts b/packages/auth/src/index.ts
@@ -72,16 +72,23 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
       listView: { fieldMode: 'hidden' },
     },
   } as const;
-  // These field names have to follow this format so that for e.g
-  // validateAuthToken() behaves correctly.
-  const tokenFields = (tokenType: 'passwordReset' | 'magicAuth') => ({
-    [`${tokenType}Token`]: password({ ...fieldConfig }),
-    [`${tokenType}IssuedAt`]: timestamp({ ...fieldConfig }),
-    [`${tokenType}RedeemedAt`]: timestamp({ ...fieldConfig }),
-  });
-  const fields = {
-    ...(passwordResetLink && tokenFields('passwordReset')),
-    ...(magicAuthLink && tokenFields('magicAuth')),
+
+  const authFields = {
+    ...(passwordResetLink
+      ? {
+          passwordResetToken: password({ ...fieldConfig }),
+          passwordResetIssuedAt: timestamp({ ...fieldConfig }),
+          passwordResetRedeemedAt: timestamp({ ...fieldConfig }),
+        }
+      : null),
+
+    ...(magicAuthLink
+      ? {
+          magicAuthToken: password({ ...fieldConfig }),
+          magicAuthIssuedAt: timestamp({ ...fieldConfig }),
+          magicAuthRedeemedAt: timestamp({ ...fieldConfig }),
+        }
+      : null),
   };
 
   /**
@@ -133,45 +140,30 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
     sessionData,
   });
 
-  /**
-   * validateConfig
-   *
-   * Validates the provided auth config; optional step when integrating auth
-   */
-  const validateConfig = (keystoneConfig: KeystoneConfig) => {
-    const listConfig = keystoneConfig.lists[listKey];
-    if (listConfig === undefined) {
-      const msg = `A createAuth() invocation specifies the list "${listKey}" but no list with that key has been defined.`;
-      throw new Error(msg);
+  function throwIfInvalidConfig<TypeInfo extends BaseKeystoneTypeInfo>(
+    config: KeystoneConfig<TypeInfo>
+  ) {
+    if (!(listKey in config.lists)) {
+      throw new Error(`withAuth cannot find the list "${listKey}"`);
     }
 
-    // TODO: Check for String-like typing for identityField? How?
-    // TODO: Validate that the identifyField is unique.
-    // TODO: If this field isn't required, what happens if I try to log in as `null`?
-    const identityFieldConfig = listConfig.fields[identityField];
-    if (identityFieldConfig === undefined) {
-      const i = JSON.stringify(identityField);
-      const msg = `A createAuth() invocation for the "${listKey}" list specifies ${i} as its identityField but no field with that key exists on the list.`;
-      throw new Error(msg);
+    // TODO: verify that the identity field is unique
+    // TODO: verify that the field is required
+    const list = config.lists[listKey];
+    if (!(identityField in list.fields)) {
+      throw new Error(`withAuth cannot find the identity field "${listKey}.${identityField}"`);
     }
 
-    // TODO: We could make the secret field optional to disable the standard id/secret auth and password resets (ie. magic links only)
-    const secretFieldConfig = listConfig.fields[secretField];
-    if (secretFieldConfig === undefined) {
-      const s = JSON.stringify(secretField);
-      const msg = `A createAuth() invocation for the "${listKey}" list specifies ${s} as its secretField but no field with that key exists on the list.`;
-      throw new Error(msg);
+    if (!(secretField in list.fields)) {
+      throw new Error(`withAuth cannot find the secret field "${listKey}.${secretField}"`);
     }
 
-    // TODO: Could also validate initFirstItem.itemData keys?
-    for (const field of initFirstItem?.fields || []) {
-      if (listConfig.fields[field] === undefined) {
-        const f = JSON.stringify(field);
-        const msg = `A createAuth() invocation for the "${listKey}" list specifies the field ${f} in initFirstItem.fields array but no field with that key exist on the list.`;
-        throw new Error(msg);
-      }
+    for (const fieldKey of initFirstItem?.fields || []) {
+      if (fieldKey in list.fields) continue;
+
+      throw new Error(`initFirstItem.fields has unknown field "${listKey}.${fieldKey}"`);
     }
-  };
+  }
 
   // this strategy wraps the existing session strategy,
   //   and injects the requested session.data before returning
@@ -184,34 +176,31 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
       get: async ({ context }) => {
         const session = await get({ context });
         const sudoContext = context.sudo();
-        if (
-          !session ||
-          !session.listKey ||
-          session.listKey !== listKey ||
-          !session.itemId ||
-          !sudoContext.query[session.listKey]
-        ) {
-          return;
-        }
+        if (!session) return;
+        if (!session.itemId) return;
+        if (session.listKey !== listKey) return;
 
         try {
           const data = await sudoContext.query[listKey].findOne({
-            where: { id: session.itemId as any }, // TODO: fix this
+            where: { id: session.itemId },
             query: sessionData,
           });
           if (!data) return;
 
           return { ...session, itemId: session.itemId, listKey, data };
         } catch (e) {
-          // TODO: the assumption is this should only be from an invalid sessionData configuration
+          console.error(e);
+          // TODO: the assumption is this could only be from an invalid sessionData configuration
           //   it could be something else though, either way, result is a bad session
           return;
         }
       },
     };
   }
 
-  async function hasInitFirstItemConditions(context: KeystoneContext) {
+  async function hasInitFirstItemConditions<TypeInfo extends BaseKeystoneTypeInfo>(
+    context: KeystoneContext<TypeInfo>
+  ) {
     // do nothing if they aren't using this feature
     if (!initFirstItem) return false;
 
@@ -222,11 +211,11 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
     return count === 0;
   }
 
-  async function authMiddleware({
+  async function authMiddleware<TypeInfo extends BaseKeystoneTypeInfo>({
     context,
     wasAccessAllowed,
   }: {
-    context: KeystoneContext;
+    context: KeystoneContext<TypeInfo>;
     wasAccessAllowed: boolean;
   }): Promise<{ kind: 'redirect'; to: string } | void> {
     const { req } = context;
@@ -246,11 +235,7 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
     if (wasAccessAllowed) return;
 
     // otherwise, redirect to signin
-    if (pathname === '/') return { kind: 'redirect', to: '/signin' };
-    return {
-      kind: 'redirect',
-      to: `/signin?from=${encodeURIComponent(req!.url!)}`,
-    };
+    return { kind: 'redirect', to: '/signin' };
   }
 
   function defaultIsAccessAllowed({ session, sessionStrategy }: KeystoneContext) {
@@ -266,12 +251,11 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
    *
    * Automatically extends your configuration with a prescriptive implementation.
    */
-  const withAuth = <TypeInfo extends BaseKeystoneTypeInfo>(
-    keystoneConfig: KeystoneConfig<TypeInfo>
-  ): KeystoneConfig<TypeInfo> => {
-    validateConfig(keystoneConfig);
-
-    let { ui } = keystoneConfig;
+  function withAuth<TypeInfo extends BaseKeystoneTypeInfo>(
+    config: KeystoneConfig<TypeInfo>
+  ): KeystoneConfig<TypeInfo> {
+    throwIfInvalidConfig(config);
+    let { ui } = config;
     if (!ui?.isDisabled) {
       const {
         getAdditionalFiles = [],
@@ -297,24 +281,30 @@ export function createAuth<ListTypeInfo extends BaseListTypeInfo>({
       };
     }
 
-    if (!keystoneConfig.session) throw new TypeError('Missing .session configuration');
+    if (!config.session) throw new TypeError('Missing .session configuration');
 
-    const { extendGraphqlSchema = defaultExtendGraphqlSchema } = keystoneConfig;
-    const listConfig = keystoneConfig.lists[listKey];
+    const { extendGraphqlSchema = defaultExtendGraphqlSchema } = config;
+    const authListConfig = config.lists[listKey];
 
     return {
-      ...keystoneConfig,
+      ...config,
       ui,
-      session: authSessionStrategy(keystoneConfig.session),
+      session: authSessionStrategy(config.session),
       lists: {
-        ...keystoneConfig.lists,
-        [listKey]: { ...listConfig, fields: { ...listConfig.fields, ...fields } },
+        ...config.lists,
+        [listKey]: {
+          ...authListConfig,
+          fields: {
+            ...authListConfig.fields,
+            ...authFields,
+          },
+        },
       },
       extendGraphqlSchema: schema => {
         return extendGraphqlSchema(authExtendGraphqlSchema(schema));
       },
     };
-  };
+  }
 
   return {
     withAuth,

diff --git a/packages/auth/src/lib/useFromRedirect.ts b/packages/auth/src/lib/useFromRedirect.ts
@@ -1,16 +1,6 @@
 import { useMemo } from 'react';
-import { useRouter } from '@keystone-6/core/admin-ui/router';
 
-export const useRedirect = () => {
-  const router = useRouter();
-  const redirect = useMemo(() => {
-    const { from } = router.query;
-    if (typeof from !== 'string') return '/';
-    if (!from.startsWith('/')) return '/';
-    if (from === '/no-access') return '/';
-
-    return from;
-  }, [router]);
-
-  return redirect;
-};
+// TODO: remove or fix
+export function useRedirect() {
+  return useMemo(() => '/', []);
+}
diff --git a/packages/auth/src/types.ts b/packages/auth/src/types.ts
@@ -65,5 +65,5 @@ export type AuthTokenRedemptionErrorCode = 'FAILURE' | 'TOKEN_EXPIRED' | 'TOKEN_
 
 export type SecretFieldImpl = {
   generateHash: (secret: string) => Promise<string>;
-  compare: (secret: string, hash: string) => Promise<string>;
+  compare: (secret: string, hash: string) => Promise<boolean>;
 };
diff --git a/packages/core/src/types/type-info.ts b/packages/core/src/types/type-info.ts
@@ -12,7 +12,7 @@ export type BaseListTypeInfo = {
     create: GraphQLInput;
     update: GraphQLInput;
     where: GraphQLInput;
-    uniqueWhere: { readonly id?: string | null } & GraphQLInput;
+    uniqueWhere: { readonly id?: string | number | null } & GraphQLInput;
     orderBy: Record<string, 'asc' | 'desc' | null>;
   };
   /**