ronantakizawa/codereview-bench huggingface dataset:
Repository: strapi/strapi
File: packages/core/admin/server/src/services/encryption.ts (TypeScript)

Code before:
import crypto from 'crypto';

const IV_LENGTH = 16; // 16 bytes for AES-GCM IV
/**
 * Encrypts a value string using AES-256-GCM.
 * Returns a string containing IV, encrypted content, and auth tag (all hex-encoded).
 */
const encrypt = (value: string) => {
  const encryptionKey: string = strapi.config.get('admin.secrets.encryptionKey');
  if (!encryptionKey) {
    strapi.log.warn('Encryption key is missing from config');
    return null;
  }
  const iv = crypto.randomBytes(IV_LENGTH);
  const cipher = crypto.createCipheriv('aes-256-gcm', Buffer.from(encryptionKey, 'hex'), iv);

  let encrypted = cipher.update(value, 'utf8', 'hex');
  encrypted += cipher.final('hex');

  const authTag = cipher.getAuthTag();

  return `${iv.toString('hex')}:${encrypted}:${authTag.toString('hex')}`;
};

/**
 * Decrypts a value encrypted by encryptValue.
 * Input must be in the format: iv:encrypted:authTag (all hex-encoded).
 */
const decrypt = (encryptedValue: string) => {
  const encryptionKey: string = strapi.config.get('admin.secrets.encryptionKey');
  if (!encryptionKey) {
    strapi.log.warn('Encryption key is missing from config');
    return null;
  }

Review comment (security):
This causes a breaking change, because `regenerate` no longer works until the user adds that config value.

Maybe we should use the existing `auth.secret` as a fallback and document that if you want to set a separate encryption key you are able to set it in `secrets.encryptionKey`? Or, if we don't want to do it globally, we could allow `encrypt` here to optionally accept a key, so that API tokens (which are technically part of auth) could explicitly pass in `auth.secret`

We definitely need a better way to manage all these secrets, we should have a single base key we use universally, unless a user explicitly wants to be more granular with it for key rotations.

@Convly @jhoward1994 @maccomaccomaccomacco what do you think?

Code after:
import crypto from 'crypto';

const IV_LENGTH = 16; // 16 bytes for AES-GCM IV
const ENCRYPTION_VERSION = 'v1';

const getHashedKey = (): Buffer | null => {
  const rawKey: string = strapi.config.get('admin.secrets.encryptionKey');
  if (!rawKey) {
    strapi.log.warn('Encryption key is missing from config');
    return null;
  }

  return crypto.createHash('sha256').update(rawKey).digest(); // Always 32 bytes
};

/**
 * Encrypts a value string using AES-256-GCM.
 * Returns a string prefixed with the encryption version and includes IV, encrypted content, and auth tag (all hex-encoded).
 */
const encrypt = (value: string) => {
  const key = getHashedKey();
  if (!key) return null;

  const iv = crypto.randomBytes(IV_LENGTH);
  const cipher = crypto.createCipheriv('aes-256-gcm', key, iv);

  let encrypted = cipher.update(value, 'utf8', 'hex');
  encrypted += cipher.final('hex');

  const authTag = cipher.getAuthTag();

  return `${ENCRYPTION_VERSION}:${iv.toString('hex')}:${encrypted}:${authTag.toString('hex')}`;
};
Repository: strapi/strapi
File: packages/core/admin/server/src/content-types/api-token.ts (TypeScript)

Code before:
    description: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: false,
      default: '',
    },
    type: {
      type: 'enumeration',
      enum: Object.values(constants.API_TOKEN_TYPE),
      configurable: false,
      required: true,
      default: constants.API_TOKEN_TYPE.READ_ONLY,
    },
    accessKey: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: true,
      searchable: false,
    },
    encryptedKey: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: true,
      searchable: false,
    },
    lastUsedAt: {
      type: 'datetime',
      configurable: false,
      required: false,
    },
    permissions: {
      type: 'relation',
      target: 'admin::api-token-permission',
      relation: 'oneToMany',
      mappedBy: 'token',
      configurable: false,
      required: false,
    },
    expiresAt: {
      type: 'datetime',
      configurable: false,
      required: false,
    },
    lifespan: {
      type: 'biginteger',
      configurable: false,
      required: false,
    },

