notifier/docs/REMEDIATION_PLAN.md

Remediation Action Plan

Quick Reference

Priority	Count	Effort	Timeline	Status
Critical	3	High	Week 1	🔴 Not Started
High	7	High	Week 2-3	🔴 Not Started
Medium	30	Medium	Sprint 2-3	🔴 Not Started
Low	10	Low	Ongoing	🔴 Not Started

---

Phase 1: Critical Issues (Week 1)

CRITICAL-1: Unbounded Memory Growth

Status: 🔴 Not Started Effort: 4-6 hours File: internal/service/service.go

Implementation Steps:

1. Create retention policy configuration:

// In config.go
type NotificationRetentionConfig struct {
    Enabled   bool          `mapstructure:"enabled"`
    TTL       time.Duration `mapstructure:"ttl"`        // Default: 7 days
    CheckFrequency time.Duration `mapstructure:"check_frequency"` // Default: 1 hour
    MaxSize   int           `mapstructure:"max_size"`   // Default: 100,000
}

2. Add to service initialization:

// In service.go
type NotificationService struct {
    // ... existing fields ...
    retentionConfig *config.NotificationRetentionConfig
    cleanupDone      chan struct{}
}

func NewNotificationService(
    factory domain.NotifierFactory,
    q domain.Queue,
    workerCount int,
    cfg *config.Config,
    retentionCfg *config.NotificationRetentionConfig,
    logger *logging.Logger,
) *NotificationService {
    // ... existing code ...
    svc := &NotificationService{
        // ... initialization ...
        retentionConfig: retentionCfg,
        cleanupDone:     make(chan struct{}),
    }

    // Start cleanup goroutine if enabled
    if retentionCfg.Enabled {
        go svc.cleanupLoop()
    }

    return svc
}

func (s *NotificationService) cleanupLoop() {
    ticker := time.NewTicker(s.retentionConfig.CheckFrequency)
    defer ticker.Stop()

    for {
        select {
        case <-s.stopChan:
            return
        case <-ticker.C:
            s.cleanupExpiredNotifications()
        }
    }
}

func (s *NotificationService) cleanupExpiredNotifications() {
    s.mu.Lock()
    defer s.mu.Unlock()

    now := time.Now()
    cutoff := now.Add(-s.retentionConfig.TTL)
    count := 0

    for id, notif := range s.notifications {
        if notif.CreatedAt.Before(cutoff) {
            delete(s.notifications, id)
            count++
        }
    }

    // Also check max size
    if len(s.notifications) > s.retentionConfig.MaxSize {
        // Sort by creation time and remove oldest
        var notifs []*domain.Notification
        for _, n := range s.notifications {
            notifs = append(notifs, n)
        }
        sort.Slice(notifs, func(i, j int) bool {
            return notifs[i].CreatedAt.Before(notifs[j].CreatedAt)
        })

        toRemove := len(notifs) - s.retentionConfig.MaxSize
        for i := 0; i < toRemove; i++ {
            delete(s.notifications, notifs[i].ID)
            count++
        }
    }

    if count > 0 {
        s.logger.Infof("Cleaned up %d expired notifications", count)
    }
}

func (s *NotificationService) Stop() error {
    // ... existing stop code ...
    <-s.cleanupDone  // Wait for cleanup to finish
    return nil
}

3. Update configuration defaults:

# config.yaml
notification:
  retention:
    enabled: true
    ttl: 168h  # 7 days
    check_frequency: 1h
    max_size: 100000

4. Add tests:

func TestNotificationCleanup(t *testing.T) {
    // Test TTL-based removal
    // Test max size enforcement
    // Test cleanup frequency
}

Acceptance Criteria:

• Notifications older than TTL are removed
• Maximum size limit is enforced
• Cleanup runs at specified frequency
• Memory doesn't grow indefinitely
• Tests pass with 100% coverage

---

CRITICAL-2: Remove TLS Verification Bypass

Status: 🔴 Not Started Effort: 2-3 hours File: internal/notifier/ntfy.go

Implementation Steps:

1. Update NtfyConfig:

// Remove InsecureSkipVerify, add custom CA support
type NtfyConfig struct {
    ServerURL   string `mapstructure:"server_url"`
    Token       string `mapstructure:"token"`
    Username    string `mapstructure:"username"`
    Password    string `mapstructure:"password"`
    DefaultTopic string `mapstructure:"default_topic"`
    // REMOVED: InsecureSkipVerify bool

    // ADD: Custom CA certificate support
    CACertPath  string `mapstructure:"ca_cert_path"`  // Path to CA cert file
    Default     bool   `mapstructure:"default"`
    AllowedRoles []string `mapstructure:"allowed_roles"`
}

