fix(audit): mailer attachment validation + policy guard (#3392)

* fix(mailer): add attachment validation (#3390)

Adds validateAttachments() called in sendMail() before provider dispatch.
Enforces non-empty string filename, required content, and 25 MB size cap.
Adds 4 unit tests covering all rejection paths and the happy path.

* fix(policy): defensive guard on req.route + JSDoc (#3391)

- organizations.policy: return false early when req.route?.path is absent
  instead of falling through with an empty string
- policy.js deriveSubjectType: document the first-match-wins non-overlapping
  constraint on registered path subjects
- config.js filterByActivation: note that changing 'activated' requires restart

* test(mailer,policy): improve coverage for attachment validation and route guard (#3390 #3391)

Author: PierreBrisorgueil

lib/helpers/config.js

@@ -121,6 +121,10 @@ const extractModuleName = (filePath) => {
  * Core modules are never filtered out.
  * Missing `activated` key defaults to active (true).
  *
+ * NOTE: Changing the `activated` flag for a module in the environment config requires
+ * a full application restart to take effect — the file list is resolved at startup
+ * and is not re-evaluated at runtime.
+ *
  * @param {string[]} files - array of file paths
  * @param {object} config - merged configuration object
  * @returns {string[]} filtered file paths

lib/helpers/mailer/index.js

@@ -37,6 +37,31 @@ const getProvider = () => {
  */
 const isConfigured = () => !!(config.mailer && config.mailer.from && !String(config.mailer.from).startsWith('DEVKIT_NODE_'));
 
+/**
+ * @desc Validate attachment list before sending.
+ * Throws if any attachment is malformed or exceeds the 25 MB size limit.
+ * @param {Array} [attachments] - Optional array of attachment objects
+ * @throws {Error} If any attachment is invalid
+ */
+const validateAttachments = (attachments) => {
+  if (!Array.isArray(attachments)) return;
+  const MAX_SIZE = 25 * 1024 * 1024; // 25 MB
+  for (const att of attachments) {
+    if (!att.filename || typeof att.filename !== 'string') {
+      throw new Error('Attachment filename must be a non-empty string');
+    }
+    if (!att.content) {
+      throw new Error('Attachment content is required');
+    }
+    const size = typeof att.content === 'string'
+      ? Buffer.byteLength(att.content)
+      : att.content.length;
+    if (size > MAX_SIZE) {
+      throw new Error(`Attachment "${att.filename}" exceeds 25 MB limit`);
+    }
+  }
+};
+
 /**
  * @desc Send an email using a handlebars template
  * @param {Object} mail - Mail configuration
@@ -51,6 +76,7 @@ const isConfigured = () => !!(config.mailer && config.mailer.from && !String(con
  */
 const sendMail = async (mail) => {
   if (!isConfigured()) return null;
+  validateAttachments(mail.attachments);
   // Sanitize template name to prevent path traversal
   const sanitizedTemplate = path.basename(mail.template);
   const file = await files.readFile(path.resolve(`./config/templates/${sanitizedTemplate}.html`));

lib/helpers/mailer/tests/mailer.unit.tests.js

@@ -105,4 +105,54 @@ describe('mailer index with resend provider unit tests:', () => {
 
     expect(result).toBeNull();
   });
+
+  test('should throw when attachment is missing content', async () => {
+    await expect(
+      mailer.sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+        attachments: [{ filename: 'file.txt' }],
+      }),
+    ).rejects.toThrow('Attachment content is required');
+  });
+
+  test('should throw when attachment exceeds 25 MB', async () => {
+    const largeContent = Buffer.alloc(26 * 1024 * 1024); // 26 MB
+    await expect(
+      mailer.sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+        attachments: [{ filename: 'large.bin', content: largeContent }],
+      }),
+    ).rejects.toThrow('exceeds 25 MB limit');
+  });
+
+  test('should throw when attachment filename is not a string', async () => {
+    await expect(
+      mailer.sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+        attachments: [{ filename: 123, content: 'data' }],
+      }),
+    ).rejects.toThrow('Attachment filename must be a non-empty string');
+  });
+
+  test('should not throw for a valid attachment', async () => {
+    mockSend.mockResolvedValue({ id: 'ok', accepted: ['user@example.com'], rejected: [] });
+    await expect(
+      mailer.sendMail({
+        to: 'user@example.com',
+        subject: 'Test',
+        template: 'welcome',
+        params: { name: 'Bob' },
+        attachments: [{ filename: 'doc.pdf', content: 'PDF content here' }],
+      }),
+    ).resolves.not.toThrow();
+  });
 });

