Skip to content

Commit 4b7a4e6

Browse files
FarenheithFarenheith
committed
fix: fixing log forging vulnerability
Adding sanitization to log to avoid log forging vulnerability from user interaction
1 parent 5e1870a commit 4b7a4e6

7 files changed

Lines changed: 201 additions & 158 deletions

File tree

‎cmd/openapi-mcp/main.go‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@ package main
33
import (
44
"flag"
55
"fmt"
6-
"log"
76
"os"
87
"path/filepath"
98
"strings"
109

1110
"github.com/ckanthony/openapi-mcp/pkg/config"
1211
"github.com/ckanthony/openapi-mcp/pkg/parser"
1312
"github.com/ckanthony/openapi-mcp/pkg/server"
13+
"github.com/ckanthony/openapi-mcp/pkg/utils"
1414
"github.com/joho/godotenv"
1515
)
1616

@@ -56,33 +56,33 @@ func main() {
5656
// --- Load .env after parsing flags ---
5757
if *specPath != "" && !strings.HasPrefix(*specPath, "http://") && !strings.HasPrefix(*specPath, "https://") {
5858
envPath := filepath.Join(filepath.Dir(*specPath), ".env")
59-
log.Printf("Attempting to load .env file from spec directory: %s", envPath)
59+
utils.SafeLogPrintf("Attempting to load .env file from spec directory: %s", envPath)
6060
err := godotenv.Load(envPath)
6161
if err != nil {
6262
// It's okay if the file doesn't exist, log other errors.
6363
if !os.IsNotExist(err) {
64-
log.Printf("Warning: Error loading .env file from %s: %v", envPath, err)
64+
utils.SafeLogPrintf("Warning: Error loading .env file from %s: %v", envPath, err)
6565
} else {
66-
log.Printf("Info: No .env file found at %s, proceeding without it.", envPath)
66+
utils.SafeLogPrintf("Info: No .env file found at %s, proceeding without it.", envPath)
6767
}
6868
} else {
69-
log.Printf("Successfully loaded .env file from %s", envPath)
69+
utils.SafeLogPrintf("Successfully loaded .env file from %s", envPath)
7070
}
7171
} else if *specPath == "" {
72-
log.Println("Skipping .env load because --spec is missing.")
72+
utils.SafeLogPrintln("Skipping .env load because --spec is missing.")
7373
} else {
74-
log.Println("Skipping .env load because spec path appears to be a URL.")
74+
utils.SafeLogPrintln("Skipping .env load because spec path appears to be a URL.")
7575
}
7676

7777
// --- Read REQUEST_HEADERS env var ---
7878
customHeadersEnv := os.Getenv("REQUEST_HEADERS")
7979
if customHeadersEnv != "" {
80-
log.Printf("Found REQUEST_HEADERS environment variable: %s", customHeadersEnv)
80+
utils.SafeLogPrintf("Found REQUEST_HEADERS environment variable: %s", customHeadersEnv)
8181
}
8282

8383
// --- Input Validation ---
8484
if *specPath == "" {
85-
log.Println("Error: --spec flag is required.")
85+
utils.SafeLogPrintln("Error: --spec flag is required.")
8686
flag.Usage()
8787
os.Exit(1)
8888
}
@@ -99,7 +99,8 @@ func main() {
9999
case string(config.APIKeyLocationCookie):
100100
apiKeyLocation = config.APIKeyLocationCookie
101101
default:
102-
log.Fatalf("Error: invalid --api-key-loc value: %s. Must be 'header', 'query', 'path', or 'cookie'.", *apiKeyLocStr)
102+
utils.SafeLogPrintln("Error: invalid --api-key-loc value:", *apiKeyLocStr, "Must be 'header', 'query', 'path', or 'cookie'.")
103+
os.Exit(1)
103104
}
104105
}
105106

@@ -120,27 +121,28 @@ func main() {
120121
CustomHeaders: customHeadersEnv,
121122
}
122123