func (nc *NtfyConfig) Validate() error {
    if nc.ServerURL == "" {
        return fmt.Errorf("server_url is required")
    }
    if nc.Token == "" && (nc.Username == "" || nc.Password == "") {
        return fmt.Errorf("either token or username/password required")
    }
    // CACertPath is optional but if provided, must exist
    if nc.CACertPath != "" {
        if _, err := os.Stat(nc.CACertPath); err != nil {
            return fmt.Errorf("ca_cert_path file not found: %w", err)
        }
    }
    return nil
}

2. Update HTTP client creation:

func NewNtfyNotifier(config *NtfyConfig) (*NtfyNotifier, error) {
    if config == nil {
        return nil, fmt.Errorf("ntfy config is required")
    }

    if err := config.Validate(); err != nil {
        return nil, err
    }

    httpClient, err := createNtfyHTTPClient(config)
    if err != nil {
        return nil, err
    }

    notifier := &NtfyNotifier{
        config:     config,
        httpClient: httpClient,
    }
    notifier.BaseNotifier.notificationType = domain.TypeNtfy
    return notifier, nil
}

func createNtfyHTTPClient(config *NtfyConfig) (*http.Client, error) {
    var tlsConfig *tls.Config

    if config.CACertPath != "" {
        caCert, err := ioutil.ReadFile(config.CACertPath)
        if err != nil {
            return nil, fmt.Errorf("failed to read CA cert: %w", err)
        }

        caCertPool := x509.NewCertPool()
        if !caCertPool.AppendCertsFromPEM(caCert) {
            return nil, fmt.Errorf("failed to parse CA cert")
        }

        tlsConfig = &tls.Config{
            RootCAs: caCertPool,
        }
    } else {
        // Use system default CA pool
        tlsConfig = &tls.Config{}
    }

    return &http.Client{
        Timeout: 30 * time.Second,
        Transport: &http.Transport{
            TLSClientConfig: tlsConfig,
            MaxIdleConns:    100,
            IdleConnTimeout: 90 * time.Second,
        },
    }, nil
}

3. Update documentation:

## TLS Configuration

### System Default (Recommended)
```yaml
notifiers:
  ntfy:
    default:
      server_url: "https://ntfy.sh"
      token: "your-token"
      # Uses system CA certificates automatically

Custom CA Certificate (Self-Signed)

notifiers:
  ntfy:
    default:
      server_url: "https://internal-ntfy.example.com"
      token: "your-token"
      ca_cert_path: "/etc/certs/ca.pem"

IMPORTANT: TLS verification is ALWAYS enabled. Insecure self-signed certificates cannot be accepted without providing a valid CA certificate.

4. Add tests:
```go
func TestNtfyTLSValidation(t *testing.T) {
    // Test that invalid certs are rejected
    // Test custom CA cert acceptance
    // Test system CA pool usage
}

Acceptance Criteria:

• InsecureSkipVerify option removed
• Custom CA certificate support works
• TLS validation always enabled
• Error messages clear when certs invalid
• Documentation updated
• Tests pass

---

CRITICAL-3: Fix CORS Configuration

Status: 🔴 Not Started Effort: 2-3 hours File: api/rest/router.go

Implementation Steps:

1. Update CORS middleware:

type CORSConfig struct {
    AllowedOrigins   []string
    AllowedMethods   []string
    AllowedHeaders   []string
    AllowCredentials bool
    MaxAge           int
}

func newCORSMiddleware(config *CORSConfig) func(http.Handler) http.Handler {
    // Build origin map for O(1) lookup
    originMap := make(map[string]bool)
    for _, origin := range config.AllowedOrigins {
        originMap[origin] = true
    }

    return func(next http.Handler) http.Handler {
        return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            origin := r.Header.Get("Origin")

            // Check if origin is allowed
            if origin != "" && originMap[origin] {
                w.Header().Set("Access-Control-Allow-Origin", origin)
                if config.AllowCredentials {
                    w.Header().Set("Access-Control-Allow-Credentials", "true")
                }
            }

            w.Header().Set("Access-Control-Allow-Methods", strings.Join(config.AllowedMethods, ", "))
            w.Header().Set("Access-Control-Allow-Headers", strings.Join(config.AllowedHeaders, ", "))
            w.Header().Set("Access-Control-Max-Age", fmt.Sprintf("%d", config.MaxAge))

            if r.Method == http.MethodOptions {
                w.WriteHeader(http.StatusOK)
                return
            }

            next.ServeHTTP(w, r)
        })
    }
}

