From 5879cfafbf856afb864205d2353fba50a3b8b305 Mon Sep 17 00:00:00 2001 From: Aaron Vu Date: Thu, 17 Sep 2026 15:33:25 +0700 Subject: [PATCH 1/3] fix(hub): grant chat staging write by role, and take it back on demotion The three call sites that give a new member access to the hidden chat staging folder granted a fixed value of 4. That value meant write before the permission bits were renumbered and means download now, so a member whose role carries no write bit could never upload an attachment and got a bare 403 with nothing in the UI to explain it. Grant write instead, but only to a role that may chat. The grant was previously unconditional, so a view-only member already carried it; left unconditional it would have handed them an upload path once the value became meaningful. set_privilege also has to move the grant in both directions. It only ever granted, so demoting a member from chat to view left the staging access behind and the demoted member kept uploading. Revoke it there instead. --- service/private/hub.js | 52 +++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/service/private/hub.js b/service/private/hub.js index 5f1af3a..58dfa11 100644 --- a/service/private/hub.js +++ b/service/private/hub.js @@ -33,6 +33,7 @@ const { butlerFrom } = require("../lib/mail-sender"); const { mailFailure } = require("../lib/mail-result"); const { resolveHubInviteName } = require("../lib/hub-invite-name"); const { resolveHubDisplayName } = require("../lib/hub-display-name"); +const { CAN_CHAT, privilegeAllows } = require("../lib/member-capability"); const { MfsTools } = require("@drumee/server-core"); const { remove_dir } = MfsTools; const { toArray } = utils; @@ -1001,10 +1002,17 @@ class __private_hub extends Hub { await this.db.await_proc( "permission_grant", "*", uid, expiry, privilege, "system", message ); - await this.db.await_proc( - "permission_grant", mfs_home.chat_upload_id, uid, 0, 4, - "no_traversal", "chat upload permission" - ); + // Chat attachments stage in a hidden folder before they become a message. + // A member who may chat has to write there even though the role carries no + // write bit for the workspace at large -- 'no_traversal' keeps that raised + // access on this one folder. A view-only member may not chat, so granting + // it would hand them an upload path they are not entitled to. + if (privilegeAllows(privilege, CAN_CHAT)) { + await this.db.await_proc( + "permission_grant", mfs_home.chat_upload_id, uid, 0, Privilege.WRITE, + "no_traversal", "chat upload permission" + ); + } await writeAudit(this, { db: this.hub.get(Attr.db_name), uid: this.uid, @@ -2139,14 +2147,16 @@ class __private_hub extends Hub { '*', uid, expiry, privilege, 'system', msg ); - // Grant chat upload permission if chat folder exists - if (mfs_home && mfs_home.chat_upload_id) { + // Write access to the chat staging folder only, and only for a role + // that may chat -- see _grantMembership for why it is scoped this way. + if (mfs_home && mfs_home.chat_upload_id && + privilegeAllows(privilege, CAN_CHAT)) { await this.yp.await_proc( `${hub_db}.permission_grant`, mfs_home.chat_upload_id, uid, 0, // no expiry on chat upload - 4, // read+write for uploads + Privilege.WRITE, 'no_traversal', 'chat upload permission' ); @@ -2812,15 +2822,25 @@ class __private_hub extends Hub { for (let uid of users) { await this.db.await_proc("permission_set", uid, privilege); - await this.db.await_proc( - "permission_grant", - mfs_home.chat_upload_id, - uid, - 0, - 4, - "no_traversal", - "chat upload permission" - ); + // A role change has to move the chat staging grant in BOTH directions. + // Granting on the way up is what lets a chat member attach a file; + // revoking on the way down is what stops a member demoted to view-only + // from keeping an upload path the new role does not carry. + if (privilegeAllows(privilege, CAN_CHAT)) { + await this.db.await_proc( + "permission_grant", + mfs_home.chat_upload_id, + uid, + 0, + Privilege.WRITE, + "no_traversal", + "chat upload permission" + ); + } else { + await this.db.await_proc( + "permission_revoke", mfs_home.chat_upload_id, uid + ); + } hub = {}; hub.privilege = privilege; From 8d6f7bb8a1696e43e2df30c74735848ff1cbf2a7 Mon Sep 17 00:00:00 2001 From: Aaron Vu Date: Thu, 17 Sep 2026 15:49:21 +0700 Subject: [PATCH 2/3] fix(hub): pin the chat staging grant value instead of reading it from the package Privilege.WRITE resolves to 15 under server-essentials 1.3.1 and to 7 under 1.3.6, which republished the pre-1.3.0 bit layout. package.json asks for ^1.3.1, so a bare npm install picks up 1.3.6 and the grant would carry no write bit at all -- the exact 403 this change exists to fix, reintroduced by a dependency bump nobody would connect to chat. Pin the value alongside the other capability bits, which are spelled out for the same reason and say so in the file header. --- service/lib/member-capability.js | 19 +++++++++++++++++++ service/private/hub.js | 10 ++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/service/lib/member-capability.js b/service/lib/member-capability.js index d723165..087cd92 100644 --- a/service/lib/member-capability.js +++ b/service/lib/member-capability.js @@ -41,6 +41,24 @@ const CAN_CHAT = 0b0000100; const CAN_WRITE = 0b0001000; const CAN_ADMIN = 0b0010000; +/** + * What a member is granted on the hidden chat staging folder + * ('/__chat__/__upload__'), where an attachment is held before it becomes a + * message. read + download + write, plus the low bit every stored role mask + * carries. The grant is written with assign_via 'no_traversal', so it applies + * to that one folder and user_permission will not let it reach anything + * inside it. + * + * 🚨 Pinned here for the same reason every other value in this file is, and + * this one has already moved twice. `Privilege.WRITE` resolves to 15 under + * server-essentials 1.3.1 and to 7 under 1.3.6, which republished the + * pre-1.3.0 layout. package.json asks for ^1.3.1, so a bare `npm install` + * picks up 1.3.6 and a grant written from `Privilege.WRITE` would carry no + * write bit at all -- the exact 403 this value exists to fix, reintroduced + * by a dependency bump nobody connected to chat. + */ +const CHAT_UPLOAD_GRANT = 0b0001111; + /** * Does this stored privilege carry every bit of `bit`? * @@ -145,6 +163,7 @@ async function memberCan(service, bit) { module.exports = { CAN_READ, + CHAT_UPLOAD_GRANT, CAN_DOWNLOAD, CAN_CHAT, CAN_WRITE, diff --git a/service/private/hub.js b/service/private/hub.js index 58dfa11..24563ef 100644 --- a/service/private/hub.js +++ b/service/private/hub.js @@ -33,7 +33,9 @@ const { butlerFrom } = require("../lib/mail-sender"); const { mailFailure } = require("../lib/mail-result"); const { resolveHubInviteName } = require("../lib/hub-invite-name"); const { resolveHubDisplayName } = require("../lib/hub-display-name"); -const { CAN_CHAT, privilegeAllows } = require("../lib/member-capability"); +const { + CAN_CHAT, CHAT_UPLOAD_GRANT, privilegeAllows +} = require("../lib/member-capability"); const { MfsTools } = require("@drumee/server-core"); const { remove_dir } = MfsTools; const { toArray } = utils; @@ -1009,7 +1011,7 @@ class __private_hub extends Hub { // it would hand them an upload path they are not entitled to. if (privilegeAllows(privilege, CAN_CHAT)) { await this.db.await_proc( - "permission_grant", mfs_home.chat_upload_id, uid, 0, Privilege.WRITE, + "permission_grant", mfs_home.chat_upload_id, uid, 0, CHAT_UPLOAD_GRANT, "no_traversal", "chat upload permission" ); } @@ -2156,7 +2158,7 @@ class __private_hub extends Hub { mfs_home.chat_upload_id, uid, 0, // no expiry on chat upload - Privilege.WRITE, + CHAT_UPLOAD_GRANT, 'no_traversal', 'chat upload permission' ); @@ -2832,7 +2834,7 @@ class __private_hub extends Hub { mfs_home.chat_upload_id, uid, 0, - Privilege.WRITE, + CHAT_UPLOAD_GRANT, "no_traversal", "chat upload permission" ); From 2662f048235ca7ba044018cdd1efa404392c198a Mon Sep 17 00:00:00 2001 From: Aaron Vu Date: Thu, 17 Sep 2026 17:00:00 +0700 Subject: [PATCH 3/3] test(hub): lock who may write to the chat staging folder, and with what value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both properties the attachment fix rests on can be undone by an edit that reads like a simplification, and neither shows up as a test failure today. The gate must refuse a view-only member. The grant went out regardless of role before, so rows for view-only members already exist in the wild; loosening the check to a partial bit overlap would admit exactly the role it exists to exclude, because the chat constant in the package overlaps read. The granted value must carry the write bit and must not be read from the package. server-essentials 1.3.6 republished the pre-1.3.0 layout, where Privilege.WRITE is 7 and carries no write bit in this schema, and package.json asks for ^1.3.1 — so a dependency bump nobody connects to chat would silently restore the 403. Reads the call sites as text rather than loading them, and imports only member-capability, which requires nothing. That keeps it in the same install-free workflow as the other regressions here. Checked against three mutations: restoring the package value, dropping the gate from one call site, and removing the revoke on demotion. Each fails the suite. --- .github/workflows/test.yml | 6 ++ offline/test/chat-upload-grant.test.js | 139 +++++++++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 offline/test/chat-upload-grant.test.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4bcb341..477f3ac 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -47,3 +47,9 @@ jobs: # the over-limit clamp keep catching it. - name: Run workspace-delete permission regression test run: node --test offline/test/hub-delete-permission.test.js + # The other permission bar with a CI step of its own: it pins that a + # view-only member gets no write on the chat staging folder, and that the + # granted value keeps the write bit whichever server-essentials is + # installed. + - name: Run chat staging grant regression test + run: node --test offline/test/chat-upload-grant.test.js diff --git a/offline/test/chat-upload-grant.test.js b/offline/test/chat-upload-grant.test.js new file mode 100644 index 0000000..8571310 --- /dev/null +++ b/offline/test/chat-upload-grant.test.js @@ -0,0 +1,139 @@ +#!/usr/bin/env node + +// Who may write to the chat staging folder, and with what value. +// +// THE REPORT: a member whose workspace role is Chat could not attach a file in +// chat. The upload answered 403 and the chat showed nothing at all -- no +// attachment chip, no error -- while the same account attached files normally +// in a workspace where it happened to be an admin. +// +// An attachment stages in the hidden folder '/__chat__/__upload__' before it +// becomes a message. A chat member has no write bit for the workspace, which is +// intended, so the membership paths give write on that one folder through a +// grant written with assign_via 'no_traversal'. Two things were wrong: the +// value granted was 4, which meant write before the permission bits were +// renumbered and means download now, and the grant went out regardless of role, +// so view-only members held it too. +// +// These lock the two properties the fix rests on, both of which can be undone +// by an edit that looks like a simplification: +// +// 1. the gate admits a role that may chat and refuses one that may not; +// 2. the granted value carries the write bit, and does not come from the +// package, whose 1.3.6 release republished the pre-1.3.0 layout. +// +// Deliberately dependency-free: member-capability.js requires nothing, and the +// call sites are read as text, so this runs on a stock runner with no install. + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { readFileSync } = require("node:fs"); +const { join } = require("node:path"); + +const { + CAN_CHAT, + CAN_WRITE, + CAN_READ, + CHAT_UPLOAD_GRANT, + privilegeAllows, +} = require("../../service/lib/member-capability"); + +// The stored privilege for each role selector the UI offers. +const VIEW = 0b000011; +const CHAT = 0b000111; +const EDIT = 0b001111; +const ADMIN = 0b011111; +const OWNER = 0b111111; + +test("the gate refuses a view-only member", () => { + assert.equal(privilegeAllows(VIEW, CAN_CHAT), false, + "a member who may only read must not be given write on the chat staging " + + "folder; the grant used to go out regardless of role, so rows for " + + "view-only members already exist in the wild"); +}); + +test("the gate admits every role at or above chat", () => { + for (const [name, privilege] of [ + ["chat", CHAT], ["edit", EDIT], ["admin", ADMIN], ["owner", OWNER], + ]) { + assert.equal(privilegeAllows(privilege, CAN_CHAT), true, + `${name} may chat and must be able to attach a file`); + } +}); + +// The trap the gate exists to avoid. `Constants.permission.chat` is 0b0000110 -- +// read OR download -- so it OVERLAPS read, and a loose `privilege & chat` is +// truthy for a view-only member (3 & 6 = 2). privilegeAllows asks for every bit +// of the mask, so the overlap is not enough. +test("a multi-bit mask that overlaps read still refuses a view-only member", () => { + const READ_OR_DOWNLOAD = 0b0000110; + assert.notEqual(VIEW & READ_OR_DOWNLOAD, 0, + "the premise: the loose form would admit a view-only member here"); + assert.equal(privilegeAllows(VIEW, READ_OR_DOWNLOAD), false, + "privilegeAllows must require the whole mask, not a partial overlap"); +}); + +test("the granted value carries the write bit", () => { + assert.equal((CHAT_UPLOAD_GRANT & CAN_WRITE) === CAN_WRITE, true, + "without the write bit the upload ACL refuses and the member gets the " + + "same silent 403 this change exists to fix"); + assert.equal((CHAT_UPLOAD_GRANT & CAN_READ) === CAN_READ, true, + "the member has to read the staging folder as well as write to it"); +}); + +// server-essentials 1.3.6 republished the pre-1.3.0 layout: Privilege.WRITE +// resolves to 7 there and to 15 under the pinned 1.3.1. package.json asks for +// ^1.3.1, so reading the value from the package means a dependency bump nobody +// connects to chat silently removes the write bit again. +test("the granted value is not the value the republished package would give", () => { + assert.notEqual(CHAT_UPLOAD_GRANT, 0b0000111, + "0b0000111 is Privilege.WRITE under server-essentials 1.3.6 and carries " + + "no write bit in this schema"); +}); + +// The gate has to sit at the call site. A check one layer up is not the same +// invariant: three separate paths grant this row, and each has its own idea of +// where `privilege` comes from. +const HUB_SOURCE = readFileSync( + join(__dirname, "..", "..", "service", "private", "hub.js"), + "utf8" +); + +test("every chat staging grant in hub.js is gated at the call site", () => { + const lines = HUB_SOURCE.split("\n"); + const sites = []; + lines.forEach((line, i) => { + if (line.includes("chat_upload_id") && !line.trim().startsWith("*")) { + sites.push(i); + } + }); + assert.ok(sites.length >= 3, + `expected the three membership paths to grant this row, found ${sites.length}`); + + for (const i of sites) { + const window = lines.slice(Math.max(0, i - 12), i + 2).join("\n"); + const isGrant = window.includes("permission_grant"); + if (!isGrant) continue; + assert.ok( + window.includes("CAN_CHAT") && window.includes("privilegeAllows"), + `the permission_grant near line ${i + 1} writes the chat staging row ` + + "without a chat-bit check above it" + ); + assert.ok( + !/,\s*4\s*,/.test(window), + `the permission_grant near line ${i + 1} still passes a bare 4, the ` + + "value that means download since the bits were renumbered" + ); + } +}); + +test("a role change gives the row back and takes it away", () => { + const start = HUB_SOURCE.indexOf("async set_privilege()"); + assert.notEqual(start, -1, "set_privilege not found in hub.js"); + const body = HUB_SOURCE.slice(start, start + 2000); + assert.ok(body.includes("permission_grant"), + "promoting a member to a chat role has to give them the staging row"); + assert.ok(body.includes("permission_revoke"), + "demoting a member to view-only has to take the staging row back; " + + "set_privilege only ever granted, so a demoted member kept uploading"); +});