Skip to content

Commit c831681

Browse files
wxiaoguangwxiaoguang
authored
Use a more general (and faster) method to sanitize URLs with credentials (#19239)
Use a more general method to sanitize URLs with credentials: Simple and intuitive / Faster / Remove all credentials in all URLs
1 parent 84038f3 commit c831681

File tree

12 files changed

+103
-194
lines changed

12 files changed

+103
-194
lines changed

‎models/migrations/v180.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func removeCredentials(payload string) (string, error) {
112112

113113
opts.AuthPassword = ""
114114
opts.AuthToken = ""
115-
opts.CloneAddr = util.NewStringURLSanitizer(opts.CloneAddr, true).Replace(opts.CloneAddr)
115+
opts.CloneAddr = util.SanitizeCredentialURLs(opts.CloneAddr)
116116

117117
confBytes, err := json.Marshal(opts)
118118
if err != nil {

‎models/task.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ func FinishMigrateTask(task *Task) error {
245245
}
246246
conf.AuthPassword = ""
247247
conf.AuthToken = ""
248-
conf.CloneAddr = util.NewStringURLSanitizer(conf.CloneAddr, true).Replace(conf.CloneAddr)
248+
conf.CloneAddr = util.SanitizeCredentialURLs(conf.CloneAddr)
249249
conf.AuthPasswordEncrypted = ""
250250
conf.AuthTokenEncrypted = ""
251251
conf.CloneAddrEncrypted = ""

‎modules/git/command.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func (c *Command) RunWithContext(rc *RunContext) error {
154154
args = make([]string, len(c.args))
155155
copy(args, c.args)
156156
for _, urlArgIndex := range argSensitiveURLIndexes {
157-
args[urlArgIndex] = util.NewStringURLSanitizer(args[urlArgIndex], true).Replace(args[urlArgIndex])
157+
args[urlArgIndex] = util.SanitizeCredentialURLs(args[urlArgIndex])
158158
}
159159
}
160160
desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(args, " "), rc.Dir)

‎modules/git/repo.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func CloneWithArgs(ctx context.Context, from, to string, args []string, opts Clo
156156
cmd.AddArguments("--", from, to)
157157

158158
if strings.Contains(from, "://") && strings.Contains(from, "@") {
159-
cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, util.NewStringURLSanitizer(from, true).Replace(from), to, opts.Shared, opts.Mirror, opts.Depth))
159+
cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, util.SanitizeCredentialURLs(from), to, opts.Shared, opts.Mirror, opts.Depth))
160160
} else {
161161
cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, from, to, opts.Shared, opts.Mirror, opts.Depth))
162162
}
@@ -209,7 +209,7 @@ func Push(ctx context.Context, repoPath string, opts PushOptions) error {
209209
cmd.AddArguments(opts.Branch)
210210
}
211211
if strings.Contains(opts.Remote, "://") && strings.Contains(opts.Remote, "@") {
212-
cmd.SetDescription(fmt.Sprintf("push branch %s to %s (force: %t, mirror: %t)", opts.Branch, util.NewStringURLSanitizer(opts.Remote, true).Replace(opts.Remote), opts.Force, opts.Mirror))
212+
cmd.SetDescription(fmt.Sprintf("push branch %s to %s (force: %t, mirror: %t)", opts.Branch, util.SanitizeCredentialURLs(opts.Remote), opts.Force, opts.Mirror))
213213
} else {
214214
cmd.SetDescription(fmt.Sprintf("push branch %s to %s (force: %t, mirror: %t)", opts.Branch, opts.Remote, opts.Force, opts.Mirror))
215215
}

‎modules/util/sanitize.go‎

Lines changed: 50 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -5,59 +5,71 @@
55
package util
66

77
import (
8-
"net/url"
9-
"strings"
10-
)
8+
"bytes"
9+
"unicode"
1110

12-
const (
13-
userPlaceholder = "sanitized-credential"
14-
unparsableURL = "(unparsable url)"
11+
"github.com/yuin/goldmark/util"
1512
)
1613

1714
type sanitizedError struct {
18-
err error
19-
replacer *strings.Replacer
15+
err error
2016
}
2117

