Use a more general (and faster) method to sanitize URLs with credentials #19239
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,59 +5,69 @@ | |
| package util | ||
|
|
||
| import ( | ||
| "bytes" | ||
|
|
||
| "github.com/yuin/goldmark/util" | ||
| ) | ||
|
|
||
| type sanitizedError struct { | ||
| err error | ||
| } | ||
|
|
||
| func (err sanitizedError) Error() string { | ||
| return SanitizeCredentialURLs(err.err.Error()) | ||
| } | ||
|
|
||
| func (err sanitizedError) Unwrap() error { | ||
| return err.err | ||
| } | ||
|
|
||
| // SanitizeErrorCredentialURLs wraps the error and make sure the returned error message doesn't contain sensitive credentials in URLs | ||
| func SanitizeErrorCredentialURLs(err error) error { | ||
| return sanitizedError{err: err} | ||
| } | ||
|
|
||
| const userPlaceholder = "sanitized-credential" | ||
|
|
||
| var schemeSep = []byte("://") | ||
|
|
||
| // SanitizeCredentialURLs remove all credentials in URLs for the input string: "https://user:[email protected]" => "https://[email protected]" | ||
| func SanitizeCredentialURLs(s string) string { | ||
| bs := util.StringToReadOnlyBytes(s) | ||
| schemeSepPos := bytes.Index(bs, schemeSep) | ||
| if schemeSepPos == -1 || bytes.IndexByte(bs[schemeSepPos:], '@') == -1 { | ||
| return s // fast return if there is no URL scheme or no userinfo | ||
| } | ||
| out := make([]byte, 0, len(bs)+len(userPlaceholder)) | ||
| for schemeSepPos != -1 { | ||
| schemeSepPos += 3 // skip the "://" | ||
| sepAtPos := -1 // the possible '@' position: "https://foo@[^here]host" | ||
| sepEndPos := schemeSepPos // the possible end position: "The https://host[^here] in log for test" | ||
| sepLoop: | ||
| for ; sepEndPos < len(bs); sepEndPos++ { | ||
| c := bs[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 it is an invalid char for URL (eg: space, '/', and others), stop the loop | ||
| } | ||
| } | ||
| if sepAtPos != -1 { | ||
| out = append(out, bs[:schemeSepPos]...) | ||
| out = append(out, userPlaceholder...) | ||
| out = append(out, bs[sepAtPos:sepEndPos]...) | ||
| } else { | ||
| out = append(out, bs[:sepEndPos]...) | ||
| } | ||
| bs = bs[sepEndPos:] | ||
| schemeSepPos = bytes.Index(bs, schemeSep) | ||
| } | ||
| out = append(out, bs...) | ||
| return util.BytesToReadOnlyString(out) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,154 +11,53 @@ import ( | |
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSanitizeErrorCredentialURLs(t *testing.T) { | ||
| err := errors.New("error with https://[email protected]") | ||
| se := SanitizeErrorCredentialURLs(err) | ||
| assert.Equal(t, "error with https://"+userPlaceholder+"@b.com", se.Error()) | ||
| } | ||
|
|
||
| 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://[email protected]/go-gitea/test_repo.git", | ||
| "https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git", | ||
| }, | ||
|
There was a problem hiding this comment. Which case? I think I have covered most. There was a problem hiding this comment. I mean why we delete these original test cases? There was a problem hiding this comment. All code are rewritten, all new cases cover old ones. If you feel which is missing, please just point out and add it. There was a problem hiding this comment. And many cases are indeed the old cases, for example, these |
||
| { | ||
| "https://user:[email protected]/go-gitea/test_repo.git", | ||
| "https://" + userPlaceholder + "@github.com/go-gitea/test_repo.git", | ||
| }, | ||
| { | ||
| "ftp://x@", | ||
| "ftp://" + userPlaceholder + "@", | ||
| }, | ||
| { | ||
| "ftp://x/@", | ||
| "ftp://x/@", | ||
| }, | ||
| { | ||
| "ftp://@x/@", // test multiple @ chars | ||
| "ftp://" + userPlaceholder + "@x/@", | ||
| }, | ||
| { | ||
| "😊ftp://@x😊", // test unicode | ||
| "😊ftp://" + userPlaceholder + "@x😊", | ||
| }, | ||
| { | ||
| "URLs in log https://u:b@h and https://u:b@h:80/, with https://h.com and [email protected]", | ||
| "URLs in log https://" + userPlaceholder + "@h and https://" + userPlaceholder + "@h:80/, with https://h.com and [email protected]", | ||
| }, | ||
| } | ||
|
|
||
| for n, c := range cases { | ||
| result := SanitizeCredentialURLs(c.input) | ||
| assert.Equal(t, c.expected, result, "case %d: error should match", n) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code only removes the credential from the first URL it finds in the string
If we want to strip out credentials from all urls in the string we're gonna need to do something else. (Possibly iterate across all "://" in the string however see below...)
One other issue is that there are URLs which do not have the ":", for example
//username:[email protected]which says use the current protocol. I think/\username:[email protected]also works.IMO I don't think we need to necessarily sanitize these - we should probably just make it clear that we're not going to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will add it to comment. IMO "//xxxx" couldn't be treated as a valid URL in strings. It's only valid in a context with scheme already (say, inside a browser)
And
/\is not standard, it's just a browser's dirty hack. Golang's url.Parse doesn't support/\either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This code only removes the credential from the first URL it finds in the string"
No, it removes all. See the test case