Review comment (suggestion):
```suggestion
      required: false,
```
Just wondering if we actually need this to be required, since it's not really, and in fact all current tokens won't have it.

Code after:
    description: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: false,
      default: '',
    },
    type: {
      type: 'enumeration',
      enum: Object.values(constants.API_TOKEN_TYPE),
      configurable: false,
      required: true,
      default: constants.API_TOKEN_TYPE.READ_ONLY,
    },
    accessKey: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: true,
      searchable: false,
    },
    encryptedKey: {
      type: 'string',
      minLength: 1,
      configurable: false,
      required: false,
      searchable: false,
    },
    lastUsedAt: {
      type: 'datetime',
      configurable: false,
      required: false,
    },
    permissions: {
      type: 'relation',
      target: 'admin::api-token-permission',
      relation: 'oneToMany',
      mappedBy: 'token',
      configurable: false,
      required: false,
    },
    expiresAt: {
      type: 'datetime',
      configurable: false,
      required: false,
    },
    lifespan: {
      type: 'biginteger',
      configurable: false,
      required: false,
    },
Repository: strapi/strapi
File: packages/core/admin/server/src/services/content-type.ts (TypeScript)

Code before:
    const action = actionProvider.get(actionId) as any;

    // todo see if it's possible to check property on action + subject (async)
    if (!actionDomain.appliesToProperty('fields', action)) {
      return permissionDomain.deleteProperty('fields', permission);
    }

    if (!subject || !strapi.contentTypes[subject]) {
      return permission;
    }

    const possibleFields = getNestedFieldsWithIntermediate(strapi.contentTypes[subject], {
      components: strapi.components,
      nestingLevel,
    });

    const currentFields: string[] = fields || [];
    const validUserFields: string[] = uniq(intersection(currentFields, possibleFields));

    const newFields = validUserFields.filter(
      (field: string) =>
        !validUserFields.some(
          (validUserField: string) =>
            validUserField !== field && startsWith(validUserField, `${field}.`)
        )
    );

    return permissionDomain.setProperty('fields', newFields, permission);
  }, []);
};

export {
  getNestedFields,
  getPermissionsWithNestedFields,
  cleanPermissionFields,
  getNestedFieldsWithIntermediate,
};


Review comment (refactor):
This is a bit hard to read. Instead of returning everything inline, could you maybe extract some of the intermediate values into variables? This way they'd have a name, which makes it easier for newcomers to grasp the logic

Code after:

    // todo see if it's possible to check property on action + subject (async)
    if (!actionDomain.appliesToProperty('fields', action)) {
      return permissionDomain.deleteProperty('fields', permission);
    }

    if (!subject || !strapi.contentTypes[subject]) {
      return permission;
    }

    const possibleFields = getNestedFieldsWithIntermediate(strapi.contentTypes[subject], {
      components: strapi.components,
    });

    const currentFields: string[] = fields || [];

    const validUserFields: string[] = uniq(
      possibleFields.filter((pf) =>
        currentFields.some((cf) => pf === cf || startsWith(`${cf}.`, pf))
      )
    );

    // A field is considered "not nested" if no other valid user field starts with this field's path followed by a dot.
    // This helps to remove redundant parent paths when a more specific child path is already included.
    // For example, if 'component.title' is present, 'component' would be filtered out by this condition.
    const isNotNestedField = (field: string) =>
      !validUserFields.some(
        (validUserField: string) =>
          validUserField !== field && startsWith(`${field}.`, validUserField)
      );

    // Filter out fields that are parent paths of other included fields.
    const newFields = validUserFields.filter(isNotNestedField);

    return permissionDomain.setProperty('fields', newFields, permission);
  }, []);
};

