Skip to content

MCP/CLI repo read tools serialize the node's error body as a fabricated result (no HTTP status check) #123

Description

@beardthelion

Summary

The MCP repo read tools parse the node response with client.get(...).await?.json().await? and never inspect the HTTP status. When a visibility-gated endpoint returns a non-2xx JSON body (the opaque 404 for a private repo the caller cannot read, or a 5xx), that error body is serialized straight back to the agent as if it were the requested resource. The tool returns Ok, so the agent has no signal that the read was denied or failed and proceeds on a fabricated object.

This is the MCP twin of the gl repo info bug fixed in PR #113 (cmd_info), and it is distinct from #115. #115 is about the clients sending unsigned requests (owner can't authenticate); this is about not checking the status before parsing. They are orthogonal: #115's proposed .get( -> .get_maybe_signed( switch lets the owner reach a 200, but it does not stop a genuine non-2xx (non-owner, deleted repo, node error) from being parsed as a result. cmd_info already used get_maybe_signed and still needed the separate status-check fix in #113.

Reproduced by execution

A throwaway test driving call_tool("repo_get", {owner, name}, node, None) against a mock node returning a JSON 404:

repo_get returned Ok on a 404 — fabricated repo handed to agent:
  Some("{\n  \"message\": \"repository 'owner/myrepo' not found\"\n}")

The dispatch returned Ok with the error body serialized as the repo. The node is correct (get_repo calls authorize_repo_read, repos.rs:269); the client is wrong.

Affected call sites

Confirmed by reading the code (same get(...).await?.json().await? shape, no status check):

  • crates/gl/src/mcp.rsrepo_get (:690), repo_commits (:701), repo_tree (~:713)

The same shape appears on the CLI side (crates/gl/src/repo.rs cmd_commits, the gl pr read subcommands) and other MCP read arms; those were not individually reproduced here. Worth a sweep when fixing.

Fix direction

Mirror the #113 cmd_info fix: keep the Response, capture status, parse with .json().await.unwrap_or_default(), and return an error on non-2xx (surface the node's message) before serializing. Pairs naturally with #115's signing switch — apply both on the same pass so a private repo's owner gets a real 200 and everyone else gets a surfaced error instead of a fabricated object.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:glgl — the contributor CLIkind:bugDefect fix — wrong or unsafe behaviorsev:mediumDegraded but workaround existssubsystem:apiNode REST API request/response surface

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions