diff --git a/docs/docs/docs/01-core/admin/01-ee/00-intro.md b/docs/docs/docs/01-core/admin/01-ee/00-intro.md index 373c1182be..8b7818bfa6 100644 --- a/docs/docs/docs/01-core/admin/01-ee/00-intro.md +++ b/docs/docs/docs/01-core/admin/01-ee/00-intro.md @@ -34,6 +34,7 @@ Everytime a new EE feature is added in Strapi, in the settings menu, you should }, to: '/settings/purchase-new-ee-feature', id: 'new-ee-feature', + // TODO: to replace with another name in v5 lockIcon: true, }, ] diff --git a/packages/core/admin/admin/src/constants.ts b/packages/core/admin/admin/src/constants.ts index becb3e7660..3d2cdf3545 100644 --- a/packages/core/admin/admin/src/constants.ts +++ b/packages/core/admin/admin/src/constants.ts @@ -125,7 +125,7 @@ export const HOOKS = { export interface SettingsMenuLink extends Omit { - lockIcon?: boolean; + lockIcon?: boolean; // TODO: to replace with another name in v5 } export type SettingsMenu = { @@ -164,7 +164,7 @@ export const SETTINGS_LINKS_CE = (): SettingsMenu => ({ intlLabel: { id: 'Settings.sso.title', defaultMessage: 'Single Sign-On' }, to: '/settings/purchase-single-sign-on', id: 'sso-purchase-page', - lockIcon: true, + lockIcon: true, // TODO: to replace with another name in v5 }, ] : []), @@ -189,7 +189,7 @@ export const SETTINGS_LINKS_CE = (): SettingsMenu => ({ intlLabel: { id: 'global.auditLogs', defaultMessage: 'Audit Logs' }, to: '/settings/purchase-audit-logs', id: 'auditLogs-purchase-page', - lockIcon: true, + lockIcon: true, // TODO: to replace with another name in v5 }, ] : []), diff --git a/packages/core/admin/admin/src/hooks/useSettingsMenu.ts b/packages/core/admin/admin/src/hooks/useSettingsMenu.ts index b40092a075..f7bcb0dabb 100644 --- a/packages/core/admin/admin/src/hooks/useSettingsMenu.ts +++ b/packages/core/admin/admin/src/hooks/useSettingsMenu.ts @@ -30,7 +30,7 @@ interface SettingsMenuLinkWithPermissions extends SettingsMenuLink { } interface StrapiAppSettingsLink extends IStrapiAppSettingLink { - lockIcon?: never; + lockIcon?: never; // TODO: to replace with another name in v5 hasNotification?: never; } diff --git a/packages/core/core/src/middlewares/__tests__/security.test.ts b/packages/core/core/src/middlewares/__tests__/security.test.ts new file mode 100644 index 0000000000..b291c082fe --- /dev/null +++ b/packages/core/core/src/middlewares/__tests__/security.test.ts @@ -0,0 +1,67 @@ +import Koa from 'koa'; +import request from 'supertest'; +import { security } from '../security'; + +const parseCspHeader = (csp: string) => + Object.fromEntries( + csp + .split(';') + .map((directive) => directive.split(' ')) + .map(([k, ...v]) => [k, v]) + ); + +describe('Security middleware', () => { + describe('Content security policy', () => { + // GIVEN + const app = new Koa(); + const securityMiddleware = security( + { + contentSecurityPolicy: { + useDefaults: true, + directives: { + 'script-src': ["'self'", 'https://cdn.custom.com'], + upgradeInsecureRequests: null, + }, + }, + }, + { + strapi: { + plugin: () => null, + } as any, + } + )!; + + // WHEN + app.use(securityMiddleware); + const agent = request.agent(app.callback()); + + // THEN + it.each(['/', '/admin', '/api'])( + 'includes user custom CSP directives in GET %s response', + async (path) => { + await agent.get(path).expect((req) => { + const csp = parseCspHeader(req.header['content-security-policy']); + expect(csp['script-src']).toContain('https://cdn.custom.com'); + }); + } + ); + + it('includes required default CSP directives in GET /admin response', async () => { + await agent.get('/admin').expect((req) => { + const csp = parseCspHeader(req.header['content-security-policy']); + expect(csp['script-src']).toContain("'unsafe-inline'"); + expect(csp['connect-src']).toContain('ws:'); + }); + }); + + it('includes required default CSP directives in GET /documentation response', async () => { + await agent.get('/documentation').expect((req) => { + const csp = parseCspHeader(req.header['content-security-policy']); + expect(csp['script-src']).toContain("'unsafe-inline'"); + expect(csp['script-src']).toContain('cdn.jsdelivr.net'); + expect(csp['img-src']).toContain('strapi.io'); + expect(csp['img-src']).toContain('cdn.jsdelivr.net'); + }); + }); + }); +}); diff --git a/packages/core/core/src/middlewares/security.ts b/packages/core/core/src/middlewares/security.ts index 6d1ccc9064..7384172de1 100644 --- a/packages/core/core/src/middlewares/security.ts +++ b/packages/core/core/src/middlewares/security.ts @@ -1,4 +1,4 @@ -import { defaultsDeep, merge } from 'lodash/fp'; +import { defaultsDeep, mergeWith } from 'lodash/fp'; import helmet, { KoaHelmet } from 'koa-helmet'; import type { Core } from '@strapi/types'; @@ -29,6 +29,14 @@ const defaults: Config = { }, }; +const mergeConfig = (existingConfig: Config, newConfig: Config) => { + return mergeWith( + (obj, src) => (Array.isArray(obj) && Array.isArray(src) ? obj.concat(src) : undefined), + existingConfig, + newConfig + ); +}; + export const security: Core.MiddlewareFactory = (config, { strapi }) => (ctx, next) => { @@ -63,7 +71,7 @@ export const security: Core.MiddlewareFactory = // TODO: we shouldn't combine playground exceptions with documentation for all routes, we should first check the path and then return exceptions specific to that if (ctx.method === 'GET' && specialPaths.some((str) => ctx.path.startsWith(str))) { - helmetConfig = merge(helmetConfig, { + helmetConfig = mergeConfig(helmetConfig, { crossOriginEmbedderPolicy: false, // TODO: only use this for graphql playground contentSecurityPolicy: { directives, @@ -80,11 +88,11 @@ export const security: Core.MiddlewareFactory = * that are part of the admin route. */ if ( - process.env.NODE_ENV === 'development' && + ['development', 'test'].includes(process.env.NODE_ENV ?? '') && ctx.method === 'GET' && ['/admin'].some((str) => ctx.path.startsWith(str)) ) { - helmetConfig = merge(helmetConfig, { + helmetConfig = mergeConfig(helmetConfig, { contentSecurityPolicy: { directives: { 'script-src': ["'self'", "'unsafe-inline'"], diff --git a/packages/core/upload/server/src/controllers/admin-file.ts b/packages/core/upload/server/src/controllers/admin-file.ts index 54cbe8b3bd..6f355cd257 100644 --- a/packages/core/upload/server/src/controllers/admin-file.ts +++ b/packages/core/upload/server/src/controllers/admin-file.ts @@ -25,10 +25,17 @@ export default { 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 async.pipe( + // 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); diff --git a/tests/api/core/upload/admin/file.test.api.js b/tests/api/core/upload/admin/file.test.api.js index 3cae41ebc7..ee3e5a2f6d 100644 --- a/tests/api/core/upload/admin/file.test.api.js +++ b/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), + }, + }); + }); }); }); diff --git a/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts b/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts index 1617f2ef20..72c935fbf5 100644 --- a/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts +++ b/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts @@ -5,7 +5,7 @@ import { waitForRestart } from '../../../utils/restart'; import { resetFiles } from '../../../utils/file-reset'; import { createCollectionType, navToHeader, skipCtbTour } from '../../../utils/shared'; -test.describe('Edit collection type', () => { +test.skip('Edit collection type', () => { // use a name with a capital and a space to ensure we also test the kebab-casing conversion for api ids const ctName = 'Secret Document'; diff --git a/tests/e2e/tests/content-type-builder/single-type/edit-single-type.spec.ts b/tests/e2e/tests/content-type-builder/single-type/edit-single-type.spec.ts index b2e7e745bc..a460ada39f 100644 --- a/tests/e2e/tests/content-type-builder/single-type/edit-single-type.spec.ts +++ b/tests/e2e/tests/content-type-builder/single-type/edit-single-type.spec.ts @@ -5,7 +5,7 @@ import { waitForRestart } from '../../../utils/restart'; import { resetFiles } from '../../../utils/file-reset'; import { createSingleType, navToHeader, skipCtbTour } from '../../../utils/shared'; -test.describe('Edit single type', () => { +test.skip('Edit single type', () => { // use a name with a capital and a space to ensure we also test the kebab-casing conversion for api ids const ctName = 'Secret Document'; @@ -36,7 +36,7 @@ test.describe('Edit single type', () => { }); test('Can toggle internationalization', async ({ page }) => { - await page.getByRole('button', { name: 'Edit' }).click(); + await page.getByRole('button', { name: 'Edit', exact: true }).click(); await page.getByRole('tab', { name: 'Advanced settings' }).click(); await page.getByText('Internationalization').click(); await page.getByRole('button', { name: 'Finish' }).click(); @@ -47,7 +47,7 @@ test.describe('Edit single type', () => { }); test('Can toggle draft&publish', async ({ page }) => { - await page.getByRole('button', { name: 'Edit' }).click(); + await page.getByRole('button', { name: 'Edit', exact: true }).click(); await page.getByRole('tab', { name: 'Advanced settings' }).click(); await page.getByText('Draft & publish').click(); await page.getByRole('button', { name: 'Yes, disable' }).click();