export {
  getNestedFields,
  getPermissionsWithNestedFields,
  cleanPermissionFields,
  getNestedFieldsWithIntermediate,
};
Repository: strapi/strapi
File: packages/core/admin/server/src/services/user.ts (TypeScript)

Code before:
  try {
    await passwordValidator.validate(password);
  } catch (error) {
    throw new ValidationError(
      'Invalid password. Expected a minimum of 8 characters with at least one number and one uppercase letter'
    );
  }

  await updateById(user.id, { password });
};

/**
 * Check if a user is the last super admin
 * @param userId user's id to look for
 */
const isLastSuperAdminUser = async (userId: Data.ID): Promise<boolean> => {
  const user = (await findOne(userId)) as AdminUser | null;
  if (!user) return false;

  const superAdminRole = await getService('role').getSuperAdminWithUsersCount();

  return superAdminRole.usersCount === 1 && hasSuperAdminRole(user);
};

/**
 * Check if a user is the last super admin
 * @param userId user's id to look for
 */
const isFirstSuperAdminUser = async (userId: Data.ID): Promise<boolean> => {
  const currentUser = (await findOne(userId)) as AdminUser | null;

  if (!currentUser || !hasSuperAdminRole(currentUser)) return false;

  const [oldestUser] = await strapi.db.query('admin::user').findMany({
    populate: {
      roles: {
        where: {
          code: { $eq: SUPER_ADMIN_CODE },
        },
      },
    },
    orderBy: { createdAt: 'asc' },
    limit: 1,
  });

  return oldestUser.id === currentUser.id;
};

/**
 * Check if a user with specific attributes exists in the database
 * @param attributes A partial user object

Review comment (suggestion):
```suggestion
 * Check if a user is the first super admin
```

Code after:
  try {
    await passwordValidator.validate(password);
  } catch (error) {
    throw new ValidationError(
      'Invalid password. Expected a minimum of 8 characters with at least one number and one uppercase letter'
    );
  }

  await updateById(user.id, { password });
};

/**
 * Check if a user is the last super admin
 * @param userId user's id to look for
 */
const isLastSuperAdminUser = async (userId: Data.ID): Promise<boolean> => {
  const user = (await findOne(userId)) as AdminUser | null;
  if (!user) return false;

  const superAdminRole = await getService('role').getSuperAdminWithUsersCount();

  return superAdminRole.usersCount === 1 && hasSuperAdminRole(user);
};

/**
 * Check if a user is the first super admin
 * @param userId user's id to look for
 */
const isFirstSuperAdminUser = async (userId: Data.ID): Promise<boolean> => {
  const currentUser = (await findOne(userId)) as AdminUser | null;

  if (!currentUser || !hasSuperAdminRole(currentUser)) return false;

  const [oldestUser] = await strapi.db.query('admin::user').findMany({
    populate: {
      roles: {
        where: {
          code: { $eq: SUPER_ADMIN_CODE },
        },
      },
    },
    orderBy: { createdAt: 'asc' },
    limit: 1,
    select: ['id'],
  });

  return oldestUser.id === currentUser.id;
};

