diff --git a/changelog.d/1838.fixed.md b/changelog.d/1838.fixed.md new file mode 100644 index 000000000..44d6f5d6e --- /dev/null +++ b/changelog.d/1838.fixed.md @@ -0,0 +1 @@ +A Project role name is now matched against the rank table's own keys instead of the prototype chain: `toString`, `__proto__` and `constructor` answer `unknown-role` and leave the record untouched, where they used to demote a Maintainer to Member and report success ([#1838](https://github.com/ThinkFlowLab/FlyBot/issues/1838)). diff --git a/core/src/host/project-board.js b/core/src/host/project-board.js index fba1967d3..c562fa5da 100644 --- a/core/src/host/project-board.js +++ b/core/src/host/project-board.js @@ -72,7 +72,9 @@ export function changeRole(project, { actorId, targetId, role } = {}) { const target = self ? text(actorId) : text(targetId) const wanted = text(role) const current = rolesOf(project) - if (PROJECT_ROLE_RANKS[wanted] === undefined) return { ok: false, reason: 'unknown-role' } + // #1838: a plain object answers for names it inherits, so `toString` used to read as a rank and + // fall through to the demote path. Only an own key of the table is a role. + if (!Object.hasOwn(PROJECT_ROLE_RANKS, wanted)) return { ok: false, reason: 'unknown-role' } if (wanted === 'owner') return { ok: false, reason: 'owner-not-assignable' } if (actor === null) return { ok: false, reason: 'not-owner' } if (self) { diff --git a/core/test/project-roles.test.js b/core/test/project-roles.test.js index 470d682ec..44eb2b1bf 100644 --- a/core/test/project-roles.test.js +++ b/core/test/project-roles.test.js @@ -45,3 +45,27 @@ test('the last owner cannot be demoted, by themselves or by anyone else (#1828)' assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'owner', role: 'owner' }), { ok: false, reason: 'owner-not-assignable' }) assert.deepEqual(rolesOf(p), { owner: ['owner'], maintainers: [], members: [] }) }) + +// #1838: `PROJECT_ROLE_RANKS` is a plain object, so a name inherited from Object.prototype used to +// read as a defined rank, skip every refusal, and land on the demote path. A role name is the rank +// table's own key or it is not a role. +test('#1838: role "toString" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: 'toString' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +}) + +test('#1838: role "__proto__" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: '__proto__' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +}) + +test('#1838: role "constructor" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: 'constructor' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +}) diff --git a/patches/ds-bot/files/src/host/project-board.js b/patches/ds-bot/files/src/host/project-board.js index fba1967d3..c562fa5da 100644 --- a/patches/ds-bot/files/src/host/project-board.js +++ b/patches/ds-bot/files/src/host/project-board.js @@ -72,7 +72,9 @@ export function changeRole(project, { actorId, targetId, role } = {}) { const target = self ? text(actorId) : text(targetId) const wanted = text(role) const current = rolesOf(project) - if (PROJECT_ROLE_RANKS[wanted] === undefined) return { ok: false, reason: 'unknown-role' } + // #1838: a plain object answers for names it inherits, so `toString` used to read as a rank and + // fall through to the demote path. Only an own key of the table is a role. + if (!Object.hasOwn(PROJECT_ROLE_RANKS, wanted)) return { ok: false, reason: 'unknown-role' } if (wanted === 'owner') return { ok: false, reason: 'owner-not-assignable' } if (actor === null) return { ok: false, reason: 'not-owner' } if (self) { diff --git a/patches/ds-bot/files/test/project-roles.test.js b/patches/ds-bot/files/test/project-roles.test.js index 470d682ec..44eb2b1bf 100644 --- a/patches/ds-bot/files/test/project-roles.test.js +++ b/patches/ds-bot/files/test/project-roles.test.js @@ -45,3 +45,27 @@ test('the last owner cannot be demoted, by themselves or by anyone else (#1828)' assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'owner', role: 'owner' }), { ok: false, reason: 'owner-not-assignable' }) assert.deepEqual(rolesOf(p), { owner: ['owner'], maintainers: [], members: [] }) }) + +// #1838: `PROJECT_ROLE_RANKS` is a plain object, so a name inherited from Object.prototype used to +// read as a defined rank, skip every refusal, and land on the demote path. A role name is the rank +// table's own key or it is not a role. +test('#1838: role "toString" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: 'toString' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +}) + +test('#1838: role "__proto__" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: '__proto__' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +}) + +test('#1838: role "constructor" is refused and a Maintainer keeps that rank', () => { + const p = project({ roles: { maintainers: ['m'], members: ['n'] } }) + const before = rolesOf(p) + assert.deepEqual(changeRole(p, { actorId: 'owner', targetId: 'm', role: 'constructor' }), { ok: false, reason: 'unknown-role' }) + assert.deepEqual(rolesOf(p), before) +})