Follow-up to my analysis on #1543.
Heads up on the shape first: this builds on #1290's poll-loop seams and
github wont let me target a fork branch, so the diff shows its two
commits too. Review only the last commit, the first two are #1290
unchanged, credit to @developer-ishan. Happy to rebase whichever way
#1290 goes, or split differently if that's easier to review.
What it does. pollForToken treats every non-pending answer as terminal,
so one flaky response kills a login the user may already have approved
in the browser. This spends a small grace budget (2 polls) on
incorrect_device_code before giving up, retries 5xx with a growing
backoff, and returns status plus body on other non-200s instead of dying
inside json.Unmarshal on an html error page. Terminal errors now carry
error_description and error_uri, and a timeout that happens after
polling recovered reports a plain timeout, not a stale error from
minutes earlier.
Also honours the interval and expires_in GitHub returns with the device
code (RFC 8628 3.2) instead of hardcoding 5/900, clamped (60s / 3600s)
so a misbehaving response can't stall the CLI or overflow the duration
math.
One thing I changed my mind on from the issue comment: I wrote there
that the client should wait one interval before the first poll. Dropped
that. My own repro test showed 40s of clean polling before authorize, so
the propagation theory doesn't hold, and the grace retry covers the same
window without taxing every fast login. First poll stays immediate,
pinned by a test.
Root cause note: I can't prove the GitHub-side race (cli/cli#9302 is the
same error class but was closed on an unconfirmed "platform race"). So
this isn't claimed as a root-cause fix, it makes the login recoverable
if the condition is transient and diagnosable if it isn't. Btw
incorrect_device_code is a GitHub extension, not an RFC 8628 3.5 code,
so there's no spec retry semantic being violated by the grace polls.
## Test
12 new tests in github_at_device_flow_internal_test.go, all green with
-race (33 in the package). #1290's four tests pass unmodified.
golangci-lint clean in the touched files under both v2.11.4 (this
branch's CI pin) and v2.13.1 (what main pins now).
## AI Disclosure
AI assistance (Claude) was used for issue research and during
development. All changes were reviewed and tested by the author.
Fixes#1543
---------
Co-authored-by: developer-ishan <igupta@groupon.com>
Co-authored-by: Ishan Gupta <62565307+developer-ishan@users.noreply.github.com>
Co-authored-by: developer-ishan <guptaishan849@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Radoslav Dimitrov <radoslav@stacklok.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary
- Handle GitHub's OAuth device-flow `slow_down` response per [RFC 8628
§3.5](https://datatracker.ietf.org/doc/html/rfc8628#section-3.5) — treat
it as retriable and bump the polling interval by 5 seconds, instead of
bailing with a fatal error.
Fixes#1289.
## Why
`pollForToken` in `cmd/publisher/auth/github-at.go` only treats
`authorization_pending` as retriable. Every other response from GitHub's
token endpoint — including the non-terminal `slow_down` signal — falls
through to:
```go
return "", fmt.Errorf("token request failed: %s", tokenResp.Error)
```
So users hitting GitHub's polling rate-limit get:
```
Error: login failed: error polling for token: token request failed: slow_down
```
…and have to re-run `mcp-publisher login github` from scratch, which is
especially painful because `slow_down` is more likely when the user
takes more than a few seconds to enter and authorize the device code.
Per RFC 8628 §3.5, `slow_down` is defined as:
> A variant of "authorization_pending", the authorization request is
still pending and polling should continue, but the interval MUST be
increased by 5 seconds for this and all subsequent requests.
## Change
Treat `slow_down` the same as `authorization_pending` but increase
`interval` by 5 seconds before sleeping, matching the RFC exactly.
A possible follow-up (not in this PR, to keep the diff minimal): seed
the initial `interval` from `DeviceCodeResponse.Interval` (already
parsed but currently unused) instead of hardcoding `5`. Happy to do that
here or as a separate PR if preferred.
## Test plan
- [x] `go build ./cmd/publisher/...` — clean
- [x] `go test ./cmd/publisher/auth/...` — passes
- [x] `gofmt -d` and `go vet ./cmd/publisher/auth/...` — clean
- [ ] CI `make check` (lint + integration tests)
I could not exercise `pollForToken` from a unit test without refactoring
`GitHubAccessTokenURL` to be injectable, which felt out of scope for a
bug-fix PR. Glad to add that refactor + a table-driven test for
`authorization_pending` / `slow_down` / fatal errors in a follow-up if
maintainers want it.
---------
Co-authored-by: developer-ishan <igupta@groupon.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Fixes#663
- **Security fix**: `mcp-publisher login` no longer writes
`.mcpregistry_github_token` and `.mcpregistry_registry_token` to the
current working directory. GitHub tokens are kept in memory during the
login→exchange flow and cleared immediately after.
- **Token location**: Unified token moved from `~/.mcp_publisher_token`
to `~/.config/mcp-publisher/token.json` (dir 0700, file 0600), following
the convention used by `gh`, `docker`, and `kubectl`.
- **Dead code removed**: `NeedsLogin()` was on the `Provider` interface
but never called — removed from interface and all 4 implementations.
`forceLogin` field also removed.
- **Auth tests added**: 11 new tests for `GitHubATProvider` (package
previously had 0 tests).
- **Test isolation**: `SetupTestToken` now uses `t.TempDir()` +
`t.Setenv("HOME", ...)` instead of writing to real `$HOME`.
### Breaking change
Users must re-login after upgrading (`mcp-publisher login <method>`).
Existing tokens at `~/.mcp_publisher_token` are not auto-migrated.
`logout` cleans up both old and new locations.
## Test plan
- [x] `make check` passes (lint, unit tests, integration tests)
- [x] `mcp-publisher login none` stores token at
`~/.config/mcp-publisher/token.json`
- [x] No `.mcpregistry_*` files created in cwd after login (with
`--token`, `MCP_GITHUB_TOKEN`, or device flow)
- [x] `mcp-publisher publish` reads token from new location
- [x] `mcp-publisher validate` reads registry URL from new location
- [x] `mcp-publisher logout` cleans up new location, legacy
`~/.mcp_publisher_token`, and cwd `.mcpregistry_*` files
- [x] `mcp-publisher logout` when not logged in shows "Not logged in"
- [x] File permissions: dir 0700, file 0600
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
<!-- Provide a brief summary of your changes -->
## Motivation and Context
<!-- Why is this change needed? What problem does it solve? -->
The following PR implements the missing support for logging in via
`--token` or `MCP_GITHUB_TOKEN`
## How Has This Been Tested?
<!-- Have you tested this in a real application? Which scenarios were
tested? -->
## Breaking Changes
<!-- Will users need to update their code or configurations? -->
## Types of changes
<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
## Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [ ] I have read the [MCP
Documentation](https://modelcontextprotocol.io)
- [ ] My code follows the repository's style guidelines
- [ ] New and existing tests pass locally
- [ ] I have added appropriate error handling
- [ ] I have added or updated documentation as needed
## Additional context
<!-- Add any other context, implementation notes, or design decisions
-->
Fixes: #698
---------
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
## Summary
Move the publisher CLI from `tools/publisher/` to `cmd/publisher/` as
it's a proper application rather than a development utility.
This addresses feedback from @rdimitrov in PR #324 that the publisher
CLI should be under `cmd/` since it's not really a tool in the same
sense as other helpers in the `tools/` folder.
## Changes
- Moved `tools/publisher/` directory to `cmd/publisher/`
- Updated Makefile build and clean targets
- Updated README.md documentation links
- Updated VS Code launch configuration
- Updated integration test script
- Updated FAQ documentation
- Fixed Go import paths in publisher code
---------
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: adam jones <domdomegg@users.noreply.github.com>
2025-08-29 14:45:36 +01:00
Renamed from tools/publisher/auth/github-at.go (Previous history)
