Browse Source

Fix SECURITY ISSUE 2: Access to boards of any Orgs/Teams, and avatar permissions.

Thanks to Siam Thanat Hack (STH) !
Lauri Ojansivu 1 day ago
parent
commit
f26d582018

+ 10 - 0
SECURITY.md

@@ -182,6 +182,16 @@ Meteor.startup(() => {
 - This means attachments are downloaded instead of rendered inline by default. This mitigates HTML/JS/SVG based stored XSS vectors.
 - Avatars and inline images remain supported but SVG uploads are blocked and never rendered inline.
 
+## Users: Client update restrictions
+
+- Client-side updates to user documents are limited to safe fields only:
+  - `username`
+  - `profile.*`
+- Sensitive fields are blocked from any client updates and can only be modified by server methods with authorization:
+  - `orgs`, `teams`, `roles`, `isAdmin`, `createdThroughApi`, `loginDisabled`, `authenticationMethod`, `services.*`, `emails.*`, `sessionData.*`
+- Attempts to update forbidden fields from the client are denied.
+- Admin operations like managing org/team membership or toggling flags must use server methods that check permissions.
+
 ## Brute force login protection
 
 - https://github.com/wekan/wekan/commit/23e5e1e3bd081699ce39ce5887db7e612616014d

+ 47 - 0
client/00-startup.js

@@ -15,3 +15,50 @@ import '/client/components/migrationProgress';
 
 // Import cron settings
 import '/client/components/settings/cronSettings';
+
+// Mirror Meteor login token into a cookie for server-side file route auth
+// This enables cookie-based auth for /cdn/storage/* without leaking ROOT_URL
+// Token already lives in localStorage; cookie adds same-origin send-on-request semantics
+Meteor.startup(() => {
+  const COOKIE_NAME = 'meteor_login_token';
+  const cookieAttrs = () => {
+    const attrs = ['Path=/', 'SameSite=Lax'];
+    try {
+      if (window.location && window.location.protocol === 'https:') {
+        attrs.push('Secure');
+      }
+    } catch (_) {}
+    return attrs.join('; ');
+  };
+
+  const setCookie = (name, value) => {
+    if (!value) return;
+    document.cookie = `${encodeURIComponent(name)}=${encodeURIComponent(value)}; ${cookieAttrs()}`;
+  };
+  const clearCookie = (name) => {
+    document.cookie = `${encodeURIComponent(name)}=; Expires=Thu, 01 Jan 1970 00:00:00 GMT; ${cookieAttrs()}`;
+  };
+
+  const syncCookie = () => {
+    try {
+      const token = Accounts && typeof Accounts._storedLoginToken === 'function' ? Accounts._storedLoginToken() : null;
+      if (token) setCookie(COOKIE_NAME, token); else clearCookie(COOKIE_NAME);
+    } catch (e) {
+      // ignore
+    }
+  };
+
+  // Initial sync on startup
+  syncCookie();
+
+  // Keep cookie in sync on login/logout
+  if (Accounts && typeof Accounts.onLogin === 'function') Accounts.onLogin(syncCookie);
+  if (Accounts && typeof Accounts.onLogout === 'function') Accounts.onLogout(syncCookie);
+
+  // Sync across tabs/windows when localStorage changes
+  window.addEventListener('storage', (ev) => {
+    if (ev && typeof ev.key === 'string' && ev.key.indexOf('Meteor.loginToken') !== -1) {
+      syncCookie();
+    }
+  });
+});

+ 1 - 1
client/components/users/userAvatar.jade

@@ -1,7 +1,7 @@
 template(name="userAvatar")
   a.member(class="js-{{#if assignee}}assignee{{else}}member{{/if}}" title="{{userData.profile.fullname}} ({{userData.username}}) {{_ memberType}}")
     if userData.profile.avatarUrl
-      img.avatar.avatar-image(src="{{userData.profile.avatarUrl}}")
+      img.avatar.avatar-image(src="{{avatarUrl}}")
     else
       +userAvatarInitials(userId=userData._id)
 

+ 15 - 0
client/components/users/userAvatar.js

@@ -15,6 +15,21 @@ Template.userAvatar.helpers({
     });
   },
 
+  avatarUrl() {
+    const user = ReactiveCache.getUser(this.userId, { fields: { profile: 1 } });
+    const base = (user && user.profile && user.profile.avatarUrl) || '';
+    if (!base) return '';
+    // Append current boardId when available so public viewers can access avatars on public boards
+    try {
+      const boardId = Utils.getCurrentBoardId && Utils.getCurrentBoardId();
+      if (boardId) {
+        const sep = base.includes('?') ? '&' : '?';
+        return `${base}${sep}boardId=${encodeURIComponent(boardId)}`;
+      }
+    } catch (_) {}
+    return base;
+  },
+
   memberType() {
     const user = ReactiveCache.getUser(this.userId);
     return user && user.isBoardAdmin() ? 'admin' : 'normal';

+ 37 - 11
models/users.js

@@ -569,15 +569,41 @@ Users.attachSchema(
   }),
 );
 
