fix: address Copilot review feedback

- Move VerifyJWT to test file to avoid exporting test-only helpers
- Use RWMutex with double-check pattern to avoid blocking reads during
  token refresh
- Add UserAgentTransport to GraphQL path when using App auth for
  consistency with REST
- Make GITHUB_APP_PRIVATE_KEY and GITHUB_APP_PRIVATE_KEY_PATH mutually
  exclusive (return error when both are set)
- Only replace literal \n when the private key has no real newlines to
  avoid corrupting correctly-passed keys
- Use safer JWT lifetime (iat=now-30s, exp=now+9m) to stay well within
  GitHub's 10-minute maximum
- Document that base transport must not inject its own Authorization
  header

Author: pyama86

README.md

@@ -254,7 +254,7 @@ The server automatically generates JWTs, fetches installation tokens, and refres
 | `GITHUB_APP_PRIVATE_KEY` | The PEM-encoded private key (inline, `\n` for newlines) |
 | `GITHUB_APP_PRIVATE_KEY_PATH` | Path to the private key file (alternative to inline) |
 
-Either `GITHUB_APP_PRIVATE_KEY` or `GITHUB_APP_PRIVATE_KEY_PATH` must be set, but not both. When all three required variables (`GITHUB_APP_ID`, `GITHUB_APP_INSTALLATION_ID`, and a private key) are set, the server uses GitHub App authentication instead of a PAT. `GITHUB_PERSONAL_ACCESS_TOKEN` is not required in this case.
+Either `GITHUB_APP_PRIVATE_KEY` or `GITHUB_APP_PRIVATE_KEY_PATH` must be set, but not both (they are mutually exclusive). When all three required variables (`GITHUB_APP_ID`, `GITHUB_APP_INSTALLATION_ID`, and a private key) are set, the server uses GitHub App authentication instead of a PAT. `GITHUB_PERSONAL_ACCESS_TOKEN` is not required in this case.
 
 #### Example: Using a private key file
 

cmd/github-mcp-server/main.go

@@ -266,6 +266,10 @@ func parseAppAuthConfig() (appID int64, privateKey []byte, installationID int64,
 		return 0, nil, 0, errors.New("incomplete GitHub App auth config: GITHUB_APP_ID, GITHUB_APP_INSTALLATION_ID, and GITHUB_APP_PRIVATE_KEY or GITHUB_APP_PRIVATE_KEY_PATH are all required")
 	}
 
