Fix SECURITY ISSUE 1: File Attachments enables stored XSS (High).

Thanks to Siam Thanat Hack (STH) !

models/attachments.js

@@ -328,11 +328,35 @@ Attachments.getAttachmentsWithBackwardCompatibility = getAttachmentsWithBackward
 
 // Override the link method to use universal URLs
 if (Meteor.isClient) {
-  // Add custom link method to attachment documents
+  // Override the original FilesCollection link method to use universal URLs
+  // This must override the ostrio:files method to avoid "Match error: Expected plain object"
+  const originalLink = Attachments.link;
+  Attachments.link = function(versionName) {
+    // Accept both direct calls and collection.helpers style calls
+    const fileRef = this._id ? this : (versionName && versionName._id ? versionName : this);
+    const version = (typeof versionName === 'string') ? versionName : 'original';
+    
+    if (fileRef && fileRef._id) {
+      const url = generateUniversalAttachmentUrl(fileRef._id, version);
+      if (process.env.DEBUG === 'true') {
+        console.log('Attachment link generated:', url, 'for ID:', fileRef._id);
+      }
+      return url;
+    }
+    // Fallback to original if somehow we don't have an ID
+    return originalLink ? originalLink.call(this, versionName) : '';
+  };
+  
+  // Also add as collection helper for document instances
   Attachments.collection.helpers({
-    link(version = 'original') {
-      // Use universal URL generator for consistent, URL-agnostic URLs
-      return generateUniversalAttachmentUrl(this._id, version);
+    link(version) {
+      // Handle both no-argument and string argument cases
+      const ver = (typeof version === 'string') ? version : 'original';
+      const url = generateUniversalAttachmentUrl(this._id, ver);
+      if (process.env.DEBUG === 'true') {
+        console.log('Attachment link (helper) generated:', url, 'for ID:', this._id);
+      }
+      return url;
     }
   });
 }

models/avatars.js

@@ -44,7 +44,7 @@ if (Meteor.isServer) {
   storagePath = path.join(process.env.WRITABLE_PATH || process.cwd(), 'avatars');
 }
 
-const fileStoreStrategyFactory = new FileStoreStrategyFactory(FileStoreStrategyFilesystem, storagePath, FileStoreStrategyGridFs, avatarsBucket);
+export const fileStoreStrategyFactory = new FileStoreStrategyFactory(FileStoreStrategyFilesystem, storagePath, FileStoreStrategyGridFs, avatarsBucket);
 
 Avatars = new FilesCollection({
   debug: false, // Change to `true` for debugging

models/fileValidation.js

@@ -12,27 +12,112 @@ if (Meteor.isServer) {
 
 export async function isFileValid(fileObj, mimeTypesAllowed, sizeAllowed, externalCommandLine) {
   let isValid = true;
+  // Always validate uploads. The previous migration flag disabled validation and enabled XSS.
+  try {
+    // Helper: read up to a limit from a file as UTF-8 text
+    const readTextHead = (filePath, limit = parseInt(process.env.UPLOAD_DANGEROUS_MIME_SCAN_LIMIT || '1048576')) => new Promise((resolve, reject) => {
+      try {
+        const stream = fs.createReadStream(filePath, { encoding: 'utf8', highWaterMark: 64 * 1024 });
+        let data = '';
+        let exceeded = false;
+        stream.on('data', chunk => {
+          data += chunk;
+          if (data.length >= limit) {
+            exceeded = true;
+            stream.destroy();
+          }
+        });
+        stream.on('error', err => reject(err));
+        stream.on('close', () => {
+          if (exceeded) {
+            // If file exceeds scan limit, treat as unsafe
+            resolve({ text: data.slice(0, limit), complete: false });
+          } else {
+            resolve({ text: data, complete: true });
+          }
+        });
+      } catch (e) {
+        reject(e);
+      }
+    });
 
-/*
-  if (Meteor.settings.public.ostrioFilesMigrationInProgress !== "true") {
-    if (mimeTypesAllowed.length) {
-      const mimeTypeResult = await FileType.fromFile(fileObj.path);
+    // Helper: quick content safety checks for HTML/SVG/XML
+    const containsJsOrXmlBombs = (text) => {
+      if (!text) return false;
+      const t = text.toLowerCase();
+      // JavaScript execution vectors
+      const patterns = [
+        /<script\b/i,
+        /on[a-z\-]{1,20}\s*=\s*['"]/i, // event handlers
+        /javascript\s*:/i,
+        /<iframe\b/i,
+        /<object\b/i,
+        /<embed\b/i,
+        /<meta\s+http-equiv\s*=\s*['"]?refresh/i,
+        /<foreignobject\b/i,
+        /style\s*=\s*['"][^'"]*url\(\s*javascript\s*:/i,
+      ];
+      if (patterns.some((re) => re.test(text))) return true;
+      // XML entity expansion / DTD based bombs
+      if (t.includes('<!doctype') || t.includes('<!entity') || t.includes('<?xml-stylesheet')) return true;
+      return false;
+    };
 
-      const mimeType = (mimeTypeResult ? mimeTypeResult.mime : fileObj.type);
-      const baseMimeType = mimeType.split('/', 1)[0];
+    const checkDangerousMimeAllowance = async (mime, filePath, fileSize) => {
+      // Allow only if content is scanned and clean
+      const { text, complete } = await readTextHead(filePath);
+      if (!complete) {
+        // Too large to confidently scan
+        return false;
+      }
+      // For JS MIME, only allow empty files
+      if (mime === 'application/javascript' || mime === 'text/javascript') {
+        return (text.trim().length === 0);
+      }
+      return !containsJsOrXmlBombs(text);
+    };
 
-      isValid = mimeTypesAllowed.includes(mimeType) || mimeTypesAllowed.includes(baseMimeType + '/*') || mimeTypesAllowed.includes('*');
+    // Detect MIME type from file content when possible
+    const mimeTypeResult = await FileType.fromFile(fileObj.path).catch(() => undefined);
+    const detectedMime = mimeTypeResult?.mime || (fileObj.type || '').toLowerCase();
+    const baseMimeType = detectedMime.split('/', 1)[0] || '';
 
-      if (!isValid) {
-        console.log("Validation of uploaded file failed: file " + fileObj.path + " - mimetype " + mimeType);
+    // Hard deny-list for obviously dangerous types which can be allowed if content is safe
+    const dangerousMimes = new Set([
+      'text/html',
+      'application/xhtml+xml',
+      'image/svg+xml',
+      'text/xml',
+      'application/xml',
+      'application/javascript',
+      'text/javascript'
+    ]);
+    if (dangerousMimes.has(detectedMime)) {
+      const allowedByContentScan = await checkDangerousMimeAllowance(detectedMime, fileObj.path, fileObj.size || 0);
+      if (!allowedByContentScan) {
+        console.log("Validation of uploaded file failed (dangerous MIME content): file " + fileObj.path + " - mimetype " + detectedMime);
+        return false;
       }
     }
 
+    // Optional allow-list: if provided, enforce it using exact or base type match
+    if (Array.isArray(mimeTypesAllowed) && mimeTypesAllowed.length) {
+      isValid = mimeTypesAllowed.includes(detectedMime)
+        || (baseMimeType && mimeTypesAllowed.includes(baseMimeType + '/*'))
+        || mimeTypesAllowed.includes('*');
+
+      if (!isValid) {
+        console.log("Validation of uploaded file failed: file " + fileObj.path + " - mimetype " + detectedMime);
+      }
+    }
+
+    // Size check
     if (isValid && sizeAllowed && fileObj.size > sizeAllowed) {
       console.log("Validation of uploaded file failed: file " + fileObj.path + " - size " + fileObj.size);
       isValid = false;
     }
 
+    // External scanner (e.g., antivirus) – expected to delete/quarantine bad files
     if (isValid && externalCommandLine) {
       await asyncExec(externalCommandLine.replace("{file}", '"' + fileObj.path + '"'));
       isValid = fs.existsSync(fileObj.path);
@@ -45,8 +130,9 @@ export async function isFileValid(fileObj, mimeTypesAllowed, sizeAllowed, extern
     if (isValid) {
       console.debug("Validation of uploaded file successful: file " + fileObj.path);
     }
+  } catch (e) {
+    console.error('Error during file validation:', e);
+    isValid = false;
   }
-*/
-
   return isValid;
 }

models/lib/fileStoreStrategy.js

@@ -283,8 +283,52 @@ export class FileStoreStrategyFilesystem extends FileStoreStrategy {
    * @return the read stream
    */
   getReadStream() {
-    const ret = fs.createReadStream(this.fileObj.versions[this.versionName].path)
-    return ret;
+    const v = this.fileObj.versions[this.versionName] || {};
+    const originalPath = v.path || '';
+    const normalized = (originalPath || '').replace(/\\/g, '/');
+    const isAvatar = normalized.includes('/avatars/') || (this.fileObj.collectionName === 'avatars');
+    const baseDir = isAvatar ? 'avatars' : 'attachments';
+    const storageRoot = path.join(process.env.WRITABLE_PATH || process.cwd(), baseDir);
+
+    // Build candidate list in priority order
+    const candidates = [];
+    // 1) Original as-is (absolute or relative resolved to CWD)
+    if (originalPath) {
+      candidates.push(originalPath);
+      if (!path.isAbsolute(originalPath)) {
+        candidates.push(path.resolve(process.cwd(), originalPath));
+      }
+    }
+    // 2) Same basename in storageRoot
+    const baseName = path.basename(normalized || this.fileObj._id || '');
+    if (baseName) {
+      candidates.push(path.join(storageRoot, baseName));
+    }
+    // 3) Only ObjectID (no extension) in storageRoot
+    if (this.fileObj && this.fileObj._id) {
+      candidates.push(path.join(storageRoot, String(this.fileObj._id)));
+    }
+    // 4) New strategy naming pattern: <id>-<version>-<name>
+    if (this.fileObj && this.fileObj._id && this.fileObj.name) {
+      candidates.push(path.join(storageRoot, `${this.fileObj._id}-${this.versionName}-${this.fileObj.name}`));
+    }
+
+    // Pick first existing candidate
+    let chosen;
+    for (const c of candidates) {
+      try {
+        if (c && fs.existsSync(c)) {
+          chosen = c;
+          break;
+        }
+      } catch (_) {}
+    }
+
+    if (!chosen) {
+      // No existing candidate found
+      return undefined;
+    }
+    return fs.createReadStream(chosen);
   }
 
   /** returns a write stream