security: enforce channel membership check in handleTopic #75

clawbot · 2026-03-17T10:14:52+01:00

clawbot commented

2026-03-17 10:14:52 +01:00

Summary

handleTopic in internal/handlers/api.go did NOT check that the user was a member of the channel before allowing them to set a topic. Any authenticated user could set the topic on any channel they hadn't joined.

Changes

internal/handlers/api.go: Added IsChannelMember check after resolving the channel ID and before calling executeTopic, mirroring the existing pattern in handleChannelMsg. Non-members now receive ERR_NOTONCHANNEL (442).
internal/handlers/api_test.go: Added TestTopicNonMember — creates a channel with one user, then verifies a second user who hasn't joined receives numeric 442 when attempting to set the topic.

Testing

All existing tests pass
New TestTopicNonMember test validates the fix
docker build . passes clean (formatting, linting, tests, build)

closes #33

## Summary `handleTopic` in `internal/handlers/api.go` did NOT check that the user was a member of the channel before allowing them to set a topic. Any authenticated user could set the topic on any channel they hadn't joined. ## Changes - **`internal/handlers/api.go`**: Added `IsChannelMember` check after resolving the channel ID and before calling `executeTopic`, mirroring the existing pattern in `handleChannelMsg`. Non-members now receive `ERR_NOTONCHANNEL` (442). - **`internal/handlers/api_test.go`**: Added `TestTopicNonMember` — creates a channel with one user, then verifies a second user who hasn't joined receives numeric 442 when attempting to set the topic. ## Testing - All existing tests pass - New `TestTopicNonMember` test validates the fix - `docker build .` passes clean (formatting, linting, tests, build) closes #33

clawbot added 1 commit 2026-03-17 10:14:52 +01:00

security: enforce channel membership check in handleTopic

check / check (push) Successful in 1m14s

Details

53e0abb134

handleTopic did not verify that the requesting user was a member of the
channel before allowing them to set a topic. Any authenticated user could
set the topic on any channel they hadn't joined.

Add an IsChannelMember check after resolving the channel and before
calling executeTopic, mirroring the existing pattern in handleChannelMsg.
Non-members now receive ERR_NOTONCHANNEL (442).

Add TestTopicNonMember to verify the fix.

clawbot added the needs-review bot labels 2026-03-17 10:15:18 +01:00

clawbot commented

2026-03-17 10:28:28 +01:00

Review: PR #75 — security: enforce channel membership check in handleTopic

Policy Compliance Check

No policy violations found.

✅ Only internal/handlers/api.go and internal/handlers/api_test.go modified — no linter config, Makefile, CI config, or test infrastructure changes
✅ Blank line before every return statement (AGENTS.md / Go styleguide)
✅ No inline error handling — plain assignment used (isMember, err := ...)
✅ Error handling follows existing handleChannelMsg pattern exactly: IsChannelMember → log + 500 on DB error → IRC numeric error on non-member
✅ Uses irc.ErrNotOnChannel (442) which is the semantically correct IRC numeric for TOPIC on an unjoined channel (vs ErrCannotSendToChan used for PRIVMSG — both are correct per RFC 1459)
✅ No new exported types or functions introduced — no doc comment requirements triggered
✅ .golangci.yml untouched
✅ No hardcoded magic numbers
✅ No secrets, PII, or credentials in the diff

