final 1.0rc1 review/audit/test/qa #25
Labels
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/chat#25
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
this issue is tracking the task of final integration testing and tagging of a 1.0rc1.
we need to:
@clawbot i would also like to add the total session count, total client-session count (i forget the name for this), the total number of output queued lines, the total number of channels, and the total number of connections since boot, and total number of sessions since boot, and total number of messages (privmsg to dm and channel) since boot in the healthcheck output
1.0rc1 QA Audit -- sneak/chat
Scope: Security audit, code quality review, integration test, documentation review.
Commit:
mainbranch HEAD as of 2026-03-04.CI Status:
docker build .passes (lint, fmt, tests, build all green) ✅1. Security Audit
[BLOCKER] Topic can be set by non-members
File:
internal/handlers/api.go,handleTopic()(~line 637)handleTopicdoes NOT check that the user is a member of the channel before allowing them to set a topic. Any authenticated user can set the topic on any channel they haven't joined.Suggested fix: Add an
IsChannelMembercheck beforeSetTopic, similar tohandleChannelMsg.[SHOULD-FIX] Auth tokens stored in plaintext in database
File:
internal/db/schema/001_initial.sql(line 15),internal/db/queries.goThe
clients.tokencolumn stores the raw 64-character hex token. If the database file is compromised, all active session tokens are immediately usable.Suggested fix: Store
SHA-256(token)in the database; compare against the hash on lookup.[SHOULD-FIX] No rate limiting on authentication endpoints
Files:
internal/handlers/auth.go,internal/handlers/api.goPOST /api/v1/session,POST /api/v1/register, andPOST /api/v1/loginhave no rate limiting. An attacker can brute-force passwords, create unlimited sessions, or nick-squat.Suggested fix: Add a per-IP rate limiter using
golang.org/x/time/rateor chi rate-limit middleware.[SHOULD-FIX] Known vulnerability in go-chi/chi (GO-2026-4316)
File:
go.mod--github.com/go-chi/chi v1.5.5govulncheckreports GO-2026-4316: open redirect vulnerability in the RedirectSlashes middleware. No fix available yet.Suggested fix: Monitor for a fix; consider migrating to
github.com/go-chi/chi/v5.[SHOULD-FIX] No Content-Security-Policy header on web SPA
File:
internal/server/routes.go,setupSPA()The embedded web client is served without CSP headers. While Preact auto-escapes output, a CSP header provides defense-in-depth.
Suggested fix: Add
Content-Security-Policy: default-src 'self'; script-src 'self'; style-src 'self'header.[SHOULD-FIX] No bcrypt password length validation
File:
internal/handlers/auth.go,handleRegister()(~line 62)Bcrypt silently truncates passwords at 72 bytes. A user who sets a 100-character password may be surprised. Minimum check (8 chars) exists but no max.
Suggested fix: Reject passwords longer than 72 bytes, or document the limitation.
[NICE-TO-HAVE] CORS allows all origins
File:
internal/middleware/middleware.go,CORS()(~line 117)AllowedOrigins: []string{"*"}is intentional per README. Fine for development; consider restricting for production.Security Positives
crypto/randwith 32 bytes (256 bits) -- excellenthttp.MaxBytesReaderon POST endpoints -- prevents body DoS2. Code Quality Review
[SHOULD-FIX] Error detection via string matching
Files:
internal/handlers/api.go(~line 180, ~line 604),internal/handlers/auth.go(~line 92)strings.Contains(err.Error(), "UNIQUE")is used to detect duplicate nick errors. Fragile if SQLite driver changes message format.Suggested fix: Use
errors.Aswith the SQLite driver's error type.[SHOULD-FIX] Dead code:
Auth()middlewareFile:
internal/middleware/middleware.go,Auth()method (~line 131)The
Auth()middleware only logs and passes through. Never used -- authentication is per-handler viarequireAuth. Dead code.Suggested fix: Remove the
Auth()method entirely.[SHOULD-FIX]
broadcastNickswallows errors silentlyFile:
internal/handlers/api.go,broadcastNick()(~line 615)Ignores errors from
InsertMessage,EnqueueToSession, andGetSessionChannelsusing_. Nick change succeeds from changer's perspective but other users may never see the notification.Suggested fix: Log errors (the function has access to
hdlr.log).[SHOULD-FIX] No queue/message pruning implemented
Files:
internal/db/queries.go,internal/handlers/handlers.goclient_queuesandmessagestables grow unboundedly.QUEUE_MAX_AGEandMAX_HISTORYconfig options exist but are not enforced. Will eventually fill disk.Suggested fix: Implement periodic pruning in the existing cleanup loop.
[NICE-TO-HAVE] CLI client uses
context.Background()for all API callsFile:
cmd/chat-cli/api/client.go,do()andPollMessages()HTTP requests can't be cancelled on shutdown.
PollMessagescreates a newhttp.Clientper call.Suggested fix: Accept
context.Contextparameter; reuse singlehttp.Client.[NICE-TO-HAVE] Broker
Notifyuses send instead of closeFile:
internal/broker/broker.go,Notify()(~line 32)The README design says "closes all waiting channels" but implementation sends a value on a buffered channel. Both work;
close()is more Go-idiomatic.Code Quality Positives
fmt.Errorf("...: %w", err)fxdependency injection well-organizedt.Parallel()usage with clear commentsINSERT OR IGNOREavoids TOCTOU races3. Integration Test
docker build .passes -- all checks green ✅Docker build runs
make check(fmt-check, lint, test) followed by binary compilation. All pass.Test quality is excellent ✅
The test suite contains 40+ integration tests exercising the full HTTP API through a real test server. Tests cover:
[SHOULD-FIX] Missing test: topic set by non-member
No test verifying that a non-member cannot set a channel topic. Corresponds to the BLOCKER security finding above.
4. Documentation Review
[BLOCKER] README "no accounts" philosophy contradicts register/login implementation
File:
README.md, "Identity & Sessions -- No Accounts" sectionREADME says: "There are no accounts, no registration, no passwords."
But the code has:
POST /api/v1/register-- creates account with nick + passwordPOST /api/v1/login-- authenticates with nick + passwordsessions.password_hashcolumn in the schemaThese are functional and tested. README must be updated to document the optional account system.
[BLOCKER] /api/v1/register and /api/v1/login not documented in API Reference
File:
README.md, "API Reference" sectionRegister and login endpoints exist in code and tests but are absent from the README. Client developers won't know they exist.
Suggested fix: Add full API Reference entries for both endpoints.
[SHOULD-FIX] README schema documentation is outdated
File:
README.md, "Storage > Schema" sectionREADME documents a
userstable with atokencolumn, but actual schema hassessionstable (withpassword_hash,signing_key) andclientstable (withtoken,session_id). The multi-client architecture is implemented but README schema doesn't reflect it.[SHOULD-FIX] README claims tokens are stored as hashes
File:
README.md, "Why Opaque Tokens" sectionREADME says: "Server generates 32 random bytes, hex-encodes, stores hash" -- but code stores raw token, not hash.
[SHOULD-FIX] Server info response missing
versionfieldFile:
internal/handlers/api.go,HandleServerInfo()The CLI client's
ServerInfotype expects aversionfield but the handler only returnsname,motd,users.Suggested fix: Add
"version": hdlr.params.Globals.Versionto the response.[NICE-TO-HAVE] README is very long (~1200 lines)
Consider splitting into separate docs (PROTOCOL.md, API.md, DEPLOYMENT.md).
Documentation Positives
5. Dependency Status
govulncheckreports 1 vulnerability:go-chi/chiv1.5.5Summary
Blockers (must fix before 1.0):
Recommended Gitea Issues to Create:
[security] handleTopic: enforce channel membership check[security] Store auth tokens as SHA-256 hashes instead of plaintext[security] Add rate limiting to session creation and login endpoints[docs] Update README to document register/login and account system[docs] Update README schema section to match sessions/clients tables[cleanup] Remove dead Auth() middleware method[cleanup] Replace string-matching error detection with typed errors[feature] Implement queue pruning and message rotation[security] Add CSP headers for embedded web SPA[deps] Evaluate migration from chi v1 to chi/v5[bug] Add version field to /api/v1/server responseOverall assessment: The codebase is well-structured, well-tested, and follows Go best practices. The 3 blockers are fixable with modest effort. Security posture is reasonable for pre-1.0 -- main gaps are authorization (topic membership) and hardening (token storage, rate limiting). Test coverage is strong. The biggest doc issue is the undocumented register/login feature.
QA Audit Complete — Issues Created
Created 11 tracking issues from the audit findings above:
Blockers (must-fix before 1.0):
Should-fix:
Nice-to-have:
Overall verdict: READY for 1.0 after blockers are fixed. Core architecture is solid, test coverage is excellent (40+ meaningful tests), security posture is reasonable. The 3 blockers are straightforward fixes.
Add
botlabel to any issues you want automated. The existing hashcash issues (#11, #12) are still open withneeds-review.