Skip to content

Refactor legacy unknwon/com package, improve golangci lint #19284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ linters:
- ineffassign
- revive
- gofumpt
- depguard
enable-all: false
disable-all: true
fast: false
Expand Down Expand Up @@ -65,7 +66,15 @@ linters-settings:
- name: modifies-value-receiver
gofumpt:
extra-rules: true
lang-version: 1.18
lang-version: "1.18"
depguard:
# TODO: use depguard to replace import checks in gitea-vet
list-type: denylist
# Check the list against standard lib.
include-go-root: true
packages-with-error-message:
- encoding/json: "use gitea's modules/json instead of encoding/json"
- github.com/unknwon/com: "use gitea's util and replacements"

issues:
exclude-rules:
Expand Down Expand Up @@ -153,5 +162,6 @@ issues:
- path: models/user/openid.go
linters:
- golint
- linters: staticcheck
- linters:
- staticcheck
text: "strings.Title is deprecated: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead."
3 changes: 1 addition & 2 deletions models/migrations/migrations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"code.gitea.io/gitea/modules/util"

"github.com/stretchr/testify/assert"
"github.com/unknwon/com"
"xorm.io/xorm"
"xorm.io/xorm/names"
)
Expand Down Expand Up @@ -204,7 +203,7 @@ func prepareTestEnv(t *testing.T, skip int, syncModels ...interface{}) (*xorm.En
deferFn := PrintCurrentTest(t, ourSkip)
assert.NoError(t, os.RemoveAll(setting.RepoRootPath))

assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
setting.RepoRootPath))
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions modules/cache/cache_redis.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import (

"code.gitea.io/gitea/modules/graceful"
"code.gitea.io/gitea/modules/nosql"
"code.gitea.io/gitea/modules/util"

"gitea.com/go-chi/cache"
"github.com/go-redis/redis/v8"
"github.com/unknwon/com"
)

