spikerj/spikersoft-issues
1
0
Fork 0
Code Issues 196 Pull Requests Actions Packages Projects Releases Wiki Activity

[Security][Chat] CrossOrganization channels are accessible to EVERYONE — CanAccessChannel returns true unconditionally ('Simplified for now') #644

New Issue
Closed
opened 2026-07-17 07:09:34 +00:00 by spikerj · 2 comments
spikerj commented 2026-07-17 07:09:34 +00:00
Owner
Copy Link
Copy Source

Found via SonarQube S1172 triage (2026-07-17) — the rule flagged the unused userId parameter on MultiTenantChatHub.CanAccessChannel, and reading the method revealed the real issue:

case ChatChannelScope.CrossOrganization:
    // Must be member of one of the allowed organizations
    return true; // Simplified for now

Any authenticated user can join/read/post in ANY cross-organization channel (call sites: JoinChannel :226, channel listing :314, SendMessage :350). Global scope is intentionally open and Organization scope is properly checked against the caller's claims (ExtractOrganizationIds()), so the gap is specifically cross-org.

Blocking design fact: ChatChannel has NO allowed-organizations field — the comment's intended check has nothing to check against. Options:

  1. Add AllowedOrganizationIds to the model + creation/admin flow, and enforce membership-of-any (the comment's stated intent).
  2. Interim tightening: for CrossOrganization channels require the caller in MemberIds (exists today 'for private channels') — depends on whether cross-org membership is actually maintained there.
  3. Accept-and-document if cross-org channels are meant to be platform-public (then the scope name and comment should say so).

Needs a product call before code changes — flagging rather than fixing to avoid locking users out. Also: the vestigial userId parameter should be dropped or used when this is resolved (the claims-based caller context is the correct authz source, matching the Organization case).

**Found via SonarQube S1172 triage (2026-07-17)** — the rule flagged the unused `userId` parameter on `MultiTenantChatHub.CanAccessChannel`, and reading the method revealed the real issue: ``` case ChatChannelScope.CrossOrganization: // Must be member of one of the allowed organizations return true; // Simplified for now ``` Any authenticated user can join/read/post in ANY cross-organization channel (call sites: JoinChannel :226, channel listing :314, SendMessage :350). Global scope is intentionally open and Organization scope is properly checked against the caller's claims (`ExtractOrganizationIds()`), so the gap is specifically cross-org. **Blocking design fact:** `ChatChannel` has NO allowed-organizations field — the comment's intended check has nothing to check against. Options: 1. Add `AllowedOrganizationIds` to the model + creation/admin flow, and enforce membership-of-any (the comment's stated intent). 2. Interim tightening: for CrossOrganization channels require the caller in `MemberIds` (exists today 'for private channels') — depends on whether cross-org membership is actually maintained there. 3. Accept-and-document if cross-org channels are meant to be platform-public (then the scope name and comment should say so). Needs a product call before code changes — flagging rather than fixing to avoid locking users out. Also: the vestigial `userId` parameter should be dropped or used when this is resolved (the claims-based caller context is the correct authz source, matching the Organization case).
spikerj commented 2026-07-18 18:08:15 +00:00
Author
Owner
Copy Link
Copy Source

Triage verdict: legitimate security gap — fix up as backend PR #413 (option 1, the comment's stated intent).

Resolution of the three options:

  • Option 2 (require MemberIds) rejected for cause: membership is only recorded after CanAccessChannel passes (AddChannelMembership, JoinChannel L237) and the field is documented "for private channels" — requiring it pre-join is a lockout catch-22.
  • Option 3 (accept-as-public) rejected: contradicts both the scope name and the in-code comment.
  • Option 1 implemented: ChatChannel.AllowedOrganizationIds (+ CreateChannelRequest parity field), enforced membership-of-any via a new pure ChatChannelAccess.CanAccess policy class carrying all scope rules — directly unit-tested (7 new tests incl. the regression: empty allow-list denies). The vestigial userId param (the original S1172 flag) is dropped.
  • Fail-closed on empty allow-list is safe today: no production code path creates channels at all (verified — only tests instantiate ChatChannel), so nothing live gets locked out; any manually seeded cross-org channel closes until its allow-list is set, which is the correct secure default for an education platform.

Common.Tests 719 green, slnf clean. Will close when the PR merges.

**Triage verdict: legitimate security gap — fix up as backend PR [#413](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/413)** (option 1, the comment's stated intent). Resolution of the three options: - **Option 2 (require MemberIds) rejected for cause:** membership is only recorded *after* `CanAccessChannel` passes (`AddChannelMembership`, JoinChannel L237) and the field is documented "for private channels" — requiring it pre-join is a lockout catch-22. - **Option 3 (accept-as-public) rejected:** contradicts both the scope name and the in-code comment. - **Option 1 implemented:** `ChatChannel.AllowedOrganizationIds` (+ `CreateChannelRequest` parity field), enforced membership-of-any via a new pure `ChatChannelAccess.CanAccess` policy class carrying all scope rules — directly unit-tested (7 new tests incl. the regression: empty allow-list denies). The vestigial `userId` param (the original S1172 flag) is dropped. - **Fail-closed on empty allow-list** is safe today: no production code path creates channels at all (verified — only tests instantiate `ChatChannel`), so nothing live gets locked out; any manually seeded cross-org channel closes until its allow-list is set, which is the correct secure default for an education platform. Common.Tests 719 green, slnf clean. Will close when the PR merges.
spikerj commented 2026-07-18 18:17:19 +00:00
Author
Owner
Copy Link
Copy Source

Resolved in backend PR #413 (merged to master 2026-07-18). ChatChannel.AllowedOrganizationIds added and enforced fail-closed via the new unit-tested ChatChannelAccess policy; empty allow-list denies everyone. Closing.

Resolved in backend PR [#413](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/413) (merged to `master` 2026-07-18). `ChatChannel.AllowedOrganizationIds` added and enforced fail-closed via the new unit-tested `ChatChannelAccess` policy; empty allow-list denies everyone. Closing.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agentic
Actionable surfaced by the agentic architecture & security review
 bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 sonarqube
Code quality issue surfaced by SonarQube (bug / code smell)
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
spikerj (Joseph Spiker)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#644