feat: register.allowedFields defaults to empty array

packages/plugins/users-permissions/server/bootstrap/index.js

@@ -10,12 +10,9 @@
 const crypto = require('crypto');
 const _ = require('lodash');
 const urljoin = require('url-join');
-const { isArray } = require('lodash/fp');
 const { getService } = require('../utils');
 const getGrantConfig = require('./grant-config');
-
 const usersPermissionsActions = require('./users-permissions-actions');
-const userSchema = require('../content-types/user');
 
 const initGrant = async (pluginStore) => {
   const apiPrefix = strapi.config.get('api.rest.prefix');
@@ -99,29 +96,6 @@ const initAdvancedOptions = async (pluginStore) => {
   }
 };
 
-const userSchemaAdditions = () => {
-  const defaultSchema = Object.keys(userSchema.attributes);
-  const currentSchema = Object.keys(
-    strapi.contentTypes['plugin::users-permissions.user'].attributes
-  );
-
-  // Some dynamic fields may not have been initialized yet, so we need to ignore them
-  // TODO: we should have a global method for finding these
-  const ignoreDiffs = [
-    'createdBy',
-    'createdAt',
-    'updatedBy',
-    'updatedAt',
-    'publishedAt',
-    'strapi_stage',
-    'strapi_assignee',
-    'locale',
-    'localizations',
-  ];
-
-  return currentSchema.filter((key) => !(ignoreDiffs.includes(key) || defaultSchema.includes(key)));
-};
-
 module.exports = async ({ strapi }) => {
   const pluginStore = strapi.store({ type: 'plugin', name: 'users-permissions' });
 
@@ -155,17 +129,4 @@ For security reasons, prefer storing the secret in an environment variable and r
       );
     }
   }
-
-  // TODO v5: Remove this block of code and default allowedFields to empty array
-  if (!isArray(strapi.config.get('plugin.users-permissions.register.allowedFields'))) {
-    const modifications = userSchemaAdditions();
-    if (modifications.length > 0) {
-      // if there is a potential vulnerability, show a warning
-      strapi.log.warn(
-        `Users-permissions registration has defaulted to accepting the following additional user fields during registration: ${modifications.join(
-          ','
-        )}`
-      );
-    }
-  }
 };

packages/plugins/users-permissions/server/controllers/auth.js

@@ -11,9 +11,6 @@ const crypto = require('crypto');
 const _ = require('lodash');
 const { concat, compact, isArray } = require('lodash/fp');
 const utils = require('@strapi/utils');
