Skip to content

Commit 586d047

Browse files
committed
Codebase quality pass: fix bugs, unify patterns, reduce complexity
Fix duplicate isStdioServer with diverging implementations by adding IsStdio() method on MCPClientConfig. Unify CSRF strategy across admin and token handlers using stateless HMAC-based tokens (removes sync.Map approach). Make StoreAuthorizeRequest return an error so Firestore failures surface clearly. Use cookie.SetSession consistently (fixes SameSite mismatch from Strict to Lax for OAuth callbacks). Split http.go into focused files (user_token_service.go, session_handler.go). Extract handleBrowserCallback and handleOAuthClientCallback from 190-line IDPCallbackHandler. Add GetUser to Storage interface for O(1) admin checks. Consolidate browserauth + oauthsession into internal/session, move envutil.IsDev into config package. Replace hardcoded Firestore collection names with constants. Delete unused GetErrorName. Update CLAUDE.md to reference ./scripts/ instead of make.
1 parent 0e099bd commit 586d047

26 files changed

+542
-537
lines changed

CLAUDE.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -245,23 +245,24 @@ When refactoring for better design:
245245

246246
```bash
247247
# Build everything
248-
make build
248+
./scripts/build
249249

250250
# Format everything
251-
make format
251+
./scripts/format
252252

253253
# Lint everything
254-
make lint
254+
./scripts/lint
255255

256256
# Test mcp-front
257+
./scripts/test
257258
go test ./internal/... -v
258259
go test ./integration -v
259260

260261
# Run mcp-front locally
261262
./mcp-front -config config.json
262263

263264
# Start docs dev server
264-
make doc
265+
./scripts/docs
265266
```
266267

267268
## Documentation Site Guidelines

internal/adminauth/admin.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,16 @@ func IsAdmin(ctx context.Context, email string, adminConfig *config.AdminConfig,
2222
return true
2323
}
2424

