security: remove the legacy OAuth client-id fallback (it reads the merged git config) #712

New issue

Closed

opened 2026-10-04 18:21:57 +00:00 by hexajon · 0 comments

hexajon commented

2026-10-04 18:21:57 +00:00

Owner

What happens

ReadOAuthClientID (internal/config/backend.go:60) reads the host-keyed client id from --global (:62), then falls back to the pre-host-keyed bare key at :67: runGit("config", "--get", oauthClientIDLegacyGitKey) where oauthClientIDLegacyGitKey = "madtea.oauth-client-id" (:51). The fallback omits --global, so it reads the merged view, which includes a repo's --local config. A madtea.oauth-client-id planted in a repo's local git config would then steer the OAuth authorize URL's application id. The reader is called by the MCP login tool (internal/mcp/tools_auth.go:876).

Decision

Remove the legacy fallback for v0.20.0, which drops pre-host-keyed legacy OAuth key support. ReadOAuthClientID resolves only the host-keyed --global key. This closes the finding at its root rather than narrowing the scope.

Scope

It steers only which OAuth application id the authorize URL names; it captures no token (the OAuth core is PKCE S256, loopback redirect, no client secret). It is not clone-transferrable - git clone does not copy the source repo's local config.

Fix

Delete the :67 fallback block and the now-unused oauthClientIDLegacyGitKey const. A missing id still surfaces as ("", false). The CLI's own reader (setup.defaultReadOAuthClientID) is a separate path and is out of scope here.

Breaking

A pre-host-keyed install that logs in through the MCP tool - and never ran the CLI setup, which migrates the key - must re-register its OAuth application. The release notes record the dropped key.

Acceptance criteria

ReadOAuthClientID reads only the host-keyed --global key; the legacy bare-key fallback is gone.
A test proves a repo-local madtea.oauth-client-id is not read.
The release notes record the dropped legacy key.

## What happens `ReadOAuthClientID` (`internal/config/backend.go:60`) reads the host-keyed client id from `--global` (`:62`), then falls back to the pre-host-keyed bare key at `:67`: `runGit("config", "--get", oauthClientIDLegacyGitKey)` where `oauthClientIDLegacyGitKey = "madtea.oauth-client-id"` (`:51`). The fallback omits `--global`, so it reads the merged view, which includes a repo's `--local` config. A `madtea.oauth-client-id` planted in a repo's local git config would then steer the OAuth authorize URL's application id. The reader is called by the MCP login tool (`internal/mcp/tools_auth.go:876`). ## Decision Remove the legacy fallback for v0.20.0, which drops pre-host-keyed legacy OAuth key support. `ReadOAuthClientID` resolves only the host-keyed `--global` key. This closes the finding at its root rather than narrowing the scope. ## Scope It steers only which OAuth application id the authorize URL names; it captures no token (the OAuth core is PKCE S256, loopback redirect, no client secret). It is not clone-transferrable - `git clone` does not copy the source repo's local config. ## Fix Delete the `:67` fallback block and the now-unused `oauthClientIDLegacyGitKey` const. A missing id still surfaces as `("", false)`. The CLI's own reader (`setup.defaultReadOAuthClientID`) is a separate path and is out of scope here. ## Breaking A pre-host-keyed install that logs in through the MCP tool - and never ran the CLI setup, which migrates the key - must re-register its OAuth application. The release notes record the dropped key. ## Acceptance criteria - [ ] `ReadOAuthClientID` reads only the host-keyed `--global` key; the legacy bare-key fallback is gone. - [ ] A test proves a repo-local `madtea.oauth-client-id` is not read. - [ ] The release notes record the dropped legacy key.

hexajon added the

breaking

security

severity/low

labels

2026-10-04 18:21:57 +00:00

hexajon added a new dependency

2026-10-05 12:34:16 +00:00

#675 epic: v0.20.0 - breaking: clearer ADR tooling and the write guard on every surface

~~hexajon referenced this issue 2026-10-05 12:34:16 +00:00~~

epic: v0.20.0 - breaking: clearer ADR tooling and the write guard on every surface #675

hexajon referenced this issue from a pull request that will close it,