diff --git a/models/secret/TestSecretGetSecretByID/secret.yml b/models/secret/TestSecretGetSecretByID/secret.yml new file mode 100644 index 0000000000..08e3e2c561 --- /dev/null +++ b/models/secret/TestSecretGetSecretByID/secret.yml @@ -0,0 +1,23 @@ +- id: 637340 + name: TEST_SECRET + owner_id: 3 + repo_id: 0 + # very secret + data: 0x0e17d357077de0c1f72e9d87e1899c8026a207fcbbc0f1a2a79d0cb305da39fa3e9fe201b085e963fa1f9eeb59b4ff9ee6d748 + created_unix: 1773692671 + +- id: 637341 + name: ANOTHER_SECRET + owner_id: 0 + repo_id: 62 # user2/test_workflows + # also very secret + data: 0xb697cd4a66b02bc36d0afddcb6f158d7602f6cf0b0d31a0c96e178469e8b66babccb30b95e01c84824add04c5bfe91b9b773a21a78088a3e + created_unix: 1773692672 + +- id: 637342 + name: TEST_SECRET + owner_id: 1 + repo_id: 0 + # super secret + data: 0xd97d8cb662c07ca94953a388bc93209f713281a8b0d25499359cbd03a1fbe565a2a0ba183b8290fc110ee6b1c6437c569451e0c3 + created_unix: 1773692673 diff --git a/models/secret/secret.go b/models/secret/secret.go index de1a8d84a4..911d94d78a 100644 --- a/models/secret/secret.go +++ b/models/secret/secret.go @@ -6,6 +6,7 @@ package secret import ( "context" "fmt" + "regexp" "strings" "forgejo.org/models/db" @@ -17,6 +18,13 @@ import ( "xorm.io/builder" ) +var ( + namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$") + forbiddenPrefixPattern = regexp.MustCompile("(?i)^FORGEJO_|GITEA_|GITHUB_") + + ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name") +) + // Secret represents a secret // // It can be: @@ -63,6 +71,9 @@ func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, dat if ownerID == 0 && repoID == 0 { return nil, fmt.Errorf("%w: ownerID and repoID cannot be both zero, global secrets are not supported", util.ErrInvalidArgument) } + if err := ValidateName(name); err != nil { + return nil, err + } secret := &Secret{ OwnerID: ownerID, @@ -129,6 +140,46 @@ func (s *Secret) GetDecryptedData() (string, error) { return string(v), nil } +func GetSecretByID(ctx context.Context, ownerID, repoID, id int64) (*Secret, error) { + query := db.GetEngine(ctx).Where("id=?", id) + + if repoID > 0 { + query = query.And(builder.Eq{"repo_id": repoID}) + } else if ownerID > 0 { + query = query.And(builder.Eq{"owner_id": ownerID}) + } else { + return nil, fmt.Errorf("ownerID and repoID cannot be simultaneously 0") + } + + var secret Secret + has, err := query.Get(&secret) + + if err != nil { + return nil, err + } else if !has { + return nil, fmt.Errorf("secret with ID %d: %w", id, util.ErrNotExist) + } + return &secret, nil +} + +func UpdateSecret(ctx context.Context, secret *Secret, columns ...string) error { + e := db.GetEngine(ctx) + + if err := ValidateName(secret.Name); err != nil { + return err + } + secret.Name = strings.ToUpper(secret.Name) + + var err error + if len(columns) == 0 { + _, err = e.ID(secret.ID).AllCols().Update(secret) + } else { + _, err = e.ID(secret.ID).Cols(columns...).Update(secret) + } + + return err +} + func FetchActionSecrets(ctx context.Context, ownerID, repoID int64) (map[string]string, error) { secrets := map[string]string{} @@ -154,3 +205,10 @@ func FetchActionSecrets(ctx context.Context, ownerID, repoID int64) (map[string] return secrets, nil } + +func ValidateName(name string) error { + if !namePattern.MatchString(name) || forbiddenPrefixPattern.MatchString(name) { + return ErrInvalidName + } + return nil +} diff --git a/models/secret/secret_test.go b/models/secret/secret_test.go index e49100eb6a..5c54b25967 100644 --- a/models/secret/secret_test.go +++ b/models/secret/secret_test.go @@ -4,6 +4,7 @@ package secret import ( + "strings" "testing" "forgejo.org/models/unittest" @@ -83,6 +84,11 @@ func TestInsertEncryptedSecret(t *testing.T) { }) }) + t.Run("Rejects invalid name", func(t *testing.T) { + _, err := InsertEncryptedSecret(t.Context(), 2, 0, "invalid name", "some secret") + require.ErrorContains(t, err, "invalid secret name") + }) + t.Run("FetchActionSecrets", func(t *testing.T) { secrets, err := FetchActionSecrets(t.Context(), 2, 1) require.NoError(t, err) @@ -123,3 +129,169 @@ func TestSecretGetDecryptedData(t *testing.T) { assert.ErrorContains(t, err, "unable to decrypt secret[id=495,name=\"A_SECRET\"]") }) } + +func TestSecretGetSecretByID(t *testing.T) { + defer unittest.OverrideFixtures("models/secret/TestSecretGetSecretByID")() + require.NoError(t, unittest.PrepareTestDatabase()) + + testCases := []struct { + name string + ownerID int64 + repoID int64 + id int64 + expectedName string + expectedData string + expectedError string + }{ + { + name: "Organization secret", + ownerID: 3, + repoID: 0, + id: 637340, + expectedName: "TEST_SECRET", + expectedData: "very secret", + }, + { + name: "Owner mismatch", + ownerID: 4, + repoID: 0, + id: 637340, + expectedError: "secret with ID 637340: resource does not exist", + }, + { + name: "Repository mismatch", + ownerID: 0, + repoID: 1, + id: 637340, + expectedError: "secret with ID 637340: resource does not exist", + }, + { + name: "Repository secret", + ownerID: 0, + repoID: 62, + id: 637341, + expectedName: "ANOTHER_SECRET", + expectedData: "also very secret", + }, + { + name: "Unsupported instance secret", + ownerID: 0, + repoID: 0, + id: 637341, + expectedError: "ownerID and repoID cannot be simultaneously 0", + }, + { + name: "User secret", + ownerID: 1, + repoID: 0, + id: 637342, + expectedName: "TEST_SECRET", + expectedData: "super secret", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + secret, err := GetSecretByID(t.Context(), testCase.ownerID, testCase.repoID, testCase.id) + + if testCase.expectedError != "" { + assert.ErrorContains(t, err, testCase.expectedError) + } else { + require.NoError(t, err) + assert.Equal(t, testCase.id, secret.ID) + assert.Equal(t, testCase.ownerID, secret.OwnerID) + assert.Equal(t, testCase.repoID, secret.RepoID) + assert.Equal(t, testCase.expectedName, secret.Name) + + data, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, testCase.expectedData, data) + } + }) + } +} + +func TestSecretUpdateSecret(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + secret, err := InsertEncryptedSecret(t.Context(), 2, 0, "a_secret", "very secret") + require.NoError(t, err) + + secret.Name = "new_name" + secret.SetData("also very secret") + + err = UpdateSecret(t.Context(), secret) + require.NoError(t, err) + + updatedSecret := unittest.AssertExistsAndLoadBean(t, &Secret{ID: secret.ID}) + decryptedData, err := updatedSecret.GetDecryptedData() + require.NoError(t, err) + + assert.Equal(t, "NEW_NAME", updatedSecret.Name) + assert.Equal(t, "also very secret", decryptedData) +} + +func TestSecretUpdateSecret_RejectsInvalidName(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + secret, err := InsertEncryptedSecret(t.Context(), 2, 0, "a_secret", "very secret") + require.NoError(t, err) + + secret.Name = "GITHUB_IS_REJECTED" // Because it starts with `GITHUB_`. + secret.SetData("also very secret") + + err = UpdateSecret(t.Context(), secret) + require.ErrorContains(t, err, "invalid secret name") + + updatedSecret := unittest.AssertExistsAndLoadBean(t, &Secret{ID: secret.ID}) + decryptedData, err := updatedSecret.GetDecryptedData() + require.NoError(t, err) + + assert.Equal(t, "A_SECRET", updatedSecret.Name) + assert.Equal(t, "very secret", decryptedData) +} + +func TestSecretValidateName(t *testing.T) { + testCases := []struct { + name string + valid bool + }{ + {"FORGEJO_", false}, + {"FORGEJO_123", false}, + {"FORGEJO_ABC", false}, + {"GITEA_", false}, + {"GITEA_123", false}, + {"GITEA_ABC", false}, + {"GITHUB_", false}, + {"GITHUB_123", false}, + {"GITHUB_ABC", false}, + {"123_TEST", false}, + {"CI", true}, + {"_CI", true}, + {"CI_", true}, + {"CI123", true}, + {"CIABC", true}, + {"FORGEJO", true}, + {"FORGEJO123", true}, + {"FORGEJOABC", true}, + {"GITEA", true}, + {"GITEA123", true}, + {"GITEAABC", true}, + {"GITHUB", true}, + {"GITHUB123", true}, + {"GITHUBABC", true}, + {"_123_TEST", true}, + } + for _, tC := range testCases { + t.Run(tC.name, func(t *testing.T) { + t.Helper() + if tC.valid { + assert.NoError(t, ValidateName(tC.name)) + assert.NoError(t, ValidateName(strings.ToLower(tC.name))) + } else { + require.ErrorIs(t, ValidateName(tC.name), ErrInvalidName) + require.ErrorIs(t, ValidateName(strings.ToLower(tC.name)), ErrInvalidName) + } + }) + } +} diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 9db0fb67c6..c6b65f8c84 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -534,6 +534,12 @@ "actions.secrets.creation.value_description": "The value of a secret can be any text. Special characters are retained. CRLF (Windows-style line breaks) is automatically converted to LF. Encode the value with Base64 if linebreaks should be retained.", "actions.variables.mutation.name_description": "The name of a variable can only contain letters, numbers, and underscores. It cannot be named CI or start with FORGEJO_, GITEA_, GITHUB_, or a number. Forgejo will automatically convert it to uppercase.", "actions.variables.mutation.value_description": "A variable's value can be any text. Special characters are retained. CRLF (Windows-style line breaks) is automatically converted to LF. Encode the value with Base64 if linebreaks should be retained.", + "actions.secrets.edit_button": "Edit secret \"%s\"", + "actions.secrets.mutation.header": "Edit secret \"%s\"", + "actions.secrets.mutation.success_message": "The secret \"%s\" has been updated.", + "actions.secrets.mutation.failure_message": "The secret \"%s\" could not be updated.", + "actions.secrets.mutation.name_description": "The name of a secret can only contain letters, numbers, and underscores. It cannot start with FORGEJO_, GITEA_, GITHUB_, or a number. Forgejo will automatically convert it to uppercase.", + "actions.secrets.mutation.value_description": "The existing value will not be shown. Leave the field empty if you do not want to modify it. Special characters are retained. CRLF (Windows-style line breaks) is automatically converted to LF. Encode the value with Base64 if linebreaks should be retained.", "pulse.n_active_issues": { "one": "%s active issue", "other": "%s active issues" diff --git a/routers/web/repo/setting/secrets.go b/routers/web/repo/setting/secrets.go index 11c83e8bd6..c8aa6e3bfb 100644 --- a/routers/web/repo/setting/secrets.go +++ b/routers/web/repo/setting/secrets.go @@ -92,7 +92,7 @@ func Secrets(ctx *context.Context) { ctx.HTML(http.StatusOK, sCtx.SecretsTemplate) } -func SecretsPost(ctx *context.Context) { +func SecretsCreatePost(ctx *context.Context) { sCtx, err := getSecretsCtx(ctx) if err != nil { ctx.ServerError("getSecretsCtx", err) @@ -104,7 +104,7 @@ func SecretsPost(ctx *context.Context) { return } - shared.PerformSecretsPost( + shared.CreateSecretPost( ctx, sCtx.OwnerID, sCtx.RepoID, @@ -112,16 +112,32 @@ func SecretsPost(ctx *context.Context) { ) } -func SecretsDelete(ctx *context.Context) { +func SecretsEditPost(ctx *context.Context) { sCtx, err := getSecretsCtx(ctx) if err != nil { ctx.ServerError("getSecretsCtx", err) return } - shared.PerformSecretsDelete( + + if ctx.HasError() { + ctx.JSONError(ctx.GetErrMsg()) + return + } + + shared.EditSecretPost(ctx, sCtx.OwnerID, sCtx.RepoID, ctx.ParamsInt64(":secret_id"), sCtx.RedirectLink) +} + +func SecretsDeletePost(ctx *context.Context) { + sCtx, err := getSecretsCtx(ctx) + if err != nil { + ctx.ServerError("getSecretsCtx", err) + return + } + shared.DeleteSecretPost( ctx, sCtx.OwnerID, sCtx.RepoID, + ctx.ParamsInt64(":secret_id"), sCtx.RedirectLink, ) } diff --git a/routers/web/shared/secrets/secrets.go b/routers/web/shared/secrets/secrets.go index fe4c22bcee..ef8fea7129 100644 --- a/routers/web/shared/secrets/secrets.go +++ b/routers/web/shared/secrets/secrets.go @@ -4,6 +4,8 @@ package secrets import ( + "errors" + "forgejo.org/models/db" secret_model "forgejo.org/models/secret" "forgejo.org/modules/log" @@ -24,23 +26,50 @@ func SetSecretsContext(ctx *context.Context, ownerID, repoID int64) { ctx.Data["Secrets"] = secrets } -func PerformSecretsPost(ctx *context.Context, ownerID, repoID int64, redirectURL string) { - form := web.GetForm(ctx).(*forms.AddSecretForm) +func CreateSecretPost(ctx *context.Context, ownerID, repoID int64, redirectURL string) { + form := web.GetForm(ctx).(*forms.CreateSecretForm) - s, _, err := secrets_service.CreateOrUpdateSecret(ctx, ownerID, repoID, form.Name, util.ReserveLineBreakForTextarea(form.Data)) + normalizedData := util.ReserveLineBreakForTextarea(form.Data) + secret, err := secret_model.InsertEncryptedSecret(ctx, ownerID, repoID, form.Name, normalizedData) if err != nil { - log.Error("CreateOrUpdateSecret failed: %v", err) + log.Error("InsertEncryptedSecret failed: %v", err) ctx.JSONError(ctx.Tr("secrets.creation.failed")) return } - ctx.Flash.Success(ctx.Tr("secrets.creation.success", s.Name)) + ctx.Flash.Success(ctx.Tr("secrets.creation.success", secret.Name)) ctx.JSONRedirect(redirectURL) } -func PerformSecretsDelete(ctx *context.Context, ownerID, repoID int64, redirectURL string) { - id := ctx.FormInt64("id") +func EditSecretPost(ctx *context.Context, ownerID, repoID, id int64, redirectURL string) { + form := web.GetForm(ctx).(*forms.EditSecretForm) + secret, err := secret_model.GetSecretByID(ctx, ownerID, repoID, id) + if errors.Is(err, util.ErrNotExist) { + ctx.NotFound("GetSecretByID", err) + return + } else if err != nil { + ctx.ServerError("GetSecretByID", err) + return + } + + secret.Name = form.Name + if form.Data != "" { + secret.SetData(util.ReserveLineBreakForTextarea(form.Data)) + } + + err = secret_model.UpdateSecret(ctx, secret) + if err != nil { + log.Error("UpdateSecret failed: %v", err) + ctx.JSONError(ctx.Tr("actions.secrets.mutation.failure_message", secret.Name)) + return + } + + ctx.Flash.Success(ctx.Tr("actions.secrets.mutation.success_message", secret.Name)) + ctx.JSONRedirect(redirectURL) +} + +func DeleteSecretPost(ctx *context.Context, ownerID, repoID, id int64, redirectURL string) { err := secrets_service.DeleteSecretByID(ctx, ownerID, repoID, id) if err != nil { log.Error("DeleteSecretByID(%d) failed: %v", id, err) diff --git a/routers/web/web.go b/routers/web/web.go index 5ea90c1eea..ac5474e7ed 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -454,8 +454,9 @@ func registerRoutes(m *web.Route) { addSettingsSecretsRoutes := func() { m.Group("/secrets", func() { m.Get("", repo_setting.Secrets) - m.Post("", web.Bind(forms.AddSecretForm{}), repo_setting.SecretsPost) - m.Post("/delete", repo_setting.SecretsDelete) + m.Post("", web.Bind(forms.CreateSecretForm{}), repo_setting.SecretsCreatePost) + m.Post("/{secret_id}/edit", web.Bind(forms.EditSecretForm{}), repo_setting.SecretsEditPost) + m.Post("/{secret_id}/delete", repo_setting.SecretsDeletePost) }) } diff --git a/services/actions/variables.go b/services/actions/variables.go index 5dd68f0130..a99966737f 100644 --- a/services/actions/variables.go +++ b/services/actions/variables.go @@ -9,13 +9,13 @@ import ( "strings" actions_model "forgejo.org/models/actions" + "forgejo.org/models/secret" "forgejo.org/modules/log" "forgejo.org/modules/util" - secrets_service "forgejo.org/services/secrets" ) func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data string) (*actions_model.ActionVariable, error) { - if err := secrets_service.ValidateName(name); err != nil { + if err := secret.ValidateName(name); err != nil { return nil, err } @@ -32,7 +32,7 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data strin } func UpdateVariable(ctx context.Context, variableID, ownerID, repoID int64, name, data string) (bool, error) { - if err := secrets_service.ValidateName(name); err != nil { + if err := secret.ValidateName(name); err != nil { return false, err } diff --git a/services/forms/secret.go b/services/forms/secret.go new file mode 100644 index 0000000000..45f3082bda --- /dev/null +++ b/services/forms/secret.go @@ -0,0 +1,38 @@ +// Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forms + +import ( + "net/http" + + "forgejo.org/modules/web/middleware" + "forgejo.org/services/context" + + "code.forgejo.org/go-chi/binding" +) + +// CreateSecretForm needs to be filled in by the user to create a new secret. +type CreateSecretForm struct { + Name string `binding:"Required;MaxSize(255)"` + Data string `binding:"Required;MaxSize(65535)"` +} + +// Validate validates the submitted CreateSecretForm. +func (f *CreateSecretForm) Validate(req *http.Request, errs binding.Errors) binding.Errors { + ctx := context.GetValidateContext(req) + return middleware.Validate(errs, ctx.Data, f, ctx.Locale) +} + +// EditSecretForm needs to be filled in by the user to change an existing secret. +type EditSecretForm struct { + Name string `binding:"Required;MaxSize(255)"` + Data string `binding:"MaxSize(65535)"` +} + +// Validate validates the submitted EditSecretForm. +func (f *EditSecretForm) Validate(req *http.Request, errs binding.Errors) binding.Errors { + ctx := context.GetValidateContext(req) + return middleware.Validate(errs, ctx.Data, f, ctx.Locale) +} diff --git a/services/forms/user_form.go b/services/forms/user_form.go index 8c95139e2c..1c6ba58058 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -346,18 +346,6 @@ func (f *AddKeyForm) Validate(req *http.Request, errs binding.Errors) binding.Er return middleware.Validate(errs, ctx.Data, f, ctx.Locale) } -// AddSecretForm for adding secrets -type AddSecretForm struct { - Name string `binding:"Required;MaxSize(255)"` - Data string `binding:"Required;MaxSize(65535)"` -} - -// Validate validates the fields -func (f *AddSecretForm) Validate(req *http.Request, errs binding.Errors) binding.Errors { - ctx := context.GetValidateContext(req) - return middleware.Validate(errs, ctx.Data, f, ctx.Locale) -} - type EditVariableForm struct { Name string `binding:"Required;MaxSize(255)"` Data string `binding:"Required;MaxSize(65535)"` diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index 498f3b6fcc..a3e841b81d 100644 --- a/services/secrets/secrets.go +++ b/services/secrets/secrets.go @@ -11,7 +11,7 @@ import ( ) func CreateOrUpdateSecret(ctx context.Context, ownerID, repoID int64, name, data string) (*secret_model.Secret, bool, error) { - if err := ValidateName(name); err != nil { + if err := secret_model.ValidateName(name); err != nil { return nil, false, err } diff --git a/services/secrets/validation.go b/services/secrets/validation.go deleted file mode 100644 index 676012f7d0..0000000000 --- a/services/secrets/validation.go +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2023 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package secrets - -import ( - "regexp" - - "forgejo.org/modules/util" -) - -// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets -var ( - namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$") - forbiddenPrefixPattern = regexp.MustCompile("(?i)^FORGEJO_|GITEA_|GITHUB_") - - ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name") -) - -func ValidateName(name string) error { - if !namePattern.MatchString(name) || forbiddenPrefixPattern.MatchString(name) { - return ErrInvalidName - } - return nil -} diff --git a/services/secrets/validation_test.go b/services/secrets/validation_test.go deleted file mode 100644 index 2dface6f6d..0000000000 --- a/services/secrets/validation_test.go +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright 2026 The Forgejo Authors. All rights reserved. -// SPDX-License-Identifier: GPL-3.0-or-later - -package secrets - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestValidateName(t *testing.T) { - testCases := []struct { - name string - valid bool - }{ - {"FORGEJO_", false}, - {"FORGEJO_123", false}, - {"FORGEJO_ABC", false}, - {"GITEA_", false}, - {"GITEA_123", false}, - {"GITEA_ABC", false}, - {"GITHUB_", false}, - {"GITHUB_123", false}, - {"GITHUB_ABC", false}, - {"123_TEST", false}, - {"CI", true}, - {"_CI", true}, - {"CI_", true}, - {"CI123", true}, - {"CIABC", true}, - {"FORGEJO", true}, - {"FORGEJO123", true}, - {"FORGEJOABC", true}, - {"GITEA", true}, - {"GITEA123", true}, - {"GITEAABC", true}, - {"GITHUB", true}, - {"GITHUB123", true}, - {"GITHUBABC", true}, - {"_123_TEST", true}, - } - for _, tC := range testCases { - t.Run(tC.name, func(t *testing.T) { - t.Helper() - if tC.valid { - assert.NoError(t, ValidateName(tC.name)) - assert.NoError(t, ValidateName(strings.ToLower(tC.name))) - } else { - require.ErrorIs(t, ValidateName(tC.name), ErrInvalidName) - require.ErrorIs(t, ValidateName(strings.ToLower(tC.name)), ErrInvalidName) - } - }) - } -} diff --git a/templates/shared/secrets/add_list.tmpl b/templates/shared/secrets/add_list.tmpl index 2aa9534771..d4550bfb47 100644 --- a/templates/shared/secrets/add_list.tmpl +++ b/templates/shared/secrets/add_list.tmpl @@ -30,8 +30,18 @@ {{ctx.Locale.Tr "settings.added_on" (DateUtils.AbsoluteShort .CreatedUnix)}} +