1015 lines
30 KiB
Markdown
1015 lines
30 KiB
Markdown
# dumpsterChat Security Remediation Plan (P0-P2)
|
|
|
|
> **For Hermes:** Use subagent-driven-development skill to implement this plan task-by-task.
|
|
|
|
**Goal:** Fix all P0, P1, and P2 security findings from the 2026-06-29 audit.
|
|
|
|
**Architecture:** Changes are confined to the Go backend (`internal/`, `cmd/`), with one frontend change (WS token transport). Each fix is independently deployable. The plan is ordered so that quick wins land first, and later tasks build on middleware infrastructure created by earlier ones.
|
|
|
|
**Tech Stack:** Go 1.24+, chi/v5, gorilla/websocket, golang.org/x/time/rate
|
|
|
|
**Project root:** `~/Projects/dumpsterChat/`
|
|
|
|
---
|
|
|
|
## Findings Summary
|
|
|
|
| # | Severity | Finding | Status |
|
|
|---|----------|---------|--------|
|
|
| 1 | P0 | WebSocket Origin Bypass | OPEN |
|
|
| 2 | P0 | Session Token in URL | OPEN |
|
|
| 3 | P0 | WebAuthn Cookie Missing Secure | OPEN |
|
|
| 4 | P0 | PostgreSQL sslmode=disable | OPEN |
|
|
| 5 | P1 | No Rate Limiting | OPEN |
|
|
| 6 | P1 | WS Broadcast to All Users (data leak) | OPEN |
|
|
| 7 | P1 | Webhook Tokens Stored Plaintext | OPEN |
|
|
| 8 | P1 | No Security Headers | OPEN |
|
|
| 9 | P2 | No CSRF Protection | OPEN |
|
|
| 10 | P2 | Channel CRUD Lacks Permission Checks | OPEN |
|
|
| 11 | P2 | Upload: No File Type/Size Validation | OPEN |
|
|
| 12 | P2 | File Serve Path Traversal | OPEN |
|
|
|
|
---
|
|
|
|
## Dependency Graph
|
|
|
|
```mermaid
|
|
graph TD
|
|
T1["#1 WS Origin Check"] --> T2["#2 WS Token Transport"]
|
|
T3["#3 WebAuthn Cookie Fix"]
|
|
T4["#4 DB sslmode"]
|
|
T5["#5 Rate Limiter Middleware"]
|
|
T6["#6 WS Broadcast Filtering"] --> T6a["#6a Hub server-membership map"]
|
|
T7["#7 Webhook Token Hashing"]
|
|
T8["#8 Security Headers Middleware"]
|
|
T9["#9 CSRF Middleware"]
|
|
T10["#10 Channel Permission Checks"]
|
|
T11["#11 Upload Validation"]
|
|
T12["#12 File Serve Path Sanitization"]
|
|
|
|
T5 --> T9
|
|
T8 --> T9
|
|
```
|
|
|
|
---
|
|
|
|
## Phase 1: Quick P0 Wins (< 15 min each)
|
|
|
|
### Task 1: WebSocket Origin Check
|
|
|
|
**Objective:** Reject WebSocket connections from untrusted origins.
|
|
|
|
**Files:**
|
|
- Modify: `internal/gateway/client.go:21-25`
|
|
|
|
**Current code (line 21-25):**
|
|
```go
|
|
var upgrader = websocket.Upgrader{
|
|
ReadBufferSize: 1024,
|
|
WriteBufferSize: 1024,
|
|
CheckOrigin: func(r *http.Request) bool { return true },
|
|
}
|
|
```
|
|
|
|
**Replace with:**
|
|
```go
|
|
// NewWSUpgrader creates a websocket.Upgrader with origin checking.
|
|
// allowedOrigins is the set of trusted origins (e.g. ["https://chat.example.com"]).
|
|
func NewWSUpgrader(allowedOrigins []string) websocket.Upgrader {
|
|
originSet := make(map[string]struct{}, len(allowedOrigins))
|
|
for _, o := range allowedOrigins {
|
|
originSet[o] = struct{}{}
|
|
}
|
|
return websocket.Upgrader{
|
|
ReadBufferSize: 1024,
|
|
WriteBufferSize: 1024,
|
|
CheckOrigin: func(r *http.Request) bool {
|
|
origin := r.Header.Get("Origin")
|
|
if origin == "" {
|
|
return false
|
|
}
|
|
_, ok := originSet[origin]
|
|
return ok
|
|
},
|
|
}
|
|
}
|
|
```
|
|
|
|
**Then in `cmd/server/main.go`:** Before `gateway.ServeWS` is called, construct the upgrader with the allowed origins and pass it through. The simplest approach is to make `ServeWS` accept the upgrader as a parameter, or set it as a package-level var:
|
|
|
|
```go
|
|
// In cmd/server/main.go, after config load:
|
|
origins := []string{"https://" + cfg.Host}
|
|
if cfg.Host == "localhost" {
|
|
origins = append(origins, "http://localhost:"+cfg.Port)
|
|
}
|
|
gateway.SetAllowedOrigins(origins)
|
|
```
|
|
|
|
Add to `internal/gateway/client.go`:
|
|
```go
|
|
// SetAllowedOrigins replaces the default upgrader with one that checks origins.
|
|
func SetAllowedOrigins(origins []string) {
|
|
upgrader = NewWSUpgrader(origins)
|
|
}
|
|
```
|
|
|
|
**Verify:** Build compiles. Manual test: connect from allowed origin succeeds, from `http://evil.com` gets 403.
|
|
|
|
**Commit:** `fix(security): enforce WebSocket origin checking`
|
|
|
|
---
|
|
|
|
### Task 2: WebAuthn Cookie Missing Secure Flag
|
|
|
|
**Objective:** Add `Secure: true` to the WebAuthn login session cookie.
|
|
|
|
**Files:**
|
|
- Modify: `internal/auth/webauthn.go:334-341`
|
|
|
|
**Current code (line 334-341):**
|
|
```go
|
|
http.SetCookie(w, &http.Cookie{
|
|
Name: "dumpster_session",
|
|
Value: token,
|
|
Path: "/",
|
|
HttpOnly: true,
|
|
MaxAge: 86400 * 30,
|
|
SameSite: http.SameSiteStrictMode,
|
|
})
|
|
```
|
|
|
|
**Replace with (reuse the existing helper):**
|
|
```go
|
|
h.setSessionCookie(w, token)
|
|
```
|
|
|
|
Wait, `WebAuthnHandler` doesn't have access to `setSessionCookie` since that's on the regular `Handler`. Two options:
|
|
|
|
**Option A (preferred):** Extract `setSessionCookie` into a shared function in the `auth` package:
|
|
|
|
Add to `internal/auth/handlers.go` (or a new `internal/auth/cookie.go`):
|
|
```go
|
|
// SetSessionCookie writes the session cookie with secure defaults.
|
|
func SetSessionCookie(w http.ResponseWriter, cookieName, token string, duration time.Duration) {
|
|
http.SetCookie(w, &http.Cookie{
|
|
Name: cookieName,
|
|
Value: token,
|
|
Path: "/",
|
|
HttpOnly: true,
|
|
Secure: true,
|
|
SameSite: http.SameSiteStrictMode,
|
|
MaxAge: int(duration.Seconds()),
|
|
})
|
|
}
|
|
```
|
|
|
|
Then update `Handler.setSessionCookie` to call it, and update `webauthn.go:334` to call it too:
|
|
```go
|
|
SetSessionCookie(w, "dumpster_session", token, 30*24*time.Hour)
|
|
```
|
|
|
|
**Option B (quick):** Just add `Secure: true` to the existing cookie block.
|
|
|
|
Go with Option A for DRY.
|
|
|
|
**Verify:** Build compiles. Login via WebAuthn sets cookie with `Secure` flag.
|
|
|
|
**Commit:** `fix(security): add Secure flag to WebAuthn session cookie`
|
|
|
|
---
|
|
|
|
### Task 3: PostgreSQL sslmode Configurable
|
|
|
|
**Objective:** Allow `sslmode` to be configured via env var, default to `require`.
|
|
|
|
**Files:**
|
|
- Modify: `internal/config/config.go:94-96`
|
|
|
|
**Current code:**
|
|
```go
|
|
func (c *Config) DatabaseDSN() string {
|
|
return "postgres://" + c.Database.User + ":***" + "@" + c.Database.Host + ":" + c.Database.Port + "/" + c.Database.Database + "?sslmode=disable"
|
|
}
|
|
```
|
|
|
|
**Step 1:** Add `SSLMode` field to `DatabaseConfig`:
|
|
```go
|
|
type DatabaseConfig struct {
|
|
Host string
|
|
Port string
|
|
User string
|
|
Password string
|
|
Database string
|
|
SSLMode string
|
|
}
|
|
```
|
|
|
|
**Step 2:** In `Load()`, populate it:
|
|
```go
|
|
Database: DatabaseConfig{
|
|
...
|
|
SSLMode: getEnv("POSTGRES_SSLMODE", "require"),
|
|
},
|
|
```
|
|
|
|
**Step 3:** Fix `DatabaseDSN()` to include the real password (it currently masks it with `***`) and use the configured sslmode:
|
|
```go
|
|
func (c *Config) DatabaseDSN() string {
|
|
return "postgres://" + c.Database.User + ":" + c.Database.Password + "@" + c.Database.Host + ":" + c.Database.Port + "/" + c.Database.Database + "?sslmode=" + c.Database.SSLMode
|
|
}
|
|
```
|
|
|
|
Note: The current DSN uses `***` instead of the actual password. That's a separate bug (the DSN used for `sql.Open` must have the real password). Verify that `db.New(cfg)` calls `cfg.DatabaseDSN()` and it works.
|
|
|
|
**Step 4:** Update `docker/compose.yml` to add `POSTGRES_SSLMODE: disable` for local dev:
|
|
```yaml
|
|
POSTGRES_SSLMODE: ${POSTGRES_SSLMODE:-disable}
|
|
```
|
|
|
|
**Verify:** `go build ./...` compiles. With no env var set, DSN contains `sslmode=require`. With `POSTGRES_SSLMODE=disable`, DSN contains `sslmode=disable`.
|
|
|
|
**Commit:** `fix(security): make PostgreSQL sslmode configurable, default require`
|
|
|
|
---
|
|
|
|
## Phase 2: P0 WS Token Transport (30-60 min)
|
|
|
|
### Task 4: WebSocket Token Transport via Initial Message
|
|
|
|
**Objective:** Remove session token from URL query parameter; authenticate via first WS message.
|
|
|
|
This is the most complex change because it touches both backend and frontend.
|
|
|
|
**Files:**
|
|
- Modify: `internal/gateway/client.go` (ServeWS function)
|
|
- Modify: `web/src/stores/ws.ts` (connect function)
|
|
|
|
**Backend changes in `internal/gateway/client.go`:**
|
|
|
|
Replace `ServeWS` (lines 123-157):
|
|
|
|
```go
|
|
func ServeWS(db *sql.DB, hub *Hub, logger *slog.Logger, w http.ResponseWriter, r *http.Request) {
|
|
// Don't authenticate here. Accept the upgrade first.
|
|
conn, err := upgrader.Upgrade(w, r, nil)
|
|
if err != nil {
|
|
logger.Error("websocket upgrade failed", "error", err)
|
|
return
|
|
}
|
|
|
|
// Wait for the auth message with a short deadline.
|
|
conn.SetReadDeadline(time.Now().Add(10 * time.Second))
|
|
_, raw, err := conn.ReadMessage()
|
|
if err != nil {
|
|
logger.Warn("ws auth: failed to read auth message", "error", err)
|
|
conn.WriteMessage(websocket.TextMessage, []byte(`{"error":"auth timeout"}`))
|
|
conn.Close()
|
|
return
|
|
}
|
|
|
|
var authMsg struct {
|
|
Token string `json:"token"`
|
|
}
|
|
if err := json.Unmarshal(raw, &authMsg); err != nil || authMsg.Token == "" {
|
|
logger.Warn("ws auth: invalid auth message")
|
|
conn.WriteMessage(websocket.TextMessage, []byte(`{"error":"missing token"}`))
|
|
conn.Close()
|
|
return
|
|
}
|
|
|
|
var userID string
|
|
err = db.QueryRowContext(context.Background(),
|
|
`SELECT user_id FROM sessions WHERE token = $1 AND expires_at > NOW()`,
|
|
authMsg.Token,
|
|
).Scan(&userID)
|
|
if err != nil {
|
|
logger.Warn("ws auth: invalid session", "error", err)
|
|
conn.WriteMessage(websocket.TextMessage, []byte(`{"error":"invalid session"}`))
|
|
conn.Close()
|
|
return
|
|
}
|
|
|
|
// Auth succeeded. Reset read deadline.
|
|
conn.SetReadDeadline(time.Time{})
|
|
|
|
client := &Client{
|
|
Hub: hub,
|
|
Conn: conn,
|
|
UserID: userID,
|
|
send: make(chan []byte, 256),
|
|
}
|
|
|
|
hub.Register(client)
|
|
go client.writePump()
|
|
go client.readPump()
|
|
}
|
|
```
|
|
|
|
**Frontend changes in `web/src/stores/ws.ts`:**
|
|
|
|
Replace the `connect` function (lines 67-73) to open a clean WS and send the auth message:
|
|
|
|
```typescript
|
|
connect: (token: string) => {
|
|
const existing = get().socket;
|
|
if (existing && (existing.readyState === WebSocket.OPEN || existing.readyState === WebSocket.CONNECTING)) {
|
|
return;
|
|
}
|
|
|
|
const socket = new WebSocket(getWsHost() + '/ws');
|
|
|
|
// Authenticate after connection opens
|
|
socket.onopen = () => {
|
|
socket.send(JSON.stringify({ token }));
|
|
};
|
|
|
|
// ... rest of handlers stay the same, but move the onopen set({ connected: true })
|
|
// into a handler for a server "authenticated" confirmation, or just set it after
|
|
// the send since the server will close the connection if auth fails.
|
|
|
|
socket.onopen = () => {
|
|
socket.send(JSON.stringify({ token }));
|
|
set({ connected: true });
|
|
reconnectDelay = 1000;
|
|
};
|
|
|
|
// ... rest unchanged
|
|
```
|
|
|
|
Actually, cleaner approach: the server should send an `{"type":"ready"}` message after auth succeeds, and the client waits for that before marking connected:
|
|
|
|
**Server side (add after `hub.Register(client)`):**
|
|
```go
|
|
conn.WriteMessage(websocket.TextMessage, []byte(`{"type":"ready"}`))
|
|
```
|
|
|
|
**Client side:**
|
|
```typescript
|
|
socket.onopen = () => {
|
|
socket.send(JSON.stringify({ token }));
|
|
};
|
|
|
|
// In onmessage, handle the 'ready' event first:
|
|
case 'ready': {
|
|
set({ connected: true });
|
|
reconnectDelay = 1000;
|
|
return;
|
|
}
|
|
```
|
|
|
|
And remove the `onopen` setting of `connected: true`.
|
|
|
|
**Verify:**
|
|
- Build Go backend: `go build ./cmd/server`
|
|
- Build frontend: `cd web && npm run build`
|
|
- Test: WS connection without token message closes with error. WS connection with valid token gets `ready` response.
|
|
|
|
**Commit:** `fix(security): move WS auth token from URL to initial message frame`
|
|
|
|
---
|
|
|
|
## Phase 3: P1 Infrastructure Middleware (1-2 hours)
|
|
|
|
### Task 5: Rate Limiter Middleware
|
|
|
|
**Objective:** Add per-IP rate limiting to sensitive endpoints.
|
|
|
|
**Files:**
|
|
- Create: `internal/middleware/ratelimit.go`
|
|
- Modify: `cmd/server/main.go`
|
|
|
|
**Create `internal/middleware/ratelimit.go`:**
|
|
```go
|
|
package middleware
|
|
|
|
import (
|
|
"net/http"
|
|
"sync"
|
|
"time"
|
|
|
|
"golang.org/x/time/rate"
|
|
)
|
|
|
|
// ipLimiter tracks rate limiters per IP address.
|
|
type ipLimiter struct {
|
|
mu sync.Mutex
|
|
limiters map[string]*rate.Limiter
|
|
rate rate.Limit
|
|
burst int
|
|
}
|
|
|
|
func newIPLimiter(r rate.Limit, burst int) *ipLimiter {
|
|
return &ipLimiter{
|
|
limiters: make(map[string]*rate.Limiter),
|
|
rate: r,
|
|
burst: burst,
|
|
}
|
|
}
|
|
|
|
func (l *ipLimiter) getLimiter(ip string) *rate.Limiter {
|
|
l.mu.Lock()
|
|
defer l.mu.Unlock()
|
|
|
|
lim, exists := l.limiters[ip]
|
|
if !exists {
|
|
lim = rate.NewLimiter(l.rate, l.burst)
|
|
l.limiters[ip] = lim
|
|
}
|
|
return lim
|
|
}
|
|
|
|
// RateLimit returns middleware that limits requests per IP.
|
|
// rate is requests per second, burst is the burst size.
|
|
func RateLimit(requestsPerSecond float64, burst int) func(http.Handler) http.Handler {
|
|
limiter := newIPLimiter(rate.Limit(requestsPerSecond), burst)
|
|
|
|
return func(next http.Handler) http.Handler {
|
|
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
ip := r.RemoteAddr
|
|
// If behind a proxy, use X-Forwarded-For or X-Real-IP
|
|
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
|
|
ip = xff
|
|
} else if xri := r.Header.Get("X-Real-IP"); xri != "" {
|
|
ip = xri
|
|
}
|
|
|
|
if !limiter.getLimiter(ip).Allow() {
|
|
http.Error(w, `{"error":"rate limited"}`, http.StatusTooManyRequests)
|
|
return
|
|
}
|
|
|
|
next.ServeHTTP(w, r)
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
**Apply in `cmd/server/main.go`:**
|
|
|
|
For the auth routes (strict limiting):
|
|
```go
|
|
r.Route("/api/v1/auth", func(r chi.Router) {
|
|
r.Use(middleware.RateLimit(5, 10)) // 5 req/s, burst 10
|
|
authHandler.RegisterPublicRoutes(r)
|
|
})
|
|
```
|
|
|
|
For the invite join endpoint (moderate limiting):
|
|
```go
|
|
r.Route("/invites", func(r chi.Router) {
|
|
r.Use(middleware.RateLimit(2, 5)) // 2 req/s, burst 5
|
|
...
|
|
})
|
|
```
|
|
|
|
For the global API (lenient limiting):
|
|
```go
|
|
r.Route("/api/v1", func(r chi.Router) {
|
|
r.Use(middleware.RateLimit(50, 100)) // 50 req/s, burst 100
|
|
...
|
|
})
|
|
```
|
|
|
|
**Verify:** `go build ./...` compiles. Send 15 rapid requests to `/api/v1/auth/login/password`; the 11th+ should get 429.
|
|
|
|
**Commit:** `feat(security): add per-IP rate limiting middleware`
|
|
|
|
---
|
|
|
|
### Task 6: Security Headers Middleware
|
|
|
|
**Objective:** Add standard security headers to all responses.
|
|
|
|
**Files:**
|
|
- Create: `internal/middleware/security.go`
|
|
- Modify: `cmd/server/main.go`
|
|
|
|
**Create `internal/middleware/security.go`:**
|
|
```go
|
|
package middleware
|
|
|
|
import "net/http"
|
|
|
|
// SecurityHeaders returns middleware that sets common security headers.
|
|
func SecurityHeaders(next http.Handler) http.Handler {
|
|
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
w.Header().Set("X-Content-Type-Options", "nosniff")
|
|
w.Header().Set("X-Frame-Options", "DENY")
|
|
w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin")
|
|
w.Header().Set("Content-Security-Policy", "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' https: data:; connect-src 'self' wss: ws:; font-src 'self';")
|
|
w.Header().Set("Permissions-Policy", "camera=(), microphone=(self), geolocation=()")
|
|
next.ServeHTTP(w, r)
|
|
})
|
|
}
|
|
```
|
|
|
|
Note: HSTS (`Strict-Transport-Security`) is intentionally omitted here because Caddy handles it at the reverse proxy layer. If the app is ever served directly, add it conditionally when TLS is detected.
|
|
|
|
**Apply in `cmd/server/main.go`:**
|
|
```go
|
|
r := chi.NewRouter()
|
|
r.Use(chimw.Logger)
|
|
r.Use(chimw.Recoverer)
|
|
r.Use(chimw.RequestID)
|
|
r.Use(middleware.SecurityHeaders) // <-- add this
|
|
```
|
|
|
|
**Verify:** `go build ./...`. Check response headers include all the new headers.
|
|
|
|
**Commit:** `feat(security): add security headers middleware`
|
|
|
|
---
|
|
|
|
### Task 7: WS Broadcast Filtering (Cross-Server Data Leak)
|
|
|
|
**Objective:** Only broadcast events to clients who are members of the relevant server.
|
|
|
|
This is the most architecturally significant change. The hub needs to know which server each event belongs to, and which servers each user is a member of.
|
|
|
|
**Files:**
|
|
- Modify: `internal/gateway/hub.go`
|
|
- Modify: `internal/gateway/client.go`
|
|
- Modify: `internal/gateway/events.go`
|
|
- Modify: callers that use `BroadcastEvent`
|
|
|
|
**Step 1: Add server context to Event**
|
|
|
|
In `internal/gateway/events.go`, update the Event struct:
|
|
```go
|
|
type Event struct {
|
|
Type string `json:"type"`
|
|
Data interface{} `json:"data,omitempty"`
|
|
ServerID string `json:"-"` // not serialized, used for routing
|
|
}
|
|
```
|
|
|
|
**Step 2: Add membership tracking to Hub**
|
|
|
|
Add to `hub.go`:
|
|
```go
|
|
type Hub struct {
|
|
// ... existing fields ...
|
|
userServers map[string]map[string]struct{} // userID -> set of serverIDs
|
|
db *sql.DB
|
|
}
|
|
|
|
// RefreshUserServers loads the server membership for a user from the database.
|
|
func (h *Hub) RefreshUserServers(userID string) {
|
|
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
|
|
defer cancel()
|
|
|
|
rows, err := h.db.QueryContext(ctx, `SELECT server_id FROM members WHERE user_id = $1`, userID)
|
|
if err != nil {
|
|
h.logger.Error("failed to load user servers", "user_id", userID, "error", err)
|
|
return
|
|
}
|
|
defer rows.Close()
|
|
|
|
servers := make(map[string]struct{})
|
|
for rows.Next() {
|
|
var sid string
|
|
if err := rows.Scan(&sid); err == nil {
|
|
servers[sid] = struct{}{}
|
|
}
|
|
}
|
|
|
|
h.mu.Lock()
|
|
h.userServers[userID] = servers
|
|
h.mu.Unlock()
|
|
}
|
|
```
|
|
|
|
Call `RefreshUserServers` when a client connects (in `Register`) and when they leave (clean up in `Unregister`).
|
|
|
|
**Step 3: Add `BroadcastToServer` method**
|
|
|
|
```go
|
|
// BroadcastToServer sends an event only to clients who are members of the given server.
|
|
func (h *Hub) BroadcastToServer(serverID string, event Event) {
|
|
data, err := json.Marshal(event)
|
|
if err != nil {
|
|
h.logger.Error("failed to marshal event", "type", event.Type, "error", err)
|
|
return
|
|
}
|
|
|
|
h.mu.RLock()
|
|
defer h.mu.RUnlock()
|
|
|
|
for client := range h.clients {
|
|
servers, ok := h.userServers[client.UserID]
|
|
if !ok {
|
|
continue
|
|
}
|
|
if _, member := servers[serverID]; member {
|
|
select {
|
|
case client.send <- data:
|
|
default:
|
|
go func(c *Client) { h.unregister <- c }(client)
|
|
}
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
**Step 4: Update callers**
|
|
|
|
Everywhere that currently calls `hub.BroadcastEvent(...)` for server-scoped events (messages, channels, reactions, webhooks) needs to:
|
|
1. Set `ServerID` on the event, OR
|
|
2. Call `hub.BroadcastToServer(serverID, event)` instead.
|
|
|
|
Key callers to update:
|
|
- `internal/message/handlers.go` (Create, Update, Delete) -- need the serverID for the channel
|
|
- `internal/channel/handlers.go` (CRUD events)
|
|
- `internal/reaction/handlers.go`
|
|
- `internal/webhook/handlers.go` (Execute)
|
|
- `internal/bot/commands.go` (if it broadcasts events)
|
|
|
|
Keep `BroadcastEvent` for truly global events like `PRESENCE_UPDATE`.
|
|
|
|
**Verify:** User in Server A does not see messages from Server B over WS.
|
|
|
|
**Commit:** `fix(security): scope WebSocket broadcasts to server members`
|
|
|
|
---
|
|
|
|
### Task 8: Webhook Token Hashing
|
|
|
|
**Objective:** Store webhook tokens as SHA-256 hashes, not plaintext.
|
|
|
|
**Files:**
|
|
- Modify: `internal/webhook/token.go`
|
|
- Modify: `internal/webhook/handlers.go`
|
|
- Modify: `internal/db/db.go` (migration for new column)
|
|
- Create: migration or alter existing schema
|
|
|
|
**Step 1: Add hash functions to `internal/webhook/token.go`:**
|
|
```go
|
|
package webhook
|
|
|
|
import (
|
|
"crypto/rand"
|
|
"crypto/sha256"
|
|
"encoding/hex"
|
|
)
|
|
|
|
// genToken generates a 64-character hex token using crypto/rand.
|
|
func genToken() string {
|
|
b := make([]byte, 32)
|
|
if _, err := rand.Read(b); err != nil {
|
|
panic("crypto/rand failed: " + err.Error())
|
|
}
|
|
return hex.EncodeToString(b)
|
|
}
|
|
|
|
// hashToken returns the SHA-256 hex digest of a token.
|
|
func hashToken(token string) string {
|
|
h := sha256.Sum256([]byte(token))
|
|
return hex.EncodeToString(h[:])
|
|
}
|
|
```
|
|
|
|
**Step 2: Add `token_hash` column to the schema in `internal/db/db.go`:**
|
|
|
|
Add after the existing webhooks CREATE TABLE:
|
|
```sql
|
|
ALTER TABLE webhooks ADD COLUMN IF NOT EXISTS token_hash VARCHAR(64);
|
|
CREATE INDEX IF NOT EXISTS idx_webhooks_token_hash ON webhooks(token_hash);
|
|
```
|
|
|
|
**Step 3: Update `webhook/handlers.go` Create:**
|
|
|
|
Store the hash, return the plaintext token to the user once:
|
|
```go
|
|
token := genToken()
|
|
tokenHash := hashToken(token)
|
|
|
|
// INSERT uses token_hash instead of token
|
|
_, err = h.db.ExecContext(r.Context(), `
|
|
INSERT INTO webhooks (channel_id, name, token_hash, avatar, created_by)
|
|
VALUES ($1, $2, $3, $4, $5)
|
|
RETURNING id, channel_id, name, avatar, created_by, created_at::text
|
|
`, channelID, req.Name, tokenHash, req.Avatar, userID)
|
|
```
|
|
|
|
**Step 4: Update `webhook/handlers.go` Execute:**
|
|
|
|
Compare hashes instead of plaintext:
|
|
```go
|
|
var storedHash, channelID, webhookName string
|
|
err := h.db.QueryRowContext(r.Context(), `
|
|
SELECT token_hash, channel_id, name, avatar FROM webhooks WHERE id = $1
|
|
`, webhookID).Scan(&storedHash, &channelID, &webhookName, &webhookAvatar)
|
|
|
|
if hashToken(token) != storedHash {
|
|
writeErr(w, http.StatusUnauthorized, "invalid token")
|
|
return
|
|
}
|
|
```
|
|
|
|
**Step 5: Migration for existing tokens:**
|
|
|
|
If there are existing webhooks with plaintext tokens, add a one-time migration:
|
|
```sql
|
|
UPDATE webhooks SET token_hash = encode(sha256(token::bytea), 'hex') WHERE token_hash IS NULL;
|
|
ALTER TABLE webhooks DROP COLUMN IF EXISTS token;
|
|
ALTER TABLE webhooks ALTER COLUMN token_hash SET NOT NULL;
|
|
```
|
|
|
|
This is destructive (old plaintext tokens become unusable since we can't reverse the hash). Only run after confirming all webhook users have the new token format, OR regenerate all tokens and notify users.
|
|
|
|
**Verify:** Create webhook, get token back. Use token to execute webhook (works). Use wrong token (401). Check DB: only hash stored.
|
|
|
|
**Commit:** `fix(security): store webhook tokens as SHA-256 hashes`
|
|
|
|
---
|
|
|
|
## Phase 4: P2 Fixes (1-2 hours)
|
|
|
|
### Task 9: CSRF Protection
|
|
|
|
**Objective:** Validate Origin/Referer headers on state-changing requests.
|
|
|
|
**Files:**
|
|
- Create: `internal/middleware/csrf.go`
|
|
- Modify: `cmd/server/main.go`
|
|
|
|
**Create `internal/middleware/csrf.go`:**
|
|
```go
|
|
package middleware
|
|
|
|
import (
|
|
"net/http"
|
|
"strings"
|
|
)
|
|
|
|
// CSRFProtect returns middleware that validates the Origin header on
|
|
// state-changing methods (POST, PUT, PATCH, DELETE).
|
|
// trustedOrigins is the list of allowed origins.
|
|
func CSRFProtect(trustedOrigins []string) func(http.Handler) http.Handler {
|
|
originSet := make(map[string]struct{}, len(trustedOrigins))
|
|
for _, o := range trustedOrigins {
|
|
originSet[o] = struct{}{}
|
|
}
|
|
|
|
return func(next http.Handler) http.Handler {
|
|
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
// Only check state-changing methods
|
|
switch r.Method {
|
|
case "POST", "PUT", "PATCH", "DELETE":
|
|
origin := r.Header.Get("Origin")
|
|
if origin == "" {
|
|
// No Origin header: check Referer as fallback
|
|
referer := r.Header.Get("Referer")
|
|
if referer != "" {
|
|
// Extract origin from referer
|
|
for _, o := range trustedOrigins {
|
|
if strings.HasPrefix(referer, o) {
|
|
origin = o
|
|
break
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
if origin != "" {
|
|
if _, ok := originSet[origin]; !ok {
|
|
http.Error(w, `{"error":"forbidden origin"}`, http.StatusForbidden)
|
|
return
|
|
}
|
|
}
|
|
// If both Origin and Referer are empty, allow through
|
|
// (could be a non-browser client like curl/bots)
|
|
}
|
|
next.ServeHTTP(w, r)
|
|
})
|
|
}
|
|
}
|
|
```
|
|
|
|
**Apply in `cmd/server/main.go`:**
|
|
```go
|
|
r.Route("/api/v1", func(r chi.Router) {
|
|
r.Use(middleware.Session(sessionStore, cfg))
|
|
r.Use(middleware.RequireAuth)
|
|
r.Use(middleware.CSRFProtect([]string{"https://" + cfg.Host, "http://localhost:" + cfg.Port}))
|
|
// ...
|
|
})
|
|
```
|
|
|
|
Only apply to the authenticated API group, not to public webhook execution or health checks.
|
|
|
|
**Verify:** POST from `http://evil.com` origin gets 403. POST from allowed origin succeeds.
|
|
|
|
**Commit:** `feat(security): add Origin-based CSRF protection middleware`
|
|
|
|
---
|
|
|
|
### Task 10: Channel Permission Checks
|
|
|
|
**Objective:** Enforce `MANAGE_CHANNELS` permission on channel update/delete.
|
|
|
|
**Files:**
|
|
- Modify: `internal/channel/handlers.go`
|
|
- Modify: `cmd/server/main.go` (to pass the permissions checker)
|
|
|
|
**Step 1:** Add a `Checker` field to the channel handler:
|
|
```go
|
|
type Handler struct {
|
|
db *sql.DB
|
|
checker *permissions.Checker
|
|
}
|
|
|
|
func NewHandler(db *sql.DB, checker *permissions.Checker) *Handler {
|
|
return &Handler{db: db, checker: checker}
|
|
}
|
|
```
|
|
|
|
**Step 2:** Update `cmd/server/main.go` to pass the checker:
|
|
```go
|
|
permissionsChecker := permissions.NewChecker(database.DB)
|
|
// ...
|
|
channel.NewHandler(database.DB, permissionsChecker).RegisterRoutes(r)
|
|
```
|
|
|
|
**Step 3:** In `channel/handlers.go` `Update`, add permission check after membership verification:
|
|
```go
|
|
// Check MANAGE_CHANNELS permission
|
|
allowed, err := h.checker.CheckPermission(r.Context(), serverID, userID, permissions.MANAGE_CHANNELS)
|
|
if err != nil {
|
|
http.Error(w, `{"error":"server error"}`, http.StatusInternalServerError)
|
|
return
|
|
}
|
|
if !allowed {
|
|
http.Error(w, `{"error":"forbidden"}`, http.StatusForbidden)
|
|
return
|
|
}
|
|
```
|
|
|
|
**Step 4:** In `Delete`, the server owner check is fine but also add permission check for non-owners with `MANAGE_CHANNELS`:
|
|
```go
|
|
if ownerID != userID {
|
|
allowed, err := h.checker.CheckPermission(r.Context(), serverID, userID, permissions.MANAGE_CHANNELS)
|
|
if err != nil || !allowed {
|
|
http.Error(w, `{"error":"forbidden"}`, http.StatusForbidden)
|
|
return
|
|
}
|
|
}
|
|
```
|
|
|
|
**Verify:** User with no special roles gets 403 on channel update. User with `MANAGE_CHANNELS` role can update. Server owner can always delete.
|
|
|
|
**Commit:** `fix(security): enforce MANAGE_CHANNELS permission on channel update/delete`
|
|
|
|
---
|
|
|
|
### Task 11: Upload File Validation
|
|
|
|
**Objective:** Validate file types and enforce size limits on upload.
|
|
|
|
**Files:**
|
|
- Modify: `internal/upload/handlers.go`
|
|
|
|
**Step 1:** Add allowed extensions and a max file size constant:
|
|
```go
|
|
const maxUploadSize = 25 << 20 // 25 MB per file
|
|
|
|
var allowedExtensions = map[string]bool{
|
|
".jpg": true, ".jpeg": true, ".png": true, ".gif": true, ".webp": true,
|
|
".mp3": true, ".wav": true, ".ogg": true, ".flac": true,
|
|
".mp4": true, ".webm": true, ".mov": true,
|
|
".pdf": true, ".txt": true, ".md": true, ".json": true, ".csv": true,
|
|
".zip": true, ".tar": true, ".gz": true,
|
|
}
|
|
```
|
|
|
|
**Step 2:** In the `Upload` handler, add validation before the MinIO put:
|
|
```go
|
|
func (h *Handler) Upload(w http.ResponseWriter, r *http.Request) {
|
|
r.Body = http.MaxBytesReader(w, r.Body, maxUploadSize)
|
|
|
|
if err := r.ParseMultipartForm(maxUploadSize); err != nil {
|
|
http.Error(w, `{"error":"file too large or invalid form"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
|
|
file, header, err := r.FormFile("file")
|
|
if err != nil {
|
|
http.Error(w, `{"error":"missing file"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
defer file.Close()
|
|
|
|
// Validate extension
|
|
ext := ""
|
|
if idx := strings.LastIndex(header.Filename, "."); idx >= 0 {
|
|
ext = strings.ToLower(header.Filename[idx:])
|
|
}
|
|
if !allowedExtensions[ext] {
|
|
http.Error(w, `{"error":"file type not allowed"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
|
|
// Validate content type (server-side detection)
|
|
buf := make([]byte, 512)
|
|
n, _ := file.Read(buf)
|
|
detectedType := http.DetectContentType(buf[:n])
|
|
// Reset read position
|
|
file.Seek(0, io.SeekStart)
|
|
|
|
// Block HTML/JS that might be served inline
|
|
blockedTypes := []string{"text/html", "application/javascript", "application/x-executable"}
|
|
for _, blocked := range blockedTypes {
|
|
if strings.HasPrefix(detectedType, blocked) {
|
|
http.Error(w, `{"error":"file type not allowed"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
}
|
|
|
|
// ... rest of upload logic unchanged ...
|
|
```
|
|
|
|
**Verify:** Upload a `.jpg` succeeds. Upload a `.exe` or `.html` gets 400. Upload a 30MB file gets rejected.
|
|
|
|
**Commit:** `fix(security): validate file types and enforce upload size limits`
|
|
|
|
---
|
|
|
|
### Task 12: File Serve Path Sanitization
|
|
|
|
**Objective:** Prevent path traversal in file serving.
|
|
|
|
**Files:**
|
|
- Modify: `internal/upload/handlers.go`
|
|
|
|
**Update `Serve` handler:**
|
|
```go
|
|
func (h *Handler) Serve(w http.ResponseWriter, r *http.Request) {
|
|
objectName := strings.TrimPrefix(r.URL.Path, "/files/")
|
|
if objectName == "" {
|
|
http.Error(w, `{"error":"missing file"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
|
|
// Sanitize: reject path traversal attempts
|
|
if strings.Contains(objectName, "..") || strings.Contains(objectName, "/") {
|
|
http.Error(w, `{"error":"invalid path"}`, http.StatusBadRequest)
|
|
return
|
|
}
|
|
|
|
// Set Content-Disposition: attachment to prevent inline rendering
|
|
w.Header().Set("Content-Disposition", "attachment")
|
|
|
|
ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second)
|
|
defer cancel()
|
|
|
|
obj, err := h.client.GetObject(ctx, h.bucket, objectName, minio.GetObjectOptions{})
|
|
if err != nil {
|
|
http.Error(w, `{"error":"not found"}`, http.StatusNotFound)
|
|
return
|
|
}
|
|
defer obj.Close()
|
|
|
|
stat, err := obj.Stat()
|
|
if err != nil {
|
|
http.Error(w, `{"error":"not found"}`, http.StatusNotFound)
|
|
return
|
|
}
|
|
|
|
w.Header().Set("Content-Type", stat.ContentType)
|
|
w.Header().Set("Content-Length", fmt.Sprintf("%d", stat.Size))
|
|
io.Copy(w, obj)
|
|
}
|
|
```
|
|
|
|
**Verify:** `GET /files/../../etc/passwd` returns 400. `GET /files/<valid-uuid>.jpg` works.
|
|
|
|
**Commit:** `fix(security): sanitize file serve paths and add Content-Disposition`
|
|
|
|
---
|
|
|
|
## Execution Order
|
|
|
|
1. **Task 1** (WS Origin) + **Task 2** (WebAuthn Cookie) + **Task 3** (DB sslmode) -- independent, fast
|
|
2. **Task 4** (WS Token Transport) -- needs Task 1 done first
|
|
3. **Task 5** (Rate Limiter) + **Task 6** (Security Headers) -- independent, create middleware
|
|
4. **Task 7** (WS Broadcast Filtering) -- independent but complex
|
|
5. **Task 8** (Webhook Token Hashing) -- independent
|
|
6. **Task 9** (CSRF) -- benefits from Rate Limiter existing
|
|
7. **Task 10** (Channel Perms) + **Task 11** (Upload Validation) + **Task 12** (Path Sanitization) -- independent
|
|
|
|
Total estimated time: 4-6 hours for all 12 tasks.
|
|
|
|
---
|
|
|
|
## Post-Remediation Checklist
|
|
|
|
- [ ] `go build ./cmd/server` succeeds
|
|
- [ ] `go build ./cmd/tui` succeeds
|
|
- [ ] `cd web && npm run build` succeeds
|
|
- [ ] `govulncheck ./...` still clean
|
|
- [ ] Manual test: login, send message, upload file, use webhook
|
|
- [ ] Manual test: WS from wrong origin rejected
|
|
- [ ] Manual test: rate limiting fires on brute-force login
|
|
- [ ] Manual test: non-member doesn't see cross-server events
|