-const {
-  contentTypes: { getNonWritableAttributes },
-} = require('@strapi/utils');
 const { getService } = require('../utils');
 const {
   validateCallbackBody,
@@ -283,45 +280,20 @@ module.exports = {
 
     const { register } = strapi.config.get('plugin.users-permissions');
     const alwaysAllowedKeys = ['username', 'password', 'email'];
-    const userModel = strapi.contentTypes['plugin::users-permissions.user'];
-    const { attributes } = userModel;
-
-    const nonWritable = getNonWritableAttributes(userModel);
 
+    // Note that we intentionally do not filter allowedFields to allow a project to explicitly accept private or other Strapi field on registration
     const allowedKeys = compact(
-      concat(
+      concat(alwaysAllowedKeys, isArray(register?.allowedFields) ? register.allowedFields : [])
-        alwaysAllowedKeys,
-        isArray(register?.allowedFields)
-          ? // Note that we do not filter allowedFields in case a user explicitly chooses to allow a private or otherwise omitted field on registration
-            register.allowedFields // if null or undefined, compact will remove it
-          : // to prevent breaking changes, if allowedFields is not set in config, we only remove private and known dangerous user schema fields
-            // TODO V5: allowedFields defaults to [] when undefined and remove this case
-            Object.keys(attributes).filter(
-              (key) =>
-                !nonWritable.includes(key) &&
-                !attributes[key].private &&
-                ![
-                  // many of these are included in nonWritable, but we'll list them again to be safe and since we're removing this code in v5 anyway
-                  // Strapi user schema fields
-                  'confirmed',
-                  'blocked',
-                  'confirmationToken',
-                  'resetPasswordToken',
-                  'provider',
-                  'id',
-                  'role',
-                  // other Strapi fields that might be added
-                  'createdAt',
-                  'updatedAt',
-                  'createdBy',
-                  'updatedBy',
-                  'publishedAt', // d&p
-                  'strapi_reviewWorkflows_stage', // review workflows
-                ].includes(key)
-            )
-      )
     );
 
+    // Check if there are any keys in requestBody that are not in allowedKeys
+    const invalidKeys = Object.keys(ctx.request.body).filter((key) => !allowedKeys.includes(key));
+
+    if (invalidKeys.length > 0) {
+      // If there are invalid keys, throw an error
+      throw new ValidationError(`Invalid parameters: ${invalidKeys.join(', ')}`);
+    }
+
     const params = {
       ..._.pick(ctx.request.body, allowedKeys),
       provider: 'local',
@@ -366,6 +338,7 @@ module.exports = {
       }
     }
 
+    console.log('params', params);
     const newUser = {
       ...params,
       role: role.id,
@@ -378,6 +351,7 @@ module.exports = {
 
     const sanitizedUser = await sanitizeUser(user, ctx);
 
+    console.log('sanitizedUser', sanitizedUser);
     if (settings.email_confirmation) {
       try {
         await getService('user').sendConfirmationEmail(sanitizedUser);

packages/plugins/users-permissions/server/controllers/validation/__tests__/auth.test.js

@@ -0,0 +1,144 @@
+'use strict';
+
+const errors = require('@strapi/utils');
+const auth = require('../../auth');
+
+const mockStrapi = {
+  store: jest.fn(() => {
+    return {
+      get: jest.fn(() => {
+        return { allow_register: true };
+      }),
+    };
+  }),
+  config: {
+    get: jest.fn(() => {
+      return {
+        register: {
+          allowedFields: [],
+        },
+      };
+    }),
+  },
+  query: jest.fn(() => {
+    return {
+      findOne: jest.fn(() => {
+        return {
+          role: 1,
+        };
+      }),
+      count: jest.fn(() => {
+        return 0;
+      }),
+    };
+  }),
+  plugins: {
+    'users-permissions': {
+      controllers: {},
+      contentTypes: {},
+      policies: {},
+      services: {},
+    },
+  },
+  getModel: jest.fn(),
+};
+
+jest.mock('@strapi/utils', () => {
+  return {
+    ...jest.requireActual('@strapi/utils'),
+    sanitizeUser: jest.fn((input) => input),
+    sanitize: {
+      contentAPI: {
+        output: jest.fn((input) => input),
+      },
+    },
+  };
+});
+
+jest.mock('../../../utils', () => {
+  return {
+    getService: jest.fn(() => {
+      return {
+        add: jest.fn((user) => {
+          return user;
+        }),
+        issue: jest.fn(),
+      };
+    }),
+  };
+});
+
+describe('user-permissions auth', () => {
+  beforeAll(() => {
+    global.strapi = mockStrapi;
+  });
+
+  describe('register', () => {
+    test('accepts valid body', async () => {
+      const ctx = {
+        state: {
+          auth: {},
+        },
+        request: {
+          body: { username: 'testuser', email: 'test@example.com', password: 'Testpassword1!' },
+        },
+        send: jest.fn(),
+      };
+
+      await auth.register(ctx);
+      expect(ctx.send).toHaveBeenCalledTimes(1);
+    });
+
+    test("throws ValidationError when given fields that aren't allowed", async () => {
+      const ctx = {
+        state: {
+          auth: {},
+        },
+        request: {
+          body: {
+            confirmed: true,
+            username: 'testuser',
+            email: 'test@example.com',
+            password: 'Testpassword1!',
+          },
+        },
+        send: jest.fn(),
+      };
+      await expect(auth.register(ctx)).rejects.toThrow(errors.ValidationError);
+
+      expect(ctx.send).toHaveBeenCalledTimes(0);
+    });
+
+    test('allows exceptions from config register.allowedFields', async () => {
+      global.strapi = {
+        ...mockStrapi,
+        config: {
+          get: jest.fn(() => {
+            return {
+              register: {
+                allowedFields: ['confirmed'],
+              },
+            };
+          }),
+        },
+      };
+      const ctx = {
+        state: {
+          auth: {},
+        },
+        request: {
+          body: {
+            confirmed: true,
+            username: 'testuser',
+            email: 'test@example.com',
+            password: 'Testpassword1!',
+          },
+        },
+        send: jest.fn(),
+      };
+
+      await auth.register(ctx);
+      expect(ctx.send).toHaveBeenCalledTimes(1);
+    });
+  });
+});