123-
log.Printf("Configuration loaded: %+v\n", cfg)
124-
log.Println("API Key (resolved):", cfg.GetAPIKey())
124+
utils.SafeLogPrintln("Configuration loaded.")
125+
utils.SafeLogPrintln("API Key (resolved):", cfg.GetAPIKey())
125126

126127
// --- Call Parser ---
127128
specDoc, version, err := parser.LoadSwagger(cfg.SpecPath)
128129
if err != nil {
129-
log.Fatalf("Failed to load OpenAPI/Swagger spec: %v", err)
130+
utils.SafeLogFatalf("Failed to load OpenAPI/Swagger spec: %v", err)
130131
}
131-
log.Printf("Spec type %s loaded successfully from %s.\n", version, cfg.SpecPath)
132+
utils.SafeLogPrintln("Spec type", version, "loaded successfully from", cfg.SpecPath)
132133

133134
toolSet, err := parser.GenerateToolSet(specDoc, version, cfg)
134135
if err != nil {
135-
log.Fatalf("Failed to generate MCP toolset: %v", err)
136+
utils.SafeLogFatalf("Failed to generate MCP toolset: %v", err)
136137
}
137-
log.Printf("MCP toolset generated with %d tools.\n", len(toolSet.Tools))
138+
utils.SafeLogPrintln("MCP toolset generated with", len(toolSet.Tools), "tools.")
138139

139140
// --- Start Server ---
140141
addr := fmt.Sprintf(":%d", *port)
141-
log.Printf("Starting MCP server on %s...", addr)
142+
utils.SafeLogPrintf("Starting MCP server on %s...", addr)
142143
err = server.ServeMCP(addr, toolSet, cfg) // Pass cfg to ServeMCP
143144
if err != nil {
144-
log.Fatalf("Failed to start server: %v", err)
145+
utils.SafeLogPrintln("Failed to start server:", err)
146+
os.Exit(1)
145147
}
146148
}

‎pkg/config/config.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
package config
22

33
import (
4-
"log"
54
"os"
5+
6+
"github.com/ckanthony/openapi-mcp/pkg/utils"
67
)
78

