From e3834cf0d6f334ec8ca581a633aa6a45ceabd3cc Mon Sep 17 00:00:00 2001 From: Indra Gunawan Date: Fri, 16 Oct 2026 07:45:55 +0700 Subject: [PATCH 1/2] fix: keep stored secrets out of form HTML (A20) Settings loads the webhook secret from the API on Show/Copy. The app edit form gets an empty basic-auth password (empty keeps it, empty username removes auth) and secret env values masked (empty + original key); the handler keeps the stored value unless it is changed, also when the row is renamed or made plain. JSON API unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/app/handler.go | 85 +++++++++++-------- internal/http/secrets_form_test.go | 115 ++++++++++++++++++++++++++ internal/http/templates/apps_add.html | 25 +++++- internal/http/templates/settings.html | 22 +++-- internal/settings/handler.go | 7 +- 5 files changed, 207 insertions(+), 47 deletions(-) create mode 100644 internal/http/secrets_form_test.go 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/http/secrets_form_test.go b/internal/http/secrets_form_test.go new file mode 100644 index 0000000..1b3e909 --- /dev/null +++ b/internal/http/secrets_form_test.go @@ -0,0 +1,115 @@ +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) + } + + 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 @@

Basic auth

Optional. Protects the app with a username and password.

- +
@@ -238,7 +238,8 @@

Basic auth

if (Array.isArray(initialEnvVars)) { initialEnvVars.forEach(function(ev) { var isSecret = ev.isSecret || false; - pendingEnvVars.push({key: ev.key || '', value: ev.value || '', isSecret: isSecret}); + // masked: a stored secret sent without its value; the server keeps it unless the value is changed. + pendingEnvVars.push({key: ev.key || '', value: ev.value || '', isSecret: isSecret, masked: !!ev.masked, orig: ev.orig || ''}); }); } {{end}} @@ -279,7 +280,7 @@

Basic auth

div.className = 'env-row row-grid row-env'; var typeLabel = ev.isSecret ? 'Secret' : 'Plain'; div.innerHTML = '' - + '' + + '' + '' + ''; list.appendChild(div); @@ -294,6 +295,7 @@

Basic auth

function updateEnvVal(index, val) { pendingEnvVars[index].value = val; + pendingEnvVars[index].masked = false; syncEnvHiddenFields(); } @@ -313,9 +315,14 @@

Basic auth

t.type = 'hidden'; t.name = 'env_type'; t.value = ev.isSecret ? 'secret' : 'plain'; + var o = document.createElement('input'); + o.type = 'hidden'; + o.name = 'env_orig'; + o.value = ev.masked ? ev.orig : ''; container.appendChild(k); container.appendChild(v); container.appendChild(t); + container.appendChild(o); }); } @@ -348,10 +355,20 @@

Basic auth

} function openExportModal() { + var appId = '{{if .IsEdit}}{{.App.ID}}{{end}}'; + if (!appId || !pendingEnvVars.some(function(ev){ return ev.masked; })) return showExport({}); + // Masked secrets are fetched only when the user asks for them. + fetch('api/apps/' + appId + '/secrets') + .then(function(r){ if (!r.ok) throw new Error(r.status); return r.json(); }) + .then(function(list){ var m = {}; (list || []).forEach(function(s){ m[s.key] = s.value; }); showExport(m); }) + .catch(function(){ showToast('Could not load secret values', true); }); +} + +function showExport(stored) { var lines = []; pendingEnvVars.forEach(function(ev) { if (ev.key) { - lines.push(ev.key + '=' + ev.value); + lines.push(ev.key + '=' + (ev.masked ? (stored[ev.orig] || '') : ev.value)); } }); if (lines.length === 0) { diff --git a/internal/http/templates/settings.html b/internal/http/templates/settings.html index 4a0abb7..262b3ab 100644 --- a/internal/http/templates/settings.html +++ b/internal/http/templates/settings.html @@ -35,7 +35,7 @@

Appearance

Webhook secret

Use this secret in GitHub Actions to authenticate webhook deploy notifications.

- + @@ -70,14 +70,26 @@

Import