feat: persistent OAuth client registry (fix disconnect-on-restart)#52
Merged
Merged
Conversation
- oauthClientStore: add Load()/Save() methods with JSON persistence - Factory loadOAuthClientStore(vaultDir) in http.go - put() calls Save() best-effort; errors logged, never block registration - Error signals: invalid_client + WWW-Authenticate header - Optional TTL with cleanupExpired() and background sweeper - Full test coverage: load/save roundtrip, read-only dir resilience, restart persistence, expired client cleanup, file format verification Fixes #45
There was a problem hiding this comment.
Pull request overview
Adds persistence for dynamically registered OAuth clients so MCP clients can survive server restarts without re-registering.
Changes:
- Introduces JSON-backed OAuth client store load/save support.
- Wires the persistent client store into HTTP server startup and adds cleanup for expired clients.
- Adds persistence-focused tests, including restart behavior and file-format checks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
internal/mcp/serverbootstrap/oauth.go |
Adds persistent OAuth client storage, TTL-related fields, cleanup, and invalid_client error signaling. |
internal/mcp/serverbootstrap/http.go |
Loads the OAuth client store from the vault directory and starts cleanup. |
internal/mcp/serverbootstrap/oauth_persistence_test.go |
Adds unit tests for persistence, file format, invalid JSON, and expiry cleanup. |
internal/mcp/serverbootstrap/serverbootstrap_test.go |
Adds restart scenario coverage for persisted OAuth client registrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+107
to
+127
| func (s *oauthClientStore) Save() error { | ||
| s.mu.Lock() | ||
| file := oauthClientStoreFile{ | ||
| Version: oauthClientsFileVersion, | ||
| Clients: make(map[string]*registeredClient, len(s.clients)), | ||
| } | ||
| for id, c := range s.clients { | ||
| file.Clients[id] = c | ||
| } | ||
| s.mu.Unlock() | ||
|
|
||
| if s.path == "" { | ||
| return nil | ||
| } | ||
|
|
||
| data, err := json.MarshalIndent(file, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("marshal oauth client store: %w", err) | ||
| } | ||
|
|
||
| if err := fileutil.AtomicWriteFile(s.path, append(data, '\n'), 0o600); err != nil { |
| TTL *int64 `json:"ttl_seconds,omitempty"` // optional TTL in seconds | ||
| ExpiresAt *time.Time `json:"expires_at,omitempty"` // computed expiration time | ||
| } | ||
|
|
| } | ||
|
|
||
| // StartCleanup launches a background goroutine that periodically sweeps | ||
| // expired client entries at the given interval. It returns a stop function. |
Comment on lines
+1084
to
+1088
| authURL := fmt.Sprintf("http://127.0.0.1:%d/mcp/oauth/authorize?response_type=code&client_id=%s&redirect_uri=%s&code_challenge=abc123&code_challenge_method=S256&state=test", | ||
| port, clientID, url.QueryEscape("http://127.0.0.1:9999/callback")) | ||
| authReq, _ := http.NewRequest(http.MethodGet, authURL, nil) | ||
| authReq.Header.Set("Origin", fmt.Sprintf("http://127.0.0.1:%d", port)) | ||
| resp2, err := client.Do(authReq) |
Comment on lines
+1068
to
+1070
| defer cancel2() | ||
| go func() { | ||
| _ = RunHTTPServerOnListener(ctx2, listener, v2, v2.Dir, "dev", mcp.New) |
Comment on lines
+320
to
323
| w.Header().Set("WWW-Authenticate", `Bearer realm="openpass",error="invalid_client",error_description="unknown client_id; register via POST /oauth/register first"`) | ||
| writeJSON(w, http.StatusBadRequest, map[string]string{ | ||
| "error": "unauthorized_client", | ||
| "error": "invalid_client", | ||
| "error_description": "unknown client_id; register via POST /oauth/register first", |
Comment on lines
+133
to
+141
| func (s *oauthClientStore) put(c *registeredClient) { | ||
| s.mu.Lock() | ||
| s.clients[c.ClientID] = c | ||
| s.mu.Unlock() | ||
|
|
||
| // Best-effort persistence: log error but never fail the registration. | ||
| if s.path != "" { | ||
| if err := s.Save(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: failed to persist OAuth client store: %v\n", err) |
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.
Fixes #45
What
Describe the change in a few sentences.
Why
Explain the problem, user need, or maintenance reason.
Testing
make fmtmake vetmake lintGOWORK=off go test ./...Safety