fix: close Ticket 11 hardening review gaps

2026-07-18 02:04:26 +08:00
parent f15ffc2301
commit ed8b527f1f
15 changed files with 1045 additions and 175 deletions
@@ -263,27 +263,140 @@ func normalizeBypassEntry(raw string) (string, error) {
 	return host, nil
 }

-var (
-	schemeCredentialPattern = regexp.MustCompile(`(?i)((?:https?|socks5h?)://)([^/@:\r\n]+):([^@\r\n]*)@`)
-	bareCredentialPattern   = regexp.MustCompile(`(?i)([^/@\s:]+):([^@/\s]*)@`)
-)
-
-// RedactText removes URL passwords from diagnostic text without requiring the
-// surrounding text to be a valid URL. The scheme-aware first pass deliberately
-// accepts spaces and other malformed password bytes up to @; diagnostics and
-// form validation must not expose a credential merely because parsing failed.
-func RedactText(value string) string {
-	value = schemeCredentialPattern.ReplaceAllString(value, `${1}${2}:REDACTED@`)
-	return bareCredentialPattern.ReplaceAllString(value, `${1}:REDACTED@`)
+type credentialSpan struct {
+	userinfoStart int
+	passwordStart int
+	at            int
 }

+// credentialSpans is deliberately a scanner rather than a URL regular
+// expression. Diagnostics routinely contain values rejected by net/url, and a
+// malformed password may contain whitespace, CR/LF, slashes, or additional @
+// signs. The scanner requires the comparatively stable user:...@host shape,
+// chooses an @ only when what follows can be an authority, and stops each
+// replacement at that @ so surrounding diagnostics remain untouched.
+func credentialSpans(value string) []credentialSpan {
+	var spans []credentialSpan
+	for cursor := 0; cursor < len(value); {
+		colon := strings.IndexByte(value[cursor:], ':')
+		if colon < 0 {
+			break
+		}
+		colon += cursor
+		if colon+2 < len(value) && value[colon+1:colon+3] == "//" {
+			cursor = colon + 3
+			continue
+		}
+		start := credentialUsernameStart(value, colon)
+		if start == colon || (colon+1 < len(value) && isAuthorityBoundary(value[colon+1]) && !hasSchemeAuthorityPrefix(value, start)) {
+			cursor = colon + 1
+			continue
+		}
+		at := credentialDelimiter(value, colon+1)
+		if at < 0 || hasNestedCredentialStart(value, colon+1, at) {
+			cursor = colon + 1
+			continue
+		}
+		spans = append(spans, credentialSpan{userinfoStart: start, passwordStart: colon + 1, at: at})
+		cursor = at + 1
+	}
+	return spans
+}
+
+func credentialUsernameStart(value string, colon int) int {
+	start := colon
+	for start > 0 && isCredentialUsernameByte(value[start-1]) {
+		start--
+	}
+	if start > 0 && (isCredentialUsernameByte(value[start-1]) || value[start-1] == '@') {
+		return colon
+	}
+	if start > 0 && value[start-1] == '/' && (start < 3 || value[start-3:start] != "://") {
+		return colon
+	}
+	return start
+}
+
+func hasSchemeAuthorityPrefix(value string, usernameStart int) bool {
+	return usernameStart >= 3 && value[usernameStart-3:usernameStart] == "://"
+}
+
+func isCredentialUsernameByte(value byte) bool {
+	return value >= 'a' && value <= 'z' || value >= 'A' && value <= 'Z' ||
+		value >= '0' && value <= '9' || strings.ContainsRune("._~%+-", rune(value))
+}
+
+func credentialDelimiter(value string, passwordStart int) int {
+	for at := passwordStart; at < len(value); at++ {
+		if value[at] != '@' {
+			continue
+		}
+		authorityStart := at + 1
+		end := authorityStart
+		for end < len(value) && !isAuthorityBoundary(value[end]) {
+			end++
+		}
+		if end == authorityStart || strings.ContainsRune(value[authorityStart:end], '@') {
+			continue
+		}
+		return at
+	}
+	return -1
+}
+
+func isAuthorityBoundary(value byte) bool {
+	switch value {
+	case ' ', '\t', '\r', '\n', '/', '?', '#', '\'', '"', '<', '>', '[', ']', '(', ')', '{', '}', ',', ';':
+		return true
+	default:
+		return value < 0x20 || value == 0x7f
+	}
+}
+
+func hasNestedCredentialStart(value string, passwordStart, at int) bool {
+	for nested := passwordStart; nested < at; nested++ {
+		if value[nested] != ':' {
+			continue
+		}
+		// A later scheme authority proves the earlier colon was prose (for
+		// example "Proxy URL:") rather than the credential separator.
+		if nested+2 < len(value) && value[nested+1:nested+3] == "//" {
+			return true
+		}
+		if credentialUsernameStart(value, nested) < nested && credentialDelimiter(value, nested+1) == at {
+			return true
+		}
+	}
+	return false
+}
+
+func redactCredentialSpans(value string, removeUsername bool) string {
+	spans := credentialSpans(value)
+	if len(spans) == 0 {
+		return value
+	}
+	var redacted strings.Builder
+	last := 0
+	for _, span := range spans {
+		redacted.WriteString(value[last:span.userinfoStart])
+		if !removeUsername {
+			redacted.WriteString(value[span.userinfoStart:span.passwordStart])
+		}
+		redacted.WriteString("REDACTED@")
+		last = span.at + 1
+	}
+	redacted.WriteString(value[last:])
+	return redacted.String()
+}
+
+// RedactText removes passwords from valid and malformed credential-bearing
+// text while retaining usernames for diagnostics where that behavior is useful.
+func RedactText(value string) string { return redactCredentialSpans(value, false) }
+
 // RedactUserinfoText removes usernames as well as passwords. Form frames use
 // this stricter boundary because even the identity of the probe source is
 // private while a Profile is being edited.
-func RedactUserinfoText(value string) string {
-	value = schemeCredentialPattern.ReplaceAllString(value, `${1}REDACTED@`)
-	return bareCredentialPattern.ReplaceAllString(value, `REDACTED@`)
-}
+func RedactUserinfoText(value string) string { return redactCredentialSpans(value, true) }

 func ObscureURL(value string) string {
 	u, err := url.Parse(value)
@@ -301,22 +414,16 @@ func ObscureURL(value string) string {
 // rune count. A text input clone can therefore retain its real cursor and
 // horizontal viewport positions without rendering either username or password.
 func ObscureURLForEditing(value string) string {
-	masked := maskCredentialMatches(value, schemeCredentialPattern, 4, 7)
-	return maskCredentialMatches(masked, bareCredentialPattern, 2, 5)
-}
-
-func maskCredentialMatches(value string, pattern *regexp.Regexp, userinfoStartGroup, userinfoEndGroup int) string {
-	matches := pattern.FindAllStringSubmatchIndex(value, -1)
-	if len(matches) == 0 {
+	spans := credentialSpans(value)
+	if len(spans) == 0 {
 		return value
 	}
 	var masked strings.Builder
 	last := 0
-	for _, match := range matches {
-		userinfoStart, userinfoEnd := match[userinfoStartGroup], match[userinfoEndGroup]
-		masked.WriteString(value[last:userinfoStart])
-		masked.WriteString(strings.Repeat("•", len([]rune(value[userinfoStart:userinfoEnd]))))
-		last = userinfoEnd
+	for _, span := range spans {
+		masked.WriteString(value[last:span.userinfoStart])
+		masked.WriteString(strings.Repeat("•", len([]rune(value[span.userinfoStart:span.at]))))
+		last = span.at
 	}
 	masked.WriteString(value[last:])
 	return masked.String()
@@ -0,0 +1,140 @@
+package profile
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestRedactTextConservativelyScansMalformedCredentials(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "carriage return in password",
+			input:    "before http://alice:first\rsecond@proxy.example:8080 after",
+			expected: "before http://alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "line feed in password",
+			input:    "before http://alice:first\nsecond@proxy.example:8080 after",
+			expected: "before http://alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "spaces in password",
+			input:    "before http://alice:two words here@proxy.example:8080 after",
+			expected: "before http://alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "slash in password",
+			input:    "before http://alice:part/other@proxy.example:8080 after",
+			expected: "before http://alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "unsupported scheme and CRLF space slash password",
+			input:    "probe failed [ftp+ssh://alice:line one/line\r\ntwo@proxy.example:8080] retrying",
+			expected: "probe failed [ftp+ssh://alice:REDACTED@proxy.example:8080] retrying",
+		},
+		{
+			name:     "bare malformed userinfo",
+			input:    "before alice:bare secret/part@proxy.example:8080 after",
+			expected: "before alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "multiple at signs",
+			input:    "before http://alice:sec@ret@proxy.example:8080 after",
+			expected: "before http://alice:REDACTED@proxy.example:8080 after",
+		},
+		{
+			name:     "multiple URLs on one line",
+			input:    "left http://alice:first@one.example:1 middle invalid-scheme://bob:second value@two.example:2 right",
+			expected: "left http://alice:REDACTED@one.example:1 middle invalid-scheme://bob:REDACTED@two.example:2 right",
+		},
+		{
+			name:     "prose labels before URLs",
+			input:    "Proxy URL: odd+scheme://alice:first@one.example:1; fallback URL: invalid://bob:second@two.example:2",
+			expected: "Proxy URL: odd+scheme://alice:REDACTED@one.example:1; fallback URL: invalid://bob:REDACTED@two.example:2",
+		},
+		{
+			name:     "surrounding diagnostics with colons remain",
+			input:    "error: connect alice:secret@proxy.example:80: refused; code: EHOSTUNREACH",
+			expected: "error: connect alice:REDACTED@proxy.example:80: refused; code: EHOSTUNREACH",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got := RedactText(test.input); got != test.expected {
+				t.Fatalf("RedactText() = %q, want %q", got, test.expected)
+			}
+		})
+	}
+}
+
+func TestRedactTextDoesNotSwallowUnrelatedTextAcrossLines(t *testing.T) {
+	input := "diagnostic:\ncontact admin@example.com\nnext diagnostic remains"
+	if got := RedactText(input); got != input {
+		t.Fatalf("RedactText crossed a diagnostic line boundary: got %q, want %q", got, input)
+	}
+
+	input = "first http://alice:first@one.example:1\nsecond http://bob:second@two.example:2\ntrailing diagnostic"
+	want := "first http://alice:REDACTED@one.example:1\nsecond http://bob:REDACTED@two.example:2\ntrailing diagnostic"
+	if got := RedactText(input); got != want {
+		t.Fatalf("RedactText swallowed across independent URL lines: got %q, want %q", got, want)
+	}
+}
+
+func TestRedactUserinfoTextRemovesCompleteMalformedUserinfoAndPreservesContext(t *testing.T) {
+	cases := []struct {
+		input    string
+		userinfo string
+		expected string
+	}{
+		{
+			input:    "BEGIN unsupported://alice:secret words/part@proxy.example:80 END",
+			userinfo: "alice:secret words/part",
+			expected: "BEGIN unsupported://REDACTED@proxy.example:80 END",
+		},
+		{
+			input:    "BEGIN alice:secret\r\nwords@proxy.example:80 END",
+			userinfo: "alice:secret\r\nwords",
+			expected: "BEGIN REDACTED@proxy.example:80 END",
+		},
+		{
+			input:    "BEGIN http://alice:sec@ret@proxy.example:80 END",
+			userinfo: "alice:sec@ret",
+			expected: "BEGIN http://REDACTED@proxy.example:80 END",
+		},
+	}
+
+	for _, test := range cases {
+		got := RedactUserinfoText(test.input)
+		if strings.Contains(got, test.userinfo) {
+			t.Fatalf("RedactUserinfoText retained userinfo %q in %q", test.userinfo, got)
+		}
+		if got != test.expected {
+			t.Fatalf("RedactUserinfoText(%q) = %q, want %q", test.input, got, test.expected)
+		}
+		if !strings.HasPrefix(got, "BEGIN ") || !strings.HasSuffix(got, " END") {
+			t.Fatalf("RedactUserinfoText swallowed surrounding diagnostics: %q", got)
+		}
+	}
+}
+
+func TestRedactionPreservesArbitrarySurroundingDiagnostics(t *testing.T) {
+	prefixes := []string{"", "error: ", "[target apt] ", "first URL failed; "}
+	suffixes := []string{"", " (retrying)", "; next check continues", "\nstack trace follows"}
+	for _, prefix := range prefixes {
+		for _, suffix := range suffixes {
+			input := prefix + "odd+scheme://user:malformed secret/path@host.example:9" + suffix
+			got := RedactText(input)
+			if strings.Contains(got, "malformed secret/path") {
+				t.Fatalf("secret survived for prefix=%q suffix=%q: %q", prefix, suffix, got)
+			}
+			if !strings.HasPrefix(got, prefix) || !strings.HasSuffix(got, suffix) {
+				t.Fatalf("surrounding diagnostics changed for prefix=%q suffix=%q: %q", prefix, suffix, got)
+			}
+		}
+	}
+}