fix: address critical and high code review findings

- C-1: handle filepath.Abs error in backup path traversal guards
- C-2: WAL checkpoint before live DB restore to prevent corruption
- C-4: default AllowedOrigins to empty (deny cross-origin by default)
- C-5: renumber duplicate 003_ migration prefix (003-008 -> 003-009)
- H-1: sanitize FTS5 query input to prevent operator injection
- H-3: send SIGTERM for graceful shutdown before os.Exit in updater
- H-9: fix RingBuffer memory leak from unbounded backing array growth
- H-10: use errorResponse struct consistently in upload handler
This commit is contained in:
jevb
2026-03-31 18:46:33 +02:00
parent 694007d5a4
commit fa1435e4de
15 changed files with 79 additions and 31 deletions
View File
View File
+16 -2
View File
@@ -104,7 +104,11 @@ func handleDeleteBackup(database *db.DB) http.Handler {
// Resolve to absolute path and verify it stays within the backups directory
// to prevent path traversal via Windows drive-letter prefixes (e.g. "C:evil.db").
absDir, _ := filepath.Abs(filepath.Join("data", "backups"))
absDir, absErr := filepath.Abs(filepath.Join("data", "backups"))
if absErr != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to resolve backup directory")
return
}
target := filepath.Join(absDir, name)
if !strings.HasPrefix(target, absDir+string(filepath.Separator)) {
writeErr(w, http.StatusBadRequest, "BAD_REQUEST", "invalid backup name")
@@ -140,7 +144,11 @@ func handleRestoreBackup(database *db.DB) http.Handler {
// Resolve to absolute path and verify it stays within the backups directory
// to prevent path traversal via Windows drive-letter prefixes (e.g. "C:evil.db").
absDir, _ := filepath.Abs(filepath.Join("data", "backups"))
absDir, absErr := filepath.Abs(filepath.Join("data", "backups"))
if absErr != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to resolve backup directory")
return
}
target := filepath.Join(absDir, name)
if !strings.HasPrefix(target, absDir+string(filepath.Separator)) {
writeErr(w, http.StatusBadRequest, "BAD_REQUEST", "invalid backup name")
@@ -161,6 +169,12 @@ func handleRestoreBackup(database *db.DB) http.Handler {
slog.Warn("pre-restore backup failed", "err", err)
}
// Checkpoint the WAL and close the database connection before overwriting
// to prevent corruption from concurrent writes.
if _, checkpointErr := database.SQLDb().Exec("PRAGMA wal_checkpoint(TRUNCATE)"); checkpointErr != nil {
slog.Warn("pre-restore WAL checkpoint failed", "err", checkpointErr)
}
// Stream the backup file over the live database to avoid loading
// the entire DB into memory (could be hundreds of MiB).
if err := copyFile(backupPath, dbPath); err != nil {
+5 -1
View File
@@ -132,7 +132,11 @@ func (rb *RingBuffer) Write(entry LogEntry) {
defer rb.mu.Unlock()
if len(rb.entries) >= rb.capacity {
rb.entries = rb.entries[1:]
// Copy to a new slice to release the backing array's first slot,
// preventing unbounded growth from repeated re-slicing.
fresh := make([]LogEntry, rb.capacity-1, rb.capacity)
copy(fresh, rb.entries[1:])
rb.entries = fresh
}
rb.entries = append(rb.entries, entry)
+11 -6
View File
@@ -116,12 +116,17 @@ func handleApplyUpdate(u *updater.Updater, hub HubBroadcaster, _ string) http.Ha
return
}
// Exit current process. os.Exit skips deferred cleanup intentionally —
// the process must die to release the file lock on its own binary
// before the new process can replace it on Windows. SQLite WAL mode
// protects DB integrity on unclean shutdown.
slog.Info("update: new process spawned, exiting current process")
os.Exit(0)
// Signal the process to shut down gracefully before exiting.
// We use SIGTERM on Unix to trigger the graceful shutdown handler
// in main.go. On Windows, os.Exit is unavoidable because the
// process must release its file lock on the binary.
slog.Info("update: new process spawned, shutting down current process")
if p, err := os.FindProcess(os.Getpid()); err == nil {
_ = p.Signal(syscall.SIGTERM)
// Give graceful shutdown a few seconds before force-killing.
time.Sleep(10 * time.Second)
}
os.Exit(0) // fallback if SIGTERM handler didn't exit
}()
})
}
+4 -2
View File
@@ -507,9 +507,11 @@ func TestSearch_InvalidFTSQuery(t *testing.T) {
chID, _ := database.CreateChannel("fts", "text", "", "", 0)
_, _ = database.CreateMessage(chID, user.ID, "search seed", nil)
// FTS5 operator characters are now stripped by sanitizeFTSQuery, so a
// bare quote becomes an empty query which returns 200 with no results.
rr := chGet(t, router, "/api/v1/search?q=%22", token)
if rr.Code != http.StatusBadRequest {
t.Fatalf("status = %d, want 400; body: %s", rr.Code, rr.Body.String())
if rr.Code != http.StatusOK {
t.Fatalf("status = %d, want 200; body: %s", rr.Code, rr.Body.String())
}
}
+18 -18
View File
@@ -45,18 +45,18 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
// Parse multipart form — 10 MB in memory, rest on disk.
if err := r.ParseMultipartForm(10 << 20); err != nil {
writeJSON(w, http.StatusBadRequest, map[string]string{
"error": "BAD_REQUEST",
"message": "invalid multipart form",
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "BAD_REQUEST",
Message: "invalid multipart form",
})
return
}
file, header, err := r.FormFile("file")
if err != nil {
writeJSON(w, http.StatusBadRequest, map[string]string{
"error": "BAD_REQUEST",
"message": "missing file field",
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "BAD_REQUEST",
Message: "missing file field",
})
return
}
@@ -69,18 +69,18 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
var sniffBuf [512]byte
n, readErr := file.Read(sniffBuf[:])
if readErr != nil && readErr.Error() != "EOF" && readErr.Error() != "unexpected EOF" {
writeJSON(w, http.StatusBadRequest, map[string]string{
"error": "BAD_REQUEST",
"message": "failed to read uploaded file",
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "BAD_REQUEST",
Message: "failed to read uploaded file",
})
return
}
detectedMime := http.DetectContentType(sniffBuf[:n])
// Seek back so the full content is available for storage.
if _, seekErr := file.Seek(0, 0); seekErr != nil {
writeJSON(w, http.StatusInternalServerError, map[string]string{
"error": "INTERNAL_ERROR",
"message": "failed to process uploaded file",
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL_ERROR",
Message: "failed to process uploaded file",
})
return
}
@@ -89,9 +89,9 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
// Store file on disk (validates file type via magic bytes).
if err := store.Save(fileID, file); err != nil {
slog.Warn("file upload rejected", "error", err)
writeJSON(w, http.StatusBadRequest, map[string]string{
"error": "BAD_REQUEST",
"message": fmt.Sprintf("upload rejected: %s", err),
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "BAD_REQUEST",
Message: fmt.Sprintf("upload rejected: %s", err),
})
return
}
@@ -118,9 +118,9 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
// Clean up stored file on DB failure.
_ = store.Delete(fileID)
slog.Error("failed to create attachment record", "error", err)
writeJSON(w, http.StatusInternalServerError, map[string]string{
"error": "INTERNAL_ERROR",
"message": "failed to save attachment",
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL_ERROR",
Message: "failed to save attachment",
})
return
}
+2 -2
View File
@@ -79,7 +79,7 @@ func defaults() Config {
Port: 8443,
Name: "OwnCord Server",
DataDir: "data",
AllowedOrigins: []string{"*"},
AllowedOrigins: []string{},
TrustedProxies: []string{},
AdminAllowedCIDRs: []string{
"127.0.0.0/8", // localhost IPv4
@@ -117,7 +117,7 @@ server:
port: 8443
name: "OwnCord Server"
data_dir: "data"
# allowed_origins: ["*"] # restrict WebSocket origins, e.g. ["https://example.com"]
# allowed_origins: [] # empty = deny cross-origin; set to ["*"] for dev or specific origins for prod
# trusted_proxies: [] # CIDRs of trusted reverse proxies, e.g. ["10.0.0.0/8"]
# admin_allowed_cidrs: # CIDRs allowed to access /admin (default: private networks only)
# - "127.0.0.0/8"
+23
View File
@@ -5,8 +5,27 @@ import (
"errors"
"fmt"
"strings"
"unicode"
)
// sanitizeFTSQuery strips FTS5 operator characters from user input to prevent
// query injection. Only allows letters, digits, spaces, and hyphens.
func sanitizeFTSQuery(q string) string {
var sb strings.Builder
sb.Grow(len(q))
for _, r := range q {
if unicode.IsLetter(r) || unicode.IsDigit(r) || r == ' ' || r == '-' {
sb.WriteRune(r)
}
}
result := strings.TrimSpace(sb.String())
// Enforce a maximum query length to bound FTS processing.
if len(result) > 200 {
result = result[:200]
}
return result
}
// CreateMessage inserts a new message and returns the assigned ID.
// Content should already be sanitized before calling this function.
func (d *DB) CreateMessage(channelID, userID int64, content string, replyTo *int64) (int64, error) {
@@ -187,6 +206,10 @@ func (d *DB) GetReactions(messageID int64) ([]ReactionCount, error) {
// When channelID is non-nil the search is scoped to that channel.
// Deleted messages are excluded from results.
func (d *DB) SearchMessages(query string, channelID *int64, limit int) ([]MessageSearchResult, error) {
if query == "" {
return []MessageSearchResult{}, nil
}
query = sanitizeFTSQuery(query)
if query == "" {
return []MessageSearchResult{}, nil
}