From 1202b9acc96e31d0dc2bd0a7e80e22e0de8f5ea6 Mon Sep 17 00:00:00 2001 From: Rogee Date: Sat, 10 Oct 2026 05:22:36 +0800 Subject: [PATCH] fix: redact single encoded forms and retain partial decode failures safely --- internal/logging/redact.go | 3 ++ internal/logging/text.go | 19 +++++---- internal/logging/text_security_test.go | 55 ++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/internal/logging/redact.go b/internal/logging/redact.go index 1f067b3..63465f1 100644 --- a/internal/logging/redact.go +++ b/internal/logging/redact.go @@ -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)) diff --git a/internal/logging/text.go b/internal/logging/text.go index 357efb4..425f782 100644 --- a/internal/logging/text.go +++ b/internal/logging/text.go @@ -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 { diff --git a/internal/logging/text_security_test.go b/internal/logging/text_security_test.go index 0ec2221..f45f370 100644 --- a/internal/logging/text_security_test.go +++ b/internal/logging/text_security_test.go @@ -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"