Repository navigation
fix(server): drop caller-sent _auth_* arguments before stamping the activity identity - #1789
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
…ctivity identity injectAuthMetadata copied the caller's arguments and then added only the identity keys the auth context had. An agent token carries no user_id, and an admin caller carries no agent_name or token_prefix, so a caller that sent those keys itself kept them. internal/runtime copies _auth_user_id and _auth_user_email onto ActivityRecord.UserID/UserEmail, and the agent filter and token name read _auth_agent_name, so the activity log recorded the forged identity. Strip the _auth_* keys from the copy before adding the real ones, the same way activityArgsFromRequest already does for the management tools.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
injectAuthMetadata(internal/server/mcp.go) builds the argument map that gets stored on the activity record. It copies the caller's arguments, then adds the_auth_*identity keys that the auth context has. It adds only the keys that exist for that caller:auth_type,agent_nameandtoken_prefix, but notuser_idoruser_email;auth_type.So when the caller sends one of the missing keys itself, it survives. Those keys are what the activity log trusts for identity:
internal/runtime/activity_service.gocopies_auth_user_id/_auth_user_emailontoActivityRecord.UserID/UserEmail. That is the field the server-edition per-user activity view filters on._auth_agent_nameis what theagent_namefilter matches and whatEffectiveTokenNamefalls back to when the record has no token name, which is always the case for an admin caller.activityArgsFromRequestalready strips caller-sent_auth_*keys for the two management tools, for exactly this reason (see the comment there). The normalcall_tool_*, direct-mode,retrieve_toolsanddescribe_toolpaths go throughinjectAuthMetadataand did not.Trigger
Any connected MCP client, or a model steered by a tool result, calls a tool with extra arguments such as:
{"query": "x", "_auth_user_id": "<another user's id>", "_auth_agent_name": "ci-bot"}The activity row is then attributed to that user, or to the
ci-botagent token, instead of the real caller.Fix
Run
security.StripInternalArgson the copy before adding the real identity keys. The caller's ownargsmap, which is what gets forwarded upstream, is not changed. Activity views already strip_auth_*from the displayed arguments, so nothing visible is lost.Test
TestInjectAuthMetadata_DropsCallerSuppliedAuthKeys: an agent context with forged_auth_user_id,_auth_user_email,_auth_agent_nameand_auth_token_prefixarguments. The result must carry the token's real agent name and prefix, and no user id or email. With no auth context, the forged keys must also be gone. The original map must be unchanged.Commands run
CGO_ENABLED=0 go test -count=1 -run 'TestInjectAuthMetadata|TestGetAuthMetadata|Activity' ./internal/server/: okmain, and again with the one-line fix reverted, the new test fails: the forged_auth_user_idand_auth_user_emailare present, and the no-auth result still has all four forged keys.go vet ./internal/server/: okI did not run the full
./internal/serverpackage locally.