func NewRouterWithAuth(service domain.NotificationService, logger *logging.Logger, authStore *auth.APIKeyStore, corsConfig *CORSConfig) *mux.Router {
    handler := NewHandler(service, logger)
    router := mux.NewRouter()

    // API v1 routes with CORS
    v1 := router.PathPrefix("/api/v1").Subrouter()
    v1.Use(newCORSMiddleware(corsConfig))

    // ... rest of router setup ...
}

2. Add configuration:

server:
  cors:
    allowed_origins:
      - "https://example.com"
      - "https://app.example.com"
    allowed_methods:
      - "GET"
      - "POST"
      - "OPTIONS"
      - "DELETE"
    allowed_headers:
      - "Content-Type"
      - "Authorization"
    allow_credentials: true
    max_age: 3600

3. Update main.go:

corsConfig := &rest.CORSConfig{
    AllowedOrigins:   cfg.Server.CORS.AllowedOrigins,
    AllowedMethods:   cfg.Server.CORS.AllowedMethods,
    AllowedHeaders:   cfg.Server.CORS.AllowedHeaders,
    AllowCredentials: cfg.Server.CORS.AllowCredentials,
    MaxAge:           cfg.Server.CORS.MaxAge,
}

restServer := startRESTServer(ctx, &wg, cfg, svc, logger, authStore, corsConfig)

4. Add tests and validation:

func TestCORSOriginValidation(t *testing.T) {
    // Test allowed origins accepted
    // Test disallowed origins rejected
    // Test preflight requests
    // Test credentials header handling
}

Acceptance Criteria:

• CORS origins are configurable
• Wildcard (*) is not accepted
• Only configured origins are allowed
• Preflight requests handled correctly
• Configuration validated at startup
• Tests pass

---

Phase 2: High Priority Issues (Week 2-3)

HIGH-1: Add Request Size Limits

Status: 🔴 Not Started Effort: 2 hours File: api/rest/handlers.go

Steps:

1. Add constant: const MaxRequestSize = 10 * 1024 * 1024 // 10MB
2. Update SendNotification handler: Add http.MaxBytesReader()
3. Update SendBatchNotifications handler: Add http.MaxBytesReader()
4. Add configuration option for max size
5. Add tests for size limit enforcement

HIGH-2: Implement Sharded Locks

Status: 🔴 Not Started Effort: 6-8 hours File: internal/service/service.go

Steps:

1. Create sharded storage structure
2. Implement shard index function (fnv hash)
3. Update all notification operations
4. Add benchmarks comparing to original
5. Add concurrency tests

HIGH-3: Fix Lock Ordering Issues

Status: 🔴 Not Started Effort: 4 hours File: internal/queue/local.go

Steps:

1. Release locks before channel operations
2. Copy references while holding locks
3. Add timeout for channel operations
4. Add deadlock detection tests

HIGH-4: Separate Service Concerns

Status: 🔴 Not Started Effort: 16-20 hours File: internal/service/service.go + new files

Steps:

1. Create internal/repository/repository.go
2. Create internal/filter/filter.go
3. Create internal/stats/stats.go
4. Update service to use these components
5. Add comprehensive tests

HIGH-5: Fix Filtering Algorithm

Status: 🔴 Not Started Effort: 3 hours File: internal/service/service.go

Steps:

1. Replace nested loops with map-based lookup
2. Add benchmarks
3. Update tests
4. Document O(n) vs O(n*m) improvement

HIGH-6: Fix RWMutex Usage in Factory

Status: 🔴 Not Started Effort: 2 hours File: internal/notifier/notifier.go

Steps:

1. Copy keys under lock
2. Process outside lock
3. Add tests for concurrent access

HIGH-7: Fix Goroutine Lifecycle

Status: 🔴 Not Started Effort: 4 hours File: internal/service/service.go

Steps:

1. Add workerDoneChan
2. Implement graceful shutdown
3. Add timeout for worker stoppage
4. Add stress tests

---

Phase 3: Medium Priority (Sprint 2-3)

MEDIUM-1: Add Custom Error Types

Status: 🔴 Not Started Effort: 4 hours File: New internal/errors/errors.go

Implementation:

// errors.go
var (
    ErrNotFound          = errors.New("notification not found")
    ErrQueueClosed       = errors.New("queue is closed")
    ErrNotifierNotFound  = errors.New("notifier not found")
    ErrRateLimited       = errors.New("rate limit exceeded")
    ErrInvalidConfig     = errors.New("invalid configuration")
)

