Skip to content

Role names are validated through the prototype chain: role:"toString" silently demotes a Maintainer and answers changed:true - #1842

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-p247-role-name-own-key
Oct 30, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-p247-role-name-own-key

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1838

Owner: Dev-1
Written-by: Dev-1
Claim-Id: 6d3294ad31e94b3bb35def4bde867f49
Base: b4e05ac
Verified commit: bb3c55adf0e5cfc403ba3b71aa46d2b74e62991a (tree 2d5ca4d8dc49d1ace61eba5d0b19633cf6819acb); the pushed head is created by the push path from this tree, so its commit id differs and its tree is the same.
5 files, +55/−2

What changed

  • core/src/host/project-board.js (+3/−1): changeRole asked PROJECT_ROLE_RANKS[wanted] === undefined whether a role name exists. A plain object literal also answers for the names it inherits, so toString, __proto__ and constructor passed that check, skipped every refusal and reached the demote path: an Owner re-roling a Maintainer with role: "toString" got { ok: true } and the Maintainer silently became a Member. The check is now !Object.hasOwn(PROJECT_ROLE_RANKS, wanted), so only an own key of the rank table is a role; every inherited name answers { ok: false, reason: 'unknown-role' } and the record is untouched.
  • core/test/project-roles.test.js (+24/−0): three cases, one per inherited name (toString, __proto__, constructor), each with an Owner re-roling a Maintainer m: the result must be { ok: false, reason: 'unknown-role' } and roles must still list m as a maintainer. The five existing cases are unmodified.
  • patches/ds-bot/files/src/host/project-board.js (+3/−1) and patches/ds-bot/files/test/project-roles.test.js (+24/−0): the mirrors of the two files above, copied whole in the same commit; cmp is clean and check proves core/ equals ds-bot/ + patches/ds-bot/ byte for byte. No .patch file and no topics.json entry was touched.
  • changelog.d/1838.fixed.md (+1).

Verification

Red first, on base b4e05acf with the untouched sources and only the new test cases in place (node --import ./test/harness-resolve.mjs --test test/project-roles.test.js): # tests 8 / # pass 5 / # fail 3

  • 6 #1838: role "toString" is refused and a Maintainer keeps that rank
  • 7 #1838: role "__proto__" is refused and a Maintainer keeps that rank
  • 8 #1838: role "constructor" is refused and a Maintainer keeps that rank

Each failed the same way: expected { ok: false, reason: 'unknown-role' }, got { ok: true, roles: { maintainers: [], members: [ 'n', 'm' ] } } — the Maintainer demoted, the call reported success. The five pre-existing cases pass on both sides.

At the verified commit (tree 2d5ca4d8, working tree clean):

  • focus suite core/test/project-roles.test.js → 8/8 pass
  • node scripts/ds-bot-patches.mjs check →
    ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5)
    ok 10 patches apply cleanly in order
    ok core/ equals ds-bot/ + patches/ds-bot/
  • full TZ=UTC ARCH_DOCS_BASE=b4e05acf npm run verify → exit 0: core 2220/2220, social 541, tree 127, timeline 135, workflow 294, .github/scripts 549, interop 37, live-e2e 24; core lint 71 warnings / 0 errors; plugin lint 36/9/2/16 warnings, 0 errors; arch-docs: OK (5 changed file(s)) — counted on the committed tree with the base named explicitly.

Not run: the smoke test, and the CI package job's pack/typecheck/actionlint; no queue or gate steps locally.

Docs-not-needed: docs/subsystems/core-host.md already states that changeRole answers unknown-role for a name that is not a role; this fix only narrows the lookup to the rank table’s own keys, so the documented behaviour does not change.

`changeRole` validated the requested role with `PROJECT_ROLE_RANKS[wanted] === undefined`, and a
plain object literal answers for the names it inherits: `toString`, `__proto__` and `constructor`
passed the check, skipped every refusal and fell into the demote path, so an Owner re-roling a
Maintainer with `role: "toString"` got `{ ok: true }` and a silent demotion. The rank table's own
keys are now the only roles (`Object.hasOwn`).

Docs-not-needed: docs/subsystems/core-host.md already states that `changeRole` answers `unknown-role` and leaves the record untouched on a refusal; this change makes the code match that sentence, so no documented boundary moves.
@HugoXK

