From eeb47e3340de5b1fdd84959cc0eac3e3a07a5302 Mon Sep 17 00:00:00 2001 From: zokki Date: Wed, 14 Jan 2026 04:19:21 +0100 Subject: [PATCH] fix: actions variable and secret names validation (#10682) Fixed action variables and secrets according to [Docu](https://forgejo.org/docs/next/user/actions/basic-concepts/#name-constraints): > Variable names must not start with the FORGEJO_, GITHUB_ or GITEA_ prefix. This wasn't correctly enforced, so I changed the regex ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [x] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [ ] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10682 Reviewed-by: Andreas Ahlenstorf Reviewed-by: Mathieu Fenniak Co-authored-by: zokki Co-committed-by: zokki --- services/actions/variables.go | 9 +- services/secrets/secrets.go | 4 - services/secrets/validation.go | 2 +- services/secrets/validation_test.go | 57 +++++++ templates/shared/secrets/add_list.tmpl | 2 +- templates/shared/variables/variable_list.tmpl | 2 +- tests/integration/api_org_secrets_test.go | 160 ++++++++++++++++++ tests/integration/api_org_variables_test.go | 35 +++- tests/integration/api_repo_secrets_test.go | 62 ++++++- tests/integration/api_repo_variables_test.go | 32 +++- tests/integration/api_user_secrets_test.go | 43 ++++- tests/integration/api_user_variables_test.go | 33 +++- 12 files changed, 411 insertions(+), 30 deletions(-) create mode 100644 services/secrets/validation_test.go create mode 100644 tests/integration/api_org_secrets_test.go diff --git a/services/actions/variables.go b/services/actions/variables.go index f06c81e474..5dd68f0130 100644 --- a/services/actions/variables.go +++ b/services/actions/variables.go @@ -50,14 +50,6 @@ func UpdateVariable(ctx context.Context, variableID, ownerID, repoID int64, name } func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error { - if err := secrets_service.ValidateName(name); err != nil { - return err - } - - if err := envNameCIRegexMatch(name); err != nil { - return err - } - v, err := GetVariable(ctx, actions_model.FindVariablesOpts{ OwnerID: ownerID, RepoID: repoID, @@ -86,6 +78,7 @@ func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*ac // reference to: // https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables // https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets +// https://forgejo.org/docs/next/user/actions/basic-concepts/#name-constraints var ( forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI$") ) diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index 2de83ef5a2..212016cf1e 100644 --- a/services/secrets/secrets.go +++ b/services/secrets/secrets.go @@ -56,10 +56,6 @@ func DeleteSecretByID(ctx context.Context, ownerID, repoID, secretID int64) erro } func DeleteSecretByName(ctx context.Context, ownerID, repoID int64, name string) error { - if err := ValidateName(name); err != nil { - return err - } - s, err := db.Find[secret_model.Secret](ctx, secret_model.FindSecretsOptions{ OwnerID: ownerID, RepoID: repoID, diff --git a/services/secrets/validation.go b/services/secrets/validation.go index 44250ba87b..676012f7d0 100644 --- a/services/secrets/validation.go +++ b/services/secrets/validation.go @@ -12,7 +12,7 @@ import ( // 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)^GIT(EA|HUB)_") + forbiddenPrefixPattern = regexp.MustCompile("(?i)^FORGEJO_|GITEA_|GITHUB_") ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name") ) diff --git a/services/secrets/validation_test.go b/services/secrets/validation_test.go new file mode 100644 index 0000000000..2dface6f6d --- /dev/null +++ b/services/secrets/validation_test.go @@ -0,0 +1,57 @@ +// 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 0cd276f64c..d4f3744717 100644 --- a/templates/shared/secrets/add_list.tmpl +++ b/templates/shared/secrets/add_list.tmpl @@ -62,7 +62,7 @@ id="secret-name" name="name" value="{{.name}}" - pattern="^(?!GITEA_|GITHUB_)[a-zA-Z_][a-zA-Z0-9_]*$" + pattern="^(?!FORGEJO_|GITEA_|GITHUB_)[a-zA-Z_][a-zA-Z0-9_]*$" placeholder="{{ctx.Locale.Tr "secrets.creation.name_placeholder"}}" > diff --git a/templates/shared/variables/variable_list.tmpl b/templates/shared/variables/variable_list.tmpl index 0ac9879264..ac7b0afe85 100644 --- a/templates/shared/variables/variable_list.tmpl +++ b/templates/shared/variables/variable_list.tmpl @@ -72,7 +72,7 @@ name="name" id="dialog-variable-name" value="{{.name}}" - pattern="^(?!GITEA_|GITHUB_)[a-zA-Z_][a-zA-Z0-9_]*$" + pattern="^(?!CI$)(?!FORGEJO_|GITEA_|GITHUB_)[a-zA-Z_][a-zA-Z0-9_]*$" placeholder="{{ctx.Locale.Tr "secrets.creation.name_placeholder"}}" > diff --git a/tests/integration/api_org_secrets_test.go b/tests/integration/api_org_secrets_test.go new file mode 100644 index 0000000000..2a0574549d --- /dev/null +++ b/tests/integration/api_org_secrets_test.go @@ -0,0 +1,160 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "strings" + "testing" + + auth_model "forgejo.org/models/auth" + org_model "forgejo.org/models/organization" + secret_model "forgejo.org/models/secret" + "forgejo.org/models/unittest" + "forgejo.org/modules/keying" + api "forgejo.org/modules/structs" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAPIOrgSecrets(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + org := unittest.AssertExistsAndLoadBean(t, &org_model.Organization{Name: "org3"}) + session := loginUser(t, "user2") + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) + + t.Run("List", func(t *testing.T) { + listURL := fmt.Sprintf("/api/v1/orgs/%s/actions/secrets", org.Name) + req := NewRequest(t, "GET", listURL).AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + secrets := []*api.Secret{} + DecodeJSON(t, res, &secrets) + assert.Empty(t, secrets) + + createData := api.CreateOrUpdateSecretOption{Data: "a secret to create"} + req = NewRequestWithJSON(t, "PUT", listURL+"/first", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + req = NewRequestWithJSON(t, "PUT", listURL+"/sec2", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + req = NewRequestWithJSON(t, "PUT", listURL+"/last", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequest(t, "GET", listURL).AddTokenAuth(token) + res = MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &secrets) + assert.Len(t, secrets, 3) + expectedValues := []string{"FIRST", "SEC2", "LAST"} + for _, secret := range secrets { + assert.Contains(t, expectedValues, secret.Name) + } + }) + + t.Run("Create", func(t *testing.T) { + cases := []struct { + Name string + ExpectedStatus int + }{ + { + Name: "", + ExpectedStatus: http.StatusMethodNotAllowed, + }, + { + Name: "-", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: "_", + ExpectedStatus: http.StatusCreated, + }, + { + Name: "ci", + ExpectedStatus: http.StatusCreated, + }, + { + Name: "secret", + ExpectedStatus: http.StatusCreated, + }, + { + Name: "2secret", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: "FORGEJO_secret", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: "GITEA_secret", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: "GITHUB_secret", + ExpectedStatus: http.StatusBadRequest, + }, + } + + for _, c := range cases { + req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, c.Name), api.CreateOrUpdateSecretOption{ + Data: "data", + }).AddTokenAuth(token) + MakeRequest(t, req, c.ExpectedStatus) + } + }) + + t.Run("Update", func(t *testing.T) { + name := "update_org_secret_and_test_data" + url := fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, name) + + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ + Data: "initial", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ + Data: "changed data", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) + data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.Equal(t, "changed data", string(data)) + }) + + t.Run("Delete", func(t *testing.T) { + name := "delete_secret" + url := fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, name) + + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ + Data: "initial", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Delete with forbidden names", func(t *testing.T) { + secret, err := secret_model.InsertEncryptedSecret(t.Context(), org.ID, 0, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) + + url := fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, secret.Name) + + req := NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) + }) +} diff --git a/tests/integration/api_org_variables_test.go b/tests/integration/api_org_variables_test.go index ffd73fadb2..f3ef009772 100644 --- a/tests/integration/api_org_variables_test.go +++ b/tests/integration/api_org_variables_test.go @@ -8,6 +8,7 @@ import ( "net/http" "testing" + actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" org_model "forgejo.org/models/organization" "forgejo.org/models/unittest" @@ -15,6 +16,7 @@ import ( "forgejo.org/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIOrgVariablesCreateOrganizationVariable(t *testing.T) { @@ -56,6 +58,10 @@ func TestAPIOrgVariablesCreateOrganizationVariable(t *testing.T) { Name: "var@test", ExpectedStatus: http.StatusBadRequest, }, + { + Name: "forgejo_var", + ExpectedStatus: http.StatusBadRequest, + }, { Name: "github_var", ExpectedStatus: http.StatusBadRequest, @@ -68,9 +74,21 @@ func TestAPIOrgVariablesCreateOrganizationVariable(t *testing.T) { for _, c := range cases { requestURL := fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, c.Name) - request := NewRequestWithJSON(t, "POST", requestURL, api.CreateVariableOption{Value: "value"}) + request := NewRequestWithJSON(t, "POST", requestURL, api.CreateVariableOption{ + Value: "value" + c.Name, + }) request.AddTokenAuth(token) MakeRequest(t, request, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + request = NewRequest(t, "GET", requestURL). + AddTokenAuth(token) + res := MakeRequest(t, request, http.StatusOK) + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) + assert.Equal(t, variable.Data, "value"+c.Name) + } } } @@ -114,6 +132,11 @@ func TestAPIOrgVariablesUpdateOrganizationVariable(t *testing.T) { UpdateName: "ci", ExpectedStatus: http.StatusBadRequest, }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, { Name: variableName, UpdateName: "updated_var_name", @@ -141,6 +164,8 @@ func TestAPIOrgVariablesDeleteOrganizationVariable(t *testing.T) { defer tests.PrepareTestEnv(t)() org := unittest.AssertExistsAndLoadBean(t, &org_model.Organization{Name: "org3"}) + variable, err := actions_model.InsertVariable(t.Context(), org.ID, 0, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) session := loginUser(t, "user2") token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) @@ -157,6 +182,14 @@ func TestAPIOrgVariablesDeleteOrganizationVariable(t *testing.T) { request = NewRequest(t, "DELETE", url).AddTokenAuth(token) MakeRequest(t, request, http.StatusNotFound) + + // deleting of forbidden names should still be possible + url = fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, variable.Name) + request = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, request, http.StatusNoContent) + + request = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, request, http.StatusNotFound) } func TestAPIOrgVariablesGetSingleOrganizationVariable(t *testing.T) { diff --git a/tests/integration/api_repo_secrets_test.go b/tests/integration/api_repo_secrets_test.go index 3da4412dde..a6a59818f7 100644 --- a/tests/integration/api_repo_secrets_test.go +++ b/tests/integration/api_repo_secrets_test.go @@ -6,18 +6,22 @@ package integration import ( "fmt" "net/http" + "strings" "testing" auth_model "forgejo.org/models/auth" "forgejo.org/models/db" repo_model "forgejo.org/models/repo" + secret_model "forgejo.org/models/secret" unit_model "forgejo.org/models/unit" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/keying" api "forgejo.org/modules/structs" repo_service "forgejo.org/services/repository" "forgejo.org/tests" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -30,9 +34,29 @@ func TestAPIRepoSecrets(t *testing.T) { token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) t.Run("List", func(t *testing.T) { - req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/secrets", repo.FullName())). - AddTokenAuth(token) - MakeRequest(t, req, http.StatusOK) + listURL := fmt.Sprintf("/api/v1/repos/%s/actions/secrets", repo.FullName()) + req := NewRequest(t, "GET", listURL).AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + secrets := []*api.Secret{} + DecodeJSON(t, res, &secrets) + assert.Empty(t, secrets) + + createData := api.CreateOrUpdateSecretOption{Data: "a secret to create"} + req = NewRequestWithJSON(t, "PUT", listURL+"/first", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + req = NewRequestWithJSON(t, "PUT", listURL+"/sec2", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + req = NewRequestWithJSON(t, "PUT", listURL+"/last", createData).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequest(t, "GET", listURL).AddTokenAuth(token) + res = MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &secrets) + assert.Len(t, secrets, 3) + expectedValues := []string{"FIRST", "SEC2", "LAST"} + for _, secret := range secrets { + assert.Contains(t, expectedValues, secret.Name) + } }) t.Run("Create", func(t *testing.T) { @@ -52,6 +76,10 @@ func TestAPIRepoSecrets(t *testing.T) { Name: "_", ExpectedStatus: http.StatusCreated, }, + { + Name: "ci", + ExpectedStatus: http.StatusCreated, + }, { Name: "secret", ExpectedStatus: http.StatusCreated, @@ -60,6 +88,10 @@ func TestAPIRepoSecrets(t *testing.T) { Name: "2secret", ExpectedStatus: http.StatusBadRequest, }, + { + Name: "FORGEJO_secret", + ExpectedStatus: http.StatusBadRequest, + }, { Name: "GITEA_secret", ExpectedStatus: http.StatusBadRequest, @@ -79,7 +111,7 @@ func TestAPIRepoSecrets(t *testing.T) { }) t.Run("Update", func(t *testing.T) { - name := "update_secret" + name := "update_repo_secret_and_test_data" url := fmt.Sprintf("/api/v1/repos/%s/actions/secrets/%s", repo.FullName(), name) req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ @@ -88,9 +120,14 @@ func TestAPIRepoSecrets(t *testing.T) { MakeRequest(t, req, http.StatusCreated) req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "changed", + Data: "changed data", }).AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) + data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.Equal(t, "changed data", string(data)) }) t.Run("Delete", func(t *testing.T) { @@ -109,10 +146,21 @@ func TestAPIRepoSecrets(t *testing.T) { req = NewRequest(t, "DELETE", url). AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) + }) - req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/repos/%s/actions/secrets/000", repo.FullName())). + t.Run("Delete with forbidden names", func(t *testing.T) { + secret, err := secret_model.InsertEncryptedSecret(t.Context(), 0, repo.ID, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) + + url := fmt.Sprintf("/api/v1/repos/%s/actions/secrets/%s", repo.FullName(), secret.Name) + + req := NewRequest(t, "DELETE", url). AddTokenAuth(token) - MakeRequest(t, req, http.StatusBadRequest) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) }) t.Run("Endpoints disabled if Actions disabled", func(t *testing.T) { diff --git a/tests/integration/api_repo_variables_test.go b/tests/integration/api_repo_variables_test.go index 9330ca6d7e..0cc79bc423 100644 --- a/tests/integration/api_repo_variables_test.go +++ b/tests/integration/api_repo_variables_test.go @@ -8,6 +8,7 @@ import ( "net/http" "testing" + actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" "forgejo.org/models/db" repo_model "forgejo.org/models/repo" @@ -62,6 +63,10 @@ func TestAPIRepoVariablesTestCreateRepositoryVariable(t *testing.T) { Name: "var@test", ExpectedStatus: http.StatusBadRequest, }, + { + Name: "forgejo_var", + ExpectedStatus: http.StatusBadRequest, + }, { Name: "github_var", ExpectedStatus: http.StatusBadRequest, @@ -74,9 +79,19 @@ func TestAPIRepoVariablesTestCreateRepositoryVariable(t *testing.T) { for _, c := range cases { req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name), api.CreateVariableOption{ - Value: "value", + Value: "value" + c.Name, }).AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name)). + AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) + assert.Equal(t, variable.Data, "value"+c.Name) + } } } @@ -119,6 +134,11 @@ func TestAPIRepoVariablesUpdateRepositoryVariable(t *testing.T) { UpdateName: "ci", ExpectedStatus: http.StatusBadRequest, }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, { Name: variableName, UpdateName: "updated_var_name", @@ -148,6 +168,8 @@ func TestAPIRepoVariablesDeleteRepositoryVariable(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + variable, err := actions_model.InsertVariable(t.Context(), 0, repo.ID, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) @@ -164,6 +186,14 @@ func TestAPIRepoVariablesDeleteRepositoryVariable(t *testing.T) { req = NewRequest(t, "DELETE", url).AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) + + // deleting of forbidden names should still be possible + url = fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), variable.Name) + req = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) } func TestAPIRepoVariablesGetSingleRepositoryVariable(t *testing.T) { diff --git a/tests/integration/api_user_secrets_test.go b/tests/integration/api_user_secrets_test.go index 50ecdedfd6..5b26aa57b1 100644 --- a/tests/integration/api_user_secrets_test.go +++ b/tests/integration/api_user_secrets_test.go @@ -6,17 +6,26 @@ package integration import ( "fmt" "net/http" + "strings" "testing" auth_model "forgejo.org/models/auth" + secret_model "forgejo.org/models/secret" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/keying" api "forgejo.org/modules/structs" "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIUserSecrets(t *testing.T) { defer tests.PrepareTestEnv(t)() - session := loginUser(t, "user1") + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"}) + session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteUser) t.Run("Create", func(t *testing.T) { @@ -36,6 +45,10 @@ func TestAPIUserSecrets(t *testing.T) { Name: "_", ExpectedStatus: http.StatusCreated, }, + { + Name: "ci", + ExpectedStatus: http.StatusCreated, + }, { Name: "secret", ExpectedStatus: http.StatusCreated, @@ -44,6 +57,10 @@ func TestAPIUserSecrets(t *testing.T) { Name: "2secret", ExpectedStatus: http.StatusBadRequest, }, + { + Name: "FORGEJO_secret", + ExpectedStatus: http.StatusBadRequest, + }, { Name: "GITEA_secret", ExpectedStatus: http.StatusBadRequest, @@ -63,7 +80,7 @@ func TestAPIUserSecrets(t *testing.T) { }) t.Run("Update", func(t *testing.T) { - name := "update_secret" + name := "update_user_secret_and_test_data" url := fmt.Sprintf("/api/v1/user/actions/secrets/%s", name) req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ @@ -72,9 +89,14 @@ func TestAPIUserSecrets(t *testing.T) { MakeRequest(t, req, http.StatusCreated) req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "changed", + Data: "changed data", }).AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) + data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.Equal(t, "changed data", string(data)) }) t.Run("Delete", func(t *testing.T) { @@ -93,9 +115,20 @@ func TestAPIUserSecrets(t *testing.T) { req = NewRequest(t, "DELETE", url). AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) + }) - req = NewRequest(t, "DELETE", "/api/v1/user/actions/secrets/000"). + t.Run("Delete with forbidden names", func(t *testing.T) { + secret, err := secret_model.InsertEncryptedSecret(t.Context(), user.ID, 0, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) + + url := fmt.Sprintf("/api/v1/user/actions/secrets/%s", secret.Name) + + req := NewRequest(t, "DELETE", url). AddTokenAuth(token) - MakeRequest(t, req, http.StatusBadRequest) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) }) } diff --git a/tests/integration/api_user_variables_test.go b/tests/integration/api_user_variables_test.go index 12b4cb5b55..d4806031b5 100644 --- a/tests/integration/api_user_variables_test.go +++ b/tests/integration/api_user_variables_test.go @@ -8,6 +8,7 @@ import ( "net/http" "testing" + actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" @@ -15,6 +16,7 @@ import ( "forgejo.org/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIUserVariablesCreateUserVariable(t *testing.T) { @@ -57,6 +59,10 @@ func TestAPIUserVariablesCreateUserVariable(t *testing.T) { Name: "var@test", ExpectedStatus: http.StatusBadRequest, }, + { + Name: "forgejo_var", + ExpectedStatus: http.StatusBadRequest, + }, { Name: "github_var", ExpectedStatus: http.StatusBadRequest, @@ -69,9 +75,19 @@ func TestAPIUserVariablesCreateUserVariable(t *testing.T) { for _, c := range cases { req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name), api.CreateVariableOption{ - Value: "value", + Value: "value" + c.Name, }).AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name)). + AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) + assert.Equal(t, variable.Data, "value"+c.Name) + } } } @@ -114,6 +130,11 @@ func TestAPIUserVariablesUpdateUserVariable(t *testing.T) { UpdateName: "ci", ExpectedStatus: http.StatusBadRequest, }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, { Name: variableName, UpdateName: "updated_var_name", @@ -142,6 +163,8 @@ func TestAPIUserVariablesDeleteUserVariable(t *testing.T) { defer tests.PrepareTestEnv(t)() user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"}) + variable, err := actions_model.InsertVariable(t.Context(), user1.ID, 0, "FORGEJO_FORBIDDEN", "illegal") + require.NoError(t, err) session := loginUser(t, user1.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteUser) @@ -159,6 +182,14 @@ func TestAPIUserVariablesDeleteUserVariable(t *testing.T) { req = NewRequest(t, "DELETE", url).AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) + + // deleting of forbidden names should still be possible + url = fmt.Sprintf("/api/v1/user/actions/variables/%s", variable.Name) + req = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "DELETE", url).AddTokenAuth(token) + MakeRequest(t, req, http.StatusNotFound) } func TestAPIUserVariablesGetSingleUserVariable(t *testing.T) {