Fix: Query Validation in Files' Admin API (#20389)

Co-authored-by: Ben Irvin <ben.irvin@strapi.io>

packages/core/upload/server/controllers/admin-file.js

@@ -1,7 +1,7 @@
 'use strict';
 
 const { merge } = require('lodash/fp');
-const { mapAsync } = require('@strapi/utils');
+const { mapAsync, pipeAsync } = require('@strapi/utils');
 const { getService } = require('../utils');
 const { ACTIONS, FILE_MODEL_UID } = require('../constants');
 const { findEntityAndCheckPermissions } = require('./utils/find-entity-and-check-permissions');
@@ -24,10 +24,17 @@ module.exports = {
       return ctx.forbidden();
     }
 
-    const pmQuery = pm.addPermissionsQueryTo(merge(defaultQuery, ctx.query));
+    // validate the incoming user query params
+    await pm.validateQuery(ctx.query);
 
-    await pm.validateQuery(pmQuery);
-    const query = await pm.sanitizeQuery(pmQuery);
+    const query = await pipeAsync(
+      // Start by sanitizing the incoming query
+      (q) => pm.sanitizeQuery(q),
+      // Add the default query which should not be validated or sanitized
+      (q) => merge(defaultQuery, q),
+      // Add the dynamic filters based on permissions' conditions
+      (q) => pm.addPermissionsQueryTo(q)
+    )(ctx.query);
 
     const { results: files, pagination } = await getService('upload').findPage(query);
 

tests/api/core/upload/admin/file.test.api.js

@@ -6,10 +6,12 @@ const path = require('path');
 const { createTestBuilder } = require('api-tests/builder');
 const { createStrapiInstance } = require('api-tests/strapi');
 const { createAuthRequest } = require('api-tests/request');
+const { createUtils } = require('api-tests/utils');
 
 const builder = createTestBuilder();
 let strapi;
 let rq;
+let utils;
 
 const dogModel = {
   displayName: 'Dog',
@@ -28,6 +30,7 @@ describe('Upload', () => {
     await builder.addContentType(dogModel).build();
     strapi = await createStrapiInstance();
     rq = await createAuthRequest({ strapi });
+    utils = createUtils(strapi);
   });
 
   afterAll(async () => {
@@ -53,6 +56,81 @@ describe('Upload', () => {
   });
 
   describe('Read', () => {
+    let uploadReaderRole;
+
+    let u1Req;
+    let u2Req;
+
+    const users = { u1: null, u2: null };
+
+    beforeAll(async () => {
+      uploadReaderRole = await utils.createRole({
+        name: 'UploadReader',
+        description: 'Can only see files created by same role as creator',
+      });
+
+      // Add permissions to the role with conditions
+      // This is important in order to dynamically add filters with sensitive fields to the final query
+      await utils.assignPermissionsToRole(uploadReaderRole.id, [
+        {
+          action: 'plugin::upload.read',
+          subject: null,
+          conditions: ['admin::has-same-role-as-creator'],
+          properties: {},
+        },
+        {
+          action: 'plugin::upload.assets.create',
+          subject: null,
+          conditions: ['admin::has-same-role-as-creator'],
+          properties: {},
+        },
+        {
+          action: 'plugin::upload.assets.update',
+          subject: null,
+          conditions: ['admin::has-same-role-as-creator'],
+          properties: {},
+        },
+      ]);
+
+      // TODO: We create 2 users in order to be able to test the condition itself (same role as creator)
+
+      users.u1 = await utils.createUser({
+        firstname: 'reader1',
+        lastname: 'reader1',
+        email: 'reader1@strapi.io',
+        password: 'Reader1',
+        isActive: true,
+        roles: [uploadReaderRole.id],
+      });
+
+      users.u2 = await utils.createUser({
+        firstname: 'reader2',
+        lastname: 'reader2',
+        email: 'reader2@strapi.io',
+        password: 'Reader2',
+        isActive: true,
+        roles: [uploadReaderRole.id],
+      });
+
+      // Users' requests
+
+      u1Req = await createAuthRequest({
+        strapi,
+        userInfo: { email: 'reader1@strapi.io', password: 'Reader1' },
+      });
+
+      u2Req = await createAuthRequest({
+        strapi,
+        userInfo: { email: 'reader2@strapi.io', password: 'Reader2' },
+      });
+    });
+
+    // Cleanup test fixtures
+    afterAll(async () => {
+      await utils.deleteUsersById([users.u1.id, users.u2.id]);
+      await utils.deleteRolesById([uploadReaderRole.id]);
+    });
+
     test('GET /upload/files => Find files', async () => {
       const res = await rq({ method: 'GET', url: '/upload/files' });
 
@@ -73,5 +151,23 @@ describe('Upload', () => {
       });
       res.body.results.forEach((file) => expect(file.folder).toBeDefined());
     });
+
+    test(`Using custom conditions don't trigger validation errors for dynamically added fields`, async () => {
+      const res = await u1Req({ method: 'GET', url: '/upload/files' });
+
+      // The request succeed, no validation error
+      expect(res.statusCode).toBe(200);
+
+      // No data is returned, the condition is successfully applied (u1 did not upload any file)
+      expect(res.body).toEqual({
+        results: [],
+        pagination: {
+          page: expect.any(Number),
+          pageSize: expect.any(Number),
+          pageCount: expect.any(Number),
+          total: expect.any(Number),
+        },
+      });
+    });
   });
 });