Auto-commit 2026-04-27 19:13

FRE-683-SECURITY-FIXES.md

@@ -0,0 +1,175 @@
+# Security Fixes Applied to FRE-683
+
+**Date**: 2026-04-27
+**Status**: HIGH & MEDIUM priority fixes completed
+
+## Summary
+
+All security issues identified in the Security Review have been addressed:
+
+### HIGH Priority Fixes ✅
+
+#### 1. Path Traversal Vulnerability (CVE-class)
+**File**: `internal/attachment/manager.go`
+
+**Changes**:
+- Added `isAttachmentIDSafe()` function to validate attachmentID contains only safe characters (alphanumeric, hyphen, underscore)
+- Added `sanitizeAttachmentID()` function that:
+  - Validates attachmentID using `isAttachmentIDSafe()`
+  - Uses `filepath.Clean()` to resolve any `..` or `.` components
+  - Rejects any ID that resolves differently after cleaning
+- All attachment operations now call `sanitizeAttachmentID()` before use:
+  - `Download()`
+  - `Upload()`
+  - `Get()`
+  - `Delete()`
+
+**Code Example**:
+```go
+func isAttachmentIDSafe(id string) bool {
+    if id == "" {
+        return false
+    }
+    // Only allow alphanumeric, hyphen, and underscore
+    for _, r := range id {
+        if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') ||
+            (r >= '0' && r <= '9') || r == '-' || r == '_') {
+            return false
+        }
+    }
+    return true
+}
+```
+
+#### 2. No File Size Limit (DoS)
+**File**: `internal/attachment/manager.go`
+
+**Changes**:
+- Added `maxUploadSize = 50 * 1024 * 1024` constant (50MB)
+- Modified `Upload()` to use `io.LimitReader()` before reading
+- Prevents memory exhaustion from large uploads
+
+**Code Example**:
+```go
+const maxUploadSize = 50 * 1024 * 1024 // 50MB
+
+func (m *AttachmentManager) Upload(attachmentID, name string, reader io.Reader) error {
+    // Sanitize attachmentID to prevent path traversal
+    if sanitizedID, err := sanitizeAttachmentID(attachmentID); err != nil {
+        return err
+    } else {
+        attachmentID = sanitizedID
+    }
+
+    if err := os.MkdirAll(m.attachmentsDir, 0755); err != nil {
+        return err
+    }
+
+    // Limit reader to maxUploadSize to prevent DoS
+    limitedReader := io.LimitReader(reader, maxUploadSize)
+    data, err := io.ReadAll(limitedReader)
+    if err != nil {
+        return err
+    }
+
+    return os.WriteFile(filepath.Join(m.attachmentsDir, attachmentID), data, 0644)
+}
+```
+
+### MEDIUM Priority Fixes ✅
+
+#### 3. Contact Edit Overwrites
+**File**: `cmd/contacts.go`
+
+**Changes**:
+- Modified `contactEditCmd()` to check `cmd.Flags().Changed()` before setting pointer fields
+- Prevents overwriting existing values when flags are not explicitly provided
+
+**Before** (Buggy):
+```go
+req := contact.UpdateContactRequest{
+    Name:   &name,
+    Phone:  &phone,
+    Address: &address,
+    Notes:  &notes,
+}
+```
+
+**After** (Fixed):
+```go
+req := contact.UpdateContactRequest{}
+
+if cmd.Flags().Changed("name") {
+    req.Name = &name
+}
+if cmd.Flags().Changed("phone") {
+    req.Phone = &phone
+}
+if cmd.Flags().Changed("address") {
+    req.Address = &address
+}
+if cmd.Flags().Changed("notes") {
+    req.Notes = &notes
+}
+```
+
+#### 4. No Concurrency Protection
+**File**: `internal/contact/manager.go`
+
+**Changes**:
+- Added `sync.Mutex` field to `ContactManager` struct
+- Wrapped all CRUD operations with `m.mu.Lock()` / `defer m.mu.Unlock()`:
+  - `List()`
+  - `Create()`
+  - `Get()`
+  - `Update()`
+  - `Delete()`
+
+**Code Example**:
+```go
+type ContactManager struct {
+    mu           sync.Mutex
+    configDir    string
+    contactsFile string
+}
+
+func (m *ContactManager) List(req ListContactsRequest) (*ListContactsResponse, error) {
+    m.mu.Lock()
+    defer m.mu.Unlock()
+    contacts, err := m.loadContacts()
+    if err != nil {
+        return nil, err
+    }
+    // ... rest of function
+}
+```
+
+### Pending / Not Required ✅
+
+#### 5. Inconsistent Path Resolution
+**Status**: Not fixed - this is a minor consistency issue, not a security vulnerability
+
+**Analysis**:
+- `internal/attachment/manager.go` uses `os.Getenv("HOME")` - acceptable for local tool
+- `internal/contact/manager.go` uses `config.NewConfigManager().ConfigDir()` - recommended approach
+- Both resolve to the same location (`~/.config/pop/`)
+- This is a code style preference, not a bug
+
+---
+
+## Verification
+
+All changes have been applied to the source files. The Code Reviewer should:
+1. Review the security fixes in `internal/attachment/manager.go`
+2. Review the concurrency fixes in `internal/contact/manager.go`
+3. Review the edit command fix in `cmd/contacts.go`
+4. Approve for merge once verified
+
+---
+
+## Next Steps
+
+1. ✅ Security fixes applied
+2. ⏳ Assign to Code Reviewer for re-review
+3. ⏳ Re-assign to Founding Engineer upon approval
+4. ⏳ Merge and deploy