+	if privateKeyStr != "" && privateKeyPath != "" {
+		return 0, nil, 0, errors.New("GITHUB_APP_PRIVATE_KEY and GITHUB_APP_PRIVATE_KEY_PATH are mutually exclusive")
+	}
+
 	appID, err = strconv.ParseInt(appIDStr, 10, 64)
 	if err != nil {
 		return 0, nil, 0, fmt.Errorf("invalid GITHUB_APP_ID: %w", err)
@@ -277,8 +281,13 @@ func parseAppAuthConfig() (appID int64, privateKey []byte, installationID int64,
 	}
 
 	if privateKeyStr != "" {
-		// Environment variables often use literal "\n" instead of actual newlines
-		privateKey = []byte(strings.ReplaceAll(privateKeyStr, `\n`, "\n"))
+		// Environment variables often use literal "\n" instead of actual newlines.
+		// Only replace when the value has no real newlines to avoid corrupting
+		// keys that were correctly passed with actual newlines.
+		if !strings.Contains(privateKeyStr, "\n") {
+			privateKeyStr = strings.ReplaceAll(privateKeyStr, `\n`, "\n")
+		}
+		privateKey = []byte(privateKeyStr)
 	} else {
 		privateKey, err = os.ReadFile(privateKeyPath)
 		if err != nil {

internal/ghmcp/server.go

@@ -95,9 +95,13 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
 	// We use NewEnterpriseClient unconditionally since we already parsed the API host
 	var gqlTransport http.RoundTripper
 	if authTransport != nil {
-		// Auth transport already sets the Authorization header
-		gqlTransport = &transport.GraphQLFeaturesTransport{
-			Transport: authTransport,
+		// Auth transport already sets the Authorization header.
+		// Wrap with UserAgentTransport for consistency with the REST path.
+		gqlTransport = &transport.UserAgentTransport{
+			Transport: &transport.GraphQLFeaturesTransport{
+				Transport: authTransport,
+			},
+			Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version),
 		}
 	} else {
 		gqlTransport = &transport.BearerAuthTransport{

pkg/github/appauth/appauth.go

@@ -13,7 +13,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"strings"
 	"sync"
 	"time"
 )
@@ -42,7 +41,7 @@ type Transport struct {
 	key    *rsa.PrivateKey
 	base   http.RoundTripper
 
-	mu    sync.Mutex
+	mu    sync.RWMutex
 	token string
 	exp   time.Time
 }
@@ -55,6 +54,8 @@ type installationToken struct {
 // NewTransport creates a new Transport that authenticates using a GitHub App
 // installation token. The transport automatically handles JWT generation and
 // installation token refresh.
+// The base transport must not inject its own Authorization header, as this
+// transport sets it for both installation token requests and API requests.
 func NewTransport(base http.RoundTripper, cfg Config) (*Transport, error) {
 	key, err := parsePrivateKey(cfg.PrivateKey)
 	if err != nil {
@@ -91,10 +92,20 @@ func (t *Transport) Token(ctx context.Context) (string, error) {
 }
 
 func (t *Transport) installationToken(ctx context.Context) (string, error) {
+	// Fast path: read lock to check cached token
+	t.mu.RLock()
+	if t.token != "" && time.Now().Add(5*time.Minute).Before(t.exp) {
+		token := t.token
+		t.mu.RUnlock()
+		return token, nil
+	}
+	t.mu.RUnlock()
+
+	// Slow path: write lock to refresh
 	t.mu.Lock()
 	defer t.mu.Unlock()
 
-	// Refresh if the token expires within 5 minutes
+	// Double-check after acquiring write lock
 	if t.token != "" && time.Now().Add(5*time.Minute).Before(t.exp) {
 		return t.token, nil
 	}
@@ -116,15 +127,15 @@ func (t *Transport) installationToken(ctx context.Context) (string, error) {
 
 // generateJWT creates a signed JWT for GitHub App authentication using RS256.
 func (t *Transport) generateJWT() (string, error) {
-	now := time.Now().Add(-30 * time.Second) // allow 30s clock drift
+	now := time.Now()
 
 	header := map[string]string{
 		"alg": "RS256",
 		"typ": "JWT",
 	}
 	payload := map[string]any{
-		"iat": now.Unix(),
-		"exp": now.Add(10 * time.Minute).Unix(),
+		"iat": now.Add(-30 * time.Second).Unix(), // allow 30s clock drift
+		"exp": now.Add(9 * time.Minute).Unix(),   // well within GitHub's 10-minute maximum
 		"iss": fmt.Sprintf("%d", t.config.AppID),
 	}
 
@@ -201,33 +212,3 @@ func parsePrivateKey(pemBytes []byte) (*rsa.PrivateKey, error) {
 		return nil, fmt.Errorf("unsupported PEM block type: %s", block.Type)
 	}
 }
-
-// VerifyJWT parses and verifies a JWT token using the given RSA public key.
-// Returns the claims map. This is used only for testing.
-func VerifyJWT(tokenString string, pubKey *rsa.PublicKey) (map[string]any, error) {
-	parts := strings.SplitN(tokenString, ".", 3)
-	if len(parts) != 3 {
-		return nil, fmt.Errorf("invalid JWT: expected 3 parts, got %d", len(parts))
-	}
-
-	signingInput := parts[0] + "." + parts[1]
-	sig, err := base64.RawURLEncoding.DecodeString(parts[2])
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode signature: %w", err)
-	}
-
-	hash := sha256.Sum256([]byte(signingInput))
-	if err := rsa.VerifyPKCS1v15(pubKey, crypto.SHA256, hash[:], sig); err != nil {
-		return nil, fmt.Errorf("invalid signature: %w", err)
-	}
-
-	payloadJSON, err := base64.RawURLEncoding.DecodeString(parts[1])
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode payload: %w", err)
-	}
-	var claims map[string]any
-	if err := json.Unmarshal(payloadJSON, &claims); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal claims: %w", err)
-	}
-	return claims, nil
-}

pkg/github/appauth/appauth_test.go

@@ -2,14 +2,18 @@ package appauth
 
 import (
 	"context"
+	"crypto"
 	"crypto/rand"
 	"crypto/rsa"
+	"crypto/sha256"
 	"crypto/x509"
+	"encoding/base64"
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -18,6 +22,35 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// verifyJWT parses and verifies a JWT token using the given RSA public key.
+func verifyJWT(tokenString string, pubKey *rsa.PublicKey) (map[string]any, error) {
+	parts := strings.SplitN(tokenString, ".", 3)
+	if len(parts) != 3 {
+		return nil, fmt.Errorf("invalid JWT: expected 3 parts, got %d", len(parts))
+	}
+
+	signingInput := parts[0] + "." + parts[1]
+	sig, err := base64.RawURLEncoding.DecodeString(parts[2])
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode signature: %w", err)
+	}
+
+	hash := sha256.Sum256([]byte(signingInput))
+	if err := rsa.VerifyPKCS1v15(pubKey, crypto.SHA256, hash[:], sig); err != nil {
+		return nil, fmt.Errorf("invalid signature: %w", err)
+	}
+
+	payloadJSON, err := base64.RawURLEncoding.DecodeString(parts[1])
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode payload: %w", err)
+	}
+	var claims map[string]any
+	if err := json.Unmarshal(payloadJSON, &claims); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal claims: %w", err)
+	}
+	return claims, nil
+}
+
 func generateTestKey(t *testing.T) (*rsa.PrivateKey, []byte) {
 	t.Helper()
 	key, err := rsa.GenerateKey(rand.Reader, 2048)
@@ -111,15 +144,15 @@ func TestTransport_GenerateJWT(t *testing.T) {
 	jwtToken, err := tr.generateJWT()
 	require.NoError(t, err)
 
-	claims, err := VerifyJWT(jwtToken, &key.PublicKey)
+	claims, err := verifyJWT(jwtToken, &key.PublicKey)
 	require.NoError(t, err)
 
 	assert.Equal(t, "12345", claims["iss"])
 
 	iat := int64(claims["iat"].(float64))
 	exp := int64(claims["exp"].(float64))
 	assert.InDelta(t, time.Now().Unix(), iat, 60)
-	assert.InDelta(t, time.Now().Add(10*time.Minute).Unix(), exp, 60)
+	assert.InDelta(t, time.Now().Add(9*time.Minute).Unix(), exp, 60)
 }
 
 func TestTransport_FetchInstallationToken(t *testing.T) {
@@ -135,7 +168,7 @@ func TestTransport_FetchInstallationToken(t *testing.T) {
 		assert.True(t, len(authHeader) > 7)
 		jwtToken := authHeader[7:] // strip "Bearer "
 
-		_, err := VerifyJWT(jwtToken, &key.PublicKey)
+		_, err := verifyJWT(jwtToken, &key.PublicKey)
 		assert.NoError(t, err)
 
 		w.WriteHeader(http.StatusCreated)