diff --git a/internal/delivery/target_redact.go b/internal/delivery/target_redact.go index a9050db..267e584 100644 --- a/internal/delivery/target_redact.go +++ b/internal/delivery/target_redact.go @@ -162,18 +162,22 @@ func targetSecrets(t *database.Target) []string { } // urlSecrets returns the substrings of a destination URL that -// must not survive into a rendered page: the whole URL, the -// parts of it MaskURL elides, and any userinfo. +// must not survive into a rendered page: the whole URL; its +// path, unless that is empty or "/"; its query string, and the +// request URI that carries it, which a remote echoing the +// request line shows even when the URL has no path; and its +// userinfo and password. // -// No length floor is applied to the path, and none to the -// userinfo. A short path or a four-byte username is treated as -// a credential exactly like a long one, because the field takes -// an arbitrary URL and no part of it can be assumed non-secret — -// the same rule MaskURL applies. headerSecrets does carry a -// floor, and the difference is deliberate: a header is picked -// out by a name-shaped guess and its value may be ordinary -// text, whereas a URL's path and userinfo are credential -// material by position. +// No length floor is applied to the path, the query string or +// the userinfo. A short path or a four-byte username is +// treated as a credential exactly like a long one, because the +// field takes an arbitrary URL and no part of it can be +// assumed non-secret — the same rule MaskURL applies. +// headerSecrets does carry a floor, and the difference is +// deliberate: a header is picked out by a name-shaped guess +// and its value may be ordinary text, whereas a URL's path, +// query string and userinfo are credential material by +// position. func urlSecrets(raw string) []string { raw = strings.TrimSpace(raw) if raw == "" { @@ -188,12 +192,11 @@ func urlSecrets(raw string) []string { } if parsed.Path != "" && parsed.Path != "/" { - requestURI := parsed.RequestURI() - secrets = append(secrets, requestURI) + secrets = append(secrets, parsed.EscapedPath()) + } - if escaped := parsed.EscapedPath(); escaped != requestURI { - secrets = append(secrets, escaped) - } + if parsed.RawQuery != "" { + secrets = append(secrets, parsed.RequestURI(), parsed.RawQuery) } if parsed.User != nil { diff --git a/internal/delivery/target_redact_test.go b/internal/delivery/target_redact_test.go index e28e302..52613de 100644 --- a/internal/delivery/target_redact_test.go +++ b/internal/delivery/target_redact_test.go @@ -202,6 +202,48 @@ func TestRedactor_RemovesHTTPURLQueryAndUserinfo(t *testing.T) { } } +// TestRedactor_RemovesEchoedQueryOfURLWithoutPath covers an +// HTTP target URL whose credential is all in its query string. +// Written with or without the "/", the request line sends it +// as "/?token=…", and a target passing the event's query string +// on sends that after an "&". The event's part stays visible: +// the event's page shows it anyway. +func TestRedactor_RemovesEchoedQueryOfURLWithoutPath(t *testing.T) { + t.Parallel() + + const secret = "s3cr3t" + + marker := delivery.RedactionMarker + + // An echoed request line, and what the event log shows of it. + echoes := map[string]string{ + "POST /?token=" + secret + " HTTP/1.1": "POST " + marker + + " HTTP/1.1", + "POST ?token=" + secret + " HTTP/1.1": "POST ?" + marker + + " HTTP/1.1", + "POST /?token=" + secret + "&a=1&b=2 HTTP/1.1": "POST " + + marker + "&a=1&b=2 HTTP/1.1", + "POST ?token=" + secret + "&a=1&b=2 HTTP/1.1": "POST ?" + + marker + "&a=1&b=2 HTTP/1.1", + } + + for _, dest := range []string{ + "https://example.com/?token=" + secret, + "https://example.com?token=" + secret, + } { + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"` + dest + `"}`, + }) + + for echoed, want := range echoes { + assert.Equal( + t, want, r.Redact(echoed), "%s: %s", dest, echoed, + ) + } + } +} + // TestRedactor_LeavesUnrelatedTextAlone pins that the // redactor matches literally: it does not guess at what a // secret looks like, so ordinary response content survives.