fix: redact single encoded forms and retain partial decode failures safely
This commit is contained in:
@@ -203,6 +203,9 @@ func redactErrorJSON(s string) string {
|
||||
|
||||
func cleanString(s string) string {
|
||||
s = privatePEMInText.ReplaceAllString(s, redacted)
|
||||
if form, ok := redactForm(s); ok {
|
||||
s = form
|
||||
}
|
||||
// Scalar messages/errors and HTTP text share the same labelled-content
|
||||
// protection, rather than protecting conversational blocks only in bodies.
|
||||
s = redactConversationLines(redactMarkup(s))
|
||||
|
||||
@@ -88,26 +88,31 @@ func voidMarkupElement(name string) bool {
|
||||
// A form can be identified without guessing semantic values. Untouched pairs
|
||||
// preserve their original encoding and order, including duplicate field names.
|
||||
func redactForm(body string) (string, bool) {
|
||||
if strings.ContainsAny(body, "\r\n<>") || !strings.Contains(body, "&") || !strings.Contains(body, "=") {
|
||||
if strings.ContainsAny(body, "\r\n<>") || !strings.Contains(body, "=") {
|
||||
return body, false
|
||||
}
|
||||
parts := strings.Split(body, "&")
|
||||
for i, part := range parts {
|
||||
key, value, ok := strings.Cut(part, "=")
|
||||
if !ok {
|
||||
return body, false
|
||||
}
|
||||
key, value, hasValue := strings.Cut(part, "=")
|
||||
decodedKey, err := url.QueryUnescape(key)
|
||||
if err != nil {
|
||||
return body, false
|
||||
// A failed field decode must not restore earlier sensitive pairs or
|
||||
// expose the unclassifiable field. Keep the failure explicit in the copy.
|
||||
parts[i] = url.QueryEscape("[REDACTED_INVALID_FORM_KEY]")
|
||||
continue
|
||||
}
|
||||
if sensitive(decodedKey) {
|
||||
parts[i] = key + "=" + url.QueryEscape(redacted)
|
||||
continue
|
||||
}
|
||||
// Bare flags are valid empty-value form fields, not a parse failure.
|
||||
if !hasValue {
|
||||
continue
|
||||
}
|
||||
decoded, err := url.QueryUnescape(value)
|
||||
if err != nil {
|
||||
return body, false
|
||||
parts[i] = key + "=" + url.QueryEscape("[REDACTED_INVALID_FORM_VALUE]")
|
||||
continue
|
||||
}
|
||||
safe := cleanString(redactConversationLines(decoded))
|
||||
if safe != decoded {
|
||||
|
||||
@@ -61,6 +61,61 @@ func TestContentFieldClassificationAndEveryLoggingPathAgree(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestPercentEncodedSingleAndMixedFormsKeepWireBytesAndHideSecrets(t *testing.T) {
|
||||
for _, body := range []string{
|
||||
"%74oken=private-access-token",
|
||||
"%6dessages=private-user-turn+private-assistant-turn",
|
||||
"task_id=4&%74oken=private-access-token&status=ready",
|
||||
"task_id=4&%74oken=private-access-token&broken%zz=x&status=ready",
|
||||
"flag&%6dessages=private-user-turn+private-assistant-turn&status=ready",
|
||||
"%74oken=private-access-token&field=x%zz&status=ready",
|
||||
"broken%zz=private-access-token&status=ready",
|
||||
} {
|
||||
for _, debug := range []bool{false, true} {
|
||||
out := installTestLogger(t, debug)
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
got, err := io.ReadAll(r.Body)
|
||||
if err != nil || string(got) != body {
|
||||
t.Error("wire request changed", err)
|
||||
}
|
||||
w.Header().Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
_, _ = io.WriteString(w, body)
|
||||
}))
|
||||
req, err := http.NewRequest(http.MethodPost, ts.URL, strings.NewReader(body))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
response, err := HTTPClient(ts.Client(), "test.form", "upstream").Do(req)
|
||||
if err != nil {
|
||||
ts.Close()
|
||||
t.Fatal(err)
|
||||
}
|
||||
got, err := io.ReadAll(response.Body)
|
||||
_ = response.Body.Close()
|
||||
ts.Close()
|
||||
if err != nil || string(got) != body {
|
||||
t.Fatal("wire response changed", err)
|
||||
}
|
||||
slog.Error("form diagnostic", "error", errors.New(body), "detail", body)
|
||||
text := out.String()
|
||||
for _, secret := range []string{"private-access-token", "private-user-turn", "private-assistant-turn"} {
|
||||
if strings.Contains(text, secret) {
|
||||
t.Fatalf("form %q debug %v leak: %s", body, debug, text)
|
||||
}
|
||||
}
|
||||
for _, keep := range []string{"task_id=4", "status=ready"} {
|
||||
if strings.Contains(body, keep) && !strings.Contains(text, keep) {
|
||||
t.Fatal("diagnostic lost", text)
|
||||
}
|
||||
}
|
||||
if strings.Contains(body, "%zz") && !strings.Contains(text, "INVALID_FORM") {
|
||||
t.Fatal("malformed encoding must be explicit", text)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestQuotedContentLabelsEstablishOwnershipOnEveryLoggingPath(t *testing.T) {
|
||||
for _, label := range []string{`"messages"`, `'conversation'`, "`history`", `“prompt”`, `‘messages’`, `\"messages\"`, `"m\u0065ssages"`, `"m\qssages"`} {
|
||||
value := "upstream failed: " + label + ":\nrole: user\nmessage: private-user-turn\nrole: assistant\nmessage: private-assistant-turn\nstatus=retry_pending\ntask_id=4\nattempts=2"
|
||||
|
||||
Reference in New Issue
Block a user