From 6cb5feda9eb9c313db7e1101010c534e0d9846e7 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 6 Apr 2026 10:08:33 +0200 Subject: [PATCH 1/5] fix(policy): warn at startup if module has abilities without SubjectRegistration (#3395) Track hasSubjectRegistration per entry during discoverPolicies; emit a logger.warn when abilities/guestAbilities are exported but no *SubjectRegistration function is found, so misconfigured modules are caught at startup instead of silently failing CASL document-level checks. --- lib/middlewares/policy.js | 8 +++ .../tests/fixtures/policy-abilities-only.js | 4 ++ .../fixtures/policy-guest-abilities-only.js | 4 ++ .../fixtures/policy-registration-only.js | 4 ++ .../fixtures/policy-with-registration.js | 8 +++ lib/middlewares/tests/policy.unit.tests.js | 67 +++++++++++++++++++ 6 files changed, 95 insertions(+) create mode 100644 lib/middlewares/tests/fixtures/policy-abilities-only.js create mode 100644 lib/middlewares/tests/fixtures/policy-guest-abilities-only.js create mode 100644 lib/middlewares/tests/fixtures/policy-registration-only.js create mode 100644 lib/middlewares/tests/fixtures/policy-with-registration.js create mode 100644 lib/middlewares/tests/policy.unit.tests.js diff --git a/lib/middlewares/policy.js b/lib/middlewares/policy.js index d251f56f6..b3f5230b4 100644 --- a/lib/middlewares/policy.js +++ b/lib/middlewares/policy.js @@ -3,6 +3,7 @@ */ import path from 'path'; import responses from '../helpers/responses.js'; +import logger from '../services/logger.js'; const methodToAction = { get: 'read', post: 'create', put: 'update', patch: 'update', delete: 'delete', @@ -255,9 +256,16 @@ const discoverPolicies = async (policyPaths) => { } // Call subject registration functions exported by module policy files if (normalizedKey.endsWith('subjectregistration')) { + entry.hasSubjectRegistration = true; value({ registerDocumentSubject, registerPathSubject }); } } + // Warn if the module exports abilities but no SubjectRegistration — document subjects will not be resolved + if ((entry.abilities || entry.guestAbilities) && !entry.hasSubjectRegistration) { + logger.warn( + `[policy] ${path.basename(policyPath)}: exports abilities but no SubjectRegistration — document subjects will not be resolved`, + ); + } // Also support old invokeRolesPolicies pattern during transition — skip if new-style exports found if (entry.abilities || entry.guestAbilities) { registerAbilities(entry); diff --git a/lib/middlewares/tests/fixtures/policy-abilities-only.js b/lib/middlewares/tests/fixtures/policy-abilities-only.js new file mode 100644 index 000000000..845342485 --- /dev/null +++ b/lib/middlewares/tests/fixtures/policy-abilities-only.js @@ -0,0 +1,4 @@ +// Fixture: policy file with abilities but no SubjectRegistration +export function taskAbilities(user, membership, { can }) { + can('read', 'Task'); +} diff --git a/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js b/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js new file mode 100644 index 000000000..6aec346cd --- /dev/null +++ b/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js @@ -0,0 +1,4 @@ +// Fixture: policy file with guestAbilities but no SubjectRegistration +export function taskGuestAbilities({ can }) { + can('read', 'Task'); +} diff --git a/lib/middlewares/tests/fixtures/policy-registration-only.js b/lib/middlewares/tests/fixtures/policy-registration-only.js new file mode 100644 index 000000000..604a468a3 --- /dev/null +++ b/lib/middlewares/tests/fixtures/policy-registration-only.js @@ -0,0 +1,4 @@ +// Fixture: policy file with SubjectRegistration only (no abilities) +export function taskSubjectRegistration({ registerDocumentSubject }) { + registerDocumentSubject('task', 'Task'); +} diff --git a/lib/middlewares/tests/fixtures/policy-with-registration.js b/lib/middlewares/tests/fixtures/policy-with-registration.js new file mode 100644 index 000000000..c2988f9ee --- /dev/null +++ b/lib/middlewares/tests/fixtures/policy-with-registration.js @@ -0,0 +1,8 @@ +// Fixture: policy file with abilities AND SubjectRegistration +export function taskAbilities(user, membership, { can }) { + can('read', 'Task'); +} + +export function taskSubjectRegistration({ registerDocumentSubject }) { + registerDocumentSubject('task', 'Task'); +} diff --git a/lib/middlewares/tests/policy.unit.tests.js b/lib/middlewares/tests/policy.unit.tests.js new file mode 100644 index 000000000..aefd616b1 --- /dev/null +++ b/lib/middlewares/tests/policy.unit.tests.js @@ -0,0 +1,67 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; +import { fileURLToPath } from 'url'; +import path from 'path'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +// Mock logger before importing policy +jest.unstable_mockModule('../../services/logger.js', () => ({ + default: { + warn: jest.fn(), + info: jest.fn(), + error: jest.fn(), + }, +})); + +// Mock @casl/ability +jest.unstable_mockModule('@casl/ability', () => ({ + AbilityBuilder: jest.fn().mockImplementation(() => ({ + can: jest.fn(), + cannot: jest.fn(), + build: jest.fn().mockReturnValue({ can: jest.fn().mockReturnValue(true) }), + })), + Ability: jest.fn(), + subject: jest.fn((type, doc) => doc), +})); + +const { default: logger } = await import('../../services/logger.js'); +const { default: policy } = await import('../policy.js'); + +const fixture = (name) => path.join(__dirname, 'fixtures', name); + +describe('policy discoverPolicies unit tests:', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('should warn when a policy file exports abilities but no SubjectRegistration', async () => { + await policy.discoverPolicies([fixture('policy-abilities-only.js')]); + + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('exports abilities but no SubjectRegistration'), + ); + }); + + test('should not warn when a policy file exports both abilities and SubjectRegistration', async () => { + await policy.discoverPolicies([fixture('policy-with-registration.js')]); + + expect(logger.warn).not.toHaveBeenCalled(); + }); + + test('should not warn when a policy file exports only SubjectRegistration without abilities', async () => { + await policy.discoverPolicies([fixture('policy-registration-only.js')]); + + expect(logger.warn).not.toHaveBeenCalled(); + }); + + test('should warn when a policy file exports guestAbilities but no SubjectRegistration', async () => { + await policy.discoverPolicies([fixture('policy-guest-abilities-only.js')]); + + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('exports abilities but no SubjectRegistration'), + ); + }); +}); From c9ba2e9fadcd4e01b083bcbf97acef2491698373 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 6 Apr 2026 10:08:38 +0200 Subject: [PATCH 2/5] test(mailer): add Buffer attachment and empty filename coverage (#3396) Add test for Buffer content encoding to base64 in ResendProvider, and a test that validates sendMail rejects attachments with an empty string filename. --- lib/helpers/mailer/tests/mailer.unit.tests.js | 12 +++++++++++ .../tests/provider.resend.unit.tests.js | 20 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/helpers/mailer/tests/mailer.unit.tests.js b/lib/helpers/mailer/tests/mailer.unit.tests.js index 86a8d96f1..faf000344 100644 --- a/lib/helpers/mailer/tests/mailer.unit.tests.js +++ b/lib/helpers/mailer/tests/mailer.unit.tests.js @@ -131,6 +131,18 @@ describe('mailer index with resend provider unit tests:', () => { ).rejects.toThrow('exceeds 25 MB limit'); }); + test('should throw when attachment filename is an empty string', async () => { + await expect( + mailer.sendMail({ + to: 'user@example.com', + subject: 'Test', + template: 'welcome', + params: { name: 'Bob' }, + attachments: [{ filename: '', content: 'data' }], + }), + ).rejects.toThrow('Attachment filename must be a non-empty string'); + }); + test('should throw when attachment filename is not a string', async () => { await expect( mailer.sendMail({ diff --git a/lib/helpers/mailer/tests/provider.resend.unit.tests.js b/lib/helpers/mailer/tests/provider.resend.unit.tests.js index e52b619d1..762dfd545 100644 --- a/lib/helpers/mailer/tests/provider.resend.unit.tests.js +++ b/lib/helpers/mailer/tests/provider.resend.unit.tests.js @@ -84,6 +84,26 @@ describe('ResendProvider unit tests:', () => { expect(result).toEqual(mockData); }); + test('should encode Buffer attachment content to base64', async () => { + const buffer = Buffer.from('binary data'); + const mockData = { id: 'email_buf' }; + sendMock.mockResolvedValue({ data: mockData, error: null }); + + await provider.send({ + from: 'from@test.com', + to: 'to@test.com', + subject: 'Test', + html: '

test

', + attachments: [{ filename: 'data.bin', content: buffer }], + }); + + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + attachments: [{ filename: 'data.bin', content: Buffer.from(buffer).toString('base64') }], + }), + ); + }); + test('should throw on Resend API error', async () => { sendMock.mockResolvedValue({ data: null, error: { message: 'Invalid API key' } }); From 324a4235f876049da519f75c9d140c14a6f2d6d9 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 6 Apr 2026 10:15:15 +0200 Subject: [PATCH 3/5] fix(jest): exclude tests/fixtures/ dirs from testPathIgnorePatterns --- jest.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jest.config.js b/jest.config.js index 638f010a6..b62d14e2d 100644 --- a/jest.config.js +++ b/jest.config.js @@ -180,7 +180,7 @@ export default { testMatch: ['/modules/*/tests/**/*.js', '/lib/**/tests/**/*.js'], // An array of regexp pattern strings that are matched against all test paths, matched tests are skipped - testPathIgnorePatterns: ['/node_modules/'], + testPathIgnorePatterns: ['/node_modules/', '/tests/fixtures/'], // The regexp pattern Jest uses to detect test files // testRegex: "", From e9fe66e23212e20c876a9cce16190a7adafbcf11 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 6 Apr 2026 10:20:35 +0200 Subject: [PATCH 4/5] fix(jest): exclude tests/fixtures from coverage collection --- jest.config.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/jest.config.js b/jest.config.js index b62d14e2d..5dd03f271 100644 --- a/jest.config.js +++ b/jest.config.js @@ -57,6 +57,8 @@ export default { '!/modules/**/migrations/**', // Exclude static upload config — just object literals, like config/defaults '!/modules/uploads/config/config.uploads.js', + // Exclude test fixtures — helper stubs used by tests, not production code + '!/**/tests/fixtures/**', ], // The directory where Jest should output its coverage files coverageDirectory: 'coverage', From 3ac4b6dc9ad2fb9efc842fdadf60a1aba498cdf8 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 6 Apr 2026 10:25:22 +0200 Subject: [PATCH 5/5] fix(policy): improve warn message wording, add JSDoc to fixtures, prefix unused params --- lib/middlewares/policy.js | 4 ++-- .../tests/fixtures/policy-abilities-only.js | 10 +++++++++- .../fixtures/policy-guest-abilities-only.js | 6 ++++++ .../tests/fixtures/policy-registration-only.js | 6 ++++++ .../tests/fixtures/policy-with-registration.js | 16 +++++++++++++++- lib/middlewares/tests/policy.unit.tests.js | 11 ++++++++--- 6 files changed, 46 insertions(+), 7 deletions(-) diff --git a/lib/middlewares/policy.js b/lib/middlewares/policy.js index b3f5230b4..3ad2be908 100644 --- a/lib/middlewares/policy.js +++ b/lib/middlewares/policy.js @@ -260,10 +260,10 @@ const discoverPolicies = async (policyPaths) => { value({ registerDocumentSubject, registerPathSubject }); } } - // Warn if the module exports abilities but no SubjectRegistration — document subjects will not be resolved + // Warn if the module exports abilities/guestAbilities but no SubjectRegistration — document subjects will not be resolved if ((entry.abilities || entry.guestAbilities) && !entry.hasSubjectRegistration) { logger.warn( - `[policy] ${path.basename(policyPath)}: exports abilities but no SubjectRegistration — document subjects will not be resolved`, + `[policy] ${path.basename(policyPath)}: exports abilities/guestAbilities but no SubjectRegistration — document subjects will not be resolved`, ); } // Also support old invokeRolesPolicies pattern during transition — skip if new-style exports found diff --git a/lib/middlewares/tests/fixtures/policy-abilities-only.js b/lib/middlewares/tests/fixtures/policy-abilities-only.js index 845342485..fd61f7436 100644 --- a/lib/middlewares/tests/fixtures/policy-abilities-only.js +++ b/lib/middlewares/tests/fixtures/policy-abilities-only.js @@ -1,4 +1,12 @@ // Fixture: policy file with abilities but no SubjectRegistration -export function taskAbilities(user, membership, { can }) { +/** + * Define task abilities for authenticated users (fixture without SubjectRegistration). + * @param {Object} _user - The authenticated user (unused in fixture) + * @param {Object} _membership - Optional organization membership (unused in fixture) + * @param {Object} context - CASL ability builder context + * @param {Function} context.can - Function to grant abilities + * @returns {void} + */ +export function taskAbilities(_user, _membership, { can }) { can('read', 'Task'); } diff --git a/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js b/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js index 6aec346cd..22e56df28 100644 --- a/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js +++ b/lib/middlewares/tests/fixtures/policy-guest-abilities-only.js @@ -1,4 +1,10 @@ // Fixture: policy file with guestAbilities but no SubjectRegistration +/** + * Define task abilities for guest users (fixture without SubjectRegistration). + * @param {Object} context - CASL ability builder context + * @param {Function} context.can - Function to grant abilities + * @returns {void} + */ export function taskGuestAbilities({ can }) { can('read', 'Task'); } diff --git a/lib/middlewares/tests/fixtures/policy-registration-only.js b/lib/middlewares/tests/fixtures/policy-registration-only.js index 604a468a3..ed150fa0d 100644 --- a/lib/middlewares/tests/fixtures/policy-registration-only.js +++ b/lib/middlewares/tests/fixtures/policy-registration-only.js @@ -1,4 +1,10 @@ // Fixture: policy file with SubjectRegistration only (no abilities) +/** + * Register task document subject for CASL resolution. + * @param {Object} context - Registration context + * @param {Function} context.registerDocumentSubject - Function to register document subjects + * @returns {void} + */ export function taskSubjectRegistration({ registerDocumentSubject }) { registerDocumentSubject('task', 'Task'); } diff --git a/lib/middlewares/tests/fixtures/policy-with-registration.js b/lib/middlewares/tests/fixtures/policy-with-registration.js index c2988f9ee..900b2445c 100644 --- a/lib/middlewares/tests/fixtures/policy-with-registration.js +++ b/lib/middlewares/tests/fixtures/policy-with-registration.js @@ -1,8 +1,22 @@ // Fixture: policy file with abilities AND SubjectRegistration -export function taskAbilities(user, membership, { can }) { +/** + * Define task abilities for authenticated users. + * @param {Object} _user - The authenticated user (unused in fixture) + * @param {Object} _membership - Optional organization membership (unused in fixture) + * @param {Object} context - CASL ability builder context + * @param {Function} context.can - Function to grant abilities + * @returns {void} + */ +export function taskAbilities(_user, _membership, { can }) { can('read', 'Task'); } +/** + * Register task document subject for CASL resolution. + * @param {Object} context - Registration context + * @param {Function} context.registerDocumentSubject - Function to register document subjects + * @returns {void} + */ export function taskSubjectRegistration({ registerDocumentSubject }) { registerDocumentSubject('task', 'Task'); } diff --git a/lib/middlewares/tests/policy.unit.tests.js b/lib/middlewares/tests/policy.unit.tests.js index aefd616b1..ec6fc1681 100644 --- a/lib/middlewares/tests/policy.unit.tests.js +++ b/lib/middlewares/tests/policy.unit.tests.js @@ -30,6 +30,11 @@ jest.unstable_mockModule('@casl/ability', () => ({ const { default: logger } = await import('../../services/logger.js'); const { default: policy } = await import('../policy.js'); +/** + * Build absolute path to a test fixture file. + * @param {string} name - Fixture filename + * @returns {string} Absolute path to the fixture + */ const fixture = (name) => path.join(__dirname, 'fixtures', name); describe('policy discoverPolicies unit tests:', () => { @@ -37,11 +42,11 @@ describe('policy discoverPolicies unit tests:', () => { jest.clearAllMocks(); }); - test('should warn when a policy file exports abilities but no SubjectRegistration', async () => { + test('should warn when a policy file exports abilities/guestAbilities but no SubjectRegistration', async () => { await policy.discoverPolicies([fixture('policy-abilities-only.js')]); expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('exports abilities but no SubjectRegistration'), + expect.stringContaining('exports abilities/guestAbilities but no SubjectRegistration'), ); }); @@ -61,7 +66,7 @@ describe('policy discoverPolicies unit tests:', () => { await policy.discoverPolicies([fixture('policy-guest-abilities-only.js')]); expect(logger.warn).toHaveBeenCalledWith( - expect.stringContaining('exports abilities but no SubjectRegistration'), + expect.stringContaining('exports abilities/guestAbilities but no SubjectRegistration'), ); }); });