Fix SECURITY ISSUE 5: Attachment API uses bearer value as userId and DoS (Low).

Thanks to Siam Thanat Hack (STH) and xet7 !

SECURITY.md

@@ -208,6 +208,21 @@ Meteor.startup(() => {
   - Only the caller's own userId is added/removed from the selected estimation bucket (e.g., one, two, five, etc.).
 - Methods cover setting/unsetting poker question/end, casting votes, replaying, and setting final estimation.
 
+## Attachment API: authentication and DoS prevention
+
+- The attachment API (`/api/attachment/*`) requires proper authentication using `X-User-Id` and `X-Auth-Token` headers.
+- Authentication validates tokens by hashing with `Accounts._hashLoginToken` and matching against stored login tokens, preventing identity spoofing.
+- Request handlers implement:
+  - 30-second timeout to prevent hanging connections.
+  - Request body size limits (50MB for uploads, 10MB for metadata operations).
+  - Proper error handling and guaranteed response completion.
+  - Request error event handlers to clean up failed connections.
+- This prevents:
+  - DoS attacks via concurrent unauthenticated or malformed requests.
+  - Identity spoofing by using arbitrary bearer tokens or user IDs.
+  - Resource exhaustion from hanging connections or excessive payloads.
+- Access control: all attachment operations verify board membership before allowing access.
+
 ## Brute force login protection
 
 - https://github.com/wekan/wekan/commit/23e5e1e3bd081699ce39ce5887db7e612616014d

server/lib/tests/attachmentApi.tests.js

@@ -0,0 +1,203 @@
+/* eslint-env mocha */
+import { expect } from 'chai';
+import sinon from 'sinon';
+import { Meteor } from 'meteor/meteor';
+import { Accounts } from 'meteor/accounts-base';
+
+describe('attachmentApi authentication', function() {
+  let findOneStub, hashStub;
+
+  beforeEach(function() {
+    hashStub = sinon.stub(Accounts, '_hashLoginToken');
+    findOneStub = sinon.stub(Meteor.users, 'findOne');
+  });
+
+  afterEach(function() {
+    if (hashStub) hashStub.restore();
+    if (findOneStub) findOneStub.restore();
+  });
+
+  // Mock request/response objects
+  function createMockReq(headers = {}) {
+    return {
+      headers,
+      on: sinon.stub(),
+      connection: { destroy: sinon.stub() },
+    };
+  }
+
+  function createMockRes() {
+    return {
+      writeHead: sinon.stub(),
+      end: sinon.stub(),
+      headersSent: false,
+    };
+  }
+
+  describe('authenticateApiRequest', function() {
+    it('denies request with missing X-User-Id header', function() {
+      const req = createMockReq({ 'x-auth-token': 'sometoken' });
+      const res = createMockRes();
+
+      // Simulate the handler behavior
+      let errorThrown = false;
+      try {
+        if (!req.headers['x-user-id'] || !req.headers['x-auth-token']) {
+          throw new Meteor.Error('unauthorized', 'Missing X-User-Id or X-Auth-Token headers');
+        }
+      } catch (error) {
+        errorThrown = true;
+        expect(error.error).to.equal('unauthorized');
+      }
+
+      expect(errorThrown).to.equal(true);
+    });
+
+    it('denies request with missing X-Auth-Token header', function() {
+      const req = createMockReq({ 'x-user-id': 'user123' });
+
+      let errorThrown = false;
+      try {
+        if (!req.headers['x-user-id'] || !req.headers['x-auth-token']) {
+          throw new Meteor.Error('unauthorized', 'Missing X-User-Id or X-Auth-Token headers');
+        }
+      } catch (error) {
+        errorThrown = true;
+        expect(error.error).to.equal('unauthorized');
+      }
+
+      expect(errorThrown).to.equal(true);
+    });
+
+    it('denies request with invalid token', function() {
+      const userId = 'user123';
+      const token = 'invalidtoken';
+      const req = createMockReq({ 'x-user-id': userId, 'x-auth-token': token });
+
+      hashStub.returns('hashedInvalidToken');
+      findOneStub.returns(null); // No user found
+
+      let errorThrown = false;
+      try {
+        const hashedToken = Accounts._hashLoginToken(token);
+        const user = Meteor.users.findOne({
+          _id: userId,
+          'services.resume.loginTokens.hashedToken': hashedToken,
+        });
+        if (!user) {
+          throw new Meteor.Error('unauthorized', 'Invalid credentials');
+        }
+      } catch (error) {
+        errorThrown = true;
+        expect(error.error).to.equal('unauthorized');
+      }
+
+      expect(errorThrown).to.equal(true);
+      expect(hashStub.calledOnce).to.equal(true);
+      expect(findOneStub.calledOnce).to.equal(true);
+    });
+
+    it('allows request with valid X-User-Id and X-Auth-Token', function() {
+      const userId = 'user123';
+      const token = 'validtoken';
+      const req = createMockReq({ 'x-user-id': userId, 'x-auth-token': token });
+
+      const hashedToken = 'hashedValidToken';
+      hashStub.returns(hashedToken);
+      findOneStub.returns({ _id: userId }); // User found
+
+      let authenticatedUserId = null;
+      try {
+        const hashed = Accounts._hashLoginToken(token);
+        const user = Meteor.users.findOne({
+          _id: userId,
+          'services.resume.loginTokens.hashedToken': hashed,
+        });
+        if (!user) {
+          throw new Meteor.Error('unauthorized', 'Invalid credentials');
+        }
+        authenticatedUserId = userId;
+      } catch (error) {
+        // Should not throw
+      }
+
+      expect(authenticatedUserId).to.equal(userId);
+      expect(hashStub.calledOnce).to.equal(true);
+      expect(hashStub.calledWith(token)).to.equal(true);
+      expect(findOneStub.calledOnce).to.equal(true);
+      const queryArg = findOneStub.getCall(0).args[0];
+      expect(queryArg._id).to.equal(userId);
+      expect(queryArg['services.resume.loginTokens.hashedToken']).to.equal(hashedToken);
+    });
+
+    it('prevents identity spoofing by validating hashed token', function() {
+      const victimId = 'victim-user-id';
+      const attackerToken = 'attacker-token';
+      const req = createMockReq({ 'x-user-id': victimId, 'x-auth-token': attackerToken });
+
+      hashStub.returns('hashedAttackerToken');
+      // Simulate victim exists but token doesn't match
+      findOneStub.returns(null);
+
+      let errorThrown = false;
+      try {
+        const hashed = Accounts._hashLoginToken(attackerToken);
+        const user = Meteor.users.findOne({
+          _id: victimId,
+          'services.resume.loginTokens.hashedToken': hashed,
+        });
+        if (!user) {
+          throw new Meteor.Error('unauthorized', 'Invalid credentials');
+        }
+      } catch (error) {
+        errorThrown = true;
+        expect(error.error).to.equal('unauthorized');
+      }
+
+      expect(errorThrown).to.equal(true);
+    });
+  });
+
+  describe('request handler DoS prevention', function() {
+    it('enforces timeout on hanging requests', function(done) {
+      this.timeout(5000);
+      
+      const req = createMockReq({ 'x-user-id': 'user1', 'x-auth-token': 'token1' });
+      const res = createMockRes();
+
+      // Simulate timeout behavior
+      const timeout = setTimeout(() => {
+        if (!res.headersSent) {
+          res.headersSent = true;
+          res.writeHead(408, { 'Content-Type': 'application/json' });
+          res.end(JSON.stringify({ success: false, error: 'Request timeout' }));
+        }
+      }, 100); // Short timeout for test
+
+      // Wait for timeout
+      setTimeout(() => {
+        expect(res.headersSent).to.equal(true);
+        expect(res.writeHead.calledWith(408)).to.equal(true);
+        clearTimeout(timeout);
+        done();
+      }, 150);
+    });
+
+    it('limits request body size', function() {
+      const req = createMockReq({ 'x-user-id': 'user1', 'x-auth-token': 'token1' });
+      let body = '';
+      const limit = 50 * 1024 * 1024; // 50MB
+
+      // Simulate exceeding limit
+      body = 'a'.repeat(limit + 1);
+      expect(body.length).to.be.greaterThan(limit);
+
+      // Handler should destroy connection
+      if (body.length > limit) {
+        req.connection.destroy();
+      }
+
+      expect(req.connection.destroy.calledOnce).to.equal(true);
+    });
+  });
+});

server/lib/tests/index.js

@@ -3,3 +3,4 @@ import './users.security.tests';
 import './boards.security.tests';
 import './cards.security.tests';
 import './cards.methods.tests';
+import './attachmentApi.tests';

server/routes/attachmentApi.js

@@ -1,4 +1,5 @@
 import { Meteor } from 'meteor/meteor';
+import { Accounts } from 'meteor/accounts-base';
 import { WebApp } from 'meteor/webapp';
 import { ReactiveCache } from '/imports/reactiveCache';
 import { Attachments, fileStoreStrategyFactory } from '/models/attachments';
@@ -11,20 +12,24 @@ import { ObjectID } from 'bson';
 
 // Attachment API HTTP routes
 if (Meteor.isServer) {
-  // Helper function to authenticate API requests
+  // Helper function to authenticate API requests using X-User-Id and X-Auth-Token
   function authenticateApiRequest(req) {
-    const authHeader = req.headers.authorization;
-    if (!authHeader || !authHeader.startsWith('Bearer ')) {
-      throw new Meteor.Error('unauthorized', 'Missing or invalid authorization header');
+    const userId = req.headers['x-user-id'];
+    const authToken = req.headers['x-auth-token'];
+
+    if (!userId || !authToken) {
+      throw new Meteor.Error('unauthorized', 'Missing X-User-Id or X-Auth-Token headers');
     }
 
-    const token = authHeader.substring(7);
-    // Here you would validate the token and get the user ID
-    // For now, we'll use a simple approach - in production, you'd want proper JWT validation
-    const userId = token; // This should be replaced with proper token validation
-    
-    if (!userId) {
-      throw new Meteor.Error('unauthorized', 'Invalid token');
+    // Hash the token and validate against stored login tokens
+    const hashedToken = Accounts._hashLoginToken(authToken);
+    const user = Meteor.users.findOne({
+      _id: userId,
+      'services.resume.loginTokens.hashedToken': hashedToken,
+    });
+
+    if (!user) {
+      throw new Meteor.Error('unauthorized', 'Invalid credentials');
     }
 
     return userId;
@@ -47,15 +52,33 @@ if (Meteor.isServer) {
       return next();
     }
 
+    // Set timeout to prevent hanging connections
+    const timeout = setTimeout(() => {
+      if (!res.headersSent) {
+        sendErrorResponse(res, 408, 'Request timeout');
+      }
+    }, 30000); // 30 second timeout
+
     try {
       const userId = authenticateApiRequest(req);
       
       let body = '';
+      let bodyComplete = false;
+      
       req.on('data', chunk => {
         body += chunk.toString();
+        // Prevent excessive payload
+        if (body.length > 50 * 1024 * 1024) { // 50MB limit
+          req.connection.destroy();
+          clearTimeout(timeout);
+        }
       });
 
       req.on('end', () => {
+        if (bodyComplete) return; // Already processed
+        bodyComplete = true;
+        clearTimeout(timeout);
+        
         try {
           const data = JSON.parse(body);
           const { boardId, swimlaneId, listId, cardId, fileData, fileName, fileType, storageBackend } = data;
@@ -154,7 +177,16 @@ if (Meteor.isServer) {
           sendErrorResponse(res, 500, error.message);
         }
       });
+      
+      req.on('error', (error) => {
+        clearTimeout(timeout);
+        if (!res.headersSent) {
+          console.error('Request error:', error);
+          sendErrorResponse(res, 400, 'Request error');
+        }
+      });
     } catch (error) {
+      clearTimeout(timeout);
       sendErrorResponse(res, 401, error.message);
     }
   });
@@ -287,15 +319,31 @@ if (Meteor.isServer) {
       return next();
     }
 
+    const timeout = setTimeout(() => {
+      if (!res.headersSent) {
+        sendErrorResponse(res, 408, 'Request timeout');
+      }
+    }, 30000);
+
     try {
       const userId = authenticateApiRequest(req);
       
       let body = '';
+      let bodyComplete = false;
+      
       req.on('data', chunk => {
         body += chunk.toString();
+        if (body.length > 10 * 1024 * 1024) { // 10MB limit for metadata
+          req.connection.destroy();
+          clearTimeout(timeout);
+        }
       });
 
       req.on('end', () => {
+        if (bodyComplete) return;
+        bodyComplete = true;
+        clearTimeout(timeout);
+        
         try {
           const data = JSON.parse(body);
           const { attachmentId, targetBoardId, targetSwimlaneId, targetListId, targetCardId } = data;
@@ -388,7 +436,16 @@ if (Meteor.isServer) {
           sendErrorResponse(res, 500, error.message);
         }
       });
+      
+      req.on('error', (error) => {
+        clearTimeout(timeout);
+        if (!res.headersSent) {
+          console.error('Request error:', error);
+          sendErrorResponse(res, 400, 'Request error');
+        }
+      });
     } catch (error) {
+      clearTimeout(timeout);
       sendErrorResponse(res, 401, error.message);
     }
   });
@@ -399,15 +456,31 @@ if (Meteor.isServer) {
       return next();
     }
 
+    const timeout = setTimeout(() => {
+      if (!res.headersSent) {
+        sendErrorResponse(res, 408, 'Request timeout');
+      }
+    }, 30000);
+
     try {
       const userId = authenticateApiRequest(req);
       
       let body = '';
+      let bodyComplete = false;
+      
       req.on('data', chunk => {
         body += chunk.toString();
+        if (body.length > 10 * 1024 * 1024) {
+          req.connection.destroy();
+          clearTimeout(timeout);
+        }
       });
 
       req.on('end', () => {
+        if (bodyComplete) return;
+        bodyComplete = true;
+        clearTimeout(timeout);
+        
         try {
           const data = JSON.parse(body);
           const { attachmentId, targetBoardId, targetSwimlaneId, targetListId, targetCardId } = data;
@@ -461,7 +534,16 @@ if (Meteor.isServer) {
           sendErrorResponse(res, 500, error.message);
         }
       });
+      
+      req.on('error', (error) => {
+        clearTimeout(timeout);
+        if (!res.headersSent) {
+          console.error('Request error:', error);
+          sendErrorResponse(res, 400, 'Request error');
+        }
+      });
     } catch (error) {
+      clearTimeout(timeout);
       sendErrorResponse(res, 401, error.message);
     }
   });