2218
func (err sanitizedError) Error() string {
23-
return err.replacer.Replace(err.err.Error())
19+
return SanitizeCredentialURLs(err.err.Error())
2420
}
2521

26-
// NewSanitizedError wraps an error and replaces all old, new string pairs in the message text.
27-
func NewSanitizedError(err error, oldnew ...string) error {
28-
return sanitizedError{err: err, replacer: strings.NewReplacer(oldnew...)}
22+
func (err sanitizedError) Unwrap() error {
23+
return err.err
2924
}
3025

31-
// NewURLSanitizedError wraps an error and replaces the url credential or removes them.
32-
func NewURLSanitizedError(err error, u *url.URL, usePlaceholder bool) error {
33-
return sanitizedError{err: err, replacer: NewURLSanitizer(u, usePlaceholder)}
26+
// SanitizeErrorCredentialURLs wraps the error and make sure the returned error message doesn't contain sensitive credentials in URLs
27+
func SanitizeErrorCredentialURLs(err error) error {
28+
return sanitizedError{err: err}
3429
}
3530

36-
// NewStringURLSanitizedError wraps an error and replaces the url credential or removes them.
37-
// If the url can't get parsed it gets replaced with a placeholder string.
38-
func NewStringURLSanitizedError(err error, unsanitizedURL string, usePlaceholder bool) error {
39-
return sanitizedError{err: err, replacer: NewStringURLSanitizer(unsanitizedURL, usePlaceholder)}
40-
}
31+
const userPlaceholder = "sanitized-credential"
4132

42-
// NewURLSanitizer creates a replacer for the url with the credential sanitized or removed.
43-
func NewURLSanitizer(u *url.URL, usePlaceholder bool) *strings.Replacer {
44-
old := u.String()
33+
var schemeSep = []byte("://")
4534

46-
if u.User != nil && usePlaceholder {
47-
u.User = url.User(userPlaceholder)
48-
} else {
49-
u.User = nil
35+
// SanitizeCredentialURLs remove all credentials in URLs (starting with "scheme://") for the input string: "https://user:[email protected]" => "https://[email protected]"
36+
func SanitizeCredentialURLs(s string) string {
37+
bs := util.StringToReadOnlyBytes(s)
38+
schemeSepPos := bytes.Index(bs, schemeSep)
39+
if schemeSepPos == -1 || bytes.IndexByte(bs[schemeSepPos:], '@') == -1 {
40+
return s // fast return if there is no URL scheme or no userinfo
5041
}
51-
return strings.NewReplacer(old, u.String())
52-
}
53-
54-
// NewStringURLSanitizer creates a replacer for the url with the credential sanitized or removed.
55-
// If the url can't get parsed it gets replaced with a placeholder string
56-
func NewStringURLSanitizer(unsanitizedURL string, usePlaceholder bool) *strings.Replacer {
57-
u, err := url.Parse(unsanitizedURL)
58-
if err != nil {
59-
// don't log the error, since it might contain unsanitized URL.
60-
return strings.NewReplacer(unsanitizedURL, unparsableURL)
42+
out := make([]byte, 0, len(bs)+len(userPlaceholder))
43+
for schemeSepPos != -1 {
44+
schemeSepPos += 3 // skip the "://"
45+
sepAtPos := -1 // the possible '@' position: "https://foo@[^here]host"
46+
sepEndPos := schemeSepPos // the possible end position: "The https://host[^here] in log for test"
47+
sepLoop:
48+
for ; sepEndPos < len(bs); sepEndPos++ {
49+
c := bs[sepEndPos]
50+
if ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') || ('0' <= c && c <= '9') {
51+
continue
52+
}
53+
switch c {
54+
case '@':
55+
sepAtPos = sepEndPos
56+
case '-', '.', '_', '~', '!', '$', '&', '\'', '(', ')', '*', '+', ',', ';', '=', ':', '%':
57+
continue // due to RFC 3986, userinfo can contain - . _ ~ ! $ & ' ( ) * + , ; = : and any percent-encoded chars
58+
default:
59+
break sepLoop // if it is an invalid char for URL (eg: space, '/', and others), stop the loop
60+
}
61+
}
62+
// if there is '@', and the string is like "s://u@h", then hide the "u" part
63+
if sepAtPos != -1 && (schemeSepPos >= 4 && unicode.IsLetter(rune(bs[schemeSepPos-4]))) && sepAtPos-schemeSepPos > 0 && sepEndPos-sepAtPos > 0 {
64+
out = append(out, bs[:schemeSepPos]...)
65+
out = append(out, userPlaceholder...)
66+
out = append(out, bs[sepAtPos:sepEndPos]...)
67+
} else {
68+
out = append(out, bs[:sepEndPos]...)
69+
}
70+
bs = bs[sepEndPos:]
71+
schemeSepPos = bytes.Index(bs, schemeSep)
6172
}
62-
return NewURLSanitizer(u, usePlaceholder)
73+
out = append(out, bs...)
74+
return util.BytesToReadOnlyString(out)
6375
}

‎modules/util/sanitize_test.go‎

Lines changed: 25 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -11,154 +11,65 @@ import (
1111
"github.com/stretchr/testify/assert"
1212
)
1313

14-
func TestNewSanitizedError(t *testing.T) {
15-
err := errors.New("error while secret on test")
16-
err2 := NewSanitizedError(err)
17-
assert.Equal(t, err.Error(), err2.Error())
18-
19-
cases := []struct {
20-
input error
21-
oldnew []string
22-
expected string
23-
}{
24-
// case 0
25-
{
26-
errors.New("error while secret on test"),
27-
[]string{"secret", "replaced"},
28-
"error while replaced on test",
29-
},
30-
// case 1
31-
{
32-
errors.New("error while sec-ret on test"),
33-
[]string{"secret", "replaced"},
34-
"error while sec-ret on test",
35-
},
36-
}
37-
38-
for n, c := range cases {
39-
err := NewSanitizedError(c.input, c.oldnew...)
40-
41-
assert.Equal(t, c.expected, err.Error(), "case %d: error should match", n)
42-
}
14+
func TestSanitizeErrorCredentialURLs(t *testing.T) {
15+
err := errors.New("error with https://[email protected]")
16+
se := SanitizeErrorCredentialURLs(err)
17+
assert.Equal(t, "error with https://"+userPlaceholder+"@b.com", se.Error())
4318
}
4419

45-
func TestNewStringURLSanitizer(t *testing.T) {
20+
func TestSanitizeCredentialURLs(t *testing.T) {
4621
cases := []struct {
47-
input string
48-
placeholder bool
49-
expected string
22+
input string
23+
expected string
5024
}{
51-
// case 0
5225
{
5326
"https://github.com/go-gitea/test_repo.git",
54-
true,
5527
"https://github.com/go-gitea/test_repo.git",
5628
},
57-
// case 1
58-
{
59-
"https://github.com/go-gitea/test_repo.git",
60-
false,
61-
"https://github.com/go-gitea/test_repo.git",
62-
},
63-
// case 2
6429
{
6530
"https://[email protected]/go-gitea/test_repo.git",
66-
true,
6731
"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
6832
},
69-
// case 3
70-
{
71-
"https://[email protected]/go-gitea/test_repo.git",
72-
false,
73-
"https://github.com/go-gitea/test_repo.git",
74-
},
75-
// case 4
7633
{
7734
"https://user:[email protected]/go-gitea/test_repo.git",
78-
true,
7935
"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
8036
},
81-
// case 5
8237
{
83-
"https://user:[email protected]/go-gitea/test_repo.git",
84-
false,
85-
"https://github.com/go-gitea/test_repo.git",
38+
"ftp://x@",
39+
"ftp://" + userPlaceholder + "@",
8640
},
87-
// case 6
8841
{
89-
"https://gi\nthub.com/go-gitea/test_repo.git",
90-
false,
91-
unparsableURL,
42+
"ftp://x/@",
43+
"ftp://x/@",
9244
},
93-
}
94-
95-
for n, c := range cases {
96-
// uses NewURLSanitizer internally
97-
result := NewStringURLSanitizer(c.input, c.placeholder).Replace(c.input)
98-
99-
assert.Equal(t, c.expected, result, "case %d: error should match", n)
100-
}
101-
}
102-
103-
func TestNewStringURLSanitizedError(t *testing.T) {
104-
cases := []struct {
105-
input string
106-
placeholder bool
107-
expected string
108-
}{
109-
// case 0
11045
{
111-
"https://github.com/go-gitea/test_repo.git",
112-
true,
113-
"https://github.com/go-gitea/test_repo.git",
114-
},
115-
// case 1
116-
{
117-
"https://github.com/go-gitea/test_repo.git",
118-
false,
119-
"https://github.com/go-gitea/test_repo.git",
46+
"ftp://u@x/@", // test multiple @ chars
47+
"ftp://" + userPlaceholder + "@x/@",
12048
},
121-
// case 2
12249
{
123-
"https://[email protected]/go-gitea/test_repo.git",
124-
true,
125-
"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
50+
"😊ftp://u@x😊", // test unicode
51+
"😊ftp://" + userPlaceholder + "@x😊",
12652
},
127-
// case 3
12853
{
129-
"https://[email protected]/go-gitea/test_repo.git",
130-
false,
131-
"https://github.com/go-gitea/test_repo.git",
54+
"://@",
55+
"://@",
13256
},
133-
// case 4
13457
{
135-
"https://user:[email protected]/go-gitea/test_repo.git",
136-
true,
137-
"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
58+
"//u:p@h", // do not process URLs without explicit scheme, they are not treated as "valid" URLs because there is no scheme context in string
59+
"//u:p@h",
13860
},
139-
// case 5
14061
{
141-
"https://user:[email protected]/go-gitea/test_repo.git",
142-
false,
143-
"https://github.com/go-gitea/test_repo.git",
62+
"s://u@h", // the minimal pattern to be sanitized
63+
"s://" + userPlaceholder + "@h",
14464
},
145-
// case 6
14665
{
147-
"https://gi\nthub.com/go-gitea/test_repo.git",
148-
false,
149-
unparsableURL,
66+
"URLs in log https://u:b@h and https://u:b@h:80/, with https://h.com and [email protected]",
67+
"URLs in log https://" + userPlaceholder + "@h and https://" + userPlaceholder + "@h:80/, with https://h.com and [email protected]",
15068
},
15169
}
15270

153-
encloseText := func(input string) string {
154-
return "test " + input + " test"
155-
}
156-
15771
for n, c := range cases {
158-
err := errors.New(encloseText(c.input))
159-
160-
result := NewStringURLSanitizedError(err, c.input, c.placeholder)
161-
162-
assert.Equal(t, encloseText(c.expected), result.Error(), "case %d: error should match", n)
72+
result := SanitizeCredentialURLs(c.input)
73+
assert.Equal(t, c.expected, result, "case %d: error should match", n)
16374
}
16475
}

‎routers/api/v1/repo/migrate.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, rem
236236
case base.IsErrNotSupported(err):
237237
ctx.Error(http.StatusUnprocessableEntity, "", err)
238238
default:
239-
err = util.NewStringURLSanitizedError(err, remoteAddr, true)
239+
err = util.SanitizeErrorCredentialURLs(err)
240240
if strings.Contains(err.Error(), "Authentication failed") ||
241241
strings.Contains(err.Error(), "Bad credentials") ||
242242
strings.Contains(err.Error(), "could not read Username") {

‎routers/web/repo/migrate.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,7 @@ func handleMigrateError(ctx *context.Context, owner *user_model.User, err error,
106106
ctx.Data["Err_RepoName"] = true
107107
ctx.RenderWithErr(ctx.Tr("repo.form.name_pattern_not_allowed", err.(db.ErrNamePatternNotAllowed).Pattern), tpl, form)
108108
default:
109-
remoteAddr, _ := forms.ParseRemoteAddr(form.CloneAddr, form.AuthUsername, form.AuthPassword)
110-
err = util.NewStringURLSanitizedError(err, remoteAddr, true)
109+
err = util.SanitizeErrorCredentialURLs(err)
111110
if strings.Contains(err.Error(), "Authentication failed") ||
112111
strings.Contains(err.Error(), "Bad credentials") ||
113112
strings.Contains(err.Error(), "could not read Username") {

0 commit comments

Comments
 (0)