auth/gcp: a UserID contained in the ContinueURI leaves most of the URI in error text
The error-body scrub in auth/gcp removes the request's own UserID and ContinueURI from service error text before that text is quoted into an error. Where the UserID is a substring of the ContinueURI and the service echoes the URI with \/ escapes, the scrub cannot match the URI at all, matches the short UserID inside it, and writes [redacted] into the middle of the URI. What comes back shows 31 of the URI's 34 characters, so the redirect URI is effectively disclosed.
This is on the error-redaction path added in #1520 and is not present on main.
go test -run TestContinueURISurvivesTheUserIDMarker ./auth/gcp/func TestContinueURISurvivesTheUserIDMarker(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusForbidden)
_, _ = io.WriteString(w, `error at https:\/\/my-app.test\/oauth\/callback`)
}))
defer srv.Close()
_, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{
Resource: authProviderResource,
UserID: "app",
ContinueURI: "https://my-app.test/oauth/callback",
})
t.Logf("%v", err)
}It logs:
gcp: credentials service returned status 403: "error at https:\\/\\/my-[redacted].test\\/oauth\\/callback" (resource "projects/p/locations/l/authProviders/ap")A UserID of alice with a ContinueURI of https://cb.test/u/alice/done behaves the same way. The acting user itself is redacted correctly in every shape tried, so what escapes is the redirect URI.
Two properties of the current code constrain any solution.
recoverablesplits the text on the literal marker at redact.go:183 before decoding it, so a value that spans the marker is examined as two halves and neither half matches. Dropping the split is not free: the marker is text this package inserted rather than anything the service disclosed, and treating it as disclosed would report a secret in text that carries none.- The doc comment at redact.go:393-399 says overlapping candidates are resolved earliest-first then longest, "which is what keeps a ContinueURI carrying the user id from being split by the user id". That holds for the unescaped spelling. The escaped URI never becomes a candidate, so the resolution has nothing to order, and the comment reads as covering a case it does not reach.
The letter of the documented contract is not broken. It promises that no secret is recoverable from the returned string by this package's decoder, and the decoder does not reconstruct the URI from that text. A person reading the error does.
A fix should demonstrate that the URI is not left partially visible when the UserID is a substring of it, in the escaped and unescaped spellings alike, and that a body naming only the UserID still comes back with the UserID redacted rather than withheld entirely.
Source: google/adk-go