Repository navigation
Make uninstall a full undo, and surface the 401 body - #2
Merged
Merged
Conversation
- Accept a profile name with or without the "@" everywhere: Resolve is now one rule (http(s) URL = endpoint, anything else = profile name), so `uninstall @name` no longer reports "no entry" while `show name` and `uninstall name` work — the asymmetry that started this - Remove the [profiles.<name>] block from config.toml on uninstall, plus the tokens stored for it: with the profile gone there is no target left to `logout` with, and users were cleaning the TOML by hand - Follow the entry's args to find the profile, so `install --name X` stays undoable; warn when the removed entry was not an mcpurl one - Make each part best-effort: a half-installed setup (entry already deleted, profile still in config.toml) is exactly what needs cleaning, so only "nothing found anywhere" is an error - Update an existing profile in place instead of refusing with "pass a different --name": the URL follows the new target, flags override, and unmentioned settings (headers above all) are carried over - Edit config.toml as text (pkg/app/tomledit.go), never a TOML round-trip: comments, alignment and other profiles survive, including a comment that documents the profile below the one being removed - Write both configs via tmp+rename so an interrupted write cannot truncate them; keep an existing file's mode, .bak as before - Fix install treating a bare profile name as a URL, which minted a duplicate profile named after the host
A server that refuses the credential usually says why ("malformed jwt:
unexpected signature algorithm HS256"), and every one of those bytes was
discarded: a response body reads exactly once, and the debug line read it
first. The user saw "upstream HTTP 401", a browser window, and another 401
(docs/tasks/02-401-body-discarded.md).
- Buffer a bounded prefix of every failed response before anything else
reads it, and put it back, so the log line and the HTTPError a caller
builds later see the same bytes
- Render the server's own text in HTTPError.Error() and hand the error's
text to the MCP client, which showed a bare "upstream HTTP 401" before
- Parse the RFC 6750 error/error_description pair out of WWW-Authenticate
and name it in the give-up message
- Stop escalating to a browser login when the rejected token is one this
process just minted: the authorization server issues it happily and the
resource server refuses it, so a login only buys a pointless window —
unless the challenge calls the token expired
- Cap the 401 ladder (maxTokenCycles) and stop when the provider re-offers
a token the server already rejected
- Answer a failed initialize before exiting: an MCP client that only
renders the error surface must still learn why the handshake failed
- Log the challenge probe's own 401 body, which was dropped the same way
- Redact JWT-shaped runs from server-supplied text (internal/redact):
gateways quote the credential they refused, and that text now reaches a
plaintext client log and the client UI
- Move internal/redact and internal/atomicfile under pkg/: both hold a rule shared by packages that deliberately do not import each other (redact by mcp and oauth, atomicfile by app and oauth), and the project keeps its packages under pkg/ - Fold Mode into atomicfile.Write, which now applies the mode to a new file only and leaves an existing file's own alone — the os.WriteFile convention, and one exported function instead of two
- Bump the go directive from 1.26.5, which CI installs verbatim via setup-go's go-version-file, so govulncheck reported five standard library vulnerabilities fixed in 1.26.6 (net/http, crypto/tls, encoding/asn1) and failed the security job - No source change: the advisories are in the toolchain, not in mcpurl, and govulncheck is clean on 1.26.7
- Suppress G703 on the .bak write: the path is the config mcpurl was pointed at (MCPURL_CONFIG, or a client's fixed location), which is the same reasoning that already excludes G304 in CI. Scoped to the one line so taint analysis stays on for new code - Add .gitleaks.toml allowing JWT-shaped fixtures under _test.go for the jwt rule alone: the redaction tests must carry tokens matching the shape, and these are the public jwt.io samples
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.
Two independent fixes, one per commit.
Problem
uninstallleft the profile behind, and refused the@spellingReported from the field:
uninstallwas the only command that took a name without passing it throughprofile resolution, so the
@— whichshowandConfig.Getstrip — reachedthe JSON lookup verbatim. Dropping the
@then removed the entry but left[profiles.asksrv]inconfig.toml, and no command could remove it:uninstallwas specified as "the entry and nothing else". The user edited theTOML by hand. Re-running
installwas no way out either — an existing namefailed with "pass a different --name".
A 401 body was thrown away, and re-login happened anyway
docs/tasks/02-401-body-discarded.md. The server answered with a completediagnosis (
oidc: malformed jwt: unexpected signature algorithm "HS256"), andnone of it reached the user: a response body reads exactly once, and the debug
line read it first. mcpurl then refreshed, opened a browser, and got the same
401 — the authorization server issues those tokens happily, the resource server
refuses them, and no amount of new tokens changes that.
Changes
uninstall / install
Config.Resolve): an http(s) URL is anendpoint, anything else is a profile name with an optional
@. A targetthat is neither produces one message naming both readings. This also fixes
claude-config asksrv, which insisted on the@from the other side.uninstallis the full undo: the client entry, the profile the entryreferences (
--namemay differ from the profile name) and the tokens storedfor it — with the profile gone there is no target left to
logoutwith. Eachpart is best-effort, so a half-installed setup still gets cleaned; only
"nothing found anywhere" is an error. Entries in other clients that still
point at the removed profile are reported.
installupdates an existing profile in place: the URL follows the newtarget, passed flags override, and everything else — headers above all — is
carried over.
config.tomlis edited as text (pkg/app/tomledit.go), never a TOMLround-trip: comments, alignment and other profiles survive byte-for-byte,
including a comment that documents the profile below the one being removed.
internal/atomicfile), so aninterrupted write cannot truncate them. Symlinks are followed, not replaced —
a
config.tomlsymlinked out of a dotfiles repository stays linked..bakasbefore.
installtreating a bare profile name as a URL, which minted aduplicate profile named after the host.
401 diagnostics
back, so the log line and the error built later see the same bytes. The
server's own text is rendered into
HTTPError.Error()and into the JSON-RPCerror the MCP client shows, which used to read
upstream HTTP 401.error/error_descriptionpair is parsed out ofWWW-Authenticateand named in the give-up message.— unless the challenge calls it expired, which a new token can still fix. The
ladder is capped, and stops when a provider re-offers a rejected token.
initializeis answered before exiting: a client that only rendersthe error surface must still learn why the handshake failed.
internal/redact) —gateways quote the credential they refused, and that text now reaches a
plaintext client log and the client UI.
Verification
go test -race ./...,make lintandgofmtclean; the first commit was alsochecked out into a separate worktree and tested on its own, so it stands up on
CI without the second.
Beyond unit tests, both paths were exercised end to end with a real binary
against a temporary
HOME: install → re-install with a different URL →uninstall @name→uninstall nameagain; a config where the removed profilesits between a comment block and another profile; a
config.tomlsymlinked intoa dotfiles directory; and an entry belonging to another MCP server.
Notes
indistinguishable from an identifier. Scrubbing the exact presented value at
capture time is the complete fix and needs a change to how the body is
captured; filed for a follow-up task together with the error bodies still
dropped in
fetchJSONand the token endpoint'serror_description.uninstall <name>still removes an entry that mcpurl did not write, andtrusts that entry's
@argwhen choosing which profile to drop. It warns andpoints at the
.bak; gating the cascade on ownership is a deliberatenon-change here.
upstream HTTP 401ratherthan "mcpurl stopped trying" — worth a distinct error later.