// Use in code:
if err := doSomething(); err != nil {
    if errors.Is(err, ErrQueueClosed) {
        // Handle specific error
    }
}

MEDIUM-2: Add Structured Logging

Status: 🔴 Not Started Effort: 8-10 hours File: internal/logging/logger.go

Implementation:

• Migrate from custom logger to log/slog (Go 1.21+)
• Support JSON output
• Add structured fields
• Update all log calls

MEDIUM-3: Extract Logger Interface

Status: 🔴 Not Started Effort: 3 hours

Implementation:

type Logger interface {
    Debug(msg string, keysAndValues ...interface{})
    Info(msg string, keysAndValues ...interface{})
    Warn(msg string, keysAndValues ...interface{})
    Error(msg string, keysAndValues ...interface{})
}

MEDIUM-4: Add Input Validation

Status: 🔴 Not Started Effort: 6 hours

Add validation for:

• Email addresses (use net/mail)
• URLs (use url.Parse())
• Domain checks
• Recipient limits
• Message size limits

MEDIUM-5: Add Configuration Validation

Status: 🔴 Not Started Effort: 4 hours File: internal/config/config.go

Validate:

• Worker count > 0
• Queue size > 0
• Timeouts reasonable
• Port ranges valid

---

Phase 4: Low Priority (Ongoing)

LOW-1: Add Package Documentation

Status: 🔴 Not Started Effort: 8 hours Action: Create doc.go in each package

LOW-2: Consistent Naming

Status: 🔴 Not Started Effort: 2 hours Action:

• Use svc for service receivers
• Use n for notifier receivers
• Use h for handler receivers

LOW-3: Remove Unused Code

Status: 🔴 Not Started Effort: 1 hour Action:

• Remove unused config fields
• Remove TODO comments
• Remove stub implementations

---

Testing Strategy

Unit Tests to Add

- [x] Notification TTL/cleanup
- [x] CORS origin validation
- [x] Request size limits
- [x] Sharded lock functionality
- [x] Filter algorithm performance
- [x] Error type handling
- [x] Configuration validation
- [x] Custom error type usage

Integration Tests to Add

- [x] End-to-end with authentication
- [x] Concurrent notifications
- [x] Graceful shutdown
- [x] TLS certificate validation
- [x] Rate limiting boundaries
- [x] Queue overflow handling

Benchmarks to Add

- [x] Filtering performance (nested vs map)
- [x] Lock contention (single vs sharded)
- [x] Memory usage over time
- [x] Concurrent operations

---

Deployment Checklist

Before deploying each phase:

• All tests pass
• No race condition warnings
• Code reviewed
• Documentation updated
• Backward compatibility verified
• Performance benchmarks acceptable
• Security review completed
• Monitoring alerts configured

---

Timeline

Phase	Effort	Timeline	Status
Phase 1 (Critical)	20 hours	Week 1	🔴 Planned
Phase 2 (High)	40 hours	Week 2-3	🔴 Planned
Phase 3 (Medium)	40 hours	Sprint 2-3	🔴 Planned
Phase 4 (Low)	20 hours	Ongoing	🔴 Planned
Total	120 hours	4-6 weeks

---

Success Metrics

After remediation:

• Zero memory leaks in long-running tests
• 95%+ latency unchanged under load
• 16x improvement in lock contention
• 100% TLS validation in place
• All critical security issues resolved
• 90%+ test coverage
• Structured logging enabled
• Custom error types in use
• Configuration validation at startup
• Documentation complete

---

Notes for Implementation

1. Backward Compatibility: Each phase should maintain backward compatibility
2. Gradual Rollout: Test thoroughly before deploying to production
3. Monitoring: Add metrics to detect improvements
4. Documentation: Update docs with each change
5. Code Review: Require review for security-critical changes
6. Testing: Add tests before implementation where possible

---

Questions & Clarifications

1. Configuration retention TTL: Should this be per-notification or global?

   • Recommendation: Global with override per environment

2. CORS origins: Should these be environment-specific?

   • Recommendation: Yes, different for dev/staging/prod

3. Custom error types: Should these be exported?

   • Recommendation: Yes, for consumers to use errors.Is()

4. Logging migration: Breaking change or gradual?

   • Recommendation: Gradual, add structured logging alongside current

5. Sharded locks: How many shards optimal?

   • Recommendation: Start with 16, benchmark for your use case