fix(actions): address review comments on deployment environments

- MatchesBranch: return false on invalid glob pattern instead of skipping
- secret.go: log real DB errors when fetching environment, silence only ErrNotExist
- variable.go: propagate real DB errors instead of nolint:nilerr suppression
- UpdateEnvironment: return 409 when renaming to an already-existing name
- web router: wrap environment name with url.PathEscape in all redirects

Author: alexosumi

models/actions/environment.go

@@ -1,4 +1,4 @@
-// Copyright 2024 The Gitea Authors. All rights reserved.
+// Copyright 2026 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package actions
@@ -155,7 +155,7 @@ func (env *ActionEnvironment) MatchesBranch(ref string) bool {
 		}
 		g, err := glob.Compile(pattern)
 		if err != nil {
-			continue
+			return false
 		}
 		ok := g.Match(branch)
 		if ok {

models/actions/variable.go

@@ -5,6 +5,8 @@ package actions
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"strings"
 	"unicode/utf8"
 
@@ -187,8 +189,10 @@ func GetVariablesOfJob(ctx context.Context, job *ActionRunJob) (map[string]strin
 
 	env, err := GetEnvironmentByRepoAndName(ctx, job.RepoID, job.EnvironmentName)
 	if err != nil {
-		// environment may not exist (was deleted after job creation); fall back to base vars
-		return variables, nil //nolint:nilerr // environment may have been deleted after job creation; fall back to base vars
+		if !errors.Is(err, util.ErrNotExist) {
+			return nil, fmt.Errorf("get environment %q for job %d: %w", job.EnvironmentName, job.ID, err)
+		}
+		return variables, nil
 	}
 	if !env.MatchesBranch(job.Run.Ref) {
 		return variables, nil

models/secret/secret.go

@@ -5,6 +5,7 @@ package secret
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -189,7 +190,11 @@ func GetSecretsOfTask(ctx context.Context, task *actions_model.ActionTask) (map[
 	// Environment-scoped secrets override repo/org secrets when the job targets a deployment environment.
 	if task.Job.EnvironmentName != "" {
 		env, err := actions_model.GetEnvironmentByRepoAndName(ctx, task.Job.Run.RepoID, task.Job.EnvironmentName)
-		if err == nil && env.MatchesBranch(task.Job.Run.Ref) {
+		if err != nil {
+			if !errors.Is(err, util.ErrNotExist) {
+				log.Error("get environment %q for task %d: %v", task.Job.EnvironmentName, task.ID, err)
+			}
+		} else if env.MatchesBranch(task.Job.Run.Ref) {
 			envSecrets, err := db.Find[actions_model.ActionEnvironmentSecret](ctx, actions_model.FindEnvSecretsOptions{
 				RepoID:        task.Job.Run.RepoID,
 				EnvironmentID: env.ID,

routers/api/v1/repo/environment.go

@@ -179,7 +179,11 @@ func UpdateEnvironment(ctx *context.APIContext) {
 	opt := web.GetForm(ctx).(*api.UpdateEnvironmentOption)
 	updated, err := actions_service.UpdateEnvironment(ctx, ctx.Repo.Repository.ID, env.ID, opt.Name, opt.ProtectedBranches)
 	if err != nil {
-		ctx.APIErrorInternal(err)
+		if errors.Is(err, util.ErrAlreadyExist) {
+			ctx.APIError(http.StatusConflict, err.Error())
+		} else {
+			ctx.APIErrorInternal(err)
+		}
 		return
 	}
 	ctx.JSON(http.StatusOK, toAPIEnvironment(updated))

routers/web/repo/setting/environment.go

@@ -6,6 +6,7 @@ package setting
 import (
 	"errors"
 	"net/http"
+	"net/url"
 	"strings"
 
 	actions_model "gitea.dev/models/actions"
@@ -64,7 +65,7 @@ func EnvironmentCreate(ctx *context.Context) {
 	}
 
 	ctx.Flash.Success(ctx.Tr("environments.creation.success", name))
-	ctx.Redirect(ctx.Repo.RepoLink + "/settings/environments/" + name)
+	ctx.Redirect(ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(name))
 }
 
 // EnvironmentEdit renders the environment edit page (secrets + variables)
@@ -101,7 +102,7 @@ func EnvironmentEdit(ctx *context.Context) {
 	ctx.Data["Variables"] = variables
 	ctx.Data["DataMaxLength"] = secret_model.SecretDataMaxLength
 	ctx.Data["DescriptionMaxLength"] = secret_model.SecretDescriptionMaxLength
-	ctx.Data["Link"] = ctx.Repo.RepoLink + "/settings/environments/" + envName
+	ctx.Data["Link"] = ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName)
 	ctx.HTML(http.StatusOK, tplEnvironmentEdit)
 }
 
@@ -121,7 +122,7 @@ func EnvironmentUpdate(ctx *context.Context) {
 	} else {
 		ctx.Flash.Success(ctx.Tr("environments.update.success"))
 	}
-	ctx.Redirect(ctx.Repo.RepoLink + "/settings/environments/" + envName)
+	ctx.Redirect(ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName))
 }
 
 // EnvironmentDelete handles POST to delete an environment
@@ -151,7 +152,7 @@ func EnvironmentSecretPost(ctx *context.Context) {
 		return
 	}
 
-	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + envName
+	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName)
 	form := web.GetForm(ctx).(*forms.AddSecretForm)
 
 	if err := secret_service.ValidateName(form.Name); err != nil {
@@ -195,7 +196,7 @@ func EnvironmentSecretDelete(ctx *context.Context) {
 	}
 
 	ctx.Flash.Success(ctx.Tr("secrets.deletion.success"))
-	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/environments/" + envName)
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName))
 }
 
 // EnvironmentVariableCreate handles POST for creating an environment variable
@@ -207,7 +208,7 @@ func EnvironmentVariableCreate(ctx *context.Context) {
 		return
 	}
 
-	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + envName
+	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName)
 	form := web.GetForm(ctx).(*forms.EditVariableForm)
 
 	_, err = actions_service.CreateEnvVariable(ctx, ctx.Repo.Repository.ID, env.ID, form.Name, form.Data, form.Description)
@@ -228,7 +229,7 @@ func EnvironmentVariableUpdate(ctx *context.Context) {
 		return
 	}
 
-	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + envName
+	redirectURL := ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName)
 	variableID := ctx.PathParamInt64("variable_id")
 	form := web.GetForm(ctx).(*forms.EditVariableForm)
 
@@ -257,5 +258,5 @@ func EnvironmentVariableDelete(ctx *context.Context) {
 	}
 
 	ctx.Flash.Success(ctx.Tr("actions.variables.deletion.success"))
-	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/environments/" + envName)
+	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/environments/" + url.PathEscape(envName))
 }

services/actions/environment.go

@@ -35,7 +35,10 @@ func UpdateEnvironment(ctx context.Context, repoID, envID int64, name, protected
 	if env.RepoID != repoID {
 		return nil, util.ErrNotExist
 	}
-	if name != "" {
+	if name != "" && name != env.Name {
+		if existing, err := actions_model.GetEnvironmentByRepoAndName(ctx, repoID, name); err == nil && existing != nil {
+			return nil, actions_model.ErrEnvironmentAlreadyExists{Name: name}
+		}
 		env.Name = name
 	}
 	env.ProtectedBranches = protectedBranches