diff --git a/internal/app/handler.go b/internal/app/handler.go index e5c2543..542c15e 100644 --- a/internal/app/handler.go +++ b/internal/app/handler.go @@ -739,21 +739,7 @@ func (h *WebHandler) AppAddForm(w http.ResponseWriter, r *http.Request, render R app.Status = StatusDraft } - envVals := r.Form["env_val"] - envTypes := r.Form["env_type"] - envList := make([]map[string]interface{}, 0, len(envKeysRaw)) - for i, k := range envKeysRaw { - k = strings.TrimSpace(k) - if k == "" || i >= len(envVals) { - continue - } - envList = append(envList, map[string]interface{}{ - "key": k, - "value": envVals[i], - "isSecret": i < len(envTypes) && envTypes[i] == "secret", - }) - } - envJSON, _ := json.Marshal(envList) + envJSON, _ := json.Marshal(formEnvVars(r)) formCtx := func(servers interface{}, errMsg string) map[string]interface{} { return map[string]interface{}{ @@ -856,13 +842,21 @@ func (h *WebHandler) AppEditPage(w http.ResponseWriter, r *http.Request, render } } - secretsJSON, _ := json.Marshal(secrets) + // Secret values stay on the server: the form gets them masked (empty, orig = key) and keeps them unless changed. + envList := make([]formEnvVar, 0, len(secrets)) + for _, s := range secrets { + ev := formEnvVar{Key: s.Key, Value: s.Value, IsSecret: s.IsSecret} + if s.IsSecret { + ev.Value, ev.Masked, ev.Orig = "", true, s.Key + } + envList = append(envList, ev) + } + secretsJSON, _ := json.Marshal(envList) render(w, r, http.StatusOK, "apps_add.html", map[string]interface{}{ "Title": "Edit " + app.Name, "Servers": servers, "App": app, - "Secrets": secrets, "SecretsJSON": template.JS(secretsJSON), "Files": files, "IsEdit": true, @@ -957,7 +951,12 @@ func (h *WebHandler) AppEditForm(w http.ResponseWriter, r *http.Request, render app.GitRepo = strings.TrimSpace(r.FormValue("git_repo")) app.GitBranch = strings.TrimSpace(r.FormValue("git_branch")) app.AuthUser = strings.TrimSpace(r.FormValue("auth_user")) - app.AuthPass = strings.TrimSpace(r.FormValue("auth_pass")) + // The form never contains the stored password: empty keeps it, an empty username removes basic auth. + if p := strings.TrimSpace(r.FormValue("auth_pass")); p != "" { + app.AuthPass = p + } else if app.AuthUser == "" { + app.AuthPass = "" + } app.MemoryLimit = memoryLimit app.CPULimit = cpuLimit app.LogMaxSize = logMaxSize @@ -966,21 +965,7 @@ func (h *WebHandler) AppEditForm(w http.ResponseWriter, r *http.Request, render app.Ports = ports app.UlimitsNofile = ulimitsNofile - envVals := r.Form["env_val"] - envTypes := r.Form["env_type"] - envList := make([]map[string]interface{}, 0, len(envKeysRaw)) - for i, k := range envKeysRaw { - k = strings.TrimSpace(k) - if k == "" || i >= len(envVals) { - continue - } - envList = append(envList, map[string]interface{}{ - "key": k, - "value": envVals[i], - "isSecret": i < len(envTypes) && envTypes[i] == "secret", - }) - } - envJSON, _ := json.Marshal(envList) + envJSON, _ := json.Marshal(formEnvVars(r)) editCtx := func(servers interface{}, errMsg string) map[string]interface{} { return map[string]interface{}{ @@ -1291,6 +1276,34 @@ func (h *WebHandler) AppRedeployAll(w http.ResponseWriter, r *http.Request, rend web.Redirect(w, r, "/apps?flash="+url.QueryEscape(fmt.Sprintf("Redeploying %d running apps...", count)), http.StatusSeeOther) } +// formEnvVar is one row of the env editor in apps_add.html. Masked: a stored secret sent without its +// value; Orig is the key it was stored under. +type formEnvVar struct { + Key string `json:"key"` + Value string `json:"value"` + IsSecret bool `json:"isSecret"` + Masked bool `json:"masked,omitempty"` + Orig string `json:"orig,omitempty"` +} + +// formEnvVars rebuilds the env rows from a submitted form (re-render after a validation error). +func formEnvVars(r *http.Request) []formEnvVar { + keys, vals, types, origs := r.Form["env_key"], r.Form["env_val"], r.Form["env_type"], r.Form["env_orig"] + list := make([]formEnvVar, 0, len(keys)) + for i, k := range keys { + k = strings.TrimSpace(k) + if k == "" || i >= len(vals) { + continue + } + ev := formEnvVar{Key: k, Value: vals[i], IsSecret: i < len(types) && types[i] == "secret"} + if ev.Value == "" && i < len(origs) && origs[i] != "" { + ev.Masked, ev.Orig = true, origs[i] + } + list = append(list, ev) + } + return list +} + func saveFormEnvVars(r *http.Request, svc *Service, appID int64) { existing, _ := svc.ListSecrets(appID) existingMap := make(map[string]AppSecret) @@ -1301,6 +1314,7 @@ func saveFormEnvVars(r *http.Request, svc *Service, appID int64) { keys := r.Form["env_key"] vals := r.Form["env_val"] types := r.Form["env_type"] + origs := r.Form["env_orig"] submitted := make(map[string]bool) for i, k := range keys { @@ -1311,6 +1325,11 @@ func saveFormEnvVars(r *http.Request, svc *Service, appID int64) { submitted[k] = true v := vals[i] isSecret := i < len(types) && types[i] == "secret" + if v == "" && i < len(origs) && origs[i] != "" { + if ev, ok := existingMap[origs[i]]; ok && ev.IsSecret { + v = ev.Value // masked row left unchanged (maybe renamed or made plain) + } + } if isSecret && v == "" { continue diff --git a/internal/app/validate.go b/internal/app/validate.go index 8a62b09..56c2293 100644 --- a/internal/app/validate.go +++ b/internal/app/validate.go @@ -52,11 +52,17 @@ func validateForm(r *http.Request, domains []string) error { return err } } + seen := map[string]bool{} for _, k := range r.Form["env_key"] { if k = strings.TrimSpace(k); k != "" { if err := ValidEnvKey(k); err != nil { return err } + // Two rows with one key would overwrite each other (and a masked secret silently). + if seen[k] { + return fmt.Errorf("environment variable %s is listed twice", k) + } + seen[k] = true } } for _, p := range r.Form["file_path"] { diff --git a/internal/http/secrets_form_test.go b/internal/http/secrets_form_test.go new file mode 100644 index 0000000..5346bb5 --- /dev/null +++ b/internal/http/secrets_form_test.go @@ -0,0 +1,124 @@ +package http + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/coderbuzz/dockify/internal/app" + "github.com/coderbuzz/dockify/internal/db" + "github.com/coderbuzz/dockify/internal/server" + "github.com/coderbuzz/dockify/internal/settings" +) + +type noServers struct{} + +func (noServers) List() ([]app.ServerInfo, error) { return nil, nil } + +// Secrets are never rendered into HTML; the edit form keeps stored values it did not change (A20). +func TestSecretsNotInFormHTML(t *testing.T) { + database, err := db.Open(":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + srvRepo := server.NewRepository(database) + appSvc := app.NewService(app.NewRepository(database), srvRepo, nil, nil) + setSvc := settings.NewService(database, "test") + r := NewRouter(server.NewService(srvRepo), appSvc, Render, noServers{}, "admin", "", "", t.TempDir(), func() string { return "" }, settings.NewHandler(setSvc), nil, "test", true, false) + do := func(method, path string, form url.Values) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader(form.Encode())) + if form != nil { + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + } + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + return rec + } + + whSecret, _ := setSvc.GetWebhookSecret() + if rec := do(http.MethodGet, "/settings", nil); rec.Code != http.StatusOK || strings.Contains(rec.Body.String(), whSecret) { + t.Errorf("/settings: %d, contains webhook secret %v", rec.Code, strings.Contains(rec.Body.String(), whSecret)) + } + if rec := do(http.MethodGet, "/api/settings/webhook-secret", nil); !strings.Contains(rec.Body.String(), whSecret) { + t.Errorf("API must still return the secret: %d %s", rec.Code, rec.Body) + } + + a := &app.App{Name: "web", Compose: "services: {}", Status: app.StatusDraft, AuthUser: "u", AuthPass: "old-pass-123"} + if err := appSvc.Create(a); err != nil { + t.Fatal(err) + } + for _, s := range []struct { + k, v string + secret bool + }{{"API_KEY", "secret-a-123", true}, {"TOKEN", "secret-c-123", true}, {"DROP", "secret-d-123", true}, {"PLAIN", "plain-b", false}} { + if err := appSvc.SetSecretWithType(a.ID, s.k, s.v, s.secret); err != nil { + t.Fatal(err) + } + } + editPath := "/apps/" + strconv.FormatInt(a.ID, 10) + "/edit" + page := do(http.MethodGet, editPath, nil).Body.String() + for _, v := range []string{"old-pass-123", "secret-a-123", "secret-c-123", "secret-d-123"} { + if strings.Contains(page, v) { + t.Errorf("edit page contains %q", v) + } + } + if !strings.Contains(page, "plain-b") || !strings.Contains(page, `placeholder="unchanged"`) { + t.Errorf("edit page: plain value or unchanged placeholder missing") + } + + // Unchanged password and masked rows: API_KEY renamed, TOKEN made plain, DROP removed, PLAIN kept. + form := url.Values{ + "name": {"web"}, "mode": {"advanced"}, "compose": {"services: {}"}, "draft": {"1"}, + "auth_user": {"u"}, "auth_pass": {""}, + "env_key": {"API_KEY2", "TOKEN", "PLAIN"}, + "env_val": {"", "", "plain-b"}, + "env_type": {"secret", "plain", "plain"}, + "env_orig": {"API_KEY", "TOKEN", ""}, + } + if rec := do(http.MethodPost, editPath, form); rec.Code != http.StatusSeeOther { + t.Fatalf("edit: %d %s", rec.Code, rec.Body) + } + got, _ := appSvc.Get(a.ID) + if got.AuthPass != "old-pass-123" { + t.Errorf("AuthPass = %q, want the old one", got.AuthPass) + } + list, _ := appSvc.ListSecrets(a.ID) + m := map[string]app.AppSecret{} + for _, s := range list { + m[s.Key] = s + } + if s := m["API_KEY2"]; s.Value != "secret-a-123" || !s.IsSecret { + t.Errorf("renamed masked secret: %+v", s) + } + if s := m["TOKEN"]; s.Value != "secret-c-123" || s.IsSecret { + t.Errorf("masked secret made plain: %+v", s) + } + if _, ok := m["API_KEY"]; ok || len(m) != 3 { + t.Errorf("secrets after edit: %+v", m) + } + + dup := url.Values{} + for k, v := range form { + dup[k] = v + } + dup["env_key"] = []string{"PLAIN", "TOKEN", "PLAIN"} + if rec := do(http.MethodPost, editPath, dup); rec.Code != http.StatusBadRequest { + t.Errorf("duplicate env key: %d, want 400", rec.Code) + } + + form.Set("auth_pass", "new-pass") + do(http.MethodPost, editPath, form) + if got, _ = appSvc.Get(a.ID); got.AuthPass != "new-pass" { + t.Errorf("AuthPass = %q, want new-pass", got.AuthPass) + } + form.Set("auth_user", "") + form.Set("auth_pass", "") + do(http.MethodPost, editPath, form) + if got, _ = appSvc.Get(a.ID); got.AuthPass != "" || got.AuthUser != "" { + t.Errorf("empty username must remove basic auth: %q/%q", got.AuthUser, got.AuthPass) + } +} diff --git a/internal/http/templates/apps_add.html b/internal/http/templates/apps_add.html index 58c7484..4d6b343 100644 --- a/internal/http/templates/apps_add.html +++ b/internal/http/templates/apps_add.html @@ -167,7 +167,7 @@
Optional. Protects the app with a username and password.
Use this secret in GitHub Actions to authenticate webhook deploy notifications.