/**
 * Check if a user with specific attributes exists in the database
Repository: strapi/strapi
File: packages/core/admin/admin/src/pages/Settings/components/Tokens/TokenBox.tsx (TypeScript)

Code before:
      }
      subtitle={
        token
          ? formatMessage(
              tokenType === 'api-token'
                ? {
                    id: 'Settings.tokens.copy.subtitle',
                    defaultMessage: 'Copy this token for use elsewhere',
                  }
                : {
                    id: 'Settings.tokens.copy.lastWarning',
                    defaultMessage:
                      "Make sure to copy this token, you won't be able to see it again!",
                  }
            )
          : formatMessage({
              id: 'Settings.tokens.copy.editMessage',
              defaultMessage: 'For security reasons, you can only see your token once.',
            })
      }
      icon={<Key />}
      iconBackground="neutral100"
    />
  );

  return tokenType === 'api-token' ? (
    <unstable_tours.apiTokens.CopyAPIToken>{contentBox}</unstable_tours.apiTokens.CopyAPIToken>
  ) : (
    contentBox
  );
};


Review comment (suggestion):
See same comment with CTB link to my CTB PR. I think we could use a getter that returns a fragment by default

Code after:
              id: 'Settings.tokens.copy.title',
              defaultMessage: 'Token',
            })}
          </Typography>
          <Typography>
            {formatMessage({
              id: 'Settings.tokens.copy.lastWarning',
              defaultMessage: 'Make sure to copy this token, you won’t be able to see it again!',
            })}
          </Typography>
        </Flex>
        <Box background="neutral100" hasRadius padding={2} borderColor="neutral150">
          <TypographyWordBreak fontWeight="semiBold" variant="pi">
            {token}
          </TypographyWordBreak>
        </Box>
        <unstable_tours.apiTokens.CopyAPIToken>
          <Button
            startIcon={<Duplicate />}
            variant="secondary"
            onClick={handleClick(token)}
            marginTop={6}
          >
            {formatMessage({ id: 'Settings.tokens.copy.copy', defaultMessage: 'Copy' })}
          </Button>
        </unstable_tours.apiTokens.CopyAPIToken>
      </Flex>
    </>
  );
};

export const TokenBox = ({ token, tokenType }: TokenBoxProps) => {
  const { formatMessage } = useIntl();
  const { toggleNotification } = useNotification();
  const { trackUsage } = useTracking();

  const { copy } = useClipboard();

  const handleClick = (token: TokenBoxProps['token']) => async () => {
    if (token) {
      const didCopy = await copy(token);

      if (didCopy) {
        trackUsage('didCopyTokenKey', {
          tokenType,
        });
        toggleNotification({
          type: 'success',
          message: formatMessage({ id: 'Settings.tokens.notification.copied' }),
        });
      }
Repository: strapi/strapi
File: packages/core/core/src/core-api/routes/validation/utils.ts (TypeScript)

Code before:
 * @param callback - A function that returns the Zod schema to be created and registered.
 * @returns The created or retrieved Zod schema.
 * @example
 * ```typescript
 * const CategorySchema = safeSchemaCreation("Category", () =>
 *   z.object({
 *     name: z.string(),
 *     products: z.array(safeSchemaCreation("Product", () =>
 *       z.object({
 *         name: z.string(),
 *         category: z.lazy(() => CategorySchema) // Cyclical reference
 *       })
 *     ))
 *   })
 * );
 * ```
 */
export const safeSchemaCreation = (id: string, callback: () => z.ZodType) => {
  const { _idmap: idMap } = z.globalRegistry;

  const transformedId = transformUidToValidOpenApiName(id);

  const existsInGlobalRegistry = idMap.has(transformedId);

  if (existsInGlobalRegistry) {
    return idMap.get(transformedId) as z.ZodType;
  }

  // Temporary any placeholder before replacing with the actual schema type
  // Used to prevent infinite loops in cyclical data structures
  safeGlobalRegistrySet(id, z.any());

  const schema = callback();

  safeGlobalRegistrySet(id, schema);

  return schema;
};


Review comment (suggestion):
`.has` doesn't work as a typeguard, we can probably replace it and do this instead to avoid `as`

```suggestion
    const item = idMap.get(transformedId);
    if (!item) {
      throw new Error(`Schema with id ${transformedId} not found in global registry`);
    }
    return item;
```

Maybe log a warning instead of throwing though? That might be better so we don't block the entire file generation from a small error.

Code after:
 * @param id - The unique identifier for the schema in the global registry.
 * @param callback - A function that returns the Zod schema to be created and registered.
 * @returns The created or retrieved Zod schema.
 * @example
 * ```typescript
 * const CategorySchema = safeSchemaCreation("Category", () =>
 *   z.object({
 *     name: z.string(),
 *     products: z.array(safeSchemaCreation("Product", () =>
 *       z.object({
 *         name: z.string(),
 *         category: z.lazy(() => CategorySchema) // Cyclical reference
 *       })
 *     ))
 *   })
 * );
 * ```
 */
