fix: address code review findings for DNS management
- CRITICAL: Change DNS zones endpoint from GET to POST to avoid leaking API token in URL query parameters - HIGH: Add sync.RWMutex to protect dnsProvider field in Server, Deployer, and proxy Manager against concurrent read/write races - HIGH: Capture old DNS provider reference synchronously before launching background cleanup goroutine - HIGH: Use getDNS()/getDNSProviderLocked() accessors instead of direct field reads in all DNS operations
This commit is contained in:
+2
-2
@@ -208,8 +208,8 @@ func (s *Server) buildConsumerNameMap() map[string]string {
|
||||
|
||||
// getOrCreateDNSProvider returns the server's DNS provider, or creates a temporary one from settings.
|
||||
func (s *Server) getOrCreateDNSProvider(settings store.Settings) dns.Provider {
|
||||
if s.dnsProvider != nil {
|
||||
return s.dnsProvider
|
||||
if p := s.getDNSProviderLocked(); p != nil {
|
||||
return p
|
||||
}
|
||||
|
||||
if settings.WildcardDNS || settings.DNSProvider == "" || settings.CloudflareAPIToken == "" {
|
||||
|
||||
+13
-2
@@ -3,6 +3,7 @@ package api
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"sync"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
|
||||
@@ -36,7 +37,8 @@ type Server struct {
|
||||
staleScanner *stale.Scanner
|
||||
proxyManager *proxy.Manager
|
||||
|
||||
dnsProvider dns.Provider
|
||||
dnsProviderMu sync.RWMutex
|
||||
dnsProvider dns.Provider
|
||||
onDNSProviderChanged DNSProviderChangedFunc
|
||||
}
|
||||
|
||||
@@ -86,9 +88,18 @@ func (s *Server) SetProxyManager(pm *proxy.Manager) {
|
||||
|
||||
// SetDNSProvider sets the current DNS provider on the server.
|
||||
func (s *Server) SetDNSProvider(provider dns.Provider) {
|
||||
s.dnsProviderMu.Lock()
|
||||
defer s.dnsProviderMu.Unlock()
|
||||
s.dnsProvider = provider
|
||||
}
|
||||
|
||||
// getDNSProviderLocked returns the current DNS provider under read lock.
|
||||
func (s *Server) getDNSProviderLocked() dns.Provider {
|
||||
s.dnsProviderMu.RLock()
|
||||
defer s.dnsProviderMu.RUnlock()
|
||||
return s.dnsProvider
|
||||
}
|
||||
|
||||
// SetDNSProviderChangedCallback sets the callback for when DNS settings change.
|
||||
func (s *Server) SetDNSProviderChangedCallback(fn DNSProviderChangedFunc) {
|
||||
s.onDNSProviderChanged = fn
|
||||
@@ -272,7 +283,7 @@ func (s *Server) Router() chi.Router {
|
||||
|
||||
// DNS management endpoints.
|
||||
r.Post("/settings/dns/test", s.testDNSConnection)
|
||||
r.Get("/settings/dns/zones", s.listDNSZones)
|
||||
r.Post("/settings/dns/zones", s.listDNSZones)
|
||||
r.Get("/dns/records", s.listDNSRecords)
|
||||
r.Post("/dns/sync", s.syncDNSRecords)
|
||||
r.Delete("/dns/records/{fqdn}", s.deleteDNSRecord)
|
||||
|
||||
@@ -176,7 +176,8 @@ func (s *Server) updateSettings(w http.ResponseWriter, r *http.Request) {
|
||||
existing.CloudflareZoneID != updated.CloudflareZoneID ||
|
||||
(req.CloudflareAPIToken != "" && req.CloudflareAPIToken != "unchanged")
|
||||
if dnsChanged {
|
||||
go s.handleDNSSettingsChange(existing, updated)
|
||||
oldProvider := s.getDNSProviderLocked()
|
||||
go s.handleDNSSettingsChange(oldProvider, existing, updated)
|
||||
}
|
||||
|
||||
respondJSON(w, http.StatusOK, map[string]string{"status": "updated"})
|
||||
@@ -374,17 +375,17 @@ func (s *Server) reapplySSLToAllProxies(settings store.Settings) {
|
||||
// handleDNSSettingsChange reacts to DNS configuration changes:
|
||||
// - If switching to wildcard mode: remove all managed DNS records from the provider.
|
||||
// - If switching provider or credentials: remove old records, create new provider, re-sync.
|
||||
func (s *Server) handleDNSSettingsChange(oldSettings, newSettings store.Settings) {
|
||||
func (s *Server) handleDNSSettingsChange(oldProvider dns.Provider, oldSettings, newSettings store.Settings) {
|
||||
ctx := context.Background()
|
||||
|
||||
// Step 1: If there was an old provider, remove all managed DNS records from it.
|
||||
if !oldSettings.WildcardDNS && oldSettings.DNSProvider != "" && s.dnsProvider != nil {
|
||||
if !oldSettings.WildcardDNS && oldSettings.DNSProvider != "" && oldProvider != nil {
|
||||
records, err := s.store.ListDNSRecords()
|
||||
if err != nil {
|
||||
slog.Error("dns settings change: list records for cleanup", "error", err)
|
||||
} else {
|
||||
for _, rec := range records {
|
||||
if err := s.dnsProvider.DeleteRecord(ctx, rec.FQDN); err != nil {
|
||||
if err := oldProvider.DeleteRecord(ctx, rec.FQDN); err != nil {
|
||||
slog.Warn("dns settings change: delete old record", "fqdn", rec.FQDN, "error", err)
|
||||
}
|
||||
if err := s.store.DeleteDNSRecord(rec.FQDN); err != nil {
|
||||
@@ -420,7 +421,7 @@ func (s *Server) handleDNSSettingsChange(oldSettings, newSettings store.Settings
|
||||
}
|
||||
|
||||
// Step 3: Update the server's DNS provider and notify dependents.
|
||||
s.dnsProvider = newProvider
|
||||
s.SetDNSProvider(newProvider)
|
||||
if s.onDNSProviderChanged != nil {
|
||||
s.onDNSProviderChanged(newProvider)
|
||||
}
|
||||
@@ -488,10 +489,19 @@ func (s *Server) testDNSConnection(w http.ResponseWriter, r *http.Request) {
|
||||
})
|
||||
}
|
||||
|
||||
// listDNSZones handles GET /api/settings/dns/zones.
|
||||
// dnsZonesRequest is the expected JSON body for listing DNS zones.
|
||||
type dnsZonesRequest struct {
|
||||
Token string `json:"token"`
|
||||
}
|
||||
|
||||
// listDNSZones handles POST /api/settings/dns/zones.
|
||||
func (s *Server) listDNSZones(w http.ResponseWriter, r *http.Request) {
|
||||
token := r.URL.Query().Get("token")
|
||||
// If no token in query, use stored one.
|
||||
var req dnsZonesRequest
|
||||
if !decodeJSON(w, r, &req) {
|
||||
return
|
||||
}
|
||||
token := req.Token
|
||||
// If no token in body, use stored one.
|
||||
if token == "" {
|
||||
settings, err := s.store.GetSettings()
|
||||
if err != nil {
|
||||
|
||||
Reference in New Issue
Block a user