pop/FRE-683-SECURITY-FIXES.md

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:

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:

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):

req := contact.UpdateContactRequest{
    Name:   &name,
    Phone:  &phone,
    Address: &address,
    Notes:  &notes,
}

After (Fixed):

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:

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