export const safeSchemaCreation = (id: Internal.UID.Schema, callback: () => z.ZodType) => {
  const { _idmap: idMap } = z.globalRegistry;

  const transformedId = transformUidToValidOpenApiName(id);

  const mapItem = idMap.get(transformedId);
  if (mapItem) {
    return mapItem;
  }

  strapi.log.warn(
    `Schema ${transformedId} not found in global registry, creating an any placeholder`
  );

  // Temporary any placeholder before replacing with the actual schema type
  // Used to prevent infinite loops in cyclical data structures
  safeGlobalRegistrySet(id, z.any());

  const schema = callback();

  safeGlobalRegistrySet(id, schema);

  return schema;
};
Repository: strapi/strapi
File: packages/core/core/src/core-api/routes/validation/utils.ts (TypeScript)

Code before:
 * @param id - The unique identifier for the schema in the global registry.
 * @param callback - A function that returns the Zod schema to be created and registered.
 * @returns The created or retrieved Zod schema.
 * @example
 * ```typescript
 * const CategorySchema = safeSchemaCreation("Category", () =>
 *   z.object({
 *     name: z.string(),
 *     products: z.array(safeSchemaCreation("Product", () =>
 *       z.object({
 *         name: z.string(),
 *         category: z.lazy(() => CategorySchema) // Cyclical reference
 *       })
 *     ))
 *   })
 * );
 * ```
 */
export const safeSchemaCreation = (id: Internal.UID.Schema, callback: () => z.ZodType) => {
  const { _idmap: idMap } = z.globalRegistry;

  const transformedId = transformUidToValidOpenApiName(id);

  const mapItem = idMap.get(transformedId);
  if (mapItem) {
    return mapItem as z.ZodType;
  }

  strapi.log.warn(
    `Schema ${transformedId} not found in global registry, creating an any placeholder`
  );

  // Temporary any placeholder before replacing with the actual schema type
  // Used to prevent infinite loops in cyclical data structures
  safeGlobalRegistrySet(id, z.any());

  const schema = callback();

  safeGlobalRegistrySet(id, schema);

  return schema;
};


Review comment (suggestion):
You shouldn't need the `as z.ZodType` anymore, it was only showing an error before because it could possibly be undefined

Code after:
 * @param id - The unique identifier for the schema in the global registry.
 * @param callback - A function that returns the Zod schema to be created and registered.
 * @returns The created or retrieved Zod schema.
 * @example
 * ```typescript
 * const CategorySchema = safeSchemaCreation("Category", () =>
 *   z.object({
 *     name: z.string(),
 *     products: z.array(safeSchemaCreation("Product", () =>
 *       z.object({
 *         name: z.string(),
 *         category: z.lazy(() => CategorySchema) // Cyclical reference
 *       })
 *     ))
 *   })
 * );
 * ```
 */
export const safeSchemaCreation = (id: Internal.UID.Schema, callback: () => z.ZodType) => {
  const { _idmap: idMap } = z.globalRegistry;

  const transformedId = transformUidToValidOpenApiName(id);

  const mapItem = idMap.get(transformedId);
  if (mapItem) {
    return mapItem;
  }

  strapi.log.warn(
    `Schema ${transformedId} not found in global registry, creating an any placeholder`
  );

  // Temporary any placeholder before replacing with the actual schema type
  // Used to prevent infinite loops in cyclical data structures
  safeGlobalRegistrySet(id, z.any());

  const schema = callback();

  safeGlobalRegistrySet(id, schema);

  return schema;
};
Repository: strapi/strapi
File: packages/core/upload/server/src/services/__tests__/upload/uploadImage.test.ts (TypeScript)

Code before:
import path from 'path';
import fs from 'fs';
import fse from 'fs-extra';
import _ from 'lodash';
import createUploadService from '../../upload';
import imageManipulation from '../../image-manipulation';

