fix(auth): keep the consent URI out of ConsentRequiredError's message - #1519
Conversation
418749d to
f03f973
Compare
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Nothing blocks. The change does what it says and the evidence for it is unusually tight — one nit inline, and two merge preconditions that are not about the code.
The premise checks out end to end. base_flow.go:1416 turns a tool error into map[string]any{"error": err.Error()}, and that map is the FunctionResponse payload the model reads and the session store keeps, so the message really was the leak channel. Worth stating precisely, though: the producer side, auth/gcp client.go:215, has no non-test caller anywhere in this repository, so the two halves are only joined by a downstream integrator wiring a provider through mcptoolset.Config.Auth. Both halves are real. The join is out of tree, which is the normal shape for a library and does not weaken the fix.
Applying only your three test files onto the merge-base fails on the URI in two independent places — the direct message and the %w wrap at transport.go:61 — and both pass with the production change. Those are the only two observable message strings in the repo, and exact equality pins both. apidiff is empty on ./auth and on ./auth/gcp, against the same base CI will compute.
Three things I looked at hard and came away satisfied with, since a reviewer is more likely to argue the other way on each:
- Exact equality over a negative check is right for a stronger reason than the comment gives. A URI-substring check would not catch a
Nonceleak at all — the fixture nonce is"n". The exact check catches a nonce leak, a key leak, and any field added later. - Documenting the
json.Marshalresidue instead of enforcing it looks like the only in-scope option. Unexporting the fields breaks every consumer reading them offerrors.As, and a redactingMarshalJSONwould also close the resume path for anything round-tripping the error. Nothing in the tree marshals it today: nolog/sloganywhere, every%#vsite is a test, and all four writers ofmap[string]any{"error": …}go througherr.Error()first. - Not redacting
AuthURIthe way service text is redacted reads as coherent doctrine rather than an inconsistency. The other errors sanitize text that is hostile —%qplus a length cap. This one is sensitive, and omission is the stronger treatment.
Two preconditions before merge, neither a code issue: the branch is behind main, and it conflicts with open #1465 on auth/gcp/client_test.go. Merge order decides who rebases.
On the sibling-implementation question: adk-python has no counterpart error. It signals consent by returning an AuthCredential that carries auth_uri rather than by raising, so there is no message there that could leak the URI — auth_credential.py:109, at b0180620. One detail from the same file that may interest you: on that model client_secret and private_key are declared Annotated[..., Field(repr=False)] and auth_uri is not, so the URI is not treated as secret-bearing there. Nothing for this PR to do about it — it just means the doctrine you are setting here is new rather than inherited.
A *ConsentRequiredError reaches the model as a tool's error and is persisted as a session event. Its message carried AuthURI, and that URI carries the state and the consent nonce that bind the credential. A 3-legged authorization URI also normally carries the acting user in a login_hint parameter. Drop the URI from Error(). It stays on the AuthURI field, which is how consumers already read it, and the only assertion on the message text in the tree was the test updated here. Document on the fields that AuthURI and Nonce go to the acting user and nowhere else — not a log, not a span, and not through json.Marshal, which publishes them whatever Error() says.
…tic readable Two gaps that only appear once Error() stops carrying the URI. Transport.RoundTrip builds a new message around the provider's error, so the invariant lives in two places and only one of them was asserted. Reintroducing the URI at the wrap site left the whole auth package green while the caller received the state, the nonce and the login_hint. Asserted exactly rather than by absence of the URI: a negative check goes vacuous the moment the fixture above it changes. TestRetrieveCredential printed the consent error with %+v, which resolves through Error() rather than reflecting over the fields. With Error() now a constant, a mismatch reported neither the AuthURI nor the Nonce it actually got.
f03f973 to
6466509
Compare
Problem
A
*ConsentRequiredErrorbecomes a tool's error, which is fed back to the model and persisted as a session event. Its message carriedAuthURI, and that URI carries thestateand the consent nonce that bind the credential. A 3-legged authorization URI also normally carries the acting user in alogin_hintparameter, so the message published an end-user identifier as well.Summary
Error()no longer prints the URI. It stays on theAuthURIfield, which is how consumers already read it — the only assertion on the message text anywhere in the tree was the one updated here.AuthURIandNoncenow say on the field that they go to the acting user and nowhere else: not a log, not a span, and not throughjson.Marshal, which publishes them whateverError()returns.AuthURIis deliberately not redacted the way service text is in other errors, because the identifier in it is what makes the URL work.apidiffreports the change as additive, since no signature moved and the message text is not part of what it compares.First of a five-part split of #1173. Nothing is stacked below this one.