From 1f81ca9eb06bc4f239d8c0d03758114603c9506f Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Sat, 28 Mar 2026 00:14:53 +0300 Subject: [PATCH] fix(docker-watcher): address final review findings Security: - Move config export behind auth middleware - Validate OIDC callback token before storing in localStorage - Use constant-time comparison for webhook secret - Encrypt OIDC client secret at rest (like registry tokens) Performance: - Make TriggerDeploy async from HTTP handlers (return deploy ID immediately, run pipeline in background goroutine) Robustness: - Wrap api.ts res.json() in try/catch for non-JSON responses i18n: - Replace ~20 hardcoded English validation messages with $t() calls - Localize ConfirmDialog cancel button, InstanceCard confirm titles, ProjectCard instance/instances pluralization - Add validation keys to both en.json and ru.json --- internal/api/auth.go | 14 +++++- internal/api/deploys.go | 12 +++-- internal/api/instances.go | 5 +- internal/api/router.go | 17 +++++-- internal/deployer/deployer.go | 50 ++++++++++++++++++- internal/webhook/handler.go | 3 +- web/src/lib/api.ts | 10 +++- web/src/lib/components/ConfirmDialog.svelte | 3 +- web/src/lib/components/InstanceCard.svelte | 3 +- web/src/lib/components/ProjectCard.svelte | 2 +- web/src/lib/i18n/en.json | 22 +++++++- web/src/lib/i18n/ru.json | 22 +++++++- web/src/routes/deploy/+page.svelte | 12 ++--- web/src/routes/login/+page.svelte | 21 ++++++-- web/src/routes/settings/+page.svelte | 10 ++-- .../routes/settings/credentials/+page.svelte | 6 +-- .../routes/settings/registries/+page.svelte | 6 +-- 17 files changed, 178 insertions(+), 40 deletions(-) diff --git a/internal/api/auth.go b/internal/api/auth.go index 2256e35..a099cc8 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -10,6 +10,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/alexei/docker-watcher/internal/auth" + "github.com/alexei/docker-watcher/internal/crypto" "github.com/alexei/docker-watcher/internal/store" ) @@ -207,12 +208,23 @@ func (s *Server) updateAuthSettings(w http.ResponseWriter, r *http.Request) { return } - // If client secret is masked, preserve the existing value. + // If client secret is masked, preserve the existing encrypted value. if req.OIDCClientSecret == "********" || req.OIDCClientSecret == "" { existing, err := s.store.GetAuthSettings() if err == nil { req.OIDCClientSecret = existing.OIDCClientSecret } + } else { + // Encrypt the new client secret before storage. + encrypted, err := crypto.Encrypt(s.encKey, req.OIDCClientSecret) + if err != nil { + respondError(w, http.StatusInternalServerError, "failed to encrypt OIDC client secret") + return + } + // Keep plaintext for OIDC init below, store encrypted. + plaintextSecret := req.OIDCClientSecret + req.OIDCClientSecret = encrypted + defer func() { req.OIDCClientSecret = plaintextSecret }() } if err := s.store.UpdateAuthSettings(req); err != nil { diff --git a/internal/api/deploys.go b/internal/api/deploys.go index a689a71..46c2b82 100644 --- a/internal/api/deploys.go +++ b/internal/api/deploys.go @@ -137,16 +137,18 @@ func (s *Server) quickDeploy(w http.ResponseWriter, r *http.Request) { return } - // Trigger deploy. - if err := s.deployer.TriggerDeploy(r.Context(), project.ID, stage.ID, req.Tag); err != nil { + // Trigger deploy asynchronously. + deployID, err := s.deployer.AsyncTriggerDeploy(r.Context(), project.ID, stage.ID, req.Tag) + if err != nil { respondError(w, http.StatusInternalServerError, "failed to trigger deploy: "+err.Error()) return } respondJSON(w, http.StatusAccepted, map[string]any{ - "project": project, - "stage": stage, - "tag": req.Tag, + "project": project, + "stage": stage, + "tag": req.Tag, + "deploy_id": deployID, "status": "deploying", }) } diff --git a/internal/api/instances.go b/internal/api/instances.go index 4dca489..81a3d9b 100644 --- a/internal/api/instances.go +++ b/internal/api/instances.go @@ -74,12 +74,14 @@ func (s *Server) deployInstance(w http.ResponseWriter, r *http.Request) { return } - if err := s.deployer.TriggerDeploy(r.Context(), projectID, stageID, req.ImageTag); err != nil { + deployID, err := s.deployer.AsyncTriggerDeploy(r.Context(), projectID, stageID, req.ImageTag) + if err != nil { respondError(w, http.StatusInternalServerError, "failed to trigger deploy: "+err.Error()) return } respondJSON(w, http.StatusAccepted, map[string]string{ "status": "deploying", + "deploy_id": deployID, "project_id": projectID, "stage_id": stageID, "image_tag": req.ImageTag, @@ -188,4 +190,5 @@ func (s *Server) controlInstance(w http.ResponseWriter, r *http.Request, action // DeployTriggerer is the interface for triggering deployments. type DeployTriggerer interface { TriggerDeploy(ctx context.Context, projectID, stageID, imageTag string) error + AsyncTriggerDeploy(ctx context.Context, projectID, stageID, imageTag string) (string, error) } diff --git a/internal/api/router.go b/internal/api/router.go index f532386..7c3c158 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -7,6 +7,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/alexei/docker-watcher/internal/auth" + "github.com/alexei/docker-watcher/internal/crypto" "github.com/alexei/docker-watcher/internal/docker" "github.com/alexei/docker-watcher/internal/events" "github.com/alexei/docker-watcher/internal/store" @@ -57,10 +58,18 @@ func NewServer( // initOIDCProvider creates an OIDC provider from settings. Errors are logged, not fatal. func (s *Server) initOIDCProvider(ctx context.Context, as store.AuthSettings) { + // Decrypt the OIDC client secret if it's encrypted. + clientSecret := as.OIDCClientSecret + if clientSecret != "" { + if decrypted, err := crypto.Decrypt(s.encKey, clientSecret); err == nil { + clientSecret = decrypted + } + // If decrypt fails, assume it's already plaintext (migration scenario). + } provider, err := auth.NewOIDCProvider(ctx, auth.OIDCConfig{ IssuerURL: as.OIDCIssuerURL, ClientID: as.OIDCClientID, - ClientSecret: as.OIDCClientSecret, + ClientSecret: clientSecret, RedirectURL: as.OIDCRedirectURL, }) if err != nil { @@ -90,13 +99,13 @@ func (s *Server) Router() chi.Router { // Webhook handler (uses its own secret-based auth). r.Mount("/webhook", s.webhook.Route()) - // Config export (public endpoint, useful for backup). - r.Get("/config/export", s.exportConfig) - // Protected routes: require valid JWT. r.Group(func(r chi.Router) { r.Use(auth.Middleware(s.localAuth)) + // Config export (protected — reveals project/infra details). + r.Get("/config/export", s.exportConfig) + // Auth management. r.Get("/auth/me", s.currentUser) r.Get("/auth/settings", s.getAuthSettings) diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go index e88d970..d192412 100644 --- a/internal/deployer/deployer.go +++ b/internal/deployer/deployer.go @@ -72,8 +72,54 @@ func (d *Deployer) Drain() { slog.Info("deployer: all deploys drained") } -// TriggerDeploy is the main entry point for deployments. It orchestrates the full flow: -// pull image -> create container -> start -> configure proxy -> health check. +// AsyncTriggerDeploy creates a deploy record and returns the deploy ID immediately, +// then runs the full deploy pipeline in a background goroutine. Use this from HTTP handlers +// to avoid blocking the request. Progress is streamed via SSE. +func (d *Deployer) AsyncTriggerDeploy(ctx context.Context, projectID, stageID, imageTag string) (string, error) { + if d.shuttingDown.Load() { + return "", fmt.Errorf("deployer is shutting down, rejecting new deploy") + } + + // Validate inputs synchronously so the caller gets immediate feedback. + project, err := d.store.GetProjectByID(projectID) + if err != nil { + return "", fmt.Errorf("get project: %w", err) + } + stage, err := d.store.GetStageByID(stageID) + if err != nil { + return "", fmt.Errorf("get stage: %w", err) + } + if err := d.validatePromoteFrom(stage, imageTag); err != nil { + return "", fmt.Errorf("promote validation: %w", err) + } + + // Create deploy record synchronously so caller gets the ID. + deploy, err := d.store.CreateDeploy(store.Deploy{ + ProjectID: projectID, + StageID: stageID, + ImageTag: imageTag, + Status: "pending", + }) + if err != nil { + return "", fmt.Errorf("create deploy record: %w", err) + } + + // Run the actual deploy in the background. + d.activeWg.Add(1) + go func() { + defer d.activeWg.Done() + // Use a detached context so client disconnect doesn't abort the deploy. + bgCtx := context.Background() + if err := d.runDeploy(bgCtx, project, stage, deploy.ID, imageTag); err != nil { + slog.Error("async deploy failed", "deploy_id", deploy.ID, "error", err) + } + }() + + return deploy.ID, nil +} + +// TriggerDeploy is the synchronous entry point for deployments (used by poller and webhook). +// It orchestrates the full flow: pull image -> create container -> start -> configure proxy -> health check. // On failure, it rolls back (removes container, deletes proxy host, updates status). func (d *Deployer) TriggerDeploy(ctx context.Context, projectID, stageID, imageTag string) error { if d.shuttingDown.Load() { diff --git a/internal/webhook/handler.go b/internal/webhook/handler.go index 502d052..9c5628e 100644 --- a/internal/webhook/handler.go +++ b/internal/webhook/handler.go @@ -2,6 +2,7 @@ package webhook import ( "context" + "crypto/subtle" "encoding/json" "fmt" "log" @@ -144,7 +145,7 @@ func (h *Handler) handleWebhook(w http.ResponseWriter, r *http.Request) { return } - if settings.WebhookSecret == "" || settings.WebhookSecret != secret { + if settings.WebhookSecret == "" || subtle.ConstantTimeCompare([]byte(settings.WebhookSecret), []byte(secret)) != 1 { http.NotFound(w, r) return } diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 15c30f7..46791cf 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -46,7 +46,15 @@ async function request(path: string, init?: RequestInit): Promise { headers }); - const envelope: ApiEnvelope = await res.json(); + let envelope: ApiEnvelope; + try { + envelope = await res.json(); + } catch { + throw new ApiError( + `Server returned non-JSON response (HTTP ${res.status})`, + res.status + ); + } if (!envelope.success) { throw new ApiError(envelope.error ?? 'Unknown API error', res.status); diff --git a/web/src/lib/components/ConfirmDialog.svelte b/web/src/lib/components/ConfirmDialog.svelte index a1c3ef2..469b95c 100644 --- a/web/src/lib/components/ConfirmDialog.svelte +++ b/web/src/lib/components/ConfirmDialog.svelte @@ -3,6 +3,7 @@ -->