// RedisCacher represents a redis cache adapter implementation.
Expand All @@ -29,15 +29,15 @@ type RedisCacher struct {
func (c *RedisCacher) Put(key string, val interface{}, expire int64) error {
key = c.prefix + key
if expire == 0 {
if err := c.c.Set(graceful.GetManager().HammerContext(), key, com.ToStr(val), 0).Err(); err != nil {
if err := c.c.Set(graceful.GetManager().HammerContext(), key, util.ToStr(val), 0).Err(); err != nil {
return err
}
} else {
dur, err := time.ParseDuration(com.ToStr(expire) + "s")
dur, err := time.ParseDuration(util.ToStr(expire) + "s")
if err != nil {
return err
}
if err = c.c.Set(graceful.GetManager().HammerContext(), key, com.ToStr(val), dur).Err(); err != nil {
if err = c.c.Set(graceful.GetManager().HammerContext(), key, util.ToStr(val), dur).Err(); err != nil {
return err
}
}
Expand Down
6 changes: 3 additions & 3 deletions modules/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ import (
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/translation"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/services/auth"

"gitea.com/go-chi/cache"
"gitea.com/go-chi/session"
chi "github.com/go-chi/chi/v5"
"github.com/unknwon/com"
"github.com/unrolled/render"
"golang.org/x/crypto/pbkdf2"
)
Expand Down Expand Up @@ -475,7 +475,7 @@ func (ctx *Context) CookieDecrypt(secret, val string) (string, bool) {
}

key := pbkdf2.Key([]byte(secret), []byte(secret), 1000, 16, sha256.New)
text, err = com.AESGCMDecrypt(key, text)
text, err = util.AESGCMDecrypt(key, text)
return string(text), err == nil
}

Expand All @@ -489,7 +489,7 @@ func (ctx *Context) SetSuperSecureCookie(secret, name, value string, expiry int)
// CookieEncrypt encrypts a given value using the provided secret
func (ctx *Context) CookieEncrypt(secret, value string) string {
key := pbkdf2.Key([]byte(secret), []byte(secret), 1000, 16, sha256.New)
text, err := com.AESGCMEncrypt(key, []byte(value))
text, err := util.AESGCMEncrypt(key, []byte(value))
if err != nil {
panic("error encrypting cookie: " + err.Error())
}
Expand Down
14 changes: 10 additions & 4 deletions modules/context/csrf.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@
package context

import (
"encoding/base32"
"fmt"
"net/http"
"time"

"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware"

"github.com/unknwon/com"
)

// CSRF represents a CSRF service and is used to get the current token and validate a suspect token.
Expand Down Expand Up @@ -162,7 +163,12 @@ func prepareOptions(options []CsrfOptions) CsrfOptions {

// Defaults.
if len(opt.Secret) == 0 {
opt.Secret = string(com.RandomCreateBytes(10))
randBytes, err := util.CryptoRandomBytes(8)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note for other reviewers/maintainers that the reduction of this isn't a concern.

Old situation:
The com.RandomCreateBytes actually doesn't give you the full 8 bits of randomness per byte(so 80 bits in the old case), because it reduces itself to only use the alphanum: https://github.com/unknwon/com/blob/b41c64acd94be7e673c9c8301344d31cce99e06c/string.go#L130 which only gives 6 bits per byte of randomness so it would only receive 48 bits of randomness.

New situation:
With CryptoRandomBytes the full 64 bits would be random.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And before, IIRC the com.RandomCreateBytes uses a math PRNG ..... not crypto-safe.

if err != nil {
// this panic can be handled by the recover() in http handlers
panic(fmt.Errorf("failed to generate random bytes: %w", err))
}
opt.Secret = base32.StdEncoding.EncodeToString(randBytes)
}
if len(opt.Header) == 0 {
opt.Header = "X-CSRFToken"
Expand Down Expand Up @@ -211,7 +217,7 @@ func Csrfer(opt CsrfOptions, ctx *Context) CSRF {
x.ID = "0"
uid := ctx.Session.Get(opt.SessionKey)
if uid != nil {
x.ID = com.ToStr(uid)
x.ID = util.ToStr(uid)
}

needsNew := false
Expand Down
12 changes: 9 additions & 3 deletions modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ import (
"code.gitea.io/gitea/modules/markup/markdown"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/util"
asymkey_service "code.gitea.io/gitea/services/asymkey"

"github.com/editorconfig/editorconfig-core-go/v2"
"github.com/unknwon/com"
)

// IssueTemplateDirCandidates issue templates directory
Expand Down Expand Up @@ -308,11 +308,17 @@ func EarlyResponseForGoGetMeta(ctx *Context) {
ctx.PlainText(http.StatusBadRequest, "invalid repository path")
return
}
ctx.PlainText(http.StatusOK, com.Expand(`<meta name="go-import" content="{GoGetImport} git {CloneLink}">`,
res, err := vars.Expand(`<meta name="go-import" content="{GoGetImport} git {CloneLink}">`,
map[string]string{
"GoGetImport": ComposeGoGetImport(username, reponame),
"CloneLink": repo_model.ComposeHTTPSCloneURL(username, reponame),
}))
})
if err != nil {
log.Error(err.Error())
ctx.PlainText(http.StatusInternalServerError, "expand vars failed")
return
}
ctx.PlainText(http.StatusOK, res)
}

// RedirectToRepo redirect to a differently-named repository
Expand Down
2 changes: 1 addition & 1 deletion modules/json/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ package json
import (
"bytes"
"encoding/binary"
"encoding/json"
"encoding/json" //nolint:depguard
"io"

jsoniter "github.com/json-iterator/go"
Expand Down
11 changes: 9 additions & 2 deletions modules/markup/html.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"code.gitea.io/gitea/modules/markup/common"
"code.gitea.io/gitea/modules/references"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/util"

"github.com/unknwon/com"
"golang.org/x/net/html"
"golang.org/x/net/html/atom"
"mvdan.cc/xurls/v2"
Expand Down Expand Up @@ -838,7 +838,14 @@ func issueIndexPatternProcessor(ctx *RenderContext, node *html.Node) {
reftext := node.Data[ref.RefLocation.Start:ref.RefLocation.End]
if exttrack && !ref.IsPull {
ctx.Metas["index"] = ref.Issue
link = createLink(com.Expand(ctx.Metas["format"], ctx.Metas), reftext, "ref-issue ref-external-issue")

res, err := vars.Expand(ctx.Metas["format"], ctx.Metas)
if err != nil {
// here we could just log the error and continue the rendering
log.Error("unable to expand template vars for ref %s, err: %v", ref.Issue, err)
}

link = createLink(res, reftext, "ref-issue ref-external-issue")
} else {
// Path determines the type of link that will be rendered. It's unknown at this point whether
// the linked item is actually a PR or an issue. Luckily it's of no real consequence because
Expand Down
10 changes: 7 additions & 3 deletions modules/repository/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/options"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/util"
asymkey_service "code.gitea.io/gitea/services/asymkey"

"github.com/unknwon/com"
)

var (
Expand Down Expand Up @@ -250,8 +249,13 @@ func prepareRepoCommit(ctx context.Context, repo *repo_model.Repository, tmpDir,
"CloneURL.HTTPS": cloneLink.HTTPS,
"OwnerName": repo.OwnerName,
}
res, err := vars.Expand(string(data), match)
if err != nil {
// here we could just log the error and continue the rendering
log.Error("unable to expand template vars for repo README: %s, err: %v", opts.Readme, err)
}
if err = os.WriteFile(filepath.Join(tmpDir, "README.md"),
[]byte(com.Expand(string(data), match)), 0o644); err != nil {
[]byte(res), 0o644); err != nil {
return fmt.Errorf("write README.md: %v", err)
}

Expand Down
3 changes: 1 addition & 2 deletions modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"code.gitea.io/gitea/modules/user"
"code.gitea.io/gitea/modules/util"

"github.com/unknwon/com"
gossh "golang.org/x/crypto/ssh"
ini "gopkg.in/ini.v1"
)
Expand Down Expand Up @@ -612,7 +611,7 @@ func loadFromConf(allowEmpty bool, extraConfig string) {

Cfg.NameMapper = ini.SnackCase

homeDir, err := com.HomeDir()
homeDir, err := util.HomeDir()
if err != nil {
log.Fatal("Failed to get home directory: %v", err)
}
Expand Down
10 changes: 4 additions & 6 deletions modules/sync/unique_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@

package sync

import (
"github.com/unknwon/com"
)
import "code.gitea.io/gitea/modules/util"

// UniqueQueue is a queue which guarantees only one instance of same
// identity is in the line. Instances with same identity will be
Expand Down Expand Up @@ -73,13 +71,13 @@ func (q *UniqueQueue) Queue() <-chan string {
// Exist returns true if there is an instance with given identity
// exists in the queue.
func (q *UniqueQueue) Exist(id interface{}) bool {
return q.table.IsRunning(com.ToStr(id))
return q.table.IsRunning(util.ToStr(id))
}

// AddFunc adds new instance to the queue with a custom runnable function,
// the queue is blocked until the function exits.
func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {
idStr := com.ToStr(id)
idStr := util.ToStr(id)
q.table.lock.Lock()
if _, ok := q.table.pool[idStr]; ok {
q.table.lock.Unlock()
Expand All @@ -105,5 +103,5 @@ func (q *UniqueQueue) Add(id interface{}) {

// Remove removes instance from the queue.
func (q *UniqueQueue) Remove(id interface{}) {
q.table.Stop(com.ToStr(id))
q.table.Stop(util.ToStr(id))
}
93 changes: 93 additions & 0 deletions modules/templates/vars/vars.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright 2022 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package vars

import (
"fmt"
"strings"
"unicode"
"unicode/utf8"
)

// ErrWrongSyntax represents a wrong syntax with a template
type ErrWrongSyntax struct {
Template string
}

func (err ErrWrongSyntax) Error() string {
return fmt.Sprintf("wrong syntax found in %s", err.Template)
}

// ErrVarMissing represents an error that no matched variable
type ErrVarMissing struct {
Template string
Var string
}

func (err ErrVarMissing) Error() string {
return fmt.Sprintf("the variable %s is missing for %s", err.Var, err.Template)
}

// Expand replaces all variables like {var} by `vars` map, it always returns the expanded string regardless of errors
// if error occurs, the error part doesn't change and is returned as it is.
func Expand(template string, vars map[string]string) (string, error) {
// in the future, if necessary, we can introduce some escape-char,
// for example: it will use `#' as a reversed char, templates will use `{#{}` to do escape and output char '{'.
var buf strings.Builder
var err error

posBegin := 0
strLen := len(template)
for posBegin < strLen {
// find the next `{`
pos := strings.IndexByte(template[posBegin:], '{')
if pos == -1 {
buf.WriteString(template[posBegin:])
break
}

// copy texts between vars
buf.WriteString(template[posBegin : posBegin+pos])

// find the var between `{` and `}`/end
posBegin += pos
posEnd := posBegin + 1
for posEnd < strLen {
if template[posEnd] == '}' {
posEnd++
break
} // in the future, if we need to support escape chars, we can do: if (isEscapeChar) { posEnd+=2 }
posEnd++
}

// the var part, it can be "{", "{}", "{..." or or "{...}"
part := template[posBegin:posEnd]
posBegin = posEnd
if part == "{}" || part[len(part)-1] != '}' {
// treat "{}" or "{..." as error
err = ErrWrongSyntax{Template: template}
buf.WriteString(part)
} else {
// now we get a valid key "{...}"
key := part[1 : len(part)-1]
keyFirst, _ := utf8.DecodeRuneInString(key)
if unicode.IsSpace(keyFirst) || unicode.IsPunct(keyFirst) || unicode.IsControl(keyFirst) {
// the if key doesn't start with a letter, then we do not treat it as a var now
buf.WriteString(part)
} else {
// look up in the map
if val, ok := vars[key]; ok {
buf.WriteString(val)
} else {
// write the non-existing var as it is
buf.WriteString(part)
err = ErrVarMissing{Template: template, Var: key}
}
}
}
}

return buf.String(), err
}
Loading