+// Security helpers for user updates
+export const USER_UPDATE_ALLOWED_EXACT = ['username'];
+export const USER_UPDATE_ALLOWED_PREFIXES = ['profile.'];
+export const USER_UPDATE_FORBIDDEN_PREFIXES = [
+  'services',
+  'emails',
+  'roles',
+  'isAdmin',
+  'createdThroughApi',
+  'orgs',
+  'teams',
+  'loginDisabled',
+  'authenticationMethod',
+  'sessionData',
+];
+
+export function isUserUpdateAllowed(fields) {
+  return fields.every((f) =>
+    USER_UPDATE_ALLOWED_EXACT.includes(f) || USER_UPDATE_ALLOWED_PREFIXES.some((p) => f.startsWith(p))
+  );
+}
+
+export function hasForbiddenUserUpdateField(fields) {
+  return fields.some((f) => USER_UPDATE_FORBIDDEN_PREFIXES.some((p) => f === p || f.startsWith(p + '.')));
+}
+
 Users.allow({
-  update(userId, doc) {
-    const user = ReactiveCache.getUser(userId) || ReactiveCache.getCurrentUser();
-    if (user?.isAdmin)
-      return true;
-    if (!user) {
-      return false;
-    }
-    return doc._id === userId;
+  update(userId, doc, fields /*, modifier */) {
+    // Only the owner can update, and only for allowed fields
+    if (!userId || doc._id !== userId) return false;
+    if (!Array.isArray(fields) || fields.length === 0) return false;
+    // Disallow if any forbidden field present
+    if (hasForbiddenUserUpdateField(fields)) return false;
+    // Allow only username and profile.*
+    return isUserUpdateAllowed(fields);
   },
   remove(userId, doc) {
     // Disable direct client-side user removal for security
@@ -588,10 +614,10 @@ Users.allow({
   fetch: [],
 });
 
-// Non-Admin users can not change to Admin
+// Deny any attempts to touch forbidden fields from client updates
 Users.deny({
-  update(userId, board, fieldNames) {
-    return _.contains(fieldNames, 'isAdmin') && !ReactiveCache.getCurrentUser().isAdmin;
+  update(userId, doc, fields /*, modifier */) {
+    return hasForbiddenUserUpdateField(fields);
   },
   fetch: [],
 });

+ 7 - 14
server/cors.js

@@ -1,18 +1,11 @@
 Meteor.startup(() => {
-  // Set Permissions-Policy header to suppress browser warnings about experimental features
-  WebApp.rawConnectHandlers.use(function(req, res, next) {
-    // Disable experimental advertising and privacy features that cause browser warnings
-    res.setHeader('Permissions-Policy', 
-      'browsing-topics=(), ' +
-      'run-ad-auction=(), ' +
-      'join-ad-interest-group=(), ' +
-      'private-state-token-redemption=(), ' +
-      'private-state-token-issuance=(), ' +
-      'private-aggregation=(), ' +
-      'attribution-reporting=()'
-    );
-    return next();
-  });
+  // Optional: Set Permissions-Policy only if explicitly provided to avoid browser warnings about unrecognized features
+  if (process.env.PERMISSIONS_POLICY && process.env.PERMISSIONS_POLICY.trim() !== '') {
+    WebApp.rawConnectHandlers.use(function(req, res, next) {
+      res.setHeader('Permissions-Policy', process.env.PERMISSIONS_POLICY);
+      return next();
+    });
+  }
 
   if (process.env.CORS) {
     // Listen to incoming HTTP requests, can only be used on the server

+ 1 - 0
server/lib/tests/index.js

@@ -1 +1,2 @@
 import './utils.tests';
+import './users.security.tests';

+ 43 - 0
server/lib/tests/users.security.tests.js

@@ -0,0 +1,43 @@
+/* eslint-env mocha */
+import { expect } from 'chai';
+import { isUserUpdateAllowed, hasForbiddenUserUpdateField } from '/models/users';
+
+describe('users security', function() {
+  describe('isUserUpdateAllowed', function() {
+    it('allows username update', function() {
+      expect(isUserUpdateAllowed(['username'])).to.equal(true);
+    });
+    it('allows profile updates', function() {
+      expect(isUserUpdateAllowed(['profile.fullname'])).to.equal(true);
+      expect(isUserUpdateAllowed(['profile.avatarUrl', 'profile.language'])).to.equal(true);
+    });
+    it('denies other top-level fields', function() {
+      expect(isUserUpdateAllowed(['orgs'])).to.equal(false);
+      expect(isUserUpdateAllowed(['teams'])).to.equal(false);
+      expect(isUserUpdateAllowed(['loginDisabled'])).to.equal(false);
+      expect(isUserUpdateAllowed(['authenticationMethod'])).to.equal(false);
+      expect(isUserUpdateAllowed(['services'])).to.equal(false);
+      expect(isUserUpdateAllowed(['emails'])).to.equal(false);
+      expect(isUserUpdateAllowed(['isAdmin'])).to.equal(false);
+    });
+  });
+
+  describe('hasForbiddenUserUpdateField', function() {
+    it('flags forbidden sensitive fields', function() {
+      expect(hasForbiddenUserUpdateField(['orgs'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['teams'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['loginDisabled'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['authenticationMethod'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['services.facebook'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['emails.0.verified'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['roles'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['isAdmin'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['createdThroughApi'])).to.equal(true);
+      expect(hasForbiddenUserUpdateField(['sessionData.totalHits'])).to.equal(true);
+    });
+    it('does not flag allowed fields', function() {
+      expect(hasForbiddenUserUpdateField(['username'])).to.equal(false);
+      expect(hasForbiddenUserUpdateField(['profile.fullname'])).to.equal(false);
+    });
+  });
+});

+ 186 - 23
server/routes/universalFileServer.js

@@ -7,8 +7,10 @@
 import { Meteor } from 'meteor/meteor';
 import { WebApp } from 'meteor/webapp';
 import { ReactiveCache } from '/imports/reactiveCache';
+import { Accounts } from 'meteor/accounts-base';
 import Attachments, { fileStoreStrategyFactory as attachmentStoreFactory } from '/models/attachments';
 import Avatars, { fileStoreStrategyFactory as avatarStoreFactory } from '/models/avatars';
+import '/models/boards';
 import { getAttachmentWithBackwardCompatibility, getOldAttachmentStream } from '/models/lib/attachmentBackwardCompatibility';
 import fs from 'fs';
 import path from 'path';
@@ -162,6 +164,154 @@ if (Meteor.isServer) {
     return false;
   }
 
+  /**
+   * Determine if an avatar request is authorized
+   * Rules:
+   *  - If a boardId query is provided and that board is public -> allow
+   *  - Else if requester is authenticated (valid token) -> allow
+   *  - Else if avatar's owner belongs to at least one public board -> allow
+   *  - Otherwise -> deny
+   */
+  function isAuthorizedForAvatar(req, avatar) {
+    try {
+      if (!avatar) return false;
+
+      // 1) Check explicit board context via query
+      const q = parseQuery(req);
+      const boardId = q.boardId || q.board || q.b;
+      if (boardId) {
+        const board = ReactiveCache.getBoard(boardId);
+        if (board && board.isPublic && board.isPublic()) return true;
+
+        // If private board is specified, require membership of requester
+        const token = extractLoginToken(req);
+        const user = token ? getUserFromToken(token) : null;
+        if (user && board && board.hasMember && board.hasMember(user._id)) return true;
+        return false;
+      }
+
+      // 2) Authenticated request without explicit board context
+      const token = extractLoginToken(req);
+      const user = token ? getUserFromToken(token) : null;
+      if (user) return true;
+
+      // 3) Allow if avatar owner is on any public board (so avatars are public only when on public boards)
+      // Use a lightweight query against Boards
+      const found = Boards && Boards.findOne({ permission: 'public', 'members.userId': avatar.userId }, { fields: { _id: 1 } });
+      return !!found;
+    } catch (e) {
+      if (process.env.DEBUG === 'true') {
+        console.warn('Avatar authorization check failed:', e);
+      }
+      return false;
+    }
+  }
+
+  /**
+   * Parse cookies from request headers into an object map
+   */
+  function parseCookies(req) {
+    const header = req.headers && req.headers.cookie;
+    const out = {};
+    if (!header) return out;
+    const parts = header.split(';');
+    for (const part of parts) {
+      const idx = part.indexOf('=');
+      if (idx === -1) continue;
+      const k = decodeURIComponent(part.slice(0, idx).trim());
+      const v = decodeURIComponent(part.slice(idx + 1).trim());
+      out[k] = v;
+    }
+    return out;
+  }
+
+  /**
+   * Get query parameters as a simple object
+   */
+  function parseQuery(req) {
+    const out = {};
+    const q = (req.url || '').split('?')[1] || '';
+    if (!q) return out;
+    const pairs = q.split('&');
+    for (const p of pairs) {
+      if (!p) continue;
+      const [rawK, rawV] = p.split('=');
+      const k = decodeURIComponent((rawK || '').trim());
+      const v = decodeURIComponent((rawV || '').trim());
+      if (k) out[k] = v;
+    }
+    return out;
+  }
+
+  /**
+   * Extract a login token from Authorization header, query param, or cookie
+   * Supported sources (priority order):
+   * - Authorization: Bearer <token>
+   * - X-Auth-Token header
+   * - authToken query parameter
+   * - meteor_login_token or wekan_login_token cookie
+   */
+  function extractLoginToken(req) {
+    // Authorization: Bearer <token>
+    const authz = req.headers && (req.headers.authorization || req.headers.Authorization);
+    if (authz && typeof authz === 'string') {
+      const m = authz.match(/^Bearer\s+(.+)$/i);
+      if (m && m[1]) return m[1].trim();
+    }
+
+    // X-Auth-Token
+    const xAuth = req.headers && (req.headers['x-auth-token'] || req.headers['X-Auth-Token']);
+    if (xAuth && typeof xAuth === 'string') return xAuth.trim();
+
+    // Query parameter
+    const q = parseQuery(req);
+    if (q.authToken && typeof q.authToken === 'string') return q.authToken.trim();
+
+    // Cookies
+    const cookies = parseCookies(req);
+    if (cookies.meteor_login_token) return cookies.meteor_login_token.trim();
+    if (cookies.wekan_login_token) return cookies.wekan_login_token.trim();
+
+    return null;
+  }
+
+  /**
+   * Resolve a user from a raw login token string
+   */
+  function getUserFromToken(rawToken) {
+    try {
+      if (!rawToken || typeof rawToken !== 'string' || rawToken.length < 10) return null;
+      const hashed = Accounts._hashLoginToken(rawToken);
+      return Meteor.users.findOne({ 'services.resume.loginTokens.hashedToken': hashed }, { fields: { _id: 1 } });
+    } catch (e) {
+      // In case accounts-base is not available or any error occurs
+      if (process.env.DEBUG === 'true') {
+        console.warn('Token resolution error:', e);
+      }
+      return null;
+    }
+  }
+
+  /**
+   * Authorization helper for board-bound files
+   * - Public boards: allow
+   * - Private boards: require valid user who is a member
+   */
+  function isAuthorizedForBoard(req, board) {
+    try {
+      if (!board) return false;
+      if (board.isPublic && board.isPublic()) return true;
+      const token = extractLoginToken(req);
+      const user = token ? getUserFromToken(token) : null;
+      return !!(user && board.hasMember && board.hasMember(user._id));
+    } catch (e) {
+      if (process.env.DEBUG === 'true') {
+        console.warn('Authorization check failed:', e);
+      }
+      return false;
+    }
+  }
+
   /**
    * Helper function to stream file with error handling
    */
@@ -205,8 +355,8 @@ if (Meteor.isServer) {
         return;
       }
 
-      // Get attachment from database
-      const attachment = ReactiveCache.getAttachment(fileId);
+      // Get attachment from database with backward compatibility
+      const attachment = getAttachmentWithBackwardCompatibility(fileId);
       if (!attachment) {
         res.writeHead(404);
         res.end('Attachment not found');
@@ -221,24 +371,28 @@ if (Meteor.isServer) {
         return;
       }
 
-      // TODO: Implement proper authentication via cookies/headers
-      // Meteor.userId() returns undefined in WebApp.connectHandlers middleware
-      // For now, allow access - ostrio:files protected() method provides fallback auth
-      // const userId = null; // Need to extract from req.headers.cookie
-      // if (!board.isPublic() && (!userId || !board.hasMember(userId))) {
-      //   res.writeHead(403);
-      //   res.end('Access denied');
-      //   return;
-      // }
+      // Enforce cookie/header/query-based auth for private boards
+      if (!isAuthorizedForBoard(req, board)) {
+        res.writeHead(403);
+        res.end('Access denied');
+        return;
+      }
 
       // Handle conditional requests
       if (handleConditionalRequest(req, res, attachment)) {
         return;
       }
 
-      // Get file strategy and stream
-  const strategy = attachmentStoreFactory.getFileStrategy(attachment, 'original');
-      const readStream = strategy.getReadStream();
+      // Choose proper streaming based on source
+      let readStream;
+      if (attachment?.meta?.source === 'legacy') {
+        // Legacy CollectionFS GridFS stream
+        readStream = getOldAttachmentStream(fileId);
+      } else {
+        // New Meteor-Files storage
+        const strategy = attachmentStoreFactory.getFileStrategy(attachment, 'original');
+        readStream = strategy.getReadStream();
+      }
 
       if (!readStream) {
         res.writeHead(404);
@@ -296,9 +450,12 @@ if (Meteor.isServer) {
         return;
       }
 
-      // TODO: Implement proper authentication for avatars
-      // Meteor.userId() returns undefined in WebApp.connectHandlers middleware
-      // For now, allow avatar viewing - they're typically public anyway
+      // Enforce visibility: avatars are public only in the context of public boards
+      if (!isAuthorizedForAvatar(req, avatar)) {
+        res.writeHead(403);
+        res.end('Access denied');
+        return;
+      }
 
       // Handle conditional requests
       if (handleConditionalRequest(req, res, avatar)) {
@@ -366,9 +523,12 @@ if (Meteor.isServer) {
         return;
       }
 
-      // TODO: Implement proper authentication via cookies/headers
-      // Meteor.userId() returns undefined in WebApp.connectHandlers middleware
-      // For now, allow access for compatibility
+      // Enforce cookie/header/query-based auth for private boards
+      if (!isAuthorizedForBoard(req, board)) {
+        res.writeHead(403);
+        res.end('Access denied');
+        return;
+      }
 
       // Handle conditional requests
       if (handleConditionalRequest(req, res, attachment)) {
@@ -435,9 +595,12 @@ if (Meteor.isServer) {
         return;
       }
 
-      // TODO: Implement proper authentication for legacy avatars
-      // Meteor.userId() returns undefined in WebApp.connectHandlers middleware
-      // For now, allow avatar viewing for compatibility
+      // Enforce visibility for legacy avatars as well
+      if (!isAuthorizedForAvatar(req, avatar)) {
+        res.writeHead(403);
+        res.end('Access denied');
+        return;
+      }
 
       // Handle conditional requests
       if (handleConditionalRequest(req, res, avatar)) {