// Set up initial mock before service creation
global.strapi = {
  plugins: {
    upload: {
      services: {
        'image-manipulation': imageManipulation,
      },
    },
  },
  plugin: (name: string) => global.strapi.plugins[name],
} as any;

const uploadService = createUploadService({} as any);

const imageFilePath = path.join(__dirname, './image.png');

const tmpWorkingDirectory = path.join(__dirname, './tmp');

function mockUploadProvider(uploadFunc: any, props?: any) {
  const { responsiveDimensions = false } = props || {};

  const defaultConfig = {
    'plugin::upload': {
      breakpoints: {
        large: 1000,
        medium: 750,
      },
    },
  };

  global.strapi = {
    config: {
      get: (path: any, defaultValue: any) => _.get(defaultConfig, path, defaultValue),
    },
    plugins: {
      upload: {

Review comment (suggestion):
There is a mockUploadProvider function below that sets up global.strapi, should this be handled there? Seems a bit strange to be mutating the global in two places. 

Code after:
import path from 'path';
import fs from 'fs';
import fse from 'fs-extra';
import _ from 'lodash';
import createUploadService from '../../upload';
import imageManipulation from '../../image-manipulation';

const defaultConfig = {
  'plugin::upload': {
    breakpoints: {
      large: 1000,
      medium: 750,
    },
  },
};

// Set up initial mock before service creation
global.strapi = {
  config: {
    get: (path: any, defaultValue: any) => _.get(defaultConfig, path, defaultValue),
  },
  plugins: {
    upload: {
      services: {
        provider: {
          upload: jest.fn(),
        },
        upload: {
          getSettings: () => ({ responsiveDimensions: false }),
        },
        'image-manipulation': imageManipulation,
      },
    },
  },
  plugin: (name: string) => global.strapi.plugins[name],
} as any;

const uploadService = createUploadService({} as any);

const imageFilePath = path.join(__dirname, './image.png');
const tmpWorkingDirectory = path.join(__dirname, './tmp');

function mockUploadProvider(uploadFunc: any, props?: any) {
Repository: strapi/strapi
File: packages/core/admin/admin/src/components/UnstableGuidedTour/Context.tsx (TypeScript)

Code before:
      length: tourLength,
      isCompleted: false,
    };

    return acc;
  }, {} as Tour);

  const [tours, setTours] = usePersistentState<State>(STORAGE_KEY, {
    tours: initialTourState,
    enabled,
    completedActions: [],
  });
  const [state, dispatch] = React.useReducer(reducer, tours);

  // Sync local storage
  React.useEffect(() => {
    if (window.strapi.future.isEnabled('unstableGuidedTour')) {
      setTours(state);
    }
  }, [state, setTours]);

  React.useEffect(() => {
    if (enabled) {
      trackUsage('didLaunchGuidedtour');
    }
  }, [enabled, trackUsage]);

  return (
    <GuidedTourProviderImpl state={state} dispatch={dispatch}>
      {children}
    </GuidedTourProviderImpl>
  );
};

export type { Action, State, ValidTourName };
export { UnstableGuidedTourContext, unstableUseGuidedTour, reducer };


Review comment (suggestion):
I'm gonna check with Yannis but this event doesn't make much sense to me. I think the didClickGuidedTourX tracking should be enough

Code after:
    };

    return acc;
  }, {} as Tour);

  const [tours, setTours] = usePersistentState<State>(STORAGE_KEY, {
    tours: initialTourState,
    enabled,
    completedActions: [],
  });
  const [state, dispatch] = React.useReducer(reducer, tours);

  // Sync local storage
  React.useEffect(() => {
    if (window.strapi.future.isEnabled('unstableGuidedTour')) {
      setTours(state);
    }
  }, [state, setTours]);

  return (
    <GuidedTourProviderImpl state={state} dispatch={dispatch}>
      {children}
    </GuidedTourProviderImpl>
  );
};

export type { Action, State, ValidTourName };
export { UnstableGuidedTourContext, unstableUseGuidedTour, reducer };