lib/middlewares/policy.js

@@ -210,6 +210,10 @@ const isAllowed = async (req, res, next) => {
  * Iterates the pathSubjectRegistry populated by module policy files.
  * Each entry's routeMatch can be an exact string, or a function(routePath) => boolean.
  * First match wins.
+ *
+ * IMPORTANT: registered route paths must be non-overlapping (or ordered from most-specific
+ * to least-specific), because resolution is first-match-wins. If two entries could match the
+ * same path, only the first registered entry will ever be used.
  * @param {string} routePath - Express route path (e.g. '/api/tasks' or '/api/users/me')
  * @returns {string|null} CASL subject type or null if not mappable
  */

modules/organizations/policies/organizations.policy.js

@@ -16,7 +16,10 @@ export function organizationSubjectRegistration({ registerDocumentSubject, regis
   // Guard: only resolve req.organization as an Organization subject on actual organization routes.
   // Other modules (billing, tasks, etc.) also set req.organization but authorize via their own subjects.
   registerDocumentSubject('organization', 'Organization', (req) => {
-    const p = req.route?.path || '';
+    if (!req.route?.path) {
+      return false;
+    }
+    const p = req.route.path;
     return p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations');
   });
   registerPathSubject((p) => p.startsWith('/api/admin/organizations'), 'Organization');

modules/organizations/tests/organizations.policy.unit.tests.js

@@ -0,0 +1,69 @@
+/**
+ * Unit tests for organizationSubjectRegistration policy guard.
+ */
+import { describe, test, expect, jest, beforeEach } from '@jest/globals';
+
+import { organizationSubjectRegistration } from '../policies/organizations.policy.js';
+
+/**
+ * Build a mock subject-registration registry that captures registered resolvers.
+ * @returns {{ registerDocumentSubject: jest.Mock, registerPathSubject: jest.Mock, _documentResolvers: Map }}
+ */
+function mockRegistry() {
+  const documentResolvers = new Map();
+  return {
+    registerDocumentSubject: jest.fn((prop, type, guard) => {
+      documentResolvers.set(prop, { type, guard });
+    }),
+    registerPathSubject: jest.fn(),
+    _documentResolvers: documentResolvers,
+  };
+}
+
+describe('organizationSubjectRegistration policy unit tests:', () => {
+  test('should register membershipDoc, organization, and path subjects', () => {
+    const registry = mockRegistry();
+    organizationSubjectRegistration(registry);
+
+    expect(registry.registerDocumentSubject).toHaveBeenCalledWith('membershipDoc', 'Membership');
+    expect(registry.registerDocumentSubject).toHaveBeenCalledWith('organization', 'Organization', expect.any(Function));
+    expect(registry.registerPathSubject).toHaveBeenCalledTimes(4);
+  });
+
+  describe('organization document subject guard:', () => {
+    let guard;
+
+    beforeEach(() => {
+      const registry = mockRegistry();
+      organizationSubjectRegistration(registry);
+      guard = registry._documentResolvers.get('organization').guard;
+    });
+
+    test('should return false when req.route is undefined', () => {
+      expect(guard({ route: undefined })).toBe(false);
+    });
+
+    test('should return false when req.route.path is undefined', () => {
+      expect(guard({ route: {} })).toBe(false);
+    });
+
+    test('should return false when req.route.path is an empty string', () => {
+      expect(guard({ route: { path: '' } })).toBe(false);
+    });
+
+    test('should return true for /api/organizations paths', () => {
+      expect(guard({ route: { path: '/api/organizations' } })).toBe(true);
+      expect(guard({ route: { path: '/api/organizations/:id' } })).toBe(true);
+    });
+
+    test('should return true for /api/admin/organizations paths', () => {
+      expect(guard({ route: { path: '/api/admin/organizations' } })).toBe(true);
+      expect(guard({ route: { path: '/api/admin/organizations/:id' } })).toBe(true);
+    });
+
+    test('should return false for unrelated paths (e.g. billing routes)', () => {
+      expect(guard({ route: { path: '/api/billing/plans' } })).toBe(false);
+      expect(guard({ route: { path: '/api/tasks' } })).toBe(false);
+    });
+  });
+});