Requirements Checklist (from issue #33)

Requirement	Status
Add `IsChannelMember` check in `handleTopic` before `SetTopic`	✅ Met — check added after `GetChannelByName` and before `executeTopic`
Mirror pattern from `handleChannelMsg`	✅ Met — identical structure: `IsChannelMember` call → DB error handling with log + 500 → non-member IRC error response
Non-members receive appropriate error	✅ Met — returns `ERR_NOTONCHANNEL` (442), correct per RFC 1459 §4.2.4
Test coverage for the fix	✅ Met — `TestTopicNonMember` creates channel with alice, bob attempts TOPIC without joining, asserts 442

Build Result

✅ docker build . passes clean — fmt-check, lint, test, and build all succeed.

Additional Review Notes

Correctness: The membership check is placed at exactly the right point — after the channel is resolved (so we have chID) but before executeTopic is called. No path can bypass the check.
Edge cases: Error path from IsChannelMember DB failure correctly returns 500 with logged error, not an IRC numeric (matches existing pattern).
Test quality: TestTopicNonMember is well-structured — creates two separate sessions, only one joins, verifies the non-member gets 442. Drains initial messages before the test command to avoid false positives.
Scope: Minimal, surgical fix — exactly what was asked for, nothing more.
No cheating: No modified linter config, no weakened assertions, no suppressed rules.

Verdict: PASS

The fix is correct, minimal, policy-compliant, and well-tested. The membership check mirrors the established pattern from handleChannelMsg and uses the semantically appropriate IRC error code.

## Review: PR #75 — security: enforce channel membership check in handleTopic ### Policy Compliance Check **No policy violations found.** - ✅ Only `internal/handlers/api.go` and `internal/handlers/api_test.go` modified — no linter config, Makefile, CI config, or test infrastructure changes - ✅ Blank line before every `return` statement (AGENTS.md / Go styleguide) - ✅ No inline error handling — plain assignment used (`isMember, err := ...`) - ✅ Error handling follows existing `handleChannelMsg` pattern exactly: `IsChannelMember` → log + 500 on DB error → IRC numeric error on non-member - ✅ Uses `irc.ErrNotOnChannel` (442) which is the semantically correct IRC numeric for TOPIC on an unjoined channel (vs `ErrCannotSendToChan` used for PRIVMSG — both are correct per RFC 1459) - ✅ No new exported types or functions introduced — no doc comment requirements triggered - ✅ `.golangci.yml` untouched - ✅ No hardcoded magic numbers - ✅ No secrets, PII, or credentials in the diff ### Requirements Checklist (from [issue #33](https://git.eeqj.de/sneak/chat/issues/33)) | Requirement | Status | |---|---| | Add `IsChannelMember` check in `handleTopic` before `SetTopic` | ✅ Met — check added after `GetChannelByName` and before `executeTopic` | | Mirror pattern from `handleChannelMsg` | ✅ Met — identical structure: `IsChannelMember` call → DB error handling with log + 500 → non-member IRC error response | | Non-members receive appropriate error | ✅ Met — returns `ERR_NOTONCHANNEL` (442), correct per RFC 1459 §4.2.4 | | Test coverage for the fix | ✅ Met — `TestTopicNonMember` creates channel with alice, bob attempts TOPIC without joining, asserts 442 | ### Build Result ✅ `docker build .` passes clean — fmt-check, lint, test, and build all succeed. ### Additional Review Notes - **Correctness**: The membership check is placed at exactly the right point — after the channel is resolved (so we have `chID`) but before `executeTopic` is called. No path can bypass the check. - **Edge cases**: Error path from `IsChannelMember` DB failure correctly returns 500 with logged error, not an IRC numeric (matches existing pattern). - **Test quality**: `TestTopicNonMember` is well-structured — creates two separate sessions, only one joins, verifies the non-member gets 442. Drains initial messages before the test command to avoid false positives. - **Scope**: Minimal, surgical fix — exactly what was asked for, nothing more. - **No cheating**: No modified linter config, no weakened assertions, no suppressed rules. ### Verdict: **PASS** The fix is correct, minimal, policy-compliant, and well-tested. The membership check mirrors the established pattern from `handleChannelMsg` and uses the semantically appropriate IRC error code.

clawbot added merge-ready and removed needs-review labels 2026-03-17 10:28:55 +01:00

sneak was assigned by clawbot

2026-03-17 10:28:56 +01:00

sneak merged commit e36bd99ef6 into main

2026-03-17 12:47:00 +01:00

sneak deleted branch fix/topic-membership-check

2026-03-17 12:47:00 +01:00

sneak referenced this issue from a commit

2026-03-17 12:47:01 +01:00

security: enforce channel membership check in handleTopic (#75)

clawbot referenced this pull request

2026-03-17 12:55:32 +01:00