Hide sensitive content on admin panel progress monitor

[lunny]

No description provided.

[zeripath]

Honestly I am suspicious that the correct thing to do here is to simply provide the correct description for these processes.

There are only three of them and we should be fine from there on.

[wxiaoguang]

Honestly I am suspicious that the correct thing to do here is to simply provide the correct description for these processes.

There are only three of them and we should be fine from there on.

@zeripath To keep the backport simple and avoid conflicts with other RunInDir refactoring, I am sure we do not need the AddURLArgument.

The only change should be done in RunWithContext:

	desc := c.desc
	if desc == "" {
		args := c.args
		var argSensitiveUrlIndices []int
		for i, arg := range c.args {
			if strings.Index(arg, "://") != -1 && strings.IndexByte(arg, '@') != -1 {
				argSensitiveUrlIndices = append(argSensitiveUrlIndices, i)
			}
		}
		if len(argSensitiveUrlIndices) > 0 {
			args = make([]string, len(c.args))
			copy(args, c.args)
			for _, urlArgIndex := range argSensitiveUrlIndices {
				args[urlArgIndex] = util.NewStringURLSanitizer(args[urlArgIndex], true).Replace(args[urlArgIndex])
			}
		}
		desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(args, " "), rc.Dir)
	}

@lunny For 1.16 backport, the code above is enough, that's the minimal change.

For 1.17 , I think we can use a more general SanitizeCredentialURLs, which can be used to process all URLs, expand the details, including tests and demo. Then remove most legacy NewStringURLSanitizeXxx, which is too complex.

Why it's faster: it doesn't do the heavy url.Parse, and it only allocates one strings.Builder{} at most.

Expand below ⬇️

func SanitizeCredentialURLs(msg string) string {
	schemeSepPos := strings.Index(msg, "://")
	if schemeSepPos == -1 && strings.IndexByte(msg, '@') == -1 {
		return msg // fast return if there is no URL scheme and no userinfo
	}
	bd := strings.Builder{}
	for schemeSepPos != -1 {
		schemeSepPos += 3 // skip the "://"
		sepAtPos := -1    // the possible '@' position: "https://foo@[^here]host"
		sepEndPos := -1   // the possible end position: "The https://host[^here] in log for test"
	sepLoop:
		for sepEndPos = schemeSepPos; sepEndPos < len(msg); sepEndPos++ {
			c := msg[sepEndPos]
			if ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') || ('0' <= c && c <= '9') {
				continue
			}
			switch c {
			case '@':
				sepAtPos = sepEndPos
			case '-', '.', '_', ':', '~', '!', '$', '&', '\'', '(', ')', '*', '+', ',', ';', '=', '%':
				continue // due to RFC 3986, userinfo can contain - . _ ~ ! $ & ' ( ) * + , ; = : and any percent-encoded chars
			default:
				break sepLoop
			}
		}
		if sepAtPos != -1 {
			bd.WriteString(msg[:schemeSepPos])
			bd.WriteString(userPlaceholder)
			bd.WriteString(msg[sepAtPos:sepEndPos])
		} else {
			bd.WriteString(msg[:sepEndPos])
		}
		msg = msg[sepEndPos:]
		schemeSepPos = strings.Index(msg, "://")
	}
	bd.WriteString(msg)
	return bd.String()
}


func TestSanitizeCredentialURLs(t *testing.T) {
	cases := []struct {
		input    string
		expected string
	}{
		{
			"https://github.com/go-gitea/test_repo.git",
			"https://github.com/go-gitea/test_repo.git",
		},
		{
			"https://mytoken@github.com/go-gitea/test_repo.git",
			"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
		},
		{
			"https://user:password@github.com/go-gitea/test_repo.git",
			"https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git",
		},
		{
			"URLs in log https://u:b@h and https://u:b@h:80/, with https://h.com and u@h.com",
			"URLs in log https://" + userPlaceholder + "@h and https://" + userPlaceholder + "@h:80/, with https://h.com and u@h.com",
		},
	}

	for n, c := range cases {
		result := SanitizeCredentialURLs(c.input)
		assert.Equal(t, c.expected, result, "case %d: error should match", n)
	}
}

[lunny]

@wxiaoguang @zeripath both done.

[wxiaoguang]

Generally LGTM.

I still worry that these changes to RunInDir will conflict with an other PR a a lot, while we might have a chance to avoid these conflicts at the moment. (Just an opinion, either is fine)

• Replace RunInDir with RunWithContext #18978 Replace RunInDir with RunWithContext

[zeripath]

@wxiaoguang that sanitizer function looks potentially interesting but I think you could improve further.

1. String builders still cause a lot of allocations and still have the final alloc to make the string. We have an actual max size here and so can actually get away with 2 allocations. Create an initial []byte len(input)+Len(placeholder) then at the end string it.
2. I have a feeling that all of those string slices also create new strings - if you use unsafe to cast the inputs to []byte initially and read them only from there on you can totally avoid that. (util.StringToReadOnlyBytes from goldmark util)
3. If possible avoid the iteration byte-by-byte through the whole string by using bytes.indexof. Counterintuitively this can be much quicker than iterating character by character. I think this is because bytes.indexof can often avoid the related bounds checks. A well considered regexp maybe able to do so similarly. Running bytes.indexof twice can still be quicker than iteration.

[zeripath]

I've fixed the conflicts and lint.

[zeripath]

I think there's still potential for an argument to be incorrectly assessed as an URL when it is not an URL here - this would then lead to that argument being turned into an unprocessable url - but... this is just a label that is supposed to be helpful to explain what this process is about and some mangling should not be a problem.

[zeripath]

Just need to adjust the descs. Do not merge yet.

[zeripath]

Please send backport

[wxiaoguang]

@wxiaoguang that sanitizer function looks potentially interesting but I think you could improve further.

1. String builders still cause a lot of allocations and still have the final alloc to make the string. We have an actual max size here and so can actually get away with 2 allocations. Create an initial []byte len(input)+Len(placeholder) then at the end string it.

2. I have a feeling that all of those string slices also create new strings - if you use unsafe to cast the inputs to []byte initially and read them only from there on you can totally avoid that. (util.StringToReadOnlyBytes from goldmark util)

3. If possible avoid the iteration byte-by-byte through the whole string by using bytes.indexof. Counterintuitively this can be much quicker than iterating character by character. I think this is because bytes.indexof can often avoid the related bounds checks. A well considered regexp maybe able to do so similarly. Running bytes.indexof twice can still be quicker than iteration.

Most done in

• Use a more general (and faster) method to sanitize URLs with credentials #19239