From 3d63caa54245d87dd057d4e853bb5dc7fc39e7db Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 8 Apr 2020 04:54:46 +0200 Subject: [PATCH] [API] Get a single commit via Ref (#10915) * GET /repos/:owner/:repo/commits/:ref * add Validation Checks * Fix & Extend TEST * add two new tast cases --- integrations/api_repo_git_commits_test.go | 42 +++++++++++---- modules/convert/convert.go | 1 - modules/git/sha1.go | 4 ++ modules/validation/binding.go | 40 ++++++++------ routers/api/v1/api.go | 4 +- routers/api/v1/repo/commits.go | 63 +++++++++++++++++++++-- templates/swagger/v1_json.tmpl | 51 +++++++++++++++++- 7 files changed, 172 insertions(+), 33 deletions(-) diff --git a/integrations/api_repo_git_commits_test.go b/integrations/api_repo_git_commits_test.go index 5291b8f4ac..f710811ea7 100644 --- a/integrations/api_repo_git_commits_test.go +++ b/integrations/api_repo_git_commits_test.go @@ -21,17 +21,41 @@ func TestAPIReposGitCommits(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session) - for _, ref := range [...]string{ - "commits/master", // Branch - "commits/v1.1", // Tag - } { - req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/%s?token="+token, user.Name, ref) - session.MakeRequest(t, req, http.StatusOK) - } + //check invalid requests for GetCommitsBySHA + req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/master?token="+token, user.Name) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) - // Test getting non-existent refs - req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/unknown?token="+token, user.Name) + req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/12345?token="+token, user.Name) session.MakeRequest(t, req, http.StatusNotFound) + + //check invalid requests for GetCommitsByRef + req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/..?token="+token, user.Name) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + + req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/branch-not-exist?token="+token, user.Name) + session.MakeRequest(t, req, http.StatusNotFound) + + for _, ref := range [...]string{ + "master", // Branch + "v1.1", // Tag + "65f1", // short sha + "65f1bf27bc3bf70f64657658635e66094edbcb4d", // full sha + } { + req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/%s?token="+token, user.Name, ref) + resp := session.MakeRequest(t, req, http.StatusOK) + commitByRef := new(api.Commit) + DecodeJSON(t, resp, commitByRef) + assert.Len(t, commitByRef.SHA, 40) + assert.EqualValues(t, commitByRef.SHA, commitByRef.RepoCommit.Tree.SHA) + req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/%s?token="+token, user.Name, commitByRef.SHA) + resp = session.MakeRequest(t, req, http.StatusOK) + commitBySHA := new(api.Commit) + DecodeJSON(t, resp, commitBySHA) + + assert.EqualValues(t, commitByRef.SHA, commitBySHA.SHA) + assert.EqualValues(t, commitByRef.HTMLURL, commitBySHA.HTMLURL) + assert.EqualValues(t, commitByRef.RepoCommit.Message, commitBySHA.RepoCommit.Message) + } } func TestAPIReposGitCommitList(t *testing.T) { diff --git a/modules/convert/convert.go b/modules/convert/convert.go index e11a599fd6..30d240f809 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -386,7 +386,6 @@ func ToCommitUser(sig *git.Signature) *api.CommitUser { func ToCommitMeta(repo *models.Repository, tag *git.Tag) *api.CommitMeta { return &api.CommitMeta{ SHA: tag.Object.String(), - // TODO: Add the /commits API endpoint and use it here (https://developer.github.com/v3/repos/commits/#get-a-single-commit) URL: util.URLJoin(repo.APIURL(), "git/commits", tag.ID.String()), } } diff --git a/modules/git/sha1.go b/modules/git/sha1.go index 59b8335736..bccc94f103 100644 --- a/modules/git/sha1.go +++ b/modules/git/sha1.go @@ -8,6 +8,7 @@ package git import ( "encoding/hex" "fmt" + "regexp" "strings" "github.com/go-git/go-git/v5/plumbing" @@ -16,6 +17,9 @@ import ( // EmptySHA defines empty git SHA const EmptySHA = "0000000000000000000000000000000000000000" +// SHAPattern can be used to determine if a string is an valid sha +var SHAPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`) + // SHA1 a git commit name type SHA1 = plumbing.Hash diff --git a/modules/validation/binding.go b/modules/validation/binding.go index 6ed75b50fb..1c67878ea1 100644 --- a/modules/validation/binding.go +++ b/modules/validation/binding.go @@ -22,12 +22,32 @@ const ( ) var ( - // GitRefNamePattern is regular expression with unallowed characters in git reference name + // GitRefNamePatternInvalid is regular expression with unallowed characters in git reference name // They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere. // They cannot have question-mark ?, asterisk *, or open bracket [ anywhere - GitRefNamePattern = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`) + GitRefNamePatternInvalid = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`) ) +// CheckGitRefAdditionalRulesValid check name is valid on additional rules +func CheckGitRefAdditionalRulesValid(name string) bool { + + // Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html + if strings.HasPrefix(name, "/") || strings.HasSuffix(name, "/") || + strings.HasSuffix(name, ".") || strings.Contains(name, "..") || + strings.Contains(name, "//") || strings.Contains(name, "@{") || + name == "@" { + return false + } + parts := strings.Split(name, "/") + for _, part := range parts { + if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") { + return false + } + } + + return true +} + // AddBindingRules adds additional binding rules func AddBindingRules() { addGitRefNameBindingRule() @@ -44,25 +64,15 @@ func addGitRefNameBindingRule() { IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) { str := fmt.Sprintf("%v", val) - if GitRefNamePattern.MatchString(str) { + if GitRefNamePatternInvalid.MatchString(str) { errs.Add([]string{name}, ErrGitRefName, "GitRefName") return false, errs } - // Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html - if strings.HasPrefix(str, "/") || strings.HasSuffix(str, "/") || - strings.HasSuffix(str, ".") || strings.Contains(str, "..") || - strings.Contains(str, "//") || strings.Contains(str, "@{") || - str == "@" { + + if !CheckGitRefAdditionalRulesValid(str) { errs.Add([]string{name}, ErrGitRefName, "GitRefName") return false, errs } - parts := strings.Split(str, "/") - for _, part := range parts { - if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") { - errs.Add([]string{name}, ErrGitRefName, "GitRefName") - return false, errs - } - } return true, errs }, diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index e5bb98033b..8fc1eeefd1 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -798,14 +798,14 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/commits", func() { m.Get("", repo.GetAllCommits) m.Group("/:ref", func() { - // TODO: Add m.Get("") for single commit (https://developer.github.com/v3/repos/commits/#get-a-single-commit) + m.Get("", repo.GetSingleCommitByRef) m.Get("/status", repo.GetCombinedCommitStatusByRef) m.Get("/statuses", repo.GetCommitStatusesByRef) }) }, reqRepoReader(models.UnitTypeCode)) m.Group("/git", func() { m.Group("/commits", func() { - m.Get("/:sha", repo.GetSingleCommit) + m.Get("/:sha", repo.GetSingleCommitBySHA) }) m.Get("/refs", repo.GetGitAllRefs) m.Get("/refs/*", repo.GetGitRefs) diff --git a/routers/api/v1/repo/commits.go b/routers/api/v1/repo/commits.go index f7da1698dc..aa949aa9ec 100644 --- a/routers/api/v1/repo/commits.go +++ b/routers/api/v1/repo/commits.go @@ -6,6 +6,7 @@ package repo import ( + "fmt" "math" "net/http" "strconv" @@ -16,12 +17,13 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/validation" "code.gitea.io/gitea/routers/api/v1/utils" ) -// GetSingleCommit get a commit via -func GetSingleCommit(ctx *context.APIContext) { - // swagger:operation GET /repos/{owner}/{repo}/git/commits/{sha} repository repoGetSingleCommit +// GetSingleCommitBySHA get a commit via sha +func GetSingleCommitBySHA(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/git/commits/{sha} repository repoGetSingleCommitBySHA // --- // summary: Get a single commit from a repository // produces: @@ -45,16 +47,68 @@ func GetSingleCommit(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/Commit" + // "422": + // "$ref": "#/responses/validationError" // "404": // "$ref": "#/responses/notFound" + sha := ctx.Params(":sha") + if !git.SHAPattern.MatchString(sha) { + ctx.Error(http.StatusUnprocessableEntity, "no valid sha", fmt.Sprintf("no valid sha: %s", sha)) + return + } + getCommit(ctx, sha) +} + +// GetSingleCommitByRef get a commit via ref +func GetSingleCommitByRef(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/commits/{ref} repository repoGetSingleCommitByRef + // --- + // summary: Get a single commit from a repository + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: ref + // in: path + // description: a git ref + // type: string + // required: true + // responses: + // "200": + // "$ref": "#/responses/Commit" + // "422": + // "$ref": "#/responses/validationError" + // "404": + // "$ref": "#/responses/notFound" + + ref := ctx.Params("ref") + + if validation.GitRefNamePatternInvalid.MatchString(ref) || !validation.CheckGitRefAdditionalRulesValid(ref) { + ctx.Error(http.StatusUnprocessableEntity, "no valid sha", fmt.Sprintf("no valid ref: %s", ref)) + return + } + + getCommit(ctx, ref) +} + +func getCommit(ctx *context.APIContext, identifier string) { gitRepo, err := git.OpenRepository(ctx.Repo.Repository.RepoPath()) if err != nil { ctx.ServerError("OpenRepository", err) return } defer gitRepo.Close() - commit, err := gitRepo.GetCommit(ctx.Params(":sha")) + commit, err := gitRepo.GetCommit(identifier) if err != nil { ctx.NotFoundOrServerError("GetCommit", git.IsErrNotExist, err) return @@ -65,7 +119,6 @@ func GetSingleCommit(ctx *context.APIContext) { ctx.ServerError("toCommit", err) return } - ctx.JSON(http.StatusOK, json) } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 6e5086d507..55094e391d 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2511,6 +2511,52 @@ } } }, + "/repos/{owner}/{repo}/commits/{ref}": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Get a single commit from a repository", + "operationId": "repoGetSingleCommitByRef", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "a git ref", + "name": "ref", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/Commit" + }, + "404": { + "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" + } + } + } + }, "/repos/{owner}/{repo}/commits/{ref}/statuses": { "get": { "produces": [ @@ -2976,7 +3022,7 @@ "repository" ], "summary": "Get a single commit from a repository", - "operationId": "repoGetSingleCommit", + "operationId": "repoGetSingleCommitBySHA", "parameters": [ { "type": "string", @@ -3006,6 +3052,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" } } }