89
// APIKeyLocation specifies where the API key is located for requests.
@@ -43,28 +44,28 @@ type Config struct {
4344

4445
// GetAPIKey resolves the API key value, prioritizing the environment variable over the direct flag.
4546
func (c *Config) GetAPIKey() string {
46-
log.Println("GetAPIKey: Attempting to resolve API key...")
47+
utils.SafeLogPrintf("GetAPIKey: Attempting to resolve API key...")
4748

4849
// 1. Check environment variable specified by --api-key-env
4950
if c.APIKeyFromEnvVar != "" {
50-
log.Printf("GetAPIKey: Checking environment variable specified by --api-key-env: %s", c.APIKeyFromEnvVar)
51+
utils.SafeLogPrintf("GetAPIKey: Checking environment variable specified by --api-key-env: %s", c.APIKeyFromEnvVar)
5152
val := os.Getenv(c.APIKeyFromEnvVar)
5253
if val != "" {
53-
log.Printf("GetAPIKey: Found key in environment variable %s.", c.APIKeyFromEnvVar)
54+
utils.SafeLogPrintf("GetAPIKey: Found key in environment variable %s.", c.APIKeyFromEnvVar)
5455
return val
5556
}
56-
log.Printf("GetAPIKey: Environment variable %s not found or empty.", c.APIKeyFromEnvVar)
57+
utils.SafeLogPrintf("GetAPIKey: Environment variable %s not found or empty.", c.APIKeyFromEnvVar)
5758
} else {
58-
log.Println("GetAPIKey: No --api-key-env variable specified.")
59+
utils.SafeLogPrintf("GetAPIKey: No --api-key-env variable specified.")
5960
}
6061

6162
// 2. Check direct flag --api-key
6263
if c.APIKey != "" {
63-
log.Println("GetAPIKey: Found key provided directly via --api-key flag.")
64+
utils.SafeLogPrintf("GetAPIKey: Found key provided directly via --api-key flag.")
6465
return c.APIKey
6566
}
6667

6768
// 3. No key found
68-
log.Println("GetAPIKey: No API key found from config (env var or direct flag).")
69+
utils.SafeLogPrintf("GetAPIKey: No API key found from config (env var or direct flag).")
6970
return ""
7071
}

‎pkg/parser/parser.go‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"encoding/json"
66
"fmt"
77
"io"
8-
"log"
98
"net/http"
109
"net/url"
1110
"os"
@@ -15,6 +14,7 @@ import (
1514

1615
"github.com/ckanthony/openapi-mcp/pkg/config"
1716
"github.com/ckanthony/openapi-mcp/pkg/mcp"
17+
"github.com/ckanthony/openapi-mcp/pkg/utils"
1818
"github.com/getkin/kin-openapi/openapi3"
1919
"github.com/go-openapi/loads"
2020
"github.com/go-openapi/spec"
@@ -38,7 +38,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
3838
var absPath string // Store absolute path if it's a file
3939

4040
if !isURL {
41-
log.Printf("Detected file path location: %s", location)
41+
utils.SafeLogPrintf("Detected file path location: %s", location)
4242
absPath, err = filepath.Abs(location)
4343
if err != nil {
4444
return nil, "", fmt.Errorf("failed to get absolute path for '%s': %w", location, err)
@@ -49,7 +49,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
4949
return nil, "", fmt.Errorf("failed reading file path '%s': %w", absPath, err)
5050
}
5151
} else {
52-
log.Printf("Detected URL location: %s", location)
52+
utils.SafeLogPrintf("Detected URL location: %s", location)
5353
// Read data first for version detection
5454
resp, err := http.Get(location)
5555
if err != nil {
@@ -81,11 +81,11 @@ func LoadSwagger(location string) (interface{}, string, error) {
8181

8282
if !isURL {
8383
// Use LoadFromFile for local files
84-
log.Printf("Loading V3 spec using LoadFromFile: %s", absPath)
84+
utils.SafeLogPrintf("Loading V3 spec using LoadFromFile: %s", absPath)
8585
doc, loadErr = loader.LoadFromFile(absPath)
8686
} else {
8787
// Use LoadFromURI for URLs
88-
log.Printf("Loading V3 spec using LoadFromURI: %s", location)
88+
utils.SafeLogPrintf("Loading V3 spec using LoadFromURI: %s", location)
8989
doc, loadErr = loader.LoadFromURI(locationURL)
9090
}
9191

@@ -99,7 +99,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
9999
return doc, VersionV3, nil
100100
} else if _, ok := detector["swagger"]; ok {
101101
// Swagger 2.0 - Still load from data as loads.Analyzed expects bytes
102-
log.Printf("Loading V2 spec using loads.Analyzed from data (source: %s)", location)
102+
utils.SafeLogPrintf("Loading V2 spec using loads.Analyzed from data (source: %s)", location)
103103
doc, err := loads.Analyzed(data, "2.0")
104104
if err != nil {
105105
return nil, "", fmt.Errorf("failed to load or validate Swagger v2 spec from '%s': %w", location, err)
@@ -139,7 +139,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
139139
// Determine Base URL once
140140
baseURL, err := determineBaseURLV3(doc, cfg)
141141
if err != nil {
142-
log.Printf("Warning: Could not determine base URL for V3 spec: %v. Operations might fail if base URL override is not set.", err)
142+
utils.SafeLogPrintf("Warning: Could not determine base URL for V3 spec: %v. Operations might fail if base URL override is not set.", err)
143143
baseURL = "" // Allow proceeding if override is set
144144
}
145145

@@ -175,7 +175,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
175175
// Handle request body
176176
requestBody, err := requestBodyToMCPV3(op.RequestBody)
177177
if err != nil {
178-
log.Printf("Warning: skipping request body for %s %s due to error: %v", method, rawPath, err)
178+
utils.SafeLogPrintf("Warning: skipping request body for %s %s due to error: %v", method, rawPath, err)
179179
} else {
180180
// Merge request body schema into the main parameter schema
181181
if requestBody.Content != nil {
@@ -189,7 +189,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
189189
}
190190
} else {
191191
// If body is not an object, represent as 'requestBody'
192-
log.Printf("Warning: V3 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
192+
utils.SafeLogPrintf("Warning: V3 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
193193
parametersSchema.Properties["requestBody"] = mediaTypeSchema
194194
}
195195
break // Only process the first content type
@@ -219,7 +219,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
219219
// Optionally, add a note if the requestBody itself was marked as required
220220
if requestBody.Required { // Check the boolean field
221221
// How to indicate this? Maybe add to description?
222-
log.Printf("Note: Request body for %s %s is marked as required.", method, rawPath)
222+
utils.SafeLogPrintf("Note: Request body for %s %s is marked as required.", method, rawPath)
223223
// Or add all top-level body props to required? Needs decision.
224224
}
225225
}
@@ -309,18 +309,18 @@ func parametersToMCPSchemaAndDetailsV3(params openapi3.Parameters, cfg *config.C
309309
opParams := []mcp.ParameterDetail{}
310310
for _, paramRef := range params {
311311
if paramRef.Value == nil {
312-
log.Printf("Warning: Skipping parameter with nil value.")
312+
utils.SafeLogPrintf("Warning: Skipping parameter with nil value.")
313313
continue
314314
}
315315
param := paramRef.Value
316316
if param.Schema == nil {
317-
log.Printf("Warning: Skipping parameter '%s' with nil schema.", param.Name)
317+
utils.SafeLogPrintf("Warning: Skipping parameter '%s' with nil schema.", param.Name)
318318
continue
319319
}
320320

321321
// Skip the API key parameter if configured
322322
if cfg.APIKeyName != "" && param.Name == cfg.APIKeyName && param.In == string(cfg.APIKeyLocation) {
323-
log.Printf("Parser V3: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
323+
utils.SafeLogPrintf("Parser V3: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
324324
continue
325325
}
326326

@@ -442,7 +442,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
442442
// Determine Base URL once
443443
baseURL, err := determineBaseURLV2(doc, cfg)
444444
if err != nil {
445-
log.Printf("Warning: Could not determine base URL for V2 spec: %v. Operations might fail if base URL override is not set.", err)
445+
utils.SafeLogPrintf("Warning: Could not determine base URL for V2 spec: %v. Operations might fail if base URL override is not set.", err)
446446
baseURL = "" // Allow proceeding if override is set
447447
}
448448

@@ -455,7 +455,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
455455
if secDef.Type == "apiKey" {
456456
apiKeyName = secDef.Name
457457
apiKeyIn = secDef.In // "query" or "header"
458-
log.Printf("Parser V2: Detected API key from security definition '%s': Name='%s', In='%s'", name, apiKeyName, apiKeyIn)
458+
utils.SafeLogPrintf("Parser V2: Detected API key from security definition '%s': Name='%s', In='%s'", name, apiKeyName, apiKeyIn)
459459
break // Assume only one apiKey definition for simplicity
460460
}
461461
}
@@ -519,7 +519,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
519519
}
520520
} else {
521521
// If body is not an object, represent as 'requestBody'
522-
log.Printf("Warning: V2 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
522+
utils.SafeLogPrintf("Warning: V2 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
523523
if parametersSchema.Properties == nil {
524524
parametersSchema.Properties = make(map[string]mcp.Schema)
525525
}
@@ -629,7 +629,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec
629629
for _, param := range params {
630630
// Skip the API key parameter if it's configured/detected
631631
if apiKeyName != "" && param.Name == apiKeyName && (param.In == "query" || param.In == "header") {
632-
log.Printf("Parser V2: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
632+
utils.SafeLogPrintf("Parser V2: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
633633
continue
634634
}
635635

@@ -643,7 +643,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec
643643
}
644644

645645
if param.In != "query" && param.In != "path" && param.In != "header" && param.In != "formData" {
646-
log.Printf("Parser V2: Skipping unsupported parameter type '%s' for parameter '%s'", param.In, param.Name)
646+
utils.SafeLogPrintf("Parser V2: Skipping unsupported parameter type '%s' for parameter '%s'", param.In, param.Name)
647647
continue
648648
}
649649

@@ -702,7 +702,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec
702702

703703
} else {
704704
// Body param defined without a schema? Treat as simple string.
705-
log.Printf("Warning: V2 body parameter '%s' defined without a schema. Treating as string.", bodyParam.Name)
705+
utils.SafeLogPrintf("Warning: V2 body parameter '%s' defined without a schema. Treating as string.", bodyParam.Name)
706706
bodySchema.Type = "string"
707707
mcpSchema.Properties[bodyParam.Name] = bodySchema
708708
if bodyParam.Required {

‎pkg/server/manager.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ package server
22

33
import (
44
"fmt"
5-
"log"
65
"net/http"
76
"sync"
7+
8+
"github.com/ckanthony/openapi-mcp/pkg/utils"
89
)
910

1011
// client holds information about a connected SSE client.
@@ -36,7 +37,7 @@ func (m *connectionManager) addClient(r *http.Request, w http.ResponseWriter, f
3637
m.clients[r] = newClient
3738
m.mu.Unlock()
3839

39-
log.Printf("Client connected: %s (Total: %d)", r.RemoteAddr, m.getClientCount())
40+
utils.SafeLogPrintf("Client connected: %s (Total: %d)", r.RemoteAddr, m.getClientCount())
4041

4142
// Send initial toolset immediately
4243
go m.sendToolset(newClient) // Send in a goroutine to avoid blocking registration?
@@ -48,9 +49,9 @@ func (m *connectionManager) removeClient(r *http.Request) {
4849
_, ok := m.clients[r]
4950
if ok {
5051
delete(m.clients, r)
51-
log.Printf("Client disconnected: %s (Total: %d)", r.RemoteAddr, len(m.clients))
52+
utils.SafeLogPrintf("Client disconnected: %s (Total: %d)", r.RemoteAddr, len(m.clients))
5253
} else {
53-
log.Printf("Attempted to remove already disconnected client: %s", r.RemoteAddr)
54+
utils.SafeLogPrintf("Attempted to remove already disconnected client: %s", r.RemoteAddr)
5455
}
5556
m.mu.Unlock()
5657
}
@@ -68,15 +69,15 @@ func (m *connectionManager) sendToolset(c *client) {
6869
if c == nil {
6970
return
7071
}
71-
log.Printf("Attempting to send toolset to client...")
72+
utils.SafeLogPrintf("Attempting to send toolset to client...")
7273
_, err := fmt.Fprintf(c.writer, "event: tool_set\ndata: %s\n\n", string(m.toolSet))
7374
if err != nil {
7475
// This error often happens if the client disconnected before/during the write
75-
log.Printf("Error sending toolset data to client: %v (client likely disconnected)", err)
76+
utils.SafeLogPrintf("Error sending toolset data to client: %v (client likely disconnected)", err)
7677
// Optionally trigger removal here if possible, though context done in handler is primary mechanism
7778
return
7879
}
7980
// Flush the data
8081
c.flusher.Flush()
81-
log.Println("Sent tool_set event and flushed.")
82+
utils.SafeLogPrintf("Sent tool_set event and flushed.")
8283
}

0 commit comments

Comments
 (0)