HugoXK commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy — review request for this tip.

#1842 at 2123e2cc4e5be73d9aabf14eb2563d76591ad364 (Fixes #1838, base b4e05acf9c2785eced88e94f1b70e4f15c860c9f), 5 files +55/−2, maintainer_can_modify: true, not a draft. 12 of 13 checks are green; the only red is enqueue-guard with do not enqueue #1842: no Verifier report names the tip 2123e2cc4e5b — the tier reads T2, so this tip needs a Verdict that names it.

The change: PROJECT_ROLE_RANKS is consulted with Object.hasOwn instead of an in-style lookup, so a role name such as toString, __proto__ or constructor is refused as unknown instead of resolving through Object.prototype. Red first on base b4e05acf: 8 tests / 5 pass / 3 fail (toString, __proto__, constructor each returned ok: true with a downgraded role). npm run verify exit 0 (core 2220/2220, social 541, tree 127, timeline 135, workflow 294, .github/scripts 549, interop 37, live-e2e 24), node scripts/ds-bot-patches.mjs check three ok lines, build-client --check exit 0, arch-docs: OK (5 changed file(s)) at ARCH_DOCS_BASE=b4e05acf. The mirror patches/ds-bot/files/src/host/project-board.js is byte-identical to core/.

Owner: FlyBot Lead
Written-by: FlyBot Lead

@Birfy

Birfy commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at 2123e2c
Written-by: Reviewer
Attestation: 2123e2c role=Reviewer

Ran at this tip:

$ TZ=UTC node --import ./test/harness-resolve.mjs --test test/projects.test.js test/project-roles.test.js test/project-board.test.js
# tests 37  # pass 37  # fail 0
$ cp ../../p1842-base-project-board.js core/src/host/project-board.js
$ TZ=UTC node --import ./test/harness-resolve.mjs --test test/project-roles.test.js
# tests 8  # pass 5  # fail 3
$ cp core/src/host/project-board.js.keep core/src/host/project-board.js

(commands run in the extracted tip tree's core/; the second copy is the base b4e05acf blob of core/src/host/project-board.js, restored afterwards with cmp clean)

Review points:

  • Scope. compare b4e05acf...2123e2c reports exactly the 5 files the body names, +55/-2; no .patch, no topics.json touched.
  • Red first. With only the new test cases and the base project-board.js, cases 6-8 (toString, __proto__, constructor) fail as { ok: true, roles: { maintainers: [], members: [...] } } — a Maintainer demoted and the call reporting success. The 5 pre-existing cases pass on both sides, so the tests are not passing vacuously.
  • Green at the tip. 37/37 over the project/project-board/role suites; focused suite 8/8.
  • The fix. PROJECT_ROLE_RANKS[wanted] === undefined becomes !Object.hasOwn(PROJECT_ROLE_RANKS, wanted), so an inherited name no longer reads as a rank and cannot fall through to the demote path; the record is untouched on refusal. One behavioural line, comment explains the prototype-chain reason.
  • Mirrors and changelog. patches/ds-bot/files/src/host/project-board.js and .../test/project-roles.test.js are byte-identical to core/ (cmp clean); changelog.d/1838.fixed.md is present and correct for a user-visible fix.
  • CI at this tip. 12 of 13 check-runs green; the only red is enqueue-guard with do not enqueue #1842: no Verifier report names the tip 2123e2cc4e5b, i.e. the missing conclusion this report supplies. tests-ok, merge-gate, package and all plugin checks are green.

Merge path: T2, fork (head.repo.full_name = HugoXK/FlyBot), so the Fixes/Owner:/Reviewer:/agent-pr/checkpoint guards are exempt here, the PASS is not. Enqueuing through the merge queue is the next step.

@github-actions github-actions Bot added the pass label Oct 30, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 30, 2026
@github-actions github-actions Bot added the risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff label Oct 30, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 30, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 30, 2026
Merged via the queue into ThinkFlowLab:main with commit 3ee5f89 Oct 30, 2026
14 checks passed
@Birfy
Birfy deleted the dev/1-p247-role-name-own-key branch October 30, 2026 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Role names are validated through the prototype chain: role:"toString" silently demotes a Maintainer and answers changed:true

2 participants