25-
// Check if user is a promoted admin in storage
25+
// Check if user is a promoted admin in storage.
26+
// Try exact match first (O(1)), fall back to case-insensitive scan
27+
// since emails may be stored with different casing.
2628
if store != nil {
29+
user, err := store.GetUser(ctx, normalizedEmail)
30+
if err == nil {
31+
return user.IsAdmin
32+
}
33+
34+
// Fallback: case-insensitive scan for emails stored with different casing
2735
users, err := store.GetAllUsers(ctx)
2836
if err == nil {
2937
for _, user := range users {

internal/browserauth/session.go

Lines changed: 0 additions & 16 deletions
This file was deleted.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package envutil
1+
package config
22

33
import (
44
"os"

internal/config/types.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,11 @@ type MCPClientConfig struct {
187187
InlineConfig json.RawMessage `json:"inline,omitempty"`
188188
}
189189

190+
// IsStdio returns true if this is a stdio-based MCP server
191+
func (c *MCPClientConfig) IsStdio() bool {
192+
return c.TransportType == MCPClientTypeStdio
193+
}
194+
190195
// SessionConfig represents session management configuration
191196
type SessionConfig struct {
192197
Timeout time.Duration

internal/cookie/cookie.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import (
44
"net/http"
55
"time"
66

7-
"github.com/dgellow/mcp-front/internal/envutil"
7+
"github.com/dgellow/mcp-front/internal/config"
88
"github.com/dgellow/mcp-front/internal/log"
99
)
1010

@@ -16,7 +16,7 @@ const (
1616

1717
// SetSession sets a session cookie with appropriate security settings
1818
func SetSession(w http.ResponseWriter, value string, maxAge time.Duration) {
19-
secure := !envutil.IsDev()
19+
secure := !config.IsDev()
2020
http.SetCookie(w, &http.Cookie{
2121
Name: SessionCookie,
2222
Value: value,
@@ -41,7 +41,7 @@ func SetCSRF(w http.ResponseWriter, value string) {
4141
Value: value,
4242
Path: "/",
4343
HttpOnly: false, // CSRF tokens need to be readable by JavaScript
44-
Secure: !envutil.IsDev(),
44+
Secure: !config.IsDev(),
4545
SameSite: http.SameSiteStrictMode,
4646
MaxAge: int((24 * time.Hour).Seconds()), // 24 hours
4747
})

internal/crypto/csrf.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package crypto
2+
3+
import (
4+
"fmt"
5+
"strconv"
6+
"strings"
7+
"time"
8+
)
9+
10+
// CSRFProtection provides stateless HMAC-based CSRF token generation and validation.
11+
// Tokens are self-contained: nonce:timestamp:signature, with configurable expiry.
12+
type CSRFProtection struct {
13+
signingKey []byte
14+
ttl time.Duration
15+
}
16+
17+
// NewCSRFProtection creates a new CSRF protection instance
18+
func NewCSRFProtection(signingKey []byte, ttl time.Duration) CSRFProtection {
19+
return CSRFProtection{
20+
signingKey: signingKey,
21+
ttl: ttl,
22+
}
23+
}
24+
25+
// Generate creates a new CSRF token
26+
func (c *CSRFProtection) Generate() (string, error) {
27+
nonce := GenerateSecureToken()
28+
if nonce == "" {
29+
return "", fmt.Errorf("failed to generate nonce")
30+
}
31+
32+
timestamp := strconv.FormatInt(time.Now().Unix(), 10)
33+
data := nonce + ":" + timestamp
34+
signature := SignData(data, c.signingKey)
35+
36+
return fmt.Sprintf("%s:%s:%s", nonce, timestamp, signature), nil
37+
}
38+
39+
// Validate checks if a CSRF token is valid and not expired
40+
func (c *CSRFProtection) Validate(token string) bool {
41+
parts := strings.SplitN(token, ":", 3)
42+
if len(parts) != 3 {
43+
return false
44+
}
45+
46+
nonce := parts[0]
47+
timestampStr := parts[1]
48+
signature := parts[2]
49+
50+
timestamp, err := strconv.ParseInt(timestampStr, 10, 64)
51+
if err != nil {
52+
return false
53+
}
54+
55+
if time.Since(time.Unix(timestamp, 0)) > c.ttl {
56+
return false
57+
}
58+
59+
data := nonce + ":" + timestampStr
60+
return ValidateSignedData(data, signature, c.signingKey)
61+
}

internal/jsonrpc/errors.go

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,24 +47,3 @@ func NewStandardError(code int) *Error {
4747
}
4848
}
4949

50-
// GetErrorName returns a human-readable name for standard error codes
51-
func GetErrorName(code int) string {
52-
switch code {
53-
case ParseError:
54-
return "PARSE_ERROR"
55-
case InvalidRequest:
56-
return "INVALID_REQUEST"
57-
case MethodNotFound:
58-
return "METHOD_NOT_FOUND"
59-
case InvalidParams:
60-
return "INVALID_PARAMS"
61-
case InternalError:
62-
return "INTERNAL_ERROR"
63-
default:
64-
// Check if it's an MCP-specific error code
65-
if code >= -32099 && code <= -32000 {
66-
return "SERVER_ERROR"
67-
}
68-
return "UNKNOWN_ERROR"
69-
}
70-
}

internal/mcpfront.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,7 @@ func buildHTTPHandler(
380380
}
381381

382382
// Create token handlers
383-
tokenHandlers := server.NewTokenHandlers(storage, cfg.MCPServers, true, serviceOAuthClient)
383+
tokenHandlers := server.NewTokenHandlers(storage, cfg.MCPServers, true, serviceOAuthClient, []byte(authConfig.EncryptionKey))
384384

385385
// Token management UI endpoints
386386
mux.Handle(route("/my/tokens"), server.ChainMiddleware(http.HandlerFunc(tokenHandlers.ListTokensHandler), tokenMiddleware...))
@@ -421,7 +421,7 @@ func buildHTTPHandler(
421421
}
422422
} else {
423423
// For stdio/SSE servers
424-
if isStdioServer(serverConfig) {
424+
if serverConfig.IsStdio() {
425425
sseServer, mcpServer, err = buildStdioSSEServer(serverName, baseURL, sessionManager)
426426
if err != nil {
427427
return nil, fmt.Errorf("failed to create SSE server for %s: %w", serverName, err)
@@ -606,7 +606,3 @@ func buildStdioSSEServer(serverName, baseURL string, sessionManager *client.Stdi
606606
return sseServer, mcpServer, nil
607607
}
608608

609-
// isStdioServer checks if this is a stdio-based server
610-
func isStdioServer(cfg *config.MCPClientConfig) bool {
611-
return cfg.TransportType == config.MCPClientTypeStdio
612-
}

internal/oauth/provider.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,9 @@ import (
1212

1313
"github.com/dgellow/mcp-front/internal/config"
1414
"github.com/dgellow/mcp-front/internal/crypto"
15-
"github.com/dgellow/mcp-front/internal/envutil"
1615
jsonwriter "github.com/dgellow/mcp-front/internal/json"
1716
"github.com/dgellow/mcp-front/internal/log"
18-
"github.com/dgellow/mcp-front/internal/oauthsession"
17+
"github.com/dgellow/mcp-front/internal/session"
1918
"github.com/dgellow/mcp-front/internal/storage"
2019
"github.com/dgellow/mcp-front/internal/urlutil"
2120
"github.com/ory/fosite"
@@ -48,8 +47,8 @@ func NewOAuthProvider(oauthConfig config.OAuthAuthConfig, store storage.Storage,
4847

4948
// Determine min parameter entropy based on environment
5049
minEntropy := 8 // Production default - enforce secure state parameters (8+ chars)
51-
log.Logf("OAuth provider initialization - MCP_FRONT_ENV=%s, isDevelopmentMode=%v", os.Getenv("MCP_FRONT_ENV"), envutil.IsDev())
52-
if envutil.IsDev() {
50+
log.Logf("OAuth provider initialization - MCP_FRONT_ENV=%s, isDevelopmentMode=%v", os.Getenv("MCP_FRONT_ENV"), config.IsDev())
51+
if config.IsDev() {
5352
minEntropy = 0 // Development mode - allow empty state parameters
5453
log.LogWarn("Development mode enabled - OAuth security checks relaxed (state parameter entropy: %d)", minEntropy)
5554
}
@@ -158,8 +157,8 @@ func NewValidateTokenMiddleware(provider fosite.OAuth2Provider, issuer string, a
158157
// - This is documented fosite behavior, not a bug
159158
// - The actual session data must be retrieved from the returned AccessRequester
160159
// See: https://github.com/ory/fosite/issues/256
161-
session := &oauthsession.Session{DefaultSession: &fosite.DefaultSession{}}
162-
_, accessRequest, err := provider.IntrospectToken(ctx, token, fosite.AccessToken, session)
160+
oauthSession := &session.OAuthSession{DefaultSession: &fosite.DefaultSession{}}
161+
_, accessRequest, err := provider.IntrospectToken(ctx, token, fosite.AccessToken, oauthSession)
163162
if err != nil {
164163
jsonwriter.WriteUnauthorizedRFC9728(w, "Invalid or expired token", metadataURI)
165164
return
@@ -180,7 +179,7 @@ func NewValidateTokenMiddleware(provider fosite.OAuth2Provider, issuer string, a
180179
// This is the correct way to retrieve session data after token introspection
181180
var userEmail string
182181
if accessRequest != nil {
183-
if reqSession, ok := accessRequest.GetSession().(*oauthsession.Session); ok {
182+
if reqSession, ok := accessRequest.GetSession().(*session.OAuthSession); ok {
184183
if reqSession.Identity.Email != "" {
185184
userEmail = reqSession.Identity.Email
186185
}

0 commit comments

Comments
 (0)